From b5e9dff33c44e93ef50d9ff390fb8872cdc6f7be Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Fri, 21 Aug 2026 11:34:48 +0100 Subject: [PATCH] fix(multiplayer): Phase 5 adversarial review fixes - reconnect, spectators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An adversarial review found five real defects in the Phase 5 lifecycle work. Two were critical and both were verified against controls. CRITICAL - a reconnecting client silently became a spectator. _try_reclaim_slot() swapped slot.peer_id, but MatchSim caches the last match_config and replays THAT to whoever asks. A reconnecting client in a fresh process requested config, received the pre-disconnect peer-id array, could not find itself, left _my_slot null and fell through to the spectator path - no ship, no input, for the rest of the match. The evidence was already in my own disconnect-test logs ("no slot for this peer - spectating", my_slot_ok=false) and I dismissed it: the host-side check only asserted the SERVER reclaimed the slot, never that the returning client owned it. Config is now rebroadcast on reclaim. Verified: my_slot_ok=false -> true. CRITICAL - spectators received no snapshots at all. §6.3 says a spectator "receives identical snapshots (the snapshot is already a broadcast - zero extra server work)". That was only ever true of the body SEGMENT: _broadcast_snapshot unicasts one packet per SLOT, so a peer without a slot got nothing - no poses, no reset_gen, no match_state byte. Spectating was entirely non-functional. The segment is still shared, so this is one extra send per spectator. Verified against a control: 0 snapshots and state stuck at LOADING before, 361 snapshots and PLAYING after. HIGH - cycling the spectator camera to the ball was a type error. ShipCameraRig.target is declared `var target: Ship` and the rig reaches into ship-only API, so it would have fired the moment anyone cycled past the last ship. Cycling is ships-only; the rig already has its own ball-cam mode for watching the ball. MEDIUM - clients never received match_ended or overtime_started. Both emitted only inside server-side logic, so a client froze and returned to the lobby without a result and its timer never switched to overtime. Derived from replicated state instead of adding two more RPCs: the client already has the authoritative score, and the transition is the event. MEDIUM - the goal cinematic ignored its authoritative window. goal_tick and resume_tick arrived and were unused; the client started a fresh fixed-length timer on RPC receipt, so a reliable retransmit could run the celebration past the server's window and into the next kickoff. _goal_pause_seconds() now returns the time actually remaining, clamped so an elapsed window cannot produce a non-positive timer. Also added: a match_bootstrap RPC carrying state, score, clock and reset_gen to one peer. match_config alone carries arena and roster only, so a late joiner or reconnecting player had no score or clock until the next goal happened to fire. It is sent on join AND on every request_match_config retry - the join-time send has exactly the same race match_config already had (the server sends it before the peer has loaded the match scene and connected its listeners), which the control run exposed: state was reaching PLAYING via the snapshot byte, not the bootstrap. New test: --role=client-spectator asserts a slotless peer receives the snapshot stream, follows the lifecycle, agrees with the wire byte, and can cycle targets without ever handing the camera a non-Ship. Verified non-vacuous. The ball-contact steering now closes all the way to 1.2m instead of coasting from 3m, which was missing the ball outright in roughly 1 run in 4. Not fixed, and still open: the 30s slot reservation is keyed on the player's display name, so any peer can claim a departed player's ship by choosing their name. §6.2 step 1 reserves auth_ticket for Phase 7; this needs a real identity token, not a name. Regression: 87 unit tests; free-flight LAN; transition gate 0.00%; ball contact 4/4; goal cycle; full match to RESULTS/LOBBY; disconnect and reconnect; spectator; two-bot CI. --- Game/scripts/match_sim.gd | 32 ++++++ Game/scripts/networked_match.gd | 124 ++++++++++++++++++++++- Game/tests/networked_match_smoke.gd | 18 ++++ Game/tests/networked_match_test_hooks.gd | 56 +++++++++- 4 files changed, 223 insertions(+), 7 deletions(-) diff --git a/Game/scripts/match_sim.gd b/Game/scripts/match_sim.gd index edfe512b..e4be7fab 100644 --- a/Game/scripts/match_sim.gd +++ b/Game/scripts/match_sim.gd @@ -30,6 +30,7 @@ signal state_change_received(state: int, at_tick: int) # §6.1 MatchState.Stat signal kickoff_received(positions: PackedVector3Array, rotations: PackedFloat32Array, countdown_start_tick: int, reset_gen: int) signal goal_scored_received(scoring_team: int, score: Dictionary, goal_tick: int, resume_tick: int) signal clock_state_received(running: bool, end_tick: int, at_tick: int) +signal match_bootstrap_received(state: int, at_tick: int, score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int) # Input validation (multiplayer-todo.md §3.1 steps 2-3, task 3.4). Deliberately # lives here rather than in NetworkedMatch: framing/rate abuse is a protocol- @@ -151,6 +152,15 @@ func send_match_config(arena_path: String, peer_ids: PackedInt32Array, teams: Pa _match_config.rpc(arena_path, peer_ids, teams, spawn_indices) +# Also the client's cue to ask for live match state — see +# NetworkedMatch._on_match_config_requested. A late joiner's bootstrap has the +# SAME race match_config has: the server sends it when the peer joins the +# roster, which is before that peer has loaded the match scene and connected +# its listeners, so a one-shot send is simply missed. Delivery has to be +# "ask until you get it" for both. +signal match_config_requested(peer_id: int) + + func request_match_config() -> void: _request_match_config.rpc_id(1) @@ -201,6 +211,19 @@ func send_clock_state(running: bool, end_tick: int, at_tick: int) -> void: _clock_state.rpc(running, end_tick, at_tick) +# §6.2 step 2 / §6.3: everything a peer needs to reconstruct the CURRENT match +# on arrival, sent to one peer rather than broadcast. +# +# match_config alone is not enough and never was: it carries arena and roster +# only, so a late joiner or a reconnecting player had no score, no clock, and +# no match state until the next goal or transition happened to fire. An +# adversarial review caught that; §6.2 step 2's `welcome` is specified to carry +# exactly this set, so this is that message under a name that does not clash +# with MatchNet's own lobby-level welcome. +func send_match_bootstrap(peer_id: int, state: int, at_tick: int, score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int) -> void: + _match_bootstrap.rpc_id(peer_id, state, at_tick, score, end_tick, clock_running, reset_gen) + + @rpc("authority", "call_remote", "reliable", 0) func _match_config(arena_path: String, peer_ids: PackedInt32Array, teams: PackedInt32Array, spawn_indices: PackedInt32Array) -> void: match_config_received.emit(arena_path, peer_ids, teams, spawn_indices) @@ -215,6 +238,7 @@ func _request_match_config() -> void: peer_id, _last_match_config["arena_path"], _last_match_config["peer_ids"], _last_match_config["teams"], _last_match_config["spawn_indices"] ) + match_config_requested.emit(peer_id) @rpc("any_peer", "call_remote", "unreliable_ordered", 1) @@ -326,6 +350,14 @@ func _clock_state(running: bool, end_tick: int, at_tick: int) -> void: clock_state_received.emit(running, end_tick, at_tick) +@rpc("authority", "call_remote", "reliable", 0) +func _match_bootstrap(state: int, at_tick: int, score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int) -> void: + if not MatchState.is_valid(state): + push_warning("MatchSim: ignoring bootstrap with unknown match_state %d" % state) + return + match_bootstrap_received.emit(state, at_tick, score, end_tick, clock_running, reset_gen) + + @rpc("authority", "call_remote", "reliable", 0) func _score_update(score: Dictionary) -> void: score_update_received.emit(score) diff --git a/Game/scripts/networked_match.gd b/Game/scripts/networked_match.gd index 010abf97..4d8b244d 100644 --- a/Game/scripts/networked_match.gd +++ b/Game/scripts/networked_match.gd @@ -291,6 +291,12 @@ var _pending_freeze_tick := -1 var _fill_bots := false # Task 5.10, server only. null unless --replay-log= was passed. var _replay_log: ReplayLog = null +# Server only: kept so match_config can be rebuilt after a slot's peer_id +# changes on reconnect (see _rebroadcast_match_config). +var _arena_path := "" +# Client only: the authoritative resume tick while a goal cinematic is playing, +# read by _goal_pause_seconds(). -1 when no goal window is open. +var _client_goal_resume_tick := -1 # §6.3 (task 5.8), client only. var _is_spectator := false var _spectator_target_index := 0 @@ -353,6 +359,7 @@ func _ready() -> void: MatchSim.kickoff_received.connect(_on_kickoff_received) MatchSim.goal_scored_received.connect(_on_goal_scored_received) MatchSim.clock_state_received.connect(_on_clock_state_received) + MatchSim.match_bootstrap_received.connect(_on_match_bootstrap_received) _request_match_config_until_received() @@ -385,6 +392,7 @@ func _exit_tree() -> void: func _start_server() -> void: var arena_path := ArenaRegistry.random_path() + _arena_path = arena_path arena = (load(arena_path) as PackedScene).instantiate() add_child(arena) for goal in arena.get_goals(): @@ -417,6 +425,9 @@ func _start_server() -> void: MatchSim.send_match_config(arena_path, peer_ids, teams, spawn_indices) MatchSim.input_received.connect(_on_input_received) NetworkManager.client_disconnected.connect(_on_client_disconnected) + # Piggyback live state on the existing retry loop, so a peer that missed + # the join-time bootstrap gets one every time it re-asks for config. + MatchSim.match_config_requested.connect(_send_match_bootstrap) MatchNet.player_joined.connect(_on_player_joined_midmatch) # §6.1: the arena, ball and every slot's ship now exist and match_config is @@ -571,6 +582,19 @@ func _apply_match_state(new_state: int, at_tick: int) -> void: # The clock only advances during live play (§6.2 step 9). Derived here # rather than tracked separately so it cannot disagree with the state. _clock_running = MatchState.is_live(new_state) and not _match_over + if not multiplayer.is_server(): + # HUDController duck-types on these two, and both previously emitted + # ONLY inside server-side logic — so a client froze and returned to the + # lobby without ever showing a result, and its timer never switched to + # overtime. Derive them from replicated state instead of adding two + # more RPCs: the client already has the authoritative score, and the + # state transition itself is the event. + if new_state == MatchState.State.OVERTIME_WARMUP: + _in_overtime = true + overtime_started.emit() + elif new_state == MatchState.State.RESULTS: + _match_over = true + match_ended.emit(_winning_team(), score.duplicate()) if new_state == MatchState.State.LOBBY and not multiplayer.is_server(): # §6.2 step 10: both sides return to the LOBBY, not the main menu. # Deferred because this runs from an RPC handler mid-tree-traversal @@ -795,7 +819,27 @@ func _on_goal_scored_received(scoring_team: int, new_score: Dictionary, goal_tic # The cinematic is bounded by [goal_tick, resume_tick] (§6.2 step 8), and # is presentation only: it never gates when play resumes, which is what # kept the server resetting while clients were mid-celebration. + # + # resume_tick is used, not just received. A reliable-channel retransmit can + # deliver this hundreds of ms after goal_tick, and starting a fresh + # fixed-length timer on ARRIVAL would then run the celebration past the + # server's own window and overlap the next kickoff. _goal_pause_seconds() + # below reads this and returns the time actually remaining. + _client_goal_resume_tick = resume_tick _play_goal_celebration(scoring_team, 1 - scoring_team) + _client_goal_resume_tick = -1 + + +# Overrides GameMode's virtual. On a client during a goal, the pause is +# whatever is LEFT of the authoritative window, not a fresh full duration. +func _goal_pause_seconds() -> float: + if _client_goal_resume_tick < 0: + return super() + var remaining := float(_client_goal_resume_tick - _current_server_tick()) / float(SimConstants.TICK_HZ) + # Clamp: a window that already elapsed must not produce a negative timer + # (Godot's create_timer asserts on <= 0), and a wildly future tick from a + # corrupt packet must not hang the celebration open. + return clampf(remaining, 0.05, super()) # --- §6.2 step 9: clock (task 5.2) ----------------------------------------- @@ -819,6 +863,16 @@ func _broadcast_clock_state() -> void: MatchSim.send_clock_state(_clock_running, _end_tick, Engine.get_physics_frames()) +func _on_match_bootstrap_received(state: int, at_tick: int, new_score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int) -> void: + score = new_score.duplicate() + score_changed.emit(score.duplicate()) + _end_tick = end_tick + _clock_running = clock_running + _reset_gen = reset_gen + _last_local_reset_gen = reset_gen + _apply_match_state(state, at_tick) + + func _on_clock_state_received(running: bool, end_tick: int, _at_tick: int) -> void: _clock_running = running _end_tick = end_tick @@ -1019,6 +1073,16 @@ func _try_reclaim_slot(peer_id: int, player_name: String) -> bool: slot.jitter_buffer = InputJitterBuffer.new() slot.consecutive_seq_rejects = 0 _swap_slot_controller(slot, RLShipController.new()) + # CRITICAL, and the reason a reconnect silently became a spectator: the + # slot's peer_id just changed, but MatchSim caches the last + # match_config and replays THAT to anyone who asks. A reconnecting + # client in a fresh process requests config, receives the pre- + # disconnect peer-id array, cannot find itself in it, leaves + # _my_slot null and falls through to the spectator path — no ship, no + # input, for the rest of the match. Re-broadcast so the cache and the + # roster agree again. + _rebroadcast_match_config() + _send_match_bootstrap(peer_id) print("NetworkedMatch: peer %d reclaimed %s's reserved slot" % [peer_id, player_name]) return true return false @@ -1040,6 +1104,9 @@ func _on_player_joined_midmatch(peer_id: int, player_name: String) -> void: # server's own peer bookkeeping inconsistent (§9 gotcha on force=true). multiplayer.multiplayer_peer.disconnect_peer(peer_id) return + # §6.3: a spectator/late joiner reconstructs from this, since match_config + # carries arena and roster only — no score, clock or match state. + _send_match_bootstrap(peer_id) print("NetworkedMatch: peer %d (%s) joined mid-match; spectating until the next kickoff" % [peer_id, player_name]) @@ -1058,6 +1125,28 @@ func _spectator_count() -> int: return count +# Rebuilds match_config from the CURRENT slot list and re-sends it. Slot order +# (and therefore snapshot body order) is preserved because _slots itself is +# never reordered — only a slot's peer_id changes on reclaim. +func _rebroadcast_match_config() -> void: + var peer_ids := PackedInt32Array() + var teams := PackedInt32Array() + var spawn_indices := PackedInt32Array() + for slot in _slots: + peer_ids.append(slot.peer_id) + teams.append(slot.team) + spawn_indices.append(slot.spawn_index) + MatchSim.send_match_config(_arena_path, peer_ids, teams, spawn_indices) + + +# §6.2 step 2: give one peer the live state it cannot get from match_config. +func _send_match_bootstrap(peer_id: int) -> void: + MatchSim.send_match_bootstrap( + peer_id, match_state, match_state_since_tick, score.duplicate(), + _end_tick, _clock_running, _reset_gen + ) + + func _expire_slot_reservations() -> void: var now := Engine.get_physics_frames() for slot in _slots: @@ -1130,6 +1219,27 @@ func _broadcast_snapshot() -> void: if _replay_log != null: _replay_log.record_snapshot(server_tick, bytes) MatchSim.send_snapshot(slot.peer_id, bytes) + # §6.3: "a spectator receives identical snapshots (the snapshot is already + # a broadcast — zero extra server work)". That was only true of the SEGMENT: + # the loop above unicasts one packet per SLOT, so a peer without a slot + # received nothing at all — no poses, no reset_gen, no match_state byte. + # An adversarial review caught it; spectating was entirely non-functional. + # The body segment is shared, so this really is just one extra send per + # spectator. The per-slot header fields are meaningless without a slot: + # there is no acknowledged input sequence, and -1 is the codec's own + # "client not established" value for buffer depth (§3.3). + var spectator_bytes := PackedByteArray() + for peer_id in connected_peers: + var has_slot := false + for slot in _slots: + if slot.peer_id == peer_id: + has_slot = true + break + if has_slot: + continue + if spectator_bytes.is_empty(): + spectator_bytes = NetCodec.pack_snapshot(0, -1, 0, segment) + MatchSim.send_snapshot(peer_id, spectator_bytes) func _ship_to_net_body_state(ship: Ship, stalled: bool) -> NetBodyState: @@ -1297,7 +1407,7 @@ func _unhandled_input(event: InputEvent) -> void: func _spectator_target_count() -> int: - return _slots.size() + (1 if is_instance_valid(ball) else 0) + return _slots.size() func _point_spectator_camera() -> void: @@ -1305,11 +1415,15 @@ func _point_spectator_camera() -> void: if count == 0: return _spectator_target_index = posmod(_spectator_target_index, count) - var target: Node3D = null + # SHIPS ONLY. ShipCameraRig.target is declared `var target: Ship` + # (ship_camera.gd:37) and the rig reaches into ship-only API (`visual`, + # `is_turbo_active`, `get_speed_ratio`), so assigning the ball here was a + # type error waiting to fire the moment anyone cycled past the last ship. + # The rig already has its own ball-cam MODE for watching the ball, which is + # the supported way to do it — this cycles whose ship we follow. + var target: Ship = null if _spectator_target_index < _slots.size(): target = _slots[_spectator_target_index].ship - else: - target = ball if not is_instance_valid(target): return if not is_instance_valid(_camera_rig): @@ -1326,7 +1440,7 @@ func _point_spectator_camera() -> void: if is_instance_valid(_camera_rig): _camera_rig.target = target if is_instance_valid(hud): - hud.ship = target if target is Ship else null + hud.ship = target func cycle_spectator_target(step: int = 1) -> void: diff --git a/Game/tests/networked_match_smoke.gd b/Game/tests/networked_match_smoke.gd index 03c3b11b..ee20da17 100644 --- a/Game/tests/networked_match_smoke.gd +++ b/Game/tests/networked_match_smoke.gd @@ -67,6 +67,16 @@ func _ready() -> void: return print("SMOKE: joining ...") MatchNet.welcomed.connect(_on_client_welcomed) + "client-spectator": + # A name nobody reserved, so the server has no slot for it. + MatchNet.local_player_name = "Watcher" + var serr := NetworkManager.join("127.0.0.1", PORT) + if serr != OK: + print("SMOKE FAIL: join() failed: %s" % error_string(serr)) + get_tree().quit(1) + return + print("SMOKE: joining as a spectator ...") + MatchNet.welcomed.connect(_on_spectator_welcomed) "client-abuse-malformed", "client-abuse-flood", "client-abuse-flood-dutycycle": # task 3.4's disconnect-abusive-peer paths: joins normally (so # it's a real connected peer, exactly like a hostile custom @@ -126,6 +136,14 @@ func _on_disconnect_host_player_joined(_peer_id: int, _name: String) -> void: hooks.run_disconnect_host_check.call_deferred(_drive_seconds) +func _on_spectator_welcomed() -> void: + MatchNet.welcomed.disconnect(_on_spectator_welcomed) + get_tree().change_scene_to_file.call_deferred("res://scenes/networked_match.tscn") + var hooks := preload("res://tests/networked_match_test_hooks.gd").new() + get_tree().root.add_child.call_deferred(hooks) + hooks.run_spectator_check.call_deferred(_drive_seconds) + + func _on_abuser_welcomed() -> void: MatchNet.welcomed.disconnect(_on_abuser_welcomed) var hooks := preload("res://tests/networked_match_test_hooks.gd").new() diff --git a/Game/tests/networked_match_test_hooks.gd b/Game/tests/networked_match_test_hooks.gd index 3daf0055..ababb12e 100644 --- a/Game/tests/networked_match_test_hooks.gd +++ b/Game/tests/networked_match_test_hooks.gd @@ -505,8 +505,11 @@ func _drive_at_ball(ship: Ship, ball_body: Node3D, timeout_seconds: float) -> vo if not is_instance_valid(ship) or not is_instance_valid(ball_body): break var to_ball := ball_body.global_position - ship.global_position - if to_ball.length() < 3.0: - break # close enough that the existing thrust carries it in + if to_ball.length() < 1.2: + break # touching distance; momentum carries it the rest of the way + # Deliberately keeps steering all the way in rather than breaking off + # early and coasting: breaking at 3m let the ship sail past the ball + # without ever touching it (0 contacts in 1 run of 3). # Bearing in the ship's own frame: -Z is forward, +X is right. var local := ship.global_transform.basis.inverse() * to_ball var yaw_error := atan2(local.x, -local.z) @@ -632,6 +635,55 @@ func run_disconnect_host_check(lifetime_seconds: float) -> void: get_tree().quit(0 if success else 1) +# §6.3 (task 5.8). A peer that joins mid-match with a name nobody reserved is a +# spectator: no slot, no ship, but it MUST still receive the snapshot stream +# and follow the lifecycle. An adversarial review found spectators received no +# snapshots at all, because _broadcast_snapshot unicasts per SLOT. +func run_spectator_check(run_seconds: float) -> void: + var snapshot_count := [0] + MatchSim.snapshot_received.connect(func(_d: Dictionary) -> void: snapshot_count[0] += 1) + + var deadline := Time.get_ticks_msec() + 10000 + while Time.get_ticks_msec() < deadline and not _is_networked_match(get_tree().current_scene): + await get_tree().process_frame + var match_scene := get_tree().current_scene + if not _is_networked_match(match_scene): + print("SMOKE FAIL: spectator never loaded the match scene") + get_tree().quit(1) + return + + await get_tree().create_timer(run_seconds).timeout + if not _is_networked_match(match_scene): + print("SMOKE FAIL: match scene torn down during the spectator run") + get_tree().quit(1) + return + + var is_spectator: bool = match_scene._my_slot == null + var got_snapshots: bool = snapshot_count[0] > int(run_seconds * 20.0) + var camera_ok: bool = is_instance_valid(match_scene._camera_rig) + var state_ok: bool = MatchState.is_valid(match_scene.match_state) and match_scene.match_state != MatchState.State.LOBBY + # Bootstrap: a late joiner must know the live clock, not wait for a goal. + var stats: Dictionary = match_scene.get_net_debug_stats() + var wire_state := int(stats.get("snapshot_match_state", -1)) + var wire_ok: bool = wire_state == int(stats.get("match_state", -2)) + # Cycling must be safe and must never hand the camera a non-Ship. + match_scene.cycle_spectator_target(1) + match_scene.cycle_spectator_target(1) + match_scene.cycle_spectator_target(-1) + var cycle_ok: bool = is_instance_valid(match_scene._camera_rig) and (match_scene._camera_rig.target == null or match_scene._camera_rig.target is Ship) + + print("SMOKE INFO: spectator is_spectator=%s snapshots=%d camera_ok=%s state=%s wire_state=%s cycle_ok=%s" % [ + str(is_spectator), snapshot_count[0], str(camera_ok), MatchState.to_name(match_scene.match_state), + MatchState.to_name(wire_state), str(cycle_ok) + ]) + var success := is_spectator and got_snapshots and camera_ok and state_ok and wire_ok and cycle_ok + print("SMOKE %s: spectator received the snapshot stream and followed the match (snapshots=%d, want > %d)" % [ + "PASS" if success else "FAIL", snapshot_count[0], int(run_seconds * 20.0) + ]) + NetworkManager.shutdown() + get_tree().quit(0 if success else 1) + + func run_malformed_abuse_check() -> void: await get_tree().create_timer(1.0).timeout # A single-element Array, not a plain bool: GDScript lambdas capture