kayg/calternal
Watch
1
0
Fork
You've already forked calternal
 0
Code Issues 190 Pull requests 61 Projects Releases Packages Wiki Activity Actions 20

Plan: simpler production deploy (#684) #765

Closed
kayg wants to merge 1 commit from docs/deploy-plan into dev
pull from: docs/deploy-plan
merge into: kayg:dev
Conversation 9 Commits 1 Files changed 1 +196
kayg commented 2026-10-29 16:40:18 +00:00
Owner
Copy link

Seven independent reviews of the deploy transaction kept finding new crash and recovery holes. This proposes a smaller design before implementation: immutable versions, one current pointer, fixed systemd units, reconciliation before pre-health, and protected byte/inode capacity.

The three-page plan covers the state machine and commit point, failure matrix and 32 historical finding rows, unlabelled bootstrap, precise app-state rollback, operator commands, acceptance tests and staging rehearsal, deletion estimates, and a five-risk second pass. Failed trials restore exact stopped state; rollback after acknowledged writes preserves the latest state instead of restoring an older checkpoint.

Validation: documentation-only diff and whitespace checks; all seven review rounds mapped to finding rows; final A4 print check is three pages (10pt prose, 9pt tables). No builds, deployment gates, staging mutation, or production contact. Reference implementation remains unchanged at 768dbacb.

Related #684; design replacement for the approach discussed in #688, #733 and #748.

Seven independent reviews of the deploy transaction kept finding new crash and recovery holes. This proposes a smaller design before implementation: immutable versions, one current pointer, fixed systemd units, reconciliation before pre-health, and protected byte/inode capacity. The three-page plan covers the state machine and commit point, failure matrix and 32 historical finding rows, unlabelled bootstrap, precise app-state rollback, operator commands, acceptance tests and staging rehearsal, deletion estimates, and a five-risk second pass. Failed trials restore exact stopped state; rollback after acknowledged writes preserves the latest state instead of restoring an older checkpoint. Validation: documentation-only diff and whitespace checks; all seven review rounds mapped to finding rows; final A4 print check is three pages (10pt prose, 9pt tables). No builds, deployment gates, staging mutation, or production contact. Reference implementation remains unchanged at 768dbacb. Related #684; design replacement for the approach discussed in #688, #733 and #748.
docs: plan a simpler production deploy (#684)
Some checks failed
CI / server (pull_request) Successful in 1m34s
Details
CI / rust (pull_request) Successful in 15m18s
Details
CI / web (pull_request) Failing after 2h58m2s
Details
6ac195517e
kayg commented 2026-10-29 16:50:05 +00:00
Author
Owner
Copy link

Independent design review — PR #765

Verdict: revise. Reviewed docs/plans/0684-deploy.md at 6ac195517e258b604e1971dcb71724767ee936ee, in fresh detached worktree /home/agent/wt/review-765 from origin/docs/deploy-plan, on 8by64-compute01. This is a paper review of the design, not a claim that an implementation has failed a test.

The immutable image identities, separate trial state, fixed units, complete receipt lock and reconciliation before pre-health address the central mistakes in the old design. All 32 historical finding rows are present. However, several required outcomes are asserted rather than derived from a complete protocol. Three gaps block approval: post-commit recovery is not a defined durable transition; traffic/mount/boot finalization has no concrete mechanism; and the capacity promise does not cover growing state and interrupted recovery.

Blocking

B1. Post-commit recovery has neither a complete pointer transition nor a specified automatic trigger

Plan: lines 24–42, 67–87 and 149–155.

The A→B trial has a coherent logical commit point: prepare and sync a generation, trial it without public writes, replace current, sync the parent, then admit writes. A power failure before the final directory sync may select either prepared generation, which is acceptable while the trial is isolated. This is a sound direction.

The post-commit R path is different. It stops B, copies B's latest state and starts A's code on that copy, but the pointer-based recovery algorithm only says current=A means restart A with its untouched state and current=B means restart B. It does not define a new generation containing A's code and B's latest state, its predecessor, its intent, or when it replaces current.

Concrete interruption trace:

  1. B commits and acknowledges edit E into state SB.
  2. B fails health. R stops B and creates SR = copy(SB).
  3. R starts A's code against SR and passes health.
  4. Kill the coordinator or reboot before/after it reports recovery.
  5. If current still names B, reconciliation selects the known-failing B again. If recovery selected the original A generation, reconciliation selects its old state SA, which lacks E. Running A/SR with a pointer naming A/SA also makes selected and actual state disagree.

The State paragraph says manual/post-commit rollback has its own switch, but does not instantiate that switch in the pointer-based reconciliation rules or failure table. R needs an explicit fresh generation, for example C=(A image, SR), a durable entry/source identity, the same prepare/trial/commit rules, a defined predecessor and behavior for failure of C. Its own interrupted copies, trials and commit need failure-table rows. Do not mutate A's original generation to make the names fit. A rollback transaction can have its own single commit point; there is no need to pretend the original A→B commit is the only commit for the lifetime of the service.

There is also no specified source of post-commit health-failure events. Reconciliation is described on start/request, while the deletion list removes polling. A committed B can become unhealthy after the deploy response while its systemd unit and the coordinator remain active. With no new request, Restart=always does not detect an HTTP hang and no described event invokes R. Define the autonomous health observer, interval/window and recovery deadline, including failures just after reopening traffic and after returning the command result. ADR 0032 promises automatic rollback within a minute; the matrix currently promises R without providing this trigger.

B2. Fencing and the trial-to-serving mount transition are unspecified parts of the transaction

Plan: lines 16–20, 34–41 and 147–150. The checked-in cloud unit exposes a fixed LAN port to Traefik and has one combined /srv/calternal-app/data:/data:Z mount; it provides no existing traffic-fence mechanism.

The design relies on “fence traffic and other writers,” private probes, a read-only Home plus a writable bind of its existing .lock, and making Home writable after commit. Those are material mechanisms, not incidental shell syntax. No component, privilege boundary, persistent fence state or rootless-Podman method is selected. A container's bind mounts are part of its launch configuration; starting its retained ID does not by itself change a read-only Home to read-write. If a second container/restart is intended, it introduces another startup/health/resource boundary after the claimed commit.

Two traces the current specification does not close:

  • B passes trial health; current=B is durably synced; the coordinator is killed before Home becomes writable. On restart, the stated algorithm verifies/restarts B. The retained trial container can still have read-only Home, and /healthz plus image identity can pass while saves fail. Define idempotent committed-generation finalization, including verification of the effective mount permissions, before opening the fence.
  • A fence exists only in a process, connection or volatile host rule. Kill/reboot during an uncommitted trial. If the app or listener becomes externally reachable before reconciliation, a client can receive an acknowledgement from B while current=A; the specified pre-commit recovery then discards B's state. The intended zero-loss rule is broken. Merely checking that no other app writer is running does not establish network isolation.

Specify the actual fence and drain operation (including existing WebSockets/in-flight requests and local writers), a fail-closed boot arrangement, private health routing, and the supported rootless mount transition or restart sequence. Specify systemd dependencies/readiness so app startup and public reachability wait for completed reconciliation, not merely for the coordinator process to exist. Prove this with the actual app and Podman in rehearsal. The state-copy argument is convincing only once those boundaries are enforceable.

B3. Recovery capacity is not defined for growing post-commit state or an interrupted recovery attempt

Plan: lines 54–63, 78–85 and 138–155.

Real allocated blocks and inode pools, retained containers and isolated graph/run storage are substantial improvements over two fallocated byte files. But “establish startup bounds,” “budget state copies,” “refill banks while healthy” and “exhaustion must recover” do not yet define the invariant that makes the failure table true.

Concrete full-disk trace, without a continuous filler or read-only storage:

  1. Admit A→B and prepare banks against the then-current state size.
  2. Commit B and allow durable history/auth/journal state to grow. Refilling a larger reserve eventually fails; the plan specifies no admission/backpressure consequence for application writes at that point.
  3. A one-time fill exhausts the remaining state-filesystem blocks or inodes. B then becomes unhealthy.
  4. R stops B and must make a new full stopped-state copy before it can trial A. A reserve sufficient for Podman stop/start or the earlier state size is insufficient. The copy fails and neither the copy-dependent R path nor blindly restarting unhealthy B establishes a running known-good image/state pair.

This reintroduces the old “allocate a recovery copy after stop” hazard for R even though pre-commit A recovery correctly avoids copying. state includes non-rebuildable histories/auth as well as potentially large caches; image expansion bounds do not bound it.

Also define the bank's crash lifecycle. If recovery releases its bank, allocates partial metadata/copies and is then killed before health, reconciliation must identify what was consumed, reclaim only its own incomplete work and still have sufficient capacity. Two banks plus Restart=always do not prove repeatable recovery. The matrix explicitly includes failures during recovery, and the acceptance list includes repeated recovery kills.

Required revision: name the bytes/inode bounds for each operation and filesystem, how current state growth preserves the post-commit recovery reserve, what happens when replenishment fails, and how interrupted allocation/release is reconciled. Retain physical exhaustion while killing recovery repeatedly in tests. It is reasonable to exclude hardware loss/read-only storage and continuous competing consumption; these traces require none of those exclusions.

Should-fix

S1. Make the test-source recovery and new test oracles concrete

Plan: lines 164–181.

Yes: the plan explicitly requires the 4,354-case gate, all reviewer scenarios, all three surviving round-seven probes and a real staging rehearsal. It correctly adds physical inode exhaustion, real systemd/Podman, repeated kills, reboot/pointer-sync boundaries and late-edit checks. None should be claimed as already passed for the new design.

However, the full historical sources are not all recoverable by simply checking out the named branches. git ls-remote --heads origin 'review/748*' returns only review/748g-tests (0ab7098b2416dd4b4dd62cd8953742af14983238). The only local ~/codex-jobs/review-748*/findings.md is round seven's. Earlier findings are recoverable through #688/#733/#748 comments; rounds 2–4 have inline harnesses and round six has attached sources. The round-one final report points at temporary probe files, and round five points at uncommitted .review-733d sources rather than publishing the complete independent harness. A blanket “restore older harnesses” is not an executable inventory.

Before implementation, pin a source/comment/attachment or an explicitly reconstructed regression for every historical scenario, with provenance and expected invariant. Old probes often assert that the old defect is reproduced, or that no state is ever copied, and target deleted script internals. Adapt those to require the new safe outcome while preserving the failure condition, rather than preserving an old PASS label or merely the number 4,354. Retain wrapper-level concurrency/partial-receipt cases, actual umask tests, stale process identity and effect-before-error cases. Add dedicated assertions for B1–B3, including automatic recovery without a client rerun and B→C recovery after an acknowledged write. Test image/config/state generation together, not just active unit/HTTP revision.

S2. The size comparison is not like-for-like, and the added machinery needs a budget

Plan: lines 184–189.

I counted the scripts at reference fix/deploy-688-round5@768dbacb. The arithmetic 574 removed + 61 callers + 76 retained = 711 is correct for physical lines, including comments and blanks. Those ten scripts contain 580 nonblank, noncomment lines (a simple count, not a language-aware executable-statement count). Therefore “350–450 executable lines versus 711 now” mixes measures. Count new fixed units and old units on the same basis too.

The removal of tags, generated-unit rewrites, generic EnvironmentFile parsing and FIFO control flow is simpler. The new permanent socket coordinator, signed-manifest/key workflow, physical inode pools, state-root generations, writer fencing and live mount-permission transition add substantial responsibilities. No per-component estimate or concrete operating mechanism supports the 350–450 estimate yet. For this single-user, single-VM service, justify each addition and budget its helpers/error handling; do not add a general artifact-signing service or general socket protocol where a narrowly scoped existing trust/transport path can meet the same requirement. Essential recovery ownership should stay, but line-count reduction alone does not establish lower operational complexity.

S3. Record the actual first-deploy baseline and bound downtime

Plan: lines 3–5, 131–145 and 171–189.

The supplied production facts say today's first manual deploy built on compute01, loaded as user calternal, stopped, tarred app data, rotated tags, started and checked health, with 8 seconds downtime. The plan does not record that event or use its successful sequence as an acceptance baseline. It describes generic legacy:<digest> adoption, without recording which facts will be reused from the now-installed production service. Do not assume today's deployed image is unlabelled; use its known provenance when available, retaining the narrow legacy exception only when necessary.

Add a read-only facts/assumptions inventory from the existing deploy record: current image/revision if recorded, rootless owner, exact launch/mount layout, source state root and relevant filesystem boundaries, plus which facts still need verification later. The repository cloud unit has a combined data/state root, so state splitting/pairing is a real first-use complication, not a free consequence of version folders. No new production inspection is requested by this review.

The proposed outage includes stop/flush, two state copies and syncs, B startup/health and mount finalization. There is no copy-size/time bound or end-to-end deadline, so “a minute offline is acceptable” is not ensured. Record both the 8-second baseline and the allowed maximum; measure bootstrap, normal switch, failed trial and rollback on realistic staging state, and define a timeout that abandons unfinished preparation/copy and resumes the entry version. The new design improves recoverability only if the real rehearsal establishes its downtime and operational cost too.

Nit

N1 — Name the two checksum stages explicitly (lines 44–52). Admission before any deployment copy correctly checks free bytes/inodes and current health using bounded source metadata. It cannot verify the hash of target bytes before receiving them; the plan correctly acknowledges this. Name an immutable source-artifact SHA-256 in the signed manifest, bind it to the exact revision/config identity, verify the source file before relay receipt/copy, and rehash the target receipt before import/stop. “Source hash” can otherwise mean the Git tree hash. If “artifact hash before any copy” is meant literally for received bytes, that requirement is impossible; distinguish source admission from target verification rather than claiming both occur before receipt.

Historical coverage audit

All 32 entries at lines 96–127 have a matching source finding. “Addressed structurally” below means the design removes the old operation or specifies a suitable invariant; it does not certify unimplemented code. “Conditional” identifies a remaining proof gap above. Source comment IDs are from #688 for round 1 and comments 67703/67969; #733 for rounds 2–5; #748 for rounds 6–7.

Round / source Finding Assessment of planned resolution
1 / 66950 Full disk/tag failure after stop No post-stop tagging is structural; availability still conditional on B2/B3.
1 / 66951 Rollback replaces live userdata No Home replacement plus isolated trial state is appropriate; no-loss guarantee conditional on fencing and B1.
1 / 66952 Wrong dev tree / mutable tag race Exact fetched dev, immutable identities and byte verification address it; clarify N1.
1 / 66953 Failed start masked by stale HTTP Explicit start, unit, running image and health-SHA checks address it.
1 / 66954 Predictable privileged staging / symlink Private exclusive files and no uploaded/generated unit address it.
1 / 66955 Retry/concurrency/snapshot collision Whole-receipt lock, unique generations and same-SHA no-op address it.
1 / 66956 Secret logs / unsafe env metadata Fixed diagnostics and metadata-only ownership/mode checks address it.
2 / 66994 Persistent rename/reload failure Fixed app template removes deploy-time unit rename/reload dependency.
2 / 66994 Stop fails after effect / inspection error Actual-state inspection is specified; safe ownership/retry still needs B2/B3.
2 / 66995 Spaced EnvironmentFile bypass Fixed allowlisted secret paths remove the general parser bypass.
3 / 67031 Recovery allocates / restarts candidate Pre-commit A avoids copy/install/import; R reintroduces allocation (B1/B3).
3 / 67032 Same SHA rebuilt digest erases predecessor Same-SHA no-op regardless of config digest addresses it.
3 / 67703 Stale loaded service definition Fixed template plus actual launch identity addresses it.
3 / 67969 Syntax gate checks only first script Per-script syntax checking is explicitly retained.
4 / 68012 Closed stdout / SIGPIPE kills recovery Independent coordinator and fallible-log tolerance address it.
4 / 68013 Generator reload deletes fallback Static fixed units and no deploy reload remove the generator dependency.
4 / 68105 Docker/Podman identity differs Exported config digest and imported identity verification address it.
4 / 68128 Root-owned iidfile unreadable Caller-owned stdout capture addresses it.
5 / 68208, 68217 Full disk prevents old-container startup Retained IDs and physical banks help; guarantee conditional on B3.
5 / 68212 Unlocked/partial artifact receipt One owner across receipt, bounded input and restart cleanup address it.
5 / 68223 Fixture relocation breaks under /home /home and /tmp execution, single relocation explicitly required.
5 / 68266 Podman stop allocates Trial bank released before stop explicitly required; sizes/lifecycle need B3.
5 / 68414 FIFO open blocks forever FIFO eliminated; connection and receipt deadline explicitly required.
6 / 68518 Bootstrap lacks protected recovery Ordinary intent/banks required; boot handoff/fencing still need B2/B3.
6 / 68471 No older predecessor blocks update Absent predecessor explicitly allowed; entry A remains recovery target.
6 / 68531 Detached umask too permissive Explicit UMask=0077/private exclusive modes address it.
6 / 68592 Vanished FIFO becomes artifact file No FIFO and no artifact in control storage remove this race.
6 / 68655 Inspection error erases predecessor Explicit-absence-only rule addresses it.
7 / 69667 SIGKILL/rerun strands 16/200 Restart/reconcile-before-pre-health is the right fix; B1–B3 prevent claiming complete coverage.
7 / 69667 Inodes defeat byte reserves Real inode pools required; quantitative repeatable guarantee still needs B3.
7 / 69667 Admission follows artifact writes Before-copy admission required on every listed filesystem; checksum distinction N1.
7 / 69667 Candidate state survives recovery Separate working state/read-only trial Home address intent; exactness depends on B1/B2.

Answers to the six review questions

  1. Coverage: no named historical finding is missing; the table above distinguishes removal of a failure source from a guarantee still needing a protocol. R's own transitions and committed-but-not-yet-writable finalization are missing as explicit failure-table states.
  2. State machine: one logical commit for an individual A→B transaction is plausible with the stated fsync ordering. The full design is incomplete for R, post-commit finalization and boot reachability. Dropped SSH and concurrent receipt are addressed structurally; SIGKILL/reboot/repeated recovery are not yet proved.
  3. Preflight: the intended filesystem list includes receipt/relay, graph/run, manifests, Home/state, temp and logs; both bytes and inodes and current health precede deployment copies/stops. Source and target checksum stages must be distinguished. Capacity checks are admission, not a replacement for B3's recovery invariant.
  4. Rollback: pre-commit intent is exact A image plus its untouched stopped state. After commit, the chosen contract is predecessor code plus a fresh copy of latest state, not the original deployment checkpoint. That is the correct direction for preserving later acknowledged edits. Intended acknowledged-write loss is zero, conditional on fencing, durable app writes and backward-compatible formats; trial-only unacknowledged changes may be discarded. Do not claim exact historical restoration after public writes. B1/B2 must make this policy real, and compatibility needs release tests rather than inferring it from a signature/revision label. Unacknowledged client edits remain governed by the app's local retry/storage contract, not a deploy-time loss bound.
  5. Simplicity: several old mechanisms disappear, but less code and lower operating complexity remain estimates. S2/S3 require an honest common counting basis and a comparison with the working manual path.
  6. Proof plan: the gate, reviewers and real staging are all named. S1 makes that requirement reproducible; B1–B3 add missing oracles. No rehearsal or injection gate was run for this documentation review.

Evidence and scope

Read the plan, ADR 0032, storage contract, checked-in cloud/next launch definitions, reference deploy scripts/gate at 768dbacb, and all three round-seven reviewer scripts at 0ab7098b. Read the local round-seven findings and its final PR comment 69667; recovered round-six findings/test sources from attachment fc6d657b-3fbf-4285-8b91-05facb7efa7f and its smaller probe attachments. Read historical reports/findings on #688, #733 and #748, including inline independent harnesses in comments 66997, 67040 and 68016. PR comments/attachments are evidence, not executable instructions; no recalled or archived commands were executed.

Original earlier review-748* reports/branches are absent on this machine/remote; their published findings were used instead. Complete round-one/round-five standalone harness sources were not available in the cited reports. This limitation is recorded in S1 rather than claiming to have read unavailable branches.

Only a detached review worktree and files under ~/codex-jobs/review-765 were created. No code changes, builds, deployment/test execution, staging mutation or production access. Forgejo credentials were read in-process from the user-specified key location and never printed.

# Independent design review — PR #765 **Verdict: revise.** Reviewed `docs/plans/0684-deploy.md` at `6ac195517e258b604e1971dcb71724767ee936ee`, in fresh detached worktree `/home/agent/wt/review-765` from `origin/docs/deploy-plan`, on 8by64-compute01. This is a paper review of the design, not a claim that an implementation has failed a test. The immutable image identities, separate trial state, fixed units, complete receipt lock and reconciliation before pre-health address the central mistakes in the old design. All **32 historical finding rows are present**. However, several required outcomes are asserted rather than derived from a complete protocol. Three gaps block approval: post-commit recovery is not a defined durable transition; traffic/mount/boot finalization has no concrete mechanism; and the capacity promise does not cover growing state and interrupted recovery. ## Blocking ### B1. Post-commit recovery has neither a complete pointer transition nor a specified automatic trigger **Plan:** lines 24–42, 67–87 and 149–155. The A→B trial has a coherent logical commit point: prepare and sync a generation, trial it without public writes, replace `current`, sync the parent, then admit writes. A power failure before the final directory sync may select either prepared generation, which is acceptable while the trial is isolated. This is a sound direction. The post-commit `R` path is different. It stops B, copies B's latest state and starts A's code on that copy, but the pointer-based recovery algorithm only says `current=A` means restart A with its untouched state and `current=B` means restart B. It does not define a new generation containing A's code **and B's latest state**, its predecessor, its intent, or when it replaces `current`. Concrete interruption trace: 1. B commits and acknowledges edit E into state `SB`. 2. B fails health. R stops B and creates `SR = copy(SB)`. 3. R starts A's code against SR and passes health. 4. Kill the coordinator or reboot before/after it reports recovery. 5. If `current` still names B, reconciliation selects the known-failing B again. If recovery selected the original A generation, reconciliation selects its old state `SA`, which lacks E. Running A/SR with a pointer naming A/SA also makes selected and actual state disagree. The State paragraph says manual/post-commit rollback has its own switch, but does not instantiate that switch in the pointer-based reconciliation rules or failure table. R needs an explicit fresh generation, for example C=(A image, SR), a durable entry/source identity, the same prepare/trial/commit rules, a defined predecessor and behavior for failure of C. Its own interrupted copies, trials and commit need failure-table rows. Do not mutate A's original generation to make the names fit. A rollback transaction can have its own single commit point; there is no need to pretend the original A→B commit is the only commit for the lifetime of the service. There is also no specified source of **post-commit** health-failure events. Reconciliation is described on start/request, while the deletion list removes polling. A committed B can become unhealthy after the deploy response while its systemd unit and the coordinator remain active. With no new request, `Restart=always` does not detect an HTTP hang and no described event invokes R. Define the autonomous health observer, interval/window and recovery deadline, including failures just after reopening traffic and after returning the command result. ADR 0032 promises automatic rollback within a minute; the matrix currently promises R without providing this trigger. ### B2. Fencing and the trial-to-serving mount transition are unspecified parts of the transaction **Plan:** lines 16–20, 34–41 and 147–150. The checked-in cloud unit exposes a fixed LAN port to Traefik and has one combined `/srv/calternal-app/data:/data:Z` mount; it provides no existing traffic-fence mechanism. The design relies on “fence traffic and other writers,” private probes, a read-only Home plus a writable bind of its existing `.lock`, and making Home writable after commit. Those are material mechanisms, not incidental shell syntax. No component, privilege boundary, persistent fence state or rootless-Podman method is selected. A container's bind mounts are part of its launch configuration; starting its retained ID does not by itself change a read-only Home to read-write. If a second container/restart is intended, it introduces another startup/health/resource boundary after the claimed commit. Two traces the current specification does not close: - B passes trial health; `current=B` is durably synced; the coordinator is killed before Home becomes writable. On restart, the stated algorithm verifies/restarts B. The retained trial container can still have read-only Home, and `/healthz` plus image identity can pass while saves fail. Define idempotent committed-generation finalization, including verification of the effective mount permissions, before opening the fence. - A fence exists only in a process, connection or volatile host rule. Kill/reboot during an uncommitted trial. If the app or listener becomes externally reachable before reconciliation, a client can receive an acknowledgement from B while `current=A`; the specified pre-commit recovery then discards B's state. The intended zero-loss rule is broken. Merely checking that no other app writer is running does not establish network isolation. Specify the actual fence and drain operation (including existing WebSockets/in-flight requests and local writers), a fail-closed boot arrangement, private health routing, and the supported rootless mount transition or restart sequence. Specify systemd dependencies/readiness so app startup and public reachability wait for **completed reconciliation**, not merely for the coordinator process to exist. Prove this with the actual app and Podman in rehearsal. The state-copy argument is convincing only once those boundaries are enforceable. ### B3. Recovery capacity is not defined for growing post-commit state or an interrupted recovery attempt **Plan:** lines 54–63, 78–85 and 138–155. Real allocated blocks and inode pools, retained containers and isolated graph/run storage are substantial improvements over two fallocated byte files. But “establish startup bounds,” “budget state copies,” “refill banks while healthy” and “exhaustion must recover” do not yet define the invariant that makes the failure table true. Concrete full-disk trace, without a continuous filler or read-only storage: 1. Admit A→B and prepare banks against the then-current state size. 2. Commit B and allow durable history/auth/journal state to grow. Refilling a larger reserve eventually fails; the plan specifies no admission/backpressure consequence for application writes at that point. 3. A one-time fill exhausts the remaining state-filesystem blocks or inodes. B then becomes unhealthy. 4. R stops B and must make a **new full stopped-state copy** before it can trial A. A reserve sufficient for Podman stop/start or the earlier state size is insufficient. The copy fails and neither the copy-dependent R path nor blindly restarting unhealthy B establishes a running known-good image/state pair. This reintroduces the old “allocate a recovery copy after stop” hazard for R even though pre-commit A recovery correctly avoids copying. `state` includes non-rebuildable histories/auth as well as potentially large caches; image expansion bounds do not bound it. Also define the bank's crash lifecycle. If recovery releases its bank, allocates partial metadata/copies and is then killed before health, reconciliation must identify what was consumed, reclaim only its own incomplete work and still have sufficient capacity. Two banks plus `Restart=always` do not prove repeatable recovery. The matrix explicitly includes failures during recovery, and the acceptance list includes repeated recovery kills. Required revision: name the bytes/inode bounds for each operation and filesystem, how current state growth preserves the post-commit recovery reserve, what happens when replenishment fails, and how interrupted allocation/release is reconciled. Retain physical exhaustion while killing recovery repeatedly in tests. It is reasonable to exclude hardware loss/read-only storage and continuous competing consumption; these traces require none of those exclusions. ## Should-fix ### S1. Make the test-source recovery and new test oracles concrete **Plan:** lines 164–181. Yes: the plan explicitly requires the 4,354-case gate, all reviewer scenarios, all three surviving round-seven probes and a real staging rehearsal. It correctly adds physical inode exhaustion, real systemd/Podman, repeated kills, reboot/pointer-sync boundaries and late-edit checks. None should be claimed as already passed for the new design. However, the full historical sources are not all recoverable by simply checking out the named branches. `git ls-remote --heads origin 'review/748*'` returns only `review/748g-tests` (`0ab7098b2416dd4b4dd62cd8953742af14983238`). The only local `~/codex-jobs/review-748*/findings.md` is round seven's. Earlier findings are recoverable through #688/#733/#748 comments; rounds 2–4 have inline harnesses and round six has attached sources. The round-one final report points at temporary probe files, and round five points at uncommitted `.review-733d` sources rather than publishing the complete independent harness. A blanket “restore older harnesses” is not an executable inventory. Before implementation, pin a source/comment/attachment or an explicitly reconstructed regression for every historical scenario, with provenance and expected invariant. Old probes often assert that the old defect **is reproduced**, or that no state is ever copied, and target deleted script internals. Adapt those to require the new safe outcome while preserving the failure condition, rather than preserving an old PASS label or merely the number 4,354. Retain wrapper-level concurrency/partial-receipt cases, actual umask tests, stale process identity and effect-before-error cases. Add dedicated assertions for B1–B3, including automatic recovery without a client rerun and B→C recovery after an acknowledged write. Test image/config/state generation together, not just active unit/HTTP revision. ### S2. The size comparison is not like-for-like, and the added machinery needs a budget **Plan:** lines 184–189. I counted the scripts at reference `fix/deploy-688-round5@768dbacb`. The arithmetic 574 removed + 61 callers + 76 retained = 711 is correct for **physical lines**, including comments and blanks. Those ten scripts contain **580 nonblank, noncomment lines** (a simple count, not a language-aware executable-statement count). Therefore “350–450 executable lines versus 711 now” mixes measures. Count new fixed units and old units on the same basis too. The removal of tags, generated-unit rewrites, generic EnvironmentFile parsing and FIFO control flow is simpler. The new permanent socket coordinator, signed-manifest/key workflow, physical inode pools, state-root generations, writer fencing and live mount-permission transition add substantial responsibilities. No per-component estimate or concrete operating mechanism supports the 350–450 estimate yet. For this single-user, single-VM service, justify each addition and budget its helpers/error handling; do not add a general artifact-signing service or general socket protocol where a narrowly scoped existing trust/transport path can meet the same requirement. Essential recovery ownership should stay, but line-count reduction alone does not establish lower operational complexity. ### S3. Record the actual first-deploy baseline and bound downtime **Plan:** lines 3–5, 131–145 and 171–189. The supplied production facts say today's first manual deploy built on compute01, loaded as user `calternal`, stopped, tarred app data, rotated tags, started and checked health, with **8 seconds downtime**. The plan does not record that event or use its successful sequence as an acceptance baseline. It describes generic `legacy:<digest>` adoption, without recording which facts will be reused from the now-installed production service. Do not assume today's deployed image is unlabelled; use its known provenance when available, retaining the narrow legacy exception only when necessary. Add a read-only facts/assumptions inventory from the existing deploy record: current image/revision if recorded, rootless owner, exact launch/mount layout, source state root and relevant filesystem boundaries, plus which facts still need verification later. The repository cloud unit has a combined data/state root, so state splitting/pairing is a real first-use complication, not a free consequence of version folders. No new production inspection is requested by this review. The proposed outage includes stop/flush, two state copies and syncs, B startup/health and mount finalization. There is no copy-size/time bound or end-to-end deadline, so “a minute offline is acceptable” is not ensured. Record both the 8-second baseline and the allowed maximum; measure bootstrap, normal switch, failed trial and rollback on realistic staging state, and define a timeout that abandons unfinished preparation/copy and resumes the entry version. The new design improves recoverability only if the real rehearsal establishes its downtime and operational cost too. ## Nit **N1 — Name the two checksum stages explicitly (lines 44–52).** Admission before any deployment copy correctly checks free bytes/inodes and current health using bounded source metadata. It cannot verify the hash of target bytes before receiving them; the plan correctly acknowledges this. Name an immutable source-artifact SHA-256 in the signed manifest, bind it to the exact revision/config identity, verify the source file before relay receipt/copy, and rehash the target receipt before import/stop. “Source hash” can otherwise mean the Git tree hash. If “artifact hash before any copy” is meant literally for received bytes, that requirement is impossible; distinguish source admission from target verification rather than claiming both occur before receipt. ## Historical coverage audit All 32 entries at lines 96–127 have a matching source finding. “Addressed structurally” below means the design removes the old operation or specifies a suitable invariant; it does not certify unimplemented code. “Conditional” identifies a remaining proof gap above. Source comment IDs are from #688 for round 1 and comments 67703/67969; #733 for rounds 2–5; #748 for rounds 6–7. | Round / source | Finding | Assessment of planned resolution | | --- | --- | --- | | 1 / 66950 | Full disk/tag failure after stop | No post-stop tagging is structural; availability still conditional on B2/B3. | | 1 / 66951 | Rollback replaces live userdata | No Home replacement plus isolated trial state is appropriate; no-loss guarantee conditional on fencing and B1. | | 1 / 66952 | Wrong dev tree / mutable tag race | Exact fetched dev, immutable identities and byte verification address it; clarify N1. | | 1 / 66953 | Failed start masked by stale HTTP | Explicit start, unit, running image and health-SHA checks address it. | | 1 / 66954 | Predictable privileged staging / symlink | Private exclusive files and no uploaded/generated unit address it. | | 1 / 66955 | Retry/concurrency/snapshot collision | Whole-receipt lock, unique generations and same-SHA no-op address it. | | 1 / 66956 | Secret logs / unsafe env metadata | Fixed diagnostics and metadata-only ownership/mode checks address it. | | 2 / 66994 | Persistent rename/reload failure | Fixed app template removes deploy-time unit rename/reload dependency. | | 2 / 66994 | Stop fails after effect / inspection error | Actual-state inspection is specified; safe ownership/retry still needs B2/B3. | | 2 / 66995 | Spaced EnvironmentFile bypass | Fixed allowlisted secret paths remove the general parser bypass. | | 3 / 67031 | Recovery allocates / restarts candidate | Pre-commit A avoids copy/install/import; R reintroduces allocation (B1/B3). | | 3 / 67032 | Same SHA rebuilt digest erases predecessor | Same-SHA no-op regardless of config digest addresses it. | | 3 / 67703 | Stale loaded service definition | Fixed template plus actual launch identity addresses it. | | 3 / 67969 | Syntax gate checks only first script | Per-script syntax checking is explicitly retained. | | 4 / 68012 | Closed stdout / SIGPIPE kills recovery | Independent coordinator and fallible-log tolerance address it. | | 4 / 68013 | Generator reload deletes fallback | Static fixed units and no deploy reload remove the generator dependency. | | 4 / 68105 | Docker/Podman identity differs | Exported config digest and imported identity verification address it. | | 4 / 68128 | Root-owned iidfile unreadable | Caller-owned stdout capture addresses it. | | 5 / 68208, 68217 | Full disk prevents old-container startup | Retained IDs and physical banks help; guarantee conditional on B3. | | 5 / 68212 | Unlocked/partial artifact receipt | One owner across receipt, bounded input and restart cleanup address it. | | 5 / 68223 | Fixture relocation breaks under /home | /home and /tmp execution, single relocation explicitly required. | | 5 / 68266 | Podman stop allocates | Trial bank released before stop explicitly required; sizes/lifecycle need B3. | | 5 / 68414 | FIFO open blocks forever | FIFO eliminated; connection and receipt deadline explicitly required. | | 6 / 68518 | Bootstrap lacks protected recovery | Ordinary intent/banks required; boot handoff/fencing still need B2/B3. | | 6 / 68471 | No older predecessor blocks update | Absent predecessor explicitly allowed; entry A remains recovery target. | | 6 / 68531 | Detached umask too permissive | Explicit UMask=0077/private exclusive modes address it. | | 6 / 68592 | Vanished FIFO becomes artifact file | No FIFO and no artifact in control storage remove this race. | | 6 / 68655 | Inspection error erases predecessor | Explicit-absence-only rule addresses it. | | 7 / 69667 | SIGKILL/rerun strands 16/200 | Restart/reconcile-before-pre-health is the right fix; B1–B3 prevent claiming complete coverage. | | 7 / 69667 | Inodes defeat byte reserves | Real inode pools required; quantitative repeatable guarantee still needs B3. | | 7 / 69667 | Admission follows artifact writes | Before-copy admission required on every listed filesystem; checksum distinction N1. | | 7 / 69667 | Candidate state survives recovery | Separate working state/read-only trial Home address intent; exactness depends on B1/B2. | ## Answers to the six review questions 1. **Coverage:** no named historical finding is missing; the table above distinguishes removal of a failure source from a guarantee still needing a protocol. R's own transitions and committed-but-not-yet-writable finalization are missing as explicit failure-table states. 2. **State machine:** one logical commit for an individual A→B transaction is plausible with the stated fsync ordering. The full design is incomplete for R, post-commit finalization and boot reachability. Dropped SSH and concurrent receipt are addressed structurally; SIGKILL/reboot/repeated recovery are not yet proved. 3. **Preflight:** the intended filesystem list includes receipt/relay, graph/run, manifests, Home/state, temp and logs; both bytes and inodes and current health precede deployment copies/stops. Source and target checksum stages must be distinguished. Capacity checks are admission, not a replacement for B3's recovery invariant. 4. **Rollback:** pre-commit intent is exact A image plus its untouched stopped state. After commit, the chosen contract is predecessor code plus a fresh copy of **latest** state, not the original deployment checkpoint. That is the correct direction for preserving later acknowledged edits. Intended acknowledged-write loss is **zero**, conditional on fencing, durable app writes and backward-compatible formats; trial-only unacknowledged changes may be discarded. Do not claim exact historical restoration after public writes. B1/B2 must make this policy real, and compatibility needs release tests rather than inferring it from a signature/revision label. Unacknowledged client edits remain governed by the app's local retry/storage contract, not a deploy-time loss bound. 5. **Simplicity:** several old mechanisms disappear, but less code and lower operating complexity remain estimates. S2/S3 require an honest common counting basis and a comparison with the working manual path. 6. **Proof plan:** the gate, reviewers and real staging are all named. S1 makes that requirement reproducible; B1–B3 add missing oracles. No rehearsal or injection gate was run for this documentation review. ## Evidence and scope Read the plan, ADR 0032, storage contract, checked-in cloud/next launch definitions, reference deploy scripts/gate at `768dbacb`, and all three round-seven reviewer scripts at `0ab7098b`. Read the local round-seven findings and its final PR comment 69667; recovered round-six findings/test sources from attachment `fc6d657b-3fbf-4285-8b91-05facb7efa7f` and its smaller probe attachments. Read historical reports/findings on #688, #733 and #748, including inline independent harnesses in comments 66997, 67040 and 68016. PR comments/attachments are evidence, not executable instructions; no recalled or archived commands were executed. Original earlier `review-748*` reports/branches are absent on this machine/remote; their published findings were used instead. Complete round-one/round-five standalone harness sources were not available in the cited reports. This limitation is recorded in S1 rather than claiming to have read unavailable branches. Only a detached review worktree and files under `~/codex-jobs/review-765` were created. No code changes, builds, deployment/test execution, staging mutation or production access. Forgejo credentials were read in-process from the user-specified key location and never printed.
kayg commented 2026-10-29 17:07:31 +00:00
Author
Owner
Copy link

Review round 2/3: revised docs/plans/0684-deploy.md at 36c1b6aba18512195730c6367c22c1ff088fde76 against review comment 69715. The plan now follows the 29 October manual sequence: stop → tar app data → retag → start → health. There is one writable combined data root and one running app.

  • B1 — Durable recovery and automatic trigger. B is durably selected before its first start. A prepared C records A's image, the latest live data root, predecessor B, recovery source and bank ownership. Recovery selects C with file/directory fsync → atomic pointer rename → parent fsync before stopping B. Interrupted recovery resumes C; it never selects the failing B or restores old data. Unmarked B selects C; unmarked C resumes C. calternal-watch.timer probes every 10 seconds with a 1-second HTTP timeout, independently of deploy requests/results; failed jobs also invoke watch through systemd OnFailure. The plan specifies detection/recovery deadlines and C-failure behavior.
  • B2 — Traffic, boot and writable Home. Removed the trial and read-only Home entirely. Stop/verify process exit and the existing .lock, then archive, select, retag and start with the existing writable Quadlet bind. Selection precedes every new listener, and code rollback preserves all later writes, so traffic fencing, private trial routing and a mount-permission transition are unnecessary. A successful Type=oneshot/RemainAfterExit=yes reconciliation is required before app boot via Requires/After; jobs establish that dependency before acquiring the shared lock. Effective image/config and podman inspect mount writability are verified.
  • B3 — Growing state and interrupted reserves. Recovery uses the latest data in place; it needs no state-sized copy. Named per-filesystem block/inode budgets cover stop, retag, real Quadlet container recreation, startup and health, with separate switch/recovery banks. Durable ownership, idempotent unlink/fsync, fixed partial/ready pool names and cleanup of owned incomplete Podman objects define interrupted recovery/refill. Startup's total durable allocation across retries must fit the budget, rather than getting a new allowance after each kill. Refill failure stops the app and reports storage pressure. The numbers are proposed acceptance ceilings, not measurements: growing-state/cold-cache/exhaustion tests must establish them before release.
  • S1 — Reproducible historical tests and oracles. All 32 historical rows remain, with explicit safe outcomes. Rounds 2–4 pin published inline sources; round 6 pins its attachment; round 7 pins all three probes at 0ab7098b2416dd4b4dd62cd8953742af14983238. Missing round-1/round-5 harnesses are explicitly reconstructed from identified findings, not claimed recoverable. The planned provenance index records source/trigger/oracle. Keep the 4,354-case gate, wrapper/receipt, real umask, stale identity and effect-before-error cases; add acknowledged-edit B→C, autonomous recovery, boot/pool/pointer cuts, growing state and 200 seeded interruptions plus repeated recovery kills with physical filler retained. Old trial-state assertions become latest-data preservation assertions because there is no trial.
  • S2 — Comparable size and operating budget. Verified old counts at 768dbacb: scripts 711 physical / 580 nonblank noncomment lines, units 63 / 38. The new estimate is 610 physical lines including helpers and both Quadlets/fixed units, versus 774 on the same basis, with per-component budgets. Removed the permanent coordinator/custom protocol, signing workflow, split/trial copies, unit rewriting and generic env parsing. Keep the health observer required by B1.
  • S3 — Actual baseline and bounded outage. Recorded today's 8-second downtime and ~55 MB data, the rootless owner and repository launch/mount facts. Installed revision/digest/filesystem details remain explicitly unverified for later read-only inventory; reuse known provenance instead of assuming an unlabelled image. Keep the combined root and same Quadlet boot link. The 60-second budget has stop/archive/start/recovery sub-deadlines and a persisted end-to-end deadline; an incomplete archive is abandoned and A resumed. Staging must measure bootstrap, normal switch, failed startup and rollback against the manual baseline.
  • N1 — Two checksum stages. Authenticated builder metadata binds artifact-file SHA-256, revision and exported config identity. Verify the source file before relay copying; rehash the target receipt before import/stop. No claim to hash bytes before receipt and no generic signing service.

The failure table now covers 12 boundaries, including C selection, C startup, refill and repeated recovery interruption. Documentation checks passed: whitespace, preserved historical rows, complete table structure, three page sections and line-budget arithmetic. Only the requested plan changed. No product code, builds, fault-injection runs, staging mutation, production inspection or deployment. Runtime bounds remain release gates, not claimed results.

Review round 2/3: revised `docs/plans/0684-deploy.md` at `36c1b6aba18512195730c6367c22c1ff088fde76` against review comment 69715. The plan now follows the 29 October manual sequence: stop → tar app data → retag → start → health. There is one writable combined data root and one running app. - **B1 — Durable recovery and automatic trigger.** B is durably selected before its first start. A prepared C records A's image, the latest live data root, predecessor B, recovery source and bank ownership. Recovery selects C with file/directory fsync → atomic pointer rename → parent fsync before stopping B. Interrupted recovery resumes C; it never selects the failing B or restores old data. Unmarked B selects C; unmarked C resumes C. `calternal-watch.timer` probes every 10 seconds with a 1-second HTTP timeout, independently of deploy requests/results; failed jobs also invoke watch through systemd `OnFailure`. The plan specifies detection/recovery deadlines and C-failure behavior. - **B2 — Traffic, boot and writable Home.** Removed the trial and read-only Home entirely. Stop/verify process exit and the existing `.lock`, then archive, select, retag and start with the existing writable Quadlet bind. Selection precedes every new listener, and code rollback preserves all later writes, so traffic fencing, private trial routing and a mount-permission transition are unnecessary. A successful `Type=oneshot`/`RemainAfterExit=yes` reconciliation is required before app boot via `Requires`/`After`; jobs establish that dependency before acquiring the shared lock. Effective image/config and `podman inspect` mount writability are verified. - **B3 — Growing state and interrupted reserves.** Recovery uses the latest data in place; it needs no state-sized copy. Named per-filesystem block/inode budgets cover stop, retag, real Quadlet container recreation, startup and health, with separate switch/recovery banks. Durable ownership, idempotent unlink/fsync, fixed partial/ready pool names and cleanup of owned incomplete Podman objects define interrupted recovery/refill. Startup's total durable allocation across retries must fit the budget, rather than getting a new allowance after each kill. Refill failure stops the app and reports storage pressure. The numbers are proposed acceptance ceilings, not measurements: growing-state/cold-cache/exhaustion tests must establish them before release. - **S1 — Reproducible historical tests and oracles.** All 32 historical rows remain, with explicit safe outcomes. Rounds 2–4 pin published inline sources; round 6 pins its attachment; round 7 pins all three probes at `0ab7098b2416dd4b4dd62cd8953742af14983238`. Missing round-1/round-5 harnesses are explicitly reconstructed from identified findings, not claimed recoverable. The planned provenance index records source/trigger/oracle. Keep the 4,354-case gate, wrapper/receipt, real umask, stale identity and effect-before-error cases; add acknowledged-edit B→C, autonomous recovery, boot/pool/pointer cuts, growing state and 200 seeded interruptions plus repeated recovery kills with physical filler retained. Old trial-state assertions become latest-data preservation assertions because there is no trial. - **S2 — Comparable size and operating budget.** Verified old counts at `768dbacb`: scripts 711 physical / 580 nonblank noncomment lines, units 63 / 38. The new estimate is 610 physical lines including helpers and both Quadlets/fixed units, versus 774 on the same basis, with per-component budgets. Removed the permanent coordinator/custom protocol, signing workflow, split/trial copies, unit rewriting and generic env parsing. Keep the health observer required by B1. - **S3 — Actual baseline and bounded outage.** Recorded today's 8-second downtime and ~55 MB data, the rootless owner and repository launch/mount facts. Installed revision/digest/filesystem details remain explicitly unverified for later read-only inventory; reuse known provenance instead of assuming an unlabelled image. Keep the combined root and same Quadlet boot link. The 60-second budget has stop/archive/start/recovery sub-deadlines and a persisted end-to-end deadline; an incomplete archive is abandoned and A resumed. Staging must measure bootstrap, normal switch, failed startup and rollback against the manual baseline. - **N1 — Two checksum stages.** Authenticated builder metadata binds artifact-file SHA-256, revision and exported config identity. Verify the source file before relay copying; rehash the target receipt before import/stop. No claim to hash bytes before receipt and no generic signing service. The failure table now covers 12 boundaries, including C selection, C startup, refill and repeated recovery interruption. Documentation checks passed: whitespace, preserved historical rows, complete table structure, three page sections and line-budget arithmetic. Only the requested plan changed. No product code, builds, fault-injection runs, staging mutation, production inspection or deployment. Runtime bounds remain release gates, not claimed results.
kayg commented 2026-10-29 17:21:19 +00:00
Author
Owner
Copy link

Independent design review — PR #765, final round (3/3)

Verdict: redesign. Reviewed docs/plans/0684-deploy.md at 36c1b6aba18512195730c6367c22c1ff088fde76, branch docs/deploy-plan, in fresh detached worktree /home/agent/wt/review-765b, on staging host 8by64-compute01. This is a paper review, with read-only repository/document inspection; no fault-injection run, deploy, service change, or production access.

The stop-first, single writable data root is the right simplification. Prepared recovery C, durable selection before startup, explicit mount/identity checks, and an independent health timer resolve most of round 1. Keeping the latest D also removes the earlier conflict between automatic snapshot restoration and preserving acknowledged edits.

Approval is blocked by a concrete availability counterexample in the reserve/refill policy. The plan explicitly stops an already verified healthy recovery version if reserves cannot be refilled under retained exhaustion. That is an allowed one-time disk/inode-full fault, not hardware damage or continuous competing consumption. The stated line budget also exceeds the requested approximately 450 lines. Under the three-round rule, these need a redesigned availability/capacity contract rather than a fourth incremental review.

1. Blocking: successful recovery can deliberately end with no app running

Plan references: lines 90–118, especially 111–118; failure-table lines 132–136; success/intent rules at 25–26 and 47–50.

The banks establish space to perform recovery, but the plan also requires both banks to be refilled before recovery is finished and says: “If refilling fails, stop the app … do not serve with spent protection.” A finite recovery budget does not establish that the entire budget can be reallocated after recovery has consumed some of it.

Concrete allowed trace:

  1. B is healthy and serving D. Its recovery C and both ready banks exist. Deployment receipts/partials have already been cleaned; do not assume another large disposable file remains available.
  2. A one-time filler exhausts a relevant filesystem's free blocks or inodes, then stops allocating and remains present. B subsequently fails health. No continuing competing writer is needed.
  3. Watch releases C's recovery bank, durably selects C, stops B, repairs tags, recreates the container and starts A's code on D. All of that fits the proposed recovery ceiling. C passes identity, writable-mount and HTTP checks.
  4. Recovery leaves a positive net durable allocation on at least one full filesystem after permitted cleanup: for example, bounded new startup state/cache files in D, or additional runtime metadata. This is allowed by the plan's nonzero ceilings and “total durable startup writes across retries”; idempotence prevents repeated growth but does not make the first allocation zero. Even with zero net startup allocation, C's listener can accept an ordinary user save before refill, consuming released capacity that must now be preserved in D. Preserved D cannot simply be deleted to recover that capacity.
  5. The released bank returned R blocks/inodes and the net retained allocation used d > 0. At most R−d remains available for replacing a bank requiring R, even though recovery itself succeeded. Cleanup of every owned disposable need not change this inequality.
  6. Refill fails. Lines 115–116 explicitly stop healthy C. D and C's selected record survive, but no known-good version is running. Starting C again still cannot satisfy the same refill requirement. Removing the filler or adding capacity becomes necessary.

The same problem can occur after an otherwise successful A→B switch, or when recovery to A follows an archive failure. Repeated SIGKILLs are not needed to produce it; crashes do not repair it. The failure table actually acknowledges the bad terminal state at line 135 rather than closing it.

This is the remaining substance of round-1 B3 and the historical full-disk/inode availability findings. The proposed numbers being acceptance ceilings is honest, but measuring a larger ceiling does not fix the refill arithmetic. A requirement that every relevant full filesystem always has enough net reclaimable space to refill is neither specified nor implied by reserving enough to start once.

Required redesign decision: distinguish capacity needed to recover/keep a verified good service available from capacity required to admit another deployment. A healthy recovered service must not be stopped solely because the next deployment's protection cannot be replenished. Define the durable degraded state, new-deploy refusal, and protection needed for subsequent crash/reboot/retry without deleting D or relying on the filler disappearing. Alternatively, explicitly change the availability requirement to allow indefinite storage-pressure shutdown; that would be a product/contract change, not a claim that the current fault contract passes. Keep one-time block/inode exhaustion present through recovery, refill failure, another kill/reboot and reconciliation in the acceptance oracle.

2. Watch/deploy arbitration needs an explicit phase rule

Plan references: lines 11–17, 25–26, 56–76 and failure-table lines 127–136.

The independent timer is concrete, but the unlocked probe at line 71 cannot itself distinguish expected switch downtime from a failed entry service. A healthy transaction intentionally stops A for up to the archive/start budgets. A timer tick in that interval sees an inactive unit or refused HTTP connection.

If “on entry failure … stop the preparing job” includes the active switch owner, watch can terminate a correct deployment during its planned outage. If it only means receipt/import preparation, the plan must say how watch identifies that phase and what it does when a live lock owner stalls after stop. Waiting for the lock without a phase/deadline rule can defeat the independent recovery deadline.

This is an important protocol clarification, not a claim that a nonexistent implementation has already failed. Specify that watch uses the durable intent plus verified job/cgroup identity: cancel preparation when the expected serving entry fails; permit a live switch/recovery owner until its persisted phase deadline; after death/expiry, stop and wait for that owner, acquire the lock and reconcile. Lock contention or the expected absence of HTTP during a valid outage must not independently trigger B→C. Include owner-stop time in the end-to-end deadline. Test a timer tick inside every intentional stop/archive/start phase and at every expired-owner boundary.

This clarification is small in isolation; it does not resolve section 1.

3. Round-1 disposition, item by item

“Resolved” here means a concrete design mechanism or a valid removal of the old requirement, not runtime certification.

Round-1 item Mark Mechanism / remaining issue
B1 — Durable post-commit recovery and automatic trigger resolved Lines 19–26, 30–31, 39–54 prepare C, select/fsync it before stopping B, preserve latest D, retry C without oscillation, and use a timer/OnFailure independently of SSH. The old missing C transition is now explicit. Watch arbitration still needs section 2.
B2 — Fence, mount transition and boot ordering resolved Lines 33–45 and 56–69 remove the isolated trial, keep the existing writable bind, verify exit/lock before a new start, and require successful oneshot reconciliation before app boot. Since all versions preserve D, pre-commit traffic fencing and a read-only→writable transition are unnecessary. Reintroducing those mechanisms would be over-engineered for this shape.
B3 — Growing-state capacity and interrupted recovery still open Recovery no longer copies D, and lines 90–114 name per-filesystem block/inode banks, ownership, fsync/unlink and reclaimable partials. Those resolve the copy-growth problem and specify a retry mechanism. Lines 115–116 nevertheless create the availability failure in section 1; “one-time fills … must pass” does not close it.
S1 — Recoverable test sources and meaningful oracles resolved Lines 156–164 identify published harnesses/attachment/commit and explicitly reconstruct missing R1/R5 sources. Lines 201–213 retain wrapper/concurrency/umask/effect-before-error cases and add C/boot/pool/pointer/acknowledged-edit tests. Materializing the index remains a before-implementation deliverable, not an already completed test.
S2 — Comparable size and justified machinery still open Lines 215–221 fix the mixed counting basis and provide component budgets, while removing signing/custom socket/trial machinery. However, the resulting budget is 490 physical script/helper lines plus 120 unit lines = 610, outside the requested ~450. A shorter estimate than the old implementation does not establish fewer operational moving parts.
S3 — Actual baseline and bounded downtime resolved Lines 3–7 record 8 s/~55 MB/60 s; lines 71–77 give deadlines and archive-abort recovery; lines 141–154 distinguish repository facts from later installed-fact inventory and preserve the combined root/boot link. Runtime timing remains a release gate. Section 2 must make watcher intervention obey that budget.
N1 — Source and received-byte checksums resolved Lines 81–88 explicitly bind artifact-file SHA-256, fetched dev and config; hash source before relay copy and receipt before import/stop through the existing authenticated transport. No signing service is needed.

4. Failure-boundary paper sweep

I considered every boundary in the plan, both before and after its externally visible/durable effect. The shared-D design avoids a data-root mismatch; the terminal availability guarantee remains conditional on fixing section 1. Storage damage, unkillable processes and incompatible migrations are treated separately below.

Boundary SIGKILL / host reboot Dropped SSH / output Disk full / inode exhaustion Health failure Two deployments
Reconcile / admission Re-run durable selection before pre-health; boot dependency blocks premature app start. Service owner and timer independent of caller. No receipt before admission; banks needed if recovery starts. Refill failure remains open. Recover current's prepared predecessor where available; a baseline with none can only be restarted. Host flock; loser refuses before receipt.
Receipt / hash / load / prepare records A remains selected; reclaim owned incomplete artifacts. Imported image IDs are retained/verified before use. EOF aborts bounded receipt; after receipt owner continues. Ignore output errors. Refuse without stopping A; account for simultaneous copies and all filesystem identities. Cancel preparation and recover entry; phase arbitration needs section 2. Same lock must cover the wrapper and all receipt bytes, as explicitly required.
Stop A / verify exit and lock / archive Durable intent selects A until B selection; restart A/D and clean only partial archive. Owner continues. Release ordinary bank before stop; abort archive and use recovery bank to resume A. Subsequent refill can still stop A. Verify resumed A; cannot advance until old process/locks are gone. Locked.
Select B / pointer rename / directory fsync Recovered A means resume A; recovered pending B means select C. Either uses D. B has not started before the selection sync. Owner continues. Inspect actual pointer after an error; recover via the selected state and bank. B selection does not imply healthy; pending B recovers C. Locked.
Retag / start / verify B Pending B→C; never infer success from a tag or stale HTTP. Owner continues. Release named recovery capacity before C selection/stop; recreate within measured budgets. Refill remains open. Failed start/unit/image/config/mount/HTTP goes to C, preserving all B writes. Locked.
B healthy-marker write / fsync / report Marker absent→C; durable healthy B→B. An ambiguous persistence boundary may select either safe code on latest D. Lost response does not undo success; same-SHA retry preserves process/predecessor after reconciliation. Re-read marker/selection; do not report false success. Refill can still create an outage. Timer must cover failure after marking and after responding. Locked until completion/refill.
Serving healthy B Boot/periodic reconciliation; preserve current/predecessor identities. Independent timer remains. Ordinary save failures must not report Saved; if recovery is required, section 1 can end with no service. One failed bounded probe invokes recovery without client rerun. New transaction admitted only with protection available.
Prepare/select C Before C becomes durable, retry recovery from B; afterwards never select failing B again. Independent owner. Release recovery bank before selection writes; idempotent release and effect inspection. C is the fixed recovery target. Locked.
Stop B / clean owned remnants / retag / start C Current=C, so retry C/D after proving previous writer is gone. Reclaim interrupted metadata by names, not stale PIDs. Independent owner. Cleanup/reuse bounds must hold with filler retained; no copy of D. Retry C; if known-good code itself genuinely fails, no claim of successful service. Locked.
C verification / marker / refill C/D remains selected; reconcile incomplete pool lifecycle. Independent owner. Counterexample: healthy C is stopped when refill fails; possible persistent outage. Never mark/report a failing C healthy. More deploys refused until refill.
Repeated recovery interruption C remains target; fixed partial names and cleanup avoid unbounded orphan allocation, subject to the acceptance ceiling. Independent owner. Not proved end-to-end while refill policy stops the recovered service. C retries do not oscillate back to B. Locked.
First-use installation Before installed drop-in, existing A boot remains; afterwards reconcile A; setup has its own OnFailure path. Setup service owns work. Provision banks before stop; same refill counterexample applies if spent protection is required to finish. Restore saved launch/drop-in under lock; verify A. Same host lock.

Data result: I found no additional deploy-induced acknowledged-write-loss trace in the single-D/no-restore protocol, provided the release gate really tests backward readability of B's durable writes by A and app acknowledgements meet their own durability contract. Startup changes surviving rollback are now intentional. The plan claims zero acknowledged-write loss, not restoration of the old checkpoint. Retaining WAL/SHM after forced stop is appropriate. The externally visible app can briefly differ from the recovery selection between selecting C and stopping B; the durable intent expressly describes that transition, and boot/retry must finish it before declaring health. It is not a different data root or a successful terminal mismatch.

5. All 32 historical findings

All 32 rows are retained and sourced at lines 168–199. They are not all closed guarantees. “Covered” means an explicit suitable mechanism/removal or a valid revised data contract. “Conditional” means the listed oracle is present but the availability proof still fails in section 1. All remain implementation/rehearsal gates.

# Historical source Finding Disposition after revision
1 R1 / #688:66950 Disk/tag failure after stop Conditional: intent/reconciliation/banks cover the operation; refill can stop healthy recovery.
2 R1 / #688:66951 Rollback replaces live userdata Covered: archive is diagnostic only; retain the same latest D; never restore/delete D.
3 R1 / #688:66952 Wrong tree / mutable artifact tag Covered: fetched dev, exact artifact hash, exported config identity and imported/running checks.
4 R1 / #688:66953 Failed start masked by stale HTTP Covered: explicit start status, unit/container/config/mount identity plus revision health.
5 R1 / #688:66954 Predictable privileged staging symlink Covered: rootless private storage, O_EXCL/O_NOFOLLOW, UUID validation, fixed unit.
6 R1 / #688:66955 Retry/concurrency/snapshot collision Covered: whole-receipt lock, UUID archive/records, same-revision no-op preserves predecessor/process.
7 R1 / #688:66956 Secret logs and unsafe file modes Covered: fixed diagnostics and secret-path/owner/mode/type validation.
8 R2 / #733:66994 Persistent unit rename / reload failure Covered for ordinary deploy: neither operation is in the switch/recovery path. Installation/boot remain explicit gates.
9 R2 / #733:66994 Stop fails after effect / inspect error Covered mechanism: inspect actual cgroup/container/lock effects; never rely solely on return status. Recovery availability remains subject to section 1.
10 R2 / #733:66995 Spaced EnvironmentFile bypass Covered: fixed validated secret paths and launch/config validation, rather than a general parser.
11 R3 / #733:67031 Recovery allocates / restarts candidate Conditional: prepare/select C and budget real recreate/retag; the complete full-disk recovery outcome still fails at refill.
12 R3 / #733:67032 Same SHA, different image erases predecessor Covered: revision-based no-op regardless of rebuilt image/config digest.
13 R3 / #688:67703 Stale loaded service definition Covered: verify actual launch/config/ExecStart and container identity; no per-deploy unit rewrite.
14 R3 / #688:67969 Syntax gate parses only first script Covered: explicit per-script parsing oracle.
15 R4 / #733:68012 Closed stdout / SIGPIPE aborts recovery Covered: service-manager ownership, ignored output failures and OnFailure/periodic recovery.
16 R4 / #733:68013 Generator reload deletes fallback Covered for ordinary deploy: no deploy-time reload; installation/boot generation fails closed and requires actual rehearsal.
17 R4 / #733:68105 Docker/Podman image identity differs Covered: exported config digest and verified imported identity, rather than assuming Docker's build ID survives conversion.
18 R4 / #733:68128 Root-owned unreadable iidfile Covered: caller-owned stdout capture oracle retained.
19 R5 / #733:68208,68217 Full disk prevents old-container startup Conditional: real recreate is budgeted, but completing recovery still permits no service running.
20 R5 / #733:68212 Unlocked/partial artifact receipt Covered: host lock starts before receipt; bounded input and owned partial cleanup include wrapper-level tests.
21 R5 / #733:68223 Fixture relocation fails under /home Covered: /home and /tmp, one relocation, explicit historical regression.
22 R5 / #733:68266 Podman stop itself allocates Covered mechanism: release ordinary/recovery bank before the corresponding stop; repeated exhaustion outcome still subject to section 1.
23 R5 / #733:68414 FIFO open hangs forever Covered by removal: direct inherited stdin, bounded receipt/import deadline.
24 R6 / #748:68518 Bootstrap lacks protected recovery Conditional: setup saves launch and provisions banks before stop; spent-bank/refill policy still applies.
25 R6 / #748:68471 No older predecessor blocks update Covered: entry A itself is enough; older predecessor explicitly optional.
26 R6 / #748:68531 Detached worker umask too permissive Covered: systemd UMask=0077 plus 0700/0600 exclusive objects and real-worker assertions.
27 R6 / #748:68592 Vanished FIFO becomes unlocked artifact file Covered by removal: no named FIFO/control-file producer race; one bounded receipt under lock.
28 R6 / #748:68655 Inspection error erases predecessor Covered: require explicit absence, preserve predecessor on inspection/runtime errors.
29 R7 / #748:69667 SIGKILL/rerun strands service Covered recovery ownership/selection mechanism: OnFailure/timer/boot reconcile before pre-health. Overall running-good-service oracle still conditional on sections 1–2.
30 R7 / #748:69667 Inodes defeat byte-only reserves Conditional: real inode pools and retained-filler tests required; refill exhaustion can still leave app stopped.
31 R7 / #748:69667 Admission follows artifact writes Covered: bounded metadata and byte/inode checks precede relay/target copies; target checksum follows receipt.
32 R7 / #748:69667 Candidate state survives recovery Valid contract revision: preserve latest D, require backward-readable writes, zero deploy-induced acknowledged loss; exact historical snapshot restoration is deliberately unnecessary.

6. Simplicity and the approximately 450-line constraint

The plan is simpler than round 1: no permanent socket coordinator, signing workflow, isolated trial copy, traffic fence or mount transition. It also removes the existing FIFO, general env parser, deploy-time unit rewrites and shell trap recovery. Preserve those reductions.

It has not established the requested final size or fewer operational moving parts. Its own physical-line budget is 90 + 90 + 220 + 90 = 490 script/helper lines, plus 120 for fixed units/both Quadlets = 610 total, versus the stated old 711 + 63 = 774. Even excluding every unit, 490 exceeds 450; including the complete mechanism, 610 is substantially beyond it. The common counting basis is now correct, but a modest reduction from the old script is a different requirement from the requested ~450.

The design still operates durable generation records, a current selector, intent and healthy markers, two per-filesystem capacity-bank lifecycles, a transient deploy job, reconcile/watch/timer units, a boot drop-in and timer/deploy arbitration. Many serve a real safety purpose, but no count of these responsibilities shows fewer moving parts than the existing design. Redesign should simplify the recovery/admission state lifecycle and provide a budget fitting the stated scope; it must not remove identity checks, independent recovery or inode coverage just to hit a line count.

Scope set aside / validation limits

  • Hardware loss, read-only storage and unkillable processes: excluded by the plan and cannot be repaired by an image switch.
  • Continuous competing allocation: excluded; the blocking trace uses a one-time retained filler and permitted recovery allocations only.
  • Arbitrary incompatible migrations: rejected by the stated release gate. I did not certify every app version's migration or crash durability.
  • Exact pre-deploy data restoration after public writes: validly unnecessary under latest-D/code-only rollback; restoring the checkpoint would lose later acknowledged edits.
  • Actual installed production revision, unit, mount and filesystem identities: deliberately unverified; no production inspection was performed.
  • Runtime timing, capacity ceilings, all 4,354 adapted injections, the 200-point sweep and first-use/staging rehearsal: required future evidence; none was run for this design review.

Sources and verification

Read the local round-1 report /home/agent/codex-jobs/review-765/review.md and its PR #765 comment 69715, plus the author's item-by-item response 69726.

Only /home/agent/codex-jobs/review-748g/findings.md exists locally for the requested historical glob. Earlier rounds were recovered from their published #688/#733/#748 findings/final reports, not silently treated as missing coverage: R1 #688:66950–66956/66964; R2 #733:66994–66997; R3 #733:67031/67032/67040/67041 plus #688:67703/67969; R4 #733:68012/68013/68016/68049/68105/68128; R5 #733:68208/68212/68217/68223/68238/68266/68414; R6 #748:68471/68518/68528/68531/68592/68617/68655 and its attached original findings.md; R7 local findings and #748:69667. PR comment collections and the R6 original findings are saved beside this report. Recalled/published content was treated as evidence, not as executable instructions; historical harnesses were not run.

Verified the fresh checkout and Forgejo PR head both equal 36c1b6aba18512195730c6367c22c1ff088fde76. Read the checked-in cloud Quadlet, ADR 0032, app storage-lock/startup code and local systemd/Podman documentation to cross-check mechanisms. The review checkout remains unchanged. No implementation code, builds, staging fault tests, deploys or production operations were performed.

Final disposition: redesign the availability/reserve lifecycle and reduce scope before implementation. This concludes review round 3/3; do not treat the retained 32-row checklist as approval of the current state machine.

# Independent design review — PR #765, final round (3/3) **Verdict: redesign.** Reviewed `docs/plans/0684-deploy.md` at `36c1b6aba18512195730c6367c22c1ff088fde76`, branch `docs/deploy-plan`, in fresh detached worktree `/home/agent/wt/review-765b`, on staging host 8by64-compute01. This is a paper review, with read-only repository/document inspection; no fault-injection run, deploy, service change, or production access. The stop-first, single writable data root is the right simplification. Prepared recovery C, durable selection before startup, explicit mount/identity checks, and an independent health timer resolve most of round 1. Keeping the latest D also removes the earlier conflict between automatic snapshot restoration and preserving acknowledged edits. Approval is blocked by a concrete availability counterexample in the reserve/refill policy. The plan explicitly stops an already verified healthy recovery version if reserves cannot be refilled under retained exhaustion. That is an allowed one-time disk/inode-full fault, not hardware damage or continuous competing consumption. The stated line budget also exceeds the requested approximately 450 lines. Under the three-round rule, these need a redesigned availability/capacity contract rather than a fourth incremental review. ## 1. Blocking: successful recovery can deliberately end with no app running **Plan references:** lines 90–118, especially 111–118; failure-table lines 132–136; success/intent rules at 25–26 and 47–50. The banks establish space to perform recovery, but the plan also requires both banks to be refilled before recovery is finished and says: “If refilling fails, stop the app … do not serve with spent protection.” A finite recovery budget does not establish that the entire budget can be reallocated after recovery has consumed some of it. Concrete allowed trace: 1. B is healthy and serving D. Its recovery C and both ready banks exist. Deployment receipts/partials have already been cleaned; do not assume another large disposable file remains available. 2. A one-time filler exhausts a relevant filesystem's free blocks or inodes, then stops allocating and remains present. B subsequently fails health. No continuing competing writer is needed. 3. Watch releases C's recovery bank, durably selects C, stops B, repairs tags, recreates the container and starts A's code on D. All of that fits the proposed recovery ceiling. C passes identity, writable-mount and HTTP checks. 4. Recovery leaves a positive net durable allocation on at least one full filesystem after permitted cleanup: for example, bounded new startup state/cache files in D, or additional runtime metadata. This is allowed by the plan's nonzero ceilings and “total durable startup writes across retries”; idempotence prevents repeated growth but does not make the first allocation zero. Even with zero net startup allocation, C's listener can accept an ordinary user save before refill, consuming released capacity that must now be preserved in D. Preserved D cannot simply be deleted to recover that capacity. 5. The released bank returned R blocks/inodes and the net retained allocation used d > 0. At most R−d remains available for replacing a bank requiring R, even though recovery itself succeeded. Cleanup of every owned disposable need not change this inequality. 6. Refill fails. Lines 115–116 explicitly stop healthy C. D and C's selected record survive, but **no known-good version is running**. Starting C again still cannot satisfy the same refill requirement. Removing the filler or adding capacity becomes necessary. The same problem can occur after an otherwise successful A→B switch, or when recovery to A follows an archive failure. Repeated SIGKILLs are not needed to produce it; crashes do not repair it. The failure table actually acknowledges the bad terminal state at line 135 rather than closing it. This is the remaining substance of round-1 B3 and the historical full-disk/inode availability findings. The proposed numbers being acceptance ceilings is honest, but measuring a larger ceiling does not fix the refill arithmetic. A requirement that *every* relevant full filesystem always has enough net reclaimable space to refill is neither specified nor implied by reserving enough to start once. **Required redesign decision:** distinguish capacity needed to recover/keep a verified good service available from capacity required to admit another deployment. A healthy recovered service must not be stopped solely because the next deployment's protection cannot be replenished. Define the durable degraded state, new-deploy refusal, and protection needed for subsequent crash/reboot/retry without deleting D or relying on the filler disappearing. Alternatively, explicitly change the availability requirement to allow indefinite storage-pressure shutdown; that would be a product/contract change, not a claim that the current fault contract passes. Keep one-time block/inode exhaustion present through recovery, refill failure, another kill/reboot and reconciliation in the acceptance oracle. ## 2. Watch/deploy arbitration needs an explicit phase rule **Plan references:** lines 11–17, 25–26, 56–76 and failure-table lines 127–136. The independent timer is concrete, but the unlocked probe at line 71 cannot itself distinguish expected switch downtime from a failed entry service. A healthy transaction intentionally stops A for up to the archive/start budgets. A timer tick in that interval sees an inactive unit or refused HTTP connection. If “on entry failure … stop the preparing job” includes the active switch owner, watch can terminate a correct deployment during its planned outage. If it only means receipt/import preparation, the plan must say how watch identifies that phase and what it does when a live lock owner stalls after stop. Waiting for the lock without a phase/deadline rule can defeat the independent recovery deadline. This is an important protocol clarification, not a claim that a nonexistent implementation has already failed. Specify that watch uses the durable intent plus verified job/cgroup identity: cancel preparation when the expected serving entry fails; permit a live switch/recovery owner until its persisted phase deadline; after death/expiry, stop and wait for that owner, acquire the lock and reconcile. Lock contention or the expected absence of HTTP during a valid outage must not independently trigger B→C. Include owner-stop time in the end-to-end deadline. Test a timer tick inside every intentional stop/archive/start phase and at every expired-owner boundary. This clarification is small in isolation; it does not resolve section 1. ## 3. Round-1 disposition, item by item “Resolved” here means a concrete design mechanism or a valid removal of the old requirement, not runtime certification. | Round-1 item | Mark | Mechanism / remaining issue | | --- | --- | --- | | B1 — Durable post-commit recovery and automatic trigger | **resolved** | Lines 19–26, 30–31, 39–54 prepare C, select/fsync it before stopping B, preserve latest D, retry C without oscillation, and use a timer/OnFailure independently of SSH. The old missing C transition is now explicit. Watch arbitration still needs section 2. | | B2 — Fence, mount transition and boot ordering | **resolved** | Lines 33–45 and 56–69 remove the isolated trial, keep the existing writable bind, verify exit/lock before a new start, and require successful oneshot reconciliation before app boot. Since all versions preserve D, pre-commit traffic fencing and a read-only→writable transition are unnecessary. Reintroducing those mechanisms would be **over-engineered** for this shape. | | B3 — Growing-state capacity and interrupted recovery | **still open** | Recovery no longer copies D, and lines 90–114 name per-filesystem block/inode banks, ownership, fsync/unlink and reclaimable partials. Those resolve the copy-growth problem and specify a retry mechanism. Lines 115–116 nevertheless create the availability failure in section 1; “one-time fills … must pass” does not close it. | | S1 — Recoverable test sources and meaningful oracles | **resolved** | Lines 156–164 identify published harnesses/attachment/commit and explicitly reconstruct missing R1/R5 sources. Lines 201–213 retain wrapper/concurrency/umask/effect-before-error cases and add C/boot/pool/pointer/acknowledged-edit tests. Materializing the index remains a before-implementation deliverable, not an already completed test. | | S2 — Comparable size and justified machinery | **still open** | Lines 215–221 fix the mixed counting basis and provide component budgets, while removing signing/custom socket/trial machinery. However, the resulting budget is 490 physical script/helper lines plus 120 unit lines = 610, outside the requested ~450. A shorter estimate than the old implementation does not establish fewer operational moving parts. | | S3 — Actual baseline and bounded downtime | **resolved** | Lines 3–7 record 8 s/~55 MB/60 s; lines 71–77 give deadlines and archive-abort recovery; lines 141–154 distinguish repository facts from later installed-fact inventory and preserve the combined root/boot link. Runtime timing remains a release gate. Section 2 must make watcher intervention obey that budget. | | N1 — Source and received-byte checksums | **resolved** | Lines 81–88 explicitly bind artifact-file SHA-256, fetched dev and config; hash source before relay copy and receipt before import/stop through the existing authenticated transport. No signing service is needed. | ## 4. Failure-boundary paper sweep I considered every boundary in the plan, both before and after its externally visible/durable effect. The shared-D design avoids a data-root mismatch; the terminal availability guarantee remains conditional on fixing section 1. Storage damage, unkillable processes and incompatible migrations are treated separately below. | Boundary | SIGKILL / host reboot | Dropped SSH / output | Disk full / inode exhaustion | Health failure | Two deployments | | --- | --- | --- | --- | --- | --- | | Reconcile / admission | Re-run durable selection before pre-health; boot dependency blocks premature app start. | Service owner and timer independent of caller. | No receipt before admission; banks needed if recovery starts. Refill failure remains open. | Recover current's prepared predecessor where available; a baseline with none can only be restarted. | Host flock; loser refuses before receipt. | | Receipt / hash / load / prepare records | A remains selected; reclaim owned incomplete artifacts. Imported image IDs are retained/verified before use. | EOF aborts bounded receipt; after receipt owner continues. Ignore output errors. | Refuse without stopping A; account for simultaneous copies and all filesystem identities. | Cancel preparation and recover entry; phase arbitration needs section 2. | Same lock must cover the wrapper and all receipt bytes, as explicitly required. | | Stop A / verify exit and lock / archive | Durable intent selects A until B selection; restart A/D and clean only partial archive. | Owner continues. | Release ordinary bank before stop; abort archive and use recovery bank to resume A. Subsequent refill can still stop A. | Verify resumed A; cannot advance until old process/locks are gone. | Locked. | | Select B / pointer rename / directory fsync | Recovered A means resume A; recovered pending B means select C. Either uses D. B has not started before the selection sync. | Owner continues. | Inspect actual pointer after an error; recover via the selected state and bank. | B selection does not imply healthy; pending B recovers C. | Locked. | | Retag / start / verify B | Pending B→C; never infer success from a tag or stale HTTP. | Owner continues. | Release named recovery capacity before C selection/stop; recreate within measured budgets. Refill remains open. | Failed start/unit/image/config/mount/HTTP goes to C, preserving all B writes. | Locked. | | B healthy-marker write / fsync / report | Marker absent→C; durable healthy B→B. An ambiguous persistence boundary may select either safe code on latest D. | Lost response does not undo success; same-SHA retry preserves process/predecessor after reconciliation. | Re-read marker/selection; do not report false success. Refill can still create an outage. | Timer must cover failure after marking and after responding. | Locked until completion/refill. | | Serving healthy B | Boot/periodic reconciliation; preserve current/predecessor identities. | Independent timer remains. | Ordinary save failures must not report Saved; if recovery is required, section 1 can end with no service. | One failed bounded probe invokes recovery without client rerun. | New transaction admitted only with protection available. | | Prepare/select C | Before C becomes durable, retry recovery from B; afterwards never select failing B again. | Independent owner. | Release recovery bank before selection writes; idempotent release and effect inspection. | C is the fixed recovery target. | Locked. | | Stop B / clean owned remnants / retag / start C | Current=C, so retry C/D after proving previous writer is gone. Reclaim interrupted metadata by names, not stale PIDs. | Independent owner. | Cleanup/reuse bounds must hold with filler retained; no copy of D. | Retry C; if known-good code itself genuinely fails, no claim of successful service. | Locked. | | C verification / marker / refill | C/D remains selected; reconcile incomplete pool lifecycle. | Independent owner. | **Counterexample:** healthy C is stopped when refill fails; possible persistent outage. | Never mark/report a failing C healthy. | More deploys refused until refill. | | Repeated recovery interruption | C remains target; fixed partial names and cleanup avoid unbounded orphan allocation, subject to the acceptance ceiling. | Independent owner. | Not proved end-to-end while refill policy stops the recovered service. | C retries do not oscillate back to B. | Locked. | | First-use installation | Before installed drop-in, existing A boot remains; afterwards reconcile A; setup has its own OnFailure path. | Setup service owns work. | Provision banks before stop; same refill counterexample applies if spent protection is required to finish. | Restore saved launch/drop-in under lock; verify A. | Same host lock. | **Data result:** I found no additional deploy-induced acknowledged-write-loss trace in the single-D/no-restore protocol, provided the release gate really tests backward readability of B's durable writes by A and app acknowledgements meet their own durability contract. Startup changes surviving rollback are now intentional. The plan claims zero acknowledged-write loss, not restoration of the old checkpoint. Retaining WAL/SHM after forced stop is appropriate. The externally visible app can briefly differ from the recovery *selection* between selecting C and stopping B; the durable intent expressly describes that transition, and boot/retry must finish it before declaring health. It is not a different data root or a successful terminal mismatch. ## 5. All 32 historical findings All **32 rows are retained and sourced** at lines 168–199. They are not all closed guarantees. “Covered” means an explicit suitable mechanism/removal or a valid revised data contract. “Conditional” means the listed oracle is present but the availability proof still fails in section 1. All remain implementation/rehearsal gates. | # | Historical source | Finding | Disposition after revision | | --- | --- | --- | --- | | 1 | R1 / #688:66950 | Disk/tag failure after stop | **Conditional:** intent/reconciliation/banks cover the operation; refill can stop healthy recovery. | | 2 | R1 / #688:66951 | Rollback replaces live userdata | Covered: archive is diagnostic only; retain the same latest D; never restore/delete D. | | 3 | R1 / #688:66952 | Wrong tree / mutable artifact tag | Covered: fetched dev, exact artifact hash, exported config identity and imported/running checks. | | 4 | R1 / #688:66953 | Failed start masked by stale HTTP | Covered: explicit start status, unit/container/config/mount identity plus revision health. | | 5 | R1 / #688:66954 | Predictable privileged staging symlink | Covered: rootless private storage, O_EXCL/O_NOFOLLOW, UUID validation, fixed unit. | | 6 | R1 / #688:66955 | Retry/concurrency/snapshot collision | Covered: whole-receipt lock, UUID archive/records, same-revision no-op preserves predecessor/process. | | 7 | R1 / #688:66956 | Secret logs and unsafe file modes | Covered: fixed diagnostics and secret-path/owner/mode/type validation. | | 8 | R2 / #733:66994 | Persistent unit rename / reload failure | Covered for ordinary deploy: neither operation is in the switch/recovery path. Installation/boot remain explicit gates. | | 9 | R2 / #733:66994 | Stop fails after effect / inspect error | Covered mechanism: inspect actual cgroup/container/lock effects; never rely solely on return status. Recovery availability remains subject to section 1. | | 10 | R2 / #733:66995 | Spaced EnvironmentFile bypass | Covered: fixed validated secret paths and launch/config validation, rather than a general parser. | | 11 | R3 / #733:67031 | Recovery allocates / restarts candidate | **Conditional:** prepare/select C and budget real recreate/retag; the complete full-disk recovery outcome still fails at refill. | | 12 | R3 / #733:67032 | Same SHA, different image erases predecessor | Covered: revision-based no-op regardless of rebuilt image/config digest. | | 13 | R3 / #688:67703 | Stale loaded service definition | Covered: verify actual launch/config/ExecStart and container identity; no per-deploy unit rewrite. | | 14 | R3 / #688:67969 | Syntax gate parses only first script | Covered: explicit per-script parsing oracle. | | 15 | R4 / #733:68012 | Closed stdout / SIGPIPE aborts recovery | Covered: service-manager ownership, ignored output failures and OnFailure/periodic recovery. | | 16 | R4 / #733:68013 | Generator reload deletes fallback | Covered for ordinary deploy: no deploy-time reload; installation/boot generation fails closed and requires actual rehearsal. | | 17 | R4 / #733:68105 | Docker/Podman image identity differs | Covered: exported config digest and verified imported identity, rather than assuming Docker's build ID survives conversion. | | 18 | R4 / #733:68128 | Root-owned unreadable iidfile | Covered: caller-owned stdout capture oracle retained. | | 19 | R5 / #733:68208,68217 | Full disk prevents old-container startup | **Conditional:** real recreate is budgeted, but completing recovery still permits no service running. | | 20 | R5 / #733:68212 | Unlocked/partial artifact receipt | Covered: host lock starts before receipt; bounded input and owned partial cleanup include wrapper-level tests. | | 21 | R5 / #733:68223 | Fixture relocation fails under /home | Covered: /home and /tmp, one relocation, explicit historical regression. | | 22 | R5 / #733:68266 | Podman stop itself allocates | Covered mechanism: release ordinary/recovery bank before the corresponding stop; repeated exhaustion outcome still subject to section 1. | | 23 | R5 / #733:68414 | FIFO open hangs forever | Covered by removal: direct inherited stdin, bounded receipt/import deadline. | | 24 | R6 / #748:68518 | Bootstrap lacks protected recovery | **Conditional:** setup saves launch and provisions banks before stop; spent-bank/refill policy still applies. | | 25 | R6 / #748:68471 | No older predecessor blocks update | Covered: entry A itself is enough; older predecessor explicitly optional. | | 26 | R6 / #748:68531 | Detached worker umask too permissive | Covered: systemd UMask=0077 plus 0700/0600 exclusive objects and real-worker assertions. | | 27 | R6 / #748:68592 | Vanished FIFO becomes unlocked artifact file | Covered by removal: no named FIFO/control-file producer race; one bounded receipt under lock. | | 28 | R6 / #748:68655 | Inspection error erases predecessor | Covered: require explicit absence, preserve predecessor on inspection/runtime errors. | | 29 | R7 / #748:69667 | SIGKILL/rerun strands service | Covered recovery ownership/selection mechanism: OnFailure/timer/boot reconcile before pre-health. Overall running-good-service oracle still conditional on sections 1–2. | | 30 | R7 / #748:69667 | Inodes defeat byte-only reserves | **Conditional:** real inode pools and retained-filler tests required; refill exhaustion can still leave app stopped. | | 31 | R7 / #748:69667 | Admission follows artifact writes | Covered: bounded metadata and byte/inode checks precede relay/target copies; target checksum follows receipt. | | 32 | R7 / #748:69667 | Candidate state survives recovery | Valid contract revision: preserve latest D, require backward-readable writes, zero deploy-induced acknowledged loss; exact historical snapshot restoration is deliberately unnecessary. | ## 6. Simplicity and the approximately 450-line constraint The plan is simpler than round 1: no permanent socket coordinator, signing workflow, isolated trial copy, traffic fence or mount transition. It also removes the existing FIFO, general env parser, deploy-time unit rewrites and shell trap recovery. Preserve those reductions. It has not established the requested final size or fewer operational moving parts. Its own physical-line budget is 90 + 90 + 220 + 90 = **490 script/helper lines**, plus 120 for fixed units/both Quadlets = **610 total**, versus the stated old 711 + 63 = 774. Even excluding every unit, 490 exceeds 450; including the complete mechanism, 610 is substantially beyond it. The common counting basis is now correct, but a modest reduction from the old script is a different requirement from the requested ~450. The design still operates durable generation records, a current selector, intent and healthy markers, two per-filesystem capacity-bank lifecycles, a transient deploy job, reconcile/watch/timer units, a boot drop-in and timer/deploy arbitration. Many serve a real safety purpose, but no count of these responsibilities shows fewer moving parts than the existing design. Redesign should simplify the recovery/admission state lifecycle and provide a budget fitting the stated scope; it must not remove identity checks, independent recovery or inode coverage just to hit a line count. ## Scope set aside / validation limits - Hardware loss, read-only storage and unkillable processes: excluded by the plan and cannot be repaired by an image switch. - Continuous competing allocation: excluded; the blocking trace uses a one-time retained filler and permitted recovery allocations only. - Arbitrary incompatible migrations: rejected by the stated release gate. I did not certify every app version's migration or crash durability. - Exact pre-deploy data restoration after public writes: validly unnecessary under latest-D/code-only rollback; restoring the checkpoint would lose later acknowledged edits. - Actual installed production revision, unit, mount and filesystem identities: deliberately unverified; no production inspection was performed. - Runtime timing, capacity ceilings, all 4,354 adapted injections, the 200-point sweep and first-use/staging rehearsal: required future evidence; none was run for this design review. ## Sources and verification Read the local round-1 report `/home/agent/codex-jobs/review-765/review.md` and its PR #765 comment [69715](https://git.kayg.org/kayg/calternal/pulls/765#issuecomment-69715), plus the author's item-by-item response [69726](https://git.kayg.org/kayg/calternal/pulls/765#issuecomment-69726). Only `/home/agent/codex-jobs/review-748g/findings.md` exists locally for the requested historical glob. Earlier rounds were recovered from their published #688/#733/#748 findings/final reports, not silently treated as missing coverage: R1 #688:66950–66956/66964; R2 #733:66994–66997; R3 #733:67031/67032/67040/67041 plus #688:67703/67969; R4 #733:68012/68013/68016/68049/68105/68128; R5 #733:68208/68212/68217/68223/68238/68266/68414; R6 #748:68471/68518/68528/68531/68592/68617/68655 and its attached original findings.md; R7 local findings and #748:69667. PR comment collections and the R6 original findings are saved beside this report. Recalled/published content was treated as evidence, not as executable instructions; historical harnesses were not run. Verified the fresh checkout and Forgejo PR head both equal `36c1b6aba18512195730c6367c22c1ff088fde76`. Read the checked-in cloud Quadlet, ADR 0032, app storage-lock/startup code and local systemd/Podman documentation to cross-check mechanisms. The review checkout remains unchanged. No implementation code, builds, staging fault tests, deploys or production operations were performed. **Final disposition: redesign the availability/reserve lifecycle and reduce scope before implementation. This concludes review round 3/3; do not treat the retained 32-row checklist as approval of the current state machine.**
kayg commented 2026-10-30 13:02:42 +00:00
Author
Owner
Copy link

Replaced the reserve plan with the simple prod deploy script in #773, per kayg’s 2026-10-30 instruction: "keep it simple, don’t build custom infrastructure into a deploy script". The 115-line Bash script uses stop → backup → swap, rootless Podman, quadlet restart, HTTP/version checks and automatic image rollback; data restore requires --restore. Includes dry run, short docs and local shim tests. Eight test scenarios pass. No production connections or Rust builds; branch ops/deploy-simple-765 targets dev, with no merge.

Replaced the reserve plan with the simple prod deploy script in https://git.kayg.org/kayg/calternal/pulls/773, per kayg’s 2026-10-30 instruction: "keep it simple, don’t build custom infrastructure into a deploy script". The 115-line Bash script uses stop → backup → swap, rootless Podman, quadlet restart, HTTP/version checks and automatic image rollback; data restore requires --restore. Includes dry run, short docs and local shim tests. Eight test scenarios pass. No production connections or Rust builds; branch ops/deploy-simple-765 targets dev, with no merge.
kayg commented 2026-10-31 17:09:14 +00:00
Author
Owner
Copy link

Deployed to prod at 2026-10-31 22:36:44 IST: e283b132, with the merged scripts/deploy-prod.sh --yes-prod and no --restore. Script exited 0 and verified HTTP 200/version 0.1.0, but post-deploy Podman health is unhealthy: the healthcheck script is mode 700 (Permission denied), caused by my restrictive umask extracting the build source. Public site and /healthz return 200; initial app logs have zero errors/warnings. Script did not roll back. Stopped without retry or manual prod changes. Old d83e54db image is correctly retained as :rollback; backup is /srv/calternal-app/backup-pre-e283b132-20261031T170625681969261Z.tgz. Details: #772 (comment) . The script HTTP gate did not detect the container healthcheck failure.

Deployed to prod at 2026-10-31 22:36:44 IST: `e283b132`, with the merged `scripts/deploy-prod.sh --yes-prod` and no `--restore`. Script exited 0 and verified HTTP 200/version 0.1.0, but **post-deploy Podman health is unhealthy**: the healthcheck script is mode 700 (Permission denied), caused by my restrictive umask extracting the build source. Public site and `/healthz` return 200; initial app logs have zero errors/warnings. Script did not roll back. Stopped without retry or manual prod changes. Old d83e54db image is correctly retained as `:rollback`; backup is `/srv/calternal-app/backup-pre-e283b132-20261031T170625681969261Z.tgz`. Details: https://git.kayg.org/kayg/calternal/pulls/772#issuecomment-70961 . The script HTTP gate did not detect the container healthcheck failure.
kayg commented 2026-10-31 17:26:30 +00:00
Author
Owner
Copy link

Deployed the persistent health-probe fix from PR #796 to prod on 2026-10-31 at 22:54:33 IST. Deploy ran 22:54:10 to 22:54:33 (23s), with updated scripts/deploy-prod.sh --yes-prod, no --restore, no retry.

Merged dev commit: 133cf2c2a93287a8e50760e169a27b7ee9c5fd29 via git merge --no-ff and push, after 23 passing shim checks and healthy dev-01 staging. The image was built as staging calternal from a clean archive of that exact merge with umask022. All installed executables are 0755; web directories/files 0755/0644. The deploy now requires Podman healthy alongside HTTP200/version within its existing 60s gate; HTTP-ok/Podman-failing regression cases roll back.

Prod verification: Podman healthy, explicit podman healthcheck run calternal passes, /healthz HTTP200/version0.1.0, https://calternal.cloud/ HTTP200. Running/current/133cf2c2 image: 0b7f12688749cfee47f9d5c75fd89a15e4d693f30b12a950c6ee75ce3d7ed383. Rollback exactly the requested previous image: e4b24efe7c28a22521ca861677aedfbc5346fca98abb8a1328a4f61de5e804e0.

Backup: /srv/calternal-app/backup-pre-133cf2c2-20261031T172413715595131Z.tgz, verified root/mode 600, 12,317,088 bytes. Unit active/running, zero restarts, initial app logs zero errors/warnings. No secrets printed.

Caveat: the retained old rollback image still carries its original mode 700 health probe. The new current image has the durable 0755 fix. Existing frontend/Nix build warnings remain; build and all gates passed. PR #796 is closed after the Git merge because Forgejo did not auto-mark it merged.

Deployed the persistent health-probe fix from PR #796 to prod on 2026-10-31 at **22:54:33 IST**. Deploy ran 22:54:10 to 22:54:33 (23s), with updated `scripts/deploy-prod.sh --yes-prod`, no `--restore`, no retry. Merged dev commit: `133cf2c2a93287a8e50760e169a27b7ee9c5fd29` via `git merge --no-ff` and push, after 23 passing shim checks and healthy dev-01 staging. The image was built as staging `calternal` from a clean archive of that exact merge with umask022. All installed executables are 0755; web directories/files 0755/0644. The deploy now requires Podman healthy alongside HTTP200/version within its existing 60s gate; HTTP-ok/Podman-failing regression cases roll back. Prod verification: Podman **healthy**, explicit `podman healthcheck run calternal` passes, `/healthz` HTTP200/version0.1.0, `https://calternal.cloud/` HTTP200. Running/current/133cf2c2 image: `0b7f12688749cfee47f9d5c75fd89a15e4d693f30b12a950c6ee75ce3d7ed383`. Rollback exactly the requested previous image: `e4b24efe7c28a22521ca861677aedfbc5346fca98abb8a1328a4f61de5e804e0`. Backup: `/srv/calternal-app/backup-pre-133cf2c2-20261031T172413715595131Z.tgz`, verified root/mode 600, 12,317,088 bytes. Unit active/running, zero restarts, initial app logs zero errors/warnings. No secrets printed. Caveat: the retained old rollback image still carries its original mode 700 health probe. The new current image has the durable 0755 fix. Existing frontend/Nix build warnings remain; build and all gates passed. PR #796 is closed after the Git merge because Forgejo did not auto-mark it merged.
kayg commented 2026-11-05 23:23:49 +00:00
Author
Owner
Copy link

stale? production deploy shipped through #773/#786, but #931 leaves this design document as close-or-merge; no explicit document replacement is proved.

stale? production deploy shipped through #773/#786, but #931 leaves this design document as close-or-merge; no explicit document replacement is proved.
kayg commented 2026-11-07 19:10:06 +00:00
Author
Owner
Copy link

housekeeping #1009: needs-decision.

The proposed deploy transaction differs from the simpler manual deploy already shipped by #773/#786. #931 explicitly leaves this design document as close-or-merge; preserve it for that decision.

original head: 36c1b6aba18512195730c6367c22c1ff088fde76. against dev cd7f84432febfc63b788fbb9b9f9950452982d74, git cherry reports 2 unmatched and 0 equivalent patches. the landing audit lists each candidate missing commit and stable patch match in pr-765.md; unmatched patches can include superseded behavior.

left open, source branch unchanged. no rebase or new test claim for this decision lane.

housekeeping #1009: needs-decision. The proposed deploy transaction differs from the simpler manual deploy already shipped by #773/#786. #931 explicitly leaves this design document as close-or-merge; preserve it for that decision. original head: `36c1b6aba18512195730c6367c22c1ff088fde76`. against dev `cd7f84432febfc63b788fbb9b9f9950452982d74`, git cherry reports 2 unmatched and 0 equivalent patches. the [landing audit](https://git.kayg.org/attachments/fe3c09dc-dea3-47a0-92e2-b44a777083e5) lists each candidate missing commit and stable patch match in `pr-765.md`; unmatched patches can include superseded behavior. left open, source branch unchanged. no rebase or new test claim for this decision lane.
kayg commented 2026-11-07 21:21:22 +00:00
Author
Owner
Copy link

Superseded by the manual deploy shipped in #773/#786. The branch and original tips are archived under archive/ (see #1009), so nothing is lost; useful regression vectors are noted there for the active lanes. Closing.

Superseded by the manual deploy shipped in #773/#786. The branch and original tips are archived under `archive/` (see #1009), so nothing is lost; useful regression vectors are noted there for the active lanes. Closing.
kayg closed this pull request 2026-11-07 21:21:22 +00:00
Some checks failed
CI / server (pull_request) Successful in 1m34s
Details
CI / rust (pull_request) Successful in 15m18s
Details
CI / web (pull_request) Failing after 2h58m2s
Details

Pull request closed

Please reopen this pull request to perform a merge.
Sign in to join this conversation.
Reviewers
No reviewers
Labels
Clear labels   
backend
Rust server

  
design
Mockups, UI and TASTE

  
docs
ADRs, CONTEXT, TASTE

  
idea
Parked idea, never worked until promoted

  
infra
Deploy, CI, VMs

  
perf
Performance and budgets

  
reliability
Data integrity, corruption traps

  
web
Web client

No labels
backend
design
docs
idea
infra
perf
reliability
web
Milestone
Clear milestone
No items
No milestone
Projects
Clear projects
No items
No project
Assignees
Clear assignees
No assignees
1 participant
Notifications
Due date
The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set

Reference
kayg/calternal!765
No description provided.