From 14698d4ccbeca89ac70a85418d05719801706fc2 Mon Sep 17 00:00:00 2001
From: Josh Creek <8179928+jcreek@users.noreply.github.com>
Date: Thu, 20 Aug 2026 12:43:33 +0100
Subject: [PATCH] fix(multiplayer): adversarial review fixes for Phase 2
An Opus subagent's adversarial review of Phase 2 found real bugs the
smoke tests couldn't catch, since constant-velocity dead reckoning still
moves a ship far enough to pass a "moved > 1.0" check:
- The interpolator never actually interpolated. NetInterpolator.to_tick()
assumes physics_frame * TICK_MS == Time.get_ticks_msec() on the server,
which is off by a steady ~45-55ms in practice (real startup work before
the first physics step, widened by any dropped tick). Every sample_at()
call took the extrapolation branch, 100% of the time, defeating the
interpolation buffer entirely. Fixed with a shared, min-filtered rolling
bias estimate in networked_match.gd, applied before every to_tick() call.
- Goals caused a ~27m visual slide: _reset_gen was bumped before the
queued teleport actually landed, so the client's buffer-clear kept
exactly the stale in-goal sample and lerped a slide to the next, real
one. Fixed by tracking the tick the goal was detected on and only
bumping the generation once strictly later ticks confirm the teleport
has landed - a naive "next _physics_process" boolean flag doesn't
work, since a goal Area's body_entered fires before that same tick's
_physics_process runs, not on the next one.
- _local_input_sampler (a Node, never added to the tree) was never freed
- this was the unexplained "3 resources still in use at exit" warning
on every Phase 2 test run.
- Ball angular velocity decoded 8x too small (rescale_avel was never
called); get_server_time_estimate_ms() was used before the clock had
synced; net_sim.gd's delayed-send timer stopped ticking while the tree
was paused and didn't check connection status before firing;
_broadcast_snapshot's ball index could silently break if a ship were
ever despawned; declared-but-unemitted HUD lifecycle signals showed a
permanently frozen timer widget.
Also confirmed, empirically, several things the review checked and found
fine: a hostile client sending malformed input cannot crash the server,
skipping GameMode's super() drops nothing load-bearing, deterministic
slot assignment is correct with 2 real simultaneous clients, and RPC
authority enforcement genuinely rejects a forging client.
All fixes verified with real two-process runs (including forcing an
actual goal and reading the server's own broadcast stream) and temporary
instrumentation, removed once each fix was confirmed. Full Phase 1 +
Phase 2 regression suite, including the net-sim-latency milestone gate,
re-run clean after every fix.
---
Game/scripts/net_sim.gd | 20 ++++-
Game/scripts/networked_match.gd | 154 +++++++++++++++++++++++++++++---
multiplayer-todo.md | 7 +-
3 files changed, 169 insertions(+), 12 deletions(-)
diff --git a/Game/scripts/net_sim.gd b/Game/scripts/net_sim.gd
index fc2a2a28..ca3c53c8 100644
--- a/Game/scripts/net_sim.gd
+++ b/Game/scripts/net_sim.gd
@@ -87,7 +87,14 @@ func _schedule(dispatch: Callable, target_peer_id: int, delay_sec: float) -> voi
if delay_sec <= 0.0:
dispatch.call()
return
- get_tree().create_timer(delay_sec, false).timeout.connect(func() -> void: _fire(dispatch, target_peer_id))
+ # process_always = true: a simulated wire delay must keep counting down
+ # even if the local SceneTree pauses (match_mode.gd's goal-pause does
+ # this today; multiplayer-todo.md §8 already flags get_tree().paused
+ # stopping the client's own send/receive loop as a separate refactor
+ # item). Pausing this timer too would let a paused client's in-flight
+ # packets pile up and arrive in a burst on unpause instead of on their
+ # simulated schedule.
+ get_tree().create_timer(delay_sec, true).timeout.connect(func() -> void: _fire(dispatch, target_peer_id))
# Re-validates the target right before a DELAYED send actually fires.
@@ -107,10 +114,21 @@ func _schedule(dispatch: Callable, target_peer_id: int, delay_sec: float) -> voi
# time to change since the caller's own validation, and matching the
# pre-NetSim behaviour exactly there is what keeps NetSim a true no-op when
# no CLI flags are given.
+#
+# Known residual gap, judged not worth the complexity for debug-only
+# tooling: if this process shuts down AND reconnects (a fresh host()/join())
+# within one delayed send's hold time, multiplayer_peer is a real peer again
+# and get_peers() may coincidentally contain the same target_peer_id from
+# the new session, so a stale send from the old session could slip through.
+# Closing that fully would need a generation counter bumped on every
+# shutdown/host/join and stamped on each scheduled send — disproportionate
+# for a latency simulator that only ever runs in manual/CI testing.
func _fire(dispatch: Callable, target_peer_id: int) -> void:
var peer := multiplayer.multiplayer_peer
if peer == null or peer is OfflineMultiplayerPeer:
return
+ if peer is ENetMultiplayerPeer and peer.get_connection_status() != MultiplayerPeer.CONNECTION_CONNECTED:
+ return
if target_peer_id != -1 and target_peer_id not in multiplayer.get_peers():
return
dispatch.call()
diff --git a/Game/scripts/networked_match.gd b/Game/scripts/networked_match.gd
index ac3e213d..a744769a 100644
--- a/Game/scripts/networked_match.gd
+++ b/Game/scripts/networked_match.gd
@@ -15,11 +15,14 @@ extends GameMode
# rather than relying on GameMode's default (arena-required-synchronously)
# flow.
-signal timer_updated(minutes: int, seconds: int)
+# Only score_changed is actually emitted in Phase 2 — Phase 5 owns the match
+# lifecycle state machine (timer, kickoff countdown, overtime, results), so
+# those signals get declared there, alongside real emission. Declaring one
+# here without emitting it isn't harmless: HUDController gates the timer
+# widget's visibility purely on has_signal("timer_updated"), so a declared-
+# but-dead signal shows a permanently frozen timer rather than correctly
+# hiding it the way free_play.gd's total absence of the signal does.
signal score_changed(score: Dictionary)
-signal match_ended(winning_team: int, score: Dictionary)
-signal kickoff_countdown(count: int)
-signal overtime_started
const NetCodec = preload("res://scripts/net_codec.gd")
const NetBodyState = preload("res://scripts/net_body_state.gd")
@@ -35,6 +38,33 @@ const INTERP_DELAY_MIN_MS := 25.0
const INTERP_DELAY_MAX_MS := 200.0
const SNAPSHOT_INTERVAL_MS := 1000.0 / 60.0
+# NetInterpolator.to_tick() assumes Time.get_ticks_msec() == physics_frame *
+# TICK_MS on the SERVER, i.e. that physics frame 0 happened at process-start
+# wall time. It doesn't: real startup work (autoloads, asset loading) elapses
+# before the first physics step, and any dropped tick widens the gap further
+# — it only ever grows. An adversarial review found this was NOT a rounding
+# error: it measured a steady +45-50ms bias on a real run, meaning EVERY
+# to_tick(get_server_time_estimate_ms()) call landed 3+ ticks past the
+# newest buffered sample, so sample_at() took the extrapolation branch 100%
+# of the time — zero real interpolation ever happened, on LAN or under
+# simulated latency alike, silently defeating the entire interpolation
+# buffer this phase was built around.
+#
+# Fix: this bias is a property of the server's clock, not of any one body,
+# so track ONE shared estimate here (not per-interpolator) from every
+# snapshot's own server_tick versus this client's server-time estimate at
+# receipt. Take the MINIMUM over a rolling window — same rationale as
+# NetworkManager's own min-RTT filtering (network_manager.gd): the sample
+# with the least one-way transit delay best isolates the constant epoch
+# bias from per-packet network noise, and a rolling (not all-time) window
+# lets a real increase in the bias — the server dropping more ticks later
+# in the match — still get picked up rather than staying pinned to a
+# now-stale historical minimum.
+const TICK_BIAS_WINDOW_SEC := 5.0
+
+var _tick_bias_samples: Array[Dictionary] = [] # [{t_ms:int, bias_ms:float}], client only
+var _tick_bias_ms := 0.0 # best current estimate; 0.0 until the first snapshot
+
class SlotInfo:
var peer_id: int
@@ -51,6 +81,31 @@ var _ball_interpolator := NetInterpolator.new() # client only
var _local_input_sampler := PlayerShipController.new() # client only: reads local input each tick to forward; never added to a Ship, never in the tree — get_action() only touches the global Input singleton
var _input_seq := 0 # client only
var _reset_gen := 0 # server only: bumped on every kickoff/goal reset so the client hard-snaps instead of interpolating across the teleport
+# Server only. _on_goal_scored's reset_ball()/reset_ships() only QUEUE
+# teleports (task 0.15's queue_teleport — applied on each body's next
+# _integrate_forces), but _broadcast_snapshot runs later in the SAME frame
+# _on_goal_scored fires in, before that teleport lands. Bumping _reset_gen
+# immediately would tag the still-pre-teleport snapshot with the new
+# generation: the client clears its buffer expecting a hard snap, then
+# keeps exactly that stale in-goal sample and lerps a full-arena slide to
+# the next, genuinely-post-teleport sample — an adversarial review measured
+# a 26.8m ball slide from this.
+#
+# A plain "bump on the next _physics_process" boolean flag turned out NOT
+# to fix it: the goal Area's body_entered signal (and so _on_goal_scored)
+# fires as part of physics tick N's OWN step processing, before tick N's
+# _physics_process callback — so a flag set there is already true by the
+# time that SAME tick's _physics_process checks it, consuming on tick N
+# instead of N+1 as intended (empirically confirmed: with a boolean flag,
+# gen still bumped on the same tick the stale position was broadcast).
+# The queued teleport, by contrast, isn't applied until tick N+1's
+# _integrate_forces. So the two must be compared by TICK NUMBER, not by
+# "next callback": only bump once the current tick is strictly later than
+# the tick the goal was detected on, which guarantees at least one full
+# _integrate_forces has run — and therefore the queued teleport has
+# landed — since the flag was set.
+var _pending_reset_gen_bump := false
+var _pending_reset_gen_bump_tick := -1
func _ready() -> void:
@@ -86,6 +141,20 @@ func _owns_world_simulation() -> bool:
return multiplayer.is_server()
+# _local_input_sampler is a plain Node (PlayerShipController extends
+# ShipController extends Node) that's deliberately never added to the tree
+# — dropping the last reference to it does not free it. An adversarial
+# review traced the "3 resources still in use at exit" warning on every
+# Phase 2 test run directly to this: --verbose named the leaked script
+# chain (player_ship_controller.gd, ship_controller.gd, ship_action.gd)
+# exactly, and adding this cleanup made the warning disappear. Runs
+# unconditionally (not just client-side) since the field is initialized
+# unconditionally too, despite its "client only" comment.
+func _exit_tree() -> void:
+ if is_instance_valid(_local_input_sampler):
+ _local_input_sampler.free()
+
+
# ============================================================
# Server
# ============================================================
@@ -141,17 +210,24 @@ func _on_goal_registered(conceding_team: int) -> void:
func _on_goal_scored(_conceding_team: int) -> void:
- _reset_gen = (_reset_gen + 1) % 256
reset_ball()
reset_ships()
+ _pending_reset_gen_bump = true
+ _pending_reset_gen_bump_tick = Engine.get_physics_frames()
func _broadcast_snapshot() -> void:
var server_tick := Engine.get_physics_frames()
var bodies: Array[NetBodyState] = []
+ # Always one entry per slot, even for a momentarily-invalid ship
+ # (placeholder zero state), so the ball always lands at the fixed index
+ # _slots.size() the client assumes in _on_snapshot_received — skipping
+ # invalid ships entirely would shift every later index. "No ship is ever
+ # despawned" (§6.4) means this is unreachable today, but it's a silent
+ # total-garbage failure mode the moment that stops being true, and the
+ # fix costs nothing.
for slot in _slots:
- if is_instance_valid(slot.ship):
- bodies.append(_ship_to_net_body_state(slot.ship))
+ bodies.append(_ship_to_net_body_state(slot.ship) if is_instance_valid(slot.ship) else NetBodyState.new())
if is_instance_valid(ball):
bodies.append(_ball_to_net_body_state(ball))
var segment := NetCodec.pack_snapshot_body_segment(server_tick, 0, _reset_gen, bodies)
@@ -273,11 +349,55 @@ func _on_snapshot_received(decoded: Dictionary) -> void:
var server_tick: int = decoded["server_tick"]
var reset_gen: int = decoded["reset_gen"]
var bodies: Array = decoded["bodies"]
+ _update_tick_bias(server_tick)
for i in _slots.size():
if i < bodies.size():
_slots[i].interpolator.add_sample(server_tick, bodies[i], reset_gen)
if bodies.size() > _slots.size():
- _ball_interpolator.add_sample(server_tick, bodies[_slots.size()], reset_gen)
+ var ball_state: NetBodyState = bodies[_slots.size()]
+ # unpack_snapshot() decodes every body's angular_velocity assuming
+ # SHIP_AVEL_RANGE; the ball was quantised at BALL_AVEL_RANGE
+ # (_ball_to_net_body_state), so it decodes 8x too small without this
+ # — dormant today (nothing reads decoded angular_velocity yet) but
+ # silently wrong the moment ball-spin VFX or Phase 4 prediction does.
+ NetCodec.rescale_avel(ball_state, NetCodec.BALL_AVEL_RANGE)
+ _ball_interpolator.add_sample(server_tick, ball_state, reset_gen)
+
+
+# See the class-level comment above _tick_bias_samples for why this exists.
+# bias_ms is how much further ahead to_tick(server_time_est) lands than the
+# server_tick this snapshot actually carries — mostly the server's own
+# physics-frame/wall-clock startup skew, plus a little real one-way transit
+# noise that the rolling minimum below filters back out.
+func _update_tick_bias(server_tick: int) -> void:
+ # get_server_time_estimate_ms() is meaningless before the first pong
+ # lands (clock_offset_ms == 0.0 until then, per network_manager.gd's own
+ # doc comment) — recording a bias sample from it during that window
+ # produced a garbage value (~-1.1s, the client's own raw pre-sync
+ # uptime standing in for a server-synced estimate) that the rolling-min
+ # window then locked onto for the rest of a short test, since 5 real
+ # seconds never fully elapsed before the test ended. Skip entirely
+ # until the clock is actually synced.
+ if NetworkManager.rtt_ms < 0.0:
+ return
+ var server_time_est := NetworkManager.get_server_time_estimate_ms()
+ var bias_ms := server_time_est - float(server_tick) * NetInterpolator.TICK_MS
+ var now_ms := Time.get_ticks_msec()
+ _tick_bias_samples.append({"t_ms": now_ms, "bias_ms": bias_ms})
+ var cutoff := now_ms - int(TICK_BIAS_WINDOW_SEC * 1000.0)
+ _tick_bias_samples = _tick_bias_samples.filter(func(s: Dictionary) -> bool: return s["t_ms"] >= cutoff)
+ var best: float = _tick_bias_samples[0]["bias_ms"]
+ for sample: Dictionary in _tick_bias_samples:
+ var sample_bias: float = sample["bias_ms"]
+ if sample_bias < best:
+ best = sample_bias
+ _tick_bias_ms = best
+
+
+# Bias-corrected replacement for NetInterpolator.to_tick(server_time_est) —
+# use this instead of calling to_tick() directly on a server-time estimate.
+func _estimated_tick(server_time_ms: float) -> float:
+ return NetInterpolator.to_tick(server_time_ms - _tick_bias_ms)
func _current_interp_delay_ms() -> float:
@@ -298,12 +418,24 @@ func _physics_process(_delta: float) -> void:
if _owns_world_simulation():
_respawn_escaped_bodies()
if multiplayer.is_server():
+ if _pending_reset_gen_bump and Engine.get_physics_frames() > _pending_reset_gen_bump_tick:
+ _reset_gen = (_reset_gen + 1) % 256
+ _pending_reset_gen_bump = false
_broadcast_snapshot()
return
_send_local_input()
+ # get_server_time_estimate_ms() is meaningless before the first pong
+ # lands (network_manager.gd's own doc comment says so explicitly) — an
+ # adversarial review found this was used unguarded here, which against
+ # a long-running dedicated server (clock_offset_ms == 0.0, so this
+ # process's own short uptime is compared against the server's enormous
+ # tick count) freezes every remote body at the oldest buffered pose for
+ # the whole first second of every match.
+ if NetworkManager.rtt_ms < 0.0:
+ return
var server_time_est := NetworkManager.get_server_time_estimate_ms()
- var collider_tick := NetInterpolator.to_tick(server_time_est)
+ var collider_tick := _estimated_tick(server_time_est)
for slot in _slots:
if is_instance_valid(slot.ship) and slot.interpolator.has_samples():
_apply_collider_state(slot.ship, slot.interpolator.sample_at(collider_tick))
@@ -330,8 +462,10 @@ func _process(_delta: float) -> void:
NetworkManager.poll()
if multiplayer.is_server() or _slots.is_empty():
return
+ if NetworkManager.rtt_ms < 0.0:
+ return
var server_time_est := NetworkManager.get_server_time_estimate_ms()
- var visual_tick := NetInterpolator.to_tick(server_time_est - _current_interp_delay_ms())
+ var visual_tick := _estimated_tick(server_time_est - _current_interp_delay_ms())
for slot in _slots:
if is_instance_valid(slot.ship) and slot.interpolator.has_samples():
_apply_ship_visual_state(slot.ship, slot.interpolator.sample_at(visual_tick))
diff --git a/multiplayer-todo.md b/multiplayer-todo.md
index 35d8f9b9..bec235bf 100644
--- a/multiplayer-todo.md
+++ b/multiplayer-todo.md
@@ -849,6 +849,8 @@ No own-ship prediction yet: the client renders everything, including its own shi
| 2.7 `[D:2.3]` `[P]` | **DONE**, also a natural consequence of the above — `spawn_camera_rig(_my_slot.ship)` and `_spawn_hud()` are called once the client's own ship is identified in `_on_match_config_received` | Smoke test asserts `_camera_rig` and `hud` both `is_instance_valid()` on the client; confirmed true in every clean run |
| 2.8 `[D:1.1]` `[P]` | **DONE.** New `NetSim` autoload (`scripts/net_sim.gd`): seeded (`--net-sim-seed=`, fixed default so a bad run reproduces), CLI-driven (`--net-sim-latency=`/`--net-sim-jitter=`/`--net-sim-loss=`/`--net-sim-dup=`), a pure passthrough (`send()` calls the dispatch immediately) unless at least one flag is non-zero — confirmed byte-for-byte inert against every Phase 1/2 regression test with no flags set. Wraps `MatchSim.send_input`/`send_snapshot` per this row's original scope, **plus `NetworkManager`'s `_ping`/`_pong` dispatch** — a deliberate scope addition, since that's the only RTT measurement that already exists and is already tested (task 1.8), so it's what makes this task's own acceptance criterion checkable today without waiting on Phase 3's per-peer snapshot echo. "Asymmetric-capable" needs no special-case code: each process reads only its own CLI args and delays only its own outgoing sends, so hosting and joining with different flags is already asymmetric. **One correctness subtlety, caught before it shipped**: callers that embed a timestamp in a wrapped RPC (`_ping`/`_pong`) must capture `Time.get_ticks_msec()` *before* calling `NetSim.send()`, not inside the wrapped `Callable` — capturing it inside would silently absorb that process's own added delay out of the round-trip measurement instead of adding to it, since the timestamp would then reflect "after my delay" rather than "when I actually tried to send". **A second real bug, found by actually running Phase 2's own milestone gate** (a full `networked_match_smoke` run under `--net-sim-latency=80 --net-sim-jitter=20`, not just the isolated ping/pong test above): a delayed send can outlive the window its target was valid in — the host hit "Attempt to call RPC with unknown peer ID" (the client had disconnected during the ~80-100ms hold, after `_broadcast_snapshot`'s existing `get_peers()` filter had already passed at *schedule* time) and the client hit "'_recv_input' on yourself is not allowed by selected mode" (its own `shutdown()` had already reset `multiplayer_peer` to a fresh `OfflineMultiplayerPeer` before a still-pending delayed send fired, so peer id 1 now meant itself). Fixed by having `send()` accept an optional `target_peer_id` and re-validating it — plus that this process still has a real (non-Offline) peer at all — at *fire* time inside a new `_fire()`, not just at schedule time; the synchronous/inactive path is deliberately left unvalidated so NetSim stays a true no-op when idle | Verified with a real two-process test (`tests/net_sim_smoke.gd`/`.tscn`): baseline (no flags) observed `rtt_ms=7.00` on loopback; `--net-sim-latency=80` on the host alone raised the client's observed `rtt_ms` to `83.00` (want ≥70, confirmed measurably higher than baseline); `--net-sim-loss=1.0` on the host produced **zero** pong samples over 7s (`rtt_ms` stayed `-1`, confirmed the drop path actually drops rather than relabels). **Phase 2's own milestone gate re-run and passing**: `networked_match_smoke` under `--net-sim-latency=80 --net-sim-jitter=20` on both peers — client still observed 26.08m of clean server-authoritative movement via interpolation, `thrust_z=1.00` confirmed mid-drive, zero RPC errors (the fire-time-revalidation fix above). Re-ran the full Phase 1 + Phase 2 regression suite (`test_runner`, `net_smoke`, `match_net_smoke`, `clock_smoke`, `lobby_smoke`, `server_boot`, `networked_match_smoke`) with NetSim present but inactive — all still pass with unchanged behaviour (clock offset converged to the same value, `networked_match_smoke` still showed clean server-authoritative movement) |
+| — | **An Opus subagent's adversarial review of all of Phase 2 found real, verified bugs the smoke tests couldn't catch, since constant-velocity dead reckoning still moves a ship far enough to pass a `moved > 1.0` check.** Fixed, all independently re-verified with real two-process runs and temporary instrumentation (removed after confirming):
**(1) The interpolator never actually interpolated — every `sample_at()` call took the extrapolation branch, 100% of the time, LAN or under simulated latency alike.** `NetInterpolator.to_tick()` assumes `Time.get_ticks_msec() == physics_frame * TICK_MS` on the server; real engine/autoload startup work before the first physics step (plus any dropped tick, which only ever widens it) breaks that by a steady +45-55ms in practice. `networked_match.gd` now tracks one shared `_tick_bias_ms` estimate (`_update_tick_bias`, called from `_on_snapshot_received`) — the **minimum** `to_tick(server_time_est) - server_tick` over a rolling 5s window, same rationale as `NetworkManager`'s own min-RTT filtering: the least-delayed sample best isolates the constant bias from per-packet transit noise, and a rolling (not all-time) window still tracks a real future increase. `_estimated_tick()` subtracts it before every `to_tick()` call. Verified: bias converged to ~50-56ms (matching the bug's own measured magnitude exactly) and real interpolation rose from 0% to ~70% of calls (`interp=436 extrap=182` out of 618, up from `interp=0 extrap=617`). **A first attempt at this fix was itself broken and made the lead ~30x worse (90+ ticks, ~1.5s)**: early snapshots arrive before `NetworkManager`'s first clock pong lands (`rtt_ms < 0`, `clock_offset_ms` still `0.0`), so `server_time_est` briefly means "my own raw local uptime" — a wildly wrong bias sample that the 5s rolling-min then locked onto for a whole short test, since 5 real seconds never fully elapsed before the test ended. Fixed by skipping bias recording entirely while `rtt_ms < 0`.
**(2) Goals caused a ~27m visual slide.** `_on_goal_scored` bumped `_reset_gen` immediately, but `reset_ball()`/`reset_ships()` only *queue* teleports (task 0.15, applied on each body's next `_integrate_forces`) — so the broadcast that same tick carried the NEW gen with the OLD (still-in-goal) position, and the client's buffer-clear-on-reset kept exactly that stale sample and lerped a full-arena slide to the next, genuinely-reset one. **The first fix attempt (defer the bump to "the next `_physics_process`" via a plain boolean) didn't work either** — emperically, the goal Area's `body_entered` signal fires as part of physics tick N's own step, *before* tick N's `_physics_process` callback, so a flag set in the handler is already true by the time that same tick checks it: no delay was actually introduced. Fixed by recording the tick the goal was detected on (`_pending_reset_gen_bump_tick`) and only bumping once `Engine.get_physics_frames() > _pending_reset_gen_bump_tick` — i.e. strictly on a later tick, which guarantees the queued teleport's `_integrate_forces` has already run. Verified by forcibly teleporting the ball into a goal mid-test and logging the server's own broadcast stream tick-by-tick: gen change and the already-reset position now land in the identical broadcast, every time.
**(3) `_local_input_sampler` (a `PlayerShipController`, i.e. a plain `Node`) was created but never added to the tree and never freed** — this was the unexplained "3 resources still in use at exit" warning on every prior Phase 2 test run, confirmed by `--verbose` naming the exact leaked script chain and by the warning disappearing once a `_exit_tree()` cleanup was added. Also leaked on the **server** despite its "client only" comment, since the field initializer is unconditional.
**(4) Ball angular velocity decoded 8x too small** — `NetCodec.rescale_avel()` exists specifically to correct a ball's decoded `angular_velocity` from the ship-range assumption `unpack_snapshot()` decodes every body with, and was never called. Dormant today (nothing read decoded `angular_velocity` yet) but silently wrong the moment ball-spin VFX or Phase 4 prediction reads it; now called in `_on_snapshot_received`.
**(5) `get_server_time_estimate_ms()` was used unguarded before the clock had synced**, contradicting its own doc comment — against a long-running dedicated server this freezes every remote body at the oldest buffered pose for the whole first second of every match (`clock_offset_ms == 0.0` compares this process's own short uptime against the server's much larger tick count). Both `_physics_process` and `_process` now skip their collider/visual update entirely while `NetworkManager.rtt_ms < 0.0`.
**Smaller fixes, all confirmed via the regression suite**: `net_sim.gd`'s delayed-send timer now uses `process_always = true` (a simulated wire shouldn't stop just because the local game pauses) and `_fire()` also checks `get_connection_status() == CONNECTION_CONNECTED`, not just non-`Offline`, before dispatching (a known, accepted residual gap remains: a shutdown-then-reconnect inside one delayed send's hold window isn't fully closed, judged disproportionate to fix for debug-only tooling); `_broadcast_snapshot()` now appends one body per slot unconditionally (a zeroed placeholder for a momentarily-invalid ship) so the ball's fixed index assumption can't silently break if "no ship is ever despawned" (§6.4) ever stops holding; `networked_match.gd` now only declares `score_changed` (the one signal it actually emits) instead of also declaring `timer_updated`/`match_ended`/`kickoff_countdown`/`overtime_started`, which — despite never being emitted — made `HUDController` show a permanently frozen timer widget purely because `has_signal("timer_updated")` was true.
**Confirmed fine, not just assumed**, via a real hostile-client stress test and a real 3-process multi-client run: a malformed/garbage/oversized `_recv_input` payload cannot crash the server (Godot's `StreamPeerBuffer` silently zero-fills past EOF; `count` is a bounded `u8`); `NetworkedMatch` skipping `GameMode._ready()`'s `super()` call drops nothing load-bearing; deterministic team/spawn-index slot assignment is correct with 2 simultaneous clients (verified with a real 3-process host+2-client run); RPC authority enforcement on `_match_config`/`_score_update`/`_snapshot` genuinely rejects a forging client server-side | Full Phase 1 + Phase 2 regression suite (`test_runner`, `net_smoke`, `match_net_smoke`, `clock_smoke`, `lobby_smoke`, `networked_match_smoke` baseline and under the `--net-sim-latency 80 --net-sim-jitter 20` milestone gate, `net_sim_smoke`) re-run clean after every fix |
+
> **`net_sim.gd` belongs in this phase, not Phase 3.** A LAN-only phase gate passes even with §4.1's flaw fully present, because LAN `INTERP_DELAY` sits at the clamp floor and closing-speed error is small. Phases 2 and 3 would both go green and Phase 4 would discover the architecture is wrong.
**Phase gate — MILESTONE:** a real 1v1 **at `--net-sim-latency 80 --net-sim-jitter 20`**, not just on LAN. Ships fly, the ball moves, goals detect server-side.
@@ -1009,7 +1011,10 @@ No own-ship prediction yet: the client renders everything, including its own shi
31. **An `@rpc` method named `_input` collides with `Node`'s built-in `_input(event: InputEvent)` virtual.** Found building task 2.1's `MatchSim` autoload: naming the client→server input RPC `_input(bytes: PackedByteArray)` produced a parse error ("function signature doesn't match the parent") — and because this was on an autoload, the error broke the **entire autoload from loading**, cascading into unrelated failures across every scene that touched `MatchSim` at all, none of which mentioned RPCs or `_input` in their own error output. Renamed to `_recv_input`. General lesson: on an autoload especially, treat any bare virtual-sounding method name (`_input`, `_process`, `_ready`, `_unhandled_input`, …) as reserved regardless of what you intend it to do — a signature mismatch there doesn't fail locally, it fails the whole autoload.
32. **Disabling automatic multiplayer polling (task 1.3) is global, not autoload-scoped — every scene that touches an RPC, not just `NetworkManager`-adjacent code, must call `NetworkManager.poll()` itself every frame it wants traffic to move.** Building task 2.1–2.3, `networked_match.gd`'s `_physics_process`/`_process` sent and listened for RPCs (`MatchSim.request_match_config`, `send_input`, snapshot RPCs) but never called `poll()` — nothing sent via `rpc()` in this scene ever reached the wire in either direction, silently, with no error in either process's log. Confirmed via debug prints: the client's request fired, but the host's handler print never appeared. The first (wrong) hypothesis was a startup race between the server's broadcast and the client's listener connecting — that fix (a request/response retry pattern, still worth keeping for the genuine late-join case) didn't resolve it alone. The real fix was adding `NetworkManager.poll()` at the top of both `_physics_process` and `_process` in the new scene. If a scene sends or receives RPCs and nothing arrives with no errors at all, check for a missing `poll()` before anything else.
33. **A request/response fallback for a one-shot broadcast can double-deliver, and the receiving handler must be idempotent.** Once gotcha 32's fix made polling actually work, `_on_match_config_received` ran **twice** per client — once from the server's original one-shot `_match_config.rpc()` broadcast (queued the whole time, since it had been sent before polling was fixed) and again from the request/response retry — producing two arenas, two ship sets, two HUDs (`_slots.size() == 2` instead of 1). Any handler reachable via both an original broadcast and a "resend on request" path needs its own guard (here: `if not _slots.is_empty(): return` at the top) rather than assuming "only sent once" from the RPC design alone.
-34. **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.
+34. **An `Area3D`'s `body_entered` signal fires as part of physics tick N's own step, strictly *before* tick N's `_physics_process` callback — not "on the next frame."** Found while fixing the goal-reset-ordering bug above: a boolean "handle this on the next `_physics_process`" flag set from inside a `body_entered` handler is a no-op, because that same tick's `_physics_process` hasn't run yet and sees the flag already true — it "defers" to the same tick it was set on, not the next one. If you actually need next-tick-or-later semantics, compare `Engine.get_physics_frames()` against the tick the flag was set on and require strictly-greater, not just "check a boolean at the top of `_physics_process`."
+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.
---