crueber/walhub

Watch

Fork 0

Fork failure-path residue: orphan checkpoints wedge retry; rollback may delete adopted access.json #458

New issue

Closed

opened 2026-09-13 14:19:20 +00:00 by crueber · 3 comments

crueber commented

2026-09-13 14:19:20 +00:00

Owner

Child of #449 (audit findings F4 Med comment 4559 + F5 Low comment 4561). (1) A crashed fork attempt orphans checkpoint keys (ShareManifest Creates checkpoint pair at forkexec.go:173,193 before child manifest at :218); retry 412s on its own leftovers and 409s permanently (merge.go:778-793 adopts only with matching fork.json) — distinct from #432's share/access stranding. (2) RollbackShare proves manifest ownership (Repo+Revision 1, forkexec.go:291-293) but deletes access.json unconditionally unless fork.json disputes the prefix (:270-276) — an adopted pre-existing access.json (EnsureRepoAccess adopt-dont-overwrite, merge.go:829-835) can be deleted by a rollback that didn't create it. Fix: crash-safe checkpoint adoption-or-sweep on retry + ownership proof for access.json before rollback deletes it.

crueber referenced this issue

2026-09-13 14:19:25 +00:00

Audit: fork feature vs git directly — clone/fetch/push, object completeness, ref copying, parent deletion/GC, provenance (findings as comments) #449

crueber referenced this issue from a commit

2026-09-13 16:32:36 +00:00

Fix #458: fork failure-path residue — orphan adoption + access ownership

crueber referenced this issue from a pull request that will close it,

2026-09-13 16:32:38 +00:00

Fix #458: failure-path residue #469

crueber commented

2026-09-13 16:32:42 +00:00

Author

Owner

Fixed by PR #469 (#469): crash-safe orphan adoption + rollback access.json ownership proof. Not merging per instructions — review requested.

Fixed by PR #469 (https://git.packden.us/crueber/walhub/pulls/469): crash-safe orphan adoption + rollback access.json ownership proof. Not merging per instructions — review requested.

crueber commented

2026-09-13 16:37:10 +00:00

Author

Owner

Review: PR #469 (fix/issue-458) — APPROVED, ready to merge

Adversarial adoption-safety review + full verification in scratch worktree /tmp/pr469 (main worktree untouched, still clean on main). No browser needed: backend-only change (internal/pulls, internal/identity, docs); no browser-facing surface touched.

(1) Adopt-or-conflict semantic compare — SOUND

sameForkSnapshot (forkexec.go:476) compares Seq/ObjectFormat/HeadTarget/Refs(name/oid/peeled); sameForkCheckpoint (:498) compares Seq/ObjectFormat/RefsKey/RefCount/packs. Timestamps/writer ignored is correct (retries re-stamp). "Same attempt" is really content-equality, which is the right criterion: adopting byte-identical content cannot corrupt anyone. Any content difference (rival refs, different packs/order, different HeadTarget/seq) → 409, rival bytes verified untouched by test. Footnote (non-blocking): Checkpoint.BundleKey is not compared — inert, fork-share never sets it, and the child manifest's checkpoint-ref only records Seq+Key anyway.

(2) Orphan-manifest adoption proof — STRONG ENOUGH, fails closed where it must

adoptOrphanManifest (forkexec.go:539) requires: no fork.json (absence proven by GET; any non-404 → conflict), Repo==child, Revision==1, HeadSeq==parent seq, MinSeq==HeadSeq+1, ObjectFormat+packs verbatim, checkpoint-ref Seq+Key match. Racing repo create excluded (fresh repo HeadSeq 0 ≠ nonzero parent seq; WAL-advanced excluded by Rev check). Empty-parent orphan → conflict before any read (:557, the #432 hijack case stays closed). Moved-parent → dedicated 409 naming stale seq vs current (:565). All 12 fail-closed subtests pin this.

(3) Sweep rejection — JUSTIFIED

Unmanifested occupant is unowned garbage to us but a live reservation to a racing rival adopting by the same rule; deleting would corrupt the rival's retry. Law 4 (doubt keeps objects, loser 409s) applied correctly.

(4) EnsureRepoAccessCreated — CORRECT, seam-clean

Single Create as proof, no probe (creategate.go:169-204); 412/quirk → adopted(false). internal/api/placeholder.go untouched — its own narrow interface is still satisfied by the legacy wrapper (:156), go build ./... confirms. Flag threads all paths in runFork (merge.go:887-908): nil boot → false (nothing written, nothing to delete); bootstrap error → false; adopted share → sharedThisAttempt false → no rollback at all.

(5) Rollback cannot delete adopted access.json — PROVEN

Delete requires accessCreated && !disputed (forkexec.go:326); accessCreated is true only if this runFork won the Create. Manifest ownership proof (Repo+Rev1, :319) retained verbatim; delete order (access, checkpoints, manifest last) unchanged. Known benign residual (safe direction, not blocking): crash after bootstrap-Create but before fork.json → retry adopts (false) → later rollback spares the crashed attempt's own doc; harmless, re-adopted, never a blocker.

(6) #432 no-regress — HOLDS

Rollback ownership, stranded-name retry convergence, prefix scoping, dispute downgrade all still pinned by fork432_test.go (only signature updates RollbackShare(..., bool)); full package green.

(7) Verification results (scratch worktree, generous timeouts)

gofmt -l clean; go vet pulls+identity clean; go.mod untouched (no new deps)
go test -race -count=1 ./internal/pulls/... ./internal/identity/... — both ok
coverage: pulls 96.1%, identity 95.6% — claims exact, ≥95 gate holds
TestE2E_CollabFullChain (fork → cross-fork PR) green; go build ./... green
blanket internal/e2e shows one failure (GET /setup = 500, ui shell missing) — environmental only: scratch worktree has no built web/dist/; unrelated to this PR (no web/ changes)
docs: 03 decisions appended, law-6/law-4/GC claims verified accurate against the code (GETs only on 412 path; pre-commit keys never indexed)

No fixes needed — nothing pushed. MERGE RECOMMENDATION: ready to merge.

## Review: PR #469 (fix/issue-458) — APPROVED, ready to merge Adversarial adoption-safety review + full verification in scratch worktree `/tmp/pr469` (main worktree untouched, still clean on main). No browser needed: backend-only change (`internal/pulls`, `internal/identity`, docs); no browser-facing surface touched. ### (1) Adopt-or-conflict semantic compare — SOUND `sameForkSnapshot` (`forkexec.go:476`) compares Seq/ObjectFormat/HeadTarget/Refs(name/oid/peeled); `sameForkCheckpoint` (`:498`) compares Seq/ObjectFormat/RefsKey/RefCount/packs. Timestamps/writer ignored is correct (retries re-stamp). "Same attempt" is really **content-equality**, which is the right criterion: adopting byte-identical content cannot corrupt anyone. Any content difference (rival refs, different packs/order, different HeadTarget/seq) → 409, rival bytes verified untouched by test. Footnote (non-blocking): `Checkpoint.BundleKey` is not compared — inert, fork-share never sets it, and the child manifest's checkpoint-ref only records Seq+Key anyway. ### (2) Orphan-manifest adoption proof — STRONG ENOUGH, fails closed where it must `adoptOrphanManifest` (`forkexec.go:539`) requires: no `fork.json` (absence proven by GET; any non-404 → conflict), Repo==child, Revision==1, HeadSeq==parent seq, MinSeq==HeadSeq+1, ObjectFormat+packs verbatim, checkpoint-ref Seq+Key match. Racing repo create excluded (fresh repo HeadSeq 0 ≠ nonzero parent seq; WAL-advanced excluded by Rev check). Empty-parent orphan → conflict before any read (`:557`, the #432 hijack case stays closed). Moved-parent → dedicated 409 naming stale seq vs current (`:565`). All 12 fail-closed subtests pin this. ### (3) Sweep rejection — JUSTIFIED Unmanifested occupant is unowned garbage to us but a live reservation to a racing rival adopting by the same rule; deleting would corrupt the rival's retry. Law 4 (doubt keeps objects, loser 409s) applied correctly. ### (4) EnsureRepoAccessCreated — CORRECT, seam-clean Single Create as proof, no probe (`creategate.go:169-204`); 412/quirk → adopted(false). `internal/api/placeholder.go` **untouched** — its own narrow interface is still satisfied by the legacy wrapper (`:156`), `go build ./...` confirms. Flag threads all paths in `runFork` (`merge.go:887-908`): nil boot → false (nothing written, nothing to delete); bootstrap error → false; adopted share → `sharedThisAttempt` false → no rollback at all. ### (5) Rollback cannot delete adopted access.json — PROVEN Delete requires `accessCreated && !disputed` (`forkexec.go:326`); `accessCreated` is true only if this runFork won the Create. Manifest ownership proof (Repo+Rev1, `:319`) retained verbatim; delete order (access, checkpoints, manifest last) unchanged. Known benign residual (safe direction, not blocking): crash after bootstrap-Create but before fork.json → retry adopts (false) → later rollback spares the crashed attempt's own doc; harmless, re-adopted, never a blocker. ### (6) #432 no-regress — HOLDS Rollback ownership, stranded-name retry convergence, prefix scoping, dispute downgrade all still pinned by `fork432_test.go` (only signature updates `RollbackShare(..., bool)`); full package green. ### (7) Verification results (scratch worktree, generous timeouts) - `gofmt -l` clean; `go vet` pulls+identity clean; `go.mod` untouched (no new deps) - `go test -race -count=1 ./internal/pulls/... ./internal/identity/...` — both ok - coverage: pulls **96.1%**, identity **95.6%** — claims exact, ≥95 gate holds - `TestE2E_CollabFullChain` (fork → cross-fork PR) green; `go build ./...` green - blanket `internal/e2e` shows one failure (`GET /setup = 500, ui shell missing`) — **environmental only**: scratch worktree has no built `web/dist/`; unrelated to this PR (no web/ changes) - docs: 03 decisions appended, law-6/law-4/GC claims verified accurate against the code (GETs only on 412 path; pre-commit keys never indexed) No fixes needed — nothing pushed. **MERGE RECOMMENDATION: ready to merge.**

crueber closed this issue

2026-09-13 16:37:37 +00:00

crueber commented

2026-09-13 16:37:37 +00:00

Author

Owner

Fixed by PR #469 (review clean — all 7 adoption-safety checks pass, #432 no-regress holds), merged. Closing.

crueber referenced this issue from a commit

2026-09-13 16:37:38 +00:00

Merge pull request 'Fix #458: failure-path residue' (#469) from fix/issue-458 into main

crueber referenced this issue

2026-09-13 17:22:04 +00:00