Fix flaky TestGetPRHeadDrift stamp-without-stream race (fixes #617) #618

Merged

crueber merged 1 commit from fix/issue-617 into main

2026-09-16 00:04:17 +00:00

crueber commented

2026-09-16 00:01:46 +00:00

Owner

Test-only fix for #617; no production-code or doc change.

Root cause: the first GetPR enqueues a detached pull-mergeable recompute pass. The test rewrote fake refs + issued GetPR#2 without waiting, so a still-running pass could observe the post-mutation head and stamp + stream the 3->4 force-push on its own goroutine, while sync GetPR#2 early-returned as already-recorded without streaming. Bucket stamp lands before the in-memory stream append in the background refreshHead -> stamp present, stream missing, intermittent under -race (logical ordering flake, no data race).

Fix: waitMergeableDrained (new test helper polling the task table Finished stamp) between GetPR#1 and the ref mutation, so the drift is always stamped + streamed synchronously by GetPR#2. No assertion weakened.

Verify: targeted TestGetPRHeadDrift -race -count=200 green (was failing ~3/30 in-suite); full package -race green incl. 8x -count=30 rounds; gofmt/vet clean; pulls coverage 96.1% (>=95% gate). One transient full-package -count=10 FAIL occurred mid-stress without a captured log and has not reproduced in ~330 subsequent package executions; disclosed, not attributed to this change.

Test-only fix for #617; no production-code or doc change. Root cause: the first GetPR enqueues a detached pull-mergeable recompute pass. The test rewrote fake refs + issued GetPR#2 without waiting, so a still-running pass could observe the post-mutation head and stamp + stream the 3->4 force-push on its own goroutine, while sync GetPR#2 early-returned as already-recorded without streaming. Bucket stamp lands before the in-memory stream append in the background refreshHead -> stamp present, stream missing, intermittent under -race (logical ordering flake, no data race). Fix: waitMergeableDrained (new test helper polling the task table Finished stamp) between GetPR#1 and the ref mutation, so the drift is always stamped + streamed synchronously by GetPR#2. No assertion weakened. Verify: targeted TestGetPRHeadDrift -race -count=200 green (was failing ~3/30 in-suite); full package -race green incl. 8x -count=30 rounds; gofmt/vet clean; pulls coverage 96.1% (>=95% gate). One transient full-package -count=10 FAIL occurred mid-stress without a captured log and has not reproduced in ~330 subsequent package executions; disclosed, not attributed to this change.

crueber added 1 commit

2026-09-16 00:01:47 +00:00

Fix flaky TestGetPRHeadDrift stamp-without-stream race (Forgejo #617 ) 1a57211905

Test-only fix; no doc amendment (no production code or concurrency-rule
change — lock order and no-locks-across-store-calls discipline untouched).

Root cause: the first GetPR enqueues a detached pull-mergeable recompute
pass (mergeable.go enqueueMergeable). The test rewrote the fake refs and
issued the second GetPR without waiting for that pass, so a still-running
pass could resolve the post-mutation head, perform the 3->4 force-push
stamp + stream itself on its own goroutine, while the synchronous GetPR
then early-returned from refreshHead as 'already recorded' without
streaming. The bucket stamp (savePR) lands before the in-memory stream
append in the background refreshHead, so the assertion saw the stamp but
no head_force_pushed stream — intermittent under -race, never a data race
(all fake state is mutex-guarded; a logical ordering flake).

Fix the race, not the threshold: drain the enqueued mergeable pass
(waitMergeableDrained polls the task table for the Finished stamp, which
orders every effect of the pass before the ref mutation) so the 3->4
drift is always observed and stamped + streamed synchronously by the
second GetPR. No assertion weakened.

crueber commented

2026-09-16 00:04:04 +00:00

Author

Owner

APPROVE — independently verified root cause, fix soundness, and stress numbers.

Root cause CORRECT, verified in code (not assumed): refreshHead (mergeable.go:227 savePR bucket write → :237 appendEvent → :247 s.stream in-memory append) has the bucket-before-stream ordering, and the sync GetPR path (mergeable.go:209-212) early-returns as already-recorded with no stream when a background pass won the CAS first. GetPR#1's enqueueMergeable spawns the detached pull-mergeable pass (ComputeMergeable :126-131 resolves live head + calls refreshHead on drift), so a pass straddling the seedRefs mutation fully explains stamp-without-stream. Both halves check out.
Fix SOUND: waitMergeableDrained polls tasks.get Finished != '' with a 5s deadline → t.Fatal (bounded, no hang). Happens-before holds: end() writes Finished under table+record mutex after ComputeMergeable returns, get() snapshots under the same mutexes — observing Finished orders all pass effects (ref resolve, stamp, stream) before seedRefs. No second-pass gap: OpenPR never enqueues (only GetPR tail :598, refreshHead :250, HandleRefEvent), GetPR#1 creates exactly one pass (second enqueue joins via single-flight begin), and nothing enqueues in the single-threaded drain→seedRefs window. GetPR#1 always enqueues (mergeable cache nil on first read), so no false-timeout path. Lock order table→record preserved; test-only helper.
Scope clean: diff is 2 _test.go files only (fakes_test.go helper +28, pulls_test.go 1 call site), no production code, no assertion/threshold touched, no lock-order impact. gofmt/vet clean.
Uncaptured transient: reviewed all sibling stream assertions — draft613 converted_to_draft/ready_for_review stream synchronously in UpdatePR (service.go:993,1055), merged streams drain via waitTask(MergeTask), mergeable tests poll via fetchConverged. TestGetPRHeadDrift was the only background-pass stream assertion without a drain, now fixed. Noting the one uncaptured mid-stress FAIL without attribution is acceptable; my own runs add no new signal.
My stress numbers: TestGetPRHeadDrift -race -count=30 green; full package -race green; coverage 96.1% (>=95% gate); gofmt/vet clean.

Verdict: APPROVE (no fix commits needed).

APPROVE — independently verified root cause, fix soundness, and stress numbers. 1. Root cause CORRECT, verified in code (not assumed): refreshHead (mergeable.go:227 savePR bucket write → :237 appendEvent → :247 s.stream in-memory append) has the bucket-before-stream ordering, and the sync GetPR path (mergeable.go:209-212) early-returns as already-recorded with no stream when a background pass won the CAS first. GetPR#1's enqueueMergeable spawns the detached pull-mergeable pass (ComputeMergeable :126-131 resolves live head + calls refreshHead on drift), so a pass straddling the seedRefs mutation fully explains stamp-without-stream. Both halves check out. 2. Fix SOUND: waitMergeableDrained polls tasks.get Finished != '' with a 5s deadline → t.Fatal (bounded, no hang). Happens-before holds: end() writes Finished under table+record mutex after ComputeMergeable returns, get() snapshots under the same mutexes — observing Finished orders all pass effects (ref resolve, stamp, stream) before seedRefs. No second-pass gap: OpenPR never enqueues (only GetPR tail :598, refreshHead :250, HandleRefEvent), GetPR#1 creates exactly one pass (second enqueue joins via single-flight begin), and nothing enqueues in the single-threaded drain→seedRefs window. GetPR#1 always enqueues (mergeable cache nil on first read), so no false-timeout path. Lock order table→record preserved; test-only helper. 3. Scope clean: diff is 2 _test.go files only (fakes_test.go helper +28, pulls_test.go 1 call site), no production code, no assertion/threshold touched, no lock-order impact. gofmt/vet clean. 4. Uncaptured transient: reviewed all sibling stream assertions — draft613 converted_to_draft/ready_for_review stream synchronously in UpdatePR (service.go:993,1055), merged streams drain via waitTask(MergeTask), mergeable tests poll via fetchConverged. TestGetPRHeadDrift was the only background-pass stream assertion without a drain, now fixed. Noting the one uncaptured mid-stress FAIL without attribution is acceptable; my own runs add no new signal. 5. My stress numbers: TestGetPRHeadDrift -race -count=30 green; full package -race green; coverage 96.1% (>=95% gate); gofmt/vet clean. Verdict: APPROVE (no fix commits needed).

crueber merged commit 18570a01a6 into main

2026-09-16 00:04:17 +00:00

crueber referenced this pull request

2026-09-16 00:04:18 +00:00

Flaky TestGetPRHeadDrift (stamp-without-stream), pre-existing on main #617

crueber referenced this pull request from a commit

2026-09-16 00:04:19 +00:00

Merge pull request 'Fix flaky TestGetPRHeadDrift stamp-without-stream race (fixes #617)' (#618) from fix/issue-617 into main