Skip to content

fix: use spawn+detach in tryStartDaemon to prevent daemon being killed on timeout #153

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
marcuspuchalla wants to merge 1 commit into
base: main
Choose a base branch
from
+57 −12
Open

fix: use spawn+detach in tryStartDaemon to prevent daemon being killed on timeout #153

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
69 changes: 57 additions & 12 deletions .claude/hooks/src/daemon-client.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@
*/

import { existsSync, readFileSync, writeFileSync, unlinkSync } from 'fs';
import { execSync, spawnSync } from 'child_process';
import { execSync, spawn } from 'child_process';
import { join, resolve } from 'path';
import { tmpdir } from 'os';
import * as net from 'net';
Expand Down Expand Up @@ -385,23 +385,44 @@ export function tryStartDaemon(projectDir: string): boolean {
const tldrPath = join(projectDir, 'opc', 'packages', 'tldr-code');
let started = false;

// Try local dev installation first (only if it exists)
// Use spawn+detach so daemon survives as independent background process.
// Do NOT use spawnSync — it kills the child process on timeout, which happens
// during initial indexing (30-60s) and causes repeated daemon spawns.

// Try local dev installation first (only if uv is available AND tldr path exists).
// spawn() emits 'error' asynchronously for ENOENT — not catchable after unref().
// Guard: check uv is in PATH synchronously, then check child.pid after spawn.
if (existsSync(tldrPath)) {
const result = spawnSync('uv', ['run', 'tldr', 'daemon', 'start', '--project', projectDir], {
timeout: 10000,
stdio: 'ignore',
cwd: tldrPath,
});
started = result.status === 0;
try {
execSync('uv --version', { stdio: 'ignore', timeout: 2000 });
const child = spawn('uv', ['run', 'tldr', 'daemon', 'start', '--project', projectDir], {
detached: true,
stdio: 'ignore',
windowsHide: true,
cwd: tldrPath,
});
// Register unconditionally — error fires when pid is undefined (spawn failed),
// so the handler must be attached before the pid check, not inside it.
child.on('error', () => { /* uv disappeared between check and spawn */ });
if (child.pid !== undefined) {
child.unref();
started = true;
}
Comment on lines +407 to +410

@coderabbitai coderabbitai Bot Feb 22, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor

Unhandled error event when child.pid is undefined — hook process will crash.

subprocess.pid is undefined when the child fails to spawn, and the error event is emitted asynchronously in that case. Because the child.on('error', ...) handler is only registered inside the if (child.pid !== undefined) branch, a spawn failure (e.g., the TOCTOU window where uv disappears between execSync('uv --version') and spawn) will emit an error event with no listener. A synchronous try/catch cannot intercept this: the error propagates as an unhandled EventEmitter event and terminates the process. The blocking spin-loop below means the event fires after tryStartDaemon returns, but before Node.js exits — so the crash still lands in the hook's process lifetime.

The global fallback at line 423 and the last-resort at line 451 both attach the error handler unconditionally — apply the same pattern here:

🐛 Proposed fix 
         const child = spawn('uv', ['run', 'tldr', 'daemon', 'start', '--project', projectDir], {
           detached: true,
           stdio: 'ignore',
           cwd: tldrPath,
         });
+        // Must be registered unconditionally; pid guard below doesn't cover the
+        // TOCTOU case where uv disappears between the version check and spawn.
+        child.on('error', () => { /* uv disappeared between check and spawn */ });
         if (child.pid !== undefined) {
-          // Suppress unhandled 'error' event if uv disappears between version-check and spawn.
-          // Note: the spin-loop below blocks the event loop, so this fires only after
-          // tryStartDaemon returns — child.pid guard already covers sync spawn failures.
-          child.on('error', () => { /* uv disappeared between check and spawn */ });
           child.unref();
           started = true;
         }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In @.claude/hooks/src/daemon-client.ts around lines 403 - 410, The branch in
tryStartDaemon registers child.on('error', ...) only when child.pid !==
undefined, which leaves an unhandled 'error' event if spawn fails and pid is
undefined; change the logic to always attach the 'error' listener to the spawned
ChildProcess (the variable child) before checking child.pid so spawn failures
are caught, then keep the existing conditional child.unref()/started assignment
when pid exists; ensure the same error handler used by the global fallback is
applied to child in the tryStartDaemon code path.

// If child.pid is undefined the OS rejected the spawn — fall through to fallback
} catch {
// uv not available — fall through to global tldr fallback below
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
}

// Fallback to global tldr if local didn't work
// Skip fallback in dev mode (TLDR_DEV=1) to prevent duplicate daemons
// Fallback to global tldr if local didn't start (or TLDR_DEV is set)
if (!started && !process.env.TLDR_DEV) {
spawnSync('tldr', ['daemon', 'start', '--project', projectDir], {
timeout: 5000,
const child = spawn('tldr', ['daemon', 'start', '--project', projectDir], {
detached: true,
stdio: 'ignore',
windowsHide: true,
});
child.on('error', () => { /* tldr not in PATH — callers degrade gracefully */ });
child.unref();
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.

// Wait for daemon to become reachable (up to 10s for slow starts)
Expand All @@ -418,6 +439,30 @@ export function tryStartDaemon(projectDir: string): boolean {
while (Date.now() < end) { /* spin */ }
}

// Last-resort fallback: uv was used but daemon never became reachable.
// This covers the case where uv is in PATH but tldr is not installed in
// the uv environment — uv exits non-zero silently, no PID file is written.
// Without this, started=true suppresses the global fallback and every
// subsequent invocation spins for 10s.
if (started && !isDaemonProcessRunning(projectDir) && !isDaemonReachable(projectDir) && !process.env.TLDR_DEV) {
const child = spawn('tldr', ['daemon', 'start', '--project', projectDir], {
detached: true,
stdio: 'ignore',
windowsHide: true,
});
child.on('error', () => {});
child.unref();

// Give the global daemon a brief window to become reachable for the current request.
// Without this wait, isDaemonReachable() below always returns false immediately.
const lrStart = Date.now();
while (Date.now() - lrStart < 3000) {
if (isDaemonReachable(projectDir)) break;
const wait = Date.now() + 100;
while (Date.now() < wait) { /* spin */ }
}
}

return isDaemonReachable(projectDir);
} finally {
// Always release lock
Expand Down