From cf73074e2740f3433820a2447d3be5eff2c8ef93 Mon Sep 17 00:00:00 2001 From: Josh Creek <8179928+jcreek@users.noreply.github.com> Date: Thu, 20 Aug 2026 18:26:12 +0100 Subject: [PATCH] fix(multiplayer): resolve composition regression from second adversarial review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A second adversarial review of the previous fix commit found two of its nine fixes silently defeated each other: the seq-range guard (fix for a MEDIUM epoch-mismatch finding) capped the exact variable the ring-overflow resync (fix for the original CRITICAL finding) depends on, making the resync unreachable in production and recreating permanent input death at a lower failure threshold, reachable via ordinary server tick loss alone. - CRITICAL: rebind the seq-range guard to InputJitterBuffer's own highest_ingested_seq (now public) instead of the consumer-side last_applied_seq, so it tracks the client's send epoch rather than a value that can lag arbitrarily far behind during a stall. - HIGH: InputLeadController's release logic still ANDed the old `lead > LEAD_MIN` gate onto the new depth-driven condition, so a backlog the controller never caused still couldn't drain. Split into two independent decisions: the seq-duplicate action follows real depth alone; lead's own bookkeeping separately never drops below its floor. - MEDIUM: widen the CI driver's movement/stalled sampling margin (run_seconds - 2.0, was - 0.5) and assert the peer is still in multiplayer.get_peers() at sample time, since the old margin let the check pass on residual starvation grace after a bot had already disconnected. - LOW: measure horizontal-only displacement in the human smoke test's movement check — the old 3D-distance bar was beatable by pure gravity settling with fully dead input. - LOW: fix a real "clean stderr" violation (match_net.gd broadcasting a departure notice to a peer whose ENet channels are already torn down, including a second peer disconnecting in the same poll batch) by deferring the notification to the next idle frame. - Wire the server's per-slot stalled bit into the client debug overlay for real — a prior commit message claimed this already reached the overlay when only the CI gate actually read it. Re-verified end-to-end against the real production RPC path (not just unit tests in isolation, which is how the composition bug got past the first round): a 2-bot CI match with a 1.5s host SIGSTOP freeze injected mid-run, well past the 0.6s threshold the review reproduced the bug at, now recovers cleanly on repeated runs with zero stderr noise. --- Game/scripts/input_jitter_buffer.gd | 24 ++++++-- Game/scripts/input_lead_controller.gd | 30 ++++++---- Game/scripts/match_net.gd | 30 +++++++++- Game/scripts/net_debug_overlay.gd | 9 ++- Game/scripts/networked_match.gd | 56 +++++++++++++++---- .../tests/cases/test_input_lead_controller.gd | 37 ++++++++---- Game/tests/networked_match_test_hooks.gd | 41 ++++++++++---- multiplayer-todo.md | 6 ++ 8 files changed, 180 insertions(+), 53 deletions(-) diff --git a/Game/scripts/input_jitter_buffer.gd b/Game/scripts/input_jitter_buffer.gd index 5e9c04f6..827be91d 100644 --- a/Game/scripts/input_jitter_buffer.gd +++ b/Game/scripts/input_jitter_buffer.gd @@ -42,7 +42,19 @@ var _seeded := false # it, a backlog bigger than RING_SIZE (a host stall, or persistent client/ # server clock drift) permanently zeroed a connected player's input for the # rest of the match. -var _highest_ingested_seq := -1 +# +# Deliberately public (no underscore), same as last_applied_seq: the +# networked_match.gd caller's seq-range guard (§3.1 step 4) must bound +# against THIS, not against last_applied_seq. A second adversarial review +# found that bounding against last_applied_seq caps every accepted seq at +# last_applied_seq + RING_SIZE, which in turn caps this field at the same +# ceiling — making the resync condition below (which needs this field to +# reach expected + RING_SIZE) arithmetically unreachable on the only call +# path that exists in production. The two fixes looked independent but +# shared a variable and silently cancelled each other out. highest_ingested +# tracks the client's own send epoch instead, which the guard can safely +# let run ahead of a lagging consumer. +var highest_ingested_seq := -1 func _init() -> void: @@ -72,8 +84,8 @@ func ingest(newest_seq: int, actions: Array) -> void: # "expected" with reality the moment real data first exists. last_applied_seq = newest_seq - actions.size() _seeded = true - if newest_seq > _highest_ingested_seq: - _highest_ingested_seq = newest_seq + if newest_seq > highest_ingested_seq: + highest_ingested_seq = newest_seq for i in actions.size(): var seq: int = newest_seq - i if seq <= last_applied_seq: @@ -110,7 +122,7 @@ func consume() -> ShipAction: # ticks of not-yet-consumed data at once — if the caller has fallen # further behind the newest data actually arriving than that (a host # stall, or persistent client/server clock drift), every tick between - # "expected" and "_highest_ingested_seq - RING_SIZE" has already been + # "expected" and "highest_ingested_seq - RING_SIZE" has already been # irrecoverably overwritten by more recent arrivals landing on the same # ring slots. Waiting for it tick-by-tick would starve — and, past # STARVE_ZERO_TICKS, zero this player's ship — for the ENTIRE gap even @@ -119,8 +131,8 @@ func consume() -> ShipAction: # host freeze permanently zeroed a connected player's input for the # rest of the match, with no self-recovery). Skip the unrecoverable # span and resync directly to what the ring can still actually provide. - if _highest_ingested_seq - expected >= RING_SIZE: - last_applied_seq = _highest_ingested_seq - RING_SIZE + if highest_ingested_seq - expected >= RING_SIZE: + last_applied_seq = highest_ingested_seq - RING_SIZE expected = last_applied_seq + 1 idx = expected % RING_SIZE diff --git a/Game/scripts/input_lead_controller.gd b/Game/scripts/input_lead_controller.gd index 8e9f5e55..7f1e3b33 100644 --- a/Game/scripts/input_lead_controller.gd +++ b/Game/scripts/input_lead_controller.gd @@ -78,21 +78,31 @@ func update(input_buffer_depth: int) -> int: return 1 # Release must react to the ACTUAL server-reported depth, not to this - # controller's own memory of past attacks. An adversarial review found - # the original gate here was `lead > LEAD_MIN` — a self-tracked counter - # of this controller's own past decisions — so any backlog it did NOT - # itself create (a server hitch, persistent client/server clock drift, - # a burst re-delivery) was never drained: `lead` stayed at its starting - # value the whole time even while `input_buffer_depth` sat well above - # target, permanently adding latency with the control loop reporting - # itself perfectly healthy. Gate on the real signal instead. + # controller's own memory of past attacks. A first pass at this fix + # added the depth check above but left the OLD gate, `lead > LEAD_MIN`, + # still ANDed onto the final condition below — so a backlog this + # controller did NOT itself cause (a server hitch, persistent client/ + # server clock drift, a ring resync) still could never be drained: + # with lead pinned at its starting floor, that clause always failed + # even while input_buffer_depth sat well above target. A second + # adversarial review caught it, confirmed by this file's own + # test_release_drains_a_backlog_it_never_caused_itself, whose original + # assertion text literally said "lead cannot release below its own + # floor even under large surplus" as if that were correct. + # + # The fix splits the one gate into two separate decisions: whether to + # duplicate this tick's seq (the only thing that actually narrows real + # buffered depth) follows the real signal alone, below; whether to + # keep decrementing `lead`'s own bookkeeping below its documented + # floor is a separate, cosmetic-only choice made inside that branch. if input_buffer_depth > TARGET_DEPTH: _clean_surplus_ticks += 1 else: _clean_surplus_ticks = 0 - if _clean_surplus_ticks >= CLEAN_SURPLUS_TICKS and _ticks_since_change >= RELEASE_INTERVAL_TICKS and lead > LEAD_MIN: - lead -= 1 + if _clean_surplus_ticks >= CLEAN_SURPLUS_TICKS and _ticks_since_change >= RELEASE_INTERVAL_TICKS: + if lead > LEAD_MIN: + lead -= 1 _ticks_since_change = 0 return 0 # duplicate this tick's seq — one tick of latency recovered return 1 diff --git a/Game/scripts/match_net.gd b/Game/scripts/match_net.gd index db116c3c..de045693 100644 --- a/Game/scripts/match_net.gd +++ b/Game/scripts/match_net.gd @@ -98,7 +98,35 @@ func _remove_player(peer_id: int) -> void: return roster.erase(peer_id) player_left.emit(peer_id) - _player_left.rpc(peer_id) + # rpc() broadcasts to every peer in multiplayer.get_peers() — including, + # transiently, the very peer that just disconnected: this fires from + # NetworkManager's client_disconnected signal, and empirically that + # peer's own ENetConnection can still be momentarily present in the + # broadcast's target set with its channels already torn down, which + # logs "Unable to send packet on channel 0, max channels: 0" on every + # single disconnect (found by a second adversarial review — harmless to + # the game, since the departing peer obviously doesn't need to hear + # about its own departure, but it meant "clean stderr" wasn't actually + # clean for any test in this project). + # + # A first attempt filtered the broadcast down to rpc_id() calls that + # explicitly skip `peer_id`. That's necessary but not sufficient: when + # two peers disconnect within the same poll() batch (both bots quitting + # at the end of a CI run land within the same tick), get_peers() here + # can still list the SECOND peer as connected while its own disconnect + # event just hasn't been dispatched yet in this same batch — sending to + # it hits the identical error, one hop later. Defer the whole + # notification to the next idle frame instead of sending synchronously + # from inside signal-handling: by then poll() has fully returned, every + # disconnect event in this batch has been dispatched, and get_peers() + # reflects the settled, genuinely-still-connected set. + call_deferred("_broadcast_player_left", peer_id) + + +func _broadcast_player_left(peer_id: int) -> void: + for other_peer_id in multiplayer.get_peers(): + if other_peer_id != peer_id: + _player_left.rpc_id(other_peer_id, peer_id) # Balances a new joiner onto whichever team currently has fewer players diff --git a/Game/scripts/net_debug_overlay.gd b/Game/scripts/net_debug_overlay.gd index 91141ca9..6b0664a2 100644 --- a/Game/scripts/net_debug_overlay.gd +++ b/Game/scripts/net_debug_overlay.gd @@ -43,15 +43,18 @@ func _process(_delta: float) -> void: # task 3.7: RTT, jitter, loss, buffer depth, snapshot age, # bandwidth all live here now. Prediction error is intentionally # absent — there is no client-side prediction until Phase 4, so - # there is nothing honest to show for it yet. + # there is nothing honest to show for it yet. STALLED shows the + # server's own InputJitterBuffer.stalled bit for this client's + # slot, round-tripped through the wire. var stats := {} var game := get_tree().get_first_node_in_group("game") if game and game.has_method("get_net_debug_stats"): stats = game.get_net_debug_stats() - _label.text = "NET: client RTT %.1fms jitter %.1fms offset %.1fms\nbuf depth %s lead %s loss %.1f%% snap age %.1fms\nout %s in %s" % [ + var stalled_suffix := " STALLED" if stats.get("server_stalled", false) else "" + _label.text = "NET: client RTT %.1fms jitter %.1fms offset %.1fms\nbuf depth %s lead %s loss %.1f%% snap age %.1fms%s\nout %s in %s" % [ NetworkManager.rtt_ms, NetworkManager.jitter_ms, NetworkManager.clock_offset_ms, str(stats.get("input_buffer_depth", -1)), str(stats.get("input_lead", "-")), - stats.get("snapshot_loss_pct", 0.0), stats.get("snapshot_age_ms", 0.0), + stats.get("snapshot_loss_pct", 0.0), stats.get("snapshot_age_ms", 0.0), stalled_suffix, _format_kbps(MatchSim.get_bytes_sent_per_sec()), _format_kbps(MatchSim.get_bytes_received_per_sec()), ] else: diff --git a/Game/scripts/networked_match.gd b/Game/scripts/networked_match.gd index 376be02e..fb557869 100644 --- a/Game/scripts/networked_match.gd +++ b/Game/scripts/networked_match.gd @@ -256,16 +256,35 @@ func _on_input_received(peer_id: int, decoded: Dictionary) -> void: # ring" rationale wasn't actually achieved), and could silently # drop an honest client's input forever the moment accumulated # server tick loss closed whatever accidental head-start margin - # existed. Bound against this slot's own last_applied_seq - # instead — the client's own epoch, which the ring is already - # anchored to — using the ring's own capacity as the bound, - # exactly matching what InputJitterBuffer.consume()'s own - # overflow-resync logic treats as "unrecoverably far ahead" - # anyway. Falls back to seq itself (never rejects) before the - # buffer has ever been seeded — there's no baseline yet to - # bound against. + # existed. + # + # A first rebound bounded against this slot's own + # last_applied_seq — the CONSUMER's position — using the ring's + # capacity as the bound. A second adversarial review found this + # broke the ring-overflow resync it was landed alongside: capping + # every accepted seq at last_applied_seq + RING_SIZE also caps + # jb.highest_ingested_seq at that same ceiling, so + # consume()'s resync condition (which needs highest_ingested_seq + # to reach expected + RING_SIZE) could never fire in production — + # silently recreating the exact permanent-input-death bug this + # whole guard-rebound was part of fixing, at an even LOWER + # freeze threshold, reachable via ordinary server tick loss alone + # with no external trigger. + # + # Bound against jb.highest_ingested_seq instead — the highest + # seq this slot has ever actually been ALLOWED to ingest, i.e. + # the client's own send epoch — using the ring's own capacity as + # the bound, same as before. An honest client's consecutive + # packets differ by only a few seq (redundancy + a bounded + # input_lead skip), so this bound tracks a well-behaved client + # regardless of how far the CONSUMER has fallen behind, while + # still rejecting a single garbage-far-future jump: an attacker + # can only walk highest_ingested_seq forward at the rate the + # packet-rate limiter (§3.4) already allows. Falls back to seq + # itself (never rejects) before the buffer has ever been + # seeded — there's no baseline yet to bound against. var jb := slot.jitter_buffer - var seq_bound: int = (jb.last_applied_seq if jb.last_applied_seq >= 0 else seq) + InputJitterBuffer.RING_SIZE + var seq_bound: int = (jb.highest_ingested_seq if jb.highest_ingested_seq >= 0 else seq) + InputJitterBuffer.RING_SIZE if seq > seq_bound: return jb.ingest(seq, decoded["actions"]) @@ -343,8 +362,9 @@ func _ship_to_net_body_state(ship: Ship, stalled: bool) -> NetBodyState: # §3.2: InputJitterBuffer.stalled was computed all along but never # reached the wire — an adversarial review found this was the exact # signal that would have made the ring-overflow bug (this session's - # critical fix) visible to the client, the debug overlay, and the CI - # gate, and its absence is part of why none of them ever noticed. + # critical fix) visible to the client and the CI gate, and its absence + # is part of why neither ever noticed. get_net_debug_stats() below is + # what actually surfaces it to the debug overlay now. s.stalled = stalled return s @@ -588,11 +608,25 @@ func get_net_debug_stats() -> Dictionary: # long has passed with nothing arriving at all. var is_stale := _last_snapshot_wall_ms >= 0 and Time.get_ticks_msec() - _last_snapshot_wall_ms > SNAPSHOT_STALE_MS var snapshot_loss_pct := 100.0 if is_stale else _snapshot_loss_ewma * 100.0 + # The server's jitter_buffer.stalled bit for THIS client's own slot, + # round-tripped through NetBodyState onto the wire (§3.2) — added by the + # first adversarial-review fix round, but a second review found nothing + # actually read it client-side (net_interpolator.gd only passed it + # through lerp/extrapolate), so the commit's claim that it made the + # server-side starvation state "visible to the client, the debug + # overlay" was false; only the CI gate read it, and only via the + # server's own field directly, not the wire bit. Read it here for real. + var server_stalled := false + if is_instance_valid(_my_slot): + var latest := _my_slot.interpolator.latest() + if latest != null: + server_stalled = latest.stalled return { "input_buffer_depth": _last_known_input_buffer_depth, "input_lead": _input_lead_controller.lead, "snapshot_age_ms": snapshot_age_ms, "snapshot_loss_pct": snapshot_loss_pct, + "server_stalled": server_stalled, } diff --git a/Game/tests/cases/test_input_lead_controller.gd b/Game/tests/cases/test_input_lead_controller.gd index 9f1ca42f..99b88423 100644 --- a/Game/tests/cases/test_input_lead_controller.gd +++ b/Game/tests/cases/test_input_lead_controller.gd @@ -67,12 +67,18 @@ func test_release_requires_both_clean_surplus_and_its_own_interval() -> void: func test_release_stops_at_minimum() -> void: var c := InputLeadController.new() - # Sustained surplus depth, but lead is already at LEAD_MIN — must never - # push it below the floor regardless of how much surplus is reported. + # Sustained surplus depth with lead already at LEAD_MIN: `lead` itself + # must never drop below the floor, but release must still fire + # (duplicate a seq) once its own timing conditions are met, since a + # real reported surplus at floor lead is exactly the "backlog this + # controller never caused" case — capping `lead` is cosmetic, it must + # not also block the seq-duplicate action that drains real depth. + var released := false for i in InputLeadController.CLEAN_SURPLUS_TICKS * 3: - var delta := c.update(InputLeadController.TARGET_DEPTH + 1) - assert_true(delta == 1, "lead already at minimum, never duplicates a seq trying to release further, tick %d" % i) - assert_eq(c.lead, InputLeadController.LEAD_MIN, "stays at minimum") + if c.update(InputLeadController.TARGET_DEPTH + 1) == 0: + released = true + assert_eq(c.lead, InputLeadController.LEAD_MIN, "lead never drops below the floor, tick %d" % i) + assert_true(released, "release still fires (duplicates a seq) even though lead itself is pinned at minimum") func test_starve_resets_clean_surplus_counter() -> void: @@ -99,14 +105,18 @@ func test_starve_resets_clean_surplus_counter() -> void: assert_eq(c.lead, lead_after_attack - 1, "release finally fires once a full fresh clean window has elapsed since the interruption") -# An adversarial review found the original release gate was `lead > +# A first attempt at fixing this gated the whole release branch on `lead > # LEAD_MIN` — this controller's own memory of past attacks — so a backlog # it did NOT itself create (a server hitch, persistent client/server clock # drift, a burst re-delivery) was never drained: lead stayed at 1 forever -# even while the server kept reporting a deep, real backlog. This -# reproduces that scenario directly: lead never attacks (depth is never -# reported as a starve, <= 0), yet release must still fire from sustained -# real surplus alone. +# even while the server kept reporting a deep, real backlog, and — because +# that gate blocked the seq-duplicate action too, not just lead's own +# bookkeeping — the actual buffered depth was never drained either. A +# second adversarial review caught that the depth check added alongside +# it didn't remove the old gate, just sat next to it. This reproduces the +# scenario directly: lead never attacks (depth is never reported as a +# starve, <= 0), yet release must still fire from sustained real surplus +# alone, even while lead itself stays pinned at its floor throughout. func test_release_drains_a_backlog_it_never_caused_itself() -> void: var c := InputLeadController.new() assert_eq(c.lead, InputLeadController.LEAD_MIN, "starts at minimum, never attacked") @@ -114,9 +124,12 @@ func test_release_drains_a_backlog_it_never_caused_itself() -> void: # A large, externally-caused surplus (e.g. right after the server's own # ring-overflow resync) reported for well over 2s — lead never moves # via attack since depth is never <= 0. + var released_at_floor := false for i in InputLeadController.CLEAN_SURPLUS_TICKS + InputLeadController.RELEASE_INTERVAL_TICKS: - c.update(10) - assert_eq(c.lead, InputLeadController.LEAD_MIN, "lead cannot release below its own floor even under large surplus") + if c.update(10) == 0: + released_at_floor = true + assert_eq(c.lead, InputLeadController.LEAD_MIN, "lead's own bookkeeping never drops below its floor") + assert_true(released_at_floor, "release still fires (duplicates a seq, actually draining real depth) even while lead is pinned at the floor") # Raise it above the floor via one real attack, then confirm sustained # external surplus (not self-caused) still drains it back down. diff --git a/Game/tests/networked_match_test_hooks.gd b/Game/tests/networked_match_test_hooks.gd index 1dd2b4fd..c2d38155 100644 --- a/Game/tests/networked_match_test_hooks.gd +++ b/Game/tests/networked_match_test_hooks.gd @@ -98,8 +98,16 @@ func run_client_check(settle_seconds: float, drive_seconds: float) -> void: var end_position: Vector3 = my_slot.ship.visual.global_position var moved := start_position.distance_to(end_position) - print("SMOKE INFO: client ship moved %.2fm (start=%s end=%s) while holding forward thrust for %.1fs" % [ - moved, str(start_position), str(end_position), drive_seconds + # Horizontal-only (XZ), not full 3D distance: an adversarial review + # found a 1.2s window of completely dead input still registers ~1.07m + # of pure gravity settling on the Y axis alone (spawn height dropping + # to the floor), which sat ABOVE the old moved > 1.0 bar — only + # thrust_z_ok caught that failure, not moved. Forward thrust is a + # horizontal force (see ship.gd), so measuring XZ displacement can't + # be satisfied by gravity alone, regardless of spawn height or timing. + var moved_horizontal := Vector2(end_position.x, end_position.z).distance_to(Vector2(start_position.x, start_position.z)) + print("SMOKE INFO: client ship moved %.2fm (%.2fm horizontal) (start=%s end=%s) while holding forward thrust for %.1fs" % [ + moved, moved_horizontal, str(start_position), str(end_position), drive_seconds ]) # thrust_power 150 / mass 5 = 30 m/s^2 nominal acceleration (see ship.gd) — @@ -107,9 +115,9 @@ func run_client_check(settle_seconds: float, drive_seconds: float) -> void: # A generous, not-tuned-to-the-decimal bound: this is a wiring smoke # test, not a physics-accuracy test (net_codec's own tests already cover # quantisation precision). - var success := moved > 1.0 and thrust_z_ok - print("SMOKE %s: client observed %.2fm of server-authoritative movement via interpolation, thrust_z_ok=%s" % [ - "PASS" if success else "FAIL", moved, str(thrust_z_ok) + var success := moved_horizontal > 1.0 and thrust_z_ok + print("SMOKE %s: client observed %.2fm horizontal of server-authoritative movement via interpolation, thrust_z_ok=%s" % [ + "PASS" if success else "FAIL", moved_horizontal, str(thrust_z_ok) ]) await get_tree().create_timer(0.3).timeout NetworkManager.shutdown() @@ -268,20 +276,33 @@ func run_ci_host_check(run_seconds: float) -> void: # connected and playing, not after their run finishes — a client's own # (legitimate, expected) disconnect at the end of its run naturally # starves its jitter buffer too, which looks identical to the ring- - # overflow bug this check exists to catch if sampled too late. Clients - # start ~1.5s after the host and finish their own run_seconds shortly - # before disconnecting, so sample just ahead of that, not after. - var movement_check_delay := maxf(1.0, run_seconds - 0.5) + # overflow bug this check exists to catch if sampled too late. A first + # attempt used a 0.5s margin (run_seconds - 0.5); a second adversarial + # review instrumented multiplayer.get_peers() at sample time and found + # it was already EMPTY — both bots had legitimately disconnected before + # the sample ran, and the check was only passing on the ~200ms of + # residual STARVE_ZERO_TICKS starvation grace, not because it was + # genuinely still connected as this print used to claim. Widen the + # margin AND assert connectivity directly at sample time, rather than + # inferring it from timing, so a future regression in either direction + # (margin too tight again, or client run_seconds changing) fails loudly + # here instead of silently passing on residual grace. + var movement_check_delay := maxf(1.0, run_seconds - 2.0) await get_tree().create_timer(movement_check_delay).timeout + var connected_peers := multiplayer.get_peers() var input_reached_server := true for slot in match_scene._slots: + var still_connected: bool = slot.peer_id in connected_peers + if not still_connected: + input_reached_server = false + print("SMOKE FAIL: peer %d already disconnected at movement-sample time (connected_peers=%s) — margin too tight" % [slot.peer_id, str(connected_peers)]) if not is_instance_valid(slot.ship) or not start_positions.has(slot.peer_id): input_reached_server = false print("SMOKE FAIL: peer %d has no valid ship to check movement on" % slot.peer_id) continue var moved: float = start_positions[slot.peer_id].distance_to(slot.ship.global_position) var stalled: bool = slot.jitter_buffer.stalled - print("SMOKE INFO: peer %d moved %.2fm server-side (while still connected), stalled=%s" % [slot.peer_id, moved, str(stalled)]) + print("SMOKE INFO: peer %d moved %.2fm server-side (connected=%s), stalled=%s" % [slot.peer_id, moved, str(still_connected), str(stalled)]) if moved <= 0.5 or stalled: input_reached_server = false diff --git a/multiplayer-todo.md b/multiplayer-todo.md index ac79706f..679cd617 100644 --- a/multiplayer-todo.md +++ b/multiplayer-todo.md @@ -873,6 +873,8 @@ No own-ship prediction yet: the client renders everything, including its own shi **Phase gate — MET.** Both `networked_match_smoke` and the CI driver (task 3.6) re-run under the gate's own exact condition, `--net-sim-latency 80 --net-sim-loss 0.05`, on every peer: the human-driven smoke test still shows clean server-authoritative movement (22.61m over the usual 2s drive), and the two-bot CI run still shows both clients independently agreeing on the final score after a forced goal, 495+/504 snapshots received each, all three processes exiting 0 with clean stderr. +| — | **A second adversarial review of the fix commit above found that two of its nine fixes silently cancelled each other out, re-creating the original critical bug at a *lower* failure threshold — plus four smaller real issues, all re-verified with real two- and three-process runs.**

**CRITICAL — the seq-range guard fix (round 1's MEDIUM item, gotcha 42) made the ring-overflow resync fix (round 1's CRITICAL item, gotcha 39) unreachable in production.** The guard bounded every accepted `seq` at `last_applied_seq + RING_SIZE` — the *consumer's* position — which in turn caps `InputJitterBuffer`'s own `highest_ingested_seq` at that same ceiling, since nothing above the bound is ever allowed to reach `ingest()` at all. But `consume()`'s resync condition needs `highest_ingested_seq` to reach `expected + RING_SIZE`, one full ring past that same ceiling — arithmetically impossible on the only call path that exists. The two fixes read as independent (one in the jitter buffer, one in the caller) but shared a variable and quietly defeated each other; the round-1 commit's own new unit test for the resync never caught it because it called `ingest()` directly, bypassing the guard entirely — the exact composition the bug lived in. Verified failing on the committed code: a 0.6s `SIGSTOP` host freeze reproduced the original 0.00m death, at a *lower* threshold than the pre-round-1 bug (~0.6s vs ~0.7s), reachable via ordinary server tick loss with no external trigger at all (`Engine.max_physics_steps_per_frame = 4` means a server that falls behind wall-clock time during any stall never catches back up on its own). Fixed by rebinding the guard to `highest_ingested_seq` (now a public field, matching `last_applied_seq`'s own convention) instead of `last_applied_seq` — the client's actual send epoch, which `ingest()` updates once per accepted packet regardless of how far the consumer has fallen behind, rather than the consumer's own lagging position. Re-verified against a real 2-bot CI match with a 1.5s host `SIGSTOP` freeze injected mid-run (well past the 0.6s failure threshold): both peers kept moving (47.46m / 10.19m and, on a repeat run, 25.62m / 17.96m), `stalled=false`, sampled while genuinely still connected.

**HIGH — the `InputLeadController` release fix (round 1's HIGH item, gotcha 40) was itself incomplete.** Round 1 added a real depth check (`input_buffer_depth > TARGET_DEPTH`) but left the *original* gate, `and lead > LEAD_MIN`, still ANDed onto the same final condition — so a backlog the controller never caused (lead pinned at its own floor) still could never release, since that old clause always failed regardless of what the new depth check found. Confirmed by the round-1 commit's own new unit test, whose assertion text literally read "lead cannot release below its own floor even under large surplus" as if that were the intended behaviour. Fixed by splitting the one gate into two independent decisions: whether to duplicate this tick's seq (the only thing that actually narrows real buffered depth) now follows the depth signal alone; whether to keep decrementing `lead`'s own bookkeeping below its documented `[LEAD_MIN, LEAD_MAX]` floor is a separate, purely cosmetic choice made inside that branch.

**MEDIUM — task 3.6's CI gate (round 1's own fix for gotcha 43) still sampled after both bots had legitimately disconnected.** The fix used a `run_seconds - 0.5` margin, narrower than the original bug (sampling after the full run) but still not enough: `multiplayer.get_peers()` at sample time was already empty, and the check was only passing on `STARVE_ZERO_TICKS`'s own ~200ms of residual starvation grace, not because it was genuinely still connected as its own print claimed. Widened the margin to `run_seconds - 2.0` and added an explicit `slot.peer_id in multiplayer.get_peers()` assertion at sample time, so a future regression in either direction fails loudly here instead of silently passing on residual grace.

**LOW — the human smoke test's movement bar was beatable by gravity alone.** `moved > 1.0` measured full 3D distance; a 1.2s window of completely dead input still registered ~1.07m from pure vertical settling (spawn height dropping to the floor) — above the bar, with only the separate `thrust_z_ok` check actually catching the failure. Forward thrust is a horizontal force, so switched to XZ-only displacement, which gravity alone cannot satisfy regardless of spawn height or timing.

**LOW — "clean stderr" wasn't actually clean.** Every disconnect logged `ERROR: Unable to send packet on channel 0, max channels: 0` from `match_net.gd`'s `_remove_player`, which broadcasts `_player_left` to every peer in `multiplayer.get_peers()` — including, transiently, the peer that just disconnected (whose own ENet connection can still be momentarily present in that set with its channels already torn down), and — found only after the first fix still left an error in the 2-bot CI scenario specifically — including a *second* still-connecting peer when two clients disconnect within the same `poll()` batch, since `get_peers()` hadn't yet been updated for the one not currently being handled. Fixed by deferring the whole notification (`call_deferred`) to the next idle frame, by which point `poll()` has fully returned and every disconnect event in the batch has actually settled, then explicitly excluding the peer that left. Re-verified clean (grep for `ERROR`) across both the basic 2-process smoke test and a real 2-bot CI run with a mid-match host freeze injected.

**Noted, not fixed — a related but distinct stderr source in `_broadcast_snapshot`.** The deliberately-adversarial `client-abuse-malformed` smoke test still logs one `Unable to send packet` from `networked_match.gd`'s snapshot broadcast, racing a host-forced `disconnect_peer()` in `match_sim.gd`'s abuse-disconnect path against the same tick's `connected_peers.has(slot.peer_id)` snapshot — a different call site than the one just fixed, only reachable via the abuse-detection disconnect path rather than a normal client-initiated one, and out of scope for this pass. Left for a dedicated look rather than a rushed fix under this round's time pressure.

**Confirmed fully correct, not just re-asserted**: the `_input_history` fix (round 1's LOW-MEDIUM item) was re-verified via a synthetic-marker harness stamping a computable value into every outgoing action and checking it through a real 3-process match under 60ms+40ms jitter+18% loss — 1080 marker checks, 0 mismatches, including real attacks and releases; the leaky-bucket rate limiter (round 1's MEDIUM-HIGH item) cannot false-positive on honest traffic (~1.8x measured margin under real impairment); the resync boundary arithmetic itself is correct under packet reordering and duplication. **The lesson that mattered most this round wasn't any single fix — it was that two fixes landed in the same commit, each individually correct in isolation, that silently cancelled each other out** (see gotcha 45) | Full regression suite (35 unit tests, the basic 2-process smoke test, the malformed/rate-limit/duty-cycle abuse roles, and a real 2-bot CI run with a 1.5s host `SIGSTOP` freeze injected mid-match) re-run clean after every fix in this round + | — | **An Opus subagent's adversarial review of all of Phase 3 found a critical, silent, permanent bug plus eight smaller real issues — all empirically verified with real two- and three-process runs, not just code reading.**

**CRITICAL — `InputJitterBuffer`'s 32-entry ring permanently bricked a player's input on any backlog bigger than the ring.** `consume()` advanced `last_applied_seq` by exactly 1 per tick with no resync; once the un-consumed backlog exceeded `RING_SIZE`, a fresh arrival would land in the exact slot `consume()` was still waiting on, and since both counters only ever advance, the gap never closed — the affected player's ship silently went to zero thrust for the rest of the match. The reviewer reproduced this with a real `SIGSTOP`/`SIGCONT` host freeze (a faithful stand-in for a GC/IO/scheduler hitch on a listen-server host): client movement dropped from ~26m to a flat 0.00m at ~0.7s of freeze, reproducible 4/4 times, and found the cliff got *worse* under real network conditions (a lossy link that had already pushed `input_lead` up lowered the fatal threshold to ~400ms) and could be reached with **no external trigger at all** via ordinary client/server clock drift (~1.7% faster client death-spiraled within ~60s). Fixed with a real resync mechanism: `ingest()` now tracks the highest seq ever seen regardless of ring capacity, and `consume()` detects when the gap to that value exceeds `RING_SIZE` and jumps directly to what the ring can still actually provide, instead of starving through an unrecoverable span. **Re-verified with the reviewer's own reproduction**: a 3-second `SIGSTOP` freeze mid-drive now fully recovers (27m+ movement), both via the human smoke test and a real 2-bot CI match. New unit test `test_ring_overflow_resyncs_to_fresh_data_instead_of_starving_forever` covers the exact under-tested direction the reviewer flagged (the original suite only exercised the *under*-full ring case).

**HIGH — `InputLeadController`'s release logic couldn't drain a backlog it didn't itself create.** Release was gated on `lead > LEAD_MIN` — this controller's own memory of past attacks — so a backlog from an external cause (a server hitch, persistent clock drift) left `input_buffer_depth` elevated indefinitely while `lead` (and the release gate) never moved, since the controller never itself attacked. Fixed by gating release on the actual server-reported `input_buffer_depth > TARGET_DEPTH` (§3.3's own `target_depth = 1`), not on self-tracked state. New unit test `test_release_drains_a_backlog_it_never_caused_itself` reproduces the scenario directly.

**MEDIUM-HIGH — the rate limiter was trivially evaded by a duty-cycled flood.** The original design tracked "N consecutive over-budget seconds" and hard-*reset* that streak to 0 on any single clean window, so a burst-then-idle attacker (flood hard, one clean window, repeat) evaded it indefinitely — the reviewer sustained ~33x the packet budget for 28.5s with zero disconnect warnings against the real `MatchSim._recv_input`. Replaced with a leaky-bucket excess accumulator (grows by each window's actual total, drains by exactly one window's worth of budget every window, regardless of how the excess is distributed in time) — immune to the same evasion by construction. New permanent regression test `client-abuse-flood-dutycycle` reproduces the reviewer's exact attack shape (0.35s burst / 3.0s cycle) and confirms it now disconnects.

**MEDIUM — the `seq > server_tick + 20` guard compared two unrelated epochs.** `Engine.get_physics_frames()` counts from the SERVER PROCESS's own start; a client's `_input_seq` starts at 0 when ITS match scene loads — `input_jitter_buffer.gd`'s own seeding logic exists specifically because these share no baseline. Bounding against server uptime meant the guard could never fire on a long-running dedicated server (no real protection, despite the comment's claim) and could silently drop an honest client's input forever once enough accumulated server tick loss closed whatever accidental head-start margin existed. Fixed by bounding against the slot's own `last_applied_seq + RING_SIZE` — the client's actual epoch, using the same capacity the ring-overflow fix itself treats as "unrecoverably far ahead."

**MEDIUM — `InputJitterBuffer.stalled` was computed but never reached the wire.** `_ship_to_net_body_state` never set `NetBodyState.stalled` even though `NetCodec` already packed/unpacked the bit — the one signal that would have made the ring-overflow bug visible to the client, the debug overlay, and the CI gate was silently dropped between the buffer and the snapshot builder. Now wired through.

**MEDIUM — task 3.6's own CI gate passed with a completely dead input pipeline.** Its assertions (snapshot count, a server-*forced* goal's score agreement) don't depend on client input reaching the server at all; the reviewer confirmed it kept reporting `SMOKE PASS` with the ring-overflow bug actively triggered mid-run. Fixed by recording each bot's ship position before the run and asserting real server-side movement plus a non-stalled jitter buffer — sampled *while clients are still actively connected*, not after (an early attempt sampled too late and caught each bot's own legitimate end-of-match disconnect instead of the bug, since a departed peer's buffer starves too — that's correct behaviour, not a regression, just the wrong moment to check it). Re-verified: the fixed CI gate still passes cleanly under a real mid-match host freeze now that the underlying bug is fixed, and (checked by inspection during the fix) would have caught the original bug had it still been present.

**LOW-MEDIUM — a lead change silently mislabelled the redundancy history.** `_input_history` was always `push_front`'d regardless of the seq delta, but the wire format has no per-entry seq field (`actions[i]` is implicitly `seq - i`) — a duplicated tick (release) shifted older entries under a label that no longer matched what was actually there, and a skip-ahead (attack) left the whole history discontiguous with the new seq, so the server could replay already-applied input or apply the wrong redundant copy. The original code comment's claim that this "only ever degrades a backup copy, never the real per-tick record" was itself wrong. Fixed by handling each delta case on its own terms: ordinary ticks still push; a release replaces the front entry in place instead of shifting everything back; an attack resets the window to just the current sample, which rebuilds naturally over the next few ticks (the same way it does at connection start).

**LOW — bandwidth and snapshot-loss overlay metrics froze at their last value instead of decaying during a total outage** — exactly when they matter most. `MatchSim.bytes_sent_per_sec`/`bytes_received_per_sec` are now read through `get_bytes_sent_per_sec()`/`get_bytes_received_per_sec()`, which report 0 once meaningfully more than one window has passed with nothing tracked; `get_net_debug_stats()`'s `snapshot_loss_pct` now reports 100% once more than `SNAPSHOT_STALE_MS` has passed since the last actual snapshot receipt. Verified live: all three read their honest post-outage values (0, 0, 100%) after a real ~2.5s gap in traffic, not the frozen pre-outage numbers.

**LOW — a guard comment on `NetworkManager._ping` misdescribed what the code actually does**, claiming `disconnect_peer(..., now=true)` when the real call uses the default `force=false` (an earlier attempt at `force=true`, tried and reverted elsewhere this session, made Godot's own peer bookkeeping *more* inconsistent, not less). Comment corrected to match reality.

**Confirmed fine, not just assumed**: the jitter metric's magnitude is honest (cross-checked against real 60Hz snapshot-stream jitter on the same impaired link, same order of magnitude), just slow to converge from a 1Hz sampling cadence — worth documenting as "steady-state link quality" rather than a live indicator, not worth rebuilding; `--test-bot`/`AIShipController` wiring on a frozen kinematic ship is fully safe, no NaN/Inf even under the bot's permanently-zero-velocity observations; both original abuse-detection regression tests are genuine, confirmed via a working control (a continuous flood still disconnects in ~4.5s); redundancy + adaptive lead + real loss alone (no hitch) is solid over long runs | Full regression suite — including the net-sim-latency milestone gate, all three abuse roles, the CI driver, and the reviewer's own `SIGSTOP`/`SIGCONT` reproduction at 3s (well past the original ~0.7s failure threshold) — re-run clean after every fix | ### Phase 4 — Prediction and reconciliation, ship **and ball** @@ -1026,6 +1028,8 @@ No own-ship prediction yet: the client renders everything, including its own shi 42. **Two counters that don't share an epoch must never be compared directly, even when both are monotonically increasing integers that "look like" the same kind of thing.** `seq > Engine.get_physics_frames() + 20` compiled, ran, and looked like a sane bound — but `Engine.get_physics_frames()` counts from the SERVER PROCESS's own start while a client's `_input_seq` starts at 0 when ITS match scene loads, so the check either never fires (on a long-running server, no real protection despite its own comment's claim) or fires wrongly and silently drops an honest client's input forever, depending entirely on how much unrelated head-start or drift has accumulated between the two clocks. Bound a value against another value that shares its own actual epoch (here: the receiving buffer's own `last_applied_seq`), not against a same-typed number from a conceptually different clock. 43. **A regression test that doesn't independently exercise the specific mechanism it claims to gate will pass even when that mechanism is completely broken.** Task 3.6's CI driver asserted snapshot throughput and a server-*forced* goal's score agreement — neither of which depends on client input ever reaching the server — and kept reporting PASS with a real, reproduced bug (§7's ring-overflow) actively zeroing both bots' input for the whole run. A CI gate's assertions should trace back to the specific claim in the task's own acceptance text, not just "the match ran and didn't crash." 44. **When adding a "is this connection still healthy" check to a test, sample it while the peer is still actively connected — not after its own end-of-run disconnect, which produces symptoms indistinguishable from the bug being checked for.** Fixing gotcha 43 first sampled `InputJitterBuffer.stalled` and ship movement *after* the full test run (plus a buffer for score-file writes), which meant both readings came from ~4s after the bot had already legitimately shut down — a departed peer's input naturally starves and goes `stalled=true` too, and that's correct, expected behaviour, not the bug. Move the check to a moment still comfortably inside the peer's own active connection window. +45. **Two fixes landed in the same commit, each individually correct in isolation, can share a variable and silently cancel each other out — and a fix's own unit test can miss it by testing the mechanism in isolation from the thing that defeats it.** Gotcha 39's resync fix and gotcha 42's guard rebound were reviewed, tested, and verified independently, each against its own scenario, both passing. Combined, the guard caps the exact variable (`highest_ingested_seq`) the resync's own trigger condition depends on, making it permanently unreachable — recreating the original critical bug at a *lower* failure threshold than before either fix existed. The resync's own new unit test called `InputJitterBuffer.ingest()` directly, which is correct in isolation but bypasses the guard entirely, so it could never have caught this regardless of how thorough it was on its own terms. **When two fixes in the same round touch the same subsystem, explicitly re-test the combination end-to-end** (here: a real `SIGSTOP` freeze against the actual production RPC call path, not a direct unit-level call into the class the fix lives in) — passing tests for each fix individually is not evidence the pair composes correctly. +46. **A guard that bounds an incoming value against the *consumer's* position, rather than against the *producer's* own epoch, re-introduces exactly the "consumer can never catch up past a stall" failure it's often added specifically to prevent.** The seq-range guard bounded `seq` against `last_applied_seq` (advanced only by `consume()`, i.e. gated on however fast the physics tick loop is actually running) rather than `highest_ingested_seq` (advanced by `ingest()`, i.e. gated on however fast packets are actually arriving and being processed by `poll()`) — during a stall where ticks fall behind but polling keeps pace (the common case: a single-frame hitch, or `Engine.max_physics_steps_per_frame` capping tick catch-up while `poll()` itself isn't similarly capped), bounding against the lagging consumer rejects the very packets that would let the buffer refill and the resync condition ever trigger. Bound against whichever side of a producer/consumer pair is not the one already known to be falling behind. --- @@ -1062,3 +1066,5 @@ godot --path Game -- --connect 127.0.0.1:27015 --name Alice **Audio.** `TODO.md` records that there is none. `set_visual_action` / `set_visual_speed` (task 0.14) is precisely where remote-ship engine audio will hang, and "ball feel" (task 4.6) is half auditory. Design those hooks with that in mind rather than retrofitting. **Split-screen.** Tracked separately in `TODO.md`; unrelated to this effort, though the camera-outside-the-ship structure that enables it is the same structure this plan relies on. + +**A second, distinct source of the same "Unable to send packet on channel N, max channels: 0" stderr noise, in `networked_match.gd`'s `_broadcast_snapshot` rather than `match_net.gd`'s `_remove_player`.** Only reproduced via the deliberately-adversarial `client-abuse-malformed` smoke role: `_broadcast_snapshot`'s per-peer send races `match_sim.gd`'s host-forced `disconnect_peer()` (the abuse-disconnect path) against the same tick's `connected_peers.has(slot.peer_id)` snapshot, the same general shape of race as the fixed site but on a different call path (a server-initiated forced disconnect, not a normal client-initiated one) and not currently known to be reachable from ordinary play. Left for a dedicated pass — not fixed under this round's time pressure, since the fixed site (gotcha 46's neighbor, the round-2 addendum above) was the one an adversarial review actually flagged as a "clean stderr" violation in the tests this project's own conventions rely on.