amadan
zdd-5: Astra's final review, round 3 (ACCEPT WITH FIXES for a dormant merge)
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
1 file changed,
+94
−0
docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5-R3.md+94 −0
CI passed — run details
diff --git a/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5-R3.md b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5-R3.md| new file mode 100644 |
| index 000000000..ecde01af0 |
| --- /dev/null |
| +++ b/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5-R3.md |
| @@ -0,0 +1,94 @@ |
| +ACCEPT WITH FIXES |
| + |
| +For **dormant merge only**: I found no new flag-off runtime defect. Before merging, obtain the complete repository gate on `55de3e4a` and record the remaining defects as enablement blockers. The supplied [wave-2 results](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/.superpowers/sdd/2026-10-07-zero-downtime-deploys-5-scheduler/final-w2-b2-report.md:225) cover selected checks at `1cc74fe5`, not that complete final-tip gate. |
| + |
| +Reviewed `6ae15fdb..55de3e4a`. No files or git state changed; no tests or simulations run. |
| + |
| +**2. R2 items** |
| + |
| +| Item | Status | Evidence | |
| +|---|---|---| |
| +| F1 — staged-instance launch gates | **ADDRESSED** | The hook reaches cgroup preparation’s far side and each launch retry: [containment.go:129](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/containment.go:129), [cgroup.go:354](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/cgroup/cgroup.go:354), [cgroup.go:367](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/cgroup/cgroup.go:367). The production-backend regressions cover preparation and retry supersession at [exec_gate_test.go:419](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/exec_gate_test.go:419). The separately parked **worker** boundary remains an enablement blocker. | |
| +| N1 — outcome fence fails open | **ADDRESSED** | Timeout and cancellation return refusal while retaining the fence: [wakeplan.go:590](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/wakeplan.go:590). `doWake` returns before planning at [activator.go:1713](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/activator.go:1713); the claims path does likewise at [claimwake.go:196](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/claimwake.go:196). | |
| +| N2 — later fallback after executed hold abandonment | **PARTIAL** | Scheduler outcomes now set a marker before releasing admission, and cancelled hold wakes report execution: [scheduler.go:212](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/scheduler.go:212), [wakeplan.go:419](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/wakeplan.go:419). Planned wakes enforce it, but scheduled worker fallback bypasses it, stale publication can erase it, and its admission exception contradicts the failure hold. See N5–N7. | |
| +| N3 — stale config-token check | **ADDRESSED** | Token validation and `activate` now share `m.mu`; acknowledgement I/O remains outside: [wakeplan.go:436](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/wakeplan.go:436), [manager.go:1031](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/manager.go:1031). The delayed-old-event regression is at [config_race_test.go:18](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/config_race_test.go:18). | |
| +| N4 — unknown execution treated as unused retry | **ADDRESSED** | Cancelled replacements join their respawn, reporting unknown on timeout: [workerswap.go:104](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/workerswap.go:104), [workerswap.go:335](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/workerswap.go:335). Nested post-start errors carry uncertainty at [backend_exec.go:1702](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/backend_exec.go:1702); overlap and hold propagate it. Failure classification remains separately parked. | |
| +| F8 — retry re-arming, through N4 | **ADDRESSED** | Re-arming requires abandonment **and proof of no execution** at [scheduler.go:224](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/scheduler.go:224), with build-scoped restoration at [queue.go:455](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/queue.go:455). The older retry-generation/interleaving obligations remain separate. | |
| + |
| +**3. New findings in wave 2** |
| + |
| +All three below are **flag-on, enablement-blocking (b)**. None requires a code fix before a strictly dormant merge. |
| + |
| +**N5 — High: the hold-ran restriction does not cover scheduled worker fallback.** |
| + |
| +The new restriction is installed at [scheduler.go:213](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/scheduler.go:213), but only wake planning consumes it. |
| + |
| +Failing sequence: |
| + |
| +1. Worker H has good A. Hold B executes. |
| +2. C supersedes B during its settle. B remains running, but returns abandoned without becoming good: [workerswap.go:173](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/workerswap.go:173). The scheduler records B’s hold-ran marker. |
| +3. The next scheduled replacement adopts overlap C. [worker.go:24](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/worker.go:24) supplies good A as fallback without consulting that marker. |
| +4. C fails; [workerswap.go:234](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/workerswap.go:234) executes A against the database B may have migrated. |
| + |
| +**Smallest fix:** apply the host recovery restriction to scheduled replacement/fallback selection and its execution gate; suppress A until a later build passes or explicit recovery authorizes it. |
| + |
| +**N6 — High: an older good-build publication can erase a newer hold-ran marker.** |
| + |
| +[manager.go:999](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/manager.go:999) clears whatever marker currently exists. [failures.go:234](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/failures.go:234) checks neither generation nor marker identity. |
| + |
| +Failing sequence: |
| + |
| +1. A’s settled event passes `acknowledged` in [manager.go:898](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/manager.go:898), then pauses before `goodRecorded`. |
| +2. A is already visible to scheduler admission—the observer clears `firstWatch` before delivering `GenSettled`: [wakeplan.go:755](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/wakeplan.go:755). |
| +3. Hold B executes and is superseded; `done` records B’s marker. That path does not acquire `goodMu`. |
| +4. A’s older publication resumes and deletes B’s marker. A subsequent wake can again select good A. |
| + |
| +**Smallest fix:** order marker creation and successful-generation clearing with a per-host execution epoch or equivalent conditional transition. An earlier acknowledged generation must not clear a restriction established by a later execution. |
| + |
| +**N7 — High: a marker becomes permission to bypass an existing cohort failure.** |
| + |
| +[decideHoldRan:132](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/wakeplan.go:132) skips `Blocks` for the marked build; [planCheck:312](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/wakeplan.go:312) repeats that exemption at execution. |
| + |
| +Failing sequence: H executes hold B without passing; a sibling fails B and supersedes H; H receives the marker. An ordinary subsequent wake executes B again despite the standing failure record and without retry authorization. This is explicitly required by the new [regression test:65](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/holdran_seq_test.go:65). |
| + |
| +Binding [spec §8.4:687](/home/paulca/amadan.net/carlos/platform-design-zero-downtime-deploys/docs/superpowers/specs/2026-10-07-zero-downtime-deploys-design.md:687) exempts instances that **passed**, not instances that merely executed. The new [simulation exemption:840](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/sim_test.go:840) conceals this violation. |
| + |
| +**Smallest fix:** retain both restrictions. If B is blocked and A is unsafe, refuse the wake until explicit retry or an admissible newer build; do not reinterpret “cannot fall back” as “may retry a held build.” Correct the test and simulation expectation. |
| + |
| +**4. New controller rulings** |
| + |
| +References are to the new lines in [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:199). |
| + |
| +| Ruling | Judgment | |
| +|---|---| |
| +| 199 — final-wave split and execution-evidence contract | **Agree.** The split is reasonable; false must mean proven unexecuted. | |
| +| 200 — fail-closed outcome timeout | **Agree.** The request expires; the safety restriction does not. | |
| +| 201 — persisted hold-ran marker | **Disagree as written.** Preserving a restriction without inventing a build failure is correct. Letting the marked build bypass a standing failure record is N7. Enforcement and clearing also need N5/N6. | |
| +| 202 — atomic config activation | **Agree.** The implementation now performs the token check and activation together. | |
| +| 205 — installed good build means explicit rollback | **Agree with conditions.** Permit an actual explicit rollback, but installed-build equality alone does not establish its provenance. Root also has automatic repoint-and-stamp machinery at [rollback.go:96](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/release/rollback.go:96). Before enablement, prove those paths cannot authorize recovery of a marked managed host, or require explicit recovery evidence. | |
| +| 206 — park worker launch hook | **Agree with conditions: dormant only.** The hook still precedes cgroup preparation at [backend_exec.go:2137](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/activator/backend_exec.go:2137). Carry the ownership check to every actual worker start before enablement. | |
| +| 206 — park declined-mode marker enforcement | **Agree with conditions: dormant only.** [rolloutdeclined.go:115](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/host/rolloutdeclined.go:115) reads failure records, not hold-ran restrictions. | |
| +| 206 — park downgrade marker loss | **Agree with conditions: dormant only.** Parsing an additive field is insufficient when an older writer deletes its safety meaning. Preserve the restriction or refuse the unsafe downgrade. | |
| +| 206 — park nested accepted-start failure classification | **Agree with conditions: dormant only.** Conservative execution evidence is correct; treating an accepted start that actually failed as abandonment can avoid hold-down/cohort failure and permit repeated starts. Resolve failure versus uncertain outcome before enablement. | |
| + |
| +**5. Consolidated obligations before enabling anywhere** |
| + |
| +This replaces the overlapping lists in [round 1’s Flag triage](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5.md:116) and [round 2’s owed items](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/docs/superpowers/plans/CODEX-FINDINGS-ZDD-IMPL-5-R2.md:68). |
| + |
| +1. **Complete hold recovery protection:** N5–N7; ordered marker creation/clearing; explicit rollback provenance; correct nested post-start failure classification. Add deterministic regressions for these sequences. |
| +2. **Durable failure state, including markers:** F5’s unresolved-attempt/outcome protocol, refusal when its prerequisite write fails, startup reconciliation, crash/restart evidence, and protection against downgrade erasure. [failures.go:128](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/internal/rollout/failures.go:128) still only logs/retains failed saves in memory. |
| +3. **Declined boxes:** enforce records and markers on worker converge/recycle and every actual launch; refresh supported retry authorization after startup. Preserve the no-good bootstrap exception. |
| +4. **Execution admission:** worker ownership/cancellation checks after preparation and before every launch retry; revalidate `Managed` against the route used for dispatch; refuse uncaptured worker recycling through the mutable live link. |
| +5. **Retry ordering:** prevent delayed pre-retry failures from undoing newer authorization; establish the older non-canary/retried-canary boundary; verify unrelated attempts cannot consume the matching retry allowance. |
| +6. **Policy/store bounds:** serialize or version policy-cache fills; bound failed-read retries; cover missing-versus-error policy reads; bound synchronous policy/pointer reads and use the wake’s context for wake-time pointer work. |
| +7. **Config/publication lifecycle:** complete delivery-order, address-route, removal and real-host wiring coverage; bound/cancel acknowledgement writes; strengthen blocked serving-publication tests; verify shutdown joins attempts, observers and blocked publishers. |
| +8. **Workers and scheduler coverage:** align `WakeWorker` and fallback budgets; verify readiness, fallback events, clean one-shot success recording and admitting `CoalesceRecycle`; cover hold/no-good wakes, running-attempt refusal, config backoff/deferral, and concurrency-one/two-cohort operation. |
| +9. **Artifact protection:** include `GoodVersion` in bucket retention; publish the selected hold build before stopping its incumbent. |
| +10. **Truthful status/events:** close stale serving/awake and mid-wake completion gaps; test every observation terminal event; emit cohort halt when failure precedes scheduler registration. |
| +11. **Relay:** bound/revalidate unreadable-baseline retention and cover the moves call site. |
| +12. **Simulation evidence:** independently assert safety and liveness, including worker fallback and delayed callbacks; remove N7’s exemption; make replay reproducible; rerun the corrected high-seed/mutation checks and verify owned-scratch cleanup. |
| +13. **Containment evidence:** populated-legacy-parent refusal, generation placement/limits, termination proof and crash-surviving descendants; the carried D1 measurement, real-unit `KillSidecar`/control checks, and nondefault-path refusal at [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). |
| +14. **Plan 6:** member-safe planned-wake errors, robust reason classification, reviewed recovery/refusal wording—including hold-ran wording—and migration guidance. |
| +15. **Claims exclusion:** keep claims-backed launches closed. Opening them additionally requires isolated launch, selected config restart/delivery coverage, pre-body ownership-error classification, and complete claimed-wake execution/outcome evidence. |
| + |
| +The **merge-time obligation (a)** is the complete final-tip build/vet/test/gofmt/cross-build gate, including resolution or identification of the unexplained activator failure noted at [progress.md:207](/home/paulca/amadan.net/carlos/platform/.claude/worktrees/zdd-5-scheduler/.superpowers/sdd/2026-10-07-zero-downtime-deploys-5-scheduler/progress.md:207). The runtime obligations above are **enablement blockers (b)**. |
| \ No newline at end of file |