pbwraith/gopher64-netplay
Archived
1
0
Fork 0
Code Issues Pull Requests 3 Actions Packages Projects Releases Wiki Activity

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

Merged
pbwraith merged 1 commits from issue-2 into main 2026-08-26 13:35:57 +02:00
Conversation 3 Commits 1 Files Changed 4 +487 -140
pbwraith commented 2026-08-25 11:39:02 +02:00
Owner
Copy Link

Closes #2

Allows savestate creation/loading while a netplay session is active, fully
in-memory (no disk I/O).

Implementation

  • SavestateData::save_to_memory() -> Vec<u8>: serializes device + saves +
    rdp/ra state into the same compressed multi-entry format used for on-disk
    savestates (postcard + zip), without touching the disk.
  • SavestateData::load_from_memory(&[u8]) -> Option<SavestateData>: reverse
    of the above.
  • create_savestate_in_memory(device) -> Option<SavestateData>: clones the
    device exactly like create_savestate (reusing the DEVICE_CLONE /
    SAVES_CLONE statics), returns the data instead of writing a file. Both
    statics are held for the whole capture so concurrent savestate callers get
    a consistent snapshot.
  • load_savestate_from_data(device, &SavestateData) -> bool: the
    std::mem::swap apply-block from load_savestate was refactored into a
    shared apply_savestate() used by both the disk path and the in-memory
    path.
  • process_savestates routes explicit save/load requests to the in-memory
    path while device.netplay.is_some(), storing the latest netplay snapshot
    in Savestate.last_netplay_state (#[serde(skip)]). The automatic rewind
    pool stays offline-only (unchanged).
  • src/device/vi.rs: process_savestates is now invoked during netplay
    too; the function itself keeps all offline behavior identical.
  • src/ui/video.rs: rdp state capture/restore/idle/onscreen are safe no-ops
    while the parallel-RDP backend is not initialized, so in-memory state ops
    work headless (tests) and before/after UI init.

Tests (cargo test, all passing)

  • netplay_in_memory_savestate_round_trip_restores_device: create ->
    save_to_memory -> load_from_memory -> apply with an active (mocked) netplay
    session; verifies cpu regs, rdram bytes and vi regs round-trip.
  • netplay_process_savestates_uses_in_memory_snapshot: explicit save/load
    requests during netplay route through process_savestates to the
    in-memory snapshot and never touch the disk.
  • offline_explicit_save_stays_on_disk_path_...: offline explicit save
    still writes the state file to disk and never populates the in-memory
    slot (offline behavior unchanged).

Environment note (worker box only, not part of this diff): the local
build uses -march=x86-64 and no LTO because the WSL worker box lacks AVX2
and rust-lld cannot consume gcc LTO sections; tests are run with
RUST_MIN_STACK raised for the postcard Device (de)serialization depth.

Closes #2 Allows savestate creation/loading while a netplay session is active, fully in-memory (no disk I/O). Implementation - `SavestateData::save_to_memory() -> Vec<u8>`: serializes device + saves + rdp/ra state into the same compressed multi-entry format used for on-disk savestates (postcard + zip), without touching the disk. - `SavestateData::load_from_memory(&[u8]) -> Option<SavestateData>`: reverse of the above. - `create_savestate_in_memory(device) -> Option<SavestateData>`: clones the device exactly like `create_savestate` (reusing the `DEVICE_CLONE` / `SAVES_CLONE` statics), returns the data instead of writing a file. Both statics are held for the whole capture so concurrent savestate callers get a consistent snapshot. - `load_savestate_from_data(device, &SavestateData) -> bool`: the `std::mem::swap` apply-block from `load_savestate` was refactored into a shared `apply_savestate()` used by both the disk path and the in-memory path. - `process_savestates` routes explicit save/load requests to the in-memory path while `device.netplay.is_some()`, storing the latest netplay snapshot in `Savestate.last_netplay_state` (`#[serde(skip)]`). The automatic rewind pool stays offline-only (unchanged). - `src/device/vi.rs`: `process_savestates` is now invoked during netplay too; the function itself keeps all offline behavior identical. - `src/ui/video.rs`: rdp state capture/restore/idle/onscreen are safe no-ops while the parallel-RDP backend is not initialized, so in-memory state ops work headless (tests) and before/after UI init. Tests (cargo test, all passing) - `netplay_in_memory_savestate_round_trip_restores_device`: create -> save_to_memory -> load_from_memory -> apply with an active (mocked) netplay session; verifies cpu regs, rdram bytes and vi regs round-trip. - `netplay_process_savestates_uses_in_memory_snapshot`: explicit save/load requests during netplay route through `process_savestates` to the in-memory snapshot and never touch the disk. - `offline_explicit_save_stays_on_disk_path_...`: offline explicit save still writes the state file to disk and never populates the in-memory slot (offline behavior unchanged). Environment note (worker box only, not part of this diff): the local build uses `-march=x86-64` and no LTO because the WSL worker box lacks AVX2 and rust-lld cannot consume gcc LTO sections; tests are run with `RUST_MIN_STACK` raised for the postcard Device (de)serialization depth.
pbwraith added 1 commit 2026-08-25 11:39:04 +02:00
Allow savestate creation/loading during netplay (#2)
Some checks failed
Rust-lint / lint-linux (pull_request) Failing after 1s
Details
Rust / build-linux (linux-x86_64, X64, ubuntu-24.04, x86_64-unknown-linux-gnu) (pull_request) Failing after 1s
Details
Rust / build-android (29.0.14206865, ubuntu-24.04, 33) (pull_request) Failing after 1s
Details
Rust / build-linux (linux-aarch64, ARM64, ubuntu-24.04-arm, aarch64-unknown-linux-gnu) (pull_request) Has been cancelled
Details
Rust / build-windows (windows-aarch64, woa64, windows-11-arm, aarch64-pc-windows-msvc) (pull_request) Has been cancelled
Details
Rust / build-windows (windows-x86_64, win64, windows-2025, x86_64-pc-windows-msvc) (pull_request) Has been cancelled
Details
Rust / build-macos (macos-aarch64, ARM64, aarch64-apple-darwin) (pull_request) Has been cancelled
Details
Rust / Collect release artifacts (pull_request) Has been cancelled
Details
a0dce58c55
pbwraith commented 2026-08-25 12:38:50 +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 commented 2026-08-26 08:39:55 +02:00
Author
Owner
Copy Link

Parked on #23: this PR's own test is the one that aborts the gate

Gating main+!21+!22+!14+!15 in a warm clone on the review box:

GATE step 3/3: cargo test --lib
thread 'savestates::tests::netplay_in_memory_savestate_round_trip_restores_device' has overflowed its stack
fatal runtime error: stack overflow, aborting
RESULT gate_exit=101

This is not a defect in your test. size_of::<Device>() is 307,968 bytes,
Device::new() builds it as a stack temporary before boxing it
(src/device.rs:345) in debug builds, and libtest gives each test thread
2 MiB. Measured: RUST_MIN_STACK=4MiB overflows, 8MiB passes -- the same
8 MiB the repo already link-args onto Windows binaries. The identical test
passes unchanged with a bigger stack.

The same cause takes down !18 via a different test, and step 3 has never run a
real test on main (it reports 0 passed), which is why this surfaced now.

Blocked on #23, which I filed with the full measurement. Not bounced: there is nothing for this branch to repair -- the fix belongs in scripts/gate.sh / a test helper, and doing it inside this PR would collide with #23's branch.

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 on #23: this PR's own test is the one that aborts the gate Gating `main`+!21+!22+!14+**!15** in a warm clone on the review box: ``` GATE step 3/3: cargo test --lib thread 'savestates::tests::netplay_in_memory_savestate_round_trip_restores_device' has overflowed its stack fatal runtime error: stack overflow, aborting RESULT gate_exit=101 ``` **This is not a defect in your test.** `size_of::<Device>()` is 307,968 bytes, `Device::new()` builds it as a stack temporary before boxing it (`src/device.rs:345`) in debug builds, and `libtest` gives each test thread 2 MiB. Measured: `RUST_MIN_STACK=4MiB` overflows, `8MiB` passes -- the same 8 MiB the repo already link-args onto Windows binaries. The identical test passes unchanged with a bigger stack. The same cause takes down !18 via a different test, and step 3 has never run a real test on `main` (it reports `0 passed`), which is why this surfaced now. **Blocked on #23**, which I filed with the full measurement. Not `bounced`: there is nothing for this branch to repair -- the fix belongs in `scripts/gate.sh` / a test helper, and doing it inside this PR would collide with #23's branch. ### 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:39:56 +02:00
pbwraith commented 2026-08-26 13:35:55 +02:00
Author
Owner
Copy Link

Merged. Gated against the MERGE RESULT on the post-!24 main.

First run discarded -- stale base

My first attempt at this gate warned test_util/with_big_stack absent on the
merge result, which was impossible if !24 was really on main. Cause: I
merged !24 through the Gitea API, so the WINDOWS checkout that ~/gate64
fetches from still had origin/main = ff5b4f3 (pre-!24). Re-fetched
(ff5b4f3 -> d6569a1, src/test_util.rs PRESENT) and re-ran. Recording it
because a stale base does not conflict and does not error -- it just quietly
gates a tree nobody proposed.

Gate

BRANCH_HEAD            a0dce58
merge origin/main      CONFLICTS=0
MERGE_RESULT_HEAD      da21ff1
precondition           test_util present (from !24)
bash scripts/gate.sh   GATE_OK   GATE_EXIT=0

step 3:  running 8 tests   (main runs 5 after !24)
  netplay::tests::state_message_constants_match_protocol ... ok
  netplay::tests::ready_ack_send_receive_within_timeout ... ok
  netplay::tests::missing_ack_times_out_with_standard_panic ... ok
  netplay::tests::large_payload_round_trips_through_chunked_codec ... ok
  test_util::tests::device_round_trip_fits_under_the_8_mib_test_stack_budget ... ok
  savestates::tests::netplay_process_savestates_uses_in_memory_snapshot ... ok
  savestates::tests::netplay_in_memory_savestate_round_trip_restores_device ... ok
  savestates::tests::offline_explicit_save_stays_on_disk_path_and_never_populates_memory ... ok
  test result: ok. 8 passed; 0 failed; finished in 55.94s

5 -> 8, all three new ones named in the output.

This is the PR that proves #23 is actually fixed

savestates::tests::netplay_in_memory_savestate_round_trip_restores_device is
the exact test #23 named as SIGABRTing the whole binary. It now runs and
passes. That is better evidence than !24's own synthetic tripwire, because it
is the real workload rather than a test written to demonstrate the problem.

One residual, filed as #25 rather than held

These tests pass because scripts/gate.sh exports RUST_MIN_STACK=8388608 --
they do NOT route through test_util::with_big_stack. So on main, a bare
cargo test --lib now aborts:

thread 'savestates::tests::netplay_in_memory_savestate_round_trip_restores_device'
  has overflowed its stack
fatal runtime error: stack overflow, aborting   (signal: 6, SIGABRT)

and takes the four not-yet-reported tests down with it. Merging anyway because
the project's documented gate command is ./scripts/gate.sh, that gate is
green, and this unblocks the netplay stack -- but it is a real trap for anyone
who types cargo test, so it is filed with the measurement and a three-line
fix rather than left to be rediscovered.

Merged. Gated against the MERGE RESULT on the post-!24 main. ## First run discarded -- stale base My first attempt at this gate warned `test_util/with_big_stack absent` on the merge result, which was impossible if `!24` was really on `main`. Cause: I merged `!24` through the Gitea API, so the WINDOWS checkout that `~/gate64` fetches from still had `origin/main = ff5b4f3` (pre-!24). Re-fetched (`ff5b4f3 -> d6569a1`, `src/test_util.rs` PRESENT) and re-ran. Recording it because a stale base does not conflict and does not error -- it just quietly gates a tree nobody proposed. ## Gate BRANCH_HEAD a0dce58 merge origin/main CONFLICTS=0 MERGE_RESULT_HEAD da21ff1 precondition test_util present (from !24) bash scripts/gate.sh GATE_OK GATE_EXIT=0 step 3: running 8 tests (main runs 5 after !24) netplay::tests::state_message_constants_match_protocol ... ok netplay::tests::ready_ack_send_receive_within_timeout ... ok netplay::tests::missing_ack_times_out_with_standard_panic ... ok netplay::tests::large_payload_round_trips_through_chunked_codec ... ok test_util::tests::device_round_trip_fits_under_the_8_mib_test_stack_budget ... ok savestates::tests::netplay_process_savestates_uses_in_memory_snapshot ... ok savestates::tests::netplay_in_memory_savestate_round_trip_restores_device ... ok savestates::tests::offline_explicit_save_stays_on_disk_path_and_never_populates_memory ... ok test result: ok. 8 passed; 0 failed; finished in 55.94s 5 -> 8, all three new ones named in the output. ## This is the PR that proves #23 is actually fixed `savestates::tests::netplay_in_memory_savestate_round_trip_restores_device` is the exact test #23 named as SIGABRTing the whole binary. It now runs and passes. That is better evidence than !24's own synthetic tripwire, because it is the real workload rather than a test written to demonstrate the problem. ## One residual, filed as #25 rather than held These tests pass because `scripts/gate.sh` exports `RUST_MIN_STACK=8388608` -- they do NOT route through `test_util::with_big_stack`. So on `main`, a bare `cargo test --lib` now aborts: thread 'savestates::tests::netplay_in_memory_savestate_round_trip_restores_device' has overflowed its stack fatal runtime error: stack overflow, aborting (signal: 6, SIGABRT) and takes the four not-yet-reported tests down with it. Merging anyway because the project's documented gate command is `./scripts/gate.sh`, that gate is green, and this unblocks the netplay stack -- but it is a real trap for anyone who types `cargo test`, so it is filed with the measurement and a three-line fix rather than left to be rediscovered.
pbwraith merged commit c857feada3 into main 2026-08-26 13:35:57 +02:00
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#15
No description provided.
Delete Branch "issue-2"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?