amadan
comments+spec: say what the code does, twice over
No behaviour change here except one journal line's wording. The comment sweep, all four sites the review named: - systemctlTryRestart claimed "hibernating hosts never reach here at all". They do: a sleeping exec child returns false from Bounce and falls through to try-restart under RestartHibernating. The doc now says the true thing, which is also why the fallthrough is safe — try-restart on a stopped or nonexistent unit is a no-op. - adoptHost's hibernate line printed "hibernating — runs it on next wake" for a host it had just bounced. With a restart callback in hand (RestartHibernating) it now reads "bounced if awake, else runs it on next wake"; without one the old wording stands, because then it really is symlink-only. Same "repointed" prefix, still one grep. - stage_relay_test's "canary loop" is the fast converge loop now. - Four stale references to lowercase release.adoptable (cli.go, appconfig/sync.go x2, e2e_test.go) follow the identifier's export. And the two accepted implementation deltas go into §4 of the spec, so the fleet and the document agree: bake-held routes are not dropped from the fast set by membership — they are refused by the pass's own hold logic and re-checked on the 2s→5min backoff, keeping bake policy in one place; and the unminted-record case is served by the serial-triggered full pass (reconcile leg) rather than by fastMembers, which computes from the registry and cannot see a record with no route — leaving a direct-bucket record on instancesLoop's 5-minute floor.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
7 files changed,
+52
−9
cli.go+1 −1docs/superpowers/specs/2026-08-08-carlos-deploy-design.md+26 −0e2e_test.go+1 −1internal/appconfig/sync.go+2 −2internal/host/agent.go+7 −2internal/host/stage_relay_test.go+2 −2internal/release/adopt.go+13 −1
diff --git a/cli.go b/cli.go| index ae236a9cd..5b5cb3b41 100644 |
| --- a/cli.go |
| +++ b/cli.go |
| @@ -437,7 +437,7 @@ func runRoute(args []string) { |
| // before the registry is even opened") while nothing performed it, and |
| // registry.SetBacking writes the column verbatim. `-backing unti` was |
| // therefore accepted, wrote "unti", and left the route neither unit-backed |
| - // (release.adoptable compares against BackingUnit exactly) nor visibly |
| + // (release.Adoptable compares against BackingUnit exactly) nor visibly |
| // wrong — the migration's one write, silently a no-op. Same wording as |
| // runAdd's check, which is the other door onto this column. |
| if backingSet && *backing != "" && *backing != registry.BackingUnit { |
diff --git a/docs/superpowers/specs/2026-08-08-carlos-deploy-design.md b/docs/superpowers/specs/2026-08-08-carlos-deploy-design.md| index 42384b34e..ea637bec4 100644 |
| --- a/docs/superpowers/specs/2026-08-08-carlos-deploy-design.md |
| +++ b/docs/superpowers/specs/2026-08-08-carlos-deploy-design.md |
| @@ -142,6 +142,32 @@ to bounce. Hibernating instances still get the pure repoint and pick the |
| new binary up on wake. With that, "new version on next refresh" holds for |
| warm and cold instances alike. |
| +**Held routes stay in the set and are refused by the pass** (implementation |
| +delta, 2026-08-08). The bake bullet above says a held route "drops out of the |
| +fast set"; `fastMembers` does not special-case bake at all. A held route has |
| +no live symlink, so it reads as outstanding work, gets swept, and the pass's |
| +own hold logic refuses to stage it — after which `noteUnstaged` doubles that |
| +host's backoff (2 s → 5 min cap) and it is re-checked on that schedule until |
| +the window expires. Same outcome as the spec promises (no 2 s polling for |
| +72 h) with one fewer place for bake policy to live: the pass is already the |
| +single authority on what is holdable, and a second copy of that judgment in |
| +the sweep's membership test could disagree with it. Cost is bounded by the |
| +backoff: a held route settles at one look per 5 minutes, which the hourly |
| +floor was going to do anyway. |
| + |
| +**The unminted-record case is served by the full pass, not by the sweep** |
| +(implementation delta, 2026-08-08). The membership bullet "its instance record |
| +exists but the route is not yet minted" is not something `fastMembers` can |
| +see: it computes from the REGISTRY, and an unminted record has no route to |
| +list. It is covered instead by the serial leg — a console instance-create |
| +bumps `control/serial`, and the bump runs the full pass set, whose leg 1 is |
| +`provision.Reconcile`, which mints the route; the route is then ordinary fast- |
| +set membership from the next tick. The gap this leaves is the direct-bucket |
| +path: a record written straight into the bucket without a console bump (an |
| +operator, a script) waits for `instancesLoop`'s own 5-minute floor rather than |
| +2 s. Accepted — console-created instances are the fast path this feature is |
| +for, and 5 minutes is already the minting cadence that path had before it. |
| + |
| ## 5. Materialize: cold wake stages on demand |
| `ExecBackend.Materialize` (`internal/activator/backend_exec.go`, nil in |
diff --git a/e2e_test.go b/e2e_test.go| index 3d1783f11..bbde67b4d 100644 |
| --- a/e2e_test.go |
| +++ b/e2e_test.go |
| @@ -3329,7 +3329,7 @@ func (f *routeTestFixture) routeOf(host string) registry.Route { |
| // TestRouteBackingRefusesAnUnknownValue is the 2026-08-03 review's I1: |
| // runRouteBacking's doc comment already claimed the value was checked "at the |
| // flag before the registry is even opened", and nothing performed the check. |
| -// registry.SetBacking writes the column verbatim, and release.adoptable |
| +// registry.SetBacking writes the column verbatim, and release.Adoptable |
| // compares against BackingUnit EXACTLY — so `-backing unti` was accepted, |
| // stored "unti", and left the route neither unit-backed nor visibly wrong: the |
| // migration's one write, silently a no-op that only shows up as an app that |
diff --git a/internal/appconfig/sync.go b/internal/appconfig/sync.go| index 083a29443..246ec1d54 100644 |
| --- a/internal/appconfig/sync.go |
| +++ b/internal/appconfig/sync.go |
| @@ -275,10 +275,10 @@ func lostKeys(cur []byte, m *Manifest) (lost []string, parsed bool) { |
| return lost, true |
| } |
| -// syncable mirrors release.adoptable: lifecycle-managed instances of a |
| +// syncable mirrors release.Adoptable: lifecycle-managed instances of a |
| // known app, excluding platform-minted alias routes (an alias shares its |
| // canonical route's process and therefore its canonical route's config — |
| -// see release.adoptable's own doc comment for why aliases are excluded |
| +// see release.Adoptable's own doc comment for why aliases are excluded |
| // from adoption the same way). |
| func syncable(rt registry.Route) bool { |
| return rt.Kind == "instance" && rt.App != "" && !rt.Platform && |
diff --git a/internal/host/agent.go b/internal/host/agent.go| index 04e1d6986..84e7e0f50 100644 |
| --- a/internal/host/agent.go |
| +++ b/internal/host/agent.go |
| @@ -1544,8 +1544,13 @@ func makeRestartRoute(reg *registry.Registry, tryRestart func(host string), boun |
| // systemctlTryRestart is makeRestartRoute's production unit path: cycle the |
| // route's carlos-app@ unit iff it's running, best-effort. A missing or |
| -// stopped unit is not this pass's problem — hibernating hosts never reach |
| -// here at all (release.Pass gives them the symlink-only path). |
| +// stopped unit is not this pass's problem — try-restart is by definition a |
| +// no-op on one, which is what makes makeRestartRoute's fallthrough safe: |
| +// an exec route whose child is asleep returns false from bounce (nothing to |
| +// cycle) and lands here under RestartHibernating, where try-restart finds no |
| +// carlos-app@ unit for it and does nothing. Sleeping hosts DO reach this |
| +// line; they simply have nothing here to restart, and pick the new version |
| +// up on their next wake. |
| func systemctlTryRestart(host string) { |
| out, err := exec.Command("systemctl", "try-restart", activator.UnitName(host)).CombinedOutput() |
| if err != nil { |
diff --git a/internal/host/stage_relay_test.go b/internal/host/stage_relay_test.go| index 66a3ab4a1..d9db7a4e2 100644 |
| --- a/internal/host/stage_relay_test.go |
| +++ b/internal/host/stage_relay_test.go |
| @@ -61,8 +61,8 @@ func TestStageRequesterThrottles(t *testing.T) { |
| _ = first |
| } |
| -// Each loop gets its own budget: the canary loop asking must not silence the |
| -// hourly one. stageRequester returns a fresh closure per call, so this is a |
| +// Each loop gets its own budget: the fast converge loop asking must not |
| +// silence the hourly one. stageRequester returns a fresh closure per call, so this is a |
| // property of the API shape rather than of the clock. |
| func TestStageRequesterBudgetIsPerRequester(t *testing.T) { |
| dd := t.TempDir() |
diff --git a/internal/release/adopt.go b/internal/release/adopt.go| index ae00eb8fc..4ad5453ab 100644 |
| --- a/internal/release/adopt.go |
| +++ b/internal/release/adopt.go |
| @@ -820,7 +820,19 @@ func adoptHost(ctx context.Context, reg *registry.Registry, s Store, optDir stri |
| return |
| } |
| if rt.Hibernate { |
| - fmt.Printf(" %s: repointed %s -> %s (%s; hibernating — runs it on next wake)\n", rt.Host, cur, ver, channel) |
| + // Two different facts, one grep. With no restart callback this pass |
| + // genuinely only moved a symlink, and the next wake is when the new |
| + // version runs. With one (RestartHibernating, the agent's loops) the |
| + // callback has ALREADY been consulted above: a warm exec child was |
| + // bounced onto ver a few lines ago, and a sleeping one was a no-op. |
| + // Which of the two happened is makeRestartRoute's to log — it prints |
| + // the bounce — so this line stops claiming the sleeping case for |
| + // both, rather than growing a third state to track. |
| + if restart != nil { |
| + fmt.Printf(" %s: repointed %s -> %s (%s; hibernating — bounced if awake, else runs it on next wake)\n", rt.Host, cur, ver, channel) |
| + } else { |
| + fmt.Printf(" %s: repointed %s -> %s (%s; hibernating — runs it on next wake)\n", rt.Host, cur, ver, channel) |
| + } |
| } else { |
| fmt.Printf(" %s: updated %s -> %s (%s)\n", rt.Host, cur, ver, channel) |
| } |