Skip to content

Commit 39b25bd

Browse files
authored
fix(init): terminate verification process trees (#1258)
## Summary Post-init verification currently kills only the direct shell process and waits for its close event. Package scripts that launch concurrently, framework dev servers, or file watchers leave descendants holding stdout and stderr open, so sentry init can remain stuck on Verifying setup even though the remote workflow already succeeded. This starts the verification command in a dedicated POSIX process group and terminates the complete tree during cleanup and signal forwarding. POSIX cleanup is bounded: it sends SIGTERM, escalates to SIGKILL after five seconds, and destroys the local pipe ends so inherited descriptors cannot keep the CLI alive. Windows force-terminates the disposable verification tree with a bounded `taskkill /T /F` invocation and falls back to the direct child if that fails. The verification path now also handles asynchronous child-process spawn errors. A missing or invalid dev command produces a best-effort verification warning instead of crashing a successful init run. ## Safety properties - Shell pipelines, concurrently tasks, framework servers, and watchers are terminated as one process tree. - POSIX descendants that ignore SIGTERM are force-killed after the grace period. - Cleanup never waits indefinitely for ChildProcess close. - SIGINT and SIGTERM are forwarded to the POSIX process group; Windows force-terminates the disposable tree before the signal is re-emitted by the CLI. - Windows `taskkill` calls have their own one-second timeout, debug diagnostics, and direct-child fallback. - Spawn failures remain non-fatal because verification runs after setup has succeeded. ## Test plan - `pnpm exec vitest run test/lib/init/verify-setup.test.ts test/lib/init/verify-setup-windows.mocked.test.ts test/lib/dev-script.test.ts test/lib/init/wizard-runner.test.ts` — 75 tests pass, including forced Windows tree cleanup, timeout/status diagnostics, and direct-child fallback. - `TZ=UTC pnpm exec vitest run test/lib test/commands test/types test/script --coverage --exclude test/lib/completions.property.test.ts` — 400 files, 8,459 tests pass, 14 skipped. - `pnpm exec tsc --noEmit` — passes. - `pnpm run lint` — 919 files pass. - `SENTRY_CLIENT_ID=test-build-only pnpm tsx script/build.ts --single` — darwin-arm64 build passes. - `git diff --check` — passes. The unmodified broad test command reaches 8,470 passing tests and 8 unrelated failures. Six time-range assertions assume UTC while the local timezone is Europe/Madrid. Two Bash completion simulations fail on the system Bash 3.2 because the generated script uses extglob syntax without enabling extglob; neither area is touched by this PR.
1 parent 6ae8f55 commit 39b25bd

3 files changed

Lines changed: 524 additions & 30 deletions

File tree

src/lib/init/verify-setup.ts

Lines changed: 148 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
* or fatal error patterns in stderr indicate failure.
1313
*/
1414

15-
import { type ChildProcess, spawn } from "node:child_process";
15+
import { type ChildProcess, spawn, spawnSync } from "node:child_process";
1616
import { resolve } from "node:path";
1717
import { captureException } from "@sentry/node-core/light";
1818
import { createSpotlightBuffer } from "@spotlightjs/spotlight/sdk";
@@ -26,6 +26,18 @@ import type { WizardUI } from "./ui/types.js";
2626
/** Verification timeout in seconds. */
2727
const VERIFY_TIMEOUT_S = 15;
2828

29+
/** POSIX grace period before escalating verification cleanup to SIGKILL. */
30+
const PROCESS_SHUTDOWN_GRACE_MS = 5000;
31+
32+
/** Maximum POSIX wait after force-killing the verification process tree. */
33+
const PROCESS_FORCE_SHUTDOWN_MS = 1000;
34+
35+
/** Poll interval while waiting for a verification process tree to exit. */
36+
const PROCESS_EXIT_POLL_MS = 50;
37+
38+
/** Maximum time for a Windows taskkill invocation to complete. */
39+
const WINDOWS_TREE_KILL_TIMEOUT_MS = 1000;
40+
2941
/**
3042
* Patterns in stderr/stdout that indicate a fatal startup failure.
3143
* Matched case-insensitively against each collected output line.
@@ -187,34 +199,124 @@ function buildVerifyEnv(
187199
return env;
188200
}
189201

190-
/** Gracefully kill a child process with SIGTERM → grace period → SIGKILL. */
191-
async function cleanupChild(child: ChildProcess): Promise<void> {
192-
if (child.exitCode !== null) {
193-
return;
202+
/** Signal the POSIX process group or force-terminate the Windows process tree. */
203+
function terminateProcessTree(
204+
child: ChildProcess,
205+
signal: NodeJS.Signals
206+
): boolean {
207+
const pid = child.pid;
208+
if (pid === undefined) {
209+
return false;
194210
}
211+
195212
try {
196-
child.kill("SIGTERM");
197-
let graceTimer: ReturnType<typeof setTimeout> | undefined;
198-
const exited = await Promise.race([
199-
new Promise<true>((r) => child.on("close", () => r(true))),
200-
new Promise<false>((r) => {
201-
graceTimer = setTimeout(() => r(false), 5000);
202-
}),
203-
]);
204-
clearTimeout(graceTimer);
205-
if (!exited && child.exitCode === null) {
206-
try {
207-
child.kill("SIGKILL");
208-
} catch {
209-
logger.debug("Child exited before SIGKILL");
213+
if (process.platform === "win32") {
214+
// Verification children are disposable. Force the complete tree in one
215+
// bounded call because Windows cannot reliably reproduce POSIX process-
216+
// group signaling or discover descendants after their parent exits.
217+
const args = ["/PID", String(pid), "/T", "/F"];
218+
const taskkill = spawnSync("taskkill", args, {
219+
stdio: "ignore",
220+
timeout: WINDOWS_TREE_KILL_TIMEOUT_MS,
221+
windowsHide: true,
222+
});
223+
if (taskkill.error) {
224+
logger.debug(
225+
`Failed to terminate Windows verification process tree while handling ${signal}`,
226+
taskkill.error
227+
);
228+
return false;
210229
}
211-
// Await close so child.exitCode is populated for crash detection.
212-
if (child.exitCode === null) {
213-
await new Promise<void>((r) => child.on("close", () => r()));
230+
if (taskkill.status !== 0) {
231+
logger.debug(
232+
`taskkill exited with status ${taskkill.status} while handling ${signal} for the verification process tree`
233+
);
234+
return false;
214235
}
236+
return true;
237+
}
238+
// Verification children are spawned detached on POSIX, making their PID
239+
// the process-group ID. A negative PID signals the whole group, including
240+
// shell pipelines, concurrently tasks, and framework watchers.
241+
process.kill(-pid, signal);
242+
return true;
243+
} catch (error) {
244+
const code = (error as NodeJS.ErrnoException).code;
245+
if (code !== "ESRCH") {
246+
logger.debug(`Failed to signal verification process tree with ${signal}`);
215247
}
248+
return false;
249+
}
250+
}
251+
252+
/** Fall back to signaling only the direct child process. */
253+
function signalDirectChild(child: ChildProcess, signal: NodeJS.Signals): void {
254+
try {
255+
child.kill(signal);
256+
} catch {
257+
logger.debug(`Verification child exited before receiving ${signal}`);
258+
}
259+
}
260+
261+
/** Check whether the POSIX verification process group is still alive. */
262+
function isPosixProcessGroupAlive(child: ChildProcess): boolean {
263+
const pid = child.pid;
264+
if (pid === undefined) {
265+
return false;
266+
}
267+
268+
try {
269+
process.kill(-pid, 0);
270+
return true;
216271
} catch (error) {
217-
logger.debug("Failed to kill verification child", error);
272+
return (error as NodeJS.ErrnoException).code !== "ESRCH";
273+
}
274+
}
275+
276+
/** Wait for a POSIX verification process group to exit, bounded by a timeout. */
277+
async function waitForPosixProcessGroupExit(
278+
child: ChildProcess,
279+
timeoutMs: number
280+
): Promise<boolean> {
281+
const deadline = Date.now() + timeoutMs;
282+
while (isPosixProcessGroupAlive(child)) {
283+
const remainingMs = deadline - Date.now();
284+
if (remainingMs <= 0) {
285+
return false;
286+
}
287+
await new Promise<void>((resolveDelay) => {
288+
setTimeout(resolveDelay, Math.min(PROCESS_EXIT_POLL_MS, remainingMs));
289+
});
290+
}
291+
return true;
292+
}
293+
294+
/** Terminate the process tree, with a grace period on POSIX. */
295+
async function cleanupProcessTree(child: ChildProcess): Promise<void> {
296+
try {
297+
if (process.platform === "win32") {
298+
if (!terminateProcessTree(child, "SIGKILL")) {
299+
signalDirectChild(child, "SIGKILL");
300+
}
301+
return;
302+
}
303+
304+
terminateProcessTree(child, "SIGTERM");
305+
const exited = await waitForPosixProcessGroupExit(
306+
child,
307+
PROCESS_SHUTDOWN_GRACE_MS
308+
);
309+
if (!exited) {
310+
terminateProcessTree(child, "SIGKILL");
311+
await waitForPosixProcessGroupExit(child, PROCESS_FORCE_SHUTDOWN_MS);
312+
}
313+
} catch (error) {
314+
logger.debug("Failed to kill verification process tree", error);
315+
} finally {
316+
// Descendants can inherit these pipes. Destroying our ends guarantees the
317+
// verification cleanup itself never waits indefinitely for `close`.
318+
child.stdout?.destroy();
319+
child.stderr?.destroy();
218320
}
219321
}
220322

@@ -272,6 +374,9 @@ export async function verifySetup(
272374
const [cmd = "", ...cmdArgs] = detected.args;
273375
child = spawn(cmd, cmdArgs, {
274376
cwd,
377+
// On POSIX this creates a dedicated process group whose entire tree can
378+
// be terminated without touching the CLI's own process group.
379+
detached: process.platform !== "win32",
275380
env: childEnv,
276381
stdio: ["ignore", "pipe", "pipe"],
277382
});
@@ -286,10 +391,8 @@ export async function verifySetup(
286391
// cleanup instead of silently continuing the wizard.
287392
let signalReceived: NodeJS.Signals | null = null;
288393
const safeKill = (sig: NodeJS.Signals) => {
289-
try {
290-
child.kill(sig);
291-
} catch {
292-
logger.debug(`Child already exited when forwarding ${sig}`);
394+
if (!terminateProcessTree(child, sig)) {
395+
signalDirectChild(child, sig);
293396
}
294397
};
295398
const onSigint = () => {
@@ -312,12 +415,20 @@ export async function verifySetup(
312415
r({ kind: "exited" as const, code: code ?? 1 })
313416
);
314417
});
418+
const childErrored = new Promise<{ kind: "spawn_error"; error: Error }>(
419+
(resolveError) => {
420+
child.once("error", (error) => {
421+
resolveError({ kind: "spawn_error", error });
422+
});
423+
}
424+
);
315425

316426
let timeoutHandle: ReturnType<typeof setTimeout> | undefined;
317427
const outcome = await Promise.race([
318428
envelopeReceived.then(() => ({ kind: "envelope" as const })),
319429
startupPromise,
320430
childExited,
431+
childErrored,
321432
new Promise<{ kind: "timeout" }>((r) => {
322433
timeoutHandle = setTimeout(
323434
() => r({ kind: "timeout" as const }),
@@ -330,11 +441,11 @@ export async function verifySetup(
330441
clearTimeout(timeoutHandle);
331442
}
332443

333-
// Capture the exit code before cleanup — cleanupChild sends SIGTERM/SIGKILL
334-
// which would set exitCode to 143/137, masking a natural crash code.
444+
// Capture the exit code before cleanup — terminating the process tree would
445+
// set exitCode to 143/137, masking a natural crash code.
335446
const preCleanupExitCode = child.exitCode;
336447

337-
await cleanupChild(child);
448+
await cleanupProcessTree(child);
338449
if (subscriptionId) {
339450
buffer.unsubscribe(subscriptionId);
340451
}
@@ -369,6 +480,7 @@ type VerifyOutcome =
369480
| { kind: "envelope" }
370481
| StartupOutcome
371482
| { kind: "exited"; code: number }
483+
| { kind: "spawn_error"; error: Error }
372484
| { kind: "timeout" };
373485

374486
type ReportContext = {
@@ -401,6 +513,12 @@ function reportOutcome(outcome: VerifyOutcome, ctx: ReportContext): void {
401513
return;
402514
}
403515

516+
if (outcome.kind === "spawn_error") {
517+
logger.debug("Failed to spawn verification child", outcome.error);
518+
ui.log.warn("Skipping verification — could not start the dev command.");
519+
return;
520+
}
521+
404522
if (outcome.kind === "errored") {
405523
const scrubbed = scrubOutputLine(outcome.errorLine).slice(0, 200);
406524
ui.log.warn(`Could not verify — startup error: ${scrubbed}`);

0 commit comments

Comments
 (0)