Flaky TestStarConcurrentConverge: concurrent Star check-then-act race

crueber commented

2026-09-04 23:19:30 +00:00

Owner

No description provided.

crueber commented

2026-09-04 23:19:36 +00:00

Author

Owner

TestStarConcurrentConverge (internal/social/service_test.go:55-77) flakes on unmodified origin/main (fails with and without -race). Predicted by the #63 review: same-principal concurrent Star in the post-recreate zero window can double-bump (check-then-act in the Star mutation path; resync +1 races an in-flight first bump). Fix the race (CAS-atomic check-and-bump or equivalent), keep the counter exact under concurrency, add/keep a stress regression test. Gates: gofmt/vet clean, go test -race (with -count), coverage >=95% on internal/social, doc Decisions entry if semantics change.

crueber referenced this issue from a commit

2026-09-04 23:26:23 +00:00

Fix #69: atomic concurrent Star (07 §4 resync-in-CAS + leaf gate)

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

2026-09-04 23:26:24 +00:00

Fix #69: atomic concurrent Star #70

crueber commented

2026-09-04 23:26:27 +00:00

Author

Owner

Fixed by #70 (#70): atomic concurrent Star — resync folded into the counter CAS loop + leaf per-Service gate. Reproduced on base (Converge flaked 3-4 vs 2; new deterministic resync test fails 8 vs 1), now green -race -count=20, coverage 99.5%.

Fixed by #70 (https://git.packden.us/crueber/walhub/pulls/70): atomic concurrent Star — resync folded into the counter CAS loop + leaf per-Service gate. Reproduced on base (Converge flaked 3-4 vs 2; new deterministic resync test fails 8 vs 1), now green -race -count=20, coverage 99.5%.

crueber commented

2026-09-04 23:28:31 +00:00

Author

Owner

REVIEW PR #70 (fix/issue-69, d944be2) — verified in scratch worktree, main tree read-only (fetch/diff only), worktree removed afterward.

LOCK-RULE SCRUTINY (13 §2) — PASS, no rework required. The starGate (internal/social/social.go:136,150-157) is NOT a RepoHandle lock: the closed syncMu→packMu→rw list is untouched, and 07's 'no cross-feature locks' header is not violated (gate is intra-feature: Star-only, single, never nested, defer-released — no ordering hazard possible). Rule 4 ('never hold a lock across a store call') governs repo locks whose holders block clones/syncs; the gate serializes only human-rate Star calls and no deadlock cycle exists (store layer never acquires the gate; holder acquires nothing else while holding; release is deferred, panic-safe). A pure-CAS alternative CANNOT close this hole: the double-count is creator-vs-observer across two keys (winner's unconditional create-path +1 in service.go:63 vs a late observer's conditional resync +1), and making the create-path bump conditional would undercount repos with nonzero stars — the gate is the correct minimal tool, resync-in-CAS (service.go:83-113) covers observer-vs-observer. Cross-process same-instant race kept as documented residual (07 §4 + Decisions) — honest.
DOC AMENDMENT — honest and complete. The §4 'No lock' line now states cross-process=CAS-alone + same-process Star gate with scope (Star-only, Unstar token, lock-free reads, resync inside CAS loop); Decisions entry names mechanism, single-call invariance, and regressions (TestStarConcurrentConverge stress + TestStarConcurrentResyncConverges). Law 12 satisfied.
CORRECTNESS — owned-delta table preserved: absent/zero→one +1, nonzero→as-is, corrupt→0 with no write and no retry (casUpdate write=false returns immediately, social.go:242-244, bounded 8 attempts), so corrupt+concurrent cannot loop. Field-scoping preserved (zeroed subtest pins watchers=1). Unstar floor + idempotent re-Star untouched (TestStarUnstarIdempotent green).
NEGATIVE CONTROLS GENUINE — new test fails on base 3/3 runs (observed stars 2,3,7,8 vs want 1, both subtests), passes on PR. One nit (non-blocking): magnitude varies with scheduling, so 'deterministic' slightly overstates it — failure itself reproduced every run.
GATES — gofmt clean, vet clean, no new imports (builtin chan), coverage 99.5% (≥95%), go test -race: full package -count=1 and -count=5 green; -run 'TestStarConcurrentConverge|TestStarConcurrentResyncConverges' -race -count=20 green. Prod wiring is a single social.New instance (cmd/walhub/social.go:22), so the gate is process-wide; zero-Service nil-gate path is pinned by TestStarZeroServiceGateSkips.

Minor nits (non-blocking, no action): chan-as-mutex instead of sync.Mutex; lockStar receive ignores ctx (a cancelled Star waits for release — bounded by human-rate Star, acceptable).

No code changes made by reviewer. MERGE RECOMMENDATION: ready to merge (not merged per instructions).

REVIEW PR #70 (fix/issue-69, d944be2) — verified in scratch worktree, main tree read-only (fetch/diff only), worktree removed afterward. 1) LOCK-RULE SCRUTINY (13 §2) — PASS, no rework required. The starGate (internal/social/social.go:136,150-157) is NOT a RepoHandle lock: the closed syncMu→packMu→rw list is untouched, and 07's 'no cross-feature locks' header is not violated (gate is intra-feature: Star-only, single, never nested, defer-released — no ordering hazard possible). Rule 4 ('never hold a lock across a store call') governs repo locks whose holders block clones/syncs; the gate serializes only human-rate Star calls and no deadlock cycle exists (store layer never acquires the gate; holder acquires nothing else while holding; release is deferred, panic-safe). A pure-CAS alternative CANNOT close this hole: the double-count is creator-vs-observer across two keys (winner's unconditional create-path +1 in service.go:63 vs a late observer's conditional resync +1), and making the create-path bump conditional would undercount repos with nonzero stars — the gate is the correct minimal tool, resync-in-CAS (service.go:83-113) covers observer-vs-observer. Cross-process same-instant race kept as documented residual (07 §4 + Decisions) — honest. 2) DOC AMENDMENT — honest and complete. The §4 'No lock' line now states cross-process=CAS-alone + same-process Star gate with scope (Star-only, Unstar token, lock-free reads, resync inside CAS loop); Decisions entry names mechanism, single-call invariance, and regressions (TestStarConcurrentConverge stress + TestStarConcurrentResyncConverges). Law 12 satisfied. 3) CORRECTNESS — owned-delta table preserved: absent/zero→one +1, nonzero→as-is, corrupt→0 with no write and no retry (casUpdate write=false returns immediately, social.go:242-244, bounded 8 attempts), so corrupt+concurrent cannot loop. Field-scoping preserved (zeroed subtest pins watchers=1). Unstar floor + idempotent re-Star untouched (TestStarUnstarIdempotent green). 4) NEGATIVE CONTROLS GENUINE — new test fails on base 3/3 runs (observed stars 2,3,7,8 vs want 1, both subtests), passes on PR. One nit (non-blocking): magnitude varies with scheduling, so 'deterministic' slightly overstates it — failure itself reproduced every run. 5) GATES — gofmt clean, vet clean, no new imports (builtin chan), coverage 99.5% (≥95%), go test -race: full package -count=1 and -count=5 green; -run 'TestStarConcurrentConverge|TestStarConcurrentResyncConverges' -race -count=20 green. Prod wiring is a single social.New instance (cmd/walhub/social.go:22), so the gate is process-wide; zero-Service nil-gate path is pinned by TestStarZeroServiceGateSkips. Minor nits (non-blocking, no action): chan-as-mutex instead of sync.Mutex; lockStar receive ignores ctx (a cancelled Star waits for release — bounded by human-rate Star, acceptable). No code changes made by reviewer. MERGE RECOMMENDATION: ready to merge (not merged per instructions).

crueber closed this issue

2026-09-04 23:29:00 +00:00

crueber referenced this issue from a commit

2026-09-04 23:29:00 +00:00

Merge pull request 'Fix #69: atomic concurrent Star' (#70) from fix/issue-69 into main

crueber commented

2026-09-04 23:29:01 +00:00

Author

Owner

Fixed by PR #70 (review: lock-rule scrutiny passed, negative controls genuine; 99.5% coverage), merged. Closing.

crueber referenced this issue

2026-09-04 23:29:07 +00:00