diff --git a/cmd/mesh-host/apply_context_test.go b/cmd/mesh-host/apply_context_test.go new file mode 100644 index 0000000..2bb99be --- /dev/null +++ b/cmd/mesh-host/apply_context_test.go @@ -0,0 +1,39 @@ +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 +// because of the apply itself (it replaces the bus's own container), which then must finish (issue 1945, live on +// 2026-10-25 at 11:23: novox's node-engine removed the bus container, its link's context ended, and the docker run that +// would have made the new one was never made). +func TestAnApplyOutlivesTheLinksContext(t *testing.T) { + type key struct{} + link, cancel := context.WithCancel(context.WithValue(context.Background(), key{}, "v")) + 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()) + } + if ctx.Value(key{}) != "v" { + 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 7dd18f9..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(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(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 @@ -567,3 +571,15 @@ func declaresNothingToBackUp(said json.RawMessage, module string) (nothing bool, } return true, true } + +// 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 731d432..64d2f06 100644 --- a/internal/apply/apply.go +++ b/internal/apply/apply.go @@ -2464,12 +2464,18 @@ func ExecRunner(ctx context.Context, name string, args ...string) (string, error if err != nil { var exit *exec.ExitError if errors.As(err, &exit) { - // The words as they always were, and the exit underneath them, so a caller asks the - // code (system.ExitCode) rather than matching text that this line is free to reword. - return string(out), &exited{ - words: fmt.Sprintf("%s exited %d: %s", name, exit.ExitCode(), strings.TrimSpace(string(exit.Stderr))), - exit: exit, + words := fmt.Sprintf("%s exited %d: %s", name, exit.ExitCode(), strings.TrimSpace(string(exit.Stderr))) + if exit.ExitCode() == -1 { + // Killed, not exited: say by what, and the context's end when that is what killed it (issue 1945: + // "docker exited -1:" said nothing while a bus container was half replaced). + words = fmt.Sprintf("%s was %s", name, exit.String()) + if ctx.Err() != nil { + words += " because its context ended: " + ctx.Err().Error() + } } + // The words, and the exit underneath them, so a caller asks the code (system.ExitCode) rather than + // matching text that this line is free to reword. + return string(out), &exited{words: words, exit: exit} } return string(out), fmt.Errorf("%s: %w", name, err) } @@ -2958,6 +2964,14 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, return out, nil } + 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" @@ -2980,15 +2994,40 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, if err := ensureImage(ctx, cri, r.Image, run); err != nil { return out, fmt.Errorf("not replacing container %s, which keeps running: %w", r.Name, err) } - if _, err := run(ctx, cri, "rm", "-f", r.Name); err != nil { - return out, fmt.Errorf("replacing container %s: %w", r.Name, err) + // **The running container is set aside, never removed, until its replacement runs** (issue 1945): stopped and + // renamed, so its name and ports are free, and started again under its own name when the replacement does not + // come up. Removed first, the bus's own container left a node without a bus when its replacement was never + // made (2026-10-25 at 11:23). A container set aside by an earlier apply that stopped midway goes first. + aside = r.Name + asideSuffix + _, _ = run(ctx, cri, "rm", "-f", aside) + if _, err := run(ctx, cri, "rename", r.Name, aside); err != nil { + return out, fmt.Errorf("replacing container %s: setting the running one aside: %w", r.Name, err) } + if _, err := run(ctx, cri, "stop", aside); err != nil { + _, _ = run(ctx, cri, "rename", aside, r.Name) + return out, fmt.Errorf("replacing container %s: stopping the running one: %w", r.Name, err) + } + } + + // putBack starts the container set aside again under its own name, after a replacement that did not come up. + putBack := func(cause error) error { + if aside == "" { + return cause + } + _, _ = run(ctx, cri, "rm", "-f", r.Name) + if _, err := run(ctx, cri, "rename", aside, r.Name); err != nil { + return fmt.Errorf("%w; and the container it replaced could not be put back: %v", cause, err) + } + if _, err := run(ctx, cri, "start", r.Name); err != nil { + return fmt.Errorf("%w; and the container it replaced could not be started again: %v", cause, err) + } + return fmt.Errorf("%w; the container it replaced was started again", cause) } args := detachedRunArgs(r, want) if _, err := run(ctx, cri, args...); err != nil { - return out, fmt.Errorf("starting container %s: %w", r.Name, err) + return out, putBack(fmt.Errorf("starting container %s: %w", r.Name, err)) } // Read back. `docker run --detach` returning an id says the container was created, not @@ -2996,15 +3035,21 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, // command exactly as one that came up does. after, err := containerState(ctx, r.Name, run) if err != nil { - return out, fmt.Errorf("started container %s and it is not there: %w", r.Name, err) + return out, putBack(fmt.Errorf("started container %s and it is not there: %w", r.Name, err)) } if !after.Running { - return out, fmt.Errorf( + return out, putBack(fmt.Errorf( "container %s was started and is not running. It exited; ask the runtime for its "+ - "logs", r.Name) + "logs", r.Name)) } if after.Spec != want { - return out, fmt.Errorf("container %s is not the one that was declared after creating it", r.Name) + return out, putBack(fmt.Errorf("container %s is not the one that was declared after creating it", r.Name)) + } + // Only now, with the replacement running, is the container set aside removed. + if aside != "" { + if _, err := run(ctx, cri, "rm", "-f", aside); err != nil { + out.Detail = fmt.Sprintf("the container it replaced, set aside as %s, could not be removed: %v", aside, err) + } } // The found networks a per-machine setting keeps for it (novox/hq ADR 0163, rule 4), joined // once it runs: a runtime starts a container on one network, and the others are connected. @@ -3040,6 +3085,9 @@ func applyContainer(ctx context.Context, r *declaration.Container, run Runner, return out, nil } +// asideSuffix names a running container set aside while its replacement is started (issue 1945). +const asideSuffix = ".replaced" + // detachedRunArgs is the `docker run` that starts a service container and leaves it running. func detachedRunArgs(r *declaration.Container, want string) []string { args := []string{"run", "--detach", "--name", r.Name, "--restart", "unless-stopped"} diff --git a/internal/apply/busy_test.go b/internal/apply/busy_test.go index 481a6c0..b9fa149 100644 --- a/internal/apply/busy_test.go +++ b/internal/apply/busy_test.go @@ -33,6 +33,7 @@ func newBusyMachine() *aBusyMachine { } func (m *aBusyMachine) run(_ context.Context, _ string, args ...string) (string, error) { + args = asideShape(args) m.mu.Lock() defer m.mu.Unlock() switch args[0] { 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/exec_killed_test.go b/internal/apply/exec_killed_test.go new file mode 100644 index 0000000..24685f6 --- /dev/null +++ b/internal/apply/exec_killed_test.go @@ -0,0 +1,19 @@ +package apply + +import ( + "context" + "strings" + "testing" + "time" +) + +// A command killed because its context ended says so, and why: "docker exited -1:" said nothing while novox's bus +// container was half replaced (issue 1945, live on 2026-10-25 at 11:23). +func TestACommandKilledByItsContextSaysWhy(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + go func() { time.Sleep(50 * time.Millisecond); cancel() }() + _, err := ExecRunner(ctx, "sleep", "5") + if err == nil || !strings.Contains(err.Error(), "killed") || !strings.Contains(err.Error(), "context canceled") { + t.Fatalf("a command its context killed said: %v", err) + } +} diff --git a/internal/apply/hold_test.go b/internal/apply/hold_test.go index e8ba6bd..73cc417 100644 --- a/internal/apply/hold_test.go +++ b/internal/apply/hold_test.go @@ -111,7 +111,10 @@ type fakeContainer struct { // options. The host passes `--` before every unit (third review of mesh-catalog #147), which // internal/system's own test holds; the fakes read the unit where they always did. func oldShape(name string, args []string) []string { - if name != "systemctl" || len(args) < 2 || args[len(args)-2] != "--" { + if name != "systemctl" { + return asideShape(args) + } + if len(args) < 2 || args[len(args)-2] != "--" { return args } v := 0 @@ -1128,3 +1131,16 @@ func TestAPathTheMeshOnlySetAccessOnIsStillFound(t *testing.T) { t.Errorf("it was re-moded to %o", info.Mode().Perm()) } } + +// asideShape reads the steps of a container set aside while its replacement starts (issue 1945) as the removal the +// tests were written against: setting it aside is its removal, and what is done to the set-aside container afterwards +// is no step of theirs. +func asideShape(args []string) []string { + if len(args) == 3 && args[0] == "rename" && args[2] == args[1]+asideSuffix { + return []string{"rm", "-f", args[1]} + } + if len(args) > 0 && strings.HasSuffix(args[len(args)-1], asideSuffix) && (args[0] == "rm" || args[0] == "stop") { + return []string{"aside"} + } + return args +} diff --git a/internal/apply/maintenance_window_test.go b/internal/apply/maintenance_window_test.go index acacdcc..d25646b 100644 --- a/internal/apply/maintenance_window_test.go +++ b/internal/apply/maintenance_window_test.go @@ -270,6 +270,7 @@ func newRegistryMachine(spec string) *aRegistryMachine { } func (m *aRegistryMachine) run(_ context.Context, _ string, args ...string) (string, error) { + args = asideShape(args) m.mu.Lock() switch args[0] { case "info", "image": diff --git a/internal/apply/replace_image_test.go b/internal/apply/replace_image_test.go index f653274..447366b 100644 --- a/internal/apply/replace_image_test.go +++ b/internal/apply/replace_image_test.go @@ -91,3 +91,129 @@ func TestAReplacementImageIsPresentBeforeTheRunningContainerIsRemoved(t *testing t.Errorf("order of runtime calls = %s, want the image pulled before rm", got) } } + +// asideRun is a runtime whose container "store" runs from an older spec, recording the steps a replacement takes; run +// fails when failRun, and the new container reads back as declared otherwise. +func asideRun(t *testing.T, want string, failRun bool, order *[]string) Runner { + created := false + return func(ctx context.Context, name string, args ...string) (string, error) { + switch args[0] { + case "info": + return "27.0\n", nil + case "container": + if created { + return "true\t" + want, nil + } + return "true\tsome-older-spec", nil + 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 + } +} + +// A container whose replacement does not come up is put back: the running one is set aside (renamed and stopped), never +// removed before the new one runs, and started again under its name when the new one fails, the apply failing loudly. +// Removed first, the bus's own container left novox without a bus when its replacement was never made (issue 1945, +// live on 2026-10-25 at 11:23). +func TestAContainerWhoseReplacementFailsIsPutBack(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{}) + var order []string + _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, asideRun(t, want, true, &order), nil, nil) + if err == nil || !strings.Contains(err.Error(), "started again") { + t.Fatalf("a failed replacement: %v", err) + } + got := strings.Join(order, "; ") + if !strings.Contains(got, "rename store store.replaced; stop store.replaced; run") || + !strings.Contains(got, "rename store.replaced store; start store") { + t.Fatalf("the steps were %q", got) + } + if strings.Contains(got, "rm -f store;") && strings.Index(got, "rm -f store;") < strings.Index(got, "run") { + t.Fatalf("the running container was removed before its replacement ran: %q", got) + } +} + +// A replacement that runs removes the container set aside, and only after it runs. +func TestAReplacementThatRunsRemovesTheContainerSetAside(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{}) + var order []string + if _, _, err := Apply(context.Background(), archHost(t), d, store.State{}, store.OriginCarried, asideRun(t, want, false, &order), nil, nil); err != nil { + t.Fatal(err) + } + got := strings.Join(order, "; ") + run, removed := strings.Index(got, "run"), strings.LastIndex(got, "rm -f store.replaced") + if run < 0 || removed < run || strings.Contains(got, "start store") { + 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) + } + } +}