| 1 | REJECT |
| 2 | |
| 3 | Read-only review of `85ffab85`. No files changed or tests run. |
| 4 | |
| 5 | 1. **High — Task 1: specify generation changes when restriction components are replaced or removed.** |
| 6 | The proposed marker-clear fix does not explicitly cover replacing B’s marker with C’s, or deleting B’s record while B’s marker survives. `markLocked` overwrites the marker at [failures.go:735](internal/rollout/failures.go:735); `ClearOnPointer` and `Prune` remove records at [failures.go:988](internal/rollout/failures.go:988) and [failures.go:1031](internal/rollout/failures.go:1031). Because the generation is a maximum of surviving components ([failures.go:923](internal/rollout/failures.go:923)), either operation can expose an earlier generation again. |
| 7 | |
| 8 | **Plan fix:** explicitly require advancing the outgoing build’s surviving restriction on marker replacement, and every surviving marker’s generation on record removal. Add stale-grant regressions for replacement, pointer clearing and pruning. The general “every change” rule needs these concrete cases before its proof is implementable. |
| 9 | |
| 10 | 2. **High — Task 1: the proof assumes older grants were issued under the new rule.** |
| 11 | Current status publishes epochs without checking persistence ([status.go:89](internal/rollout/status.go:89)); failed saves retain changed memory ([failures.go:551](internal/rollout/failures.go:551)). Removing `reopenAdvance` while retaining the existing book identity allows an upgraded run eventually to reuse an epoch an earlier binary published unsaved. Moving the transport object does not invalidate a signed envelope copied into the new log. |
| 12 | |
| 13 | **Plan fix:** introduce a persisted migration marker and durably rotate the book identity when adopting the new discipline, invalidating earlier grants. Test upgrading a book with an outstanding, previously published unsaved epoch. State that removing `reopenAdvance` is safe only after this transition. |
| 14 | |
| 15 | 3. **High — Task 8: retaining the failed seed’s database does not cause reseeding after one TTL.** |
| 16 | `Restore` treats any existing database as a warm cache ([backend_exec.go:713](internal/activator/backend_exec.go:713)); `doWake` seeds only when `fresh` ([activator.go:1969](internal/activator/activator.go:1969)). Thus the proposed sequence is: failed seed, unproven cleanup, retained partial database, lease expiry, next wake skips seeding and starts on partial data. The current nested path having the same behavior does not validate the ruling. |
| 17 | |
| 18 | **Plan fix:** retain a failed-seed cleanup obligation and prevent normal startup until termination is proven and the seed-owned partial database is removed. Make that obligation survive restart. Test the next wake, not only the first error. |
| 19 | |
| 20 | 4. **High — Task 7: case G’s leader kill cannot establish containment.** |
| 21 | PID-move fallback starts the executable before moving its PID ([cgroup.go:509](internal/cgroup/cgroup.go:509)); descendants forked beforehand remain outside the child cgroup, explicitly documented at [cgroup.go:352](internal/cgroup/cgroup.go:352). Killing and reaping the leader after a failed move leaves those descendants alive. `markStartUncertain` records execution uncertainty; it does not supply a termination proof. `KillGen` subsequently reaches only the generation child ([containment.go:301](internal/activator/containment.go:301)). |
| 22 | |
| 23 | **Plan fix:** refuse PID-move fallback before execution when generation containment is required, or specify a launch mechanism that contains descendants before executable code runs. Add a forked-descendant regression. I disagree with the case-G ruling as sufficient closure. |
| 24 | |
| 25 | 5. **High — Task 8: normal-stop proof still races a sidecar start.** |
| 26 | With generations off, `stop` stops waiting for an in-flight sidecar start after a timeout ([activator.go:3474](internal/activator/activator.go:3474)). That start runs asynchronously ([activator.go:4233](internal/activator/activator.go:4233)). It can therefore start after `EmptyHost` reports empty and after the lease is released. Recon B acknowledges this gap, but Task 8 still claims normal-stop containment is closed. |
| 27 | |
| 28 | **Plan fix:** include outstanding sidecar starts and their discarded children in the terminal-stop proof. Keep the stop pending until they finish; test a start deliberately delayed beyond the existing timeout. |
| 29 | |
| 30 | 6. **High — Task 8: successful seeds need proof even without `ErrWaitDelay`.** |
| 31 | A seed can fork a helper that closes inherited output descriptors, then exit zero. `Wait` returns nil, and the current success path merely removes the child directory and returns success ([backend_exec.go:978](internal/activator/backend_exec.go:978), [backend_exec.go:1014](internal/activator/backend_exec.go:1014)). The plan’s special treatment of the lingering-output-pipe branch misses that helper. Generation cleanup does not reach `role-seed` ([containment.go:301](internal/activator/containment.go:301)). |
| 32 | |
| 33 | **Plan fix:** require seed-child emptiness on every successful flag-on seed completion, including ordinary nil-error completion. Add a helper that redirects its output before the seed exits. |
| 34 | |
| 35 | 7. **High — Task 2: I9’s ghost definition is insufficiently precise.** |
| 36 | A record-wide epoch change affects every restricted host, not just the failing host ([failures.go:697](internal/rollout/failures.go:697), [failures.go:927](internal/rollout/failures.go:927)). The proposed per-failing-host ghost can miss that invalidation. Conversely, counting incoming failure notifications indiscriminately rejects legitimate ignored outcomes: `PreRetry`, unchanged records and duplicate execution markers are intentional ([failures.go:655](internal/rollout/failures.go:655), [failures.go:703](internal/rollout/failures.go:703), [failures.go:722](internal/rollout/failures.go:722)). |
| 37 | |
| 38 | **Plan fix:** define independent model transitions for record-wide effects, host effects and marker identity, including which notifications change nothing. Track component removals too if I9 is meant to prove exact-generation binding. Preserve ghosts across restart and book recreation. Do not derive expected transitions from the mutated book’s verdicts. |
| 39 | |
| 40 | 8. **Medium — Tasks 1–2: the proposed tests and M15 need corrected witnesses.** |
| 41 | Merely recording another failing host does not necessarily change a standing record; the merge requires a spent retry, added hold-down or newer promotion ([failures.go:674](internal/rollout/failures.go:674)). Task 1’s first test must specify one. Its stale-grant test starts with an already-saved epoch and advances beyond it, so removing the dirty check does not establish the promised failure. |
| 42 | |
| 43 | M15’s halved sequence is also repaired whenever surviving markers or record epochs contain the disk maximum ([failures.go:374](internal/rollout/failures.go:374)). |
| 44 | |
| 45 | **Plan fix:** give exact event sequences. For M15, remove the high-valued components before reopening, then force numerical reuse. For M13, assert the dirty publication against the last saved file. Give M14 an explicit marker-create/clear/replay witness; retain the proposed dedicated-test fallback for M16. |
| 46 | |
| 47 | 9. **Medium — Task 3: “every mixed-version pairing” still silently drops retries.** |
| 48 | The current console writes `restart-scope.json` and returns “requested” ([apiretry.go:94](internal/console/apiretry.go:94), [apiretry.go:161](internal/console/apiretry.go:161)). A new box following Task 3 discards that entry. The new console’s capability check cannot fix an old console’s response. |
| 49 | |
| 50 | Copying seed-don’t-replay also discards an unprocessed grant when the agent restarts before delivery: first sight advances the cache without applying requests ([restartscope.go:113](internal/host/restartscope.go:113)). |
| 51 | |
| 52 | **Plan fix:** specify the supported transition and handling of pending grants, then test old-console/new-box and request-before-agent-restart sequences. Either provide checked delivery/acknowledgement or explicitly narrow the guarantee; the current “never silently dropped” claim is false. |
| 53 | |
| 54 | 10. **Medium — Tasks 1, 3–4: declined books need an independent save-retry path.** |
| 55 | Declined startup flushes only when intents were reconciled ([rolloutdeclined.go:87](internal/host/rolloutdeclined.go:87)). Opening can also dirty a book through identity or epoch migration ([failures.go:371](internal/rollout/failures.go:371), [failures.go:401](internal/rollout/failures.go:401)). Pointer processing is conditional and may perform no mutation ([manager.go:657](internal/rollout/manager.go:657), [manager.go:698](internal/rollout/manager.go:698)). Once publication returns zero, the console refuses the retry that otherwise could have triggered a save. |
| 56 | |
| 57 | **Plan fix:** flush dirty declined books at startup and retry outstanding saves periodically, independently of applicable pointers or grants. Test disk recovery with no matching pointer operation. |
| 58 | |
| 59 | 11. **Medium — Tasks 1–2: mutation runs inspect the previous commit.** |
| 60 | `mutate.sh` constructs its test tree with `git archive HEAD` ([mutate.sh:143](internal/rollout/testdata/rolloutsim/mutate.sh:143)). Both tasks run it before committing their changes. Task 1 therefore tests the old implementation; Task 2’s new markers/tests are absent from the archived tree. |
| 61 | |
| 62 | **Plan fix:** create a local checkpoint commit before each mutation run, or explicitly change the runner to snapshot the intended working-tree revision. Record the tested revision. |
| 63 | |
| 64 | 12. **Low — Tasks 3–4 and verification instructions: correct the factual mismatches.** |
| 65 | `retryHostBuild` reads the restriction; `requestHostRetry` performs the append ([apiretry.go:59](internal/console/apiretry.go:59), [apiretry.go:86](internal/console/apiretry.go:86)). Name the latter in Task 3. The template’s dot is `instanceDetail`, so specify an `instanceDetail.CanRetry` forwarding method or use `.Rollout.CanRetry` ([instancepage.go:17](internal/console/instancepage.go:17), [app_instance.html:28](internal/console/templates/app_instance.html:28)). The mutation invariant regex already accepts arbitrary digits ([mutate.sh:180](internal/rollout/testdata/rolloutsim/mutate.sh:180)); it is not limited to I1–I8. Finally, replace the plan’s per-task cache and `/tmp/k` instructions with the supplied shared-machine rules. |
| 66 | |
| 67 | I agree with claims-on declining, the separate amendment file, real-box checks remaining out of scope, and Task 6’s clean-park approach. I agree conditionally with separate retry transport, declined-sidecar recovery and authorized normal-stop proof. The failed-seed recovery and case-G rulings need the fixes above. The remaining code blockers are principally incomplete closure of existing epoch, retry-delivery and containment obligations—not claims-on swaps. No additional lock-order inversion or direct disk I/O under `Service.mu` was established in the proposed changes. |
amadan