lacs-project / lacs-project/sysknife
cargo test fails intermittently on main: a test sets a process-global env var
- Dominant language
- Rust
- Stars
- 12
- Forks
- 19
- Avg merge
- 18h 57m
- Merged PRs (30d)
- 116
Description
`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-09 at `5673d20`.** Read this block first; every figure
> further down was taken at an older commit and is kept for history. #384 moved
> both panic sites, which are now `mcp_server.rs:2196:10` and
> `mcp_server.rs:2212:9`. One run of the command selects 268 tests rather than
> 267, and `cargo nextest run --workspace --locked` reports
> `1851 tests run: 1851 passed, 6 skipped`; neither number means you broke
> something. I ran the command once today and it passed, which settles nothing
> either way, so the reproduction figure stands at five failures in forty runs.
>
> ```
> $ git rev-parse --short=8 HEAD
> 5673d20f
> $ grep -n 'direct read-only query over socket' apps/sysknife-cli/src/mcp_server.rs
> 2196: .expect("direct read-only query over socket");
> $ sed -n '2212p' apps/sysknife-cli/src/mcp_server.rs
> assert!(
> $ grep -c ENV_LOCK apps/sysknife-cli/src/mcp_server.rs
> 0
> $ 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:
```sh
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](https://github.com/lacs-project/sysknife/blob/main/CONTRIBUTING.md), and [docs/developer-guide.md](https://github.com/lacs-project/sysknife/blob/main/docs/developer-guide.md) covers why this project uses nextest. No CLA. MIT.
Contributor guide
Research direction
Start in apps/sysknife-cli/src/mcp_server.rs at mcp_tools_integrate_with_a_daemon_over_the_socket and trace resolve_socket_target(), then read the ENV_LOCK precedent in apps/sysknife-cli/src/runner.rs:3079. Compare the two scoped options in the issue and grep test code for other set_var callers. Done means cargo test -p sysknife-cli --bins passes across 40 repeated runs without relying on nextest.
Written by the indexing model from the issue text.
Assessment
- Tech stack
- rust
- Domain
- cli, testing-qa
- Issue type
- Bug
- Difficulty
- 3/5
- Estimated time
- 1-2 days
- Activity status
- Active
- Clarity
- Clearly specified
- Newbie friendliness
- 74/100