carlos / platform Public

zdd-5: Astra's final review, round 1 (REJECT)

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

claude 2 weeks ago db81e734b151c3f7521027661a500bb7758a300f parent 5b6c95e

1 file changed, +221 −0

docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5.md +221 −0

diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5.md

new file mode 100644

index 000000000..7142573bc

--- /dev/null

+++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5.md

@@ -0,0 +1,221 @@

+REJECT

+Reviewed `a3bf7d6c..5b6c95ed`; the supplied diff matches exactly. Nothing was changed, and no tests or simulations were run. The recorded 3,000-seed pass and six caught mutations are useful evidence, but the simulation models the activator and does not exercise the production launch preparation, delayed callbacks, or failed persistence described below. [Recorded results](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/.superpowers/sdd/2026-10-07-zero-downtime-deploys-5-scheduler/task-16-fix-report.md:272)

+These findings are latent until the flag is **ON**, most severe first:

+1. **High — instance gates run before potentially blocking launch preparation, not at execution.**

+ [swaphold.go:359](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/swaphold.go:359), [swap.go:311](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/swap.go:311).

+ A planned wake, fallback, hold pin, or overlap successor passes its gate and enters `StartStaged`. The backend then reads config, prepares sockets/files, and, for nested instances, contacts root. During that work the lease can disappear, a fence can arrive, or the attempt/build can become superseded or held. Execution still proceeds: the flat path reaches `exec.Command` at `backend_exec.go:1448`; the nested path reaches `startRole` at `backend_exec.go:1630`, without another gate.

+ This violates §7.4, PF7, and the P5/P6 execution-bound ownership obligation. Killing the process afterward cannot undo a hold build’s migration. **Smallest fix:** carry the gate through `StartStaged` into the final launch point after preparation, covering both backends and every staged-instance caller; honour cancellation there too.

+2. **High — a hold worker can restore its predecessor after the hold build executed.**

+ [workerswap.go:101](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/workerswap.go:101), [activator.go:2770](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/activator.go:2770).

+ Worker A is replaced by hold build B. B executes and can migrate the database. Before its installation into the entry, a newer desire supersedes B. The post-exec check sheds B and leaves the old incarnation unchanged. `ReplaceWorker` sees `gateSuperseded` and calls `restoreLocked(e, prev)`, automatically executing A.

+ This violates §§6.4/8.3 and P5’s hold-everywhere invariant. “Not installed” is not evidence that B never executed. **Smallest fix:** preserve explicit execution evidence through the respawn result; permit predecessor restoration only when the hold build provably never executed. Otherwise stop and preserve the required recovery restriction.

+3. **High — cold-wake admission reopens before its failure is recorded.**

+ [activator.go:1394](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/activator.go:1394), [wakeplan.go:503](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/wakeplan.go:503).

+ Hold B executes and fails its first health check. `wake` emits the failure event, deletes the entry, unlocks, and closes `ready`; only afterward does `afterWake → OnGen` install `HeldDown`. A request scheduled in that gap creates another wake and plans B before the record exists. Planned wakes have no running scheduler slot to block it.

+ The observer has the same ordering weakness: it clears `firstWatch` at `wakeplan.go:579` before delivering `GenFailed`. This violates §8.1’s failure-before-publication/admission ordering and O7. **Smallest fix:** retain an outcome/admission fence until the off-lock failure transition completes; publish failure and reopen admission afterward. Join the observer’s outcome with terminal cleanup.

+4. **High — an old outcome can remove a newer planned wake’s failure-record protection.**

+ [rollout/wakeplan.go:244](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/wakeplan.go:244), [manager.go:637](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/manager.go:637).

+ An old first-wake observer has a delayed `GenCancelled`. The host begins a new first wake of B with good build A. A sibling fails B; root moves the installing routes to C. The old event executes `delete(m.planned, host)`, deleting B’s newer registration. `Pass` now prunes B’s failure record, and the new wake’s `planCheck` admits B.

+ This recreates simulation invariant I1 and violates §8.4. It is not a minor “three-way coincidence.” Host-keyed `observing` and mutable `wakeRev` also let old events affect newer observation/config state. **Smallest fix:** carry a unique wake token through `WakePlan` and `GenEvent`; clear or activate only matching state, with the config revision captured in that wake’s event.

+5. **High — failed persistence loses failure holds across agent restart.**

+ [failures.go:100](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/failures.go:100), [scheduler.go:203](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/scheduler.go:203).

+ B fails, but the atomic file replacement fails, for example because storage is full. The transition retains the record only in memory, logs the error, and continues. Restart the agent before a successful retry: `OpenFailureBook` reads the previous valid file, or treats its absence as an empty book. B is eligible again, including after a hold failure.

+ This violates §8.4’s restart durability. The dirty flag does not close the crash window. **Smallest safe fix:** persist an unresolved-attempt marker before dangerous execution, refuse execution if that write fails, and clear it only after a durable outcome. Startup must conservatively reconcile or refuse unresolved markers; write failures cannot be treated as journal-only errors.

+6. **High — losing delegation silently bypasses existing failure records.**

+ [host/rollout.go:137](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/host/rollout.go:137), [host/rollout.go:287](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/host/rollout.go:287).

+ An armed agent records a hold failure for installed B. It restarts with `CARLOS_SWAP=overlap` still set but delegation unavailable. `swapArming` declines the feature; `armRollout` returns before loading the book or installing refusal hooks. Legacy wake/converge paths can execute B.

+ This violates §§6.4/8.4 and O1’s fail-closed intent. An infrastructure prerequisite failing is not an operator clearing a failure hold. **Smallest fix:** leave generation containment disabled but refuse managed starts and stand down their bounce legs when the requested feature cannot arm.

+7. **Medium — config delivery does not consistently reach activation or truthful acknowledgement.**

+ [syncexec.go:615](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/appconfig/syncexec.go:615), [host/rollout.go:382](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/host/rollout.go:382).

+ Three concrete paths remain:

+ - An awake native sidecar receives a changed config file, but `hostBounce == nil` clears `Delivered`. No config attempt is queued; unchanged-file passes subsequently do nothing.

+ - An in-scope socket route outside `Managed` receives the global successful no-op restart. `ConfigDelivered` ignores it, and its ack can say `Restarted:true` while the old process continues.

+ - In refusing mode, a sleeping managed exec route also receives a successful no-op; that path does not apply `configTrack`’s `Hibernating` correction.

+ These violate §§6.5/8.1/10. **Smallest fix:** separate delivery notification from legacy bounce eligibility; retain `Delivered` for managed sidecars, preserve the existing restart for unmanaged exec routes, and explicitly acknowledge refused sleepers as delivered-on-wake.

+8. **Medium — a pre-exec refusal permanently consumes a host retry.**

+ [queue.go:348](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/queue.go:348), [failures.go:200](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/failures.go:200).

+ Retry a host in a halted B cohort. Admission clears `forceBuild`; staging or an execution gate then abandons the attempt before B executes. `finish` releases the canary but does not restore the allowance. The cohort remains halted, while another `RetryHost` returns false because the host already appears in `Retried`.

+ This violates §8.4’s promised next canary and the scheduler liveness obligation. **Smallest fix:** retain/re-arm the build-specific retry allowance on pre-exec abandonment; consume it on a meaningful executed outcome.

+The following affects the box with the flag **OFF**:

+9. **Low — an existing member-visible failure string changes outside the flag.**

+ [workerbuild.go:36](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/workerbuild.go:36).

+ A worker’s generations-file publication fails with the flag unset. The shared sentinel now says `build not published…` instead of `worker build not published…`; `memberSafeWakeReason` passes that changed wording through. This violates index review focus 1’s unchanged legacy behavior outside the assigned exceptions. **Smallest fix:** preserve the existing worker wording and use a separate member-safe mapping for the new instance path.

+I found no additional lock-order inversion in the reviewed scheduler paths. The single generations writer and preservation of both version and path pins are sound structural choices; they do not repair the execution and outcome-ordering gaps above.

+The following judges all **35 ledger rulings**. Line numbers refer to [progress.md](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/.superpowers/sdd/2026-10-07-zero-downtime-deploys-5-scheduler/progress.md:62).

+| Ruling | Judgment | Reason |

+|---|---|---|

+| PF1, L63 | Agree | Merged producer signatures supersede stale plan signatures. |

+| PF2, L64 | Agree | Compile-time interface contracts have a useful, limited purpose. |

+| PF3, L65 | Agree | Appending columns preserves the established scan/write ordering. |

+| PF4, L66 | Agree | A verified newer signed pointer is the specified retry signal; install authorization remains root’s. |

+| PF5, L67 | Agree with conditions | Fixed scheduler reasons are correct, but planned-wake error passthrough still needs the same protection before enablement. |

+| PF6, L68 | Agree | The anchor changes follow the merged implementation. |

+| PF7, L69 | Agree with conditions | The composed checks are right, but must reach the actual backend launch boundary, finding 1. |

+| PF8, L70 | Agree | Consuming the existing worker publication/gate contract avoids a second lifecycle. |

+| PF9, L71 | Agree with conditions | Publication refusal is abandonment, but preserve flag-off wording and do not assume startup catches later storage failures. |

+| PF10, L72 | Agree | Binding the legacy sidecar to the chosen staged identity is required. |

+| PF11, L73 | Disagree | Supersession alone does not authorize restoration after a hold build executed, finding 2. |

+| PF12, L74 | Agree | Extending the existing writer and retaining both pin namespaces is correct. |

+| PF13, L75 | Agree | Existing sweep interfaces already consume the generations file. |

+| PF14, L76 | Agree | One flag-independent writer configuration is the intended ownership. |

+| PF15, L77 | Disagree | Declining into legacy execution discards previously established failure protection, finding 6. |

+| PF16, L78 | Agree | Proof-mint requests must not enter the stage relay. |

+| PF17, L79 | Agree with conditions | Deferral is valid only while every claims-backed launch remains closed. |

+| PF18, L80 | Agree with conditions | The scheduler-slot transition is ordered, but cold-wake/observer outcomes still violate O7, finding 3. |

+| T2 fingerprint, L85 | Agree | Empty new columns must not force snapshots across the fleet. |

+| T2 archive canonicalization, L86 | Agree with conditions | It fixes upgrade comparisons; dropping unknown future ownership-bearing fields must remain forbidden. |

+| T4 slice copying, L93 | Agree | Returned records must not share mutable slices with the book. |

+| T4 dirty persistence, L94 | Disagree | Retrying later does not establish restart durability, finding 5. |

+| T5 early cohort type, L99 | Agree | This repairs a declaration-order defect without changing behavior. |

+| T5 adoption dedupe, L100 | Agree | Re-adopting the serving artifact has no work; restart/config remain distinct. |

+| T6 forced canary, L104 | Agree | A retry must still wait behind an unsettled canary. |

+| T8 install-by-hand, L116 | Agree with conditions | The supported same-build retry signals must be explicit; ordinary identical restamping cannot be advertised as one. |

+| T8 sleeper/prune scope, L118 | Agree | Sleepers need no redundant config attempt, and every installing route retains the record. |

+| T9 verdict floor, L123 | Agree with conditions | Skipping B is safe only before execution and with later paced adoption still possible; document the additional 20-second policy. |

+| T10 no-good bootstrap, L128 | Agree | This preserves the explicit bootstrap exception without lifting host-specific hold-down. |

+| T11 respawn deviations, L134 | Disagree | Omitting execution evidence makes the supersession restoration unsafe, finding 2. |

+| T14 refusal hooks, L145 | Agree with conditions | Standing down convergence is correct; an uncaptured worker must also be refused rather than recycled from the live link. |

+| T14 refusing config ack, L149 | Agree with conditions | Awake refusal is correct, but the actual sleeper path must set `Hibernating`, finding 7. |

+| T15 relay refinement, L155 | Agree | Any changed install stamp ends the recorded request; moves remain separate. |

+| T10 planned retention, L161 | Disagree | “Any event for the host” can clear a newer plan, finding 4. |

+| T10 early registration, L163 | Agree with conditions | Early registration closes the slow-read window only when subsequent removal is token-scoped. |

+For the deferred minors, **Merge** means fix before this branch lands, **Flag** means before enabling anywhere, and **Defer** means safe to carry. Related test gaps are grouped on one line.

+| Ledger item | Triage |

+|---|---|

+| T1 L83: whitespace/near-miss flag cases; stale surface-test comment | **Defer.** |

+| T2 L88: unknown/duplicate/case-variant JSON keys; future timer-field compatibility; isolated replay-refusal test | **Defer**, retaining the unknown-ownership-field condition above. |

+| T3 L90: older policy load overwriting a newer result | **Flag:** serialize or version cache fills. |

+| T3 L90: refresh backoff and policy-loader edge-case tests | **Flag:** bound repeated failed reads and cover missing/error distinctions. |

+| T3 L90: literal default hold value | **Defer.** |

+| T4 L97: persistence under the book lock and missing durable-write protocol | **Merge:** finding 5; keep the in-memory safety transition ordered. |

+| T5 L101: running-attempt, config/backoff and deferral test gaps | **Flag.** |

+| T5 L101: `wantConfig("")` behavior | **Defer:** production revision inputs should remain nonempty. |

+| T6 L107: abandoned forced attempt loses retry | **Merge:** finding 8. |

+| T6 L107: older non-canary failure interacting with a retried canary | **Flag:** establish and test the retry-generation boundary. |

+| T6 L107: retry bound to current `wantBuild` | **Defer/closed:** current manager binding occurs under the same scheduler lock as book clearance. |

+| T6 L109: full-capacity admission postpones later view updates | **Defer:** bounded by the next admission pass. |

+| T6 L109: cohort retention/later-wave canary behavior | **Defer:** retain settled state for the same cohort; explicit retry must reset it. |

+| T6 L109: unbounded test loop | **Merge:** bound its failure path on this shared box. |

+| T6 L109: missing concurrency-one/two-cohort coverage | **Flag.** |

+| T6 L109: unrelated attempt consuming retry flag | **Flag:** test alongside finding 8 and consume only the matching adoption allowance. |

+| T7 L113: delivery-order, addr and removal coverage | **Flag:** include native sidecars and the real host wiring. |

+| T7 L113: claims-enabled delivery coverage | **Defer until claims launch opens**, then required. |

+| T7 L113: `configBackoff(0)` documentation | **Defer.** |

+| T8 L120: unbounded ack write holding a slot | **Flag:** bound/cancel publication and test shutdown with a blocked writer. |

+| T8 L120: substring reason mapping | **Flag:** replace fragile classification where necessary during plan 6’s reason audit. |

+| T8 L120: duplicated halted-event code | **Defer.** |

+| T8 L120: scheduler state never shrinks | **Defer/partly closed:** departed hosts/config tracks are removed; idle app/cohort collection can follow. |

+| T8 L120: execution does not recheck `Managed` | **Flag:** revalidate eligibility before dispatching against a changed route. |

+| T8 L120: synchronous policy/pointer reads on the tick | **Flag:** bound latency so one store failure cannot stall convergence. |

+| T8 L120: missing shutdown test; timing-dependent sleeper regression | **Flag.** |

+| T9 L126: claims refusal is log-only; claimed result lacks `Worker` | **Defer while claims remain closed.** |

+| T9 L126: `WakeWorker` timeout shorter than successful fallback | **Flag:** align caller and operation budgets. |

+| T9 L126: raw planned-wake error passthrough | **Flag:** sanitize before member publication; plan 6 wording alone is insufficient. |

+| T9 L126: stale `unexpectedExit` | **Defer/closed for replacement:** T11 resets it for its observation. |

+| T9 L126: worker fallback event assertions | **Flag.** |

+| T9 L126: repeated drift notification | **Defer/closed:** `Manager.Nudge` coalesces it. |

+| T9 L126: missing short-window gate-refusal assertion | **Merge:** add with finding 1’s execution-bound tests. |

+| T10 L132: pointer read uses agent rather than wake context | **Flag:** pass the wake’s cancellation/deadline. |

+| T10 L132: repeated staging refusals/root touches | **Defer:** preserve existing throttling. |

+| T10 L132: missing cohort-halted event before scheduler registration | **Flag.** |

+| T10 L132: panic cleanup in `Nudge`; redundant condition | **Defer.** |

+| T10 L132: config active before first-wake settle | **Defer:** health-passing execution can activate config; promotion to good still waits for settle. |

+| T10 L132: missing hold/no-good test | **Flag.** |

+| T10 L164: late event clears newer planned entry | **Merge:** finding 4. |

+| T10 L169: inaccurate moved-twice refusal wording | **Flag:** plan 6 inventory/review. |

+| T11 L135: blocking serving callback | **Defer/implemented mitigation:** agent wiring uses the ordered asynchronous writer; strengthen its blocked-write test before **Flag**. |

+| T11 L135: untested admitting branch of `CoalesceRecycle` | **Flag.** |

+| T11 L135: reused em-dash error message | **Flag:** replace/review the new attempt-facing use. |

+| T11 L135: successful parked one-shot never records good | **Flag:** §6.5 treats its clean exit as success. |

+| T12 L140: bucket retention omits `GoodVersion` | **Flag:** preserve recoverable good artifacts across retention. |

+| T12 L140: outdated adoption comment | **Defer.** |

+| T12 L140: hold publication occurs after stopping incumbent | **Flag:** publish the chosen pin before causing the outage. |

+| T12 L140: missing route makes generation-pin resolution fail box-wide | **Defer:** fail-closed is preferable; follow up with retained self-contained identities. |

+| T12 L140: lifetime-long plan pins; duplicated filtering | **Defer:** these over-retain rather than omit live builds. |

+| T13 L142: briefly stale serving/awake status; missing mid-wake marker | **Flag:** ensure deploy-watch completion cannot race these states. |

+| T13 L142: only cancellation closes observation in tests | **Flag:** cover every terminal event. |

+| T13 L142: observation keyed only by host | **Merge:** finding 4’s token fix. |

+| T13 L142: unknown config error becomes “health” | **Flag:** correct reason classification with plan 6. |

+| T13 L142: shared report pointer | **Defer**, provided published reports remain immutable. |

+| T14 L152: all statuses count as held on box | **Defer:** conservative retention is correct; fix the comment later. |

+| T14 L152: weak slow-serving-write test | **Flag.** |

+| T14 L152: refusing recycle with no captured build uses live link | **Flag:** refuse that spawn. |

+| T14 L152: loose nudge throttle; misleading refusal log | **Defer.** |

+| T14 L152: unmanaged config no-op | **Merge:** finding 7. |

+| T14 L146: populated legacy parent not rejected | **Flag:** implement the missing containment transition guard. |

+| T14 L153: absent `SelectedRestart` | **Defer while claims remain closed; required before enabling on a claims-on box.** |

+| T15 L157: unreadable baseline can retain relay indefinitely | **Flag:** provide a bounded/revalidated exit condition. |

+| T15 L157: moves call-site test | **Flag.** |

+| T15 L157: relay state exists only in memory | **Defer:** normal blocked-head observation can reconstruct it. |

+| T16 L165: self-derived I3 exemptions, unproven secondary checks, early-return holes | **Flag:** strengthen independent liveness assertions. |

+| T16 L165: omitted running-attempt refusal, hidden stale-row/callback interleavings | **Merge:** add deterministic regressions for findings 1–4. |

+| T16 L165: copied timing constants | **Defer.** |

+| T16 L165: unknown mutant silently succeeds; script leaves scratch | **Merge:** fail unknown selections and clean owned scratch on exit. |

+| T16 L165: nondeterministic replay | **Flag:** stabilize event ordering or record enough scheduling information to reproduce failures. |

+The carried obligations resolve as follows:

+| Obligation | Disposition |

+|---|---|

+| O1: usable containment, no populated legacy parent | **Open:** finding 6 before merge; populated-parent guard before flag. |

+| O2: planned/legacy-sidecar staged identity | Implemented for the managed planned paths; retain the documented unmanaged boundary. |

+| O3: claims-backed launches remain refused | Satisfied on this base; do not weaken it during fixes. |

+| O4: wrap pre-body hostguard refusals | Deferred only until the isolated claims launch opens. |

+| O5: abandonment backoff | Implemented; retry allowance still needs finding 8. |

+| O6: failure identity comes from dispatch | Implemented. |

+| O7: held-down admission before reopening | **Open:** cold wakes and observers need finding 3. |

+| O8: member-safe failure reasons | Scheduler events improved; planned-wake passthrough remains a pre-flag obligation. |

+| O9: extend the one generations writer | Implemented. |

+| T3/T4 carries: uncertain demand means hold; missing-policy distinction; failure timestamp; complete prune scope | Implemented, subject to findings 4–5’s retention/durability corrections. |

+| T5/T6 carries: backoff, repeated signed retry, atomic finish/failure, config failure accounting, canary settle | Implemented in the principal scheduler paths; retry-abandonment remains open. |

+| T7 carry: older activation must not reset newer config failures | Implemented in `configTrack`; wake events still need immutable token/revision association. |

+| T8 carries: off-lock callbacks, serialized delivery after file write | Implemented structurally; preserve that lock order while fixing outcome fences. |

+| T11/T13 carries: recycle coalescing, cheap serving callback, departed-host pruning, nonblocking status nudge | Wired; the identified lifecycle/coverage work remains before flag. |

+| T14→T15: relay wiring and shared marker helper | Implemented. |

+Beyond plan 6, enabling `CARLOS_SWAP` anywhere still owes:

+- All merge blockers above, plus the **Flag** items in the triage.

+- The populated-legacy-parent guard and real containment evidence: generation placement, parent limits, termination proof, and crash-surviving children.

+- The explicitly carried **D1 daemon-reload measurement**, real-unit **KillSidecar/control checks**, and proof that unsupported nondefault `CARLOS_OPT_DIR` layouts fail closed. [Ledger:19](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/.superpowers/sdd/2026-10-07-zero-downtime-deploys-5-scheduler/progress.md:19)

+- Crash/restart evidence for failure persistence, execution-bound gate regressions using production backend preparation, and delayed-observer/rewake regressions.

+- Shutdown evidence covering scheduler attempts, first-wake observers, blocked config publication, and serving publication.

+- Final-tip repository gate and supported cross-build evidence after fixes. Earlier task-level passes and the final simulation do not establish that whole-branch gate.

+- Claims-on boxes remain excluded until isolated launch, selected config restart, pre-body classification, and the deferred claimed-wake details are completed.

+The inherited flag-off descendant-proof limitation remains recorded technical debt; it is not evidence that generation containment is safe to enable.

\ No newline at end of file