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.
2026-09-11 08:23:45 +00:00 · 2026-08-20 12:43:33 +01:00
parent 7b150ef72e
commit 14698d4ccb
3 changed files with 169 additions and 12 deletions
@@ -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()
@@ -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))