Flaky merge-gate test asserts transient running state #180

New issue

Closed

opened 2026-09-06 18:05:43 +00:00 by crueber · 3 comments

crueber commented

2026-09-06 18:05:43 +00:00

Owner

Flaky TestMergeConsultsReviewGate: asserts transient `running` state

make race (full parallel load) failed internal/pulls TestMergeConsultsReviewGate/deny_blocks_the_publish: must return running but the record already showed State:error.

Analysis

StartMerge (merge.go:75) returns entry.rec.snapshot() AFTER spawning the worker (:63). With the fake gate denying fast (no slow git on this path), a descheduled test goroutine lets the worker run to terminal error before the snapshot is taken. Solo/unloaded the main goroutine wins; loaded it loses. The test pins a scheduling outcome, not behavior — same flake class as #178.

Fix (no weakening)

Make the fake gate controllable (block on a test-owned channel): StartMerge → deterministically running → release gate → terminal assertions (error, narration, gate-once, no publish, not merged) unchanged. Preserves the async-start pin deterministically instead of deleting it. Coverage gate holds.

Acceptance criteria

Test deterministic under load (-race -count=20 solo + full ./internal/... parallel run green).
make ci gate (race tier) green.

# Flaky TestMergeConsultsReviewGate: asserts transient `running` state `make race` (full parallel load) failed `internal/pulls` `TestMergeConsultsReviewGate/deny_blocks_the_publish`: `must return running` but the record already showed `State:error`. ## Analysis `StartMerge` (`merge.go:75`) returns `entry.rec.snapshot()` AFTER spawning the worker (`:63`). With the fake gate denying fast (no slow git on this path), a descheduled test goroutine lets the worker run to terminal `error` before the snapshot is taken. Solo/unloaded the main goroutine wins; loaded it loses. The test pins a scheduling outcome, not behavior — same flake class as #178. ## Fix (no weakening) Make the fake gate controllable (block on a test-owned channel): StartMerge → deterministically `running` → release gate → terminal assertions (error, narration, gate-once, no publish, not merged) unchanged. Preserves the async-start pin deterministically instead of deleting it. Coverage gate holds. ## Acceptance criteria - [ ] Test deterministic under load (`-race -count=20` solo + full `./internal/...` parallel run green). - [ ] `make ci` gate (race tier) green.

crueber referenced this issue from a commit

2026-09-06 18:09:40 +00:00

Fix #180: deterministic merge-gate tests via controllable fake gates

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

2026-09-06 18:09:44 +00:00

Fix #180: deterministic merge-gate tests #181

crueber commented

2026-09-06 18:09:46 +00:00

Author

Owner

Fixed by PR #181 (#181): controllable fake gates (test-owned entered/release channels) make the StartMerge → running snapshot deterministic in TestMergeConsultsReviewGate/deny, TestMergeConsultsChecksGate/deny, and TestMergeStrategies. Test-only, no production change. Targeted -race -count=20 green, package -race green, coverage 97.7%.

Fixed by PR #181 (https://git.packden.us/crueber/walhub/pulls/181): controllable fake gates (test-owned entered/release channels) make the StartMerge → running snapshot deterministic in TestMergeConsultsReviewGate/deny, TestMergeConsultsChecksGate/deny, and TestMergeStrategies. Test-only, no production change. Targeted -race -count=20 green, package -race green, coverage 97.7%.

crueber commented

2026-09-06 18:12:41 +00:00

Author

Owner

Review of PR #181 (fix/issue-180, now at f472c35 after one review fix pushed to the branch):

AGENTS.md law 11 — PASS. The async-start pin is preserved, not weakened: StartMerge still spawns the worker and returns entry.rec.snapshot() (merge.go:63-75, production untouched); all three sites still assert rec.State == TaskRunning. The blocking gate only makes that snapshot deterministic instead of a scheduling win.

Blocking-gate deadlock — one real leak found and FIXED (pushed as f472c35): on any pre-release failure (StartMerge/rec-state Fatalf, awaitGate timeout) the worker blocked forever on <-release, because its ctx is context.WithoutCancel (merge.go:65) so the <-ctx.Done() arm can never fire. No t.Cleanup existed. It could not hang the suite (leaked goroutine only; fresh testEnv per subtest), but it leaked the worker + task entry. All three sites (review_gate_test.go deny, checks_gate_test.go deny, merge_test.go strategies) now use t.Cleanup + sync.Once releaseGate; close(release) replaced by the idempotent call.

awaitGate — PASS: channel-close signal + 5s timeout, no sleep-based waiting.

Terminal assertions — PASS, nothing dropped. Review-deny keeps error/narration/gate-once/no-publish/not-merged; checks-deny keeps verbatim message + live-head/base/merger args; strategies keeps sha/publish/meta/pr/thread/closer/index/head-delete/stream. Only addition is the new gate-calls==1 pin in strategies (strengthening).

Gate-once — PASS and genuine: single call site per gate in runMerge (merge.go:159 checks, :170 reviews); a second worker call would bump calls to 2 and trip the assertion (release already closed, so a re-entry passes through instead of masking).

Coverage — PASS: all three gated sites converted; the allow subtests use _ = rec (no snapshot assertion, nothing to pin). Diff is tests-only (5x _test.go), no new non-stdlib imports (sync, log, io only).

Note (non-blocking): the branch also bundles notify test refinements (tasks_test.go two-phase delivery/termination windows, webhooks_test.go TLS ErrorLog silence, both referencing #178) not mentioned in the commit message. Test-only and in the flake-fighting theme, so left as-is — consider naming it in the merge commit.

Verified in scratch worktree: gofmt clean, go vet clean, targeted 3 tests -race -count=10 PASS, full pulls + notify -race -count=3 PASS, coverage pulls 97.7% / notify 95.9% (gate holds).

MERGE RECOMMENDATION: ready to merge.

Review of PR #181 (fix/issue-180, now at f472c35 after one review fix pushed to the branch): AGENTS.md law 11 — PASS. The async-start pin is preserved, not weakened: StartMerge still spawns the worker and returns entry.rec.snapshot() (merge.go:63-75, production untouched); all three sites still assert rec.State == TaskRunning. The blocking gate only makes that snapshot deterministic instead of a scheduling win. Blocking-gate deadlock — one real leak found and FIXED (pushed as f472c35): on any pre-release failure (StartMerge/rec-state Fatalf, awaitGate timeout) the worker blocked forever on <-release, because its ctx is context.WithoutCancel (merge.go:65) so the <-ctx.Done() arm can never fire. No t.Cleanup existed. It could not hang the suite (leaked goroutine only; fresh testEnv per subtest), but it leaked the worker + task entry. All three sites (review_gate_test.go deny, checks_gate_test.go deny, merge_test.go strategies) now use t.Cleanup + sync.Once releaseGate; close(release) replaced by the idempotent call. awaitGate — PASS: channel-close signal + 5s timeout, no sleep-based waiting. Terminal assertions — PASS, nothing dropped. Review-deny keeps error/narration/gate-once/no-publish/not-merged; checks-deny keeps verbatim message + live-head/base/merger args; strategies keeps sha/publish/meta/pr/thread/closer/index/head-delete/stream. Only addition is the new gate-calls==1 pin in strategies (strengthening). Gate-once — PASS and genuine: single call site per gate in runMerge (merge.go:159 checks, :170 reviews); a second worker call would bump calls to 2 and trip the assertion (release already closed, so a re-entry passes through instead of masking). Coverage — PASS: all three gated sites converted; the allow subtests use _ = rec (no snapshot assertion, nothing to pin). Diff is tests-only (5x _test.go), no new non-stdlib imports (sync, log, io only). Note (non-blocking): the branch also bundles notify test refinements (tasks_test.go two-phase delivery/termination windows, webhooks_test.go TLS ErrorLog silence, both referencing #178) not mentioned in the commit message. Test-only and in the flake-fighting theme, so left as-is — consider naming it in the merge commit. Verified in scratch worktree: gofmt clean, go vet clean, targeted 3 tests -race -count=10 PASS, full pulls + notify -race -count=3 PASS, coverage pulls 97.7% / notify 95.9% (gate holds). MERGE RECOMMENDATION: ready to merge.

crueber closed this issue

2026-09-06 18:13:13 +00:00

crueber commented

2026-09-06 18:13:13 +00:00

Author

Owner

Fixed by PR #181 incl. review worker-leak fix (deterministic gates, pin preserved; all green), merged. Closing.

crueber referenced this issue from a commit

2026-09-06 18:13:14 +00:00

Merge pull request 'Fix #180: deterministic merge-gate tests' (#181) from fix/issue-180 into main

crueber added the

actions