GC probe cap defers popular-parent sweeps indefinitely; stale one-level-per-pass comments #460

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 F7 Low, comment 4563). forknet.go:31,82-145: maxForkNetworkProbes=64 at two probes per child means ~32 direct children exhaust the cap; exhaustion aborts the sweep (compact.go:132-134, fail-closed correct) so a parent with a large fork fan-out never compacts — superseded packs accumulate with only a log line. Also stale 'one level per pass' comments. Fix: raise/page the probe budget for wide fan-outs (bounded), and refresh the stale comments.

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 17:14:54 +00:00

Fix #460: raise GC probe cap 64->512, refresh stale one-level-per-pass comments

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

2026-09-13 17:15:14 +00:00

Fix #460: GC probe cap #471

crueber commented

2026-09-13 17:15:48 +00:00

Author

Owner

Fix ready for review: PR #471 (#471) — raises the probe cap 64→512, keeps fail-closed on exhaustion, refreshes the stale one-level-per-pass comments, regression tests green (-race, coverage 95.5%).

Fix ready for review: PR #471 (https://git.packden.us/crueber/walhub/pulls/471) — raises the probe cap 64→512, keeps fail-closed on exhaustion, refreshes the stale one-level-per-pass comments, regression tests green (-race, coverage 95.5%).

crueber commented

2026-09-13 17:21:21 +00:00

Author

Owner

Review of PR #471 (fix/issue-460) — verified in scratch worktree at 532ca95 (removed afterward; main worktree untouched, still clean apart from pre-existing untracked .opencode/).

Cap raise sound (64->512): YES. Two probes/child (manifest + own index, forknet.go:128-137) so old cap died at ~32 children; 40-child sweep now compacts with measured 81 GETs (1 parent index + 2x40), pinned exactly by TestForkNetworkGCWideFanout. Linear cost confirmed. Law 6: walk runs only on background leased compact unit (TryLock-or-defer, exact-key GETs, never LIST, never push hot path) — 512 GETs/pass is bounded maintain-path cost. No browser check needed (no browser-facing change) — noted explicitly.
Fail-closed preserved: YES. Cap exhaustion returns error before any deletion (forkNetworkLive runs before the delete loop in gcSuperseded); TestForkNetworkGCCapExceeded (300 children, 600 probes needed) asserts err contains 'probe cap', removed==0, AND gone-old.pack survives — nothing deleted, next pass retries from scratch.
Residual bound honest: YES. ~256-child limit stated in forknet.go const comment + header, and docs/features/03_pull_requests.md Decisions entry; durable paged cursor named as follow-up requiring bucket-side cursor state (schema decision per law 4) — correctly scoped out.
Stale 'one level per pass' comments: corrected accurately. The walk is a queue-based BFS extending within the same loop (forknet.go:100-145) — cap, not depth, bounds it. Fixed in all three places: forknet.go header + inline, pulls/model.go:139, features/03 §7.
Child-index error comment fix: correct. Old text claimed child path skips on error; code returns fmt.Errorf on child index read failure (fail closed), skipping only on absent index (nil,nil leaf). New readForkIndex comment matches implementation at both parent and child levels.
No behavior change besides cap: confirmed. Only the const value + comments changed in non-test code; GC decisions identical under the old cap (same abort/skip/delete rules, same lock protocol).
Hygiene: maintain suite -race green, coverage 95.5% (>=95% gate), targeted WideFanout + CapExceeded pass -race, pulls package green, go build ./... clean, go vet + gofmt clean, no new deps, law 12 docs updated in same change (03 Decisions + code comments).

MERGE RECOMMENDATION: ready to merge. No fixes pushed (nothing to fix).

Review of PR #471 (fix/issue-460) — verified in scratch worktree at 532ca95 (removed afterward; main worktree untouched, still clean apart from pre-existing untracked .opencode/). 1) Cap raise sound (64->512): YES. Two probes/child (manifest + own index, forknet.go:128-137) so old cap died at ~32 children; 40-child sweep now compacts with measured 81 GETs (1 parent index + 2x40), pinned exactly by TestForkNetworkGCWideFanout. Linear cost confirmed. Law 6: walk runs only on background leased compact unit (TryLock-or-defer, exact-key GETs, never LIST, never push hot path) — 512 GETs/pass is bounded maintain-path cost. No browser check needed (no browser-facing change) — noted explicitly. 2) Fail-closed preserved: YES. Cap exhaustion returns error before any deletion (forkNetworkLive runs before the delete loop in gcSuperseded); TestForkNetworkGCCapExceeded (300 children, 600 probes needed) asserts err contains 'probe cap', removed==0, AND gone-old.pack survives — nothing deleted, next pass retries from scratch. 3) Residual bound honest: YES. ~256-child limit stated in forknet.go const comment + header, and docs/features/03_pull_requests.md Decisions entry; durable paged cursor named as follow-up requiring bucket-side cursor state (schema decision per law 4) — correctly scoped out. 4) Stale 'one level per pass' comments: corrected accurately. The walk is a queue-based BFS extending within the same loop (forknet.go:100-145) — cap, not depth, bounds it. Fixed in all three places: forknet.go header + inline, pulls/model.go:139, features/03 §7. 5) Child-index error comment fix: correct. Old text claimed child path skips on error; code returns fmt.Errorf on child index read failure (fail closed), skipping only on absent index (nil,nil leaf). New readForkIndex comment matches implementation at both parent and child levels. 6) No behavior change besides cap: confirmed. Only the const value + comments changed in non-test code; GC decisions identical under the old cap (same abort/skip/delete rules, same lock protocol). 7) Hygiene: maintain suite -race green, coverage 95.5% (>=95% gate), targeted WideFanout + CapExceeded pass -race, pulls package green, go build ./... clean, go vet + gofmt clean, no new deps, law 12 docs updated in same change (03 Decisions + code comments). MERGE RECOMMENDATION: ready to merge. No fixes pushed (nothing to fix).

crueber closed this issue

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

crueber commented

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

Author

Owner

Fixed by PR #471 (review clean — all 7 points pass, fail-closed preserved, residual honestly bounded), merged. Closing.

crueber referenced this issue

2026-09-13 17:22:04 +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 17:22:05 +00:00