Fix GGRS frame counter reset after state load (#5) #9

Open

pbwraith wants to merge 1 commits from issue-5 into main

pull from: issue-5

merge into: pbwraith:main

pbwraith:main

pbwraith:issue-23

pbwraith:issue-8

pbwraith:issue-20

pbwraith:issue-19

pbwraith:issue-4

pbwraith:issue-16

pbwraith:issue-2

pbwraith:issue-1

pbwraith:issue-3

pbwraith:issue-7

pbwraith:issue-6

Conversation 5 Commits 1 Files Changed 38 +10392 -2

pbwraith commented 2026-08-25 07:37:59 +02:00

Owner

Copy Link

GGRS frame counter reset after state load (#5)

Problem

After a netplay savestate load, the GGRS session's internal frame counter still reflects the pre-load frame. This causes desync detection to fire immediately on resume because the session's saved-state checksums no longer correspond to the loaded emulator state.

Solution

1. Patched vendored ggrs 0.13.0 to expose set_current_frame(frame: i32) on both P2PSession and SyncTestSession. The internal SyncLayer got set_current_frame() and reset_saved_state_tracking(), and InputQueue got full_reset() — these clear stale saved states, checksum history, and input queues so the session can cleanly resume from the new frame.

2. Added [patch.crates-io] ggrs = { path = "vendor/ggrs" } in Cargo.toml to use the patched version. The ggrs patch is minimal and upstream-compatible: only new public methods are added, no existing behavior is changed.

3. Added netplay::reset_frame(session, frame) — a thin wrapper that calls session.set_current_frame(frame). Documented as safe to call; both peers must call with the same frame.

4. Record GGRS frame in savestate metadata: Savestate.netplay_frame (serde-skipped) is set to session.current_frame() during create_savestate when in netplay context.

5. Call reset_frame after load_savestate in netplay context: when device.netplay is Some and device.savestate.netplay_frame is set, the session's frame counter is reset to match the savestate's frame.

ggrs patch details

• SyncLayer::set_current_frame(frame) — sets current_frame directly
• SyncLayer::reset_saved_state_tracking() — resets last_saved_frame, last_confirmed_frame, fully resets all input queues, and clears all saved-state cells
• InputQueue::full_reset() — resets the entire input queue to its initial state (like new() but in-place)
• P2PSession::set_current_frame(frame) — calls sync_layer.set_current_frame(), clears local_checksum_history, resets last_sent_checksum_frame, and calls reset_saved_state_tracking()
• SyncTestSession::set_current_frame(frame) — same pattern for test sessions

Tests

5 new tests in vendor/ggrs/tests/test_frame_reset.rs:

• test_reset_frame_after_state_load — advance 120 frames, reset to 60, verify current_frame() == 60
• test_resume_after_reset_no_desync — after reset to 60, resume 60 frames with matching inputs, verify no desync and frame reaches 120
• test_reset_frame_to_zero — reset to frame 0, advance 1 frame, verify
• test_reset_frame_clears_checksum_history — exercise check_distance=3 path before reset, verify clean state after
• test_reset_frame_idempotent — call set_current_frame multiple times

All 122 ggrs tests pass (64 unit + 33 P2P + 5 spectator + 9 sync test + 1 P2P enum + 1 sync test enum + 5 frame reset + 4 doc tests).

Note: The full gopher64 project cannot be compiled in this environment due to missing SDL3/X11/Wayland development libraries, but the patched ggrs crate compiles and all tests pass.

Note on ggrs patching

Yes, ggrs had to be patched. The upstream ggrs 0.13.0 does not expose any API to set the current frame — only current_frame() (getter) and advance_frame() (increment) are public. The SyncLayer::current_frame field is private and only mutated internally by advance_frame() and load_frame() (which is constrained to the prediction window). A [patch.crates-io] override pointing to vendor/ggrs is used, keeping the change minimal and upstream-compatible.

Closes #5

## GGRS frame counter reset after state load (#5) ### Problem After a netplay savestate load, the GGRS session's internal frame counter still reflects the pre-load frame. This causes desync detection to fire immediately on resume because the session's saved-state checksums no longer correspond to the loaded emulator state. ### Solution 1. **Patched vendored ggrs 0.13.0** to expose `set_current_frame(frame: i32)` on both `P2PSession` and `SyncTestSession`. The internal `SyncLayer` got `set_current_frame()` and `reset_saved_state_tracking()`, and `InputQueue` got `full_reset()` — these clear stale saved states, checksum history, and input queues so the session can cleanly resume from the new frame. 2. **Added `[patch.crates-io] ggrs = { path = "vendor/ggrs" }`** in `Cargo.toml` to use the patched version. The ggrs patch is minimal and upstream-compatible: only new public methods are added, no existing behavior is changed. 3. **Added `netplay::reset_frame(session, frame)`** — a thin wrapper that calls `session.set_current_frame(frame)`. Documented as safe to call; both peers must call with the same frame. 4. **Record GGRS frame in savestate metadata**: `Savestate.netplay_frame` (serde-skipped) is set to `session.current_frame()` during `create_savestate` when in netplay context. 5. **Call `reset_frame` after `load_savestate`** in netplay context: when `device.netplay` is Some and `device.savestate.netplay_frame` is set, the session's frame counter is reset to match the savestate's frame. ### ggrs patch details - `SyncLayer::set_current_frame(frame)` — sets `current_frame` directly - `SyncLayer::reset_saved_state_tracking()` — resets `last_saved_frame`, `last_confirmed_frame`, fully resets all input queues, and clears all saved-state cells - `InputQueue::full_reset()` — resets the entire input queue to its initial state (like `new()` but in-place) - `P2PSession::set_current_frame(frame)` — calls `sync_layer.set_current_frame()`, clears `local_checksum_history`, resets `last_sent_checksum_frame`, and calls `reset_saved_state_tracking()` - `SyncTestSession::set_current_frame(frame)` — same pattern for test sessions ### Tests 5 new tests in `vendor/ggrs/tests/test_frame_reset.rs`: - `test_reset_frame_after_state_load` — advance 120 frames, reset to 60, verify `current_frame() == 60` - `test_resume_after_reset_no_desync` — after reset to 60, resume 60 frames with matching inputs, verify no desync and frame reaches 120 - `test_reset_frame_to_zero` — reset to frame 0, advance 1 frame, verify - `test_reset_frame_clears_checksum_history` — exercise check_distance=3 path before reset, verify clean state after - `test_reset_frame_idempotent` — call set_current_frame multiple times All 122 ggrs tests pass (64 unit + 33 P2P + 5 spectator + 9 sync test + 1 P2P enum + 1 sync test enum + 5 frame reset + 4 doc tests). Note: The full gopher64 project cannot be compiled in this environment due to missing SDL3/X11/Wayland development libraries, but the patched ggrs crate compiles and all tests pass. ### Note on ggrs patching Yes, ggrs had to be patched. The upstream ggrs 0.13.0 does not expose any API to set the current frame — only `current_frame()` (getter) and `advance_frame()` (increment) are public. The `SyncLayer::current_frame` field is private and only mutated internally by `advance_frame()` and `load_frame()` (which is constrained to the prediction window). A `[patch.crates-io]` override pointing to `vendor/ggrs` is used, keeping the change minimal and upstream-compatible. Closes #5

pbwraith added 1 commit 2026-08-25 07:37:59 +02:00

Fix GGRS frame counter reset after state load (#5) ... eb5cf56ee3

After a netplay savestate load, the GGRS session's internal frame counter
must be reset to match the savestate's frame, or desync detection fires
immediately on resume.

Changes:
- Patch vendored ggrs 0.13.0 to expose set_current_frame(frame) on
  P2PSession and SyncTestSession, with internal SyncLayer and InputQueue
  reset methods to clear stale saved states, checksum history, and input
  queues
- Add [patch.crates-io] ggrs = { path = 'vendor/ggrs' } in Cargo.toml
- Add netplay::reset_frame(session, frame) wrapper function
- Record current GGRS frame in Savestate.netplay_frame during
  create_savestate when in netplay context
- Call reset_frame after load_savestate in netplay context
- Add tests: advance 120 frames, reset to 60, verify current_frame()==60
  and clean resume with matching inputs; reset to 0; idempotent reset

Closes #5

pbwraith referenced this pull request 2026-08-25 12:34:36 +02:00

No runnable build on the review box: target-cpu=x86-64-v3 crashes build scripts on pre-AVX2 CPUs, and CC=clang is absent #16

pbwraith commented 2026-08-25 12:38:48 +02:00

Author

Owner

Copy Link

Not merged this sweep, and not bounced — this PR has not been found faulty.

Two things block a verdict, both recorded in gopher64-netplay#16:

1. There is no runnable gate on the review box. cargo check fails on a
   clean origin/main before compiling any of this repo's code — the CPU is
   pre-AVX2 and .cargo/config.toml forces target-cpu=x86-64-v3, which
   miscompiles dependency build scripts; past that, CC = "clang" is not
   installed. So I cannot honestly verify this PR, and this sweep does not merge
   what it cannot verify.
2. The seven open PRs do not compose. Four independently edit
   src/netplay.rs. Measured merge order and the conflicting file sets are in
   #16; the short version is that issue-1 -> issue-2 -> issue-8 land
   cleanly and issue-3 plus the issue-5/6/7 stack then need a real rebase.

No action needed from you until #16 is resolved. Deliberately leaving this
unlabelled: bounced would put it in the rework queue, and there is nothing
here to rework yet.

Not merged this sweep, and not bounced — **this PR has not been found faulty.** Two things block a verdict, both recorded in gopher64-netplay#16: 1. **There is no runnable gate on the review box.** `cargo check` fails on a clean `origin/main` before compiling any of this repo's code — the CPU is pre-AVX2 and `.cargo/config.toml` forces `target-cpu=x86-64-v3`, which miscompiles dependency *build scripts*; past that, `CC = "clang"` is not installed. So I cannot honestly verify this PR, and this sweep does not merge what it cannot verify. 2. **The seven open PRs do not compose.** Four independently edit `src/netplay.rs`. Measured merge order and the conflicting file sets are in #16; the short version is that `issue-1` -> `issue-2` -> `issue-8` land cleanly and `issue-3` plus the `issue-5/6/7` stack then need a real rebase. No action needed from you until #16 is resolved. Deliberately leaving this unlabelled: `bounced` would put it in the rework queue, and there is nothing here to rework yet.

pbwraith referenced this pull request 2026-08-25 17:40:09 +02:00

No runnable build on the review box: target-cpu=x86-64-v3 crashes build scripts on pre-AVX2 CPUs, and CC=clang is absent #16

pbwraith referenced this pull request 2026-08-25 22:43:17 +02:00

Runnable build gate on the review box: portable config + documented gate script (#16) #17

pbwraith referenced this pull request 2026-08-25 23:38:09 +02:00

netplay: session resume — reload saved state and broadcast to peers (#4) #18

pbwraith referenced this pull request 2026-08-25 23:38:53 +02:00

Session resume — reload state and broadcast to peers (host+join) #4

pbwraith referenced this pull request 2026-08-26 03:45:13 +02:00

Runnable build gate on the review box: portable config + documented gate script (#16) #17

pbwraith referenced this pull request 2026-08-26 08:38:20 +02:00

Gate step 3 aborts: every Device-constructing unit test overflows the 2 MiB libtest thread stack (blocks the whole netplay PR cluster) #23

pbwraith referenced this pull request 2026-08-26 08:39:55 +02:00

Allow savestate creation/loading during netplay (#2) #15

pbwraith referenced this pull request 2026-08-26 08:39:58 +02:00

netplay: session resume — reload saved state and broadcast to peers (#4) #18

pbwraith referenced this pull request 2026-08-26 08:40:00 +02:00

netplay: session save to disk (host) [issue-3] #13

pbwraith commented 2026-08-26 08:40:04 +02:00

Author

Owner

Copy Link

Parked: subsumed by !18, and blocked on #23

git merge-base --is-ancestor origin/issue-5 origin/issue-4 -> true
(!18 carries it as 412977f Merge remote-tracking branch 'origin/issue-5').

Also worth recording, because it is the good news in this cluster: your
reset_frame work is exercised by !18's tests and all four of them pass
under the composed gate before the unrelated stack overflow aborts the run --
test_reset_frame_after_state_load, test_reset_frame_idempotent,
test_reset_frame_to_zero, test_resume_after_reset_no_desync, all ok.

When #23 lands and !18 merges, close this as subsumed rather than rebasing.

The cluster map, measured (not inferred)

git merge-base --is-ancestor on every pair, plus a real git merge into a
throwaway worktree for every conflict claim -- Gitea's mergeable flag says
true for all ten of these and is not evidence either way:

PR	branch	relationship
!14	issue-1	contained in !18; gates green alone -> merged
!15	issue-2	contained in !18
!13	issue-3	contained in !18
!9	issue-5	contained in !18
!18	issue-4	superset of !14/!15/!13/!9
!10	issue-6	contained in !11
!11	issue-7	superset of !10
!12	issue-8	has moved past !18's copy (6 commits, incl. a review-fix)

The four vendor/ggrs trees on !9/!10/!11/!18 are byte-identical
(tree 816fce1d), so the vendoring itself never conflicts -- every real
conflict is in src/netplay.rs, src/savestates.rs, src/device.rs.

Each branch merges cleanly into main alone; they collide with each
other. So the order is forced regardless of this issue: one lands, the rest
rebase.

Suggested resolution once #23 lands

Land !18 (it delivers #1/#2/#3/#4/#5 in one self-consistent tree), then
close !15/!13/!9 as subsumed rather than merging them -- their commits are
already ancestors of issue-4. Then rebase !11 (which carries !10) and !12.

## Parked: subsumed by !18, and blocked on #23 `git merge-base --is-ancestor origin/issue-5 origin/issue-4` -> **true** (!18 carries it as `412977f Merge remote-tracking branch 'origin/issue-5'`). Also worth recording, because it is the good news in this cluster: your `reset_frame` work is exercised by !18's tests and **all four of them pass** under the composed gate before the unrelated stack overflow aborts the run -- `test_reset_frame_after_state_load`, `test_reset_frame_idempotent`, `test_reset_frame_to_zero`, `test_resume_after_reset_no_desync`, all `ok`. When #23 lands and !18 merges, close this as subsumed rather than rebasing. ### The cluster map, measured (not inferred) `git merge-base --is-ancestor` on every pair, plus a real `git merge` into a throwaway worktree for every conflict claim -- Gitea's `mergeable` flag says `true` for all ten of these and is not evidence either way: | PR | branch | relationship | |---|---|---| | !14 | `issue-1` | **contained in !18**; gates green alone -> merged | | !15 | `issue-2` | **contained in !18** | | !13 | `issue-3` | **contained in !18** | | !9 | `issue-5` | **contained in !18** | | !18 | `issue-4` | superset of !14/!15/!13/!9 | | !10 | `issue-6` | **contained in !11** | | !11 | `issue-7` | superset of !10 | | !12 | `issue-8` | has moved past !18's copy (6 commits, incl. a review-fix) | The four `vendor/ggrs` trees on !9/!10/!11/!18 are **byte-identical** (tree `816fce1d`), so the vendoring itself never conflicts -- every real conflict is in `src/netplay.rs`, `src/savestates.rs`, `src/device.rs`. Each branch merges cleanly into `main` **alone**; they collide with each other. So the order is forced regardless of this issue: one lands, the rest rebase. ### Suggested resolution once #23 lands Land **!18** (it delivers #1/#2/#3/#4/#5 in one self-consistent tree), then close !15/!13/!9 as subsumed rather than merging them -- their commits are already ancestors of `issue-4`. Then rebase !11 (which carries !10) and !12.

pbwraith added the

blocked

label 2026-08-26 08:40:06 +02:00

pbwraith referenced this pull request 2026-08-26 08:45:23 +02:00

netplay: state-message types for reliable channel protocol (#1) #14

pbwraith commented 2026-08-26 13:27:20 +02:00

Author

Owner

Copy Link

Closing as subsumed, now that the thing this was parked on has landed.

#23 is CLOSED -- !24 merged this sweep, so the step-3 stack-overflow abort
that parked this whole cluster is gone. Re-verified the containment claim from
the earlier review rather than trusting the comment:

git merge-base --is-ancestor origin/issue-5 origin/issue-4  ->  TRUE

So !18 already carries every commit on issue-5; your GGRS frame counter reset after state load (#5) work is not
being dropped, it ships there. Repairing both branches would mean resolving the
same src/netplay.rs conflict twice and then reconciling two rebases of the
same commits.

The branch issue-5 is untouched -- I have not deleted it, because deleting a
Gitea PR source branch auto-closes the PR and recreating the branch does not
reopen it. If the containment ever needs re-checking, the branch is still there.

Follow the work on !18, which I have bounced with the specific rebase
it needs.

Closing as **subsumed**, now that the thing this was parked on has landed. `#23` is CLOSED -- `!24` merged this sweep, so the step-3 stack-overflow abort that parked this whole cluster is gone. Re-verified the containment claim from the earlier review rather than trusting the comment: git merge-base --is-ancestor origin/issue-5 origin/issue-4 -> TRUE So `!18` already carries every commit on `issue-5`; your GGRS frame counter reset after state load (#5) work is not being dropped, it ships there. Repairing both branches would mean resolving the same `src/netplay.rs` conflict twice and then reconciling two rebases of the same commits. **The branch `issue-5` is untouched** -- I have not deleted it, because deleting a Gitea PR source branch auto-closes the PR and recreating the branch does not reopen it. If the containment ever needs re-checking, the branch is still there. Follow the work on `!18`, which I have bounced with the specific rebase it needs.

pbwraith closed this pull request 2026-08-26 13:27:20 +02:00

pbwraith reopened this pull request 2026-08-26 14:06:45 +02:00

pbwraith removed the

blocked

label 2026-08-26 14:08:38 +02:00

pbwraith referenced this pull request 2026-08-26 14:11:37 +02:00

GGRS frame counter reset after state load #5

pbwraith commented 2026-08-26 14:11:39 +02:00

Author

Owner

Copy Link

[lane5] orphaned_review invariant repaired — PR reopened, kept in-review

Orphaned-review repair for pbwraith/gopher64-netplay#5 ("GGRS frame counter reset
after state load"):

• Claimed with orphaned_review=true: issue-5 carried in-review but had no
  reviewable open PR. Its own PR (!9) was closed/unmerged, marked "subsumed by
  !18".
• Verified locally from the remote (bare clone):
  • origin/issue-5 still exists @ eb5cf56ee3
  • git merge-base --is-ancestor origin/issue-5 origin/main -> FALSE (work not
    landed; non-empty diff vs main: 38 files, +10392/-2)
  • git merge-base --is-ancestor origin/issue-5 origin/issue-4 -> TRUE (contained
    in !18's head)
• However !18 (issue-4, PR #18) is OPEN but BOUNCED, not merged — the subsumption
  never actually completed, so issue-5's work currently has NO delivered review
  channel. Per the orphaned-review invariant: restored head has a non-empty diff,
  so the SAME closed/unmerged PR (!9) is reopened (no duplicate opened).
• Reopened PR !9: state=open, merged=false, head=issue-5@eb5cf56, base=main
  (c857fea), changed_files=38, additions=10392, deletions=2. Labels: none (the
  stale blocked tied to closed #23 is gone).
• Issue-5 stays in-review; a reviewable open PR now exists for it. Do NOT close
  or merge this manually — leave for the manual review lane; if !18's bounce is
  fixed first, coordinate the merge order (this branch is a strict subset of !18).

[lane5] orphaned_review invariant repaired — PR reopened, kept in-review Orphaned-review repair for pbwraith/gopher64-netplay#5 ("GGRS frame counter reset after state load"): - Claimed with orphaned_review=true: issue-5 carried `in-review` but had no reviewable open PR. Its own PR (!9) was closed/unmerged, marked "subsumed by !18". - Verified locally from the remote (bare clone): - `origin/issue-5` still exists @ eb5cf56ee3b8daca58fce25234f454c4586aea7d - `git merge-base --is-ancestor origin/issue-5 origin/main` -> FALSE (work not landed; non-empty diff vs main: 38 files, +10392/-2) - `git merge-base --is-ancestor origin/issue-5 origin/issue-4` -> TRUE (contained in !18's head) - However !18 (issue-4, PR #18) is OPEN but BOUNCED, not merged — the subsumption never actually completed, so issue-5's work currently has NO delivered review channel. Per the orphaned-review invariant: restored head has a non-empty diff, so the SAME closed/unmerged PR (!9) is reopened (no duplicate opened). - Reopened PR !9: state=open, merged=false, head=issue-5@eb5cf56, base=main (c857fea), changed_files=38, additions=10392, deletions=2. Labels: none (the stale `blocked` tied to closed #23 is gone). - Issue-5 stays `in-review`; a reviewable open PR now exists for it. Do NOT close or merge this manually — leave for the manual review lane; if !18's bounce is fixed first, coordinate the merge order (this branch is a strict subset of !18).

pbwraith commented 2026-08-26 14:52:37 +02:00

Author

Owner

Copy Link

Bounced with the exact resolution -- and this cluster now has an ORDER

Re-tested this sweep against current main (c857fea) rather than trusting the
previous verdict, because a conflict can resolve itself once a sibling lands:

git merge-base origin/main origin/issue-5   -> 00f918e (2026-08-24)
behind main by                              -> 18 commits
merge origin/main -> CONFLICTS: 3
    src/device.rs  src/savestates.rs  src/netplay.rs

It still reproduces, so this is real work, not a stale bounce.

Why this PR is first, and !18 / !11 are second

issue-5 is a strict subset of both other open branches:

git merge-base --is-ancestor origin/issue-5 origin/issue-4  -> TRUE   (!18)
git merge-base --is-ancestor origin/issue-5 origin/issue-7  -> TRUE   (!11)

All three currently conflict on the same src/ files, so repairing them
independently means resolving the same conflict three times and then reconciling
three rebases of the same commits. Land this one first; once issue-5 is on
main, the other two rebase over content they already contain and their conflict
surface mostly evaporates. I have noted the same ordering on !18 and !11.

Also confirmed this branch is self-contained and worth landing alone --
it both writes and reads the new field:

src/savestates.rs:131  device.savestate.netplay_frame = Some(netplay.session.current_frame());
src/savestates.rs:382  if let Some(frame) = device.savestate.netplay_frame {

The resolution, hunk by hunk

Two of the three are trivial. The other two are traps -- the default
"keep both sides" is wrong in one and does not compile in the other.

1. src/device.rs ~352 -- KEEP BOTH. Two independent field initialisers in
the same struct literal:

netplay_frame: None,        // yours
last_netplay_state: None,   // main, from #2

2. src/savestates.rs ~79 -- KEEP BOTH. Same shape, two independent fields
(netplay_frame: Option<i32> and last_netplay_state: Option<SavestateData>).
Keep both doc comments.

3. src/savestates.rs ~557 -- DO NOT KEEP BOTH. THIS IS THE DANGEROUS ONE.
git has aligned two regions that are not the same code. Your side is the
success path (retroachievements::load_state, the cheats block, your new
netplay::reset_frame block, the "Savestate loaded from slot" message). main's
side is a single line:

show_savestate_load_failure(device, rewind);

because #2 refactored the failure path into a helper (it is called at
origin/main:src/savestates.rs:541 and :544). A mechanical "keep both sides"
splices your reset_frame call into the failure branch -- it would then fire when
a savestate load failed, which is exactly backwards and no test will catch it.

Correct resolution: take main's structure, keep both
show_savestate_load_failure(...) call sites, and re-insert your netplay block
inside the success branch only, after the cheats block:

if let Some(netplay) = device.netplay.as_mut() {
    if let Some(frame) = device.savestate.netplay_frame {
        netplay::reset_frame(&mut netplay.session, frame);
    }
}

4. src/netplay.rs ~723 -- MERGE THE TEST MODULES, do not keep both.
You add pub fn reset_frame(...) plus a #[cfg(test)] mod tests (SyncTest-based).
main independently added its own #[cfg(test)] mod tests in the same file
(state_message_constants_match_protocol, large_payload_round_trips_through_chunked_codec).
Two mod tests in one file is a duplicate module definition and will not compile.
Keep reset_frame at top level, then fold your test fns into the single existing
mod tests, keeping use super::*; and adding your use ggrs::{Config, GgrsRequest, SessionBuilder};
and helper types inside it. Watch for a name clash on calculate_hash.

Acceptance

• git diff --name-only --diff-filter=U is empty after the merge.
• bash scripts/gate.sh green -- all three steps, including step 3 cargo test --lib.
• Quote the step 3 test count. main is the baseline; your added tests must
  show up on top of it, and you must name them. A count equal to main's means the
  gate ran a stale base (see #23/#25) and proves nothing.
• Do not regress #2: show_savestate_load_failure must still be called on both
  failure paths, and last_netplay_state must survive.

Out of scope: anything in !18 or !11, and any change to vendor/ggrs beyond
what this branch already carries.

(Gate note: I could not run scripts/gate.sh myself this sweep -- the review box
is at 0.67 GB free RAM of 15.9 GB and WSL is intermittently refusing connections.
That is an environment fault on my side, filed separately; it does not change any
of the above, which is all measured from git.)

## Bounced with the exact resolution -- and this cluster now has an ORDER Re-tested this sweep against **current** `main` (`c857fea`) rather than trusting the previous verdict, because a conflict can resolve itself once a sibling lands: git merge-base origin/main origin/issue-5 -> 00f918e (2026-08-24) behind main by -> 18 commits merge origin/main -> CONFLICTS: 3 src/device.rs src/savestates.rs src/netplay.rs It still reproduces, so this is real work, not a stale bounce. ### Why this PR is first, and !18 / !11 are second `issue-5` is a strict subset of both other open branches: git merge-base --is-ancestor origin/issue-5 origin/issue-4 -> TRUE (!18) git merge-base --is-ancestor origin/issue-5 origin/issue-7 -> TRUE (!11) All three currently conflict on the *same* `src/` files, so repairing them independently means resolving the same conflict three times and then reconciling three rebases of the same commits. **Land this one first**; once `issue-5` is on `main`, the other two rebase over content they already contain and their conflict surface mostly evaporates. I have noted the same ordering on !18 and !11. Also confirmed this branch is self-contained and worth landing alone -- it both writes and reads the new field: src/savestates.rs:131 device.savestate.netplay_frame = Some(netplay.session.current_frame()); src/savestates.rs:382 if let Some(frame) = device.savestate.netplay_frame { ### The resolution, hunk by hunk Two of the three are trivial. **The other two are traps** -- the default "keep both sides" is wrong in one and does not compile in the other. **1. `src/device.rs` ~352 -- KEEP BOTH.** Two independent field initialisers in the same struct literal: netplay_frame: None, // yours last_netplay_state: None, // main, from #2 **2. `src/savestates.rs` ~79 -- KEEP BOTH.** Same shape, two independent fields (`netplay_frame: Option<i32>` and `last_netplay_state: Option<SavestateData>`). Keep both doc comments. **3. `src/savestates.rs` ~557 -- DO NOT KEEP BOTH. THIS IS THE DANGEROUS ONE.** git has aligned two regions that are not the same code. Your side is the **success** path (`retroachievements::load_state`, the cheats block, your new `netplay::reset_frame` block, the "Savestate loaded from slot" message). `main`'s side is a single line: show_savestate_load_failure(device, rewind); because `#2` refactored the **failure** path into a helper (it is called at `origin/main:src/savestates.rs:541` and `:544`). A mechanical "keep both sides" splices your `reset_frame` call into the failure branch -- it would then fire when a savestate load *failed*, which is exactly backwards and no test will catch it. Correct resolution: take `main`'s structure, keep both `show_savestate_load_failure(...)` call sites, and re-insert your netplay block **inside the success branch only**, after the cheats block: if let Some(netplay) = device.netplay.as_mut() { if let Some(frame) = device.savestate.netplay_frame { netplay::reset_frame(&mut netplay.session, frame); } } **4. `src/netplay.rs` ~723 -- MERGE THE TEST MODULES, do not keep both.** You add `pub fn reset_frame(...)` plus a `#[cfg(test)] mod tests` (SyncTest-based). `main` independently added its *own* `#[cfg(test)] mod tests` in the same file (`state_message_constants_match_protocol`, `large_payload_round_trips_through_chunked_codec`). Two `mod tests` in one file is a duplicate module definition and will not compile. Keep `reset_frame` at top level, then fold your test fns into the single existing `mod tests`, keeping `use super::*;` and adding your `use ggrs::{Config, GgrsRequest, SessionBuilder};` and helper types inside it. Watch for a name clash on `calculate_hash`. ### Acceptance - `git diff --name-only --diff-filter=U` is empty after the merge. - `bash scripts/gate.sh` green -- all three steps, including step 3 `cargo test --lib`. - Quote the **step 3 test count**. `main` is the baseline; your added tests must show up on top of it, and you must name them. A count equal to main's means the gate ran a stale base (see #23/#25) and proves nothing. - Do not regress `#2`: `show_savestate_load_failure` must still be called on both failure paths, and `last_netplay_state` must survive. Out of scope: anything in `!18` or `!11`, and any change to `vendor/ggrs` beyond what this branch already carries. *(Gate note: I could not run `scripts/gate.sh` myself this sweep -- the review box is at 0.67 GB free RAM of 15.9 GB and WSL is intermittently refusing connections. That is an environment fault on my side, filed separately; it does not change any of the above, which is all measured from git.)*

pbwraith added the

bounced

label 2026-08-26 14:52:39 +02:00

pbwraith referenced this pull request 2026-08-26 14:52:40 +02:00

netplay: session resume — reload saved state and broadcast to peers (#4) #18

pbwraith referenced this pull request 2026-08-26 14:52:40 +02:00

Desync auto-resync: client receives and loads full state (#7) #11

pbwraith added the

manual-only

label 2026-08-26 20:25:31 +02:00

pbwraith referenced this pull request 2026-08-26 23:54:29 +02:00

Role boundary unrecorded: every open item in this repo is `manual-only`, so 3 bounced PRs carry findings no lane can ever act on #26

This repo is archived. You cannot comment on pull requests.

Reviewers

No reviewers

Labels

Clear labels   

blocked

Has a named, verified prerequisite that is not yet met

  

bounced

Reviewed and sent back with a specific finding; makes the PR claimable by the worker lane (priority 0).

  

in-progress

  

in-review

  

manual-only

Worker lanes must not claim this. Rust builds materialise 8-17 GB target/ dirs per worktree and exhaust the review box; the gate runs on the Actions runner instead.

  

needs-guidance

Requires an owner/human decision before further work

  

netplay

  

p1

  

p2

  

p3

  

rust

No Label

blocked

bounced

in-progress

in-review

manual-only

needs-guidance

netplay

p1

p2

p3

rust

Milestone

Clear milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

No Assignees

1 Participants

Due Date

The due date is invalid or out of range. Please use the format 'yyyy-mm-dd'.

No due date set.

Dependencies

No dependencies set.

Reference: pbwraith/gopher64-netplay#9

No description provided.