Sim tier specified but missing: no internal/sim, make sim is a no-op, budgets unasserted #338

New issue

Closed

opened 2026-09-11 17:22:38 +00:00 by crueber · 3 comments

crueber commented

2026-09-11 17:22:38 +00:00

Owner

Child of #331 (audit findings 3228/3229, commit 70d29dd). AGENTS.md law 6 says happy-path budgets 'are asserted in the sim (docs/go/15_testing.md)' and working rules say 'make sim when you touched internal/wal'; 15_testing.md:180-206 normatively specifies internal/sim scenarios (TestSim_SafetyThenLiveness, TestSim_HealthyRequestRoundTripBudgets via FaultStore Stats.Ops, TestSim_LivenessUnderRandomSeeds). Reality: internal/sim/ does not exist, Makefile:76 lists sim in .PHONY with no recipe ('Nothing to be done for sim'), FaultStore (internal/store/fault/fault.go:105) exists but nothing consumes it for budgets. Also doc 15 D3 names Make targets with no recipe: test-slow, contract-fs, dev. Fix: either land the sim package + recipes (preferred, the law depends on it) or amend the law/doc to stop claiming it; either way amend D3 target list to the actual Makefile.

Child of #331 (audit findings 3228/3229, commit 70d29dd). AGENTS.md law 6 says happy-path budgets 'are asserted in the sim (docs/go/15_testing.md)' and working rules say 'make sim when you touched internal/wal'; 15_testing.md:180-206 normatively specifies internal/sim scenarios (TestSim_SafetyThenLiveness, TestSim_HealthyRequestRoundTripBudgets via FaultStore Stats.Ops, TestSim_LivenessUnderRandomSeeds). Reality: internal/sim/ does not exist, Makefile:76 lists sim in .PHONY with no recipe ('Nothing to be done for sim'), FaultStore (internal/store/fault/fault.go:105) exists but nothing consumes it for budgets. Also doc 15 D3 names Make targets with no recipe: test-slow, contract-fs, dev. Fix: either land the sim package + recipes (preferred, the law depends on it) or amend the law/doc to stop claiming it; either way amend D3 target list to the actual Makefile.

crueber referenced this issue

2026-09-11 17:22:42 +00:00

Audit: laws & design-spec conformance across the codebase — findings documented as comments on this ticket #331

crueber referenced this issue from a commit

2026-09-11 22:38:03 +00:00

Fix #338: land the sim tier (internal/sim + make sim) + three wal liveness fixes the sim exposed

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

2026-09-11 22:38:06 +00:00

Fix #338: land the sim tier #350

crueber commented

2026-09-11 22:38:10 +00:00

Author

Owner

Preferred path taken — sim tier landed: #350 (branch fix/issue-338-sim; note fix/issue-338 is held by another active worktree, possible duplicate effort).

What landed: internal/sim (11/12 scenarios, budgets asserted, law-6 reconciliation written into §4.1), make sim + contract-fs, sim in ci, D3 corrected. Gaps G1/G2/G3 stated in package doc + D8.

The sim earned its keep before landing: proving safety exposed 3 real wal liveness bugs, all fixed + regression-pinned in the PR (orphan-sweep race corrupting listed segments; orphan-backlog death spiral to the ErrCorrupt cap; wiped version token spinning the CAS ladder). Full details + test evidence in the PR description. Not merging — review requested.

Preferred path taken — sim tier landed: https://git.packden.us/crueber/walhub/pulls/350 (branch fix/issue-338-sim; note fix/issue-338 is held by another active worktree, possible duplicate effort). What landed: internal/sim (11/12 scenarios, budgets asserted, law-6 reconciliation written into §4.1), make sim + contract-fs, sim in ci, D3 corrected. Gaps G1/G2/G3 stated in package doc + D8. The sim earned its keep before landing: proving safety exposed 3 real wal liveness bugs, all fixed + regression-pinned in the PR (orphan-sweep race corrupting listed segments; orphan-backlog death spiral to the ErrCorrupt cap; wiped version token spinning the CAS ladder). Full details + test evidence in the PR description. Not merging — review requested.

crueber commented

2026-09-11 22:43:09 +00:00

Author

Owner

REVIEW PR #350 (fix/issue-338-sim, commit 76454cd incl. 1 review fix): READY TO MERGE (no blockers).

VERIFIED (scratch worktree /tmp/pr350, removed after):

make sim green (12.4s); go test -race sim + wal clean; coverage sim 96.5% / wal 95.4% (matches PR claims, both >=95%); go build ./..., gofmt, vet clean; -short sim skips TestSim_ (6.5s fast-tier); contract-fs recipe works; WALHUB_SIM_SEED/SEEDS override works, bad seed list fails loudly.
11 TestSim_ scenarios confirmed in tree (safety, exactly-one-winner, 7 liveness, budgets, seeds); 12th = G1, stated.
Chaos is non-vacuous: SafetyThenLiveness traces show err_before/cas_fail/truncate/err_after firing on all 3 links.

THE 3 WAL FIXES (all sound):
(a) sweepBurned recheck-latest: SOUND on success path — CAS linearity makes it airtight (a manifest newly listing S can only CAS from a base with head<S, so any such commit linearizes before our post-commit recheck; ladder never commits seq<=head per claimSlot publish.go:783 restart). Regression test pins it.
(b) failure-path sweep: same guard, batch-local map + committedBatch flag so no double-sweep; cross-batch overlap idempotent via Head-absent skip. No corruption vector.
(c) token adopt on guard-reject: SOUND — writes only into empty slot (handle.go:170), never overwrites a held token; ref view/heldRev untouched (StaleInstance pins rev==2); worst case a wrong token self-heals via 412->re-sync. Exactly-one-winner untouched (CAS decides; ConcurrentPushers proves loser convergence).

Lock discipline: sweepBurned called AFTER syncMu.Unlock in commitLocal (publish.go:997/1001); failure defer holds no locks; FaultStore panics lock-free (firedMu released before panic, fault.go:349-358); no new deps (go.mod clean); store surface untouched (empty diff); e2e unaffected (no server/git/e2e files touched).
Budgets honest: sidecar claim verified in code (batchWritesSidecar true for Push/Compact/RefUpdate, settings-only skip; parallel CAS+sidecar, +1 op +0 depth); with-pack case pins exactly 6; link-Stats.Ops deltas on acting link only. Law 6 reconciliation (6/5 = Rust 5/4 + parallel sidecar) holds.
Gaps G1/G2/G3 acceptable: G1 needs maintain kill hook (rebuild_test.go exists for resume); G2 hard-pinned as 412 assertion (goes away with idempotence together); G3 SIGTERM is e2e-tier. Doc-05 + D8 amendments match landed code.

FINDINGS (1 fixed, 2 suggestions):

FIXED+pushed (76454cd): runBatch comment claimed the failure sweep 'cannot harm a concurrent committer' — overclaim. Residual ms-window TOCTOU remains on the FAILURE path only (owner CAS landing between recheck-GET and segment-DELETE; success path is airtight per above). Comment now states the window honestly (same accepted check-then-act class as 20.9 S3 delete).
Suggestion (non-blocking): CheckTruth verifies listed SEGMENTS present but not PACKS (doc 15 3.4 says 'no pack missing') — vacuous today since sim pushes are ref-only. Either extend oracle or soften 3.4.
Suggestion (non-blocking): safety/seeds tests don't assert Stats.Faults()>0, so a quiet seed still passes as a (weaker) concurrency test. Consider pinning faults-fired per default seed (deterministic) — but NOT for env-overridden seeds.

REVIEW PR #350 (fix/issue-338-sim, commit 76454cd incl. 1 review fix): READY TO MERGE (no blockers). VERIFIED (scratch worktree /tmp/pr350, removed after): - make sim green (12.4s); go test -race sim + wal clean; coverage sim 96.5% / wal 95.4% (matches PR claims, both >=95%); go build ./..., gofmt, vet clean; -short sim skips TestSim_ (6.5s fast-tier); contract-fs recipe works; WALHUB_SIM_SEED/SEEDS override works, bad seed list fails loudly. - 11 TestSim_ scenarios confirmed in tree (safety, exactly-one-winner, 7 liveness, budgets, seeds); 12th = G1, stated. - Chaos is non-vacuous: SafetyThenLiveness traces show err_before/cas_fail/truncate/err_after firing on all 3 links. THE 3 WAL FIXES (all sound): (a) sweepBurned recheck-latest: SOUND on success path — CAS linearity makes it airtight (a manifest newly listing S can only CAS from a base with head<S, so any such commit linearizes before our post-commit recheck; ladder never commits seq<=head per claimSlot publish.go:783 restart). Regression test pins it. (b) failure-path sweep: same guard, batch-local map + committedBatch flag so no double-sweep; cross-batch overlap idempotent via Head-absent skip. No corruption vector. (c) token adopt on guard-reject: SOUND — writes only into empty slot (handle.go:170), never overwrites a held token; ref view/heldRev untouched (StaleInstance pins rev==2); worst case a wrong token self-heals via 412->re-sync. Exactly-one-winner untouched (CAS decides; ConcurrentPushers proves loser convergence). - Lock discipline: sweepBurned called AFTER syncMu.Unlock in commitLocal (publish.go:997/1001); failure defer holds no locks; FaultStore panics lock-free (firedMu released before panic, fault.go:349-358); no new deps (go.mod clean); store surface untouched (empty diff); e2e unaffected (no server/git/e2e files touched). - Budgets honest: sidecar claim verified in code (batchWritesSidecar true for Push/Compact/RefUpdate, settings-only skip; parallel CAS+sidecar, +1 op +0 depth); with-pack case pins exactly 6; link-Stats.Ops deltas on acting link only. Law 6 reconciliation (6/5 = Rust 5/4 + parallel sidecar) holds. - Gaps G1/G2/G3 acceptable: G1 needs maintain kill hook (rebuild_test.go exists for resume); G2 hard-pinned as 412 assertion (goes away with idempotence together); G3 SIGTERM is e2e-tier. Doc-05 + D8 amendments match landed code. FINDINGS (1 fixed, 2 suggestions): 1. FIXED+pushed (76454cd): runBatch comment claimed the failure sweep 'cannot harm a concurrent committer' — overclaim. Residual ms-window TOCTOU remains on the FAILURE path only (owner CAS landing between recheck-GET and segment-DELETE; success path is airtight per above). Comment now states the window honestly (same accepted check-then-act class as 20.9 S3 delete). 2. Suggestion (non-blocking): CheckTruth verifies listed SEGMENTS present but not PACKS (doc 15 3.4 says 'no pack missing') — vacuous today since sim pushes are ref-only. Either extend oracle or soften 3.4. 3. Suggestion (non-blocking): safety/seeds tests don't assert Stats.Faults()>0, so a quiet seed still passes as a (weaker) concurrency test. Consider pinning faults-fired per default seed (deterministic) — but NOT for env-overridden seeds.

crueber closed this issue

2026-09-11 22:43:25 +00:00

crueber referenced this issue from a commit

2026-09-11 22:43:25 +00:00

Merge pull request 'Fix #338: land the sim tier' (#350) from fix/issue-338-sim into main

crueber commented

2026-09-11 22:43:25 +00:00

Author

Owner

Fixed by PR #350 (review clean — harness, 3 wal fixes, budgets, gates all verified + one comment-honesty fix by reviewer; sim green, -race clean, coverage holds), merged. Closing.

crueber referenced this issue

2026-09-11 22:45:41 +00:00

Audit: laws & design-spec conformance across the codebase — findings documented as comments on this ticket #331