Files
CosmicClash/Game/scripts/input_jitter_buffer.gd
T
Josh Creek cf73074e27 fix(multiplayer): resolve composition regression from second adversarial review
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.
2026-08-20 18:26:12 +01:00

155 lines
7.0 KiB
GDScript

class_name InputJitterBuffer
extends RefCounted
# Per-player server-side input state (multiplayer-todo.md §3, task 3.2).
# Deliberately a standalone RefCounted with no scene/RPC dependency — same
# reason net_codec.gd and net_interpolator.gd are pure classes — so task
# 3.5's unit tests can drive it with scripted arrival traces with no live
# match. NetworkedMatch owns one instance per connected slot and is the only
# thing that talks to the network layer; this class only knows about
# sequence numbers and ShipActions.
#
# Ring is fixed-size and slot-tagged (§3.1 step 5's "a client can never make
# the server allocate"): ingest() writes seq % RING_SIZE regardless of how
# large or malicious seq is, and consume() only ever trusts a slot whose
# stored seq exactly matches the one it expects — a stale or wrapped-around
# entry is indistinguishable from an empty one. Range/rate validation of seq
# against the current server tick is the CALLER's job (task 3.4), not this
# class's, since only the caller knows the current server tick.
const RING_SIZE := 32
# 500ms at 60Hz (multiplayer-todo.md §3.2's own numbers) — a duration, not a
# tick-rate-derived constant, so left as a literal rather than pulling in
# SimConstants for one number.
const STARVE_ZERO_TICKS := 30
var last_applied_seq := -1 # -1: consume() has never been called yet
var last_action := ShipAction.new()
var starved_ticks := 0
var stalled := false
var _ring_action: Array = []
var _ring_seq: PackedInt32Array = PackedInt32Array()
# True once ingest() has ever been called for real. Consumption is a no-op
# (no starvation counted, no advancement) until then — see ingest()'s own
# 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.
#
# 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:
_ring_action.resize(RING_SIZE)
_ring_seq.resize(RING_SIZE)
for i in RING_SIZE:
_ring_seq[i] = -1
# newest_seq/actions match NetCodec.unpack_input's own "seq"/"actions"
# fields directly: actions[i] is the action for sequence (newest_seq - i),
# newest-first. Already-consumed or stale entries are silently discarded
# (§3.1 step 5) — this is what makes redundant re-delivery of an already-
# applied tick harmless.
func ingest(newest_seq: int, actions: Array) -> void:
if not _seeded:
# The server starts calling consume() every tick the instant this
# slot exists — well before this player's first packet has had time
# to arrive (connection handshake, arena/ship spawn, first
# _physics_process tick on the client all take real time first). An
# un-seeded last_applied_seq of -1 would have consume() "expecting"
# sequence 0, 1, 2, ... via pure starvation the whole time, racing
# arbitrarily far ahead of whatever the client's own from-1
# numbering has actually reached by the time real packets show up —
# and since both sides only ever advance monotonically with no
# resync mechanism, that gap would never close. Seed to align
# "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:
continue
var idx := seq % RING_SIZE
_ring_seq[idx] = seq
_ring_action[idx] = actions[i]
# Contiguous run of not-yet-applied entries starting right after
# last_applied_seq — reported as input_buffer_depth in every snapshot
# (§3.3) and consumed client-side by the input_lead control loop (task 3.3).
func depth() -> int:
if not _seeded or last_applied_seq < 0:
return 0
var d := 0
var seq := last_applied_seq + 1
while d < RING_SIZE and _ring_seq[seq % RING_SIZE] == seq:
d += 1
seq += 1
return d
# Called once per server physics tick, before the step (§3.2). A no-op
# (returns the zero-initialized last_action, no starvation counted) until
# this player's first real packet has ever arrived — see ingest()'s comment.
func consume() -> ShipAction:
if not _seeded:
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
stalled = false
else:
# Repeat-last, not zero: inputs are heavily autocorrelated at 60Hz,
# and the client already predicted with the real input either way,
# so repeating minimises expected divergence (§3.2). Only zero after
# a sustained stall, so a disconnecting player's ship doesn't fly
# into a wall at full throttle forever.
starved_ticks += 1
if starved_ticks > STARVE_ZERO_TICKS:
last_action = ShipAction.new()
stalled = true
last_applied_seq = expected
return last_action