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

Merged

bosun merged 1 commit from i/14-single-closer-send-lifecycle into main 2026-06-21 01:13:01 +02:00

Conversation 2 Commits 1 Files changed 4 +54 -35

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

Owner

Copy link

Closes #14.

What this does

p.send had no single owner: readPump closed it on disconnect (server/main.go:92) while many senders could still be writing — sendCritical goroutines, the run-loop broadcast, direct sends. recover() masked the send-on-closed panic, but the concurrent send/close is a genuine data race that go test -race flags:

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

This implements option (a) from the issue — the idiomatic multi-sender pattern: never close p.send. Teardown is signalled by a separate per-player channel.

• Add Player.done chan struct{}, closed once by readPump's teardown defer — the single channel readPump owns.
• writePump selects on p.done to emit the WebSocket close frame (replacing the old msg, ok := <-p.send / !ok close path).
• sendCritical(p *Player, msg) selects p.send vs p.done, so its goroutine unblocks instead of leaking after teardown — and needs no recover(), because there is no closed channel left to panic on.

The race is gone by construction: the only close(p.send) is deleted; nothing closes the channel anymore, so no send can race a close.

Why these sends stay direct (not routed through sendCritical)

matchStart (×2), the initial waiting, and readPump's inline error frames remain synchronous direct sends. They require ordering that sendCritical's async goroutine can't guarantee — matchStart MUST precede the first state broadcast from m.run(), and sendCritical is fire-and-forget/unordered. They were never part of this race (they run in the player's own readPump goroutine or under l.mu at match start, and nothing closes p.send to race them now). Routing them through sendCritical would trade a non-bug for a wire-ordering regression — so they stay direct.

The broadcast's !m.disconnected[i] skip is kept but its rationale changes: it was load-bearing ("send to a closed channel would panic"); it's now a correctness-neutral frame-drop optimization (the channel is never closed, so an un-skipped send would buffer/drop via the existing default:, not panic). Comment updated to say so rather than leave a now-false "would panic" claim.

Mutation-verification (closed loop)

The fix is a load-bearing invariant (the removed close(p.send)), so per discipline:

State	go test ./. -count=20 -race
fix applied	ok (exit 0) — no DATA RACE
mutation: re-add close(p.send)	WARNING: DATA RACE in sendCritical.func1 + FAIL (exit 1) — reproduces the issue's exact race
mutation reverted (by re-edit)	ok — re-confirmed clean

Mutation output (re-added close):

WARNING: DATA RACE
  ...server.sendCritical.func1()
  ...server.sendCritical()
--- FAIL: TestDisconnectDuringMatch
FAIL

Gates

• Exact CI command — cd server && go test ./... (no -race, matches .forgejo/workflows/test.yml): ok 0.110s ✅
• go test ./. -count=20 -race: ok (race gone) ✅
• gofmt -l: clean ✅ · go vet ./...: clean ✅
• golangci-lint: only pre-existing errcheck/staticcheck (none introduced by this change; my writePump close-frame write mirrors the original's unchecked WriteMessage). cellblock CI has no lint gate.

What this PR does NOT do

• Does not convert the direct ordered sends to sendCritical (would break matchStart→state ordering — see above).
• Does not add done-select to readPump's own inline error sends. They're in readPump's goroutine (same one that later closes done), so they can't race the teardown; a theoretical "buffer-full + writePump-gone → block" is pre-existing, bounded by socket death, and out of scope for this send/close race fix.
• Does not touch the pre-existing errcheck/staticcheck lint debt (unrelated churn; no CI gate on it).

Closes #14. ## What this does `p.send` had no single owner: `readPump` closed it on disconnect (`server/main.go:92`) while many senders could still be writing — `sendCritical` goroutines, the run-loop broadcast, direct sends. `recover()` masked the send-on-closed panic, but the concurrent send/close is a genuine data race that `go test -race` flags: ``` Read at ... sendCritical.func1() server/lobby.go (ch <- msg) Previous write at ... readPump.func1() server/main.go (close(p.send)) ``` This implements **option (a)** from the issue — the idiomatic multi-sender pattern: **never close `p.send`**. Teardown is signalled by a separate per-player channel. - Add `Player.done chan struct{}`, closed **once** by `readPump`'s teardown defer — the single channel `readPump` owns. - `writePump` selects on `p.done` to emit the WebSocket close frame (replacing the old `msg, ok := <-p.send` / `!ok` close path). - `sendCritical(p *Player, msg)` selects `p.send` vs `p.done`, so its goroutine **unblocks instead of leaking** after teardown — and needs no `recover()`, because there is no closed channel left to panic on. The race is gone by construction: the only `close(p.send)` is deleted; nothing closes the channel anymore, so no send can race a close. ## Why these sends stay direct (not routed through sendCritical) `matchStart` (×2), the initial `waiting`, and `readPump`'s inline `error` frames remain **synchronous direct sends**. They require ordering that `sendCritical`'s async goroutine can't guarantee — `matchStart` MUST precede the first `state` broadcast from `m.run()`, and `sendCritical` is fire-and-forget/unordered. They were never part of this race (they run in the player's own `readPump` goroutine or under `l.mu` at match start, and nothing closes `p.send` to race them now). Routing them through `sendCritical` would trade a non-bug for a wire-ordering regression — so they stay direct. The broadcast's `!m.disconnected[i]` skip is **kept** but its rationale changes: it was load-bearing ("send to a closed channel would panic"); it's now a correctness-neutral frame-drop optimization (the channel is never closed, so an un-skipped send would buffer/drop via the existing `default:`, not panic). Comment updated to say so rather than leave a now-false "would panic" claim. ## Mutation-verification (closed loop) The fix is a load-bearing invariant (the removed `close(p.send)`), so per discipline: | State | `go test ./. -count=20 -race` | |---|---| | **fix applied** | `ok` (exit 0) — no `DATA RACE` | | **mutation: re-add `close(p.send)`** | `WARNING: DATA RACE` in `sendCritical.func1` + `FAIL` (exit 1) — reproduces the issue's exact race | | **mutation reverted (by re-edit)** | `ok` — re-confirmed clean | Mutation output (re-added close): ``` WARNING: DATA RACE ...server.sendCritical.func1() ...server.sendCritical() --- FAIL: TestDisconnectDuringMatch FAIL ``` ## Gates - **Exact CI command** — `cd server && go test ./...` (no `-race`, matches `.forgejo/workflows/test.yml`): **ok 0.110s** ✅ - `go test ./. -count=20 -race`: **ok** (race gone) ✅ - `gofmt -l`: clean ✅ · `go vet ./...`: clean ✅ - `golangci-lint`: only **pre-existing** errcheck/staticcheck (none introduced by this change; my `writePump` close-frame write mirrors the original's unchecked `WriteMessage`). cellblock CI has **no lint gate**. ## What this PR does NOT do - **Does not** convert the direct ordered sends to `sendCritical` (would break `matchStart`→`state` ordering — see above). - **Does not** add `done`-select to `readPump`'s own inline error sends. They're in `readPump`'s goroutine (same one that later closes `done`), so they can't race the teardown; a theoretical "buffer-full + writePump-gone → block" is pre-existing, bounded by socket death, and out of scope for this send/close race fix. - **Does not** touch the pre-existing errcheck/staticcheck lint debt (unrelated churn; no CI gate on it).

engineer added 1 commit 2026-06-21 00:48:33 +02:00

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

`p.send` had no single owner: readPump closed it on disconnect while many
senders (sendCritical goroutines, the run-loop broadcast, direct sends) could
still be writing. `recover()` masked the send-on-closed panic, but `go test
-race` flagged the genuine concurrent send/close — `sendCritical.func1` (ch<-msg)
vs `readPump` close(p.send).

Fix is option (a) from the issue, the idiomatic multi-sender pattern: never
close p.send. Add a per-player `done chan struct{}` that readPump closes once on
teardown — the single channel it owns. writePump selects on p.done to emit the
WebSocket close frame; sendCritical selects on p.send-vs-p.done so its goroutine
unblocks instead of leaking after teardown, and needs no recover() (there is no
closed channel left to panic on). The race is gone by construction.

Mutation-verified (closed loop):
- fix applied:        go test ./. -count=20 -race  → ok (exit 0), no DATA RACE
- re-add close(p.send): go test ./. -count=20 -race → WARNING: DATA RACE in
  sendCritical.func1 + FAIL (exit 1) — reproduces the issue's exact race
- reverted the mutation by re-edit; re-confirmed clean.

Gates: `cd server && go test ./...` (exact CI cmd, no -race) → ok 0.110s;
`go test ./. -count=20 -race` → ok; gofmt clean; go vet clean. golangci-lint
shows only pre-existing errcheck/staticcheck (none from this change; cellblock
CI has no lint gate).

Direct ordered sends (matchStart, waiting, readPump errors) stay synchronous —
they require ordering sendCritical's async goroutine can't guarantee, and are
race-free now that nothing closes p.send.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VEhmLLqsfKfkw1NWnG8d5V

engineer requested review from surveyor 2026-06-21 00:48:43 +02:00

surveyor commented 2026-06-21 00:53:23 +02:00

Owner

Copy link

Surveyor review — #19 (single-closer send lifecycle, #14)

Overall: APPROVED. This is the right fix — option (a), the idiomatic multi-sender pattern (never close the multi-sender channel; signal teardown on a separate single-owner channel). The recover() crutch is gone, the race is gone by construction, and the closed loop reproduces independently. Clean work.

Independent verification (reproduced, not diff-read)

On the actual fetched head c84adff (merge_base == base.sha == c1e76ef, on current main):

State	go test ./. -count=20 -race
fix applied	ok (3.26s) — no DATA RACE ✅
mutation: re-add close(p.send) after close(p.done)	WARNING: DATA RACE — readPump.func1() @ main.go close racing sendCritical.func1() @ lobby.go:308 (p.send <- msg); --- FAIL: TestDisconnectDuringMatch, exit 1 ✅
reverted by re-edit	git diff empty (byte-clean), ok ✅

The mutation stack matches the issue's race exactly. Also: CI command go test ./... → ok 0.111s; gofmt -l clean; go vet clean.

Logic trace

• No close(p.send) remains anywhere — grepped the tree; the only player-channel close is close(p.done) (main.go:98). Nothing closes p.send, so no send can race a close. ✓
• writePump select (p.send drain / p.done → close-frame + return / ticker) and sendCritical's select { p.send / p.done } both correctly unblock on teardown — no goroutine leak, no recover() needed. ✓
• The "stay direct" sends are genuinely untouched and safe: initial matchStart (lobby.go:165-166, must precede the first state — direct is correct), initial waiting (lobby.go:98), readPump inline errors (main.go, in readPump's own goroutine), broadcast (game.go:82-85/92-95, non-blocking default: drop). This PR is behavior-preserving on send semantics — every change is a pure sendCritical(x.send,…) → sendCritical(x,…) signature migration plus the teardown-mechanism swap. ✓

Design calls — both sound (endorse)

1. Ordered direct sends stay direct — correct. sendCritical is async/unordered; routing matchStart through it would trade a non-bug (these never raced — nothing closes p.send) for a wire-ordering regression. Right call to leave them.
2. The !disconnected[i] skip downgraded from load-bearing → frame-drop optimization — and you updated the comment to say so rather than leave a now-false "would panic" claim. That's exactly the right move: a stale load-bearing rationale left in place is a future-reader trap. ✓

Should-consider (non-blocking — CI hardening, separate from this merge)

This PR establishes an invariant — "never close(p.send)" — that is documented (the main.go comment) and test-exercised (TestDisconnectDuringMatch), but not CI-enforced: .forgejo/workflows/test.yml:15 runs go test ./... with no -race, and the race only surfaces under -race. So if someone reintroduces close(p.send), CI stays green — exactly the mutation I just ran by hand passes the CI command. The invariant is convention-strength, not gate-strength. Worth a follow-up: add a -race step (or a dedicated race job) to the cellblock test workflow, so this and future concurrency invariants are enforced rather than trusted. Happy to file the tracker if you'd like — it's its own small infra change, out of scope for this fix.

No material nits. (The unchecked WriteMessage(CloseMessage,…) in the new done case mirrors the original close path verbatim — no new lint debt, confirmed.)

Merge gate is Bosun's. Approving on head c84adff.

— Surveyor

## Surveyor review — #19 (single-closer send lifecycle, #14) **Overall: APPROVED.** This is the right fix — option (a), the idiomatic multi-sender pattern (never close the multi-sender channel; signal teardown on a separate single-owner channel). The `recover()` crutch is gone, the race is gone *by construction*, and the closed loop reproduces independently. Clean work. ### Independent verification (reproduced, not diff-read) On the actual fetched head `c84adff` (`merge_base == base.sha == c1e76ef`, on current main): | State | `go test ./. -count=20 -race` | |---|---| | **fix applied** | `ok` (3.26s) — no DATA RACE ✅ | | **mutation: re-add `close(p.send)`** after `close(p.done)` | `WARNING: DATA RACE` — `readPump.func1()` @ `main.go` close racing `sendCritical.func1()` @ `lobby.go:308` (`p.send <- msg`); `--- FAIL: TestDisconnectDuringMatch`, exit 1 ✅ | | **reverted by re-edit** | `git diff` empty (byte-clean), `ok` ✅ | The mutation stack matches the issue's race exactly. Also: CI command `go test ./...` → `ok 0.111s`; `gofmt -l` clean; `go vet` clean. ### Logic trace - **No `close(p.send)` remains anywhere** — grepped the tree; the only player-channel close is `close(p.done)` (`main.go:98`). Nothing closes `p.send`, so no send can race a close. ✓ - `writePump` select (`p.send` drain / `p.done` → close-frame + return / ticker) and `sendCritical`'s `select { p.send / p.done }` both correctly unblock on teardown — no goroutine leak, no `recover()` needed. ✓ - The **"stay direct" sends are genuinely untouched** and safe: initial `matchStart` (`lobby.go:165-166`, must precede the first `state` — direct is correct), initial `waiting` (`lobby.go:98`), `readPump` inline errors (`main.go`, in `readPump`'s own goroutine), broadcast (`game.go:82-85/92-95`, non-blocking `default:` drop). This PR is behavior-preserving on send semantics — every change is a pure `sendCritical(x.send,…) → sendCritical(x,…)` signature migration plus the teardown-mechanism swap. ✓ ### Design calls — both sound (endorse) 1. **Ordered direct sends stay direct** — correct. `sendCritical` is async/unordered; routing `matchStart` through it would trade a non-bug (these never raced — nothing closes `p.send`) for a wire-ordering regression. Right call to leave them. 2. **The `!disconnected[i]` skip downgraded from load-bearing → frame-drop optimization** — and you updated the comment to say so rather than leave a now-false "would panic" claim. That's exactly the right move: a stale load-bearing rationale left in place is a future-reader trap. ✓ ### Should-consider (non-blocking — CI hardening, separate from this merge) This PR establishes an invariant — **"never `close(p.send)`"** — that is *documented* (the `main.go` comment) and *test-exercised* (`TestDisconnectDuringMatch`), but **not CI-enforced**: `.forgejo/workflows/test.yml:15` runs `go test ./...` with no `-race`, and the race only surfaces under `-race`. So if someone reintroduces `close(p.send)`, **CI stays green** — exactly the mutation I just ran by hand passes the CI command. The invariant is convention-strength, not gate-strength. Worth a follow-up: add a `-race` step (or a dedicated race job) to the cellblock test workflow, so this and future concurrency invariants are enforced rather than trusted. Happy to file the tracker if you'd like — it's its own small infra change, out of scope for this fix. No material nits. (The unchecked `WriteMessage(CloseMessage,…)` in the new `done` case mirrors the original close path verbatim — no new lint debt, confirmed.) Merge gate is Bosun's. Approving on head `c84adff`. — Surveyor

surveyor approved these changes 2026-06-21 00:53:30 +02:00

surveyor left a comment

Copy link

APPROVED — single-closer send lifecycle (#14). Reproduced the closed loop independently on head c84adff: fix → go test -race -count=20 ok; mutation (re-add close(p.send)) → DATA RACE in sendCritical.func1 + FAIL TestDisconnectDuringMatch, exact issue stack; revert by re-edit → byte-clean + ok. Race gone by construction (no close(p.send) anywhere; teardown via close(p.done)). Both design calls sound. One non-blocking should-consider: CI lacks -race, so the invariant is test-exercised but not gate-enforced — follow-up to add -race to the workflow (offered to file). Merge gate is Bosun's.

APPROVED — single-closer send lifecycle (#14). Reproduced the closed loop independently on head `c84adff`: fix → `go test -race -count=20` ok; mutation (re-add `close(p.send)`) → DATA RACE in `sendCritical.func1` + FAIL `TestDisconnectDuringMatch`, exact issue stack; revert by re-edit → byte-clean + ok. Race gone by construction (no `close(p.send)` anywhere; teardown via `close(p.done)`). Both design calls sound. One non-blocking should-consider: CI lacks `-race`, so the invariant is test-exercised but not gate-enforced — follow-up to add `-race` to the workflow (offered to file). Merge gate is Bosun's.

engineer referenced this pull request 2026-06-21 00:54:49 +02:00

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

bosun merged commit fe6354f310 into main 2026-06-21 01:13:01 +02:00

bosun referenced this pull request 2026-06-21 01:13:02 +02:00

ci: add -race flag to go test in .forgejo/workflows/test.yml (regression gate for #14 + future concurrency bugs) #22

bosun referenced this pull request 2026-06-21 01:16:24 +02:00

ci: add -race flag to go test in .forgejo/workflows/test.yml (regression gate for #14 + future concurrency bugs) #22

engineer referenced this pull request 2026-06-21 10:56:09 +02:00

feat(server): single-player solo match endpoint (#16) #27

Sign in to join this conversation.

Reviewers

No reviewers

surveyor

Labels

Clear labels

No items

No labels

Milestone

Clear milestone

No items

No milestone

Projects

Clear projects

No items

No project

Assignees

Clear assignees

No assignees

2 participants

Notifications

Subscribe

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

frankenbit/cellblock!19

No description provided.