cargo test fails intermittently on main: a test sets a process-global env var

[vladimirrott]

cargo test -p sysknife-cli --bins fails intermittently on main. cargo nextest run --workspace --locked always passes, which is why CI never sees it.

Re-measured 2026-09-21 at fab9053f. Read this block first; every figure
further down was taken at an older commit and is kept for history. Both panic
sites have moved again since the 2026-09-09 pass at 5673d20: they are now
mcp_server.rs:2198 and mcp_server.rs:2214, where they were 2196 and
2212. The columns are not re-measured here, so go by the line. One run of the
command selects 268 tests, and cargo nextest run --workspace --locked reports
1861 tests run: 1861 passed, 6 skipped rather than the 1851 this block used
to say; neither number means you broke something. I ran the reproduction
command once today and it passed, which settles nothing either way, so the
figure stands at five failures in forty runs.

$ git rev-parse --short=8 HEAD
fab9053f
$ grep -n 'direct read-only query over socket' apps/sysknife-cli/src/mcp_server.rs
2198:        .expect("direct read-only query over socket");
$ grep -n 'the rejection reason must reach the caller' apps/sysknife-cli/src/mcp_server.rs
2216:            "the rejection reason must reach the caller"
$ grep -c ENV_LOCK apps/sysknife-cli/src/mcp_server.rs
0
$ sed -n '3079p' apps/sysknife-cli/src/runner.rs
    static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());
$ out="$(cargo test -p sysknife-cli --bins 2>&1)"; rc=$?
$ n=$(printf '%s' "$out" | sed -n 's/^running \([0-9]*\) test.*/\1/p' | tail -1)
$ echo "rc=$rc selected=${n:-PARSE-FAILED}"
rc=0 selected=268

Option 2 in Scope already exists one file over. apps/sysknife-cli/src/runner.rs:3079
declares an ENV_LOCK mutex whose doc comment says every test calling
set_var or remove_var must hold it for the full duration of the env read.
mcp_server.rs holds nothing. Read that precedent before picking between the
two options.

Forty consecutive runs on a Linux host, five of them red, measured 2026-09-07
at adab560, every failure the same test at mcp_server.rs:2176:10, which is
the line number that commit had:

$ fails=0; runs=0
$ for i in $(seq 1 40); do
    out="$(cargo test -p sysknife-cli --bins 2>&1)"; rc=$?
    runs=$((runs+1))
    n=$(printf '%s' "$out" | sed -n 's/^running \([0-9]*\) test.*/\1/p' | tail -1)
    if [ "$rc" -ne 0 ]; then
      fails=$((fails+1))
      printf '%s' "$out" | grep -E 'panicked at apps/sysknife-cli/src/mcp_server.rs' | head -1
    fi
  done
thread 'mcp_server::tests::mcp_tools_integrate_with_a_daemon_over_the_socket' (3911343) panicked at apps/sysknife-cli/src/mcp_server.rs:2176:10:
runs=40 fails=5 tests_selected_last_run=267

Re-measured 2026-09-03 at 60af0ad. The original figures were taken at 679b594 and no longer describe what you will see. Two things changed: #348 landed and moved the assertion that trips first, and the failure rate is far lower than the first table suggested. Read the table below, not the one in the edit history. The bug itself is unchanged.

Re-measured on main at 60af0ad, on a Linux host, same tree each time:

Command	Runs	Result
cargo test -p sysknife-cli --bins	54	9 failed, ≈17%
cargo test -p sysknife-cli --bins -- --test-threads=1	5	5 passed, 267 tests each
cargo nextest run --workspace --locked	1	1837 passed, 6 skipped

Every failure is the same test, mcp_server::tests::mcp_tools_integrate_with_a_daemon_over_the_socket, and it lands on one of two assertions inside it:

panicked at apps/sysknife-cli/src/mcp_server.rs:2178:10
  .expect("direct read-only query over socket")

panicked at apps/sysknife-cli/src/mcp_server.rs:2194:9
  the rejection reason must reach the caller

Both panics above are output from 60af0ad. The two sites moved up two lines
when #360 deleted a three-line constant and added one import, so at
7d19864 you will see mcp_server.rs:2176:10 and mcp_server.rs:2192:9
instead. The only change to the file was one added import and one deleted
constant, both far above the test:

$ git diff --stat 60af0adb 7d19864 -- apps/sysknife-cli/src/mcp_server.rs
 apps/sysknife-cli/src/mcp_server.rs | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)
$ git show 7d19864:apps/sysknife-cli/src/mcp_server.rs | grep -n 'direct read-only query over socket'
2176:        .expect("direct read-only query over socket");
$ git show 7d19864:apps/sysknife-cli/src/mcp_server.rs | sed -n '2192p'
        assert!(

Expect to need patience reproducing it. In one batch of 8 consecutive runs it never fired; in a batch of 25 it fired 5 times. A single green run tells you nothing.

Why it matters

The test calls std::env::set_var("SYSKNIFE_SOCKET", ...) and then makes several await calls that each read it back through resolve_socket_target(). set_var is process-global. cargo test runs a binary's tests as threads in one process, so any other test that touches the same variable, or that runs while this one is between calls, changes the answer underneath it. The first call in the test succeeds and a later one falls back to the default path.

nextest gives each test its own process, so the variable cannot be shared and the race cannot happen. Both our CI and scripts/ci-local.sh use nextest, so this is invisible to the project and visible to every contributor who types the more obvious command. It cost a contributor time on #348 before I traced it, and the failure looks like their change because it names a function their PR touched.

Rust made std::env::set_var unsafe in the 2024 edition for this reason. This workspace sits on edition = "2021" (Cargo.toml:15), which is why these two calls compile without an unsafe block and why the compiler says nothing.

Scope

Remove the process-global dependency from the test rather than serialising the suite. Options, in the order I would try them:

1. Thread the socket target through the call rather than through the environment, if resolve_socket_target() can take an override argument at these call sites.
2. Failing that, give the affected tests a shared mutex and set and restore the variable inside it, so at least they cannot race each other.

--test-threads=1 in a config file is not a fix. It hides the race and slows the suite for everyone.

Tests first

Run the target test alongside the rest, in one process, repeatedly:

for i in $(seq 1 40); do cargo test -p sysknife-cli --bins || echo "FAILED on run $i"; done

Forty green runs is the bar, raised from twenty because at ≈17% a twenty-run batch comes back clean about 2% of the time by luck alone. Before the fix, expect roughly seven failures in forty. Do not use nextest to check the fix; it passes either way, which is the whole reason this survived.

Also grep for other set_var callers in test code and say in the PR whether any of them share a variable with this one.

Difficulty

medium. The diagnosis is done and written above; the work is deciding between the two options and not accidentally papering over it.

Getting started

CONTRIBUTING.md, and docs/developer-guide.md covers why this project uses nextest. No CLA. MIT.

[vladimirrott]

Re-measured this at 60af0ad and corrected the body. The bug is unchanged; two of the numbers describing it were wrong, and this issue gets read by people arriving without repo context.

What changed:

• The panic I quoted came from code that did not exist at the 679b594 the body named. direct read-only query over socket arrived with feat(mcp): expose direct read-only actions #348, so that output cannot have come from the SHA I wrote.
• "4 of 4 runs failed" overstates it badly. At 60af0ad I got 9 failures in 54 runs of cargo test -p sysknife-cli --bins, about 17%. One batch of 8 came back entirely clean.
• The test now trips on either of two assertions depending which await loses the race: mcp_server.rs:2178 and mcp_server.rs:2194.

I raised the reproduction bar from twenty runs to forty. At 17% a twenty-run batch comes back clean about 2% of the time on luck alone, and somebody would reasonably have concluded the race was gone.

Still true: -- --test-threads=1 passes, 5 for 5 at 267 tests, and cargo nextest run --workspace --locked is 1837 passed with 6 skipped. Nextest passes either way, which remains the reason this survived in the tree.

[vladimirrott]

Re-checked at b2c823e6. The diagnosis holds, the race is untouched, and every line number in the 2026-09-09 block has moved by two.

$ git log --oneline 5673d20..HEAD -- apps/sysknife-cli/src/mcp_server.rs
08c08ef feat(network): report nftables and firewall backend observations (#413)
$ grep -n 'direct read-only query over socket' apps/sysknife-cli/src/mcp_server.rs
2198:        .expect("direct read-only query over socket");
$ sed -n '2212,2217p' apps/sysknife-cli/src/mcp_server.rs
            "a daemon-rejected receipt must surface as an MCP error, got: {result:?}"
        );
        assert!(
            result.unwrap_err().contains("stale_approval"),
            "the rejection reason must reach the caller"
        );

So the two panic sites are mcp_server.rs:2198 and mcp_server.rs:2214. #413 inserted two lines above them and nothing else in this file changed.

The shape the issue is about is exactly as described:

$ grep -c ENV_LOCK apps/sysknife-cli/src/mcp_server.rs
0
$ grep -n 'set_var("SYSKNIFE_SOCKET"' apps/sysknife-cli/src/mcp_server.rs
2172:        std::env::set_var("SYSKNIFE_SOCKET", sock.to_str().unwrap());
2238:        std::env::set_var("SYSKNIFE_SOCKET", sock.to_str().unwrap());
$ sed -n '3076,3079p' apps/sysknife-cli/src/runner.rs
    /// Serialize env-var mutations so concurrent tests do not race on
    /// `SYSKNIFE_SOCKET`.  All tests that call `set_var` / `remove_var` must
    /// hold this lock for the full duration of the env read.
    static ENV_LOCK: std::sync::Mutex<()> = std::sync::Mutex::new(());

Two files set the same process-global variable, one of them takes the lock written for it, and the other does not. Option 2 in Scope is still the shortest route and still has its precedent one file over.

One more figure in that block is stale. It quotes a workspace baseline of 1851:

$ cat tests/evidence/workspace-tests.json
{
  "commands": {
    "frontend_tests": "vitest run (apps/sysknife-shell)",
    "tests": "cargo nextest run --workspace --locked"
  },
  "frontend_tests": 72,
  "tests": 1861,
  "version": 2
}

I did not re-measure the failure rate or the 268 selected tests. Both want repeated full builds of the workspace, and I would rather say that than quote the old numbers as though I had. The ≈17% in the title is from the September measurements and stands unrechallenged rather than reconfirmed.

[vladimirrott]

@QinXi-ai offering you this one, and it is the first of five I am putting in front of you today. Take any subset in any order; five is what the pool has, not a queue I expect you to clear. Say no to all of them and nothing is owed.

This one is the flaky cargo test on main. A test sets a process-wide environment variable and the parallel runner lets a neighbour read it mid-flight, so the suite fails on a different assertion each run and everyone learns to re-run until green. That is the habit that waves a real failure through.

You are the right person for it because of #382 and #360: both were about a boundary that behaved differently depending on what else was in flight, and you fixed the boundary rather than the symptom. The fix here is the same shape, and the decision is whether the variable should exist at all or whether the test needs its own process.

Worth knowing before you start: reproducing it wants repetition, not luck. Run the suite single-threaded first to confirm the failure disappears, then bisect toward the pair that collide. A fix proved by one green run has proved nothing.

[vladimirrott]

Correcting a number I published on this thread earlier today. The second panic site is mcp_server.rs:2214, not 2210.

My block this evening verified it with sed -n '2210p', which printed assert!( and looked like confirmation. The file has two assert!( lines four apart, so that command cannot tell them apart:

$ sed -n '2210p;2214p' apps/sysknife-cli/src/mcp_server.rs
        assert!(
        assert!(

Anchoring on the message instead separates them:

$ grep -n 'direct read-only query over socket\|the rejection reason must reach the caller\|a daemon-rejected receipt must surface' apps/sysknife-cli/src/mcp_server.rs
2198:        .expect("direct read-only query over socket");
2212:            "a daemon-rejected receipt must surface as an MCP error, got: {result:?}"
2216:            "the rejection reason must reach the caller"

2210 opens the result.is_err() assertion whose message is at 2212. 2214 opens the stale_approval assertion whose message is at 2216, and that is the one this issue has always meant. My 16 September comment had it right at 2214 and I overwrote a correct number with a wrong one that verified against a non-discriminating command.

Go by the message lines, :2212 and :2216. They cannot collide the way the bare assert!( lines do.

The race itself is untouched:

$ grep -c ENV_LOCK apps/sysknife-cli/src/mcp_server.rs
0

runner.rs:3079 still declares the mutex that mcp_server.rs never takes. I have not re-measured the five-in-forty failure rate or the 268 selected tests; both need repeated full builds, and quoting the old figures as though I had re-run them is the thing I just did wrong above.

[QinXi-ai]

Thanks for the invitation. I'll take this one. I'll first reproduce with the single-process cargo test runner, compare single-threaded runs, and inspect all SYSKNIFE_SOCKET mutations before choosing the fix. I'll report the before/after run counts, including the requested forty parallel runs after the fix.

[QinXi-ai]

Implementation: #470. On the same 271-test CLI binary, the baseline passed 5/5 single-threaded runs but failed 14/40 ordinary parallel cargo test runs. After removing both MCP SYSKNIFE_SOCKET mutations and injecting the socket/client, the same 5 single-threaded and 40 parallel runs all pass. Both run links are in the PR, and upstream CI passes. No test count changes or suite-wide serialization.