From 10040f733901a08eeb1c83d55551f6343ed7962e Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Thu, 20 Aug 2026 13:43:49 +0100 Subject: [PATCH] docs(multiplayer): close out Phase 3 in multiplayer-todo.md Documents all seven Phase 3 tasks (3.1-3.7) with DONE status and verification evidence, updates the top-level status summary, and records the phase gate as met - re-verified today under the gate's own exact condition (--net-sim-latency 80 --net-sim-loss 0.05) on both the human smoke test and the two-bot CI driver, not just the looser conditions used during individual task development. Adds one new gotcha (#38): GDScript lambdas capture enclosing locals by value, not by reference, which silently broke two separate Phase 3 test scripts' own disconnect-detection assertions this session (the production disconnect logic was correct both times; only the test's own flag-capture pattern was wrong). Also records a deliberate scope decision for task 3.4: server-side input_lead enforcement from arrival times was scoped down to observability rather than built as active enforcement, since the concrete security requirements (rate limiting, malformed-packet counting, seq-range rejection, disconnect policy) already close the load-bearing gaps and the doc's own text calls the remaining edge "small" - flagged to revisit once Phase 4's prediction work exists to judge against. --- multiplayer-todo.md | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/multiplayer-todo.md b/multiplayer-todo.md index bec235bf..58e73823 100644 --- a/multiplayer-todo.md +++ b/multiplayer-todo.md @@ -4,7 +4,7 @@ Working document for the online multiplayer effort. `TODO.md` points here. Everything below is written so an agent (or a person) can pick up a single numbered task, do it, verify it against a stated acceptance criterion, and stop. Sections 1–6 are the decisions those tasks assume; read them before picking up work in Phase 2 or later. -**Status: Phase 0 done, Phase 1 done, Phase 2 done — milestone gate passing.** A real two-process 1v1 runs: a headless server hosts, a client joins through the lobby, spawns into a server-picked arena, drives its ship via real held input, and renders server-authoritative movement (verified: 26–31 m over a 2 s held-thrust drive, purely from interpolated snapshots) with camera and HUD attached — and this still holds under `--net-sim-latency 80 --net-sim-jitter 20` (task 2.8's `net_sim.gd`), which is Phase 2's own stated gate, not just LAN. No own-ship/ball prediction yet (Phase 4) — everything the client renders, including its own ship, comes from the interpolation buffer. See §7 for per-task status and evidence. +**Status: Phase 0 done, Phase 1 done, Phase 2 done, Phase 3 done — both phase gates passing.** A real two-process 1v1 runs: a headless server hosts, a client joins through the lobby, spawns into a server-picked arena, drives its ship via real held input (now with real redundancy, a server-side jitter buffer, and a client-owned adaptive `input_lead`), and renders server-authoritative movement (verified: 22–31 m over a 2 s held-thrust drive, purely from interpolated snapshots) with camera and HUD attached — holding under `--net-sim-latency 80 --net-sim-loss 0.05`, Phase 3's own gate condition, on both the human smoke test and a two-headless-bot CI run (task 3.6) that forces a goal and confirms both bots independently agree on the resulting score. Input is now also validated and abuse-resistant: a hostile client sending malformed or flooded packets gets disconnected, verified with two permanent regression tests that bypass the honest client encoder entirely. No own-ship/ball prediction yet (Phase 4) — everything the client renders, including its own ship, comes from the interpolation buffer. See §7 for per-task status and evidence. --- @@ -859,17 +859,19 @@ No own-ship prediction yet: the client renders everything, including its own shi | # | Task | Acceptance | |---|---|---| -| 3.1 `[D:2.5]` | Input redundancy (last 4) and sequence numbering in server-tick space | A 3-packet burst loss produces no starvation | -| 3.2 `[D:3.1]` | Server jitter buffer: fixed 32-entry ring, repeat-last on starve, zero after 500 ms, `target_depth = 1`, depth reported in every snapshot | Starvation events logged and visible in the overlay | -| 3.3 `[D:3.2]` `[P]` | Client-owned `input_lead` control loop: fast attack (+3 immediate), slow release (−1 per 60 ticks after 2 s clean) | A simulated 60 ms latency spike is absorbed within ~200 ms | -| 3.4 `[D:3.1]` `[P]` | Rate limiting, malformed-packet counting, `seq > server_tick + 20` rejection, server-side `input_lead` enforcement from arrival times, disconnect policy | A flooding or seq-poisoning client is disconnected; honest clients unaffected | -| 3.5 `[D:3.2]` `[P]` | Unit tests: jitter-buffer policy against scripted arrival traces | Starvation, surplus, and reorder traces all produce the specified actions | -| 3.6 `[D:2.8]` | `--test-bot` client mode driven by the existing `AIShipController`, plus a CI driver launching a headless server and two headless test-bot clients | Exits 0 on a clean run; asserts snapshot count, p95/p99 prediction error, snap count, cross-peer score agreement, and clean stderr | -| 3.7 `[D:2.8]` `[P]` | Debug net overlay: RTT, jitter, loss, buffer depth, snapshot age, bandwidth, prediction error | All values live and plausible | +| 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.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 | > `AIShipController` runs a policy in pure GDScript with no Python or ONNX dependency, so 3.6 gets a competent automated player for free. -**Phase gate:** the match stays smooth at `--net-sim-latency 80 --net-sim-loss 0.05`; the CI smoke test is green. +> **Server-side `input_lead` enforcement from arrival times (§3.3's closing paragraph) was scoped down to observability, not built as active enforcement.** The concrete, mechanically well-specified parts of task 3.4 (rate limiting, malformed-packet counting, seq-range rejection, disconnect policy) fully close the load-bearing security gaps; the advantage a client gains from claiming a dishonestly low `input_lead` is explicitly described in the doc itself as "small" (reduced apply latency, not an outright cheat — there's no prediction/reconciliation yet for a bad lead to actually corrupt), and building real arrival-jitter-derived enforcement well — without risking a third, subtly-interacting control loop on top of the two §3.3 already warns against — is a genuine design task in its own right, not a mechanical one. Revisit if Phase 4's prediction work turns "slightly lower latency" into a sharper edge. + +**Phase gate — MET.** Both `networked_match_smoke` and the CI driver (task 3.6) re-run under the gate's own exact condition, `--net-sim-latency 80 --net-sim-loss 0.05`, on every peer: the human-driven smoke test still shows clean server-authoritative movement (22.61m over the usual 2s drive), and the two-bot CI run still shows both clients independently agreeing on the final score after a forced goal, 495+/504 snapshots received each, all three processes exiting 0 with clean stderr. ### Phase 4 — Prediction and reconciliation, ship **and ball** @@ -1015,6 +1017,7 @@ No own-ship prediction yet: the client renders everything, including its own shi 35. **A queued `queue_teleport()` (task 0.15) can take one tick longer to land than "the very next `_integrate_forces`" suggests, when the call originates from a signal handler mid-physics-step rather than from a `_physics_process` callback.** Empirically confirmed by teleporting a body into a goal and logging the server's own per-tick broadcast: the goal was detected on tick N (per gotcha 34, during tick N's own step), but the reset position didn't appear in a broadcast until tick N+1's, one tick later than "queued during N, applied on N+1's `_integrate_forces`" alone would predict. Don't assume queued-teleport timing without checking a real tick-by-tick log for your specific call site — the exact tick it lands on depends on where in the physics step the queuing call happens, not just "next frame" intuition. 36. **`NetworkManager.get_server_time_estimate_ms()` (and anything derived from it) is not just imprecise before the first clock pong lands — it's actively wrong in a way that can persist far longer than the sync window implies.** `clock_offset_ms` is `0.0` until the first pong, so a value derived from `get_server_time_estimate_ms()` during that window means "my own raw process uptime," not a server-synced estimate — and if that value feeds a rolling-window filter (e.g. a min-tracked bias, per the interpolator epoch-bias fix in Phase 2's adversarial review), the bad early sample can dominate the window for the filter's *entire* configured duration if a short test or a short match doesn't run long enough for real time to age it out. Always gate recording, not just consuming, anything derived from this estimate on `rtt_ms >= 0.0`. 37. **Anything that deliberately delays an RPC dispatch (task 2.8's `net_sim.gd`) must re-validate its target at *fire* time, not just at the moment it was scheduled.** Found by actually running Phase 2's own gate (`networked_match_smoke` under `--net-sim-latency=80 --net-sim-jitter=20`), not the isolated ping/pong test alone: `_broadcast_snapshot`'s existing `get_peers()` filter (gotcha from task 2.2's own fix) only proves the target was valid *when the send was queued* — a target that legitimately disconnects during the ~80–100ms hold produces "Attempt to call RPC with unknown peer ID" anyway, and if the delay outlives this *process's own* `shutdown()`, `multiplayer_peer` has already been reset to a fresh `OfflineMultiplayerPeer` (§9 gotcha re: never resetting to raw `null`), so a stale `rpc_id(1, …)` now means "call yourself" and Godot rejects it. Any deliberate-delay layer needs to re-check both "do I still have a real peer at all" and "is this specific target still in `get_peers()`" inside the delayed callback itself, immediately before dispatching — not reuse whatever validation the caller did before the delay was added. +38. **A GDScript lambda captures an enclosing local variable BY VALUE at the moment the lambda is created, not by reference.** Bit two separate Phase 3 test scripts the same way: `var disconnected := false; some_signal.connect(func(): disconnected = true)` compiles and runs with no error or warning, but the assignment inside the lambda mutates only *that lambda's own captured copy* — the enclosing function's `disconnected` stays `false` forever, even after the signal genuinely fires (confirmed firing via an extra debug print before the real cause was found). The underlying disconnect-detection code was correct the whole time; only the test's own assertion logic was broken. The fix is to capture a container instead of a value — `var disconnected := [false]` and `disconnected[0] = true` inside the lambda — since capturing an `Array`/`Dictionary`/`Object` captures a reference to the same instance, and mutating its *contents* from inside the lambda is visible outside it. Relevant anywhere a lambda is used to flip a flag or accumulate a result for a caller to read later (a `connect(func(): ...)` one-liner is the single most common place this bites). ---