fix(anime): report a failed bridge kill instead of a generic timeout

- Track kill errors separately from exit/spawn so a signal failure (e.g. EPERM) surfaces its own message instead of a bare "did not exit after SIGKILL"
- Attach stop() failures as the cause on the readiness-timeout error so a surviving child's kill failure isn't dropped
- Skip stop() entirely when spawn itself failed, and stop polling once the ready deadline has passed
- Add stopTimeoutMs test hook and a test covering a kill that fails without the child exiting
This commit is contained in:
2026-08-01 01:50:19 -07:00
parent 0dfba67ae8
commit 4eaaf5769f
2 changed files with 64 additions and 13 deletions
+27 -2
View File
@@ -11,12 +11,17 @@ const binaries: BundleBinaries = {
jarPath: '/tmp/MExtensionServer.jar',
};
/** A ChildProcess stand-in: an EventEmitter with the bits startSidecar touches. */
function fakeChild(onKill?: (child: EventEmitter) => void): ChildProcess {
/**
* A ChildProcess stand-in: an EventEmitter with the bits startSidecar touches.
* A `pid` marks a child that did spawn, which is how a kill failure is told
* apart from a spawn failure.
*/
function fakeChild(onKill?: (child: EventEmitter) => void, pid?: number): ChildProcess {
const child = new EventEmitter();
Object.assign(child, {
stdout: null,
stderr: null,
pid,
kill: () => {
onKill?.(child);
return true;
@@ -52,6 +57,26 @@ test('a readiness timeout shuts the child down and reports the timeout', async (
);
});
test('a kill that fails without an exit is reported, not counted as a shutdown', async () => {
const port = await allocatePort();
// Signalling fails (EPERM-style) and the child never goes: it may still hold
// the port, so the stop must not report success.
const child = fakeChild(
(emitter) => queueMicrotask(() => emitter.emit('error', new Error('kill EPERM'))),
4242,
);
const spawnImpl = (() => child) as unknown as typeof spawnType;
await assert.rejects(
() => startSidecar({ binaries, port, readyTimeoutMs: 50, stopTimeoutMs: 10, spawnImpl }),
(error: Error) => {
assert.match(error.message, /did not become ready within 50ms/);
assert.match((error.cause as Error).message, /could not be killed.*EPERM/);
return true;
},
);
});
test('an early exit is reported with its code rather than waiting out the deadline', async () => {
const port = await allocatePort();
const child = fakeChild();
+37 -11
View File
@@ -51,6 +51,8 @@ export interface StartSidecarOptions {
binaries: BundleBinaries;
port?: number;
readyTimeoutMs?: number;
/** How long to wait for the child to go after each stop signal. Tests only. */
stopTimeoutMs?: number;
spawnImpl?: typeof spawn;
onLog?: (line: string) => void;
}
@@ -83,6 +85,8 @@ export async function startSidecar(options: StartSidecarOptions): Promise<Sideca
let exited: { code: number | null; signal: NodeJS.Signals | null } | null = null;
let spawnError: Error | null = null;
let killError: Error | null = null;
const stopTimeoutMs = options.stopTimeoutMs ?? STOP_TIMEOUT_MS;
const hasExited = new Promise<void>((resolve) => {
child.once('exit', (code, signal) => {
exited = { code, signal };
@@ -90,16 +94,24 @@ export async function startSidecar(options: StartSidecarOptions): Promise<Sideca
});
// A ChildProcess is an EventEmitter: without this listener a failed spawn
// (a missing or non-executable java) throws in the main process instead of
// failing the readiness loop below.
child.once('error', (error: Error) => {
spawnError = error;
if (exited === null) exited = { code: null, signal: null };
resolve();
// failing the readiness loop below. `error` never proves the child is gone
// -- only `exit` does -- so it must never set `exited`, or a kill that
// failed would look like a clean shutdown. A spawn failure is told apart
// from a later kill failure by the missing pid.
child.on('error', (error: Error) => {
if (child.pid === undefined) {
spawnError = error;
resolve();
return;
}
killError = error;
});
});
const stop = async (): Promise<void> => {
if (exited !== null) return;
// Nothing was ever spawned, so there is nothing to signal or wait for.
if (spawnError !== null) return;
// Graceful first: the server exposes a shutdown endpoint.
try {
await fetch(`${baseUrl}/stop`, { signal: AbortSignal.timeout(2000) });
@@ -110,14 +122,20 @@ export async function startSidecar(options: StartSidecarOptions): Promise<Sideca
child.kill();
// kill() only sends the signal. Wait for the process to actually go, so a
// restart cannot race the old one still holding the port.
await Promise.race([hasExited, delay(STOP_TIMEOUT_MS)]);
await Promise.race([hasExited, delay(stopTimeoutMs)]);
if (exited === null) {
child.kill('SIGKILL');
await Promise.race([hasExited, delay(STOP_TIMEOUT_MS)]);
await Promise.race([hasExited, delay(stopTimeoutMs)]);
}
// Never report success while the child may still hold the port: a caller
// that restarts on the same port would race the survivor.
if (exited === null) {
const failedKill = killError as Error | null;
if (failedKill !== null) {
throw new Error(`Anime bridge could not be killed: ${failedKill.message}`, {
cause: failedKill,
});
}
throw new Error('Anime bridge did not exit after SIGKILL.');
}
};
@@ -143,12 +161,20 @@ export async function startSidecar(options: StartSidecarOptions): Promise<Sideca
};
return { baseUrl, port, client, stop, onExit };
}
await delay(READY_POLL_INTERVAL_MS);
// Sleeping past the deadline would only delay the timeout report.
const remaining = deadline - Date.now();
if (remaining <= 0) break;
await delay(Math.min(READY_POLL_INTERVAL_MS, remaining));
}
// A failed shutdown must not mask why startup failed.
await stop().catch(() => {});
throw new Error(
const timeout = new Error(
`Anime bridge did not become ready within ${options.readyTimeoutMs ?? DEFAULT_READY_TIMEOUT_MS}ms.`,
);
// A failed shutdown must not mask why startup failed, but it must not be
// dropped either: a surviving child still holds the port, which decides
// whether a caller may retry on it.
await stop().catch((error: unknown) => {
timeout.cause = error;
});
throw timeout;
}