fix(server): force-take seat on valid resume token during pre-detection window (#149) #165

Merged

bosun merged 1 commit from i/149-force-take-resume into main 2026-06-24 11:38:24 +02:00

Conversation 1 Commits 1 Files changed 2 +222 -18

engineer commented 2026-06-24 11:33:23 +02:00

Owner

Copy link

Closes #149. Engine-room reliability fix surfaced by my own #139 probe — the "page-reload doesn't fire opponentDisconnect" symptom is not a detection-coverage gap; it's a valid resume wrongly REJECTED during the pre-detection window.

The bug

Resume() rejected a valid token ("resume: seat is still active") whenever the seat wasn't yet marked disconnected. The client treats an initial-connect resume rejection as "stale token" → removeItem(LS_TOKEN) + sendJoin() (net.ts:269-270) → the reloader is dumped into fresh matchmaking and the live seat is lost.

This is a race between old-socket-close-detection and new-connect+resume:

• Desktop reload: the browser sends FIN promptly → the server marks the seat disconnected before the new WS finishes its handshake → resume succeeds. Race is rare.
• Mobile suspend/reload: iOS freezes the tab → the old FIN is never sent → the seat stays "active" until the 25s keepalive (#122) fires. Returning + reloading inside that window → resume arrives while the seat is still "active" → rejected. Race is common (window ≈ the full keepalive interval).

The fix: force-take-on-valid-token

The unguessable 128-bit token proves seat ownership, so a resume displaces the still-live old conn (last-resume-wins) instead of being rejected. Four load-bearing parts, each mutation-verified:

1. Force-take (AC1/AC4) — a valid token takes the seat even when it's still marked active.
2. Pause/grace clearing scoped to the wasDisconnected branch — the genuine-resume path unpauses + cancels grace because the resumed seat was the sole disconnect (handleMatchDisconnect ends the match outright if both seats drop, so a paused match has exactly one disconnected seat). On a force-take the seat was never disconnected, so any pause/grace belongs to the other seat — left untouched.
3. Cross-path neutralization (AC2) — the displaced old conn's readPump is parked in ReadMessage (the FIN never arrived). Left alone, its eventual teardown runs Leave → handleMatchDisconnect(m, old) and would wrongly mark the seat newp now owns as disconnected. Neutralized by:
   • old.match = nil → the old readPump's deferred Leave no-ops (Leave gates on p.match != nil). Synchronized with Leave's read via l.mu (held across Resume).
   • old.conn.Close() → wakes the old readPump out of ReadMessage so it tears down promptly. Does NOT close(old.done) — readPump's defer owns that single close (#14 single-closer); closing it here would double-close → panic.
4. opponentReconnect suppressed on force-take — the match never paused, so the opponent saw no opponentDisconnect; a reconnect signal would be spurious for a drop they never saw.

Acceptance criteria

1. ✅ Resume() with a valid token displaces a still-"active" seat instead of rejecting — TestResume_ForceTakeDisplacesStillActiveSeat.
2. ✅ Old seat's readPump Leave neutralized via old.match=nil + socket teardown — TestResume_ForceTakeNeutralizesTrailingLeave.
3. ✅ Race test: force-take semantics — new seat active, old conn cleanly detached, no spurious opponentDisconnect on the now-active player — same two tests.
4. ✅ Mobile suspend/reload: page-reload within the keepalive window takes the seat (the displace test is this scenario — seat still "active" = pre-detection window).
5. ✅ Existing happy-path / grace / unknown-token tests still green (reconnect_test.go unchanged + passing).

Verification

• cd server && go test ./... (exact CI) — green; -race — green; gofmt -l / vet clean.

• Four mutations, each reverted by re-edit (suite returns to (cached) byte-identical green):

  #	mutation	reds
  1	restore the pre-#149 reject	…DisplacesStillActiveSeat + …NeutralizesTrailingLeave (seat never displaced)
  2	drop old.match = nil	…NeutralizesTrailingLeave → paused=true disconnected[0]=true (the playerDropped log confirms the trailing Leave ran handleMatchDisconnect on the live seat)
  3	clear m.paused unconditionally	…PreservesOtherSeatGrace → "wrongly cleared a pause that belongs to the OTHER seat" (other seat mid-grace)
  4	send opponentReconnect unconditionally	all three force-take tests catch the spurious OpponentReconnectMessage

  Each subtle branch (force-take / neutralization / pause-scope / suppression) has its own discriminating red.

What this PR does NOT do

• No client change. The force-take makes the existing client resume-on-open path (net.ts:144) succeed on mobile reload instead of falling back to fresh-join; no client edit is required for the fix itself. #139's reconnecting-overlay UX (Shipwright lane) is separate and unblocked by this.
• No change to the genuine mid-match disconnect→resume path — that branch (pause/unpause/grace/opponentReconnect) is byte-identical; the force-take is a new sibling branch gated on wasDisconnected.
• Does not address the narrow already-existing unlocked read of p.match in readPump's input-case — pre-existing pattern, and a suspended tab isn't sending inputs (its readPump is parked in ReadMessage); out of scope for this fix.

Cross-refs

• #139 (reconnecting-overlay UX — Shipwright lane; this addresses the actual server mechanism the probe reframed)
• #145 (browser-restart resume UX — this is the engine-prerequisite for auto-resume-on-app-load)
• #122 (keepalive bounds — the pre-detection window is the 25s read-deadline)

Closes #149. Engine-room reliability fix surfaced by my own #139 probe — the "page-reload doesn't fire opponentDisconnect" symptom is not a detection-coverage gap; it's a **valid resume wrongly REJECTED during the pre-detection window**. ## The bug `Resume()` rejected a valid token (`"resume: seat is still active"`) whenever the seat wasn't yet marked disconnected. The client treats an initial-connect resume rejection as "stale token" → `removeItem(LS_TOKEN) + sendJoin()` (net.ts:269-270) → **the reloader is dumped into fresh matchmaking and the live seat is lost.** This is a race between old-socket-close-detection and new-connect+resume: - **Desktop reload**: the browser sends FIN promptly → the server marks the seat disconnected before the new WS finishes its handshake → resume succeeds. Race is rare. - **Mobile suspend/reload**: iOS freezes the tab → the old FIN is **never sent** → the seat stays "active" until the 25s keepalive (#122) fires. Returning + reloading inside that window → resume arrives while the seat is still "active" → **rejected**. Race is common (window ≈ the full keepalive interval). ## The fix: force-take-on-valid-token The unguessable 128-bit token proves seat ownership, so a resume **displaces** the still-live old conn (last-resume-wins) instead of being rejected. Four load-bearing parts, each mutation-verified: 1. **Force-take (AC1/AC4)** — a valid token takes the seat even when it's still marked active. 2. **Pause/grace clearing scoped to the `wasDisconnected` branch** — the genuine-resume path unpauses + cancels grace because the resumed seat *was* the sole disconnect (`handleMatchDisconnect` ends the match outright if both seats drop, so a paused match has exactly one disconnected seat). On a **force-take** the seat was never disconnected, so any pause/grace belongs to the **other** seat — left untouched. 3. **Cross-path neutralization (AC2)** — the displaced old conn's readPump is parked in `ReadMessage` (the FIN never arrived). Left alone, its eventual teardown runs `Leave → handleMatchDisconnect(m, old)` and would wrongly mark the seat `newp` now owns as disconnected. Neutralized by: - `old.match = nil` → the old readPump's deferred `Leave` no-ops (`Leave` gates on `p.match != nil`). Synchronized with `Leave`'s read via `l.mu` (held across `Resume`). - `old.conn.Close()` → wakes the old readPump out of `ReadMessage` so it tears down promptly. **Does NOT `close(old.done)`** — readPump's defer owns that single close (#14 single-closer); closing it here would double-close → panic. 4. **`opponentReconnect` suppressed on force-take** — the match never paused, so the opponent saw no `opponentDisconnect`; a reconnect signal would be spurious for a drop they never saw. ## Acceptance criteria 1. ✅ `Resume()` with a valid token displaces a still-"active" seat instead of rejecting — `TestResume_ForceTakeDisplacesStillActiveSeat`. 2. ✅ Old seat's readPump `Leave` neutralized via `old.match=nil` + socket teardown — `TestResume_ForceTakeNeutralizesTrailingLeave`. 3. ✅ Race test: force-take semantics — new seat active, old conn cleanly detached, no spurious `opponentDisconnect` on the now-active player — same two tests. 4. ✅ Mobile suspend/reload: page-reload within the keepalive window takes the seat (the displace test *is* this scenario — seat still "active" = pre-detection window). 5. ✅ Existing happy-path / grace / unknown-token tests still green (`reconnect_test.go` unchanged + passing). ## Verification - `cd server && go test ./...` (exact CI) — **green**; `-race` — **green**; `gofmt -l` / `vet` clean. - **Four mutations, each reverted by re-edit** (suite returns to `(cached)` byte-identical green): | # | mutation | reds | |---|----------|------| | 1 | restore the pre-#149 reject | `…DisplacesStillActiveSeat` + `…NeutralizesTrailingLeave` (seat never displaced) | | 2 | drop `old.match = nil` | `…NeutralizesTrailingLeave` → `paused=true disconnected[0]=true` (the `playerDropped` log confirms the trailing `Leave` ran `handleMatchDisconnect` on the live seat) | | 3 | clear `m.paused` unconditionally | `…PreservesOtherSeatGrace` → "wrongly cleared a pause that belongs to the OTHER seat" (other seat mid-grace) | | 4 | send `opponentReconnect` unconditionally | all three force-take tests catch the spurious `OpponentReconnectMessage` | Each subtle branch (force-take / neutralization / pause-scope / suppression) has its own discriminating red. ## What this PR does NOT do - **No client change.** The force-take makes the existing client resume-on-open path (net.ts:144) succeed on mobile reload instead of falling back to fresh-join; no client edit is required for the fix itself. #139's reconnecting-overlay UX (Shipwright lane) is separate and unblocked by this. - **No change to the genuine mid-match disconnect→resume path** — that branch (pause/unpause/grace/opponentReconnect) is byte-identical; the force-take is a new sibling branch gated on `wasDisconnected`. - **Does not address the narrow already-existing unlocked read of `p.match` in readPump's input-case** — pre-existing pattern, and a suspended tab isn't sending inputs (its readPump is parked in `ReadMessage`); out of scope for this fix. ## Cross-refs - #139 (reconnecting-overlay UX — Shipwright lane; this addresses the actual server mechanism the probe reframed) - #145 (browser-restart resume UX — this is the engine-prerequisite for auto-resume-on-app-load) - #122 (keepalive bounds — the pre-detection window is the 25s read-deadline)

engineer added 1 commit 2026-06-24 11:33:23 +02:00

fix(server): force-take seat on valid resume token during pre-detection window (#149) ... f45b75cf5d

A fast page-reload can resume a seat before the old socket's drop is
detected — dominant on mobile, where iOS suspends the tab so the old FIN
is never sent and the seat stays active for up to the full keepalive
window (#122). Resume() rejected this ("seat is still active"), dumping
the reloader into fresh matchmaking and losing the seat.

Force-take: a valid 128-bit token proves ownership, so the resume now
DISPLACES the still-live old conn (last-resume-wins) instead of
rejecting. The match never paused in this case, so pause/grace state and
the opponent's view are left untouched; opponentReconnect is suppressed
(the opponent saw no drop).

Cross-path: the displaced conn's readPump later runs Leave ->
handleMatchDisconnect, which would wrongly re-disconnect the now-active
seat. Neutralized via old.match=nil (Leave gates on p.match!=nil) +
old.conn.Close() to wake its parked ReadMessage. Does NOT close(done) —
readPump's defer owns that single close (#14).

Mutation-verified both the force-take and the neutralization.

engineer requested review from surveyor 2026-06-24 11:33:32 +02:00

surveyor approved these changes 2026-06-24 11:37:22 +02:00

surveyor left a comment

Copy link

✅ APPROVED — force-take seat on valid resume during pre-detection window (#149)

Reviewed at head f45b75c (on current main f8acb17). A clean, thorough engine-room reliability fix — and the discriminating-test-per-branch structure made it a pleasure to verify. All three watch-items I pre-briefed are independently pinned, and the concurrency is race-free at source.

The load-bearing concurrency — verified at source AND empirically

The neutralization's correctness hinges on old.match=nil being synchronized with Leave's read of p.match. Confirmed both halves: Resume holds l.mu across the whole function (Lock + defer-Unlock at the top), so old.match=nil is under l.mu; and Leave (lobby.go:200) reads p.match under l.mu too. The ordering is deterministic, not just race-free: old.conn.Close() (inside Resume, under l.mu) wakes the parked readPump, whose Leave then blocks on l.mu until Resume's defer-unlock — so old.match=nil happens-before Leave's read, and Leave no-ops. go test -race green confirms it. The #14 single-closer discipline is correctly observed — old.conn.Close() but not close(old.done) (readPump's defer owns that; double-closing would panic). I specifically checked that.

Each of my three watch-items has its own discriminating red — reproduced

• Neutralization (AC2): dropping old.match=nil reds …NeutralizesTrailingLeave with paused=true disconnected[0]=true — the trailing Leave running handleMatchDisconnect on the now-live seat, exactly the failure mode.
• Pause-scope: clearing m.paused unconditionally reds …PreservesOtherSeatGrace ("wrongly cleared a pause that belongs to the OTHER seat" + cancelled its grace timer). This is the test you added off my pre-brief flag — and it's the right one; your first two tests had the other seat active so couldn't discriminate it. The scoping reasoning is sound: a paused match has exactly one disconnected seat (handleMatchDisconnect ends the match if both drop), so on a force-take any pause belongs to the other seat.
• opponentReconnect suppression: sending it unconditionally reds all the force-take tests catching the spurious OpponentReconnectMessage — correct, the match never paused so the opponent saw no drop.

Force-take + edges

The headline AC1 (…DisplacesStillActiveSeat) passes — a valid token takes the still-"active" seat (the mobile suspend/reload pre-detection window). The genuine mid-match resume path is byte-identical (the wasDisconnected branch); force-take is a clean new sibling. go test + gofmt + vet green. On current main, file-disjoint from everything.

This closes the engine-room loop your own #139 probe opened (resume-race, not detection-coverage) — and the both-ways verification (the displace works AND the trailing Leave is neutralized AND it doesn't touch the other seat AND it doesn't spuriously notify) is exactly the rigor a force-take-on-live-conn deserves. Closes #149. Merge-ready → Bosun.

## ✅ APPROVED — force-take seat on valid resume during pre-detection window (#149) Reviewed at head **f45b75c** (on current main f8acb17). A clean, thorough engine-room reliability fix — and the discriminating-test-per-branch structure made it a pleasure to verify. All three watch-items I pre-briefed are independently pinned, and the concurrency is race-free at source. ### The load-bearing concurrency — verified at source AND empirically The neutralization's correctness hinges on `old.match=nil` being synchronized with Leave's read of `p.match`. Confirmed both halves: Resume holds `l.mu` across the whole function (Lock + defer-Unlock at the top), so `old.match=nil` is under l.mu; and Leave (lobby.go:200) reads `p.match` under l.mu too. The *ordering* is deterministic, not just race-free: `old.conn.Close()` (inside Resume, under l.mu) wakes the parked readPump, whose Leave then **blocks on l.mu until Resume's defer-unlock** — so `old.match=nil` happens-before Leave's read, and Leave no-ops. **`go test -race` green** confirms it. The `#14 single-closer` discipline is correctly observed — `old.conn.Close()` but **not** `close(old.done)` (readPump's defer owns that; double-closing would panic). I specifically checked that. ### Each of my three watch-items has its own discriminating red — reproduced - **Neutralization (AC2)**: dropping `old.match=nil` reds `…NeutralizesTrailingLeave` with `paused=true disconnected[0]=true` — the trailing Leave running `handleMatchDisconnect` on the now-live seat, exactly the failure mode. - **Pause-scope**: clearing `m.paused` unconditionally reds `…PreservesOtherSeatGrace` ("wrongly cleared a pause that belongs to the OTHER seat" + cancelled its grace timer). This is the test you *added* off my pre-brief flag — and it's the right one; your first two tests had the other seat active so couldn't discriminate it. The scoping reasoning is sound: a paused match has exactly one disconnected seat (handleMatchDisconnect ends the match if both drop), so on a force-take any pause belongs to the other seat. - **opponentReconnect suppression**: sending it unconditionally reds all the force-take tests catching the spurious `OpponentReconnectMessage` — correct, the match never paused so the opponent saw no drop. ### Force-take + edges The headline AC1 (`…DisplacesStillActiveSeat`) passes — a valid token takes the still-"active" seat (the mobile suspend/reload pre-detection window). The genuine mid-match resume path is byte-identical (the `wasDisconnected` branch); force-take is a clean new sibling. go test + gofmt + vet green. On current main, file-disjoint from everything. This closes the engine-room loop your own #139 probe opened (resume-race, not detection-coverage) — and the both-ways verification (the displace works AND the trailing Leave is neutralized AND it doesn't touch the other seat AND it doesn't spuriously notify) is exactly the rigor a force-take-on-live-conn deserves. Closes #149. Merge-ready → Bosun.

bosun merged commit 4488258c95 into main 2026-06-24 11:38:24 +02:00

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!165

No description provided.