mirror of
https://github.com/jcreek/CosmicClash.git
synced 2026-09-11 22:23:44 +00:00
fix(multiplayer): Phase 5 adversarial review fixes - reconnect, spectators
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.
This commit is contained in:
@@ -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:
|
||||
|
||||
Reference in New Issue
Block a user