feat: child-process isolation for benchmarks#512
Conversation
Run each engine/model benchmark in a forked subprocess so that segfaults (e.g. from the native Rust addon) only kill the child — the parent survives and collects partial results from whichever engines succeeded. - New shared utility: scripts/lib/fork-engine.js (isWorker, workerEngine, forkEngines) handles subprocess lifecycle, timeout, crash recovery, and partial JSON result extraction - benchmark.js, query-benchmark.js, incremental-benchmark.js: fork per engine (wasm/native) via forkEngines() - embedding-benchmark.js: fork per model (ONNX runtime can OOM/segfault) - JSON output contract unchanged — report scripts and CI need no changes
|
Claude finished @carlos-alm's task —— View job PR Review: Child-Process Isolation for BenchmarksComprehensive review of subprocess isolation implementation to handle segfaults gracefully. Review Tasks ✅
Root Cause AnalysisProblem Being Solved: Native Rust addon segfaults kill entire benchmark processes, preventing collection of results from working engines (WASM vs native). This causes benchmark CI failures and incomplete performance data. Why This Approach: Child-process isolation is the standard solution for handling potentially unstable native code. Alternative approaches (fixing all segfaults in Rust addon, or abandoning native benchmarks) are either infeasible or reduce test coverage. Risk Assessment: Low risk - this is defensive programming that improves reliability without changing functionality. Implementation Quality Assessmentfork-engine.js Design ⭐⭐⭐⭐
Integration Quality ⭐⭐⭐⭐
Code Quality ⭐⭐⭐⭐
Configuration & Test Integrity ✅
Backlog Compliance
Critical ConcernsNone identified. This is well-implemented defensive programming. Minor Observations
Final Recommendation
The code quality is high, error handling is robust, and the approach is standard practice for isolating unstable native code. Recommendation: Approve and merge. |
Greptile SummaryThis PR introduces child-process isolation for all four benchmark scripts so that native-addon segfaults only kill the child worker rather than the entire benchmark run. A new shared utility All previously raised review issues have been addressed: the dual-timeout problem, the double-resolution race, the One remaining issue:
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A([node benchmark.js]) --> B{isWorker?}
B -- No: parent path --> C[resolveBenchmarkSource\nget version + versionCleanup]
C --> D[forkEngines]
D --> E[resolveBenchmarkSource\nget srcDir + cleanup]
E --> F{hasWasm?\nhasNative?}
F -- neither --> G[throw Error]
G --> H[caller catches,\ncalls versionCleanup,\nprocess.exit 1]
F -- at least one --> I[forkWorker wasm\nor native sequentially]
I --> J[fork child process\nSIGKILL timer 10 min]
J --> K{child close event}
K -- killed by signal --> L[settle null]
K -- code != 0 --> M{valid JSON\nin stdout?}
M -- yes --> N[settle parsed partial]
M -- no --> O[settle null]
K -- code 0 --> P[settle parsed JSON]
L & N & O & P --> Q[results.wasm / results.native]
Q --> R[assemble output JSON\nversionCleanup\nprocess.exit 0]
B -- Yes: worker path --> S[workerEngine\nrun benchmark for engine]
S --> T[write JSON to stdout\ncleanup\nprocess.exit 0]
Last reviewed commit: "fix: replace process..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
scripts/lib/fork-engine.js
Outdated
| const child = fork(scriptPath, argv, { | ||
| env: { ...process.env, [WORKER_ENV_KEY]: engine }, | ||
| stdio: ['ignore', 'pipe', 'inherit', 'ipc'], | ||
| timeout: timeoutMs, | ||
| }); | ||
|
|
||
| let stdout = ''; | ||
| child.stdout.on('data', (chunk) => { stdout += chunk; }); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| console.error(`[fork] ${engine} worker timed out after ${timeoutMs / 1000}s — killing`); | ||
| child.kill('SIGKILL'); | ||
| }, timeoutMs); |
There was a problem hiding this comment.
Both fork()'s built-in timeout option and the manual setTimeout are set to the same timeoutMs value. The built-in timeout option sends SIGTERM at T ms, while the manual timer sends SIGKILL at T ms — nearly simultaneously. Since SIGKILL arrives at essentially the same moment as SIGTERM, the child never has an opportunity for graceful cleanup before it's forcibly killed.
If the intent is SIGKILL-only (fast, no grace period), remove the timeout option from fork() and let the manual timer be the sole mechanism:
const child = fork(scriptPath, argv, {
env: { ...process.env, [WORKER_ENV_KEY]: engine },
stdio: ['ignore', 'pipe', 'inherit', 'ipc'],
// Remove `timeout` here — the manual timer below handles it with SIGKILL
});If the intent is graceful-then-force (SIGTERM + grace + SIGKILL), offset the manual timer:
const child = fork(scriptPath, argv, {
env: { ...process.env, [WORKER_ENV_KEY]: engine },
stdio: ['ignore', 'pipe', 'inherit', 'ipc'],
timeout: timeoutMs, // sends SIGTERM at T
});
const timer = setTimeout(() => {
console.error(`[fork] ${engine} worker timed out — sending SIGKILL`);
child.kill('SIGKILL');
}, timeoutMs + 5_000); // SIGKILL 5 s after SIGTERM grace periodThe same pattern applies to embedding-benchmark.js (lines 140–152).
There was a problem hiding this comment.
Fixed — removed the timeout option from fork() so the manual SIGKILL timer is the sole timeout mechanism. No more simultaneous SIGTERM+SIGKILL. This is now in the shared forkWorker() helper, so the fix applies to both fork-engine.js and embedding-benchmark.js.
scripts/benchmark.js
Outdated
| const { version } = await resolveBenchmarkSource(); | ||
| const { wasm, native } = await forkEngines(import.meta.url, process.argv.slice(2)); |
There was a problem hiding this comment.
cleanup() from first resolveBenchmarkSource() call is never invoked
resolveBenchmarkSource() is called here solely to extract version, but the returned cleanup callback is never captured or called. forkEngines() then calls resolveBenchmarkSource() again internally (and does call its own cleanup). If resolveBenchmarkSource() creates any temporary resources (symlinks, temp dirs, npm installs), those from this first call will be leaked for the lifetime of the parent process.
Contrast this with incremental-benchmark.js line 22, which correctly captures and calls parentCleanup().
| const { version } = await resolveBenchmarkSource(); | |
| const { wasm, native } = await forkEngines(import.meta.url, process.argv.slice(2)); | |
| const { version, cleanup: versionCleanup } = await resolveBenchmarkSource(); | |
| const { wasm, native } = await forkEngines(import.meta.url, process.argv.slice(2)); |
Then call versionCleanup() before process.exit(0) on line 69.
There was a problem hiding this comment.
Fixed — parent path now captures cleanup as versionCleanup and calls it before process.exit(0), matching how incremental-benchmark.js handles it.
scripts/embedding-benchmark.js
Outdated
| function forkModel(modelKey) { | ||
| return new Promise((resolve) => { | ||
| console.error(`\n[fork] Spawning ${modelKey} worker (pid isolation)...`); | ||
|
|
||
| const child = fork(scriptPath, process.argv.slice(2), { | ||
| env: { ...process.env, [MODEL_WORKER_KEY]: modelKey }, | ||
| stdio: ['ignore', 'pipe', 'inherit', 'ipc'], | ||
| timeout: TIMEOUT_MS, | ||
| }); | ||
|
|
||
| let stdout = ''; | ||
| child.stdout.on('data', (chunk) => { stdout += chunk; }); | ||
|
|
||
| const timer = setTimeout(() => { | ||
| console.error(`[fork] ${modelKey} worker timed out after ${TIMEOUT_MS / 1000}s — killing`); | ||
| child.kill('SIGKILL'); | ||
| }, TIMEOUT_MS); | ||
|
|
||
| child.on('close', (code, signal) => { | ||
| clearTimeout(timer); | ||
|
|
||
| if (signal) { | ||
| console.error(`[fork] ${modelKey} worker killed by signal ${signal}`); | ||
| resolve(null); | ||
| return; | ||
| } | ||
|
|
||
| if (code !== 0) { | ||
| console.error(`[fork] ${modelKey} worker exited with code ${code}`); | ||
| try { | ||
| const parsed = JSON.parse(stdout); | ||
| console.error(`[fork] ${modelKey} worker produced partial results despite non-zero exit`); | ||
| resolve(parsed); | ||
| } catch { | ||
| resolve(null); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| resolve(JSON.parse(stdout)); | ||
| } catch (err) { | ||
| console.error(`[fork] ${modelKey} worker produced invalid JSON: ${err.message}`); | ||
| resolve(null); | ||
| } | ||
| }); | ||
|
|
||
| child.on('error', (err) => { | ||
| clearTimeout(timer); | ||
| console.error(`[fork] ${modelKey} worker failed to start: ${err.message}`); | ||
| resolve(null); | ||
| }); | ||
| }); | ||
| } |
There was a problem hiding this comment.
forkModel() duplicates runWorker() logic from fork-engine.js
The forkModel() function is a near-identical copy of runWorker() in scripts/lib/fork-engine.js — same stdout accumulation, same dual-timeout pattern, same partial-JSON fallback, same error handlers. The PR description frames fork-engine.js as a shared utility, but embedding-benchmark.js never imports it, maintaining a parallel implementation.
This means any bug fix or improvement to the fork lifecycle (e.g., the double-timeout issue above) must be applied in two places. Consider generalising forkEngines / fork-engine.js to accept an arbitrary worker key and a list of worker names, or extract a lower-level forkWorker(scriptPath, envKey, workerName, argv, opts) helper that both forkEngines and embedding-benchmark.js can call.
There was a problem hiding this comment.
Fixed — extracted a generic forkWorker(scriptPath, envKey, workerName, argv, timeoutMs) helper in fork-engine.js. embedding-benchmark.js now imports and uses it instead of maintaining its own copy. Bug fixes (timeout, settle guard) are in one place.
scripts/lib/fork-engine.js
Outdated
| child.on('close', (code, signal) => { | ||
| clearTimeout(timer); | ||
|
|
||
| if (signal) { | ||
| console.error(`[fork] ${engine} worker killed by signal ${signal}`); | ||
| resolve(null); | ||
| return; | ||
| } | ||
|
|
||
| if (code !== 0) { | ||
| console.error(`[fork] ${engine} worker exited with code ${code}`); | ||
| // Try to parse partial output anyway | ||
| try { | ||
| const parsed = JSON.parse(stdout); | ||
| console.error(`[fork] ${engine} worker produced partial results despite non-zero exit`); | ||
| resolve(parsed); | ||
| } catch { | ||
| resolve(null); | ||
| } | ||
| return; | ||
| } | ||
|
|
||
| try { | ||
| resolve(JSON.parse(stdout)); | ||
| } catch (err) { | ||
| console.error(`[fork] ${engine} worker produced invalid JSON: ${err.message}`); | ||
| resolve(null); | ||
| } | ||
| }); | ||
|
|
||
| child.on('error', (err) => { | ||
| clearTimeout(timer); | ||
| console.error(`[fork] ${engine} worker failed to start: ${err.message}`); | ||
| resolve(null); | ||
| }); |
There was a problem hiding this comment.
Node.js child-process documentation notes that when a process fails to spawn, both the 'error' event and the 'close' event are emitted (in that order, with close carrying code: null, signal: null). Both handlers call resolve(null), so the Promise is resolved twice. Since subsequent resolutions are silently ignored by the Promise spec, this is harmless today, but it could mask future cases where the two events carry different values.
A common guard is a resolved flag:
let resolved = false;
function settle(value) {
if (resolved) return;
resolved = true;
resolve(value);
}Then replace all resolve(...) calls with settle(...) inside the 'close' and 'error' handlers. The same pattern should be applied to forkModel() in embedding-benchmark.js.
There was a problem hiding this comment.
Fixed — added a settled flag with a settle() wrapper function. Both close and error handlers call settle() instead of resolve() directly, preventing double resolution on spawn failure.
…#512) - Extract forkWorker() as a generic reusable subprocess helper - Remove fork()'s built-in timeout option so the manual SIGKILL timer is the sole timeout mechanism (they previously fired simultaneously) - Add a settled flag to prevent double Promise resolution on spawn failure - forkEngines() now delegates to forkWorker() instead of inline runWorker()
…512) The first resolveBenchmarkSource() call (for version extraction) never had its cleanup invoked, potentially leaking temporary resources.
embedding-benchmark.js had a near-identical copy of runWorker() from fork-engine.js (including the dual-timeout bug). Now imports and uses the shared forkWorker() helper instead.
| @@ -130,95 +145,35 @@ function benchDiffImpact(hubName) { | |||
| affectedFiles: lastResult?.affectedFiles?.length || 0, | |||
| }; | |||
| } finally { | |||
| // Restore: unstage + revert content | |||
| execFileSync('git', ['restore', '--staged', hubFile], { cwd: root, stdio: 'pipe' }); | |||
| fs.writeFileSync(hubFile, original); | |||
| } | |||
There was a problem hiding this comment.
benchDiffImpact() runs git add hubFile (line 132), then benchmarks, then restores via git restore --staged hubFile in a finally block (line 148). However, a SIGSEGV from the native addon — exactly the crash scenario this PR is designed to handle — kills the child process at the OS level before any JavaScript finally block can execute. The finally cleanup is therefore not crash-safe.
If the worker is killed (by signal or SIGKILL timeout) after git add but before git restore --staged, the parent process survives (as intended) but the working tree's index is permanently left with the hub file staged. This can corrupt the developer's git state and interfere with subsequent benchmark runs or unrelated git operations.
One practical mitigation is for forkWorker (or the benchmark orchestrator) to run git restore --staged in the parent when the child exits abnormally. Alternatively, benchDiffImpact could record the staged file in a temp file before git add, allowing a parent-side recovery step.
There was a problem hiding this comment.
Fixed in two commits:
-
Parent cleanup capture:
resolveBenchmarkSource()return value is now destructured to capturecleanupasversionCleanup, called on both the success and error exit paths. -
Git staging safety net: After
forkEngines()returns, the parent now checksgit diff --cached --name-only. If any staged files are left over from a crashed worker (segfault duringbenchDiffImpact'sgit add), the parent runsgit restore --staged .andgit checkout .to reset both the index and working tree. This ensures a worker crash never corrupts the developer's git state.
versionCleanup() was only called on the success path. When both engines fail and the parent exits early via process.exit(1), temporary resources from resolveBenchmarkSource() were leaked.
Two issues addressed: 1. resolveBenchmarkSource() cleanup was never captured or called in the parent process, leaking temporary resources on every run. 2. benchDiffImpact() runs git-add inside the worker — if a segfault kills the worker mid-execution, the finally block never runs and the git staging area is left dirty. The parent now checks for leftover staged files after workers exit and cleans them up.
| cleanup(); | ||
|
|
||
| if (!hasWasm && !hasNative) { | ||
| console.error('Error: Neither WASM grammars nor native engine are available.'); | ||
| console.error('Run "npm run build:wasm" to build WASM grammars, or install the native platform package.'); | ||
| process.exit(1); | ||
| } |
There was a problem hiding this comment.
process.exit(1) bypasses callers' cleanup callbacks
forkEngines correctly calls its own cleanup() (line 150) before exiting, but process.exit(1) on line 155 aborts the call-stack immediately — the caller never regains control. This means the versionCleanup / parentCleanup captured by each benchmark script before calling forkEngines is never invoked when neither engine is available:
benchmark.js:23—versionCleanupleakedquery-benchmark.js:27—versionCleanupleakedincremental-benchmark.js:22—parentCleanupleaked
Rather than exiting internally, forkEngines should signal the error to the caller and let it handle cleanup before exiting:
if (!hasWasm && !hasNative) {
console.error('Error: Neither WASM grammars nor native engine are available.');
console.error('Run "npm run build:wasm" to build WASM grammars, or install the native platform package.');
return null; // or throw an Error
}Each caller can then check for null / catch the error, call its own cleanup, and exit.
There was a problem hiding this comment.
Fixed — forkEngines() now throws an Error instead of calling process.exit(1). All three callers (benchmark.js, query-benchmark.js, incremental-benchmark.js) catch the error, invoke their own cleanup callback, and exit gracefully.
forkEngines() called process.exit(1) when no engines were available, which aborted the call-stack immediately and prevented callers from invoking their own cleanup callbacks (versionCleanup, parentCleanup). Now throws an Error instead; all three callers catch it, run cleanup, and exit gracefully. Impact: 1 functions changed, 3 affected
1 participant
Summary
scripts/lib/fork-engine.jshandles subprocess lifecycle, timeout (10min), crash recovery, and partial JSON extraction from non-zero exitsbenchmark.js,query-benchmark.js,incremental-benchmark.jsfork per engine (wasm/native);embedding-benchmark.jsforks per model (ONNX can OOM/segfault)Test plan
node scripts/benchmark.jslocally — verify JSON output matches previous schemanode scripts/query-benchmark.jslocally — verify JSON outputnode scripts/incremental-benchmark.jslocally — verify JSON outputnode scripts/embedding-benchmark.jslocally — verify JSON output[fork]log messages appear on stderr showing subprocess spawn/exit