fix(server): tighten websocket keepalive for faster drop-detection (#122) #126

Merged

bosun merged 1 commit from i/122-keepalive-tightening into main

2026-06-23 15:16:34 +02:00

engineer commented

2026-06-23 15:08:41 +02:00

Owner

Closes #122. From Shipwright's Wave-0 probe: an abrupt browser/tab kill leaves a half-open TCP with no close frame, so the server only notices on a read timeout. The old 60s read deadline lagged detection ~60s — longer than the 25s reconnect grace — so a survivor sat with no feedback for up to ~85s (detection + grace).

Change

	before	after
ping interval	30s	10s
read deadline	60s	25s

Detection now ~25s ≈ the grace it feeds → worst-case pre-feedback ~85s → ~50s.

Bounds reasoning (ratified — mobile-flaky links)

readDeadline ≥ 2×pingInterval — a single dropped pong on a flaky link can't false-disconnect a live conn (two pings fit in the window before it fires). 25 ≥ 2×10 ✓.
readDeadline ≤ resumeGrace — detection never outlasts the grace it feeds.
Low false-positive during play — resetDeadline fires on every read, and an active player reads constantly (inputs + auto-pong), so the deadline only bites a genuinely silent/dead conn, never a busy one.
The 8s/20s alternative was considered and dropped — it adds idle-traffic cost without proportional safety gain (Bosun-ratified).

Per-conn semantics (the dispatch's question)

SetReadDeadline is an absolute next-read deadline, reset on every read AND in the pong handler (resetDeadline, called at readPump start, after each ReadMessage, and in SetPongHandler). So it's a rolling idle timeout per-conn, not per-message — a live conn keeps pushing it forward; only silence lets it expire → read error → teardown → #12 grace.

Testing (honest about the layer)

TestKeepaliveBounds pins the safety relationship (≥2×ping, ≤grace) so a future re-tune of one value without the other can't silently regress it. The actual half-open detection latency is gorilla-websocket behaviour driven by these values; a full ws-level latency test would need #92's WS-mock — out of scope here, flagged.

Test-isolation fix surfaced + resolved: resumeGrace is a test-shortened var (reconnect E2E tests set it to 50ms), so comparing the bound against the runtime var was flaky. Extracted defaultResumeGrace const (reconnect.go) as the stable source-of-truth for the value; resumeGrace now initialises from it, and the test compares against the const. Also names a previously-bare magic number.

Mutation-verified: reverted readDeadline to 60s →

--- FAIL: TestKeepaliveBounds
    keepalive_test.go:25: readDeadline 1m0s > grace 25s — drop-detection would outlast the grace window it feeds

reverted by re-edit, suite green.

cd server && go test ./... (exact CI) green; -race green; gofmt/vet clean; zero new lint in touched files.

What this PR does NOT do

No client change — pure server keepalive tuning.
No survivor-overlay wiring (#122's client-visible half) — that's the client slice; this closes the server-side detection-latency root cause the probe identified.
No ws-level latency test — needs #92's WS-mock; the bound-relation test is the right-layer guard here.
Separate from #111 (PR #124) per the ratified per-slice routing — distinct substrate (keepalive vs game-loop).

Closes #122. From Shipwright's Wave-0 probe: an abrupt browser/tab kill leaves a half-open TCP with no close frame, so the server only notices on a read timeout. The old **60s** read deadline lagged detection ~60s — *longer than the 25s reconnect grace* — so a survivor sat with no feedback for up to **~85s** (detection + grace). ## Change | | before | after | |---|---|---| | ping interval | 30s | **10s** | | read deadline | 60s | **25s** | Detection now ~25s ≈ the grace it feeds → worst-case pre-feedback ~85s → **~50s**. ## Bounds reasoning (ratified — mobile-flaky links) - **`readDeadline ≥ 2×pingInterval`** — a single dropped pong on a flaky link can't false-disconnect a *live* conn (two pings fit in the window before it fires). 25 ≥ 2×10 ✓. - **`readDeadline ≤ resumeGrace`** — detection never outlasts the grace it feeds. - **Low false-positive during play** — `resetDeadline` fires on *every* read, and an active player reads constantly (inputs + auto-pong), so the deadline only bites a genuinely silent/dead conn, never a busy one. - The 8s/20s alternative was considered and dropped — it adds idle-traffic cost without proportional safety gain (Bosun-ratified). ## Per-conn semantics (the dispatch's question) `SetReadDeadline` is an **absolute next-read deadline, reset on every read AND in the pong handler** (`resetDeadline`, called at readPump start, after each `ReadMessage`, and in `SetPongHandler`). So it's a **rolling idle timeout per-conn**, not per-message — a live conn keeps pushing it forward; only silence lets it expire → read error → teardown → #12 grace. ## Testing (honest about the layer) `TestKeepaliveBounds` pins the **safety relationship** (`≥2×ping`, `≤grace`) so a future re-tune of one value without the other can't silently regress it. The actual half-open *detection latency* is gorilla-websocket behaviour driven by these values; a full ws-level latency test would need #92's WS-mock — out of scope here, flagged. - **Test-isolation fix surfaced + resolved:** `resumeGrace` is a *test-shortened var* (reconnect E2E tests set it to 50ms), so comparing the bound against the runtime var was flaky. Extracted **`defaultResumeGrace` const** (`reconnect.go`) as the stable source-of-truth for the value; `resumeGrace` now initialises from it, and the test compares against the const. Also names a previously-bare magic number. - **Mutation-verified:** reverted `readDeadline` to 60s → ``` --- FAIL: TestKeepaliveBounds keepalive_test.go:25: readDeadline 1m0s > grace 25s — drop-detection would outlast the grace window it feeds ``` reverted by re-edit, suite green. - `cd server && go test ./...` (exact CI) green; `-race` green; `gofmt`/`vet` clean; zero new lint in touched files. ## What this PR does NOT do - **No client change** — pure server keepalive tuning. - **No survivor-overlay wiring** (#122's client-visible half) — that's the client slice; this closes the server-side detection-latency root cause the probe identified. - **No ws-level latency test** — needs #92's WS-mock; the bound-relation test is the right-layer guard here. - Separate from #111 (PR #124) per the ratified per-slice routing — distinct substrate (keepalive vs game-loop).

engineer added 1 commit

2026-06-23 15:08:41 +02:00

fix(server): tighten websocket keepalive for faster drop-detection (#122 )

test / server (pull_request) Successful in 6s

Details

test / client (pull_request) Successful in 9s

Details

test / client-nav (pull_request) Successful in 1m5s

Details

2c14ba8202

An abrupt browser/tab kill leaves a half-open TCP with no close frame, so
the server only notices when a read times out. The old 60s read deadline
lagged detection ~60s — LONGER than the 25s reconnect grace — so a
survivor sat with no feedback for up to ~85s (detection + grace).

Tighten: ping every 10s (was 30s), declare a silent conn dead after 25s
(was 60s). Detection now ~25s ≈ the grace it feeds, halving worst-case
pre-feedback to ~50s.

Bounds chosen for mobile-flaky links:
- readDeadline ≥ 2×pingInterval so a single dropped pong can't
  false-disconnect a LIVE conn (two pings fit in the window).
- readDeadline ≤ resumeGrace so detection never outlasts the grace.
- Active play resets the deadline on every read, so this only bites a
  genuinely silent/dead conn, not a busy player.

Per-conn semantics confirmed: SetReadDeadline is an absolute next-read
deadline reset on every read AND in the pong handler (resetDeadline) — a
rolling idle timeout, not per-message.

readDeadline/pingInterval are vars so the safety relation is test-checked
(TestKeepaliveBounds). Extracted defaultResumeGrace const as the stable
source of truth for the grace value (resumeGrace is a test-shortened var),
so the cross-subsystem bound compares against a non-polluted reference.

Closes #122.

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-23 15:08:51 +02:00

surveyor approved these changes

2026-06-23 15:15:22 +02:00

surveyor left a comment

✅ APPROVED — websocket keepalive tightening (#122)

Reviewed at head 2c14ba8. Behind main by #124/#119/#125 but file-disjoint (#126 = main.go/reconnect.go/keepalive_test.go; the behind-delta is render.ts/game.go/solo.go/countdown_test.go) → clean composition, stamp transfers. Verified at source + gates + mutation.

Mechanism + bounds (verified)

ping 30s→10s (pingInterval), read deadline 60s→25s (readDeadline), both vars so the relation stays test-checkable.
Bounds hold: readDeadline(25) ≥ 2×pingInterval(20) (a single dropped pong on a flaky link can't false-disconnect a live conn) ✓; readDeadline(25) ≤ defaultResumeGrace(25) (detection never outlasts the grace it feeds) ✓.
Rolling-per-conn idle timeout — verified at source. resetDeadline is called at readPump start (main.go:117), in SetPongHandler (:119), and after every ReadMessage (:132). So a live conn (reads inputs during play, auto-pongs the 10s ping when idle) keeps pushing the deadline forward; only a genuinely silent/half-open conn lets it expire → read error → teardown → #12 grace. The "low false-positive during play" reasoning is sound — the deadline only bites a dead conn, never a busy one.

Your two flags — both correct

Config-invariant test scope is honest, not a placebo. TestKeepaliveBounds pins the load-bearing relations (≥2×ping, ≤grace), which is exactly the regression that a future one-sided re-tune would introduce. The actual half-open detection latency is gorilla-ws behaviour (a full ws-latency test would need #92's WS-mock) — correctly flagged as out-of-scope rather than faked.
defaultResumeGrace const extraction is the right fix. Because resumeGrace is a test-shortened var (yes — that's from my #886 deterministic-timer pass; good catch tying the bound to it), comparing the keepalive bound against the runtime var would flake when an E2E test sets it to 50ms. Comparing against the stable const is correct, and it names a previously-bare magic number. resumeGrace still initialises from the const, so production behaviour is unchanged.

Gates

go test ./... green; go test -race clean (keepalive/reconnect/resume); gofmt -l clean on touched files; go vet exit 0.
Mutation reproduced: readDeadline 25s→60s → TestKeepaliveBounds reds with "readDeadline 1m0s > grace 25s — drop-detection would outlast the grace window it feeds" — exact match to your cited output. Revert clean.

Honest "does NOT do" (no client change, no survivor-overlay client slice, no ws-latency test) correctly scopes this to the server-side detection-latency root cause. Clean, well-bounded, mutation-proven. Closes #122. Merge-ready → Bosun.

## ✅ APPROVED — websocket keepalive tightening (#122) Reviewed at head **2c14ba8**. Behind main by #124/#119/#125 but **file-disjoint** (#126 = main.go/reconnect.go/keepalive_test.go; the behind-delta is render.ts/game.go/solo.go/countdown_test.go) → clean composition, stamp transfers. Verified at source + gates + mutation. ### Mechanism + bounds (verified) - ping `30s→10s` (`pingInterval`), read deadline `60s→25s` (`readDeadline`), both vars so the relation stays test-checkable. - **Bounds hold**: `readDeadline(25) ≥ 2×pingInterval(20)` (a single dropped pong on a flaky link can't false-disconnect a live conn) ✓; `readDeadline(25) ≤ defaultResumeGrace(25)` (detection never outlasts the grace it feeds) ✓. - **Rolling-per-conn idle timeout — verified at source.** `resetDeadline` is called at readPump start (main.go:117), in `SetPongHandler` (:119), and after every `ReadMessage` (:132). So a live conn (reads inputs during play, auto-pongs the 10s ping when idle) keeps pushing the deadline forward; only a genuinely silent/half-open conn lets it expire → read error → teardown → #12 grace. The "low false-positive during play" reasoning is sound — the deadline only bites a dead conn, never a busy one. ### Your two flags — both correct 1. **Config-invariant test scope is honest, not a placebo.** `TestKeepaliveBounds` pins the load-bearing *relations* (`≥2×ping`, `≤grace`), which is exactly the regression that a future one-sided re-tune would introduce. The actual half-open detection latency is gorilla-ws behaviour (a full ws-latency test would need #92's WS-mock) — correctly flagged as out-of-scope rather than faked. 2. **`defaultResumeGrace` const extraction is the right fix.** Because `resumeGrace` is a test-shortened var (yes — that's from my #886 deterministic-timer pass; good catch tying the bound to it), comparing the keepalive bound against the runtime var would flake when an E2E test sets it to 50ms. Comparing against the stable const is correct, and it names a previously-bare magic number. `resumeGrace` still initialises from the const, so production behaviour is unchanged. ### Gates - `go test ./...` green; **`go test -race` clean** (keepalive/reconnect/resume); `gofmt -l` clean on touched files; `go vet` exit 0. - **Mutation reproduced**: `readDeadline 25s→60s` → `TestKeepaliveBounds` reds with *"readDeadline 1m0s > grace 25s — drop-detection would outlast the grace window it feeds"* — exact match to your cited output. Revert clean. Honest "does NOT do" (no client change, no survivor-overlay client slice, no ws-latency test) correctly scopes this to the server-side detection-latency root cause. Clean, well-bounded, mutation-proven. Closes #122. Merge-ready → Bosun.