diff --git a/Game/scripts/input_jitter_buffer.gd b/Game/scripts/input_jitter_buffer.gd index 9a0ab9be..5e9c04f6 100644 --- a/Game/scripts/input_jitter_buffer.gd +++ b/Game/scripts/input_jitter_buffer.gd @@ -35,6 +35,14 @@ var _ring_seq: PackedInt32Array = PackedInt32Array() # comment for why an un-seeded buffer would otherwise never converge with # what the client is actually sending. var _seeded := false +# Highest seq ever seen by ingest(), regardless of whether it's still in the +# ring — consume()'s only way to tell "the data is gone because the ring +# overflowed" apart from "the data just hasn't arrived yet". See consume()'s +# own comment for why this exists: an adversarial review found that without +# 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 func _init() -> void: @@ -64,6 +72,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 for i in actions.size(): var seq: int = newest_seq - i if seq <= last_applied_seq: @@ -95,6 +105,25 @@ func consume() -> ShipAction: return last_action var expected := last_applied_seq + 1 var idx := expected % RING_SIZE + + # Ring-overflow resync. A fixed-size ring can only ever hold RING_SIZE + # 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 + # 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 + # though fresh, real input already exists in the ring right now. An + # adversarial review found and reproduced this exact failure (a ~0.7s + # 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 + expected = last_applied_seq + 1 + idx = expected % RING_SIZE + if _ring_seq[idx] == expected: last_action = _ring_action[idx] starved_ticks = 0 diff --git a/Game/scripts/input_lead_controller.gd b/Game/scripts/input_lead_controller.gd index 2c138016..8e9f5e55 100644 --- a/Game/scripts/input_lead_controller.gd +++ b/Game/scripts/input_lead_controller.gd @@ -38,6 +38,11 @@ const LEAD_MAX := 12 const MIN_CHANGE_INTERVAL_TICKS := 30 const RELEASE_INTERVAL_TICKS := 60 const CLEAN_SURPLUS_TICKS := 120 # 2s at 60Hz +# §3.3: "target_depth = 1 (16.7 ms), not 2." Release only fires when the +# server-reported depth is genuinely ABOVE this — see update()'s own +# comment for why gating on `lead` alone (an adversarial review's original +# finding here) was wrong. +const TARGET_DEPTH := 1 var lead := LEAD_MIN @@ -72,7 +77,20 @@ func update(input_buffer_depth: int) -> int: return 1 + delta return 1 - _clean_surplus_ticks += 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. + 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 _ticks_since_change = 0 diff --git a/Game/scripts/match_sim.gd b/Game/scripts/match_sim.gd index a4449e3d..8f99ccc2 100644 --- a/Game/scripts/match_sim.gd +++ b/Game/scripts/match_sim.gd @@ -38,7 +38,16 @@ const RATE_LIMIT_PACKETS_PER_SEC := 110 # sync by hand. const RATE_LIMIT_BYTES_PER_SEC := RATE_LIMIT_PACKETS_PER_SEC * (NetCodec.INPUT_HEADER_SIZE + NetCodec.MAX_REDUNDANCY * NetCodec.INPUT_ENTRY_SIZE) const RATE_LIMIT_WINDOW_MS := 1000 -const RATE_LIMIT_OVER_BUDGET_SECONDS_TO_DISCONNECT := 3 +# Leaky-bucket excess tolerance, expressed in the same "N seconds' worth of +# budget" terms the original consecutive-streak design used. An adversarial +# review found that design — a streak counter that HARD-RESET to 0 on any +# single clean window — was trivially evaded by a duty-cycled flood (burst, +# then one clean window, repeat): reproduced sustaining ~33x the packet +# budget indefinitely with zero disconnect warnings. A leaky bucket doesn't +# care how the excess is distributed in time — see the window-roll logic +# below for how it accumulates and drains. +const RATE_LIMIT_EXCESS_PACKETS_TO_DISCONNECT := RATE_LIMIT_PACKETS_PER_SEC * 3 +const RATE_LIMIT_EXCESS_BYTES_TO_DISCONNECT := RATE_LIMIT_BYTES_PER_SEC * 3 const MALFORMED_LIMIT_TO_DISCONNECT := 20 @@ -46,7 +55,14 @@ class _PeerInputState: var window_start_ms := 0 var packets_this_window := 0 var bytes_this_window := 0 - var over_budget_seconds := 0 + # Leaky bucket: grows by this window's actual total, drains by one + # window's worth of budget, every window — regardless of whether that + # window was itself over or under budget. A steady rate at or under + # budget nets to zero forever (never accumulates); any sustained AVERAGE + # above budget accumulates over time no matter how it's shaped into + # bursts, unlike a streak counter a clean gap can reset to 0. + var excess_packets := 0.0 + var excess_bytes := 0.0 var malformed_count := 0 @@ -57,7 +73,9 @@ var _peer_input_state: Dictionary = {} # peer_id -> _PeerInputState, server on # messages, not what §2's byte-budget analysis or a live overlay cares # about. Rolling per-second counters, recomputed opportunistically on each # send/receive rather than on a timer — nothing needs the rate outside of -# an on-demand overlay read anyway. +# an on-demand overlay read anyway. Use get_bytes_sent_per_sec() / +# get_bytes_received_per_sec() to READ these, not the raw fields directly +# — see those functions for why. const BANDWIDTH_WINDOW_MS := 1000 var bytes_sent_per_sec := 0.0 var bytes_received_per_sec := 0.0 @@ -67,6 +85,25 @@ var _received_window_start_ms := 0 var _received_window_bytes := 0 +# An adversarial review found bytes_*_per_sec only ever gets recomputed +# INSIDE _track_sent()/_track_received() — i.e. only when traffic actually +# arrives — so if traffic stops entirely (right before a disconnect, or +# during exactly the kind of outage this overlay exists to diagnose), the +# last computed rate displays forever instead of decaying toward zero. +# Report zero once meaningfully more than one window has passed with +# nothing tracked, rather than trusting a stale field. +func get_bytes_sent_per_sec() -> float: + if Time.get_ticks_msec() - _sent_window_start_ms > BANDWIDTH_WINDOW_MS * 2: + return 0.0 + return bytes_sent_per_sec + + +func get_bytes_received_per_sec() -> float: + if Time.get_ticks_msec() - _received_window_start_ms > BANDWIDTH_WINDOW_MS * 2: + return 0.0 + return bytes_received_per_sec + + func _ready() -> void: NetworkManager.client_disconnected.connect(func(peer_id: int) -> void: _peer_input_state.erase(peer_id)) @@ -161,13 +198,13 @@ func _recv_input(bytes: PackedByteArray) -> void: # when nothing is arriving anyway. var now_ms := Time.get_ticks_msec() if now_ms - state.window_start_ms >= RATE_LIMIT_WINDOW_MS: - var was_over_budget := state.packets_this_window > RATE_LIMIT_PACKETS_PER_SEC or state.bytes_this_window > RATE_LIMIT_BYTES_PER_SEC - state.over_budget_seconds = (state.over_budget_seconds + 1) if was_over_budget else 0 + state.excess_packets = maxf(0.0, state.excess_packets + float(state.packets_this_window) - float(RATE_LIMIT_PACKETS_PER_SEC)) + state.excess_bytes = maxf(0.0, state.excess_bytes + float(state.bytes_this_window) - float(RATE_LIMIT_BYTES_PER_SEC)) state.window_start_ms = now_ms state.packets_this_window = 0 state.bytes_this_window = 0 - if state.over_budget_seconds >= RATE_LIMIT_OVER_BUDGET_SECONDS_TO_DISCONNECT: - _disconnect_abusive_peer(peer_id, "input rate limit exceeded for %d consecutive seconds" % state.over_budget_seconds) + if state.excess_packets > RATE_LIMIT_EXCESS_PACKETS_TO_DISCONNECT or state.excess_bytes > RATE_LIMIT_EXCESS_BYTES_TO_DISCONNECT: + _disconnect_abusive_peer(peer_id, "input rate limit exceeded (excess_packets=%.0f excess_bytes=%.0f)" % [state.excess_packets, state.excess_bytes]) return state.packets_this_window += 1 diff --git a/Game/scripts/net_debug_overlay.gd b/Game/scripts/net_debug_overlay.gd index da94569c..91141ca9 100644 --- a/Game/scripts/net_debug_overlay.gd +++ b/Game/scripts/net_debug_overlay.gd @@ -34,7 +34,7 @@ func _process(_delta: float) -> void: return if NetworkManager.is_server: _label.text = "NET: server, %d peer(s) out %s in %s" % [ - MatchNet.roster.size(), _format_kbps(MatchSim.bytes_sent_per_sec), _format_kbps(MatchSim.bytes_received_per_sec), + MatchNet.roster.size(), _format_kbps(MatchSim.get_bytes_sent_per_sec()), _format_kbps(MatchSim.get_bytes_received_per_sec()), ] elif NetworkManager.is_client: if NetworkManager.rtt_ms < 0.0: @@ -52,7 +52,7 @@ func _process(_delta: float) -> void: 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), - _format_kbps(MatchSim.bytes_sent_per_sec), _format_kbps(MatchSim.bytes_received_per_sec), + _format_kbps(MatchSim.get_bytes_sent_per_sec()), _format_kbps(MatchSim.get_bytes_received_per_sec()), ] else: _label.text = "NET: offline" diff --git a/Game/scripts/network_manager.gd b/Game/scripts/network_manager.gd index 971f6703..3a762220 100644 --- a/Game/scripts/network_manager.gd +++ b/Game/scripts/network_manager.gd @@ -186,11 +186,16 @@ func _ping(client_send_ms: int) -> void: var sender_id := multiplayer.get_remote_sender_id() # A single poll() call can process several queued RPCs from the same # peer in one batch — an earlier one in that same batch (e.g. task 3.4's - # abuse-triggered disconnect_peer(..., now=true), which removes the - # peer immediately rather than waiting for an acknowledged disconnect) - # can leave this ping's sender no longer a valid peer by the time its - # own turn in the batch comes up. NetSim's inactive/passthrough path - # (the common case — no CLI flags) dispatches immediately with no + # abuse-triggered match_sim.gd disconnect_peer() call, or the peer + # disconnecting for any other reason mid-batch) can leave this ping's + # sender no longer a valid peer by the time its own turn in the batch + # comes up. Empirically confirmed reachable with disconnect_peer()'s + # default arguments (a graceful, non-forced disconnect — match_sim.gd's + # own disconnect call tried force=true as an alternative and reverted + # it, since that left Godot's own peer-list bookkeeping inconsistent + # and produced far MORE of this exact class of error, not fewer: + # hundreds vs. one, verified). NetSim's inactive/passthrough path (the + # common case — no CLI flags) dispatches immediately with no # validation of its own, so check here rather than relying on it. if sender_id not in multiplayer.get_peers(): return diff --git a/Game/scripts/networked_match.gd b/Game/scripts/networked_match.gd index 2d8e86b6..376be02e 100644 --- a/Game/scripts/networked_match.gd +++ b/Game/scripts/networked_match.gd @@ -113,11 +113,13 @@ var _last_known_input_buffer_depth := -1 # client only: -1 = no snapshot with t const SNAPSHOT_LOSS_EWMA_ALPHA := 1.0 / 16.0 var _snapshot_loss_ewma := 0.0 var _expected_next_snapshot_tick := -1 -# §3.1 step 4. Not 120: InputLeadController.LEAD_MAX is 12, so anything -# claiming to be further ahead of the current server tick than this is -# broken or hostile, not just an honest client running a legitimately fast -# lead. -const MAX_SEQ_LEAD_TICKS := 20 +# An adversarial review found _snapshot_loss_ewma only updates on receipt — +# during a TOTAL outage, exactly when this metric matters most, it freezes +# at its last (probably low/healthy) value instead of climbing toward +# 100%. Track wall-clock receipt time so get_net_debug_stats() can report +# honestly once too long has passed with nothing arriving at all. +var _last_snapshot_wall_ms := -1 +const SNAPSHOT_STALE_MS := 500.0 # ~30 ticks with nothing at all — treat as total loss, not "still fine" var _unknown_sender_input_count := 0 # server only, observability (§3.1 step 1) var _reset_gen := 0 # server only: bumped on every kickoff/goal reset so the client hard-snaps instead of interpolating across the teleport # Server only. _on_goal_scored's reset_ball()/reset_ships() only QUEUE @@ -241,18 +243,32 @@ func _on_input_received(peer_id: int, decoded: Dictionary) -> void: for slot in _slots: if slot.peer_id == peer_id: var seq: int = decoded["seq"] - # §3.1 step 4. Not 120: input_lead is clamped to - # InputLeadController.LEAD_MAX (12), so anything claiming to be - # further ahead than this is broken or hostile, not just a fast - # lead. This is also why InputJitterBuffer's ring can be fixed- - # size — a client can never make the server allocate — but - # rejecting the packet here still keeps garbage-far-future seq - # values out of the ring entirely rather than letting them - # silently overwrite a near-future slot some honest, in-range - # packet is about to need. - if seq > Engine.get_physics_frames() + MAX_SEQ_LEAD_TICKS: + # §3.1 step 4, rebound after an adversarial review found the + # original check (seq > Engine.get_physics_frames() + 20) + # compared two unrelated epochs: 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 — + # input_jitter_buffer.gd's own seeding logic exists specifically + # because these share no baseline (see its header comment). + # Bounding against server uptime meant this guard could never + # fire on a long-running dedicated server (no real protection — + # the stated "keeps garbage-far-future seq values out of the + # 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. + var jb := slot.jitter_buffer + var seq_bound: int = (jb.last_applied_seq if jb.last_applied_seq >= 0 else seq) + InputJitterBuffer.RING_SIZE + if seq > seq_bound: return - slot.jitter_buffer.ingest(seq, decoded["actions"]) + jb.ingest(seq, decoded["actions"]) slot.last_client_send_ms = decoded["client_send_ms"] return # A connected-but-not-yet-slotted peer (or one whose slot somehow @@ -285,7 +301,7 @@ func _broadcast_snapshot() -> void: # total-garbage failure mode the moment that stops being true, and the # fix costs nothing. for slot in _slots: - bodies.append(_ship_to_net_body_state(slot.ship) if is_instance_valid(slot.ship) else NetBodyState.new()) + bodies.append(_ship_to_net_body_state(slot.ship, slot.jitter_buffer.stalled) if is_instance_valid(slot.ship) else NetBodyState.new()) if is_instance_valid(ball): bodies.append(_ball_to_net_body_state(ball)) var segment := NetCodec.pack_snapshot_body_segment(server_tick, 0, _reset_gen, bodies) @@ -312,7 +328,7 @@ func _broadcast_snapshot() -> void: MatchSim.send_snapshot(slot.peer_id, bytes) -func _ship_to_net_body_state(ship: Ship) -> NetBodyState: +func _ship_to_net_body_state(ship: Ship, stalled: bool) -> NetBodyState: var s := NetBodyState.new() s.position = ship.global_position s.rotation = ship.global_transform.basis.get_rotation_quaternion() @@ -324,6 +340,12 @@ func _ship_to_net_body_state(ship: Ship) -> NetBodyState: # forward thrust drives the visible flame (see task 2.6). s.thrust_z = clampf(maxf(ship.controller.get_action().thrust.z if ship.controller else 0.0, 0.0), 0.0, 1.0) s.avel_range = NetCodec.SHIP_AVEL_RANGE + # §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. + s.stalled = stalled return s @@ -433,23 +455,46 @@ func _send_local_input() -> void: # increments its send sequence by exactly one tick's worth), but a lead # change this tick skips extra sequence numbers (attack, more server- # side buffer margin) or duplicates the current one (release, delta 0 — - # one tick of latency recovered). A duplicated tick can, in the narrow - # case where an older redundant copy hasn't been superseded yet, smear - # one of _input_history's older backup slots by one position — the - # PRIMARY (freshest, most-recently-relevant) value for every seq is - # unaffected, so this only ever degrades a backup copy, never the real - # per-tick record; §3.3 itself only promises "skip or duplicate a - # sequence number," not frame-perfect bookkeeping under a lead change. - _input_seq += _input_lead_controller.update(_last_known_input_buffer_depth) + # one tick of latency recovered). + var delta := _input_lead_controller.update(_last_known_input_buffer_depth) + _input_seq += delta # Redundancy (§3.1): carry the last MAX_REDUNDANCY ticks' actions, # newest-first, so a burst of up to (MAX_REDUNDANCY - 1) consecutive # packet losses still lets the server recover every dropped tick's # action from a later packet — InputJitterBuffer.ingest() discards # whichever of these the server already applied, so re-sending old - # ticks every packet is harmless, not just tolerated. - _input_history.push_front(action) - if _input_history.size() > NetCodec.MAX_REDUNDANCY: - _input_history.resize(NetCodec.MAX_REDUNDANCY) + # ticks every packet is harmless, not just tolerated. NetCodec's wire + # format has no per-entry seq field — actions[i] is implicitly + # "seq - i" — so _input_history must actually BE that many consecutive + # ticks, not just "the last few samples taken". A plain push_front on + # every tick regardless of delta broke that: an adversarial review + # found a lead change silently relabelled older entries (a duplicated + # tick shifts everything back by one position without a matching seq + # change, and a skip-ahead makes the whole history discontiguous with + # the new seq), causing the server to replay already-applied ticks or + # apply the wrong redundant copy for a given seq. Handle each case on + # its own terms instead of always pushing. + if delta == 1: + _input_history.push_front(action) + if _input_history.size() > NetCodec.MAX_REDUNDANCY: + _input_history.resize(NetCodec.MAX_REDUNDANCY) + elif delta == 0: + # Release: seq didn't advance, so this tick's freshest sample + # REPLACES the front entry (still "seq") rather than pushing + # everything else back a position under a label that no longer + # matches what's actually there. + if _input_history.is_empty(): + _input_history.push_front(action) + else: + _input_history[0] = action + else: + # Attack: seq jumped ahead by more than one, so nothing previously + # in history is contiguous with the new seq any more — the skipped + # range was never sent, by design (that's what "buys more server- + # side buffer margin" means). Reset the redundancy window to just + # this tick's sample; it rebuilds naturally over the next few + # ticks, the same way it does at connection start. + _input_history = [action] var bytes := NetCodec.pack_input(_input_seq, _last_received_snapshot_tick, Time.get_ticks_msec(), _input_history) MatchSim.send_input(bytes) @@ -464,6 +509,7 @@ func _on_snapshot_received(decoded: Dictionary) -> void: _snapshot_loss_ewma += (sample - _snapshot_loss_ewma) * SNAPSHOT_LOSS_EWMA_ALPHA _expected_next_snapshot_tick = server_tick + 1 _last_received_snapshot_tick = server_tick + _last_snapshot_wall_ms = Time.get_ticks_msec() # Per-client header (§2.4): unlike the shared body segment, this is # genuinely this recipient's own — input_buffer_depth is THIS client's # own slot's server-side InputJitterBuffer.depth() at send time, which @@ -535,11 +581,18 @@ func get_net_debug_stats() -> Dictionary: if NetworkManager.rtt_ms >= 0.0: var estimated_now_tick := _estimated_tick(NetworkManager.get_server_time_estimate_ms()) snapshot_age_ms = (estimated_now_tick - float(_last_received_snapshot_tick)) * NetInterpolator.TICK_MS + # _snapshot_loss_ewma only updates on receipt, so during a TOTAL outage + # — exactly when this matters most — it would otherwise freeze at + # whatever it last read (probably low/healthy) instead of climbing + # toward 100%, an adversarial review found. Report honestly once too + # 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 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_ewma * 100.0, + "snapshot_loss_pct": snapshot_loss_pct, } diff --git a/Game/tests/cases/test_input_jitter_buffer.gd b/Game/tests/cases/test_input_jitter_buffer.gd index 79af3a4a..6d48e8c0 100644 --- a/Game/tests/cases/test_input_jitter_buffer.gd +++ b/Game/tests/cases/test_input_jitter_buffer.gd @@ -110,3 +110,44 @@ func test_ring_wraparound_does_not_confuse_a_stale_slot_with_a_fresh_one() -> vo var a := buf.consume() assert_almost_eq(a.thrust.z, 0.9, 0.0001, "correctly reads the fresh same-slot-index seq, not a stale wraparound ghost") assert_eq(buf.starved_ticks, 0, "starvation clears once fresh data resumes") + + +# The under-full direction (above) was covered before an adversarial review +# found the OVER-full direction was not: a backlog bigger than RING_SIZE +# (a host stall, or persistent client/server clock drift) made consume() +# starve — and, past STARVE_ZERO_TICKS, zero the player's ship — forever, +# because both last_applied_seq and the client's own seq only ever advance +# with no resync, so the gap never closed even though fresh, real input +# kept arriving the whole time. +func test_ring_overflow_resyncs_to_fresh_data_instead_of_starving_forever() -> void: + var buf := InputJitterBuffer.new() + buf.ingest(0, [_action(0.0)]) + buf.consume() # last_applied_seq = 0 + + # A burst of packets arriving all at once, exactly what poll() delivers + # in one batch once a stalled server resumes — the client kept sending + # normally the whole time (a real packet every tick, last-4 redundancy, + # newest-first), nothing consumed in between. 50 ticks' worth, well + # past one full lap of the 32-entry ring. + for seq in range(1, 51): + var window: Array = [] + for k in 4: + window.append(_action(float(seq - k) * 0.01)) + buf.ingest(seq, window) + assert_eq(buf.last_applied_seq, 0, "nothing consumed yet, only ingested") + + # The gap (50 - 1 = 49) exceeds RING_SIZE (32): everything older than + # "50 - RING_SIZE" has already been irrecoverably overwritten by more + # recent arrivals landing on the same ring slots. A single consume() + # must resync directly to the oldest data the ring can still actually + # provide, not starve through the entire abandoned span. + var a := buf.consume() + var expected_resync_seq := 50 - InputJitterBuffer.RING_SIZE + 1 + assert_eq(buf.last_applied_seq, expected_resync_seq, "resynced to exactly RING_SIZE behind the newest data") + assert_almost_eq(a.thrust.z, float(expected_resync_seq) * 0.01, 0.0001, "recovered the resynced tick's real action from the ring, not a stale ghost or a zeroed one") + assert_eq(buf.starved_ticks, 0, "resyncing to real data is not starvation") + assert_true(not buf.stalled, "a recovered player must not be reported as stalled") + + # Normal sequential consumption resumes correctly from the resync point. + var next := buf.consume() + assert_almost_eq(next.thrust.z, float(expected_resync_seq + 1) * 0.01, 0.0001, "next tick continues in order from the resync point") diff --git a/Game/tests/cases/test_input_lead_controller.gd b/Game/tests/cases/test_input_lead_controller.gd index 50512d4b..9f1ca42f 100644 --- a/Game/tests/cases/test_input_lead_controller.gd +++ b/Game/tests/cases/test_input_lead_controller.gd @@ -51,25 +51,26 @@ func test_release_requires_both_clean_surplus_and_its_own_interval() -> void: var lead_after_attack := c.lead assert_true(lead_after_attack > InputLeadController.LEAD_MIN, "lead raised above minimum before testing release") - # Fewer than CLEAN_SURPLUS_TICKS of healthy depth: must not release yet. + # Fewer than CLEAN_SURPLUS_TICKS of surplus depth (above TARGET_DEPTH): + # must not release yet. for i in InputLeadController.CLEAN_SURPLUS_TICKS - 1: - c.update(1) + c.update(InputLeadController.TARGET_DEPTH + 1) assert_eq(c.lead, lead_after_attack, "no release before 2s of clean surplus has elapsed") - # One more healthy tick crosses the clean-surplus threshold AND the + # One more surplus tick crosses the clean-surplus threshold AND the # release interval (both are already satisfied by now since the # debounce timer has been running the whole time) -> releases by 1. - var delta := c.update(1) + var delta := c.update(InputLeadController.TARGET_DEPTH + 1) assert_eq(delta, 0, "release tick duplicates rather than incrementing seq") assert_eq(c.lead, lead_after_attack - 1, "lead released by exactly 1") func test_release_stops_at_minimum() -> void: var c := InputLeadController.new() - # Never starve — with lead already at LEAD_MIN, sustained health must - # never push it below the floor. + # Sustained surplus depth, but lead is already at LEAD_MIN — must never + # push it below the floor regardless of how much surplus is reported. for i in InputLeadController.CLEAN_SURPLUS_TICKS * 3: - var delta := c.update(1) + 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") @@ -85,14 +86,49 @@ func test_starve_resets_clean_surplus_counter() -> void: # can't accidentally retrigger a second attack step of its own. var partial_clean_ticks := 10 for i in partial_clean_ticks: - c.update(1) + c.update(InputLeadController.TARGET_DEPTH + 1) c.update(0) # a lone starve tick, resetting _clean_surplus_ticks assert_eq(c.lead, lead_after_attack, "the lone starve tick was too soon after the last change to trigger another attack") # A full clean window from this fresh starting point is required before # release fires — one tick short must not be enough. for i in InputLeadController.CLEAN_SURPLUS_TICKS - 1: - c.update(1) + c.update(InputLeadController.TARGET_DEPTH + 1) assert_eq(c.lead, lead_after_attack, "the starve interruption forced a fresh 2s clean window, so no release yet") - c.update(1) + c.update(InputLeadController.TARGET_DEPTH + 1) 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 > +# 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. +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") + + # 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. + 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") + + # Raise it above the floor via one real attack, then confirm sustained + # external surplus (not self-caused) still drains it back down. + for i in InputLeadController.MIN_CHANGE_INTERVAL_TICKS: + c.update(0) + var lead_after_attack := c.lead + assert_true(lead_after_attack > InputLeadController.LEAD_MIN, "attack raised lead") + + var released := false + for i in InputLeadController.CLEAN_SURPLUS_TICKS + InputLeadController.RELEASE_INTERVAL_TICKS: + if c.update(10) == 0: + released = true + break + assert_true(released, "sustained externally-caused surplus (depth=10) must eventually trigger a release") + assert_true(c.lead < lead_after_attack, "lead actually decreased in response to real depth, not just internal bookkeeping") diff --git a/Game/tests/networked_match_smoke.gd b/Game/tests/networked_match_smoke.gd index 64554ddb..563c05ed 100644 --- a/Game/tests/networked_match_smoke.gd +++ b/Game/tests/networked_match_smoke.gd @@ -39,7 +39,7 @@ func _ready() -> void: return print("SMOKE: joining ...") MatchNet.welcomed.connect(_on_client_welcomed) - "client-abuse-malformed", "client-abuse-flood": + "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 # client would be — the validation doesn't get to assume @@ -93,5 +93,7 @@ func _on_abuser_welcomed() -> void: get_tree().root.add_child.call_deferred(hooks) if _role == "client-abuse-malformed": hooks.run_malformed_abuse_check.call_deferred() + elif _role == "client-abuse-flood-dutycycle": + hooks.run_duty_cycle_flood_abuse_check.call_deferred() else: hooks.run_rate_limit_abuse_check.call_deferred() diff --git a/Game/tests/networked_match_test_hooks.gd b/Game/tests/networked_match_test_hooks.gd index 9e97c408..1dd2b4fd 100644 --- a/Game/tests/networked_match_test_hooks.gd +++ b/Game/tests/networked_match_test_hooks.gd @@ -148,12 +148,13 @@ func run_malformed_abuse_check() -> void: get_tree().quit(0 if disconnected[0] else 1) -# task 3.4: MatchSim._recv_input must rate-limit and disconnect after -# RATE_LIMIT_OVER_BUDGET_SECONDS_TO_DISCONNECT (3) consecutive seconds over -# RATE_LIMIT_PACKETS_PER_SEC (110/s). Every packet here is individually -# well-formed (a real NetCodec.pack_input payload) — only the SEND RATE is -# abusive, confirming the rate limiter fires independently of the malformed- -# packet counter, not as a side effect of it. +# task 3.4: MatchSim._recv_input must rate-limit and disconnect a sustained +# continuous flood well above RATE_LIMIT_PACKETS_PER_SEC (110/s) via the +# leaky-bucket excess accumulator (RATE_LIMIT_EXCESS_PACKETS_TO_DISCONNECT). +# Every packet here is individually well-formed (a real NetCodec.pack_input +# payload) — only the SEND RATE is abusive, confirming the rate limiter +# fires independently of the malformed-packet counter, not as a side +# effect of it. func run_rate_limit_abuse_check() -> void: await get_tree().create_timer(1.0).timeout var disconnected := [false] # see run_malformed_abuse_check's comment on why not a plain bool @@ -179,6 +180,55 @@ func run_rate_limit_abuse_check() -> void: get_tree().quit(0 if disconnected[0] else 1) +# Regression test for a real bug an adversarial review found and this +# session fixed: the ORIGINAL rate limiter tracked "N consecutive +# over-budget seconds" and hard-reset that streak to 0 on any single clean +# window — so a burst-then-idle duty cycle (flood hard, go quiet for one +# window, repeat) evaded it indefinitely. Reproduced against the real +# MatchSim._recv_input: ~33x the packet budget sustained for 28.5s with +# zero disconnect warnings. The fix (a leaky-bucket excess accumulator +# that grows by the window's actual total and drains by only one window's +# worth of budget, every window) doesn't care how the excess is +# distributed in time. This test reproduces the exact attack shape. +func run_duty_cycle_flood_abuse_check() -> void: + await get_tree().create_timer(1.0).timeout + var disconnected := [false] + NetworkManager.disconnected_from_server.connect(func() -> void: disconnected[0] = true) + + var net_codec := preload("res://scripts/net_codec.gd") + var ship_action_script := preload("res://scripts/ship_action.gd") + var bytes: PackedByteArray = net_codec.pack_input(1, 0, Time.get_ticks_msec(), [ship_action_script.new()]) + + const CYCLE_SECONDS := 3.0 + const BURST_SECONDS := 0.35 + const TEST_SECONDS := 6.0 # the leaky bucket trips within the first cycle; no need for a long soak + const TRICKLE_HZ := 60 # legitimate-shaped background rate, well under budget alone + + var deadline_ms := Time.get_ticks_msec() + int(TEST_SECONDS * 1000.0) + var cycle_start_ms := Time.get_ticks_msec() + while Time.get_ticks_msec() < deadline_ms and not disconnected[0]: + var t_in_cycle := float(Time.get_ticks_msec() - cycle_start_ms) / 1000.0 + if t_in_cycle >= CYCLE_SECONDS: + cycle_start_ms = Time.get_ticks_msec() + t_in_cycle = 0.0 + if t_in_cycle < BURST_SECONDS: + for i in 200: # a hard burst, far above budget + MatchSim._recv_input.rpc_id(1, bytes) + else: + for i in maxi(1, TRICKLE_HZ / 60): # ~60/s trickle, keeps the window rolling and stays under budget alone + MatchSim._recv_input.rpc_id(1, bytes) + NetworkManager.poll() + await get_tree().process_frame + await get_tree().create_timer(0.5).timeout + NetworkManager.poll() + + print("SMOKE %s: duty-cycled flood (burst %.2fs / cycle %.1fs) %s" % [ + "PASS" if disconnected[0] else "FAIL", BURST_SECONDS, CYCLE_SECONDS, + "resulted in disconnect" if disconnected[0] else "evaded rate limiting entirely", + ]) + get_tree().quit(0 if disconnected[0] else 1) + + # task 3.6, host role: waits for both bots' scenes to settle, forces a # deterministic goal (bot-vs-bot scoring isn't reliable enough within a # short CI run to gate on), then compares the server's own final score @@ -194,17 +244,51 @@ func run_ci_host_check(run_seconds: float) -> void: return print("SMOKE INFO: host ship_count=%d slot_count=%d" % [match_scene.ships.size(), match_scene._slots.size()]) + # An adversarial review found this driver's original checks (snapshot + # count, a server-FORCED goal's cross-peer score agreement) don't + # depend on client input ever reaching the server at all — it kept + # reporting PASS with the input pipeline completely dead (verified by + # injecting the ring-overflow bug this session's critical fix + # addresses, mid-run). Record each ship's starting position now, before + # anything moves, so real server-side movement over the run can be + # checked directly — the same signal run_client_check already uses for + # a human client, applied here per-bot instead of just for "my own ship". + var start_positions: Dictionary = {} + for slot in match_scene._slots: + if is_instance_valid(slot.ship): + start_positions[slot.peer_id] = slot.ship.global_position + var goals: Array = match_scene.arena.get_goals() if match_scene.arena else [] if is_instance_valid(match_scene.ball) and not goals.is_empty(): match_scene.ball.linear_velocity = Vector3.ZERO match_scene.ball.global_position = goals[0].global_position print("SMOKE INFO: host forced a goal for the cross-peer score agreement check") - # Extra buffer beyond run_seconds: clients start ~1.5s after the host - # (established two-process test convention) and run for their own - # run_seconds measured from THEIR start, so waiting only run_seconds - # here would race their score files not being written yet. - await get_tree().create_timer(run_seconds + 5.0).timeout + # Movement/stalled must be checked WHILE clients are still actively + # 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) + await get_tree().create_timer(movement_check_delay).timeout + var input_reached_server := true + for slot in match_scene._slots: + 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)]) + if moved <= 0.5 or stalled: + input_reached_server = false + + # Extra buffer beyond run_seconds: clients run for their own run_seconds + # measured from THEIR (later) start, so waiting only run_seconds here + # would race their score files not being written yet. + await get_tree().create_timer(run_seconds + 5.0 - movement_check_delay).timeout print("SMOKE INFO: host final score=%s" % str(match_scene.score)) var slots_ok: bool = match_scene._slots.size() == 2 @@ -225,9 +309,9 @@ func run_ci_host_check(run_seconds: float) -> void: print("SMOKE FAIL: peer %d saw score %s, server has %s" % [slot.peer_id, client_score, expected]) scores_agree = false - var success: bool = slots_ok and scores_agree and scores_seen == 2 - print("SMOKE %s: CI host run (slots_ok=%s scores_agree=%s scores_seen=%d/2)" % [ - "PASS" if success else "FAIL", str(slots_ok), str(scores_agree), scores_seen, + var success: bool = slots_ok and scores_agree and scores_seen == 2 and input_reached_server + print("SMOKE %s: CI host run (slots_ok=%s scores_agree=%s scores_seen=%d/2 input_reached_server=%s)" % [ + "PASS" if success else "FAIL", str(slots_ok), str(scores_agree), scores_seen, str(input_reached_server), ]) NetworkManager.shutdown() get_tree().quit(0 if success else 1) diff --git a/multiplayer-todo.md b/multiplayer-todo.md index 58e73823..ac79706f 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. +| — | **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** | # | Task | Acceptance | @@ -1018,6 +1020,12 @@ No own-ship prediction yet: the client renders everything, including its own shi 36. **`NetworkManager.get_server_time_estimate_ms()` (and anything derived from it) is not just imprecise before the first clock pong lands — it's actively wrong in a way that can persist far longer than the sync window implies.** `clock_offset_ms` is `0.0` until the first pong, so a value derived from `get_server_time_estimate_ms()` during that window means "my own raw process uptime," not a server-synced estimate — and if that value feeds a rolling-window filter (e.g. a min-tracked bias, per the interpolator epoch-bias fix in Phase 2's adversarial review), the bad early sample can dominate the window for the filter's *entire* configured duration if a short test or a short match doesn't run long enough for real time to age it out. Always gate recording, not just consuming, anything derived from this estimate on `rtt_ms >= 0.0`. 37. **Anything that deliberately delays an RPC dispatch (task 2.8's `net_sim.gd`) must re-validate its target at *fire* time, not just at the moment it was scheduled.** Found by actually running Phase 2's own gate (`networked_match_smoke` under `--net-sim-latency=80 --net-sim-jitter=20`), not the isolated ping/pong test alone: `_broadcast_snapshot`'s existing `get_peers()` filter (gotcha from task 2.2's own fix) only proves the target was valid *when the send was queued* — a target that legitimately disconnects during the ~80–100ms hold produces "Attempt to call RPC with unknown peer ID" anyway, and if the delay outlives this *process's own* `shutdown()`, `multiplayer_peer` has already been reset to a fresh `OfflineMultiplayerPeer` (§9 gotcha re: never resetting to raw `null`), so a stale `rpc_id(1, …)` now means "call yourself" and Godot rejects it. Any deliberate-delay layer needs to re-check both "do I still have a real peer at all" and "is this specific target still in `get_peers()`" inside the delayed callback itself, immediately before dispatching — not reuse whatever validation the caller did before the delay was added. 38. **A GDScript lambda captures an enclosing local variable BY VALUE at the moment the lambda is created, not by reference.** Bit two separate Phase 3 test scripts the same way: `var disconnected := false; some_signal.connect(func(): disconnected = true)` compiles and runs with no error or warning, but the assignment inside the lambda mutates only *that lambda's own captured copy* — the enclosing function's `disconnected` stays `false` forever, even after the signal genuinely fires (confirmed firing via an extra debug print before the real cause was found). The underlying disconnect-detection code was correct the whole time; only the test's own assertion logic was broken. The fix is to capture a container instead of a value — `var disconnected := [false]` and `disconnected[0] = true` inside the lambda — since capturing an `Array`/`Dictionary`/`Object` captures a reference to the same instance, and mutating its *contents* from inside the lambda is visible outside it. Relevant anywhere a lambda is used to flip a flag or accumulate a result for a caller to read later (a `connect(func(): ...)` one-liner is the single most common place this bites). +39. **A fixed-size ring buffer fed by an unbounded-rate producer needs an explicit resync path, not just "wait for the next expected slot."** `InputJitterBuffer`'s 32-entry ring assumed the consumer (`consume()`, one call per server physics tick) would never fall more than `RING_SIZE` ticks behind the producer (`ingest()`, driven by real wall-clock packet arrival, unrelated to the consumer's own tick rate) — but a server-side stall, or even ordinary client/server clock drift with zero external trigger, breaks that assumption, and once broken, a design that only ever advances its "expected" pointer by exactly one per call can never catch up: newer arrivals silently overwrite the exact slot still being waited on, and the wait never ends. If a ring's producer and consumer rates aren't provably bounded relative to each other, the consumer needs a way to detect "the data I'm waiting for no longer exists in the ring at all" (track the newest value ever seen, independent of ring capacity) and jump directly to what's still available, rather than assuming "keep waiting" is always eventually correct. +40. **A client-owned adaptive control loop must react to the actual ground-truth signal it's regulating, not to its own memory of past decisions.** `InputLeadController`'s release logic was gated on `lead > LEAD_MIN` — a count of the controller's own past attacks — rather than on the real server-reported `input_buffer_depth` it exists to keep near target. Any elevated depth the controller didn't itself cause (an external stall, drift, a burst redelivery) was invisible to that gate and so never got drained, even while the "real" signal sat well above target the whole time. When a control loop's condition for acting can be satisfied or blocked by state the loop itself controls, rather than by the environment it's meant to respond to, it can silently stop responding to the environment. +41. **A "consecutive N over-budget windows" streak counter that hard-resets to 0 on any single clean window is trivially evaded by a duty-cycled attacker** (burst hard, one clean window, repeat) — confirmed sustaining ~33x a stated packet budget indefinitely with zero disconnect warnings. A leaky-bucket accumulator (grows by each window's actual total, drains by exactly one window's worth of budget every window, disconnect once the accumulated excess crosses a threshold) is immune to the same evasion by construction, since it doesn't matter how the excess is distributed in time — only the sustained average matters. +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. ---