Repository navigation
test(tui): fix the tests that share process-wide state and fail at random (plan decision 10) - #28
Merged
zy1233 merged 15 commits intoOct 20, 2026
Conversation
Four tests of the pager's effects set GROK_HOME to a temporary directory and never put it back; two of them could also run at the same time. The folder-trust test set GROK_HOME and GROK_TEST_VERSION (the simulated release build) for the rest of the process. A test that ran later read a path that was gone, or saw a release build. The four tests now take the serial key of the other GROK_HOME tests, and each override is a guard that restores the old value before the temporary directory is removed.
…config tests lock config/tests.rs protects GROK_SUBAGENTS, GROK_RESPECT_GITIGNORE and the GROK_*_MODEL overrides with its own mutexes. The #[serial] tests of agent/config_tests.rs clear the same variables under the serial_test lock. The two locks do not exclude each other, so a serial test could remove GROK_SUBAGENTS while a config test had set it to 1 (config::tests::subagents_config_models_with_env_var_enables failed in 1 of 10 runs with shuffled order and 32 test threads). The three mutexes move to config/test_env_locks.rs, and clear_runtime_env_vars() now holds all three until the test ends.
…rite The trace sink is process-wide. A test that does not hold the MEMTRACE_SINK serial key can write a line into it while sample_events_are_valid_jsonl_and_rotate runs: closing an agent writes a purge line. The test failed on that line in 3 of 32 runs with shuffled order and 32 test threads. It now checks the sample shape on the sample lines only and wants at least one. crash_sample_lands_even_when_the_trace_directory_is_missing parsed the whole file as one line; it now counts the crash lines.
dropping_the_guard_silences_the_heartbeat_before_anyone_else_can_hold_the_lock took the lock again at once after the guard dropped and failed with WouldBlock in 1 of 20 runs with 32 test threads. flock belongs to the open file description, and a child that another test forks keeps a copy of the descriptor until it execs. The test now retries the second lock for up to 5 s.
scripts/tui_envspy/ finds the tests of the TUI tree that share process-wide state. envspy.sh runs cargo test with an LD_PRELOAD shim over getenv, setenv, unsetenv, putenv and chdir, and analyze.py lists the tests that change a variable while another test runs, leave it changed, or read what another test has changed (a chaos mode widens the race window). stress.sh runs the lib tests of one package many times with a shuffled order and many test threads. scripts/tests/test_tui_envspy.py tests the analyzer. AGENTS.md says where the tools are and that the vendored tests expect cargo nextest.
With ENVSPY_TAG=<tag>, stress.sh starts every run through the env spy, so the report lists the tests that change an environment variable at the same time in the random orders, also when no test fails. The README says that chaos mode found no failure on this tree (it widens a window only between tests that already overlap), and AGENTS.md says that the locks of the vendored tests are partial because they were written for nextest.
Words after `--` now go to the test binary as filters. With two filters and --test-threads 2, the script runs one pair of tests many times. This shows which test breaks which: the first test fails beside the second and never beside an unrelated test.
stress.sh ran every test of the lib of a package. pi-acp-lib has a test that hangs forever, and the manifest skips it (`skip` of the suite), so the first run of that package never ended. The script now reads the `skip` list of the suite in scripts/tui_baseline.toml and gives each name to the test binary as --skip, as tui_baseline.py does.
…imal-mode tests
Some tests of the pager change a process-wide static: the colour level, the native palette, the minimal-mode flag. Every test that reads the static while the change is in place can fail, and there are hundreds of readers. A lock helps only when every reader takes it.
in_own_process(|| { ... }) in test_util.rs runs the body of a test in a child process. The test starts the same test binary again with `--exact <name> --test-threads 1` and an environment variable; the child runs the body, the parent does not. The parent fails when the child does not pass exactly one test, and it prints the output of the child. Two self-tests check that a change stays in the child and that a failure in the body fails the parent.
The three tests that set the colour level, the native palette or the minimal-mode flag now run in their own process: thinking_body_dim_italic_survives_the_terminal_native_palette, reseed_round_trip_flips_every_mode_derived_gate and minimal_double_ctrl_c_arms_then_quits.
RTL_BIDI_ENABLED is a process-wide latch. The six rtl tests of the text selection turn it on for a few microseconds, and every test that calls set_appearance (more than 60 of them, none takes a lock) turns it off. The rtl tests are both the writers and the victims: beside the tests of one module that calls set_appearance, 52, 44 and 29 of 60 runs failed. with_rtl_bidi now runs the body in its own process.
…heir own process modal_window::EMBEDDED and VOICE_MODE_ENABLED are process-wide. Five tests set them for a short time. Four tests of the settings modal read the first flag, and one test that lists the settings of PR 14 reads the second; none takes a lock, so each fails when it runs beside one of the five. In a pair run of 100, the voice victim failed 100 times. The five tests now run in their own process.
The theme (theme::cache) is process-wide. 43 tests set it through two helpers, with_theme_test_env and with_test_env of the theme command, and every test that renders text reads it. The pin_theme() helper of the project takes a lock, but only the tests that call it exclude each other. In a pair run of 100, a mermaid test failed 96 times and a ghost-text test failed 92 times. The two helpers now run their closure in its own process, so each of the 43 tests has the theme to itself.
Plan section 10.20, log entry B.12, the baselines (r17, section 3.4), the CHANGELOG, AGENTS.md and the README of the env spy now say what the race check found, what changed, and what it did not cover.
…lines TUI CI ran on ea9609f: 8,490 passed, 0 failed, 21 ignored. CI passed too. The notes now say that the first test run on 6438b59 is this CI run, that 88e04e9 is a local commit that is not pushed, and that the lib time of pi-pager in CI (15.55 s) is inside the spread of earlier runs (8.21 to 14.83 s), so CI cannot tell the cost of the 57 child processes.
Two TUI CI runs (ea9609f and 7b27775) show a lib time of 15.55 s and 15.79 s for pi-pager. Five earlier runs without the isolation took 8.21 to 14.83 s. The notes said that CI cannot tell the cost. Two runs in the top of the range suggest about 3.5 s more, about 60 ms for each isolated test. The notes now say this as an estimate from a small sample.
This was referenced Oct 20, 2026
zy1233
added a commit
that referenced
this pull request
Oct 25, 2026
…to #31 (plan r19) (#33) The owner asked for three things: merge the five open PRs (#27 to #31) into main with `gh stack`, set the V0.6 milestone from the present state, and pause development for 1 to 2 days. The merge is done (stack #32, `gh stack merge 32 --yes --squash`). main goes from 7d82c27 to b33017a with five squash commits. The tree of each commit is the tree of its PR head. CI and TUI CI pass on b33017a (8,490 passed, 0 failed, 22 skipped). The plan gets a new section "发布里程碑 V0.6": the scope (the 10 commits on main after v0.5.0), six exit conditions with their state, the known issues (two are new), and what is not in V0.6. It also gets the r19 status, the pause, and the fixed status of the places that still said "waiting for merge" for #25 to #28. The execution log gets section 10.22: the install of `gh stack` (the first download was cut short), the merge, the check of the trees, CI on main, the milestone, the pause, and what is not verified. The GitHub milestone v0.6.0 (#2) is created. It has no due date and no PRs, as v0.5.0. No condition is met yet. Only documents change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Plan decision 10, follow-up. On 2026-10-19 the owner chose
race_audit: look for more tests that fail at random because they share process-wide state. #26 found one such pair inpi-shell. This PR looks for the rest and fixes what it finds. It changes tests, scripts and documents only. Production code does not change.TUI CIcannot find these races.cargo testruns all tests of one binary as threads of one process. They share the environment variables, the current directory and everystatic. The vendored tests were written forcargo nextest, which starts one process for each test, so their locks cover only a part of what the tests share.TUI CIruns 4 threads in a fixed order, and that order does not find the races. With 32 threads in random order (WSL2, 12 CPUs), 6 of 32 runs of thepi-pagerlib at the base of this PR had a failure. After this PR, 0 of 60 runs have one.The audit found 19 tests that fail at random, and 5 tests that change an environment variable and do not put it back:
GROK_*variables with two locks that do not exclude each other (a#[serial]key and aMutex)pi-shell)config/test_env_locks.rs)pi-pager)exec, so the heartbeat test getsWouldBlockwhen it locks again right after it drops its guardpi-shell)statics that tests set: colour level and native palette, minimal-mode flag, bidi latch, embedded-modal flag, voice gate, themepi-pager)GROK_HOME(one also setsGROK_TEST_VERSION) and do not restore itserial(GROK_HOME)on 4 of themWhy not a lock for the readers: the readers number in the hundreds (any test that renders text reads the theme), and each round of experiments found new ones. A first attempt with
pin_theme()fixed one test at a time and left others. The writers are 57. A test in its own process cannot reach the other tests, so every reader, found or not, is safe.in_own_process(|| { ... })(new,pi-pager/src/test_util.rs) runs the body of a test in a child process. The parent starts the same test binary with--exact <name> --test-threads 1, waits, and fails unless the child exits with 0 and reports1 passed. A failure in the child fails the parent, and the message holds the output of the child. The cost is about 30 ms for each test.Base:
refactor/tui-session-store-leftovers(#27,739d2bb). Not merged. #27 is not merged either.Read
test_util.rsandconfig/test_env_locks.rsfirst. In the other test files, the change moves a test body into the closure, sogit diff -wshows far less.Evidence
All runs are on WSL2 (12 CPUs). "Fails" means that a run has at least one failed test. Each cell is runs with a failure / runs.
pi-pagerlib unless noted)739d2bb)pi-shell, 32 threads, random order, seeds 1-20views::14 / 60,app::9 / 60,scrollback::7 / 60 before; 0 / 60 for each after. The 3rtl_*tests that failed, with three groups of tests that callset_appearance: 52, 44 and 29 / 60 before. After, I ran only the first group again: 0 / 60.ENVSPY_TAG; 5 runs ofpi-pagerand 5 ofpi-shell, random order, 32 threads): 0 overlapping changes, 0 leaks, 0 variables still changed at exit. One run of the wholepi-pagerlib before the fix of the 5 tests (default order, 12 threads): 3 overlaps, 1 leak, 2 variables still changed at exit./bin/truein a loop. 363 of 1,500 loops fail before the fix and 0 of 1,500 without the storm. The fixed test passes 5 of 5 under the storm.in_own_processitself. The 2 self-tests pass. I also broke one isolated test on purpose, in a clone:set_embedded(true)becameset_embedded(false)inembedded_fills_area_without_centering, so its assertion fails in the child. The parent test failed, and its message held the output of the child. I restored the clone.scripts/tui_baseline.py test, WSL2, clean environment, incremental build, about 4 m 56 s) on88e04e9: 8,490 passed, 0 failed, 21 ignored.88e04e9is a local commit from before I put the commits in order. It is not in this PR. Thetui/tree of the last code commit (6438b59) differs from it in 2 places intest_util.rs: a blank line, and the format string of one panic message. I did not run the tests on6438b59(see "Not verified"). The lib tests ofpi-pager: 5,377 -> 5,379 (the 2 self-tests). With 4 threads on my machine, they take 27.2 to 28.0 s in 3 runs (28.8 s before): the 57 child processes add no time that these runs can show. In CI the lib is slower; see "Cost in CI" below.6438b59.scripts/tui_baseline.py checkpasses: 0 warnings, 970 packages in the dependency graph,leader243 andx.ai297 (both at the ceiling),cargo check --workspace --testsexit 0.rustfmt --checkon the 20 changed Rust files: no new hunk (rustfmt 1.8.0, edition 2024).6438b59.ruff check .andruff format --check .pass. Fullpytest(real_llmdeselected): Windows 2,569 passed, 39 skipped, 31 deselected; Linux (WSL2) 2,578 passed, 30 skipped, 31 deselected. Both include the 10 new tests.ea9609fand on the last commit7b27775.7b27775changes documents only, butTUI CIruns again because its path filter looks at the whole PR. Both runs ofTUI CI(second) pass: 8,490 passed, 0 failed, 21 ignored, and every suite equals itsreference_passed. The gates give the same numbers as my run (970 packages, 0 warnings,leader243,x.ai297). Thecargo testjob took 22 m 04 s and 22 m 43 s.CI(second) passes too (ruff,pytest3.11, 3.12 and 3.13).TUI CIruns 4 threads in a fixed order, so it cannot find these races: a pass does not show that they are gone.pi-pagertook 15.55 s and 15.79 s in the two runs (libtest time). Five earlier runs without the isolation took 8.21 to 14.83 s (mean 12.1 s). That is about 3.5 s more, or about 60 ms for each isolated test, against 30 ms on my machine. Two runs are a small sample (the earlier runs differ by up to 6.6 s), so this is an estimate. The wholecargo testjob takes about 22 minutes. A docs-only commit that I add after this text has a CI run that this text does not list.6438b59on my machine, andcargo clippy. I wanted to run the whole manifest, the 60 runs, the 2 self-tests and a negative check on6438b59. The disk that holds my WSL2 disk file was full (that file only grows), WSL stopped, and one run ended withBus error. I freed the space, but did not run the tests again. The first test run on this code is the CI above.cargo clippygave no result in the same incident, so it was not run on the last code commit;TUI CIdoes not run it.set_show_thinking_blocks69,set_group_tool_verbs20, and so on). I did not isolate them. No pair or population run found a victim of them.blank_quote_line_survives_drag_copy_as_blankfailed once and did not repeat.command_palette_search_bar_cursor_only_when_focusedfailed in 1 of 60 runs before the isolation, and not after. I suspect the vim-mode setting.GROK_HOME,GROK_TEST_VERSIONorGROK_AUTH_EARLY_INVALIDATION_SECSwhile another test changes them (6 groups inpi-pager, 25 inpi-shell). None of them failed in the runs above. I did not read each group.serialkeys ofpi-shellthat the r16 notes list: no test readGROK_AUTH_FORCE_DARK_WAKEwhile it changed (5 probe runs).heap_profile_hookshas one test. Forattribution_emit_count, I did not check which tests reach the counter from outside.Not in this PR (found, not done)
These need the owner to decide.
cargo nextestinTUI CI. One process for each test removes this whole class of races, also for tests that someone writes later. In 16 of 16 runs onaa76df4(before the isolation), nextest had no failure. The price: the lib tests ofpi-pagertake 79.5 s instead of 29.7 s with 4 threads (wall time). nextest does not run doctests, socargo test --docstays. It changes the CI and the baseline runner, so I did not do it.protoc. Whenfind_protoc()findsprotoconPATH, it returns the bare name.pi-proto-buildthen printscargo:rerun-if-changed=protoc. That path does not exist, so cargo rebuildspi-tools-apiand the crates above it on each call. InTUI CIrun 39193752451, onecargo testjob compilespi-tools-api7 times. Locally, an absolutePROTOCavoids it. Three fixes are possible: return an absolute path infind_protoc, print norerun-if-changedfor a bare name, or setPROTOCintui-ci.yml.load_mcp_servers/util/config/mcp.rs, andpi-pager/src/recent_dirs.rs): the owner did not choose them.Merge Danger
Door: two-way
The change adds tests, scripts and documents, and changes test code. A revert brings back the old tests and the old races. No product code, data or wire format changes.
Blast Radius: the test suites of
pi-pagerandpi-shell,scripts/, and the baseline manifestpi-pagereach start one more process (about 30 ms each on my machine, about 1.7 s in total; the two CI runs suggest about 60 ms each, about 3.5 s in total). If the child cannot start, the test fails with the reason. If the test thread has no name that holds::(a test that does not run in libtest), the body runs in the same process and there is no isolation. The self-testa_change_in_the_body_stays_in_the_childfinds that case.cargo nexteststill works: each test is then already in its own process, and the child runs the body once more.reference_passedis 8,490 (it was 8,488).min_passeddoes not change.main. If refactor(tui): delete the code that the session-store removal left unused in pi-shell (plan decision 10) #27 is squashed, mergemaininto this branch withgit merge -s ours(the trees are equal), as was done for refactor(tui): delete the session store of pi-shell (plan decision 10) #26.