fix(engine,droid-cli): stop subagents with parent task and harden droid-cli probes

engine: reviewer subagents previously kept running after the parent task
was moved out of in-progress, paused, or globally paused — they spawn
their own AgentSession outside `activeSessions`/`activeStepExecutors`,
so the existing kill paths never reached them. Track them in a per-task
`activeSubagentSessions` map (mirrored in TriageProcessor) and dispose
on the same triggers as the main session. ReviewOptions gains
`onSessionCreated` / `onSessionEnded` callbacks the executor and triage
processor wire to register/unregister.

droid-cli: probe timeouts (`validateCliPresence`, `validateCliAuth`,
`runDroidProbe`) raised from 5s to 45s — observed cold-start is ~20s,
so 5s reported the binary as missing even when present. Provider gains
a `FIRST_LINE_TIMEOUT_MS` (60s) cold-start guard so a hung droid binary
is reported with an actionable error instead of being indistinguishable
from a slow-thinking turn. Fix the await race in `streamViaCli`: when
SIGKILL destroys stdout mid-buffer, `rl` may never emit "close", so the
promise also resolves on `proc.close` and forces `rl.close()` — prevents
the engine's "executor did not unwind within 60s — hung subprocess".

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
gsxdsm
2026-05-01 06:44:07 -07:00
parent 4e82bd2eb3
commit 61d8fa2012
6 changed files with 238 additions and 4 deletions

View File

@@ -214,7 +214,7 @@ export function captureStderr(proc: ChildProcess): () => string {
*/
export function validateCliPresence(): void {
try {
execSync("droid --version", { stdio: "pipe", timeout: 5000 });
execSync("droid --version", { stdio: "pipe", timeout: 45000 });
} catch {
throw new Error(
"Droid CLI not found on PATH. Install Droid CLI and then run: droid auth login",
@@ -230,7 +230,7 @@ export function validateCliPresence(): void {
*/
export function validateCliAuth(): boolean {
try {
execSync("droid auth status", { stdio: "pipe", timeout: 5000 });
execSync("droid auth status", { stdio: "pipe", timeout: 45000 });
return true;
} catch {
console.warn(
@@ -251,7 +251,7 @@ export function validateCliAuth(): boolean {
* This async variant uses spawn so the loop keeps turning while the subprocess
* starts up.
*/
function runDroidProbe(args: string[], timeoutMs = 5000): Promise<number> {
function runDroidProbe(args: string[], timeoutMs = 45000): Promise<number> {
return new Promise((resolve) => {
const proc = spawn("droid", args, { stdio: "ignore" });
const timer = setTimeout(() => {

View File

@@ -61,6 +61,15 @@ import { isPiKnownDroidTool } from "./tool-mapping.js";
* arrives (e.g. someone embeds droid-cli without a stuck detector).
*/
const INACTIVITY_TIMEOUT_MS = 30 * 60_000;
/**
* Cold-start ceiling: kill the subprocess if it hasn't produced a single line
* of stdout within this window. Distinct from INACTIVITY_TIMEOUT_MS so a hung
* binary (no output ever) is reported with a clear cause instead of being
* indistinguishable from a slow-thinking turn. Observed cold-start on a healthy
* droid is ~20s; 60s gives 3x headroom for slow machines / cold caches.
*/
const FIRST_LINE_TIMEOUT_MS = 60_000;
function isDebugStreamEnabled(): boolean {
return process.env.PI_DROID_CLI_DEBUG === "1";
}
@@ -265,6 +274,19 @@ export function streamViaCli(
// Start inactivity timer after writing user message
resetInactivityTimer();
// Cold-start ceiling: only fires if firstLineReceived stays false. Cleared
// when the first line arrives, when proc closes, or on break-early. This
// distinguishes "droid never started" from "droid is taking a long time
// between thinking deltas" so the inactivity kill carries actionable info.
const firstLineTimer: ReturnType<typeof setTimeout> = setTimeout(() => {
if (firstLineReceived) return;
forceKillProcess(proc!);
endStreamWithError(
`Droid CLI produced no output within ${FIRST_LINE_TIMEOUT_MS / 1000}s — likely binary hang or auth failure (try \`droid --version\` and \`droid auth status\`)`,
);
}, FIRST_LINE_TIMEOUT_MS);
proc.on("close", () => clearTimeout(firstLineTimer));
// Process NDJSON lines from stdout using event-based callback
// NOTE: Using 'line' event instead of `for await` because the async
// iterator batches lines, breaking real-time streaming to pi.
@@ -319,6 +341,7 @@ export function streamViaCli(
debugLog("break-early triggered at message_stop after pi-known tool_use");
broken = true; // Set guard BEFORE rl.close() to prevent buffered lines
clearTimeout(inactivityTimer);
clearTimeout(firstLineTimer);
// Pi will execute these tools. Kill subprocess to prevent CLI from executing them.
forceKillProcess(proc!);
rl.close();
@@ -334,14 +357,24 @@ export function streamViaCli(
}
// For both success and error: clean up the subprocess
clearTimeout(inactivityTimer);
clearTimeout(firstLineTimer);
cleanupProcess(proc!);
rl.close();
}
});
// Wait for readline to close (result received or process ended)
// Wait for readline to close (result received or process ended).
// Also resolve on subprocess close: if SIGKILL races readline (e.g. after
// an external abort or watchdog kill), `rl` may never emit "close" because
// its input stream was destroyed mid-buffer. Forcing rl.close() from the
// proc close handler guarantees this await unblocks instead of hanging
// and triggering the engine's "executor did not unwind within 60s" path.
await new Promise<void>((resolve) => {
rl.on("close", resolve);
proc!.on("close", () => {
try { rl.close(); } catch { /* already closed */ }
resolve();
});
});
// Push done event after readline closes (async). Pushing synchronously