diff --git a/Game/scripts/match_sim.gd b/Game/scripts/match_sim.gd index 6f14786d..0396ed3c 100644 --- a/Game/scripts/match_sim.gd +++ b/Game/scripts/match_sim.gd @@ -264,6 +264,11 @@ func send_input(bytes: PackedByteArray) -> void: func send_snapshot(peer_id: int, bytes: PackedByteArray) -> void: + # A server-side disconnect can leave peer_id in get_peers() until the + # current poll batch settles. Do not enter Godot's RPC path for that stale + # target; NetSim repeats this check at fire time for delayed sends. + if not NetworkManager.can_send_to_peer(peer_id): + return _track_sent(bytes.size()) NetSim.send(func() -> void: _snapshot.rpc_id(peer_id, bytes), peer_id) @@ -477,6 +482,7 @@ func _disconnect_abusive_peer(peer_id: int, reason: String) -> void: # which does not carry the peer, the reason or a timestamp into the log # stream a container actually captures. ServerLog.warn("peer_kicked", {"peer_id": peer_id, "reason": reason}) + NetworkManager.invalidate_peer(peer_id) _peer_input_state.erase(peer_id) if multiplayer.multiplayer_peer is ENetMultiplayerPeer: multiplayer.multiplayer_peer.disconnect_peer(peer_id) diff --git a/Game/scripts/network_manager.gd b/Game/scripts/network_manager.gd index deb2475e..351ec129 100644 --- a/Game/scripts/network_manager.gd +++ b/Game/scripts/network_manager.gd @@ -71,6 +71,7 @@ var is_server := false var is_client := false var _peer: MultiplayerPeer # keep a strong ref alongside multiplayer.multiplayer_peer var active_transport := "" +var _invalidated_peer_ids: Dictionary = {} var rtt_ms := -1.0 # min-RTT sample currently in the window; -1 = no sample yet var clock_offset_ms := 0.0 # add to a local Time.get_ticks_msec() reading to estimate the server's clock @@ -127,6 +128,27 @@ func poll() -> void: multiplayer.poll() +# A peer can be removed from the transport while Godot is still draining the +# same poll batch. During that interval get_peers() may still contain it, but +# an RPC send already fails because ENet has torn down its channels. +func invalidate_peer(peer_id: int) -> void: + _invalidated_peer_ids[peer_id] = true + + +func can_send_to_peer(peer_id: int) -> bool: + if _invalidated_peer_ids.has(peer_id): + return false + if _peer == null or _peer is OfflineMultiplayerPeer: + return false + # A listening server's peer status is transport/version-specific; the + # authoritative server is valid as soon as it owns a peer and the target + # appears in get_peers(). Clients, however, must not dispatch while their + # connection is still handshaking. + if not is_server and _peer.get_connection_status() != MultiplayerPeer.CONNECTION_CONNECTED: + return false + return peer_id in multiplayer.get_peers() + + func available_transports() -> PackedStringArray: var transports := PackedStringArray([TRANSPORT_ENET]) if SteamTransportScript.new().is_available(): @@ -185,6 +207,7 @@ func shutdown() -> void: peer.close() multiplayer.multiplayer_peer = OfflineMultiplayerPeer.new() _peer = null + _invalidated_peer_ids.clear() active_transport = "" is_server = false is_client = false diff --git a/multiplayer-next.md b/multiplayer-next.md index 7423ef04..5d556874 100644 --- a/multiplayer-next.md +++ b/multiplayer-next.md @@ -127,7 +127,7 @@ product policy are in [`docs/MATCHMAKING.md`](docs/MATCHMAKING.md). - [ ] Replace display-name slot reclaim with verified Steam identity. - [ ] Investigate occasional transport input loss during a long server stall. -- [ ] Fix the remaining `_broadcast_snapshot` packet-send stderr race. +- [x] Fix the remaining `_broadcast_snapshot` packet-send stderr race. ## Decide after the latency playtest diff --git a/multiplayer-todo.md b/multiplayer-todo.md index 1142736b..fba34825 100644 --- a/multiplayer-todo.md +++ b/multiplayer-todo.md @@ -31,13 +31,13 @@ The one place to look before planning. Everything here is also written up where These two are independent and can be done in either order, but B is the cheaper of the two to arrange and would also exercise A's conditions incidentally. -### Known defects, not fixed +### Known defects | # | What | Severity | Detail | |---|---|---|---| | C | **Slot reservation and takeover are keyed on display name alone.** Any peer connecting with a departed player's name inside the 30 s window claims their slot, ship and team. | Real, demonstrated. Bounded by needing a genuine disconnect to race. | §11 | | D | **Input is still lost at the transport layer during a long server stall**, variably — 7 of 8 runs measured 0.00 % of the sequence stream missing, the eighth 23.54 %. | Low. Distinct from the rate-limiter cause, which is fixed. The seq-guard resync visibly recovers it. | Phase 5 notes | -| E | **A second `Unable to send packet on channel N` stderr race**, in `_broadcast_snapshot` rather than the fixed site in `_remove_player`. | Cosmetic, but it violates the clean-stderr convention the tests rely on. Only reproduced via the adversarial abuse role. | §11 | +| E | **A second `Unable to send packet on channel N` stderr race**, in `_broadcast_snapshot` rather than the fixed site in `_remove_player`. | **Fixed.** Server-side abuse disconnects invalidate the peer before closing it, and snapshot sends re-check that invalidation at the transport boundary. | §11 | C is the one to plan around: it is fixed for free by task **7.4** (Steam auth tickets in `hello`), which is why it has not been given a bespoke solution. Anything that ships to strangers before Phase 7 needs it addressed first.