From afcb01d155906bd9aeef852531a8321535036e2c Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Tue, 1 Sep 2026 08:27:56 +0100 Subject: [PATCH] fix: guard match simulation after transport shutdown --- Game/scripts/match_sim.gd | 7 ++++++- multiplayer-todo.md | 2 +- 2 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Game/scripts/match_sim.gd b/Game/scripts/match_sim.gd index 0396ed3c..84e040a4 100644 --- a/Game/scripts/match_sim.gd +++ b/Game/scripts/match_sim.gd @@ -192,7 +192,12 @@ func _physics_process(_delta: float) -> void: var now := Time.get_ticks_msec() var gap := now - _last_physics_ms _last_physics_ms = now - if not multiplayer.is_server() or _peer_input_state.is_empty(): + # NetworkManager.shutdown() swaps in an OfflineMultiplayerPeer before the + # smoke harness's deferred quit runs. Querying MultiplayerAPI.is_server() + # during that hand-off can call get_unique_id() on an inactive ENet peer and + # emit errors every physics frame; the NetworkManager role flag is the safe + # lifecycle guard at this boundary. + if not NetworkManager.is_server or _peer_input_state.is_empty(): return if gap < STALL_DETECT_MS: return diff --git a/multiplayer-todo.md b/multiplayer-todo.md index 6b14bc9a..40cc8b8f 100644 --- a/multiplayer-todo.md +++ b/multiplayer-todo.md @@ -919,7 +919,7 @@ No own-ship prediction yet: the client renders everything, including its own shi | 3.1 `[D:2.5]` | **DONE.** Client sends the last `NetCodec.MAX_REDUNDANCY` (4) ticks' actions per packet, newest-first (the wire format already supported this from Phase 1 — Phase 2 just wasn't using it). Server gains a real per-slot ring buffer, new standalone `scripts/input_jitter_buffer.gd` (`InputJitterBuffer`, `RefCounted`, no scene dependency — same reason `net_codec.gd`/`net_interpolator.gd` are pure classes), consuming exactly one sequence number per physics tick | Verified both by unit test (`test_redundancy_survives_3_packet_burst_loss`) and live: 25% random simulated input loss produced zero observed starvation ticks; 100% loss correctly produced zero seeding/consumption (no crash, ship simply never receives a command) | | 3.2 `[D:3.1]` | **DONE.** `InputJitterBuffer.consume()`: repeat-last on starve, zero + `stalled=true` only after `STARVE_ZERO_TICKS` (30 = 500ms). `input_buffer_depth`/`last_input_seq`/`echo_client_send_ms` are now genuinely per-peer in every snapshot (`_broadcast_snapshot` builds them from each slot's own `InputJitterBuffer`), replacing Phase 2's hardcoded zeros | One real bug found wiring this into a live match: the server's ring buffer started counting its own "expected sequence" from a local 0 the instant a slot was created — well before that player's first real packet could possibly arrive (connection/spawn setup takes real time) — so the two numberings never converged and the ship silently never moved. Fixed by seeding `last_applied_seq` from the client's own numbering on first real `ingest()`, not assuming a shared from-zero baseline. Verified with real two-process runs before and after the fix | | 3.3 `[D:3.2]` `[P]` | **DONE.** New standalone `scripts/input_lead_controller.gd` (`InputLeadController`, unit-tested like `InputJitterBuffer`): clamp `[1,12]`, fast attack (+3, debounced to once per 30 ticks) on any server-reported starve, slow release (−1 per 60 ticks) gated behind a one-time 2s clean-surplus bar. A lead change is realized as extra distance between the client's own outgoing seq and what the server has consumed — attack skips extra seq numbers, release duplicates (re-sends) the current one; the server's ring buffer needs no special handling for either, since a skip is an ordinary drop and a duplicate is a same-seq resend already discarded | Verified live: on a clean LAN, one early attack (a momentary connection-setup hiccup) recovered via two releases within ~4s, settling back near minimum; under sustained 30% simulated loss, lead climbed to 7 via repeated attacks and never released while genuine loss continued — confirming debounce, attack, and release gates all fire correctly on real conditions | -| 3.4 `[D:3.1]` `[P]` | **DONE.** `MatchSim._recv_input` validates before decoding: per-peer rolling-1s rate limit (packet count AND byte budget, §3.1's own numbers), disconnect after 3 consecutive over-budget seconds; framing (redundancy count + payload size checked against `NetCodec`'s own layout, since `StreamPeerBuffer` silently zero-fills past EOF instead of erroring — a Phase 2 adversarial-review finding), disconnect after 20 malformed packets. `networked_match.gd` additionally rejects `seq > server_tick + 20` and counts (rather than silently ignoring) input from a peer with no slot. Server-side `input_lead` enforcement from arrival times was scoped down to observability rather than active enforcement — see the note below the table | Two new **permanent** regression tests (`networked_match_smoke.gd --role=client-abuse-malformed` / `client-abuse-flood`) call `MatchSim._recv_input` directly with garbage bytes and a legitimate-but-too-frequent flood respectively — bypassing the honest client encoder entirely, i.e. exactly what a hostile custom client sending raw ENet packets looks like. Both confirm a real disconnect, not just tolerance. Found and fixed two smaller bugs getting these to pass cleanly: a GDScript lambda-captures-by-value mistake in the tests themselves (fixed by capturing a single-element `Array` instead of a plain `bool`), and a real race where `NetworkManager`'s own ping/pong reply could target a peer a concurrent abuse-disconnect had just removed from the same `poll()` batch (now guarded) | +| 3.4 `[D:3.1]` `[P]` | **DONE.** `MatchSim._recv_input` validates before decoding: per-peer rolling-1s rate limit (packet count AND byte budget, §3.1's own numbers), disconnect after 3 consecutive over-budget seconds; framing (redundancy count + payload size checked against `NetCodec`'s own layout, since `StreamPeerBuffer` silently zero-fills past EOF instead of erroring — a Phase 2 adversarial-review finding), disconnect after 20 malformed packets. `networked_match.gd` additionally rejects `seq > server_tick + 20` and counts (rather than silently ignoring) input from a peer with no slot. Server-side `input_lead` enforcement from arrival times was scoped down to observability rather than active enforcement — see the note below the table | Two new **permanent** regression tests (`networked_match_smoke.gd --role=client-abuse-malformed` / `client-abuse-flood`) call `MatchSim._recv_input` directly with garbage bytes and a legitimate-but-too-frequent flood respectively — bypassing the honest client encoder entirely, i.e. exactly what a hostile custom client sending raw ENet packets looks like. Both confirm a real disconnect, not just tolerance. Found and fixed two smaller bugs getting these to pass cleanly: a GDScript lambda-captures-by-value mistake in the tests themselves (fixed by capturing a single-element `Array` instead of a plain `bool`), and a real race where `NetworkManager`'s own ping/pong reply could target a peer a concurrent abuse-disconnect had just removed from the same `poll()` batch (now guarded); the post-shutdown physics path now also uses the safe NetworkManager lifecycle flag and stays error-free | | 3.5 `[D:3.2]` `[P]` | **DONE.** `tests/cases/test_input_jitter_buffer.gd` and `test_input_lead_controller.gd`: sequential consumption, redundancy surviving a 3-packet burst loss (3.1's own acceptance text, verbatim), starvation repeat-then-zero timing, stale/reordered-packet handling, buffered-depth reporting, ring-wraparound slot-tagging safety, and the full attack/debounce/release state machine including a starve mid-release-window forcing a fresh clean-surplus wait | 14 new tests, all passing (`test_runner.tscn`: 33 total, 0 failed) | | 3.6 `[D:2.8]` | **DONE**, with one honest scope note. `networked_match.gd`'s client can swap its input sampler for a real `AIShipController` (`--test-bot`, optionally `--test-bot-model=`) instead of `PlayerShipController` — parented onto the client's own ship via `Ship.set_controller()` since (unlike the human sampler) it needs real scene context. **Known limitation, documented in code**: this client's ships are all `FREEZE_MODE_KINEMATIC`, driven purely by transform writes, so nothing ever writes `linear_velocity`/`angular_velocity` onto them — the bot's observations always see every ship as stationary. It still produces well-formed, bounded actions from that degraded input (the policy network's output layer is bounded regardless of input quality), sufficient for this task's actual job (CI traffic generation, not bot skill). New CI driver `tests/networked_match_ci.gd`/`.tscn`: headless server + two headless `--test-bot` clients. **This task's own original acceptance text names "p95/p99 prediction error" and "snap count" — both Phase 4 concepts that don't exist yet** (no client-side prediction or hard-snap threshold exists before Phase 4); asserting on data that doesn't exist would be fabricated, so those two are explicitly not checked, with the gap called out in the driver's own header comment rather than silently dropped | Real 3-process runs: both bots' independently-written final scores agreed after a deterministically forced goal (bot-vs-bot scoring isn't reliable enough within a short run to gate on), both saw 500+ snapshots over an 8s run (well above the 60Hz-scaled floor), all three processes exited 0. "Clean stderr" is the external invocation's job (grep the captured output), same as every other smoke test in this project — verified manually, not self-asserted by the script | | 3.7 `[D:2.8]` `[P]` | **DONE**, with prediction error deliberately omitted (documented, not silently dropped — same Phase 4 gap as 3.6). Extends `net_debug_overlay.gd` with jitter (new RFC3550-style EWMA in `NetworkManager`, from raw per-sample RTT — Phase 1's `rtt_ms` is a min-filtered sample, deliberately jitter-insensitive by design, so it can't answer this on its own), snapshot loss (new EWMA in `networked_match.gd` over each received snapshot's own `server_tick` gap — snapshots go out at a steady one-tick cadence, so a gap is direct evidence of a drop or reorder), snapshot age (computed on demand from the same bias-corrected tick estimate the interpolator itself uses), input buffer depth and `input_lead` (both already tracked client-side for 3.3), and bandwidth (new rolling per-second byte counters in `MatchSim`, the two 60Hz hot-path channels only) | Verified values are live and plausible, not just present, by calling `get_net_debug_stats()` directly in a real two-process test: bandwidth matched the wire format's own byte math almost exactly (measured ≈2400 B/s sent against a computed 40B×60Hz, ≈3540 B/s received against 59B×60Hz for a 1v1), and buffer depth/lead/loss all moved in the correct direction between a clean LAN run and one under simulated 60ms latency + 10% loss |