carlos / platform Public

Astra review P1 of the zero-downtime plan set (REJECT, 16 findings)

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

claude 2 weeks ago 632daf0ab00b607968b23c2815f3513517e99938 parent 6aee9ed

1 file changed, +33 −0

docs/superpowers/plans/CODEX-FINDINGS-ZDD-PLANS-P1.md +33 −0

CI passed — run details

diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-PLANS-P1.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-PLANS-P1.md

new file mode 100644

index 000000000..77679be5b

--- /dev/null

+++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-PLANS-P1.md

@@ -0,0 +1,33 @@

+REJECT

+1. **P1 — Plan 3, Task 2: the barrier proves the leader exited, not that its generation is empty.** Spec §§7.3–7.4 require termination of every generation before restoring limits or releasing the lease. `stopMember` accepts `Proc.Exited()`, but the flat backend’s [`childProc.Stop`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/internal/activator/backend_exec.go:1531) signals only the leader. A surviving descendant is forgotten when `leaveSet` removes the member; subsequent stops can release the lease over it. **Fix:** make generation termination include child-cgroup emptiness, using `KillGen` after the graceful drain and retaining the member when that proof fails.

+2. **P1 — Plan 3, Task 7 undoes Plan 2, Task 10’s sidecar containment.** The replacement [`startSidecarChildFrom`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:3394) calls `spawnInCgroup`, discarding Plan 2’s `role-sidecar` placement. That reapplies the single-tier limit and `memory.oom.group=1` to the generation parent. With controllers delegated, placement into that parent can also fail and leave the sidecar unaccounted. This breaks spec §7.3. **Fix:** preserve Plan 2’s `placeFor`/`spawnInCgroupAt` and child cleanup while adding the executable parameter.

+3. **P1 — Plan 3, Tasks 1/9 and Plan 5, Task 9: real first-wake health failures bypass fallback and failure recording.** `StartStaged` retains the backend health waits. On failure, both current [flat](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/internal/activator/backend_exec.go:989) and [nested](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/internal/activator/backend_exec.go:1112) starts drain the process and return a nil handle. `spawnPlanned` interprets that nil as “nothing executed” and returns before recording failure or booting the good build. Its shortened health deadline is applied only after this internal wait. Spec §8.2 therefore fails; the fake-backed budget test misses it. **Fix:** separate staged spawning from health checking, return the process handle immediately, and let the caller own health deadlines and termination.

+4. **P1 — Plan 3, Task 6: `hold` canaries have no settle.** [`swapHold`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:2913) returns `SwapDone` as soon as the pinned wake becomes healthy, ignoring `at.Canary`. Plan 5 deliberately bypasses the planner for `e.pin`, so its first-wake observer does not compensate. The scheduler then marks the cohort settled and the build good; a crash seconds later does not halt rollout. This violates §§6.2, 6.4 and 8.5. **Fix:** observe a hold canary for ten seconds before completing the attempt; an unexpected exit must produce `SwapDown` without fallback.

+5. **P1 — Plan 5, Task 11: worker replacements never re-check supersession after admission.** [`ReplaceWorker`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-5-scheduler.md:5826) calls `newest` only before retiring the incumbent. The existing respawn’s [pre-spawn check](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/internal/activator/activator.go:1904) checks entry/state/shutdown, and the new completion path checks neither revision nor cohort halt. A newer release or sibling failure during retirement still allows the obsolete build to execute and become good. This violates §§6.3, 7.4 and 8.1. **Fix:** carry the attempt’s admission and revision checks into the actual respawn and re-check before accepting its result.

+6. **P1 — Plan 5, Task 8: failure completion opens admission before halting the cohort.** [`scheduler.done`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-5-scheduler.md:4090) calls `q.finish`, freeing the concurrency slot and canary, then unlocks and reads the registry before recording and halting the failure. A concurrent scheduler kick can admit another attempt during that gap; an existing attempt can also pass its newest check. Spec §8.1 requires the failure transition to stop those attempts. **Fix:** record the failure and supersede the cohort under the same scheduler critical section that finishes the failed attempt.

+7. **P1 — Plan 5, Task 12: failed pin publication still permits spawning.** [`persistGenerations`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-5-scheduler.md:6388) logs write failures and continues. Root then reads a valid but stale file—or an absent file interpreted as no generations. Once install-history pins move on, root can sweep or overwrite a running/pending artifact absent from that file. This violates §§4.2 and 8.5 and the index’s generations-file contract. **Fix:** require successful publication of added pins before spawning; a publication failure must prevent that spawn.

+8. **P1 — Plan 3, Tasks 1/10: overlap limits use the wrong tiers.** [`RaiseGenParent`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:590) sums the live-link tier twice; the claims path sums its stored parent tier twice. Neither computes incumbent plus successor as required by §7.3 and the accepted amendment. A 1 GiB incumbent followed by a 512 MiB build gets only a 1 GiB parent during overlap, permitting an OOM to kill the incumbent. Staged spawns also obtain their tier from the mutable live link. **Fix:** capture each artifact’s tier and use those captured values for child limits, overlap sum and restoration.

+9. **P1 — Plan 3, Tasks 1/6 and Plan 5, Task 9: foreign-layout instances cannot perform planned wakes.** The index explicitly preserves their canonical socket and disallows overlap. However, [`StartStaged`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:563) rejects `LayoutForeign` altogether, while Plan 5 routes planned wakes through it. A previously working foreign-layout instance therefore fails its next wake with the flag enabled; the promised hold path fails too. **Fix:** support canonical-socket staged spawning for cold/hold starts and force foreign-layout replacements through hold.

+10. **P1 — Plan 3, Task 7: nested legacy sidecars never receive the retained executable.** [`StartSidecarGen`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:3441) checks the live version and then starts the existing live-link-based unit. Following fallback, it refuses the sidecar indefinitely because installed and serving differ; a repoint between the check and start can instead launch the wrong build. Spec §7.5 explicitly requires the serving artifact at every sidecar spawn. **Fix:** extend the nested sidecar launch/mount path to accept and validate the captured staged executable.

+11. **P2 — Plan 3, Task 7: an already-running supervisor can defeat sidecar replacement.** [`replaceSidecar`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:3491) returns when `sidecarStarting` is true. That supervisor captured the old generation, and its unchanged completion check validates only entry identity/state. If it finishes after the switch, the old sidecar remains beside the new server with no replacement pending, violating §7.5. **Fix:** join the existing sidecar operation and replace it, or reject its completion when the captured generation is stale and schedule replacement.

+12. **P2 — Plan 5, Task 9: socketless first wakes retain incompatible readiness rules.** `spawnPlannedWorker` calls the current [`awaitWorkerSettle`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/internal/activator/activator.go:1632), which accepts an early ready report and rejects every early exit, including exit zero. A successful one-shot worker is consequently recorded as a failed build and can halt its cohort. Spec §6.5 explicitly treats its clean exit as success. **Fix:** give planned worker wakes the same ten-second/clean-exit readiness rules as replacements, preserving the existing helper for flag-off behavior.

+13. **P2 — Plan 1, Task 5: response identity is sampled before the connection chooses its generation.** [`genRoundTripper.RoundTrip`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-1-generations.md:2713) captures generation A, but its transport re-resolves `AppSocket` during dialing. A concurrent switch can send the request to B while the response is stamped and attributed to A. That violates §7.2’s serving-version and cache-attribution requirements. **Fix:** bind response attribution to the generation of the actual connection, keeping that connection in the corresponding generation pool.

+14. **P2 — Plan 5, Tasks 9/13: cancelling a first-wake observation leaves status settling forever.** [`watchFirstWake`](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-5-scheduler.md:4863) emits nothing on intentional cancellation. Task 13 sets `m.observing[host]` on `GenHealthy` and clears it only on a subsequent event. Hibernation or maintenance during the window therefore leaves a permanent open observation, blocking deploy-watch completion under §9.1. **Fix:** emit a cancellation/completion notification that clears observation state without recording a build failure.

+15. **P2 — Plan 6, Task 10: final hosts retain stale scheduler status and make successful deploys time out.** The [watch loop](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-6-surfaces.md:3123) skips refresh whenever `h.done()` is true. A sleeper observed installed while another instance settles is final, but retains `SchedulerIdle=false` permanently; `boxDone` consequently never succeeds. The plan’s own `TestWatchRolloutFollowsTheBoxToDone` supplies exactly this sequence. **Fix:** continue refreshing every rollout host until the whole watch completes; use `printed` only to suppress duplicate output.

+16. **P2 — Plan 3, Task 4 cannot compile after Plan 1, Task 4.** Both introduce `servingProc` and its `Stop` method in package `activator`, with incompatible fields: [Plan 1’s definition](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-1-generations.md:1735) and [Plan 3’s definition](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/plans/2026-10-07-zero-downtime-deploys-3-swap.md:1726). Following the prescribed landing order produces duplicate declarations. **Fix:** rename Plan 3’s helper and its uses, or reuse a single compatible helper.

\ No newline at end of file