amadan
host: the adopt pass restarts the unit a route names
Third arm on makeRestartRoute's dispatch, ahead of the other two: a route whose Unit column is set is try-restarted by that name and returns. Such a route is addr-backed, so the activator owns no child to Bounce and there is no carlos-app@<host> unit to try-restart — before this both existing arms ran and cycled nothing, while adoption went on to stamp Version and therefore X-Carlos-Version over a process still serving the old build (issue #110). The root pass gets the arm too, and that is not belt-and-braces: the agent runs inside carlos-edge.service's ProtectSystem=strict sandbox and cannot write /opt/carlos, so on a real box its stage is blocked and relayed to carlos-adopt.service, which does the staging, the repoint, the restart and the stamp. An arm wired only into the agent's loops would be one the consoles never reach. restartRoute becomes rootRestartRoute(reg) for that reason; with a nil bounce every route it used to try-restart by host still is. try-restart, not restart, on the named unit as everywhere else: a service an operator stopped stays stopped, and nothing on any arm can wake a hibernator.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 files changed,
+235
−26
internal/caps/converge.go+1 −1internal/host/agent.go+76 −22internal/host/agent_test.go+157 −2internal/release/adopt.go+1 −1
diff --git a/internal/caps/converge.go b/internal/caps/converge.go| index b931cbb0e..a7998b317 100644 |
| --- a/internal/caps/converge.go |
| +++ b/internal/caps/converge.go |
| @@ -948,7 +948,7 @@ func Restore(o Ops, prev Snapshot) error { |
| // Restart is the daemon route's restart: `systemctl restart`, and its error |
| // is REAL — it is what makes adoptHost's rollback fire. Non-daemon routes are |
| -// not this function's business: restartRoute's try-restart keeps owning them, |
| +// not this function's business: rootRestartRoute's try-restart keeps owning them, |
| // byte-identically. |
| func Restart(o Ops, p Plan) error { |
| if err := validHost(p.Host); err != nil { |
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 84e7e0f50..1a281e02a 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -266,7 +266,7 @@ func adoptLoop(reg *registry.Registry, svc *activator.Service) { |
| return |
| } |
| optDir := release.OptDir() |
| - restart := makeRestartRoute(reg, systemctlTryRestart, bounceFn(svc)) |
| + restart := makeRestartRoute(reg, systemctlTryRestart, systemctlTryRestartUnit, bounceFn(svc)) |
| log.Printf("agent: adopt loop armed (hourly + jitter)") |
| // First pass shortly after boot (Eleven's OnBootSec=5min, compressed — |
| // the edge is already up by the time this fires), then hourly + jitter. |
| @@ -946,7 +946,7 @@ func fastConvergeLoop(reg *registry.Registry, svc *activator.Service) { |
| } |
| dd := dataDir() |
| optDir := release.OptDir() |
| - restart := makeRestartRoute(reg, systemctlTryRestart, bounceFn(svc)) |
| + restart := makeRestartRoute(reg, systemctlTryRestart, systemctlTryRestartUnit, bounceFn(svc)) |
| // Built once, used by both pass shapes below — see fastPassOpts for the |
| // DO-NOT list and why the stage requester is per-loop, not per-tick. |
| opts := fastPassOpts(dd, stageRequester(dd), restart) |
| @@ -1496,15 +1496,25 @@ func statusTick(ctx context.Context, s release.Store, reg *registry.Registry, p |
| } |
| // makeRestartRoute builds the adopt pass's restart callback, dispatched on |
| -// the route's backing: a unit tenant is try-restarted (a no-op when the unit |
| -// isn't running — hibernators are never woken by this), while an exec child |
| -// is Bounced through the activator, the same graceful cycle config sync |
| -// uses. systemctl cannot reach a child of THIS process, which is why |
| -// exec-backed warm instances used to keep serving the old binary until they |
| -// happened to hibernate: the symlink was repointed under them and nothing |
| -// cycled the process. |
| +// what owns the route's process. THREE arms, in this order: |
| // |
| -// Hosts the registry cannot answer for take the unit path: try-restart of a |
| +// 1. a route that NAMES its own unit (registry.Route.Unit — an operator-run |
| +// bespoke service, console.service) is try-restarted by that name, and |
| +// returns. This arm comes first because it is the only one that can be |
| +// right when it applies: such a route is addr-backed, so the activator |
| +// owns no child to Bounce and there is no carlos-app@<host> unit to |
| +// try-restart — both of the arms below would run and cycle nothing, |
| +// which is issue #110 (adoption stamped Version, and therefore |
| +// X-Carlos-Version, behind a console still serving the old build). |
| +// 2. a carlos-app@<host> unit tenant is try-restarted by host (a no-op when |
| +// the unit isn't running — hibernators are never woken by this). |
| +// 3. an exec child is Bounced through the activator, the same graceful |
| +// cycle config sync uses. systemctl cannot reach a child of THIS |
| +// process, which is why exec-backed warm instances used to keep serving |
| +// the old binary until they happened to hibernate: the symlink was |
| +// repointed under them and nothing cycled the process. |
| +// |
| +// Hosts the registry cannot answer for take arm 2: try-restart of a |
| // nonexistent unit is a no-op, whereas a bounce of a process we might not own |
| // is not. |
| // |
| @@ -1518,19 +1528,31 @@ func statusTick(ctx context.Context, s release.Store, reg *registry.Registry, p |
| // only fires when the version actually moves (adoptHost early-returns on |
| // cur == ver), so it is one line per version change per host, never per tick. |
| // |
| -// Neither path can WAKE anything, which is what lets release.Pass hand this |
| +// The named-unit arm takes NO part in that fallthrough, in either direction. |
| +// It does not fall through (a route that names its unit has said where its |
| +// process is; trying the other two afterwards could only cycle something |
| +// else), and nothing falls through INTO it (the other arms are reached only |
| +// when Unit is empty, i.e. when there is no name to try). |
| +// |
| +// No path can WAKE anything, which is what lets release.Pass hand this |
| // callback hibernating routes (RestartHibernating): Bounce returns false |
| // without starting anything when the host is not awake, and try-restart is |
| -// defined as a no-op on a unit that is not running. |
| +// defined as a no-op on a unit that is not running — including the named one, |
| +// which is why it is try-restart and not restart (a console an operator has |
| +// deliberately stopped stays stopped). |
| // |
| // Errors are logged, never returned: a failed restart must not poison the |
| // adopted version for the other hosts in the pass (release/adopt.go's rule). |
| -func makeRestartRoute(reg *registry.Registry, tryRestart func(host string), bounce func(host string) bool) func(host string) error { |
| +func makeRestartRoute(reg *registry.Registry, tryRestart func(host string), tryRestartUnit func(unit string), bounce func(host string) bool) func(host string) error { |
| return func(host string) error { |
| rt, err := reg.Get(host) |
| if err != nil { |
| log.Printf("adopt: registry lookup %s: %v (restarting as a unit)", host, err) |
| } |
| + if err == nil && rt != nil && rt.Unit != "" { |
| + tryRestartUnit(rt.Unit) |
| + return nil |
| + } |
| if err == nil && rt != nil && rt.Backing != registry.BackingUnit { |
| if bounce != nil && bounce(host) { |
| log.Printf("adopt: bounced exec instance %s onto its new version", host) |
| @@ -1558,13 +1580,40 @@ func systemctlTryRestart(host string) { |
| } |
| } |
| -// restartRoute is the root adopt pass's restart callback (RunAdopt, out of |
| -// carlos-adopt.service): pure systemctl, no bounce. That process has no |
| -// activator and no children of its own — the exec instances belong to the |
| -// agent, and another process's children are not its to stop. |
| -func restartRoute(host string) error { |
| - systemctlTryRestart(host) |
| - return nil |
| +// systemctlTryRestartUnit is makeRestartRoute's NAMED-unit path: cycle the |
| +// unit the route itself names, iff it's running. Same best-effort, same |
| +// try-restart-not-restart reasoning as systemctlTryRestart above — a service |
| +// an operator has stopped stays stopped — and the same swallow-and-log, since |
| +// one route's failed restart must not poison the pass for the others. |
| +// |
| +// The unit reaches exec.Command as ONE argv element, so there is no shell for |
| +// a name to escape into; the shape rail lives at the write (registry's |
| +// ValidUnitName), where a typo can still be reported to the person who typed |
| +// it. |
| +func systemctlTryRestartUnit(unit string) { |
| + out, err := exec.Command("systemctl", "try-restart", unit).CombinedOutput() |
| + if err != nil { |
| + log.Printf("adopt: try-restart %s: %v: %s (continuing)", unit, err, strings.TrimSpace(string(out))) |
| + } |
| +} |
| + |
| +// rootRestartRoute is the root adopt pass's restart callback (RunAdopt, out of |
| +// carlos-adopt.service): no bounce. That process has no activator and no |
| +// children of its own — the exec instances belong to the agent, and another |
| +// process's children are not its to stop. With bounce nil the dispatch's arm |
| +// 3 is unreachable and every route it used to try-restart by host still is, |
| +// byte-identically; what it gains is arm 1. |
| +// |
| +// Arm 1 has to be HERE, not only on the agent's in-process loops, and the |
| +// reason is the sandbox. carlos-edge.service runs as User=carlos with |
| +// ProtectSystem=strict and ReadWritePaths=/data /run/carlos, so the agent |
| +// cannot write /opt/carlos: on a real box its pass reports the stage as |
| +// blocked and relays to this verb (carlos-stage.path), which does the |
| +// staging, the repoint, the restart and the version stamp. A restart arm |
| +// wired only into the agent would therefore be a restart arm the consoles |
| +// never reach. |
| +func rootRestartRoute(reg *registry.Registry) func(host string) error { |
| + return makeRestartRoute(reg, systemctlTryRestart, systemctlTryRestartUnit, nil) |
| } |
| // bounceFn adapts an activator to makeRestartRoute's bounce parameter, |
| @@ -1628,6 +1677,11 @@ func RunAdopt(args []string) { |
| if err != nil { |
| log.Fatalf("adopt: %v", err) |
| } |
| + // One callback for both halves below (config sync's restarts and the |
| + // release pass's), built once here now that it needs the registry — it |
| + // reads each route's `unit` column to decide what to cycle. See |
| + // rootRestartRoute for why the named-unit arm has to exist on THIS pass. |
| + restart := rootRestartRoute(reg) |
| // Parsed here, before config sync, exactly as the full pass always has |
| // (a box with an unusable release key must die before it converges |
| // anything), but SKIPPED entirely for -config-only: that mode never |
| @@ -1649,7 +1703,7 @@ func RunAdopt(args []string) { |
| secretsPriv, err := appconfig.ParsePrivateKey(raw, "CARLOS_SECRETS_KEY") |
| if err != nil { |
| log.Printf("adopt: config sync disabled: %v", err) |
| - } else if err := appconfig.Sync(ctx, reg, store, secretsPriv, appconfig.DefaultEnvDir, restartRoute); err != nil { |
| + } else if err := appconfig.Sync(ctx, reg, store, secretsPriv, appconfig.DefaultEnvDir, restart); err != nil { |
| log.Printf("adopt: config sync: %v", err) |
| } |
| } |
| @@ -1709,7 +1763,7 @@ func RunAdopt(args []string) { |
| if !*stageOnly { |
| passOpts = append(passOpts, release.NagExpiredCanaries(os.Stdout)) |
| } |
| - if err := release.Pass(ctx, reg, store, pub, release.OptDir(), restartRoute, passOpts...); err != nil { |
| + if err := release.Pass(ctx, reg, store, pub, release.OptDir(), restart, passOpts...); err != nil { |
| log.Fatalf("adopt: %v", err) |
| } |
diff --git a/internal/host/agent_test.go b/internal/host/agent_test.go| index 96f6c8c8a..a44b45427 100644 |
| --- a/internal/host/agent_test.go |
| +++ b/internal/host/agent_test.go |
| @@ -5,6 +5,7 @@ import ( |
| "crypto/ed25519" |
| "encoding/base64" |
| "errors" |
| + "os" |
| "path/filepath" |
| "runtime" |
| "slices" |
| @@ -517,6 +518,7 @@ func TestMakeRestartRouteDispatchesOnBacking(t *testing.T) { |
| var restarted, bounced []string |
| restart := makeRestartRoute(reg, |
| func(h string) { restarted = append(restarted, h) }, |
| + func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) }, |
| func(h string) bool { bounced = append(bounced, h); return true }) |
| if err := restart("unit.test"); err != nil { |
| t.Fatalf("restart unit.test: %v", err) |
| @@ -538,7 +540,8 @@ func TestMakeRestartRouteDispatchesOnBacking(t *testing.T) { |
| func TestMakeRestartRouteUnknownHostFallsBackToUnit(t *testing.T) { |
| reg := restartTestRegistry(t) |
| var restarted []string |
| - restart := makeRestartRoute(reg, func(h string) { restarted = append(restarted, h) }, nil) |
| + restart := makeRestartRoute(reg, func(h string) { restarted = append(restarted, h) }, |
| + func(u string) { t.Fatalf("named-unit arm fired for %q on a host the registry cannot answer for", u) }, nil) |
| if err := restart("ghost.test"); err != nil { |
| t.Fatalf("restart ghost.test: %v", err) |
| } |
| @@ -573,7 +576,8 @@ func TestMakeRestartRouteExecFallsThroughWhenNothingBounced(t *testing.T) { |
| t.Fatalf("upsert exec route: %v", err) |
| } |
| var restarted []string |
| - restart := makeRestartRoute(reg, func(h string) { restarted = append(restarted, h) }, tc.bounce) |
| + restart := makeRestartRoute(reg, func(h string) { restarted = append(restarted, h) }, |
| + func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) }, tc.bounce) |
| if err := restart("exec.test"); err != nil { |
| t.Fatalf("restart exec.test: %v", err) |
| } |
| @@ -584,6 +588,155 @@ func TestMakeRestartRouteExecFallsThroughWhenNothingBounced(t *testing.T) { |
| } |
| } |
| +// TestMakeRestartRouteRestartsTheUnitARouteNames pins issue #110's fix: an |
| +// addr-backed route that names its own unit (the consoles, `-addr |
| +// 127.0.0.1:8090` under a bespoke console.service) is cycled BY THAT NAME, |
| +// and neither of the other two arms runs — both would cycle nothing here, |
| +// which is exactly how adoption came to stamp a Version the running process |
| +// did not have. |
| +func TestMakeRestartRouteRestartsTheUnitARouteNames(t *testing.T) { |
| + reg := restartTestRegistry(t) |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "console.carloku.com", Kind: "instance", App: "console", |
| + Addr: "127.0.0.1:8090", Status: "ready", Unit: "console.service", |
| + }); err != nil { |
| + t.Fatalf("upsert console route: %v", err) |
| + } |
| + var units, restarted, bounced []string |
| + restart := makeRestartRoute(reg, |
| + func(h string) { restarted = append(restarted, h) }, |
| + func(u string) { units = append(units, u) }, |
| + func(h string) bool { bounced = append(bounced, h); return true }) |
| + if err := restart("console.carloku.com"); err != nil { |
| + t.Fatalf("restart console.carloku.com: %v", err) |
| + } |
| + if len(units) != 1 || units[0] != "console.service" { |
| + t.Fatalf("named-unit arm: %v, want [console.service]", units) |
| + } |
| + if len(restarted) != 0 { |
| + t.Fatalf("a route that names its unit must not also be try-restarted as carlos-app@<host>: %v", restarted) |
| + } |
| + if len(bounced) != 0 { |
| + t.Fatalf("a route that names its unit must not be offered to the activator: %v", bounced) |
| + } |
| +} |
| + |
| +// The named-unit arm wins over BACKING, not merely over its absence: a route |
| +// carrying both a unit name and unit backing names where its process is, and |
| +// the name is the more specific answer. Pinned so a later reordering of the |
| +// dispatch has to argue with a test rather than silently cycle |
| +// carlos-app@<host> for a route whose process is somewhere else. |
| +func TestMakeRestartRouteUnitNameBeatsBacking(t *testing.T) { |
| + reg := restartTestRegistry(t) |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "odd.test", Kind: "instance", App: "a", Status: "ready", |
| + Backing: registry.BackingUnit, Unit: "bespoke.service", |
| + }); err != nil { |
| + t.Fatalf("upsert: %v", err) |
| + } |
| + var units, restarted []string |
| + restart := makeRestartRoute(reg, |
| + func(h string) { restarted = append(restarted, h) }, |
| + func(u string) { units = append(units, u) }, |
| + nil) |
| + if err := restart("odd.test"); err != nil { |
| + t.Fatalf("restart odd.test: %v", err) |
| + } |
| + if len(units) != 1 || units[0] != "bespoke.service" || len(restarted) != 0 { |
| + t.Fatalf("units = %v, host try-restarts = %v; want the named unit alone", units, restarted) |
| + } |
| +} |
| + |
| +// The root pass (RunAdopt, carlos-adopt.service) must carry the named-unit |
| +// arm, because on a real box it is the pass that actually adopts a console: |
| +// the agent runs inside carlos-edge.service's ProtectSystem=strict sandbox |
| +// and cannot write /opt/carlos, so its stage is blocked and relayed here. |
| +// Everything else about it is unchanged — no activator, so every route it |
| +// used to try-restart by host still is. |
| +func TestRootRestartRouteCarriesTheNamedUnitArm(t *testing.T) { |
| + reg := restartTestRegistry(t) |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "console.carloku.com", Kind: "instance", App: "console", |
| + Addr: "127.0.0.1:8090", Status: "ready", Unit: "console.service", |
| + }); err != nil { |
| + t.Fatalf("upsert: %v", err) |
| + } |
| + // A fake systemctl on PATH records the real argv this callback builds — |
| + // the same recording trick TestBootstrapStagingRunsNoSystemctl uses — so |
| + // this covers the production path (systemctlTryRestartUnit's exec.Command) |
| + // and not just the constructor. |
| + bin := t.TempDir() |
| + callLog := filepath.Join(t.TempDir(), "systemctl-calls") |
| + if err := os.WriteFile(filepath.Join(bin, "systemctl"), |
| + []byte("#!/bin/sh\necho \"$@\" >> "+callLog+"\n"), 0o755); err != nil { |
| + t.Fatal(err) |
| + } |
| + t.Setenv("PATH", bin+string(os.PathListSeparator)+os.Getenv("PATH")) |
| + |
| + if err := rootRestartRoute(reg)("console.carloku.com"); err != nil { |
| + t.Fatalf("root restart: %v", err) |
| + } |
| + got, err := os.ReadFile(callLog) |
| + if err != nil { |
| + t.Fatalf("the root pass ran no systemctl at all: %v", err) |
| + } |
| + if want := "try-restart console.service\n"; string(got) != want { |
| + t.Fatalf("systemctl calls = %q, want %q", got, want) |
| + } |
| +} |
| + |
| +// TestAdoptPassRestartsANamedUnitOnAdoption is the end-to-end: a promote the |
| +// pass adopts must reach the route's own unit, not just repoint a symlink and |
| +// stamp a version. This is the shape of the deploy that produced issue #110. |
| +func TestAdoptPassRestartsANamedUnitOnAdoption(t *testing.T) { |
| + dir := t.TempDir() |
| + optDir := t.TempDir() |
| + s := release.NewDirStore(dir) |
| + ctx := context.Background() |
| + plat := runtime.GOOS + "-" + runtime.GOARCH |
| + |
| + pub, priv, err := ed25519.GenerateKey(nil) |
| + if err != nil { |
| + t.Fatalf("generate key: %v", err) |
| + } |
| + reg := restartTestRegistry(t) |
| + if err := reg.Upsert(registry.Route{ |
| + Host: "console.test", Kind: "instance", App: "console", Channel: "canary/console", |
| + Addr: "127.0.0.1:8090", Status: "ready", Unit: "console.service", |
| + }); err != nil { |
| + t.Fatalf("upsert console.test: %v", err) |
| + } |
| + m := release.Manifest{ |
| + App: "console", Version: "v1", Kind: "binary", |
| + Artifacts: map[string]release.Artifact{plat: {}}, |
| + } |
| + if err := release.Ship(ctx, s, "bab", m, map[string][]byte{plat: []byte("build one")}); err != nil { |
| + t.Fatalf("Ship: %v", err) |
| + } |
| + if err := release.Promote(ctx, s, priv, "bab", "console", "canary/console", "v1"); err != nil { |
| + t.Fatalf("Promote: %v", err) |
| + } |
| + |
| + var units []string |
| + restart := makeRestartRoute(reg, |
| + func(h string) { t.Fatalf("carlos-app@ arm fired for %q — the console has no such unit", h) }, |
| + func(u string) { units = append(units, u) }, |
| + func(h string) bool { |
| + t.Fatalf("bounce arm fired for %q — the activator owns no console child", h) |
| + return false |
| + }) |
| + |
| + if err := release.Pass(ctx, reg, s, pub, optDir, restart, release.RestartHibernating(restart)); err != nil { |
| + t.Fatalf("Pass: %v", err) |
| + } |
| + if len(units) != 1 || units[0] != "console.service" { |
| + t.Fatalf("adoption cycled %v, want [console.service] — the header would claim v1 over the old process", units) |
| + } |
| + if v, err := release.LiveVersion(optDir, "console.test"); err != nil || v != "v1" { |
| + t.Fatalf("console.test live = %q, %v; want v1", v, err) |
| + } |
| +} |
| + |
| // TestAdoptPassBouncesAWarmHibernatingInstance is the end-to-end this task |
| // exists for, through the real release.Pass with the real closure: an exec |
| // instance minted the way provision does it (Hibernate: true, Backing "") is |
| @@ -632,6 +785,7 @@ func TestAdoptPassBouncesAWarmHibernatingInstance(t *testing.T) { |
| // it never starts anything. |
| restart := makeRestartRoute(reg, |
| func(h string) { tried = append(tried, h) }, |
| + func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) }, |
| func(h string) bool { |
| bounced = append(bounced, h) |
| return h == "warm.test" |
| @@ -937,6 +1091,7 @@ func TestFastSweepBouncesAWarmHibernatingCanary(t *testing.T) { |
| var bounced []string |
| restart := makeRestartRoute(env.reg, |
| func(string) {}, |
| + func(u string) { t.Fatalf("named-unit arm fired for %q on a route that names no unit", u) }, |
| func(h string) bool { bounced = append(bounced, h); return true }) |
| d := env.deps(nil) |
| d.restart = restart |
diff --git a/internal/release/adopt.go b/internal/release/adopt.go| index 4ad5453ab..46f6925ff 100644 |
| --- a/internal/release/adopt.go |
| +++ b/internal/release/adopt.go |
| @@ -765,7 +765,7 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| if restart != nil { |
| // A converged DAEMON's restart is caps.Restart, not the caller's |
| // callback: this is the only call site that knows the plan, and it is |
| - // the only restart whose error must be real. restartRoute's |
| + // the only restart whose error must be real. rootRestartRoute's |
| // try-restart (and its swallow-and-continue) keeps owning every other |
| // route, byte-identically to before. |
| // |