Conversation
Rust spawns threads with a 2 MiB stack; the Linux main thread gets 8 MiB. The bootstrap runtime raises this for the server, but test runtimes are built by the tokio test macro and keep the default, so debug-profile runs of the native and wire suites abort with "thread ... has overflowed its stack" (2 MiB: 6/7 abort; 4 MiB and above: clean). Set RUST_MIN_STACK for every process cargo runs, so a local run needs no exported environment variable.
farhan-syah
left a comment
There was a problem hiding this comment.
Request changes.
Blocker: .cargo/config.toml is not committed in this repository. .gitignore ignores .cargo/ because that file is each contributor's local override ([build] jobs, [patch]). Tracking it makes every existing local copy a merge conflict on the next pull and turns every local edit into a permanent modification. git add -f around the ignore rule is not a fix. Remove the file from the PR.
Where the floor lives instead. CI already sets it: .github/workflows/test.yml exports RUST_MIN_STACK: "33554432" (32 MiB) for the test job, with the reason in the comment above it. The local run must match CI, so the change is one line in CONTRIBUTING.md, in the "Running tests" block next to cargo nextest run --all-features:
RUST_MIN_STACK=33554432 cargo nextest run --all-featureswith one sentence on why (debug-profile stack depth, same value CI uses). No config file, no ignore change.
False claim, in the body and in the committed comment. "CI passes at the default because the ci profile has smaller frames" — CI never runs at the default. It runs at 32 MiB (test.yml, RUST_MIN_STACK).
Wrong value. 8 MiB is below the margin the tree documents: nodedb-test-support/src/core_loop_runner.rs states the OS-default 8 MiB blocking-thread stack overflows in debug builds and 32 MiB is sufficient. test.yml chose 32 MiB for that reason. The PR's ladder covers two tests.
Wording. RUST_MIN_STACK is not a floor. It replaces the default size for every std::thread::spawn that sets none, on every platform.
Checked statically against the branch ref. The steps-to-test were not re-run.
| # raises this for the server, but test runtimes are built by `#[tokio::test]` | ||
| # and keep the default. Give every process cargo runs the main-thread size, so | ||
| # `cargo test`, `cargo nextest run` and `cargo run` behave the same without an | ||
| # exported RUST_MIN_STACK. CI already passes at the default under the `ci` |
There was a problem hiding this comment.
False claim. CI does not pass at the default. .github/workflows/test.yml sets RUST_MIN_STACK: "33554432" for the test job. This line is wrong on arrival. Moot once the file goes: see the review body.
| # exported RUST_MIN_STACK. CI already passes at the default under the `ci` | ||
| # profile; this is for the debug profile every contributor runs. | ||
| [env] | ||
| RUST_MIN_STACK = "8388608" |
There was a problem hiding this comment.
Wrong value. nodedb-test-support/src/core_loop_runner.rs documents 8 MiB as insufficient in debug builds and 32 MiB as sufficient. CI runs at 33554432. Local must match CI. Moot once the file goes: the value moves to the CONTRIBUTING.md command line.
A tracked .cargo/config.toml is not this repository's convention: .gitignore ignores .cargo/ because that file is each contributor's local override ([build] jobs, [patch]). Tracking it turns every existing local copy into a merge conflict on the next pull. The value moves to the documented command line instead.
RUST_MIN_STACK replaces the default stack size for every thread that sets none, on every platform. Debug-profile test binaries overflow the 2 MiB default; 32 MiB is the value CI already sets in .github/workflows/test.yml.
|
Addressed and pushed (
No red-proof test: documentation-only change, stated under "Exemption" in the body. |
Why
Rust spawns threads at 2 MiB. Debug-profile test binaries overflow that default: at the default the native suite aborts with
SIGABRT(measured on this branch), at 32 MiB it passes. CI already carries the fix —.github/workflows/test.yml:135setsRUST_MIN_STACK: "33554432"for the test job, matching the marginnodedb-test-support/src/core_loop_runner.rs:7documents — and a local run without the variable does not..cargo/config.tomlwas the first attempt and is removed per review:.gitignore:111ignores.cargo/because that file is each contributor's local override ([build] jobs,[patch]); tracking it turns every existing local copy into a merge conflict. The value lives on the documented command line instead.What changed
CONTRIBUTING.md, "Running tests" block — one comment sentence and the command:No config file. No product change.
Wording
RUST_MIN_STACKis not a floor. It replaces the default stack size for everystd::thread::spawnthat sets none, on every platform. 32 MiB matches the value CI sets.Steps to test
env -u RUST_MIN_STACK cargo nextest run -p nodedb --test native -E 'test(native_error_code_classification)'→ aborts at the default (SIGABRT, rc=100). Re-measured on this branch.RUST_MIN_STACK=33554432 cargo nextest run -p nodedb --test native -E 'test(native_error_code_classification)'→ 7 passed.RUST_MIN_STACK=33554432 cargo nextest run -p nodedb --test native -E 'test(vector_search_serializes_distance)'→ 1 passed.Exemption
No red-proof test: this change is documentation only and alters no product behaviour. No issue to close — the stack default is the task.