diff --git a/cmd/mesh-host/apply_context_test.go b/cmd/mesh-host/apply_context_test.go index 6b7c820..2bb99be 100644 --- a/cmd/mesh-host/apply_context_test.go +++ b/cmd/mesh-host/apply_context_test.go @@ -3,6 +3,7 @@ package main import ( "context" "testing" + "time" ) // An apply runs in a context the bus's loss cannot end: the link's context ends when the bus goes, and the bus may go @@ -12,7 +13,8 @@ import ( func TestAnApplyOutlivesTheLinksContext(t *testing.T) { type key struct{} link, cancel := context.WithCancel(context.WithValue(context.Background(), key{}, "v")) - ctx := applyContext(link) + ctx, done := applyContext(link) + defer done() cancel() if ctx.Err() != nil { t.Fatalf("the apply's context ended with the link's: %v", ctx.Err()) @@ -21,3 +23,17 @@ func TestAnApplyOutlivesTheLinksContext(t *testing.T) { t.Fatal("the apply's context lost the link's values") } } + +// An apply is bounded all the same: a step that hangs (a docker call that never returns) cannot hold the node agent for +// ever (the review of issue 1945). +func TestAnApplyIsBounded(t *testing.T) { + ctx, done := applyContext(context.Background()) + defer done() + at, ok := ctx.Deadline() + if !ok { + t.Fatal("an apply has no bound") + } + if left := time.Until(at); left > applyBound || left < applyBound-time.Minute { + t.Fatalf("the bound is %s away, want about %s", left, applyBound) + } +} diff --git a/cmd/mesh-host/nodeagent.go b/cmd/mesh-host/nodeagent.go index 80f0bc3..ca55fd3 100644 --- a/cmd/mesh-host/nodeagent.go +++ b/cmd/mesh-host/nodeagent.go @@ -298,13 +298,17 @@ func nodeAgent(ctx context.Context, opts options, signer ed25519.PublicKey, sess Backup: backupNode(session), Say: say, Apply: func(ctx context.Context, c delivery.Composed, others []delivery.Composed) agent.Result { - res := applyComposition(applyContext(session.WithLink(ctx)), opts, c, others, sched, say) + actx, done := applyContext(session.WithLink(ctx)) + res := applyComposition(actx, opts, c, others, sched, say) + done() afterApply() sayFacts(ctx, ag) return res }, Remove: func(ctx context.Context, module string, others []delivery.Composed) agent.Result { - res := removeComposition(applyContext(session.WithLink(ctx)), opts, module, others, sched, say) + actx, done := applyContext(session.WithLink(ctx)) + res := removeComposition(actx, opts, module, others, sched, say) + done() afterApply() sayFacts(ctx, ag) return res @@ -568,8 +572,14 @@ func declaresNothingToBackUp(said json.RawMessage, module string) (nothing bool, return true, true } -// applyContext is the context an apply or a removal runs in: the link's values, never its end. The link's context ends -// when the bus goes, and the bus may go because of the apply itself (it replaces the bus's own container), which must -// then finish: a container removed and never made again took the mesh down (issue 1945, live on 2026-10-25 at 11:23). -// An apply is bounded by its own steps' bounds, not by the link. -func applyContext(ctx context.Context) context.Context { return context.WithoutCancel(ctx) } +// applyBound is how long one apply or removal may take: long enough for the slowest image pull and backup step, short +// enough that a step that hangs cannot hold the node agent for ever. +const applyBound = 30 * time.Minute + +// applyContext is the context an apply or a removal runs in: the link's values, never its end, and a bound of its own. +// The link's context ends when the bus goes, and the bus may go because of the apply itself (it replaces the bus's own +// container), which must then finish: a container removed and never made again took the mesh down (issue 1945, live on +// 2026-10-25 at 11:23). applyBound keeps a hung step from blocking the agent for ever. +func applyContext(ctx context.Context) (context.Context, context.CancelFunc) { + return context.WithTimeout(context.WithoutCancel(ctx), applyBound) +} diff --git a/internal/apply/apply.go b/internal/apply/apply.go index 9f1965d..64d2f06 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -2965,6 +2965,13 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, } aside := "" + if !existed { + // A container an engine set aside and then stopped before it ran the replacement (it died between the two): + // the set-aside one of this apply, put back if the replacement fails and removed once it runs (issue 1945). + if _, err := containerState(ctx, r.Name+asideSuffix, run); err == nil { + aside = r.Name + asideSuffix + } + } switch { case existed && (before.Spec == want || legacy) && before.Running && len(reasons) == 0: out.Action = "unchanged" diff --git a/internal/apply/containers_kept_test.go b/internal/apply/containers_kept_test.go index 3bc8684..8549dac 100644 --- a/internal/apply/containers_kept_test.go +++ b/internal/apply/containers_kept_test.go @@ -20,6 +20,9 @@ func TestAContainerJoinsTheNetworksItKeeps(t *testing.T) { if name != "docker" { return "", errors.New("not installed") } + if len(args) > 0 && args[0] == "container" && strings.HasSuffix(args[len(args)-1], asideSuffix) { + return "", errors.New("no such container") // nothing was left set aside + } ran = append(ran, strings.Join(args, " ")) switch args[0] { case "info": diff --git a/internal/apply/replace_image_test.go b/internal/apply/replace_image_test.go index f81c4f0..447366b 100644 --- a/internal/apply/replace_image_test.go +++ b/internal/apply/replace_image_test.go @@ -162,3 +162,58 @@ func TestAReplacementThatRunsRemovesTheContainerSetAside(t *testing.T) { t.Fatalf("the steps were %q: the set-aside container is removed after the replacement runs", got) } } + +// A container left set aside by an engine that stopped between setting it aside and running its replacement is the +// set-aside one of the next apply: put back when the replacement fails, removed once it runs (the review of issue +// 1945). +func TestAContainerLeftAsideIsPutBackOrRemovedByTheNextApply(t *testing.T) { + d := parseTrusted(t, `{"declaration":1,"resources":[ + {"id":"store","type":"container","name":"store","image":"`+pinned+`","env":{"PGDATA":"/data"}} + ]}`) + want := containerSpec(d.Resources[0].(*declaration.Container), inputs{}) + for _, failRun := range []bool{true, false} { + var order []string + created := false + run := func(ctx context.Context, name string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "container": + switch args[len(args)-1] { + case "store": + if created { + return "true\t" + want, nil + } + return "", errors.New("no such container") + case "store.replaced": + return "false\tsome-older-spec", nil + } + return "", errors.New("no such container") + case "image": + return "", nil + case "rm", "rename", "stop", "start": + order = append(order, strings.Join(args, " ")) + return "", nil + case "run": + order = append(order, "run") + if failRun { + return "", errors.New("port is already allocated") + } + created = true + return "deadbeef\n", nil + } + return "", nil + } + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, run, nil, nil) + got := strings.Join(order, "; ") + if failRun { + if err == nil || !strings.Contains(got, "rename store.replaced store; start store") { + t.Fatalf("a failed run beside a container left aside: %v; steps %q", err, got) + } + continue + } + if err != nil || !strings.Contains(got, "run; rm -f store.replaced") { + t.Fatalf("a run beside a container left aside: %v; steps %q", err, got) + } + } +}