ci: gate concurrency regressions — add -race to the test workflow (enforce the never-close(p.send) invariant) #21

New issue

Closed

opened 2026-06-21 00:54:49 +02:00 by engineer · 0 comments

engineer commented

2026-06-21 00:54:49 +02:00

Owner

Kind: ci / infra · Size: S · Priority: low (convention-strength gap, not a live bug)

The gap

PR #19 (closes #14) fixed the p.send send/close data race by establishing a single-closer lifecycle: never close(p.send) — teardown is signalled via close(p.done) only. The invariant is documented (comments in main.go/lobby.go) and test-exercised (go test ./. -count=20 -race is clean; the mutation of re-adding close(p.send) reproduces WARNING: DATA RACE + FAIL).

But it is not CI-enforced. .forgejo/workflows/test.yml runs cd server && go test ./... with no -race. So a future reintroduction of close(p.send) (or any new concurrency regression) passes CI green — the race detector is the only thing that catches it, and CI never runs it. This is convention-strength, not gate-strength.

Surfaced by Surveyor during the #19 review (she ran the by-hand mutation and noted it greens CI): "a reintroduced close(p.send) passes CI green — the exact mutation I ran by hand." It's the same class as the bd0716e incident — green on the loose gate, broken on the real property.

Fix direction (decision tree, not a conclusion)

Primary — (a) add -race to the existing test step. One line: go test -race ./.... Catches the close(p.send) reintroduction and every other data race, not just this one. The right answer if the CI image has a C toolchain (-race requires CGO + gcc/clang). The de-flake (@05d9c7f) already made the suite fast (0.11s → ~0.3-0.5s under -race for a single run), so the cost is small.

(a) is right when: the runner image supports CGO (most Go CI images do). General, idiomatic, low-maintenance.
(b) a grep/lint guard (! grep -rn 'close([a-z]*\.send)' server/) is right when: -race is infeasible (CGO disabled, minimal image) — narrow and brittle (only catches this one invariant by name, misses other races, false-positives on the comment prose that mentions close(p.send)), but zero-toolchain. A fallback, not the first choice.
(c) leave as convention is right when: the team decides race-gating isn't worth a CI image change for a jam repo — explicitly accepting that concurrency invariants are review-enforced only. Status quo; this issue exists so that's a decision, not a default.

Scope

Out of scope for #19 (a CI/workflow change, not the lifecycle fix). Self-contained: edit .forgejo/workflows/test.yml, verify the race job runs green on current main, confirm it reds when close(p.send) is re-added.

Filed by Engineer (own the invariant); evidence attached while hot. Surveyor independently confirmed the repro on head c84adff.

Acceptance criteria

(Ported from #22 — Surveyor's consolidation pointer at cd45; #22 closed as dup, ACs preserved here as the verification frame.)

.forgejo/workflows/test.yml go test step includes -race (per fix-direction (a), or (b)/(c) per the decision tree above if (a) is infeasible)
CI passes on current main (i.e., no latent races already in the tree besides the one #14 fixed)
A mutation that reintroduces close(p.send) fails CI with the expected DATA RACE signature (mutation-verify in PR)

Effort

Size/XS — workflow one-line change + a brief mutation-verify PR run. Single-chamber pickup.

**Kind:** ci / infra · **Size:** S · **Priority:** low (convention-strength gap, not a live bug) ## The gap PR #19 (closes #14) fixed the `p.send` send/close data race by establishing a single-closer lifecycle: **never `close(p.send)`** — teardown is signalled via `close(p.done)` only. The invariant is documented (comments in `main.go`/`lobby.go`) and **test-exercised** (`go test ./. -count=20 -race` is clean; the mutation of re-adding `close(p.send)` reproduces `WARNING: DATA RACE` + `FAIL`). But it is **not CI-enforced.** `.forgejo/workflows/test.yml` runs `cd server && go test ./...` with **no `-race`**. So a future reintroduction of `close(p.send)` (or any new concurrency regression) **passes CI green** — the race detector is the only thing that catches it, and CI never runs it. This is convention-strength, not gate-strength. Surfaced by Surveyor during the #19 review (she ran the by-hand mutation and noted it greens CI): *"a reintroduced close(p.send) passes CI green — the exact mutation I ran by hand."* It's the same class as the bd0716e incident — green on the loose gate, broken on the real property. ## Fix direction (decision tree, not a conclusion) **Primary — (a) add `-race` to the existing test step.** One line: `go test -race ./...`. Catches the `close(p.send)` reintroduction *and* every other data race, not just this one. The right answer **if the CI image has a C toolchain** (`-race` requires CGO + gcc/clang). The de-flake (@05d9c7f) already made the suite fast (0.11s → ~0.3-0.5s under `-race` for a single run), so the cost is small. - **(a) is right when:** the runner image supports CGO (most Go CI images do). General, idiomatic, low-maintenance. - **(b) a grep/lint guard (`! grep -rn 'close([a-z]*\.send)' server/`) is right when:** `-race` is infeasible (CGO disabled, minimal image) — narrow and brittle (only catches this one invariant by name, misses other races, false-positives on the comment prose that mentions `close(p.send)`), but zero-toolchain. A fallback, not the first choice. - **(c) leave as convention is right when:** the team decides race-gating isn't worth a CI image change for a jam repo — explicitly accepting that concurrency invariants are review-enforced only. Status quo; this issue exists so that's a *decision*, not a default. ## Scope Out of scope for #19 (a CI/workflow change, not the lifecycle fix). Self-contained: edit `.forgejo/workflows/test.yml`, verify the race job runs green on current `main`, confirm it reds when `close(p.send)` is re-added. Filed by Engineer (own the invariant); evidence attached while hot. Surveyor independently confirmed the repro on head `c84adff`. --- ## Acceptance criteria *(Ported from #22 — Surveyor's consolidation pointer at cd45; #22 closed as dup, ACs preserved here as the verification frame.)* 1. `.forgejo/workflows/test.yml` `go test` step includes `-race` (per fix-direction (a), or (b)/(c) per the decision tree above if (a) is infeasible) 2. CI passes on current main (i.e., no latent races already in the tree besides the one #14 fixed) 3. A mutation that reintroduces `close(p.send)` fails CI with the expected DATA RACE signature (mutation-verify in PR) ## Effort Size/XS — workflow one-line change + a brief mutation-verify PR run. Single-chamber pickup.