mirror of
https://github.com/OpenRouterTeam/spawn.git
synced 2026-05-20 01:11:18 +00:00
fix(ssh): always escalate to SIGKILL in killWithTimeout (#2752)
proc.killed is true as soon as kill() is called, not when the process exits. This meant SIGKILL escalation was always skipped, leaving stuck processes hanging indefinitely. Remove the faulty guard and always attempt SIGKILL after the grace period — try/catch handles already-dead processes. Co-authored-by: Claude <claude@anthropic.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
a557fb1002
commit
035ee3ca63
2 changed files with 55 additions and 6 deletions
54
packages/cli/src/__tests__/kill-with-timeout.test.ts
Normal file
54
packages/cli/src/__tests__/kill-with-timeout.test.ts
Normal file
|
|
@ -0,0 +1,54 @@
|
|||
import { describe, expect, it } from "bun:test";
|
||||
import { killWithTimeout } from "../shared/ssh";
|
||||
|
||||
describe("killWithTimeout", () => {
|
||||
it("sends SIGKILL after grace period if process ignores SIGTERM", async () => {
|
||||
const signals: (number | undefined)[] = [];
|
||||
const proc = {
|
||||
kill(signal?: number) {
|
||||
signals.push(signal);
|
||||
},
|
||||
};
|
||||
|
||||
killWithTimeout(proc, 100);
|
||||
await new Promise((r) => setTimeout(r, 200));
|
||||
|
||||
expect(signals).toEqual([
|
||||
undefined,
|
||||
9,
|
||||
]);
|
||||
});
|
||||
|
||||
it("does not throw if process is already dead when SIGKILL fires", async () => {
|
||||
let callCount = 0;
|
||||
const proc = {
|
||||
kill(signal?: number) {
|
||||
callCount++;
|
||||
if (callCount > 1) {
|
||||
throw new Error("No such process");
|
||||
}
|
||||
},
|
||||
};
|
||||
|
||||
killWithTimeout(proc, 50);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
// Should not throw — the error is caught internally
|
||||
expect(callCount).toBe(2);
|
||||
});
|
||||
|
||||
it("does not send SIGKILL if initial SIGTERM throws", async () => {
|
||||
const signals: (number | undefined)[] = [];
|
||||
const proc = {
|
||||
kill(_signal?: number) {
|
||||
throw new Error("No such process");
|
||||
},
|
||||
};
|
||||
|
||||
killWithTimeout(proc, 50);
|
||||
await new Promise((r) => setTimeout(r, 150));
|
||||
|
||||
// No signals recorded because the first kill() threw
|
||||
expect(signals).toEqual([]);
|
||||
});
|
||||
});
|
||||
|
|
@ -112,7 +112,6 @@ export function sleep(ms: number): Promise<void> {
|
|||
export function killWithTimeout(
|
||||
proc: {
|
||||
kill(signal?: number): void;
|
||||
readonly killed: boolean;
|
||||
},
|
||||
gracePeriodMs = 5000,
|
||||
): void {
|
||||
|
|
@ -121,11 +120,7 @@ export function killWithTimeout(
|
|||
return;
|
||||
}
|
||||
const sigkillTimer = setTimeout(() => {
|
||||
tryCatch(() => {
|
||||
if (!proc.killed) {
|
||||
proc.kill(9);
|
||||
}
|
||||
});
|
||||
tryCatch(() => proc.kill(9));
|
||||
}, gracePeriodMs);
|
||||
// Don't let this timer keep the event loop alive — the process may already
|
||||
// be dead from SIGTERM, so there's no reason to block exit for 5 seconds.
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue