Repository navigation
fix(netplay): grade the desync verdict so a transient doesn't end the game - #278
Conversation
… game
First PR of the `v1.27.0` netplay-depth rung.
## The behaviour that changed
Until now the FIRST checksum mismatch was a fatal `NetplayError::Desync`,
and `app.rs` disconnected on it. That is too eager: a burst-reordered
pair of `Checksum` messages can momentarily disagree before the deferred
`compare_pending_checksums` pass reconciles them, so a transient network
event ended a healthy game.
`diagnostics::DesyncDiagnostics` now records EVERY comparison -- matching
ones included -- and folds them into one graded `DesyncStatus`:
InSync
Suspect { consecutive, first_desync_frame }
Desynced { first_desync_frame }
Three consecutive mismatches (~1.5 s at the default 30-frame interval)
confirm. The fatal error fires only on `Desynced`, so a transient is
survived while a genuine divergence still ends the session -- which it
must, since a rollback desync cannot recover without a full state
resync.
`Desynced` is STICKY. A later stray match resets the live consecutive run
but never downgrades the verdict, because the underlying condition cannot
heal. A surface that flapped between "desynced" and "fine" would train
the user to ignore it.
## The framebuffer hash was being thrown away
`compare_pending_checksums` destructured the remote report as
`let (_, remote_hash, _) = ...`, discarding the framebuffer-only hash the
message already carried. It now reaches the diagnostics, and it is what
makes a mismatch diagnosable rather than merely alarming: same picture
with a different combined digest means a TIMING divergence; a different
picture means a STATE one.
## Determinism is untouched
The module only reads values the session already computed and stores
copies; it never feeds back into rollback, the checksum exchange, or the
emulator, and it holds no wall-clock (that belongs in the connection
layer, a later PR). Deleting it would leave every produced frame,
checksum, and rollback byte-identical. `tests/determinism.rs` passes
unchanged.
`DesyncDiagnostics::new`/`with_threshold` are `const fn` so
`RollbackSession::new` keeps being one; that is why the threshold clamp
is spelled out rather than `.max(1)`, which is not const.
## Tests
`diagnostics.rs`'s 9 unit tests cover the verdict arithmetic. The new
`tests/desync_hysteresis.rs` covers what actually changed, by driving a
real session against forged peer checksums:
* a single mismatched checksum no longer ends the session;
* a sustained divergence still does, having crossed the threshold
rather than firing early;
* a clean session stays `InSync`.
The transient test carries an ANTI-VACUOUS GUARD: surviving is only
meaningful if the forged checksum actually reached the comparator, so it
asserts `total == 1` and `mismatches == 1` rather than merely that no
error came back. Measured before asserting.
Both were verified by re-injecting the old fail-on-first behaviour and
confirming they fail -- the transient one with exactly the
`Err(Desync { frame: 0, .. })` this PR removes.
Verified: `cargo fmt --check`; clippy `-D warnings` on the crate and on
`rustysnes-frontend --features netplay`; `cargo doc` warnings-as-errors;
29 netplay tests green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
WalkthroughThe change adds bounded checksum diagnostics to netplay. ChangesNetplay desync diagnostics
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant RollbackSession
participant DesyncDiagnostics
participant NetplayError
RollbackSession->>DesyncDiagnostics: record checksum and framebuffer hashes
DesyncDiagnostics-->>RollbackSession: return graded status
RollbackSession->>NetplayError: raise Desync after confirmed threshold
🚥 Pre-merge checks | ✅ 10✅ Passed checks (10 passed)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/rustysnes-netplay/src/session.rs (1)
496-554: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
NetplayError::Desync's payload is not consistently the first-divergence data diagnostics promises.
first_mismatch(Line 512, 528-530) captures the mismatch found in the CURRENT call tocompare_pending_checksums, notself.diagnostics.first_desync_frame(). When the threshold-confirming mismatch is processed in this call (the common case), the returnedframe/local_hash/remote_hashdescribe the CONFIRMING comparison (e.g. the 3rd consecutive mismatch), not the earliest divergence. This is exactly the value diagnostics.rs itself says is the one worth reporting:record()'s own comment states the earliest diverging frame "is where a bisect would start." A user diagnosing a "desync detected at frame 90" error has no way to know the divergence actually started at frame 0.The fallback path (Lines 538-546, used when
first_mismatchisNone) is worse: it takesframefromfirst_desync_frame()butlocal_hash/remote_hashfromlast().last()is the single most recent comparison of any kind, matched or not. Ifis_desynced()stays true (sticky, by design) while a later comparison happens to match, this fallback reportslocal_hash == remote_hashinside a "desync detected" error — self-contradictory output.Add a sticky field to
DesyncDiagnosticsthat preserves the hash values of the record that first pushedfirst_desync_frame, and always source the error payload from diagnostics rather than from this call's localfirst_mismatch, so the reported frame/hashes are consistent regardless of which pass confirms the desync.🐛 Proposed fix: sticky first-mismatch payload
--- a/crates/rustysnes-netplay/src/diagnostics.rs +++ b/crates/rustysnes-netplay/src/diagnostics.rs @@ struct DesyncDiagnostics { first_desync_frame: Option<u32>, + /// `(local, remote)` digests of the comparison that first set `first_desync_frame`, kept + /// alongside it so the reported hashes always describe an actual mismatch, never the ring's + /// most recent (possibly matching) entry. + first_mismatch_hashes: Option<(u64, u64)>, @@ pub fn record(&mut self, frame: u32, local: u64, remote: u64, local_fb: u64, remote_fb: u64) { } else { self.mismatches = self.mismatches.saturating_add(1); self.consecutive_mismatches = self.consecutive_mismatches.saturating_add(1); self.peak_consecutive = self.peak_consecutive.max(self.consecutive_mismatches); - self.first_desync_frame = Some(self.first_desync_frame.map_or(frame, |f| f.min(frame))); + if self.first_desync_frame.is_none_or(|f| frame < f) { + self.first_desync_frame = Some(frame); + self.first_mismatch_hashes = Some((local, remote)); + } } + + /// `(local, remote)` digests of the earliest recorded mismatch, if any. + #[must_use] + pub const fn first_mismatch_hashes(&self) -> Option<(u64, u64)> { + self.first_mismatch_hashes + }--- a/crates/rustysnes-netplay/src/session.rs +++ b/crates/rustysnes-netplay/src/session.rs @@ fn compare_pending_checksums(&mut self) -> Result<(), NetplayError> { - if self.diagnostics.is_desynced() { - let (frame, local_hash, remote_hash) = first_mismatch.unwrap_or_else(|| { - let frame = self.diagnostics.first_desync_frame().unwrap_or_default(); - let last = self.diagnostics.last(); - ( - frame, - last.map_or(0, |c| c.local), - last.map_or(0, |c| c.remote), - ) - }); - return Err(NetplayError::Desync { - frame, - local_hash, - remote_hash, - }); - } + if self.diagnostics.is_desynced() { + let frame = self.diagnostics.first_desync_frame().unwrap_or_default(); + let (local_hash, remote_hash) = + self.diagnostics.first_mismatch_hashes().unwrap_or((0, 0)); + return Err(NetplayError::Desync { + frame, + local_hash, + remote_hash, + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/rustysnes-netplay/src/session.rs` around lines 496 - 554, Update DesyncDiagnostics::record to persist the local and remote hashes associated with the first frame that establishes first_desync_frame, retaining them thereafter. Expose that sticky first-divergence payload through the diagnostics API, then change compare_pending_checksums to build NetplayError::Desync entirely from it when is_desynced() becomes true; remove reliance on first_mismatch and last(), while preserving the existing error behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/rustysnes-netplay/src/session.rs`:
- Around line 496-554: Update DesyncDiagnostics::record to persist the local and
remote hashes associated with the first frame that establishes
first_desync_frame, retaining them thereafter. Expose that sticky
first-divergence payload through the diagnostics API, then change
compare_pending_checksums to build NetplayError::Desync entirely from it when
is_desynced() becomes true; remove reliance on first_mismatch and last(), while
preserving the existing error behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0f74bd1c-4344-40b2-94cf-ce52ea94ddbc
📒 Files selected for processing (6)
CHANGELOG.mdcrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/lib.rscrates/rustysnes-netplay/src/session.rscrates/rustysnes-netplay/tests/desync_hysteresis.rsdocs/netplay.md
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: test-light
- GitHub Check: lint
- GitHub Check: accuracysnes
- GitHub Check: build demo + docs
🧰 Additional context used
📓 Path-based instructions (11)
docs/**/*.md
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Before changing a subsystem, consult
docs/architecture.md,docs/STATUS.md,CONTRIBUTING.md, the relevant subsystem documentation, and applicable ADRs.New subsystems must add documentation under
docs/.
Files:
docs/netplay.md
**/*.{rs,md}
📄 CodeRabbit inference engine (CONTRIBUTING.md)
Chip-behavior changes must update both the chip implementation and the corresponding
docs/<subsystem>.mddocumentation.A chip change must update both the chip implementation and its corresponding
docs/<chip>.mddocumentation in the same change.
Files:
docs/netplay.mdcrates/rustysnes-netplay/src/lib.rsCHANGELOG.mdcrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
**/*
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*: Do not commit or vendor the generatedsnesdev_wiki/mirror; it is gitignored and intended only as a local reference.
Keep commits focused and use Conventional Commits:<type>(<scope>): <subject>, with an imperative subject of at most 72 characters.
Do not use emojis in code, comments, or commit messages.
Before opening a PR, ensure formatting, Clippy, workspace tests, the core embedded build, rustdoc with warnings denied, documentation coverage, and changelog requirements pass.
Ticket completion must be reflected in the relevantto-dos/sprint file.
**/*: Preserve the one-directional crate graph: chip crates must not depend on one another;rustysnes-coreties them together.
Never commit commercial ROMs; only commit derived screenshots and hashes.
Keepdocs/STATUS.mdas the authoritative per-subsystem status and update project documentation in the same PR as code changes.
Do not treat RustyNESv2.0orengine-lineageanchors as project releases.
Files:
docs/netplay.mdcrates/rustysnes-netplay/src/lib.rsCHANGELOG.mdcrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
docs/**
⚙️ CodeRabbit configuration file
docs/**: Docs are the spec, not a history log. Flag claims that contradict the code, counts that
contradict the generateddocs/accuracysnes-coverage.md, and any statement of coverage that
is broader than what the corresponding test actually asserts.
Files:
docs/netplay.md
**/*.md
⚙️ CodeRabbit configuration file
**/*.md: Docs are the spec, not a changelog. Flag prose that has drifted from the code it describes
rather than style nits. The markdownlint gate is pinned to v0.39.0 via pre-commit —
do not report rules that version does not have (MD060 in particular).
Files:
docs/netplay.mdCHANGELOG.md
crates/**/*.rs
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
crates/**/*.rs: Preserve the master-clock lockstep timing model.
rustysnes-core::Busowns mutable machine state, and the CPU borrows&mut Bus.
Preserve determinism: seed, ROM, and input must produce bit-identical output.
Treat test ROMs as the behavioral specification; when documentation disagrees with passing ROM behavior, update the documentation.
Keepunsafeconfined to existing allowed areas, namely frontend and FFI code, and document everyunsafeblock with a// SAFETY:comment.
Files:
crates/rustysnes-netplay/src/lib.rscrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
**/*.rs
📄 CodeRabbit inference engine (CONTRIBUTING.md)
**/*.rs: Use Rust edition 2024 and the toolchain pinned inrust-toolchain.toml(Rust 1.96).
Runcargo fmt --all --check; Rust code must remain rustfmt-compliant.
Run Clippy withcargo clippy --workspace --all-targets -- -D warnings; warnings must not remain.
New public Rust items must have rustdoc becausemissing_docsis a workspace lint.
Do not runcargo clippy --all-features;scriptingandscript-wasmare mutually exclusive. Use explicit per-feature jobs instead.
**/*.rs: Do not introduce.unwrap(),.expect(), orpanic!()on untrusted external input—such as ROM/save-state bytes, netplay messages, Lua or scripting input, or user-supplied paths—outside#[cfg(test)]code. Use typed errors at those boundaries; locally constructed values or values immediately protected by a checked invariant are allowed.
Every newunsafe { ... }block orunsafe fnmust have an adjacent// SAFETY:comment naming the relied-on invariant and its guarantor. Unsafe code outside the frontend and FFI shims should additionally be questioned becauseunsafe_codeis a workspace lint.
**/*.rs: Use Rust edition 2024 with the pinned 1.96 toolchain; satisfy workspacepedantic,nursery,missing_docs, andunsafe_codewarnings because CI runs with-D warnings. Document every public item.
Keepunsafecode restricted to the frontend and FFI, and include a// SAFETY:justification for each use.
Keep hot paths allocation-free.
Treatrustysnes_core::Busas the owner of mutable emulator state; the CPU borrows&mut Bus.
Use the master clock at 21477270 Hz as the timing master; advance the scheduler in lockstep and run other chips on their divisors.
Maintain determinism: seed, ROM, and input must produce bit-identical audio/video; frontend rate control must not alter emulation results.
When implementing hardware behavior, pin and run the failing test ROM first; treat test ROMs as the specification.
Files:
crates/rustysnes-netplay/src/lib.rscrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
crates/rustysnes-*/**/*
📄 CodeRabbit inference engine (Custom checks)
For the full pull request diff against its base branch, any observable behavior change under
crates/rustysnes-<chip>/must be accompanied by an edit to the matchingdocs/<chip>.md; a crate change passes without documentation only when it does not alter observable behavior, with the non-behavioral change stated explicitly.
Files:
crates/rustysnes-netplay/src/lib.rscrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
**/*.{rs,toml}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{rs,toml}: Additive features must be default-off so shipped/native,no_std, and wasm builds remain byte-identical.
Never use or configure--all-features; validate opt-in feature combinations individually as required by the project recipe.
Files:
crates/rustysnes-netplay/src/lib.rscrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
crates/**
⚙️ CodeRabbit configuration file
crates/**: Emulator core. Hot paths are allocation-free;unsaferequires a// SAFETY:comment
naming the invariant. Any change to save-stated fields needs aFORMAT_VERSIONbump and a
docs/adr/0006bump-log entry. Behavior changes must update the matchingdocs/<chip>.md
in the same change.
Files:
crates/rustysnes-netplay/src/lib.rscrates/rustysnes-netplay/tests/desync_hysteresis.rscrates/rustysnes-netplay/src/diagnostics.rscrates/rustysnes-netplay/src/session.rs
CHANGELOG.md
📄 CodeRabbit inference engine (CONTRIBUTING.md)
User-visible changes must be recorded under the
[Unreleased]section.For the full pull request diff against its base branch, modify
CHANGELOG.mdwhen user-visible behavior changes, including emulator output, frontend features, CLI flags, public APIs, or AccuracySNES cartridge contents. Do not require it for purely internal changes, tests, comments, or CI configuration.
Files:
CHANGELOG.md
🔇 Additional comments (6)
crates/rustysnes-netplay/src/diagnostics.rs (1)
1-461: LGTM!crates/rustysnes-netplay/src/lib.rs (1)
17-17: LGTM!Also applies to: 28-28
crates/rustysnes-netplay/src/session.rs (1)
27-27: LGTM!Also applies to: 176-182, 212-224
crates/rustysnes-netplay/tests/desync_hysteresis.rs (1)
137-165: LGTM!Once the
NetplayError::Desyncpayload consistency fix fromsession.rslands, consider assertingframe == 0(the true first poisoned frame) here to pin the corrected behavior — the current assertion only checks the error variant, not its payload, which is why the inconsistency described in thesession.rsreview comment oncompare_pending_checksumswas not caught by this test.docs/netplay.md (1)
28-88: LGTM!CHANGELOG.md (1)
14-43: LGTM!
Review found two real defects in the graded-verdict PR. Both are mine.
## 1. The run counted consecutive RECORDS, not consecutive FRAMES
`consecutive_mismatches` incremented in the order `record()` was called,
with no frame-continuity check. On a lossy link the checksums in between
may never arrive to be compared, so three isolated transients seconds
apart would be recorded back-to-back and confirm a desync that never
happened.
It also contradicted the threshold's own stated rationale, which is
expressed in TIME ("~1.5 s at the default interval") — a claim the
implementation did not actually make true.
A run now continues only if the mismatch is within `max_run_gap_frames`
of the previous comparison, derived from the session's own
`checksum_interval` via `gap_for_interval` (two intervals). That
tolerates a single lost checksum inside a genuine run — a real desync
mismatches EVERY checksum, so its members are one interval apart — while
refusing to assemble a run out of widely separated transients.
Both directions are pinned, because a gap check that is too tight is its
own bug: `widely_separated_transients_do_not_stack_into_a_false_desync`
(three mismatches minutes apart stay unconfirmed, and it asserts all
three were actually recorded so it cannot pass vacuously) and
`a_real_run_survives_one_lost_checksum` (a 60-frame gap inside a real run
still confirms). Verified by disabling the continuity check and
confirming the first fails while the second still passes.
## 2. The error payload could describe two different frames
`NetplayError::Desync` was built from whatever this pass saw, falling
back — when the confirming pass held no mismatch of its own, which
happens whenever the run was built across earlier passes — to
`first_desync_frame` paired with the hashes from `diagnostics.last()`.
Those are different comparisons. The result was an error naming one
frame and quoting another frame's hashes, which can even be a frame that
MATCHED: precise-looking and wrong.
`DesyncDiagnostics` now retains the first diverging `CrcCompare` whole,
and the session reports that one record. The frame is also the right one
to report on its own merits: the divergence's start is where a bisect
begins, not where a counter crossed a threshold.
`a_sustained_divergence_still_fails_the_session` now destructures the
error and asserts frame + both hashes come from `first_desync()`, that
the two hashes actually differ, and that later comparisons exist — so
reporting the first is a genuine choice rather than a coincidence of a
one-comparison run.
## Also
`compare_pending_checksums` no longer `.unwrap()`s the `VecDeque::remove`
(project rule on the untrusted-network path, regardless of the local
`position()` reasoning making it infallible today).
Declined: `VecDeque::with_capacity(CAPACITY)` for the history ring —
`with_threshold_and_gap` is `const fn` so `RollbackSession::new` can stay
one, and `with_capacity` is not const. The cost is a handful of
reallocations once per session, against losing `const` on a public
constructor.
Verified: fmt, clippy `-D warnings`, `cargo doc` warnings-as-errors, 32
netplay tests green (25 lib + 3 integration + 4 determinism).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Review adjudicationBoth blocking findings were real, and both were mine. Fixed in Blocking 1 — the run counted consecutive records, not consecutive frames ✅ fixedCorrect, and it also meant the code did not make true the claim its own doc makes. The threshold's rationale is stated in time ("~1.5 s at the default interval"), but A run now continues only if the mismatch is within Both directions are pinned, because a gap check that is too tight is its own bug — it would let a real desync go unconfirmed on any lossy link:
Verified by disabling the continuity check: the first fails, the second still passes. Blocking 2 — the error payload could describe two different frames ✅ fixedAlso correct, and the sharper version of it is that the payload could quote the hashes of a frame that matched. When the confirming pass held no mismatch of its own — which happens whenever the run was built across earlier passes — the fallback paired
Suggestion — avoid
|
Antigravity review (Gemini via Ultra)This PR replaces immediate session teardown on the first netplay checksum mismatch with a hysteresis-based diagnostic tracker ( Blocking issuesNone found. Suggestions
Nitpicks
Automated first-pass review by |
1 participant
First PR of the
v1.27.0netplay-depth rung. Client-side depth only, per the scoping decision — no signalling server, no lobby.The behaviour that changed
Until now the first checksum mismatch was a fatal
NetplayError::Desync, andapp.rsdisconnected on it. That's too eager: a burst-reordered pair ofChecksummessages can momentarily disagree before the deferredcompare_pending_checksumspass reconciles them — so a transient network event ended a healthy game.diagnostics::DesyncDiagnosticsnow records every comparison (matching ones included) and folds them into one graded verdict:InSyncSuspect { consecutive, first_desync_frame }Desynced { first_desync_frame }The fatal error fires only on
Desynced. A transient is survived; a genuine divergence still ends the session — which it must, since a rollback desync can't recover without a full state resync.Desyncedis sticky. A later stray match resets the live run but never downgrades the verdict, because the condition cannot actually heal. A surface that flapped between "desynced" and "fine" would train the user to ignore it.The framebuffer hash was being thrown away
compare_pending_checksumsdestructured the remote report aslet (_, remote_hash, _) = ..., discarding the framebuffer-only hash the message already carried. It now reaches the diagnostics, and it's what makes a mismatch diagnosable rather than merely alarming:Determinism is untouched
The module only reads values the session already computed and stores copies. It never feeds back into rollback, the checksum exchange, or the emulator, and it holds no wall-clock — that belongs in the connection layer, a later PR in this rung. Deleting it would leave every frame, checksum, and rollback byte-identical.
tests/determinism.rspasses unchanged.DesyncDiagnostics::new/with_thresholdareconst fnsoRollbackSession::newkeeps being one — which is why the threshold clamp is spelled out rather than.max(1)(Ord::maxisn't const).Tests
9 unit tests cover the verdict arithmetic. New
tests/desync_hysteresis.rscovers what actually changed, driving a real session against forged peer checksums:InSync.The transient test carries an anti-vacuous guard: surviving is only meaningful if the forged checksum actually reached the comparator, so it asserts
total == 1andmismatches == 1rather than merely that no error came back. I probed the real values before writing the assertions rather than assuming them.Both were verified by re-injecting the old fail-on-first behaviour and confirming they fail — the transient one with exactly the
Err(Desync { frame: 0, .. })this PR removes.Verification
cargo fmt --all --check; clippy-D warningson the crate and onrustysnes-frontend --features netplay;cargo docwith warnings-as-errors.docs/netplay.md— the subsystem had no doc; later PRs in this rung extend it.🤖 Generated with Claude Code
Summary
Netplay now tolerates isolated checksum mismatches and reports a fatal desync only after three consecutive mismatches. The observable behavior changes from immediate disconnect to graded
InSync,Suspect, and stickyDesyncedstatuses.The change claims that transient reordered
Checksummessages no longer end healthy sessions, while sustained divergence still raisesNetplayError::Desync. The claim is false if one mismatch still disconnects a session, three consecutive mismatches do not disconnect it, or framebuffer and full-state differences are classified incorrectly.DesyncDiagnosticspreserves bounded comparison history and exposes mismatch statistics without affecting rollback or emulator determinism. New unit and integration coverage exercises verdict transitions, history behavior, framebuffer classification, and session termination.