netplay: 8 MiB test-stack budget for Device-constructing unit tests (issue #23) #24

Merged

pbwraith merged 1 commits from issue-23 into main 2026-08-26 13:23:29 +02:00

Conversation 2 Commits 1 Files Changed 3 +83

pbwraith commented 2026-08-26 09:48:10 +02:00

Owner

Copy Link

Closes #23

What

Fixes the step-3 gate abort: every Device-constructing unit test overflowed
the 2 MiB libtest thread stack in a debug build, aborting the whole test
binary (fatal runtime error: stack overflow / SIGABRT) and taking down
every other test in the run. One cause, five PR-shaped symptoms
(!15/!13/!9/!18 + any future netplay/savestate test).

Root cause recap (measured in the issue)

• Device is the entire N64 machine state; size_of::<Device>() on this
  run = 307968 bytes (~301 KiB) (criterion 5; well under the ~700 KiB
  revisit threshold).
• Device::new() returns Box::new(Device { ... }); in a debug build the
  struct literal and nested field literals are materialised on the stack
  before being moved into the box, so peak stack use is a multiple of the
  struct size.
• libtest runs each test on a spawned thread whose default stack is 2 MiB —
  not the 8 MiB main-thread stack the project already standardises on
  (.cargo/config.toml ships /STACK:8388608 on Windows).
• Bracketing on the failing cluster test: RUST_MIN_STACK=4MiB overflows,
  8MiB passes — large-frame pressure, not recursion.

Fix taken: (b) durable helper + (a) gate immunisation — both

The issue says pick (a) or (b). I took both because they cover different
failure surfaces and together satisfy every acceptance criterion:

• (b) src/test_util.rs — with_big_stack (the durable fix): runs a
  test body on an explicitly sized 8 MiB thread. Correct under any runner,
  no env var required. All Device-constructing tests (this one plus the
  netplay cluster's as they land) route their bodies through it, so a bare
  cargo test --lib passes — acceptance criterion 3, which fix (a) alone
  fails.
• (a) scripts/gate.sh: RUST_MIN_STACK=8388608 exported around step
  3. Cheap insurance so the documented gate also passes for
  Device-constructing tests that are not yet wrapped — the netplay
  cluster's !15/!18 tests land after this fix on contended files and cannot
  be assumed rewritten yet.

Regression test (criterion 4)

test_util::tests::constructing_two_devices_fits_under_the_8_mib_test_stack_budget
constructs two Devices (plus clone_state) via the helper. If the
wrapping is ever removed the test binary aborts with a stack overflow
instead of failing normally — the mitigation cannot silently regress once
step 3 has real content. It also asserts size_of::<Device>() < 700 KiB
(criterion 5 tripwire: if Device grows past ~700 KiB the 8 MiB budget
must be revisited).

Evidence (own runs, this box)

1. Probe, unwrapped body (two Device::new(false) + clone_state) on the
   ambient libtest thread, cargo test --lib, no env:
   fatal runtime error: stack overflow — SIGABRT (exit 134). This is the
   failure mode the issue documents; it also proves the regression test
   would fail if the mitigation were removed.
2. Same unwrapped body with RUST_MIN_STACK=8388608: passes — stack
   budget, not recursion.
3. Shipped wrapped test, bare cargo test --lib, no wrapper, no env:
   test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
4. Full gate: ./scripts/gate.sh → GATE_OK, step 3
   test result: ok. 1 passed; 0 failed; ... (non-zero count; step 3 on
   main alone used to run 0 tests).
5. git diff --check: clean.

Out of scope (per issue)

• Device layout / Device::new signature changes (option (c)) — invasive
  change to a hot upstream-shared file, own issue.
• vendor/ggrs/, the mutual netplay-PR merge conflicts, -C target-cpu
  portability.

Closes #23 ## What Fixes the step-3 gate abort: every `Device`-constructing unit test overflowed the 2 MiB libtest thread stack in a debug build, aborting the whole test binary (`fatal runtime error: stack overflow` / SIGABRT) and taking down every other test in the run. One cause, five PR-shaped symptoms (!15/!13/!9/!18 + any future netplay/savestate test). ## Root cause recap (measured in the issue) - `Device` is the entire N64 machine state; `size_of::<Device>()` on this run = **307968 bytes (~301 KiB)** (criterion 5; well under the ~700 KiB revisit threshold). - `Device::new()` returns `Box::new(Device { ... })`; in a debug build the struct literal and nested field literals are materialised on the stack before being moved into the box, so peak stack use is a multiple of the struct size. - libtest runs each test on a spawned thread whose default stack is 2 MiB — not the 8 MiB main-thread stack the project already standardises on (`.cargo/config.toml` ships `/STACK:8388608` on Windows). - Bracketing on the failing cluster test: `RUST_MIN_STACK=4MiB` overflows, `8MiB` passes — large-frame pressure, not recursion. ## Fix taken: (b) durable helper + (a) gate immunisation — both The issue says pick (a) or (b). I took **both** because they cover different failure surfaces and together satisfy every acceptance criterion: - **(b) `src/test_util.rs` — `with_big_stack`** (the durable fix): runs a test body on an explicitly sized 8 MiB thread. Correct under any runner, no env var required. All Device-constructing tests (this one plus the netplay cluster's as they land) route their bodies through it, so a bare `cargo test --lib` passes — acceptance criterion 3, which fix (a) alone fails. - **(a) `scripts/gate.sh`**: `RUST_MIN_STACK=8388608` exported around step 3. Cheap insurance so the documented gate also passes for Device-constructing tests that are not yet wrapped — the netplay cluster's !15/!18 tests land after this fix on contended files and cannot be assumed rewritten yet. ## Regression test (criterion 4) `test_util::tests::constructing_two_devices_fits_under_the_8_mib_test_stack_budget` constructs two `Device`s (plus `clone_state`) via the helper. If the wrapping is ever removed the test binary aborts with a stack overflow instead of failing normally — the mitigation cannot silently regress once step 3 has real content. It also asserts `size_of::<Device>() < 700 KiB` (criterion 5 tripwire: if `Device` grows past ~700 KiB the 8 MiB budget must be revisited). ## Evidence (own runs, this box) 1. Probe, unwrapped body (two `Device::new(false)` + `clone_state`) on the ambient libtest thread, `cargo test --lib`, no env: `fatal runtime error: stack overflow` — SIGABRT (exit 134). This is the failure mode the issue documents; it also proves the regression test would fail if the mitigation were removed. 2. Same unwrapped body with `RUST_MIN_STACK=8388608`: **passes** — stack budget, not recursion. 3. Shipped wrapped test, bare `cargo test --lib`, no wrapper, no env: `test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out` 4. Full gate: `./scripts/gate.sh` → `GATE_OK`, step 3 `test result: ok. 1 passed; 0 failed; ...` (non-zero count; step 3 on `main` alone used to run 0 tests). 5. `git diff --check`: clean. ## Out of scope (per issue) - `Device` layout / `Device::new` signature changes (option (c)) — invasive change to a hot upstream-shared file, own issue. - `vendor/ggrs/`, the mutual netplay-PR merge conflicts, `-C target-cpu` portability.

pbwraith added 1 commit 2026-08-26 09:48:12 +02:00

gopher64-netplay#23: give Device-constructing unit tests an 8 MiB test-stack budget ... d400b53886

libtest runs each test on a thread with the default 2 MiB stack. In a debug
build, Device::new() materialises ~301 KiB of struct literals on the stack,
and postcard::from_bytes::<Device> rebuilds the whole value on the stack, so
any Device-constructing unit test (netplay savestate round trips etc.) peaks
at several times that and aborts the ENTIRE test binary with a stack
overflow, taking every other test down with it.

Fix, both layers:
- src/test_util.rs: with_big_stack() runs a test body on an explicitly sized
  8 MiB thread (the budget the project already standardises on in
  .cargo/config.toml). Regression test: a device serialise/restore/clone
  round trip measured to overflow the 2 MiB libtest thread (SIGABRT) and
  pass at 8 MiB, plus a size_of::<Device>() < 700 KiB tripwire.
- scripts/gate.sh: export RUST_MIN_STACK=8388608 around step 3 so the
  documented gate also covers Device-constructing tests that are not yet
  wrapped (the netplay cluster's !15/!18 tests land after this fix).

Verified: cargo test --lib bare (no env) -> 1 passed; full gate -> GATE_OK
with step 3 reporting 1 passed (was 0 on main).

pbwraith commented 2026-08-26 09:49:49 +02:00

Author

Owner

Copy Link

Evidence for gopher64-netplay#23 (implementer lane 2, commit d400b538, PR #24):

Fix taken: (b) durable helper + (a) gate immunisation — both. (b) is the durable fix satisfying criterion 3; (a) additionally immunises the documented gate path so the netplay cluster's unwrapped Device-constructing tests (!15/!18, landing after this fix) pass step 3 without depending on rebase timing.

Measured on this box (own runs):

• size_of::<Device>() = 307968 bytes (~301 KiB) — criterion 5, well under the ~700 KiB revisit threshold (also asserted in the test as a tripwire).
• Probe: the exact round-trip body shipped in the regression test, run UNWRAPPED on the ambient libtest thread, cargo test --lib, no env: thread 'stack_probe...' has overflowed its stack -> fatal runtime error: stack overflow, aborting (SIGABRT, exit 134). Reproduces the issue's documented failure mode with main-tree APIs only (Device::new + postcard serialise/restore + clone_state — the same profile as the !15/!18 netplay-savestate round trips).
• Same body with RUST_MIN_STACK=8388608: passes (proves large-frame stack pressure, not runaway recursion).
• Shipped wrapped test, bare cargo test --lib, no wrapper, no env var: test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s — criterion 3 (fix (a) alone cannot satisfy this).
• Full gate ./scripts/gate.sh: steps 1-3 all green, GATE_OK; step 3 now reports a non-zero count (1 passed) where main's step 3 ran 0 passed — criteria 1/2 step-3 requirement (the main+!15 / main+!18 combinations get exercised by the reviewer once those branches merge; gate.sh's RUST_MIN_STACK=8388608 export covers their not-yet-wrapped tests).
• git diff --check: clean.

Files: scripts/gate.sh (RUST_MIN_STACK export around step 3), src/test_util.rs (new: with_big_stack + regression test), src/lib.rs (#[cfg(test)] mod test_util;). No changes to device.rs/netplay.rs/savestates.rs, no vendor/ggrs changes. Out-of-scope items (Device layout, merge conflicts, target-cpu) untouched.

Evidence for gopher64-netplay#23 (implementer lane 2, commit d400b538, PR #24): **Fix taken: (b) durable helper + (a) gate immunisation — both.** (b) is the durable fix satisfying criterion 3; (a) additionally immunises the documented gate path so the netplay cluster's unwrapped Device-constructing tests (!15/!18, landing after this fix) pass step 3 without depending on rebase timing. Measured on this box (own runs): - `size_of::<Device>()` = **307968 bytes (~301 KiB)** — criterion 5, well under the ~700 KiB revisit threshold (also asserted in the test as a tripwire). - Probe: the exact round-trip body shipped in the regression test, run UNWRAPPED on the ambient libtest thread, `cargo test --lib`, no env: `thread 'stack_probe...' has overflowed its stack` -> `fatal runtime error: stack overflow, aborting` (SIGABRT, exit 134). Reproduces the issue's documented failure mode with main-tree APIs only (Device::new + postcard serialise/restore + clone_state — the same profile as the !15/!18 netplay-savestate round trips). - Same body with `RUST_MIN_STACK=8388608`: **passes** (proves large-frame stack pressure, not runaway recursion). - Shipped wrapped test, bare `cargo test --lib`, no wrapper, no env var: `test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out; finished in 0.02s` — criterion 3 (fix (a) alone cannot satisfy this). - Full gate `./scripts/gate.sh`: steps 1-3 all green, **GATE_OK**; step 3 now reports a **non-zero** count (`1 passed`) where main's step 3 ran `0 passed` — criteria 1/2 step-3 requirement (the main+!15 / main+!18 combinations get exercised by the reviewer once those branches merge; gate.sh's `RUST_MIN_STACK=8388608` export covers their not-yet-wrapped tests). - `git diff --check`: clean. Files: `scripts/gate.sh` (RUST_MIN_STACK export around step 3), `src/test_util.rs` (new: `with_big_stack` + regression test), `src/lib.rs` (`#[cfg(test)] mod test_util;`). No changes to device.rs/netplay.rs/savestates.rs, no vendor/ggrs changes. Out-of-scope items (Device layout, merge conflicts, target-cpu) untouched.

pbwraith commented 2026-08-26 13:23:28 +02:00

Author

Owner

Copy Link

Merged. This is the one that unwedges the gate, so I gated it twice -- the
first run was void and I want that on the record rather than quietly redone.

My first gate was invalid (TRAP 2), disregard any earlier green

The warm clone ~/gate64 has the WINDOWS CHECKOUT as its origin, and I
fetched +refs/heads/*, which pulls only that checkout's LOCAL heads -- and
its only local head is main. So origin/issue-23 did not exist,
git checkout -B verify origin/issue-23 failed with

fatal: 'origin/issue-23' is not a commit and a branch 'verify' cannot be
created from it

and because my script lacked set -e it carried on and gated plain main,
printing GATE_OK. The tell was exactly the one this repo has been bitten by
before: the step-3 test count did not move (4 tests, all netplay::*), and
grep -c device_round_trip_fits_under_the_8_mib_test_stack_budget on the log
returned 0. A green gate that never compiled the PR's code proves nothing.

Corrected by fetching the checkout's remote-tracking refs
(+refs/remotes/origin/*:refs/remotes/origin/*) and asserting every
precondition loudly instead of letting a failure fall through.

Real gate (merge result)

issue-23 HEAD          d400b53   (as proposed)
merge origin/main      CONFLICTS=0
MERGE_RESULT_HEAD      3083c8f
precondition           src/test_util.rs present, 71 lines
bash scripts/gate.sh   GATE step 1/3 ok, 2/3 ok, 3/3 ok -> GATE_OK, exit 0

step 3:  running 5 tests   (main runs 4)
  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
  test result: ok. 5 passed; 0 failed; finished in 55.90s

4 -> 5 with the new test named in the output: the PR's code was exercised.

The check that decided it

Taking both (a) the gate env var and (b) the with_big_stack helper is the
right call, but it raises an obvious question: is (b) actually doing anything,
or is RUST_MIN_STACK=8388608 in gate.sh silently carrying the test? If it
were the env var alone, the helper would be decorative and every future test
would still depend on being run through gate.sh.

So I re-ran the lib tests with the stack budget pinned back DOWN to the
libtest default, using gate.sh's own provisioning env:

RUST_MIN_STACK=2097152 cargo test --lib
running 5 tests
test test_util::tests::device_round_trip_fits_under_the_8_mib_test_stack_budget ... ok
test result: ok. 5 passed; 0 failed; finished in 55.68s
BARE_2MIB_EXIT=0

At 2 MiB -- the exact budget that SIGABRTs the whole binary today -- the
Device round trip passes. with_big_stack is load-bearing on its own, so a
test that routes through it is safe under a bare cargo test --lib on any
runner, not just under gate.sh. That is the durable half, and it is real.

Notes for whoever picks up the blocked stack

!9, !10, !11, !13, !15 and !18 were all blocked behind this one
abort. Landing this does NOT auto-fix them: RUST_MIN_STACK in gate.sh
immunises the gate, but each Device-constructing test should still route its
body through test_util::with_big_stack so it survives a bare
cargo test --lib. Five of those six also currently report
mergeable=false, so they need a rebase onto this main regardless.

The MAX_DEVICE_SIZE = 700 * 1024 tripwire is a good touch -- it turns the
8 MiB choice into something that fails loudly if Device grows, instead of a
magic number that silently stops being enough.

Merged. This is the one that unwedges the gate, so I gated it twice -- the first run was void and I want that on the record rather than quietly redone. ## My first gate was invalid (TRAP 2), disregard any earlier green The warm clone `~/gate64` has the WINDOWS CHECKOUT as its `origin`, and I fetched `+refs/heads/*`, which pulls only that checkout's LOCAL heads -- and its only local head is `main`. So `origin/issue-23` did not exist, `git checkout -B verify origin/issue-23` failed with fatal: 'origin/issue-23' is not a commit and a branch 'verify' cannot be created from it and because my script lacked `set -e` it carried on and gated plain `main`, printing `GATE_OK`. The tell was exactly the one this repo has been bitten by before: **the step-3 test count did not move** (4 tests, all `netplay::*`), and `grep -c device_round_trip_fits_under_the_8_mib_test_stack_budget` on the log returned **0**. A green gate that never compiled the PR's code proves nothing. Corrected by fetching the checkout's remote-tracking refs (`+refs/remotes/origin/*:refs/remotes/origin/*`) and asserting every precondition loudly instead of letting a failure fall through. ## Real gate (merge result) issue-23 HEAD d400b53 (as proposed) merge origin/main CONFLICTS=0 MERGE_RESULT_HEAD 3083c8f precondition src/test_util.rs present, 71 lines bash scripts/gate.sh GATE step 1/3 ok, 2/3 ok, 3/3 ok -> GATE_OK, exit 0 step 3: running 5 tests (main runs 4) 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 test result: ok. 5 passed; 0 failed; finished in 55.90s 4 -> 5 with the new test named in the output: the PR's code was exercised. ## The check that decided it Taking both (a) the gate env var and (b) the `with_big_stack` helper is the right call, but it raises an obvious question: is (b) actually doing anything, or is `RUST_MIN_STACK=8388608` in `gate.sh` silently carrying the test? If it were the env var alone, the helper would be decorative and every future test would still depend on being run through `gate.sh`. So I re-ran the lib tests with the stack budget pinned back DOWN to the libtest default, using `gate.sh`'s own provisioning env: RUST_MIN_STACK=2097152 cargo test --lib running 5 tests test test_util::tests::device_round_trip_fits_under_the_8_mib_test_stack_budget ... ok test result: ok. 5 passed; 0 failed; finished in 55.68s BARE_2MIB_EXIT=0 At 2 MiB -- the exact budget that SIGABRTs the whole binary today -- the Device round trip passes. `with_big_stack` is load-bearing on its own, so a test that routes through it is safe under a bare `cargo test --lib` on any runner, not just under `gate.sh`. That is the durable half, and it is real. ## Notes for whoever picks up the blocked stack `!9`, `!10`, `!11`, `!13`, `!15` and `!18` were all blocked behind this one abort. Landing this does NOT auto-fix them: `RUST_MIN_STACK` in `gate.sh` immunises the gate, but each Device-constructing test should still route its body through `test_util::with_big_stack` so it survives a bare `cargo test --lib`. Five of those six also currently report `mergeable=false`, so they need a rebase onto this main regardless. The `MAX_DEVICE_SIZE = 700 * 1024` tripwire is a good touch -- it turns the 8 MiB choice into something that fails loudly if `Device` grows, instead of a magic number that silently stops being enough.

pbwraith merged commit d6569a1d2c into main 2026-08-26 13:23:29 +02:00

pbwraith referenced this pull request 2026-08-26 13:35:54 +02:00

savestates Device tests abort under a bare `cargo test --lib`: adopt with_big_stack #25

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

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

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#24

No description provided.