fix: address CodeRabbit review feedback on PR #1491

- Update POST_SPAWN_WAIT test assertion from 5000 to 15000 to match
  the constant change in hook-constants.ts
- Remove redundant readPidFile() from aggressiveStartupCleanup() —
  start() writes the new PID before this runs, so it always returns
  process.pid (already protected)
- Add waitForReadiness() to the reused-worker path in
  ensureWorkerStarted() to prevent concurrent hooks from racing
  past a cold-starting worker's initialization guard

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
This commit is contained in:
Ryan Malia
2026-03-25 15:02:09 -07:00
parent 88b47f9e9c
commit 83f61177c7
4 changed files with 93 additions and 87 deletions

File diff suppressed because one or more lines are too long

View File

@@ -453,17 +453,18 @@ export async function aggressiveStartupCleanup(): Promise<void> {
const pidsToKill: number[] = []; const pidsToKill: number[] = [];
const allPatterns = [...AGGRESSIVE_CLEANUP_PATTERNS, ...AGE_GATED_CLEANUP_PATTERNS]; const allPatterns = [...AGGRESSIVE_CLEANUP_PATTERNS, ...AGE_GATED_CLEANUP_PATTERNS];
// Protect parent process (the hook that spawned us) and the PID-file-registered // Protect parent process (the hook that spawned us) from being killed.
// worker from being killed. Without this, a new daemon kills its own parent hook // Without this, a new daemon kills its own parent hook process (#1426).
// process (#1426) and any already-running worker the PID file points to. //
// Note: readPidFile() is not used here because start() writes the new PID
// before initializeBackground() calls this function, so readPidFile() would
// just return process.pid (already protected). If a pre-existing worker needs
// protection, ensureWorkerStarted() handles that by returning early when a
// healthy worker is detected — we never reach this code in that case.
const protectedPids = new Set<number>([currentPid]); const protectedPids = new Set<number>([currentPid]);
if (process.ppid && process.ppid > 0) { if (process.ppid && process.ppid > 0) {
protectedPids.add(process.ppid); protectedPids.add(process.ppid);
} }
const pidFileInfo = readPidFile();
if (pidFileInfo?.pid && pidFileInfo.pid > 0) {
protectedPids.add(pidFileInfo.pid);
}
try { try {
if (isWindows) { if (isWindows) {

View File

@@ -985,6 +985,11 @@ async function ensureWorkerStarted(port: number): Promise<boolean> {
// than a dead worker. Users must manually restart after genuine plugin updates. // than a dead worker. Users must manually restart after genuine plugin updates.
// See also: #566, #665, #667, #669, #689, #1124, #1145 (same pattern across 8+ releases). // See also: #566, #665, #667, #669, #689, #1124, #1145 (same pattern across 8+ releases).
if (await waitForHealth(port, 1000)) { if (await waitForHealth(port, 1000)) {
// Health passed — worker is listening. Also wait for readiness in case
// another hook just spawned it and background init is still running.
// This mirrors the fresh-spawn path (line ~1025) so concurrent hooks
// don't race past a cold-starting worker's initialization guard.
await waitForReadiness(port, getPlatformTimeout(HOOK_TIMEOUTS.READINESS_WAIT));
logger.info('SYSTEM', 'Worker already running and healthy'); logger.info('SYSTEM', 'Worker already running and healthy');
return true; return true;
} }

View File

@@ -32,8 +32,8 @@ describe('hook-constants', () => {
expect(HOOK_TIMEOUTS.HEALTH_CHECK).toBe(3000); expect(HOOK_TIMEOUTS.HEALTH_CHECK).toBe(3000);
}); });
it('should define POST_SPAWN_WAIT as 5s', () => { it('should define POST_SPAWN_WAIT as 15s', () => {
expect(HOOK_TIMEOUTS.POST_SPAWN_WAIT).toBe(5000); expect(HOOK_TIMEOUTS.POST_SPAWN_WAIT).toBe(15000);
}); });
it('should define PORT_IN_USE_WAIT as 3s', () => { it('should define PORT_IN_USE_WAIT as 3s', () => {