From 7a1668c902b66fd3c7ead1a6863410c0a0c9bc11 Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Fri, 21 Aug 2026 12:17:06 +0100 Subject: [PATCH] fix(multiplayer): second adversarial review - Esc, stranded clients, clock MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second adversarial review (this one able to RUN things, unlike the first) reproduced five defects. Fixing the critical and high ones. CRITICAL - Esc no longer left a networked match, and a client whose server vanished was stranded forever. Two independent bugs composing: _unhandled_input (added for spectator target cycling) overrode GameMode._unhandled_input and returned early for every non-spectator without ever calling super(), silently killing ui_cancel -> main menu; and NetworkedMatch never connected NetworkManager.disconnected_from_ server the way lobby.gd does. Measured: a client whose host exited emitted 7,235 engine errors in ~18s and only left because a test timer fired. Now 1 benign teardown error, and it returns to the main menu. HIGH - the match clock lost up to 3 seconds of regulation per goal. _on_goal_registered extended end_tick by the celebration only (resume_tick - goal_tick) and never by the 180-tick kickoff countdown that follows it, while _update_clock derived remaining time from the current tick regardless of _clock_running - so regulation drained during every stoppage. Measured 660 PLAYING ticks for a 14s match against 840 expected: exactly one WARMUP lost. The HUD also opened at 0:17 for a 14s match because the initial arm folded WARMUP into end_tick. Replaced the per-goal arithmetic with bank-and-rebase: entering any non-live state banks the remaining ticks, leaving it rebases end_tick off the banked value. That covers celebration and countdown together and cannot drift, since nothing has to predict how long a stoppage will be. clock_state and match_bootstrap now carry remaining_ticks, which is authoritative whenever the clock is stopped. Verified with the reviewer's own metric: 840 PLAYING ticks for a 14s match, exactly. MEDIUM - clients never froze at FULL_TIME/RESULTS. The freeze handling sat inside `if multiplayer.is_server()`, so a local player flew around for the whole 8s results screen while every other peer saw their ship parked. Not fixed, and now demonstrated rather than merely suspected: - The 30s slot reservation is keyed on display NAME, so a stranger can take a departed player's ship and the real player is then locked out (reproduced). Worse than first thought: MatchNet.local_player_name defaults to "Player" and uniqueness is never enforced, so collisions are the common case, not an attack setup. Needs a real identity token; §6.2 step 1 reserves auth_ticket for Phase 7. - §6.3's "late joiner takes the slot at the next kickoff" is unimplemented - _is_spectator is assigned once and never revisited - while the server logs that it happened. - Replay log still ignores store_* return values, never records malformed/rejected inputs, and close() has no caller. - --role=host-disconnect grades the reconnecting client on ~1s of life before the host quits, and never asserts the client owns _my_slot. Regression: 87 unit tests; free-flight LAN; transition gate 0.00%; goal cycle; spectator; disconnect and reconnect; full match to RESULTS. --- Game/scripts/match_sim.gd | 23 ++++--- Game/scripts/networked_match.gd | 87 ++++++++++++++++++++---- Game/tests/networked_match_test_hooks.gd | 7 ++ 3 files changed, 93 insertions(+), 24 deletions(-) diff --git a/Game/scripts/match_sim.gd b/Game/scripts/match_sim.gd index e4be7fab..47fa7003 100644 --- a/Game/scripts/match_sim.gd +++ b/Game/scripts/match_sim.gd @@ -29,8 +29,8 @@ signal state_change_received(state: int, at_tick: int) # §6.1 MatchState.Stat # rotations is 4 floats per body (x, y, z, w). 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) +signal clock_state_received(running: bool, end_tick: int, remaining_ticks: int, at_tick: int) +signal match_bootstrap_received(state: int, at_tick: int, score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int, remaining_ticks: 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- @@ -207,8 +207,11 @@ func send_goal_scored(scoring_team: int, score: Dictionary, goal_tick: int, resu _goal_scored.rpc(scoring_team, score, goal_tick, resume_tick) -func send_clock_state(running: bool, end_tick: int, at_tick: int) -> void: - _clock_state.rpc(running, end_tick, at_tick) +# remaining_ticks is authoritative while `running` is false: a stopped clock +# cannot be derived from end_tick minus the current tick, or it drains through +# every goal pause and kickoff countdown. +func send_clock_state(running: bool, end_tick: int, remaining_ticks: int, at_tick: int) -> void: + _clock_state.rpc(running, end_tick, remaining_ticks, at_tick) # §6.2 step 2 / §6.3: everything a peer needs to reconstruct the CURRENT match @@ -220,8 +223,8 @@ func send_clock_state(running: bool, end_tick: int, at_tick: int) -> void: # 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) +func send_match_bootstrap(peer_id: int, state: int, at_tick: int, score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int, remaining_ticks: int) -> void: + _match_bootstrap.rpc_id(peer_id, state, at_tick, score, end_tick, clock_running, reset_gen, remaining_ticks) @rpc("authority", "call_remote", "reliable", 0) @@ -346,16 +349,16 @@ func _goal_scored(scoring_team: int, score: Dictionary, goal_tick: int, resume_t @rpc("authority", "call_remote", "reliable", 0) -func _clock_state(running: bool, end_tick: int, at_tick: int) -> void: - clock_state_received.emit(running, end_tick, at_tick) +func _clock_state(running: bool, end_tick: int, remaining_ticks: int, at_tick: int) -> void: + clock_state_received.emit(running, end_tick, remaining_ticks, 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: +func _match_bootstrap(state: int, at_tick: int, score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int, remaining_ticks: 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) + match_bootstrap_received.emit(state, at_tick, score, end_tick, clock_running, reset_gen, remaining_ticks) @rpc("authority", "call_remote", "reliable", 0) diff --git a/Game/scripts/networked_match.gd b/Game/scripts/networked_match.gd index 4d8b244d..7ef9ef74 100644 --- a/Game/scripts/networked_match.gd +++ b/Game/scripts/networked_match.gd @@ -264,6 +264,9 @@ const RESULTS_TICKS := 8 * SimConstants.TICK_HZ # how long RESULTS holds befor # -1 until the first kickoff arms it. var _end_tick := -1 var _clock_running := false +# Ticks of regulation left, banked whenever the clock stops. Authoritative +# while _clock_running is false; end_tick is rebased from it on resume. +var _clock_remaining_ticks := -1 var _last_emitted_second := -1 @export var match_length_seconds := 150.0 @@ -360,6 +363,12 @@ func _ready() -> void: 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) + # lobby.gd does this; the match scene never did. Without it a client + # whose host exits stays in a dead match forever, emitting thousands of + # "multiplayer instance isn't currently active" / "RPC via a peer which + # is not connected" errors per run — it only ever left because a test + # timer happened to fire. + NetworkManager.disconnected_from_server.connect(_on_disconnected_from_server) _request_match_config_until_received() @@ -439,7 +448,7 @@ func _start_server() -> void: _set_match_state(MatchState.State.WARMUP) # The clock covers regulation only and is armed once; the goal-pause # extension below (§6.2 step 9) adjusts end_tick rather than restarting it. - _arm_clock(int(match_length_seconds * SimConstants.TICK_HZ) + WARMUP_TICKS) + _arm_clock(int(match_length_seconds * SimConstants.TICK_HZ)) _begin_kickoff() @@ -581,7 +590,22 @@ func _apply_match_state(new_state: int, at_tick: int) -> void: _set_bodies_frozen(false) # The clock only advances during live play (§6.2 step 9). Derived here # rather than tracked separately so it cannot disagree with the state. + var was_running := _clock_running _clock_running = MatchState.is_live(new_state) and not _match_over + if _end_tick >= 0 and multiplayer.is_server(): + if was_running and not _clock_running: + # Stopping: bank whatever is left. This replaces the old + # per-goal `end_tick += resume_tick - goal_tick` arithmetic, which + # only ever compensated for the CELEBRATION and silently ate the + # kickoff countdown that follows it. + _clock_remaining_ticks = maxi(0, _end_tick - at_tick) + _broadcast_clock_state() + elif not was_running and _clock_running: + # Resuming: rebase the absolute end tick off the banked remainder, + # so every stoppage costs exactly zero regulation time regardless + # of how long it lasted. + _end_tick = at_tick + maxi(0, _clock_remaining_ticks) + _broadcast_clock_state() 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 @@ -589,6 +613,12 @@ func _apply_match_state(new_state: int, at_tick: int) -> void: # 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. + # Bodies stop on the server at FULL_TIME/RESULTS but the client only + # ever froze at kickoff and on a goal — so a player flew around for the + # whole 8s results screen while every other peer saw their ship parked. + if new_state in [MatchState.State.FULL_TIME, MatchState.State.RESULTS, MatchState.State.LOBBY]: + _pending_freeze_tick = -1 + _set_bodies_frozen(true) if new_state == MatchState.State.OVERTIME_WARMUP: _in_overtime = true overtime_started.emit() @@ -855,27 +885,42 @@ func _current_server_tick() -> int: func _arm_clock(length_ticks: int) -> void: + # Bank the full regulation length AND set an end tick. The clock is stopped + # during the opening kickoff, so the banked value is what is displayed + # until play starts, and the resume path rebases end_tick off it. WARMUP is + # deliberately NOT folded into end_tick any more: doing so made a 14s match + # open its HUD at 0:17. + _clock_remaining_ticks = length_ticks _end_tick = Engine.get_physics_frames() + length_ticks _broadcast_clock_state() func _broadcast_clock_state() -> void: - MatchSim.send_clock_state(_clock_running, _end_tick, Engine.get_physics_frames()) + MatchSim.send_clock_state(_clock_running, _end_tick, _clock_remaining_ticks, 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: +func _on_disconnected_from_server() -> void: + # Deferred: this arrives from inside NetworkManager's poll, and gotcha 27 + # requires change_scene_to_file never run synchronously from a callback + # mid-traversal. + get_tree().change_scene_to_file.call_deferred(ScenePaths.MAIN_MENU) + + +func _on_match_bootstrap_received(state: int, at_tick: int, new_score: Dictionary, end_tick: int, clock_running: bool, reset_gen: int, remaining_ticks: int) -> void: score = new_score.duplicate() score_changed.emit(score.duplicate()) _end_tick = end_tick _clock_running = clock_running + _clock_remaining_ticks = remaining_ticks _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: +func _on_clock_state_received(running: bool, end_tick: int, remaining_ticks: int, _at_tick: int) -> void: _clock_running = running _end_tick = end_tick + _clock_remaining_ticks = remaining_ticks # Both peers. Emits timer_updated only when the displayed second changes, the @@ -883,7 +928,13 @@ func _on_clock_state_received(running: bool, end_tick: int, _at_tick: int) -> vo func _update_clock() -> void: if _end_tick < 0: return - var remaining_ticks := maxi(0, _end_tick - _current_server_tick()) + # While the clock is STOPPED the remaining time is frozen, not derived from + # the current tick. Deriving it regardless meant regulation time drained + # during every goal pause and kickoff countdown: measured 180 ticks — one + # whole WARMUP — lost per goal, plus the pre-kickoff display opening at + # 0:17 for a 14s match. _clock_remaining_ticks is the authority whenever + # _clock_running is false. + var remaining_ticks := maxi(0, _end_tick - _current_server_tick()) if _clock_running else maxi(0, _clock_remaining_ticks) var remaining_seconds := int(ceil(float(remaining_ticks) / float(SimConstants.TICK_HZ))) if remaining_seconds != _last_emitted_second: _last_emitted_second = remaining_seconds @@ -973,11 +1024,10 @@ func _on_goal_registered(conceding_team: int) -> void: return var goal_tick := Engine.get_physics_frames() var resume_tick := goal_tick + int(_goal_pause_seconds() * SimConstants.TICK_HZ) - # The clock stops for the celebration and resumes after it — expressed as - # a shift of the absolute end tick (§5.2's own formula), never as pausing - # a Timer, so no float drift accumulates across ten goals. - if _end_tick >= 0 and not _in_overtime: - _end_tick += resume_tick - goal_tick + # No end_tick arithmetic here any more: entering GOAL_PAUSE banks the + # remaining ticks and leaving it rebases end_tick (see _apply_match_state), + # which covers the celebration AND the kickoff countdown after it. Still + # tick-derived, so no float drift accumulates across ten goals. MatchSim.send_goal_scored(scoring_team, score.duplicate(), goal_tick, resume_tick) _set_bodies_frozen(true) _set_match_state(MatchState.State.GOAL_PAUSE) @@ -1143,7 +1193,7 @@ func _rebroadcast_match_config() -> void: 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 + _end_tick, _clock_running, _reset_gen, _clock_remaining_ticks ) @@ -1399,11 +1449,15 @@ func _spawn_hud() -> void: # already a mode-level key and is meaningless to a spectator (it only fires in # Free Play), rather than adding a new binding to project.godot for one mode. func _unhandled_input(event: InputEvent) -> void: - if not _is_spectator: - return - if event.is_action_pressed("reset_ball"): + # super() is NOT optional here. GameMode._unhandled_input owns ui_cancel -> + # main menu, and this override returned early for every non-spectator + # without ever chaining, which silently killed Esc for every networked + # player. Handle the spectator key, then always fall through. + if _is_spectator and event.is_action_pressed("reset_ball"): cycle_spectator_target(1) get_viewport().set_input_as_handled() + return + super(event) func _spectator_target_count() -> int: @@ -2026,6 +2080,11 @@ func _process(_delta: float) -> void: NetworkManager.poll() if multiplayer.is_server() or _slots.is_empty(): return + # The scene change on disconnect is deferred, so this can run one more time + # against a torn-down peer — which throws from get_unique_id() rather than + # returning anything. + if multiplayer.multiplayer_peer == null or multiplayer.multiplayer_peer.get_connection_status() != MultiplayerPeer.CONNECTION_CONNECTED: + return if NetworkManager.rtt_ms < 0.0: return var server_time_est := NetworkManager.get_server_time_estimate_ms() diff --git a/Game/tests/networked_match_test_hooks.gd b/Game/tests/networked_match_test_hooks.gd index ababb12e..71b87c65 100644 --- a/Game/tests/networked_match_test_hooks.gd +++ b/Game/tests/networked_match_test_hooks.gd @@ -198,6 +198,13 @@ func run_client_check(settle_seconds: float, drive_seconds: float, exercise_ball # tasks 5.3/5.4 freeze the local ship for the kickoff countdown and the # goal pause, and a goal can land anywhere in a drive, so asserting # unconditionally reports a correctly-frozen ship as a prediction failure. + # The match scene can be freed underneath this — a lost server sends the + # client back to the main menu (§6.4), same class of teardown as RESULTS. + if not _is_networked_match(match_scene) or not is_instance_valid(my_slot.ship): + print("SMOKE INFO: match scene torn down mid-drive (server lost?)") + NetworkManager.shutdown() + get_tree().quit(1) + return var live_now: bool = MatchState.is_live(match_scene.match_state) var structure_ok: bool = my_slot.ship.controller != null \ and my_slot.ship.controller.get_parent() == my_slot.ship \