fix(components): avoid startup hangs on zombie process groups - #2085
fix(components): avoid startup hangs on zombie process groups#2085kriszyp wants to merge 5 commits into
Conversation
|
Guided tour for reviewers:
Where to look hardest: the Linux-specific Verification: build, focused component spawn test (13 passing), lint, and the full integration suite passed. The broad unit gate is blocked before discovery by the local RocksDB build's missing LZ4 support; the components glob also treats a fixture as a test. Independent coverage: Gemini, Cursor Grok, and Cursor Composer ran. The final receipt requests human review (grade 4) because the exact policy omits a graded/adjudication leg and leaves the platform/performance tradeoff for review. — GPT-5 Codex |
|
Reviewed; no blockers found. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request refactors process group status checks by introducing a robust isProcessGroupAlive function that scans /proc on Linux to ensure a process group is only considered alive if it contains non-zombie processes. It also updates the unit tests to use waitFor instead of arbitrary delays and adds test coverage for the new zombie process group detection. The reviewer suggested using assert.strictEqual instead of assert.equal in the new tests to align with the repository's style guide.
| assert.equal(zombieGroup(['123', '456']), false); | ||
| assert.equal( | ||
| isProcessGroupAlive(123, { | ||
| platform: 'linux', | ||
| processGroupExists: () => true, | ||
| readDirectory: () => ['123', '456'], | ||
| readStat: (path) => | ||
| path === '/proc/123/stat' ? '123 (installer) Z 1 123 123' : '456 (still running) S 1 123 123', | ||
| }), | ||
| true | ||
| ); | ||
| assert.equal(zombieGroup(['123']), false); | ||
| assert.equal( | ||
| isProcessGroupAlive(123, { | ||
| platform: 'linux', | ||
| processGroupExists: () => true, | ||
| readDirectory: () => ['456'], | ||
| readStat: (path) => { | ||
| if (path === '/proc/123/stat') throw new Error('leader reaped'); | ||
| return '456 (unreaped child) Z 1 123 123'; | ||
| }, | ||
| }), | ||
| false | ||
| ); | ||
| assert.equal( | ||
| isProcessGroupAlive(123, { | ||
| platform: 'linux', | ||
| processGroupExists: () => true, | ||
| readDirectory: () => ['456'], | ||
| readStat: (path) => { | ||
| if (path === '/proc/123/stat') throw new Error('leader reaped'); | ||
| return '456 (running child) S 1 123 123'; | ||
| }, | ||
| }), | ||
| true | ||
| ); |
There was a problem hiding this comment.
According to the repository style guide, strict assertion methods like assert.strictEqual should be used explicitly where strict semantics are needed (such as asserting boolean values), rather than loose equality checks like assert.equal.
assert.strictEqual(zombieGroup(['123', '456']), false);
assert.strictEqual(
isProcessGroupAlive(123, {
platform: 'linux',
processGroupExists: () => true,
readDirectory: () => ['123', '456'],
readStat: (path) =>
path === '/proc/123/stat' ? '123 (installer worker) Z 1 123 123' : '456 (installer child) Z 1 123 123',
}),
true
);
assert.strictEqual(zombieGroup(['123']), false);
assert.strictEqual(
isProcessGroupAlive(123, {
platform: 'linux',
processGroupExists: () => true,
readDirectory: () => ['456'],
readStat: (path) => {
if (path === '/proc/123/stat') throw new Error('leader reaped');
return '456 (unreaped child) Z 1 123 123';
},
}),
false
);
assert.strictEqual(
isProcessGroupAlive(123, {
platform: 'linux',
processGroupExists: () => true,
readDirectory: () => ['456'],
readStat: (path) => {
if (path === '/proc/123/stat') throw new Error('leader reaped');
return '456 (running child) S 1 123 123';
},
}),
true
);References
- Use assert.strictEqual/assert.deepStrictEqual explicitly where strict semantics are needed. (link)
- Use the bare
node:assertmodule instead ofnode:assert/strictfor test assertions to comply with linting rules, while still utilizing strict assertion methods likeassert.strictEqualfrom the bare module.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| function higherPriorityResolutionCandidates( | ||
| specifier: string, | ||
| referrerPath: string, | ||
| resolvedPath: string | ||
| ): string[] | undefined { | ||
| if (!specifier.startsWith('.')) return; | ||
| const basePath = resolve(dirname(referrerPath), specifier); | ||
| const candidates = [...new Set(RESOLUTION_EXTENSIONS.map((extension) => basePath + extension))]; | ||
| candidates.push(resolve(basePath, 'package.json')); | ||
| for (const extension of RESOLUTION_EXTENSIONS.slice(1)) candidates.push(resolve(basePath, `index${extension}`)); | ||
| const resolvedIndex = candidates.indexOf(resolvedPath); | ||
| if (resolvedIndex === -1) return; | ||
| return candidates.slice(0, resolvedIndex); |
There was a problem hiding this comment.
1. New Bun candidate-detection path has no test that exercises it
File: components/RuntimeModuleTracker.ts:131-170 (higherPriorityResolutionCandidates, candidateState, candidateStateAsync)
What: This commit (89a6c94) adds a new independent staleness-detection mechanism: for relative specifiers, it enumerates the filesystem candidates that would have been checked with higher priority than the resolved path, snapshots their state, and re-checks that state in #compare() before falling through to the pre-existing invalidateResolutionCache + createRequire().resolve() check.
The only test that superficially covers this scenario — unitTests/components/RuntimeModuleTracker.test.js › "detects a new higher-priority extensionless resolution candidate" — is unmodified by this PR and already exists unchanged on main (i.e. it passed before this candidate-tracking code existed). It passes because Node's own require.resolve() re-resolution (the old fallback, still present right after the new check) already picks up the newly-created helper.js and returns a different resolvedUrl. Deleting the entire new higherPriorityCandidates block would not make this test fail — so it isn't actually validating the new code path.
Per the commit message ("detect Bun resolution candidate changes") and the PR body's HARPER_RUNTIME=bun verification step, the new mechanism exists specifically to catch cases where the old resolve-based fallback doesn't reliably detect a new higher-priority file (e.g. under Bun, where Module._pathCache invalidation may not force a real re-resolution). That specific scenario — new logic detects a change the fallback misses — has no direct unit coverage.
Why it matters: This is new, non-trivial (~80 LOC) production logic gating whether a redeploy is treated as a hot-swap vs. requiring a restart. If the candidate/priority-ordering logic (e.g. the assumed extension-check order in RESOLUTION_EXTENSIONS) is wrong, a stale module could silently continue being served after a redeploy — and no test in this PR would catch that regression, since the existing test passes via a different mechanism entirely.
Suggested fix: Add a unit test that isolates the new branch — e.g. stub/neutralize the fallback re-resolution (as the existing "fails closed when Node resolution cache invalidation is unavailable" test does for Module._pathCache) so only higherPriorityCandidates can produce a true/false result, then verify it correctly flags a newly-added higher-priority file and correctly reports "unchanged" when candidates are untouched.
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
Co-Authored-By: GPT-5 Codex <noreply@openai.com>
89a6c94 to
0f03313
Compare
| assert.equal(zombieGroup(['123', '456']), false); | ||
| assert.equal( | ||
| isProcessGroupAlive(123, { | ||
| platform: 'linux', | ||
| processGroupExists: () => true, | ||
| readDirectory: () => ['123', '456'], | ||
| readStat: (path) => | ||
| path === '/proc/123/stat' ? '123 (installer) Z 1 123 123' : '456 (still running) S 1 123 123', | ||
| }), | ||
| true | ||
| ); | ||
| assert.equal(zombieGroup(['123']), false); | ||
| assert.equal( | ||
| isProcessGroupAlive(123, { | ||
| platform: 'linux', | ||
| processGroupExists: () => true, | ||
| readDirectory: () => ['456'], | ||
| readStat: (path) => { | ||
| if (path === '/proc/123/stat') throw new Error('leader reaped'); | ||
| return '456 (unreaped child) Z 1 123 123'; | ||
| }, | ||
| }), | ||
| false | ||
| ); | ||
| assert.equal( | ||
| isProcessGroupAlive(123, { | ||
| platform: 'linux', | ||
| processGroupExists: () => true, | ||
| readDirectory: () => ['456'], | ||
| readStat: (path) => { | ||
| if (path === '/proc/123/stat') throw new Error('leader reaped'); | ||
| return '456 (running child) S 1 123 123'; | ||
| }, | ||
| }), | ||
| true | ||
| ); |
There was a problem hiding this comment.
Suggestion (non-blocking): these boolean checks still use assert.equal — gemini's earlier comment on this same block asked for assert.strictEqual, which matches the repo's own house style (AGENTS.md: use assert.strictEqual/assert.deepStrictEqual explicitly when a check needs strict semantics, e.g. booleans). Not gating merge, just flagging it's still outstanding.
1 participant
Human-Review-Need: 4 @ 0f03313
Summary
Prevent component preparation from waiting forever on an unreaped Linux zombie process group. This CI follow-up uses RocksDB's database-wide transaction-log purge scope, resolves import-only ESM package exports under Bun, and detects higher-priority extensionless candidates without relying on Bun's CommonJS resolver cache.
Refs #2072
Related #2076
Verification
npm run buildnpm run lint:requirednpx mocha unitTests/components/RuntimeModuleTracker.test.js(6 passing)npm run test:integration -- --isolation=none integrationTests/database/txnlog-purge-stale-read-blast.test.ts(14 passing; RocksDB and LMDB arms)HARPER_RUNTIME=bun npm run test:integration -- --isolation=none integrationTests/deploy/redeploy-runtime-equivalence.test.ts(18 passing; both repaired Bun regressions)1/6and5/6shards pass on Bun, Node.js, uWS HTTP, and Windows.Generated by GPT-5 Codex.