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

New Issue

pbwraith · 2026-08-26T08:38:20+02:00

pbwraith commented

2026-08-26 08:38:20 +02:00

Summary

cargo test --lib -- step 3 of the documented gate (scripts/gate.sh) --
dies with fatal runtime error: stack overflow, aborting / SIGABRT on every
branch that adds a unit test constructing a Device. Because a stack overflow
aborts the whole test binary, one such test takes down every other test in
the run, so the gate is not merely red, it is uninformative.

This is one cause with five PR-shaped symptoms. Filing it once rather than
bouncing each PR for a defect none of them can reasonably fix alone.

Why nobody hit this before

cargo test --lib on main runs zero tests:

GATE step 3/3: cargo test --lib
test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out
RESULT CONTROL_main gate_exit=0 elapsed=289s

Step 3 is currently vacuous. It was green when #16 established it because
there was nothing to run. The first PR to add a real lib test discovers that
the step cannot execute one.

Measurement

Gated in a warm clone on the review box (WSL), origin/main @ 8a46ac5:

tree	gate_exit	where it dies
`main` (control)	0	step 3 runs 0 tests
`main`+!21+!22+!14	0	step 3 passes (no `Device` in its tests)
`main`+!21+!22+!14+!15	101	`savestates::tests::netplay_in_memory_savestate_round_trip_restores_device`
`main`+!21+!22+!18	101	`netplay::tests::resume_host_loads_state_from_disk_and_resets_frame`

Two different tests on two different branches, same failure mode. !18 subsumes
!14/!15/!13/!9 (they are ancestors of issue-4), so this single cause blocks
!15, !13, !9, !18 and any future netplay/savestate test.

Root cause, with numbers

Device is the entire N64 machine state (cpu, pif, cart, memory,
rsp, rdp, rdram, mi, pi, vi, ai, si, ri, vru, 4 x
TransferPak, cheats). Measured directly:
```
std::mem::size_of::<Device>() = 307968 bytes   (301 KiB)
```
Device::new() returns Box<Device> (src/device.rs:337) but builds it as
Box::new(Device { ... }) (src/device.rs:345). In a debug build the
struct literal and its nested field literals are materialized on the
stack and then moved into the box -- the temporaries are not elided at
-C opt-level=0. Peak stack use is therefore a multiple of 301 KiB, and
tests that build two devices (or one plus a savestate clone, e.g.
savestates.rs:324) multiply it again.
libtest runs each test on a spawned thread, whose Rust default stack
is 2 MiB -- not the 8 MiB main-thread stack.

Bracketed by measurement on the failing test:

RUST_MIN_STACK=4MiB  -> OVERFLOW
RUST_MIN_STACK=8MiB  -> PASS
RUST_MIN_STACK=16MiB -> PASS

The repo already knows the number. .cargo/config.toml ships:

[target.'cfg(target_os = "windows")']
rustflags = ["-C", "link-arg=/STACK:8388608"]     # 8 MiB

That 8 MiB link-arg sizes the built executable's main thread. Nothing gives
libtest's test threads the same budget, on any platform. The measured
requirement (>4, <=8 MiB) matches the figure the project already chose.

Confirms it is a large-frame problem and not runaway recursion: raising the
stack alone makes the identical test pass unchanged.

Fix

Pick one; (a) is the minimum that unblocks the cluster, (b) is the durable one.
State which you took and why.

(a) Give test threads the stack the project already standardises on. Export
RUST_MIN_STACK=8388608 in scripts/gate.sh around step 3 (and the same in
the CI test job, if one runs cargo test). Cheap and honest, but it only fixes
runners that go through those two paths -- a bare cargo test --lib still
aborts.

(b) Make the tests independent of the ambient stack. A shared test helper
that runs the body on an explicitly sized thread:

fn with_big_stack<F: FnOnce() + Send + 'static>(f: F) {
    std::thread::Builder::new()
        .stack_size(8 * 1024 * 1024)
        .spawn(f).unwrap().join().unwrap();
}

Correct under any runner, no env var required.

(c) Out of scope for this issue, but note it: removing the stack temporary
in Device::new (field-by-field init into Box::new_uninit, or boxing the
large sub-structs) would fix the class rather than the symptom. That is an
invasive change to a hot upstream-shared file and should be its own issue if
anyone wants it -- do not attempt it here.

Acceptance criteria

./scripts/gate.sh from a clean checkout of main with !15 merged in
exits 0, and step 3 reports a non-zero passing test count -- quote the
test result: line. A run reporting 0 passed does not satisfy this.
The same holds for main + !18.
cargo test --lib executed directly, with no wrapper and no env var
set by the caller, does not abort. (Fix (a) alone fails this; say so
explicitly if you choose (a) anyway.)
Add one test that would fail if the mitigation were removed -- e.g. a test
constructing two Devices -- so this cannot silently regress once step 3
has real content.
Quote size_of::<Device>() from your own run; if it has grown past ~700 KiB
the 8 MiB figure needs revisiting.

Explicitly OUT of scope

Changing Device's layout or Device::new's signature (see (c)).
Anything in vendor/ggrs/ -- the vendored fork is byte-identical across
!9/!10/!11/!18 (tree 816fce1d) and is not implicated.
The mutual merge conflicts between the netplay PRs. That is a separate
sequencing problem and does not belong in this fix.
-C target-cpu / portability (#16, #20) -- unrelated.

Coordination

This should land before !15/!13/!9/!18, on a branch off current main
touching only scripts/gate.sh and/or a test helper, so it does not collide
with the netplay cluster's contended files (src/netplay.rs,
src/savestates.rs, src/device.rs). !14 and !21/!22 are unaffected and can
land independently.

## Summary `cargo test --lib` -- **step 3 of the documented gate** (`scripts/gate.sh`) -- dies with `fatal runtime error: stack overflow, aborting` / SIGABRT on every branch that adds a unit test constructing a `Device`. Because a stack overflow aborts the **whole test binary**, one such test takes down every other test in the run, so the gate is not merely red, it is uninformative. This is one cause with five PR-shaped symptoms. Filing it once rather than bouncing each PR for a defect none of them can reasonably fix alone. ## Why nobody hit this before `cargo test --lib` on `main` runs **zero tests**: ``` GATE step 3/3: cargo test --lib test result: ok. 0 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out RESULT CONTROL_main gate_exit=0 elapsed=289s ``` Step 3 is currently vacuous. It was green when #16 established it because there was nothing to run. The first PR to add a real lib test discovers that the step cannot execute one. ## Measurement Gated in a warm clone on the review box (WSL), `origin/main` @ `8a46ac5`: | tree | gate_exit | where it dies | |---|---|---| | `main` (control) | **0** | step 3 runs 0 tests | | `main`+!21+!22+**!14** | **0** | step 3 passes (no `Device` in its tests) | | `main`+!21+!22+!14+**!15** | **101** | `savestates::tests::netplay_in_memory_savestate_round_trip_restores_device` | | `main`+!21+!22+**!18** | **101** | `netplay::tests::resume_host_loads_state_from_disk_and_resets_frame` | Two different tests on two different branches, same failure mode. !18 subsumes !14/!15/!13/!9 (they are ancestors of `issue-4`), so this single cause blocks **!15, !13, !9, !18** and any future netplay/savestate test. ## Root cause, with numbers 1. `Device` is the entire N64 machine state (`cpu`, `pif`, `cart`, `memory`, `rsp`, `rdp`, `rdram`, `mi`, `pi`, `vi`, `ai`, `si`, `ri`, `vru`, 4 x `TransferPak`, `cheats`). Measured directly: ``` std::mem::size_of::<Device>() = 307968 bytes (301 KiB) ``` 2. `Device::new()` returns `Box<Device>` (`src/device.rs:337`) but builds it as `Box::new(Device { ... })` (`src/device.rs:345`). In a **debug** build the struct literal and its nested field literals are materialized **on the stack** and then moved into the box -- the temporaries are not elided at `-C opt-level=0`. Peak stack use is therefore a multiple of 301 KiB, and tests that build two devices (or one plus a savestate clone, e.g. `savestates.rs:324`) multiply it again. 3. `libtest` runs each test on a **spawned thread**, whose Rust default stack is **2 MiB** -- not the 8 MiB main-thread stack. Bracketed by measurement on the failing test: ``` RUST_MIN_STACK=4MiB -> OVERFLOW RUST_MIN_STACK=8MiB -> PASS RUST_MIN_STACK=16MiB -> PASS ``` **The repo already knows the number.** `.cargo/config.toml` ships: ```toml [target.'cfg(target_os = "windows")'] rustflags = ["-C", "link-arg=/STACK:8388608"] # 8 MiB ``` That 8 MiB link-arg sizes the *built executable's main thread*. Nothing gives libtest's *test threads* the same budget, on any platform. The measured requirement (>4, <=8 MiB) matches the figure the project already chose. Confirms it is a large-frame problem and **not** runaway recursion: raising the stack alone makes the identical test pass unchanged. ## Fix Pick one; (a) is the minimum that unblocks the cluster, (b) is the durable one. State which you took and why. **(a) Give test threads the stack the project already standardises on.** Export `RUST_MIN_STACK=8388608` in `scripts/gate.sh` around step 3 (and the same in the CI test job, if one runs `cargo test`). Cheap and honest, but it only fixes runners that go through those two paths -- a bare `cargo test --lib` still aborts. **(b) Make the tests independent of the ambient stack.** A shared test helper that runs the body on an explicitly sized thread: ```rust fn with_big_stack<F: FnOnce() + Send + 'static>(f: F) { std::thread::Builder::new() .stack_size(8 * 1024 * 1024) .spawn(f).unwrap().join().unwrap(); } ``` Correct under any runner, no env var required. **(c) Out of scope for this issue, but note it:** removing the stack temporary in `Device::new` (field-by-field init into `Box::new_uninit`, or boxing the large sub-structs) would fix the class rather than the symptom. That is an invasive change to a hot upstream-shared file and should be its own issue if anyone wants it -- **do not attempt it here.** ## Acceptance criteria 1. `./scripts/gate.sh` from a clean checkout of `main` **with !15 merged in** exits 0, and step 3 reports a non-zero passing test count -- quote the `test result:` line. A run reporting `0 passed` does not satisfy this. 2. The same holds for `main` + !18. 3. `cargo test --lib` executed **directly**, with no wrapper and no env var set by the caller, does not abort. (Fix (a) alone fails this; say so explicitly if you choose (a) anyway.) 4. Add one test that would fail if the mitigation were removed -- e.g. a test constructing two `Device`s -- so this cannot silently regress once step 3 has real content. 5. Quote `size_of::<Device>()` from your own run; if it has grown past ~700 KiB the 8 MiB figure needs revisiting. ## Explicitly OUT of scope - Changing `Device`'s layout or `Device::new`'s signature (see (c)). - Anything in `vendor/ggrs/` -- the vendored fork is byte-identical across !9/!10/!11/!18 (tree `816fce1d`) and is not implicated. - The mutual merge conflicts between the netplay PRs. That is a separate sequencing problem and does not belong in this fix. - `-C target-cpu` / portability (#16, #20) -- unrelated. ## Coordination This should land **before** !15/!13/!9/!18, on a branch off current `main` touching only `scripts/gate.sh` and/or a test helper, so it does not collide with the netplay cluster's contended files (`src/netplay.rs`, `src/savestates.rs`, `src/device.rs`). !14 and !21/!22 are unaffected and can land independently.

pbwraith added the

p1

rust

labels 2026-08-26 08:38:20 +02:00

pbwraith referenced this issue

2026-08-26 08:39:55 +02:00

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

pbwraith referenced this issue

2026-08-26 08:39:58 +02:00

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

pbwraith referenced this issue

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