test(daemon-state): isolate the states dir so a parallel daemon's prune can't delete test files - #131
Conversation
…ne can't delete test files The daemon-state tests write state files for synthetic DEAD pids, and the dist module resolves DAEMON_STATES_DIR once, at import time, from env-paths' data dir. Left at its default that dir is machine-global: every daemon startup prunes dead-pid files there (daemon.ts -> pruneStaleStates), so a daemon started by any test file running in parallel deleted these files mid-test. Windows CI, being slower, lost the race and failed "should write multiple state files" with `expected [ 9999903 ] to include 9999902` — pid1's file pruned between the two writes and the read. Point HOME / XDG_DATA_HOME / LOCALAPPDATA / APPDATA at a tempdir before importing the dist module (env-paths reads a different one per platform). PKC_DATA_PATH and DAEMON_STATES_DIR are both computed eagerly at module load, so the override has to precede the import — hence the dynamic imports. daemon-supervisor.test.ts had the same hazard and gets the same isolation. The regression test reproduces the race deterministically: it writes a state file, then runs pruneStaleStates() in a spawned process carrying the real, un-isolated env — exactly what a daemon starting in a parallel test file does — and asserts the file survives. Red before the isolation (`expected [] to include 9999902`), green after. Refs #130
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe tests now isolate daemon state in temporary platform-specific data directories before importing eagerly initialized modules. A child-process regression test runs stale-state pruning against the shared directory and confirms that isolated test state remains available. ChangesDaemon test isolation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change isolates daemon test state files to prevent parallel tests from deleting one another’s fixtures; it is merge-ready after normal checks, with no actionable merge-blocking risk remaining. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
test/common-utils/daemon-state.test.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc test/common-utils/daemon-supervisor.test.tsOops! Something went wrong! :( ESLint: 8.27.0 Error: ESLint configuration in --config » eslint-config-oclif is invalid:
Referenced from: /.eslintrc Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
1 participant
Closes #130
Problem
Windows CI on #129 failed a test that branch does not touch:
Ubuntu and macOS passed. Note the actual array: pid2's file is there, pid1's is gone. Nothing failed to write — something deleted pid1's file in the 76 ms between
writeDaemonState(pid1)andreadAllDaemonStates().Root cause
src/common-utils/daemon-state.ts:9resolves the states dir once, at import time, from env-paths' data dir:The test imports that dist module in-process without overriding the env, so — unlike
update-install-restart-race.test.ts/update-install-systemd-aware.test.ts, which overrideHOME/XDG_DATA_HOMEfor the children they spawn — its state files land in the machine-global.daemon_statesdir every daemon shares. And the pids it writes (FAKE_PID_BASE = 9999900) are deliberately dead.Meanwhile
fileParallelism: trueruns other files concurrently, and every daemon startup prunes that shared dir:src/cli/commands/daemon.ts:349→pruneStaleStates()→getAliveDaemonStates(), which unlinks the state file of any pid that is not alive. A synthetic9999xxxpid is exactly that.The log timing confirms the overlap: when the assertion fired at 06:42:56,
daemon.test.ts(finished 06:43:11) andchallenge.test.ts(06:43:00) were both mid-flight starting daemons. Windows-only in practice because it is ~3-4x slower there, which widens the window; the race is latent on Linux and macOS too.Distinct from #94 / #75, which hardened the daemon against losing the prune race and skipped the pid-reuse tests on Windows — neither stopped these tests from being pruned out from under themselves.
Changes
daemon-state.test.ts— pointHOME/XDG_DATA_HOME/LOCALAPPDATA/APPDATAat a tempdir (env-paths reads a different one per platform:XDG_DATA_HOMEon linux,HOMEon macOS,LOCALAPPDATAon windows). SincePKC_DATA_PATHandDAEMON_STATES_DIRare both computed eagerly at module load, the override has to precede the import — so the two dist imports become dynamic. The type-only import stays static (erased at compile time).daemon-supervisor.test.ts— same hazard at itswriteDaemonStateround-trip tests (fake pids 9999700/9999701 in the shared dir), same isolation.Tests
keeps its own state file when another daemon prunes the shared states dirreproduces the race deterministically instead of hoping to catch it: it writes a state file, then runspruneStaleStates()in a spawned process carrying the real, un-isolated env — exactly what a daemon starting in a parallel test file does — and asserts the file survives.expected [] to include 9999902It is also a standing guard: reverting the isolation makes it fail deterministically on every platform, not just under a lost race on Windows.
The captured real env is stored before the override;
child_processdrops env keys whose value isundefined, so a var that was never set stays unset in the child. Running the pruner against the shared dir is safe — it removes only dead-pid files, the same best-effort cleanup any daemon start performs.Verification
npm run build && npm run build:test— cleannpm run test:cli— 43 files, 339 passed, 1 skippedSummary by CodeRabbit