Data race (-race only): concurrent send/close on p.send — sendCritical vs readPump close(p.send) #14

New issue

Closed

opened 2026-06-20 21:32:42 +02:00 by engineer · 0 comments

engineer commented

2026-06-20 21:32:42 +02:00

Owner

Summary

go test -race flags a data race on a player's send channel: sendCritical's async send (ch <- msg, server/lobby.go:304) races readPump's close(p.send) (server/main.go:92).

Read at  ... sendCritical.func1()  server/lobby.go:304   (ch <- msg)
Previous write at ... readPump.func1()  server/main.go:92   (close(p.send))

Severity — NOT deploy-blocking

-race-ONLY. sendCritical wraps the send in recover(), so a send-on-closed-channel is defined behavior (panic → recovered) under plain go test. The race detector flags the concurrent send/close regardless, but it does not fail the suite without -race.
CI uses cd server && go test ./... (.forgejo/workflows/test.yml) — no -race — so this does not red CI or block deploy.
Pre-existing. Predates the #12 reconnect work; it only became visible once the suite was made fast enough to run -count … -race (de-flake @05d9c7f dropped the suite 25s → 0.11s).

Root cause

p.send has no single owner: readPump closes it on disconnect, while sendCritical (and the run-loop broadcast) may still be sending. recover() masks the panic but the concurrent send/close is a genuine data race.

Fix direction (deliberate, post-sprint)

Single-closer lifecycle. Options:

(a, idiomatic) Don't close(p.send) from readPump. Signal teardown via a separate done channel; let writePump (the sole sender's drain point) own the channel close after sends stop.
(b) A per-player mutex guarding send + close with a closed flag; sendCritical checks it under the lock.

(a) is the cleaner refactor.

Repro

cd server && go test ./. -count=20 -race   # → WARNING: DATA RACE on p.send

Filed per Herald's call during the 2026-06-20 CELLBLOCK polish sprint: real latent concurrency issue worth not losing, but recover-handled + -race-only → correctly out of scope for the sprint (a single-closer refactor, not sprint-pressure work).

## Summary `go test -race` flags a data race on a player's `send` channel: `sendCritical`'s async send (`ch <- msg`, `server/lobby.go:304`) races `readPump`'s `close(p.send)` (`server/main.go:92`). ``` Read at ... sendCritical.func1() server/lobby.go:304 (ch <- msg) Previous write at ... readPump.func1() server/main.go:92 (close(p.send)) ``` ## Severity — NOT deploy-blocking - **`-race`-ONLY.** `sendCritical` wraps the send in `recover()`, so a send-on-closed-channel is defined behavior (panic → recovered) under plain `go test`. The race detector flags the concurrent send/close regardless, but it does not fail the suite without `-race`. - **CI uses `cd server && go test ./...`** (`.forgejo/workflows/test.yml`) — **no `-race`** — so this does not red CI or block deploy. - **Pre-existing.** Predates the #12 reconnect work; it only became *visible* once the suite was made fast enough to run `-count … -race` (de-flake @05d9c7f dropped the suite 25s → 0.11s). ## Root cause `p.send` has no single owner: `readPump` closes it on disconnect, while `sendCritical` (and the run-loop broadcast) may still be sending. `recover()` masks the panic but the concurrent send/close is a genuine data race. ## Fix direction (deliberate, post-sprint) Single-closer lifecycle. Options: - **(a, idiomatic)** Don't `close(p.send)` from `readPump`. Signal teardown via a separate `done` channel; let `writePump` (the sole sender's drain point) own the channel close after sends stop. - **(b)** A per-player mutex guarding send + close with a `closed` flag; `sendCritical` checks it under the lock. (a) is the cleaner refactor. ## Repro ``` cd server && go test ./. -count=20 -race # → WARNING: DATA RACE on p.send ``` Filed per Herald's call during the 2026-06-20 CELLBLOCK polish sprint: real latent concurrency issue worth not losing, but recover-handled + `-race`-only → correctly out of scope for the sprint (a single-closer refactor, not sprint-pressure work).

engineer self-assigned this

2026-06-21 00:23:01 +02:00

engineer referenced this issue from a commit

2026-06-21 00:48:11 +02:00

fix(server): single-closer send lifecycle — kill the p.send send/close data race (#14)

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

2026-06-21 00:48:33 +02:00

fix(server): single-closer send lifecycle — kill the p.send send/close data race (#14) #19