forkChain handle-lifetime cache goes stale on Root backfill #459

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 finding F6 Low, comment 4562). forkread.go:44-57 resolves the ancestry chain once per handle and caches it, justified by 'fork.json parent pointer never moves' — but merge.go:871-896 CAS-updates fork.json Root after create (adopted-provenance backfill), a field the chain is built from (:98-99). Impact is degraded-only (Parent never moves so fallback still terminates) but a stale Root skips the short-circuit. Fix: invalidate/re-resolve the cached chain when fork.json version advances, or document the degraded window.

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:42:15 +00:00

Fix #459: fork-chain cache revalidates on fork.json advance

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

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

Fix #459: chain cache staleness #470

crueber commented

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

Author

Owner

Fixed by PR #470 (#470): handle caches own Parent+Root+Version with the chain and revalidates per use (one exact-key GET, failure path only); Root backfill now bumps Version. Regression test fails on old code, passes on fix; -race + coverage gates hold.

Fixed by PR #470 (https://git.packden.us/crueber/walhub/pulls/470): handle caches own Parent+Root+Version with the chain and revalidates per use (one exact-key GET, failure path only); Root backfill now bumps Version. Regression test fails on old code, passes on fix; -race + coverage gates hold.

crueber commented

2026-09-13 16:53:31 +00:00

Author

Owner

Review of PR #470 (9e4ef96, fix/issue-459) — verified in scratch worktree /tmp/pr470 (main worktree untouched, still clean on main).

(1) Revalidation placement — PASS. All forkChain entry points live in wal's miss/fallback machinery: sharedGet (forkread.go:159, own GET first, chain only after own-NotFound), downloadShared known-size branch (:182-200, ancestors only after own-NotFound), fetchPackFile/fetchSideFile/buildRemoteIndex/remoteReaderFor (reconcile.go:210,244,380,452 — cold-sync/fetch paths). Nothing on the push/publish, refs-serve, or checkpoint paths, so the law-6 budgeted hot paths pay zero. Two precision notes (non-blocking): unknown-size downloadShared (:201) builds ancestorKeys before the own HEAD, and engineFor (reconcile.go:409) wires prefixes during remote-reader setup — neither waits for an own-NotFound, but both sit on multi-trip sync/fetch paths, not budgeted hot paths. The doc's 'failure path only' line is accurate in spirit.

(2) Version bump — PASS, complete. Create sets Version 1 (merge.go:915); backfill bumps only on a real Root change, already-correct docs untouched / no churn (:964-970); #457 stamp bumps (:391). Every fork.json write path now advances Version, so the Parent+Root+Version compare converges on all advances.

(3) forkMu discipline — PASS. Leaf lock confirmed: taken only inside forkChain (forkread.go:48,56,68), never nested with syncMu/packMu/rw; never held across a store call (snapshot fields under lock, unlock, GET+compare outside, store result under lock). Concurrent resolvers stay idempotent, last write wins. Matches the 13-concurrency note in the doc decision.

(4) Stale window — PASS, fully closed. Every use revalidates against the live own fork.json; a mid-handle backfill is caught on the very next use (no window past one call). TestForkChainRootBackfillInvalidates proves it: stale [f/c] pre-backfill, Root short-circuit to a/b post-backfill, plus a Version-only-stamp stability check.

(5) Failure modes — acceptable (fail-closed direction). One observation, not blocking: readForkDoc collapses transport errors into ok=false, so a transient GET failure on revalidation overwrites a good cached chain with empty for exactly one call (self-heals on the next use; ancestor GETs would likely fail in the same outage anyway). Empty chain = fallback disabled = own-miss surfaces verbatim — closed, not stale. Optional follow-up: distinguish IsNotFound from transport errors in the revalidate path and serve stale on the latter.

(6) Gates — PASS. go test ./internal/wal/ ./internal/pulls/ -race -count=1 green; coverage wal 95.2% / pulls 96.1% (>=95 holds); gofmt clean; go vet clean; go build ./... clean; go.mod/go.sum untouched (no new deps); docs/features/03_pull_requests.md decision appended in-change (law 12 OK); law 8 intact (wal's forkDoc mirror extended, no upward import). New test fails on base code (stale [f/c], verified by overlaying main's forkread.go/handle.go) and passes on the fix. No browser needed: no browser-facing surface touched (store/cache/doc/test-only change); noted explicitly.

MERGE RECOMMENDATION: ready to merge.

Review of PR #470 (9e4ef96, fix/issue-459) — verified in scratch worktree /tmp/pr470 (main worktree untouched, still clean on main). (1) Revalidation placement — PASS. All forkChain entry points live in wal's miss/fallback machinery: sharedGet (forkread.go:159, own GET first, chain only after own-NotFound), downloadShared known-size branch (:182-200, ancestors only after own-NotFound), fetchPackFile/fetchSideFile/buildRemoteIndex/remoteReaderFor (reconcile.go:210,244,380,452 — cold-sync/fetch paths). Nothing on the push/publish, refs-serve, or checkpoint paths, so the law-6 budgeted hot paths pay zero. Two precision notes (non-blocking): unknown-size downloadShared (:201) builds ancestorKeys before the own HEAD, and engineFor (reconcile.go:409) wires prefixes during remote-reader setup — neither waits for an own-NotFound, but both sit on multi-trip sync/fetch paths, not budgeted hot paths. The doc's 'failure path only' line is accurate in spirit. (2) Version bump — PASS, complete. Create sets Version 1 (merge.go:915); backfill bumps only on a real Root change, already-correct docs untouched / no churn (:964-970); #457 stamp bumps (:391). Every fork.json write path now advances Version, so the Parent+Root+Version compare converges on all advances. (3) forkMu discipline — PASS. Leaf lock confirmed: taken only inside forkChain (forkread.go:48,56,68), never nested with syncMu/packMu/rw; never held across a store call (snapshot fields under lock, unlock, GET+compare outside, store result under lock). Concurrent resolvers stay idempotent, last write wins. Matches the 13-concurrency note in the doc decision. (4) Stale window — PASS, fully closed. Every use revalidates against the live own fork.json; a mid-handle backfill is caught on the very next use (no window past one call). TestForkChainRootBackfillInvalidates proves it: stale [f/c] pre-backfill, Root short-circuit to a/b post-backfill, plus a Version-only-stamp stability check. (5) Failure modes — acceptable (fail-closed direction). One observation, not blocking: readForkDoc collapses transport errors into ok=false, so a transient GET failure on revalidation overwrites a good cached chain with empty for exactly one call (self-heals on the next use; ancestor GETs would likely fail in the same outage anyway). Empty chain = fallback disabled = own-miss surfaces verbatim — closed, not stale. Optional follow-up: distinguish IsNotFound from transport errors in the revalidate path and serve stale on the latter. (6) Gates — PASS. go test ./internal/wal/ ./internal/pulls/ -race -count=1 green; coverage wal 95.2% / pulls 96.1% (>=95 holds); gofmt clean; go vet clean; go build ./... clean; go.mod/go.sum untouched (no new deps); docs/features/03_pull_requests.md decision appended in-change (law 12 OK); law 8 intact (wal's forkDoc mirror extended, no upward import). New test fails on base code (stale [f/c], verified by overlaying main's forkread.go/handle.go) and passes on the fix. No browser needed: no browser-facing surface touched (store/cache/doc/test-only change); noted explicitly. MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-13 16:55:33 +00:00

crueber commented

2026-09-13 16:55:33 +00:00

Author

Owner

Fixed by PR #470 (review clean — failure-path-only placement, lock discipline, stale window closed with fail-on-base test), merged. Closing.

crueber referenced this issue from a commit

2026-09-13 16:55:34 +00:00

Merge pull request 'Fix #459: chain cache staleness' (#470) from fix/issue-459 into main

crueber referenced this issue

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