fix(tui): stop vitest auto-kill firing on garbage memory metric and killing non-vitest processes#1361
Merged
Merged
Conversation
…illing non-vitest processes Two compounding bugs made the memory-pressure vitest auto-kill a 30-second SIGKILL sweep of anything mentioning vitest: 1. False pressure: getAvailableMemory probed os.availableMemory, which does not exist, and silently fell back to os.freemem() — on macOS that reads ~99% used on an idle 256GB machine, permanently above the 90% threshold. Now reads process.availableMemory() (Node 22+) and refuses to auto-kill when only the unreliable freemem fallback is available. 2. Overbroad targeting: pgrep -f vitest matches full command lines, so the sweep also killed wrapper shells (zsh -c '... npx vitest run'), monitor loops, and anything else whose argv mentions vitest — stranding exit handlers and taking out unrelated process trees. New shared findVitestProcessIds (@fusion/core) filters matches to actual node executables. Surface enumeration (all vitest-process kill/count surfaces): - TUI memory-pressure auto-kill (controller.killVitestProcesses) - TUI manual kill-vitest command (same method) - dashboard POST /api/kill-vitest - dashboard GET /api/system-stats vitestProcessCount (display) All four now route through findVitestProcessIds.
Contributor
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR introduces shared helpers for safer vitest process discovery and memory measurement, then integrates both into the TUI controller and dashboard routes to improve auto-kill guard behavior by filtering to actual node processes and refusing auto-kill on unreliable memory readings. ChangesVitest auto-kill guard refactoring
Sequence DiagramsequenceDiagram
participant TUIController as TUI Controller
participant Memory as getAvailableMemoryInfo()
participant Discovery as findVitestProcessIds()
participant Kill as SIGKILL
TUIController->>Memory: probe available memory
Memory-->>TUIController: { bytes, reliable }
alt reliable == true
TUIController->>Discovery: request vitest node PIDs
Discovery-->>TUIController: filtered PIDs
TUIController->>Kill: execute SIGKILL on filtered PIDs
else reliable == false
TUIController->>TUIController: skip auto-kill
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
packages/core/src/__tests__/vitest-processes.test.ts (1)
30-51: ⚡ Quick winAdd the missing regression for a non-Vitest
nodeprocess.This suite proves wrapper shells are excluded, but it never models a plain
nodeprocess whose argv merely containsvitest. That is the false-positive path this PR is trying to remove, and the current tests would still pass with that bug intact. Add one candidate that looks likenodeinpsoutput but is not actually a Vitest runner, and assert it is filtered out.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/core/src/__tests__/vitest-processes.test.ts` around lines 30 - 51, The test for findVitestProcessIds lacks a case for a plain `node` process whose argv contains "vitest" (a non-Vitest false-positive) so add a ps entry that shows "node" but whose command/args do not indicate the Vitest runner and update the expected pids to exclude that PID; modify the makeExecFileMock call in the test (packages/core/src/__tests__/vitest-processes.test.ts) to include an additional ps line representing a non-Vitest node process and assert that findVitestProcessIds still returns only the real Vitest PIDs (keep existing expectations for pgrep and ps calls intact).packages/dashboard/src/__tests__/routes-system.test.ts (2)
408-417: 💤 Low valueConsider explicitly checking for pgrep in the fallthrough case.
The mock checks
if (file === "ps")but the else case assumes any other command is pgrep. While this works for the current test, it could return incorrect mock data if the route handler unexpectedly calls another command during refactoring.🔍 Suggested defensive check
mockExecFile.mockImplementation((...callArgs: unknown[]) => { const [file] = callArgs as [string]; const cb = callArgs[callArgs.length - 1] as (err: unknown, stdout?: string, stderr?: string) => void; if (file === "ps") { // comm filter pass: all candidates are real node processes. cb(null, ` ${process.pid} node\n 111 /opt/homebrew/bin/node\n 222 node\n`, ""); return; } + if (file === "pgrep") { + cb(null, `${process.pid}\n111\n222\n`, ""); + return; + } - cb(null, `${process.pid}\n111\n222\n`, ""); + // Unexpected command - fail explicitly + cb(new Error(`Unexpected execFile command: ${file}`), "", ""); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/src/__tests__/routes-system.test.ts` around lines 408 - 417, The mock implementation for mockExecFile currently only checks if (file === "ps") and assumes any other command is pgrep; update mockExecFile.mockImplementation to explicitly handle the "pgrep" command (e.g., if (file === "pgrep") return the pid-list payload) and add a default branch that throws or fails the test (or calls back with an error) for any unexpected command to catch future refactors; reference the mockExecFile, the file variable inside the mockImplementation, and the test behavior in routes-system.test.ts when making this change.
593-604: 💤 Low valueConsider validating the command in each mockImplementationOnce for clarity.
The chained mocks implicitly rely on the order of
execFileinvocations (pgrep first, then ps), but neither mock validates which command it's handling. If the implementation changes the call order during refactoring, the test would fail in confusing ways.📝 Suggested validation for clarity
mockExecFile .mockImplementationOnce((...callArgs: unknown[]) => { - // pgrep -f vitest: matches the dashboard itself, two node processes, - // a wrapper shell whose command line mentions vitest, and garbage. + const [file, argsOrCb] = callArgs as [string, unknown]; + const args = Array.isArray(argsOrCb) ? argsOrCb : []; const cb = callArgs[callArgs.length - 1] as (err: unknown, stdout?: string, stderr?: string) => void; + // Verify this is the pgrep call we expect + if (file !== "pgrep" || args[0] !== "-f" || args[1] !== "vitest") { + throw new Error(`Mock 1: expected pgrep -f vitest, got: ${file} ${args.join(" ")}`); + } + // pgrep -f vitest: matches the dashboard itself, two node processes, + // a wrapper shell whose command line mentions vitest, and garbage. cb(null, `${process.pid}\n1001\n1002\n1003\nnot-a-pid\n`, ""); }) .mockImplementationOnce((...callArgs: unknown[]) => { - // ps comm filter: 1003 is a zsh wrapper and must be spared. + const [file] = callArgs as [string]; const cb = callArgs[callArgs.length - 1] as (err: unknown, stdout?: string, stderr?: string) => void; + // Verify this is the ps call we expect + if (file !== "ps") { + throw new Error(`Mock 2: expected ps, got: ${file}`); + } + // ps comm filter: 1003 is a zsh wrapper and must be spared. cb(null, ` ${process.pid} node\n 1001 /opt/homebrew/bin/node\n 1002 node\n 1003 zsh\n`, ""); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/dashboard/src/__tests__/routes-system.test.ts` around lines 593 - 604, The tests register two mockImplementationOnce handlers on mockExecFile but don't verify which command each handler is servicing, so refactors changing call order will mis-route responses; update the mockImplementationOnce callbacks used in the routes-system.test (the mockExecFile setup) to inspect the command/args (e.g., check callArgs[0] or callArgs[1] for the invoked executable and its args) and assert or branch on whether it's the pgrep invocation vs the ps invocation (return the pgrep stdout for the pgrep call and the process list for the ps call, otherwise throw or call back an error) so each mock only responds to the intended command.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/core/src/vitest-processes.ts`:
- Around line 84-86: findVitestProcessIds currently uses pgrep -f vitest then
filterToNodeProcesses only checks ps comm, allowing unrelated Node processes
that mention "vitest" to slip through; update the filtering to inspect the full
argv/args. Modify filterToNodeProcesses (and its ps call) to use "ps -o
pid=,args=" (not just comm) via execToStdout/execFileImpl, require the
executable be node/node.exe AND also match a Vitest-specific marker in args
(e.g., "vitest", "node_modules/vitest", or the known Vitest entrypoint), and
return only those PIDs; ensure findVitestProcessIds still calls parsePids and
passes candidates into the tightened filter.
---
Nitpick comments:
In `@packages/core/src/__tests__/vitest-processes.test.ts`:
- Around line 30-51: The test for findVitestProcessIds lacks a case for a plain
`node` process whose argv contains "vitest" (a non-Vitest false-positive) so add
a ps entry that shows "node" but whose command/args do not indicate the Vitest
runner and update the expected pids to exclude that PID; modify the
makeExecFileMock call in the test
(packages/core/src/__tests__/vitest-processes.test.ts) to include an additional
ps line representing a non-Vitest node process and assert that
findVitestProcessIds still returns only the real Vitest PIDs (keep existing
expectations for pgrep and ps calls intact).
In `@packages/dashboard/src/__tests__/routes-system.test.ts`:
- Around line 408-417: The mock implementation for mockExecFile currently only
checks if (file === "ps") and assumes any other command is pgrep; update
mockExecFile.mockImplementation to explicitly handle the "pgrep" command (e.g.,
if (file === "pgrep") return the pid-list payload) and add a default branch that
throws or fails the test (or calls back with an error) for any unexpected
command to catch future refactors; reference the mockExecFile, the file variable
inside the mockImplementation, and the test behavior in routes-system.test.ts
when making this change.
- Around line 593-604: The tests register two mockImplementationOnce handlers on
mockExecFile but don't verify which command each handler is servicing, so
refactors changing call order will mis-route responses; update the
mockImplementationOnce callbacks used in the routes-system.test (the
mockExecFile setup) to inspect the command/args (e.g., check callArgs[0] or
callArgs[1] for the invoked executable and its args) and assert or branch on
whether it's the pgrep invocation vs the ps invocation (return the pgrep stdout
for the pgrep call and the process list for the ps call, otherwise throw or call
back an error) so each mock only responds to the intended command.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: be7225d6-0fba-41b3-a6ce-c3ab93d5873a
📒 Files selected for processing (8)
.changeset/vitest-autokill-guard.mdpackages/cli/src/commands/dashboard-tui/__tests__/available-memory.test.tspackages/cli/src/commands/dashboard-tui/controller.tspackages/core/src/__tests__/vitest-processes.test.tspackages/core/src/index.tspackages/core/src/vitest-processes.tspackages/dashboard/src/__tests__/routes-system.test.tspackages/dashboard/src/routes.ts
Comment on lines
+84
to
+86
| const candidates = parsePids(await execToStdout(execFileImpl, "pgrep", ["-f", "vitest"])); | ||
| const nodePids = await filterToNodeProcesses(execFileImpl, candidates); | ||
| return nodePids.filter((pid) => !excluded.has(pid)); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Inspecting the vitest PID selection pipeline..."
sed -n '46,86p' packages/core/src/vitest-processes.ts
echo
echo "Searching for where the helper proves a process is Vitest vs only proving it is node..."
rg -n -C2 'pgrep|-o|comm=|args=|node\.exe|executable === "node"' packages/core/src/vitest-processes.tsRepository: Runfusion/Fusion
Length of output: 3393
Tighten vitest PID selection: comm === node still allows unrelated Node processes that merely mention “vitest”.
findVitestProcessIds() uses pgrep -f vitest to create candidates, then filterToNodeProcesses() only checks ps -o pid=,comm= and keeps node / node.exe processes—there’s no inspection of the full command/argv. Any unrelated Node process whose command line contains “vitest” can therefore be returned and later SIGKILLed/count as vitest. Tighten the second stage to match Vitest-specific argv/entrypoint (e.g., via ps -o pid=,args= plus a Vitest marker) in addition to the node executable check.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/core/src/vitest-processes.ts` around lines 84 - 86,
findVitestProcessIds currently uses pgrep -f vitest then filterToNodeProcesses
only checks ps comm, allowing unrelated Node processes that mention "vitest" to
slip through; update the filtering to inspect the full argv/args. Modify
filterToNodeProcesses (and its ps call) to use "ps -o pid=,args=" (not just
comm) via execToStdout/execFileImpl, require the executable be node/node.exe AND
also match a Vitest-specific marker in args (e.g., "vitest",
"node_modules/vitest", or the known Vitest entrypoint), and return only those
PIDs; ensure findVitestProcessIds still calls parsePids and passes candidates
into the tightened filter.
Contributor
Greptile SummaryThis PR fixes two compounding bugs in the TUI's vitest auto-kill feature: a nonexistent
Confidence Score: 5/5Safe to merge — both bugs are fixed with correct logic, test coverage is thorough, and the shared helper is properly exercised across all four surfaces. The memory API fix is straightforward: No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Memory check tick every 2s] --> B[getAvailableMemoryInfo]
B --> C{process.availableMemory\nexists and works?}
C -- Yes --> D[reliable: true\nbytes: accurate value]
C -- No / throws --> E[reliable: false\nbytes: os.freemem]
D --> F{reliable && total > 0?}
E --> F
F -- No --> G[Skip — garbage signal\ndo NOT SIGKILL]
F -- Yes --> H{usedRatio > threshold\n&& gap > 30s?}
H -- No --> I[No action]
H -- Yes --> J[findVitestProcessIds]
J --> K[pgrep -f vitest]
K --> L{Any candidates?}
L -- No --> M[Return empty list]
L -- Yes --> N[ps -o pid=,comm= -p PIDs]
N --> O[Filter: comm basename == node\nor nodejs or node.exe]
O --> P[Exclude process.pid and excludePids]
P --> Q[SIGKILL remaining PIDs]
Reviews (2): Last reviewed commit: "Update packages/core/src/vitest-processe..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Contributor
|
Want your agent to iterate on Greptile's feedback? Try greploops. |
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
1 participant
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
With the TUI's "auto-kill vitest on memory pressure" toggle enabled, every vitest process on the machine was being SIGKILLed every 30 seconds regardless of actual memory pressure — and the sweep also killed innocent processes (wrapper shells, monitor loops) whose command line merely mentioned vitest. In practice this made full test-suite runs die silently mid-run with truncated output and no exit status, which burned hours of debugging before the killer was identified.
Two compounding bugs:
False pressure.
getAvailableMemory()probedos.availableMemory— an API that does not exist — and silently fell back toos.freemem(). On macOS, freemem excludes the reclaimable file cache, so an idle 256GB machine reads ~99% "used", permanently above the 90% kill threshold. The function's own comment shows it was written specifically to avoid this trap; the correct API isprocess.availableMemory()(Node 22+), which reports ~46% used on the same machine. The guard now uses it, and refuses to auto-kill when only the unreliable freemem fallback is available rather than killing on a garbage ratio.Overbroad targeting.
pgrep -f vitestmatches full command lines, so the sweep matched and SIGKILLedzsh -c '... npx vitest run ...'wrapper shells (stranding their$?handlers — which is why dead runs looked like silent truncation) and even a monitoring loop whose command line containedgrep 'node (vitest'. A new sharedfindVitestProcessIds()in@fusion/corefilters pgrep candidates to actual node executables viaps -o pid=,comm=before anything signals them.Surface Enumeration
All vitest-process kill/count surfaces, all now routed through
findVitestProcessIds:controller.killVitestProcesses)POST /api/kill-vitestGET /api/system-statsvitestProcessCountTest plan
findVitestProcessIds(node-comm filtering spares shells/monitors, self/exclude pids, empty pgrep, win32 no-op)getAvailableMemoryInfo(reliable viaprocess.availableMemory, unreliable freemem fallback, throw fallback)routes-systemtests: kill route now proves a zsh wrapper in the pgrep results is sparedtsc --noEmitclean on core/cli/dashboardNote: a running
fnTUI/dashboard keeps the old behavior until restarted with this fix (or the auto-kill toggle is turned off in the TUI).Summary by CodeRabbit