test: make pid-liveness fixtures deterministic under load - #1556
Merged
Conversation
Three tests classify a fixture owner's liveness via classifyOwnerLiveness (or the daemon-process equivalent), which re-reads the owner's process start time via a real `ps -p <pid> -o lstart=` shell-out with a 1s timeout. Under full-suite CPU contention that second read can miss its deadline and return null, mismatching the value captured earlier and flipping a genuinely-live owner to 'owner-process-dead'. Pin the pid->start-time (and, for the daemon-client case, pid->command) mapping to a deterministic value per test file instead of letting a second real subprocess call race the first, without weakening the dead-owner path (isProcessAlive stays real and un-mocked everywhere).
Size Report
Startup median (7 runs, lower is better):
Top changed chunks: no changes in the largest emitted chunks. |
Member
Author
|
Review verdict: clean and merge-ready at |
|
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
Three tests fail intermittently with assertions (not timeouts) during full-suite runs while passing in isolation. All three share one root cause: their fixture represents a live owner via a real pid, and the production liveness check re-reads that owner's process start time (or command line) with a second, independent
psshell-out (utils/host-process.ts,PS_TIMEOUT_MS = 1000). Under full-suite CPU contention, that secondpscall can miss its 1s deadline and returnnull, which mismatches the value captured earlier and flips a genuinely-live owner toowner-process-dead. This is exactly the "liveness can race under CPU load" shape the repo's timeouts-only flake taxonomy doesn't cover, since the failure surfaces as a wrong assertion, not a hang.Per-test root cause, fix, and counterfactual
1.
runner-session.test.ts— "runner session startup rejects live foreign runner lease"Root cause: The fixture uses
ownerPid: process.pid, ownerStartTime: RUNNER_OWNER_START_TIME(runner-lease.ts:123,classifyRunnerLease→isRunnerLeaseOwnerProcessAlive→classifyOwnerLiveness).RUNNER_OWNER_START_TIMEis captured once via a realpscall at module import;classifyOwnerLivenessre-readsreadProcessStartTime(pid)at classification time. Under load the second call can time out and returnnull, mismatching the cached value and flippinglive→stale('owner-process-dead').Fix: Added
mockReadProcessStartTimeto the file's existingvi.mock('.../host-process.ts', ...). It returns a fixed deterministic string forprocess.pid(used both at module-import time, soRUNNER_OWNER_START_TIMEitself becomes deterministic, and during classification) andnullfor any other pid — removing the real subprocess call entirely while still requiring an exact pid match.isProcessAlivestays mocked as before (unchanged), so the existing dead-lease test (fabricated huge pid, short-circuits beforereadProcessStartTimeis ever consulted) is untouched and still proves the dead-owner path.Counterfactual: Inverted
owner-identity.ts'sif (!isProcessAlive(owner.pid))toif (isProcessAlive(owner.pid)):Restored, test passes again.
2.
cli-device-status.test.ts— "keeps normal status compact while retaining proven-stale claims for explicit inspection"Root cause: This is a black-box CLI integration test (
runCliCaptureruns the realrunCliin-process, no mocks).readCurrentOwnerIdentity()captures the test's own pid+startTime once via a realpscall to build a "live" claim fixture;device-claim-inspection.ts→classifyOwnerLivenessre-readsreadProcessStartTime(pid)again when classifying claims fordevice status. Under load, the second realpscall can time out, misclassifying the live claim as stale — inflating the stale count from the expected 1 (the deliberately-fabricated dead claim) to 2.Fix: Added a scoped
vi.mock('../utils/host-process.ts', ...)that keeps everything real exceptreadProcessStartTime, which now caches the first real read for our own pid and returns that same value on every subsequent call (self-consistent with the fixture regardless of whether that firstpscall itself succeeds or times out — if it times out, both the fixture and the classifier consistently seenull, whichclassifyOwnerLivenesstreats as "skip the freshness check", not as a mismatch).isProcessAliveis untouched/real, so the fabricated-dead-pid (999_999_999) fixtures in this file still classify as stale through a real, non-flaky syscall check.Counterfactual: Same inversion in
owner-identity.ts:Restored, test passes again.
3.
daemon-client.test.ts— "cleanupFailedDaemonStartupMetadata retains live startup daemon on timeout" (third sighting, same mechanism, in scope)Root cause: This test spawns a real child process to stand in for a daemon (needed because
isAgentDeviceDaemonProcessalso checks the command line against daemon patterns, soprocess.pidalone won't do). It reads the child's real start time/command once to build the fixture, thencleanupFailedDaemonStartupMetadata→isAgentDeviceDaemonProcess(daemon-process.ts:20) re-reads both via realpscalls (once per info-file check, once per lock-file check) to confirm liveness. Any of those re-reads can time out under load and returnnull/mismatch, flipping the live daemon to "not live" and failingretainedInfoProcess/retainedLockProcess.Fix: Added a file-scoped
vi.mock('../host-process.ts', ...)forreadProcessStartTime/readProcessCommandthat defaults to the real implementation for every pid in every other test in the file (pure passthrough, zero behavior change elsewhere), and only this one test configures a pinned override for its spawned pid — using the real values it already captured — clearing the override infinally.Counterfactual: Inverted
daemon-process.ts's identity check (if (actualStartTime === expectedStartTime) return false;):Restored, test passes again.
Validation
pnpm typecheck && pnpm lint && pnpm format:check— all clean.npx vitest runon all three touched files — 107/107 pass.node -e 'while(true){}'CPU-load processes ran concurrently in the same foreground command — 107/107 pass; load processes killed within the same command.Generated by Claude Code