Files
software-mansion__argent/packages/tool-server/test/boot-electron-kill.test.ts
Ignacy Łątka 13f43bd0cd test(chromium): pin the ports-file guard and the readiness failure code (#862)
Follow-up to #709, which merged while this was still under review. #709
landed the port fix; these are the two gaps a review pass over it turned
up, both in the same two files.

## The ports-file guard was pinned by nothing

#714 added `vi.mock("../src/utils/chromium-discovery", …)` to
`boot-electron-spawn-error.test.ts` so a completed boot stops persisting
its port into the developer's real `~/.argent/chromium-cdp-ports.json`.
Nothing asserted it still worked.

Measured, on the pre-change file: deleting the mock leaves the suite
**green at 9/9** while `$HOME/.argent/chromium-cdp-ports.json` is
created containing a live ephemeral port. Replacing it with a
call-through variant — the shape `list-devices.test.ts:76-81` already
uses on this same module — does the same:

```ts
vi.mock("../src/utils/chromium-discovery", async () => {
  const actual = await vi.importActual<...>("../src/utils/chromium-discovery");
  return { ...actual,
    trackChromiumPort: vi.fn(actual.trackChromiumPort),
    untrackChromiumPort: vi.fn(actual.untrackChromiumPort) };
});
```
→ `Tests 9 passed (9)`, and the ports file contains `[55770]`.

That is how the file accumulated entries before #714, so the guard
rotting back is not hypothetical. Each file now scopes `HOME` with
#714's own `scopeTempHome()` helper and asserts, after a completed boot,
that no ports file exists. Both mutations now fail.

**All three files that carry this mock are fixed, not just the one.**
`boot-electron-kill.test.ts` and `boot-electron-ready-race.test.ts` hold
the byte-identical mock — under a comment naming each other — and each
completes a real boot. Measured before this change: deleting the mock
left `kill` green at **15/15** while eleven live ports, one per boot,
were written; the call-through variant did the same to `ready-race`
**while still satisfying its own
`expect(trackChromiumPort).toHaveBeenCalledWith(port)` at line 175**,
which is precisely why the absence assertion is the right shape.

Asserting the absence rather than
`expect(trackChromiumPort).toHaveBeenCalledWith(port)` is deliberate:
the call is already pinned by `boot-electron-ready-race.test.ts:175`,
and a spy assertion only proves the export *is* a spy — the call-through
mutation satisfies it while still writing.

## The readiness timeout was pinned by its wording, not its code

The catch-path test matched `/CDP never became reachable/`.
`FAILURE_CODES.CHROMIUM_ELECTRON_CDP_TIMEOUT` had **no assertion
anywhere in the repo** — `grep -rn CHROMIUM_ELECTRON_CDP_TIMEOUT
packages/ --include=*.ts` outside `dist/` hits only the throw site and
the enum. Swapping the throw site to `CHROMIUM_ELECTRON_SPAWN_FAILED`
left `boot-electron-spawn-error` + `-kill` + `-ready-race` +
`failure-classification` + `chromium-discovery` at **60/60 passed**.

So the half agents and telemetry bucket on was free to change, while a
harmless reword would have broken the test. Now pinned by code via
`getFailureSignal`, matching how `boot-electron-ready-race.test.ts:137`
pins its own failure path. It still discriminates the vacuity the
message match was added for: a nonexistent `appPath` yields
`CHROMIUM_ELECTRON_APP_PATH_INVALID` and fails. `failure_stage` is
pinned alongside it, as `ready-race` already does for its own path —
`electron_cdp_ready` likewise had no assertion anywhere.

## Two rationales corrected

- The catch-path comment blamed "no HTTP server on the port" for the
probe failing. `fetch` declines port 1 as a WHATWG bad port *before it
dials* — verified with `net.connect` hooked: **0 sockets opened** for
port 1, 2 sockets for ports 2, 1023 and 19222. No listener is involved
either way, and the file's own constant JSDoc says so.
- The `chromium-discovery.test.ts` constant pointed at "a `listen(0)`
fake CDP server **in a sibling test file**", when the server that makes
the point is `startFakeCdpServer` in this same file, above the constant.
- It also credited "anything in the unprivileged range" with being
bindable by `listen(0)`. `listen(0)` draws from the kernel's *ephemeral*
range — `net.inet.ip.portrange` 49152-65535 on macOS,
`ip_local_port_range` 32768-60999 on the Ubuntu runner. That narrower
range is what put the old `43211` at risk; calling it "unprivileged"
also revives the privilege reasoning the sibling constant explicitly
disowns.

## `chromium-discovery.test.ts` gets a second layer too

It is the one file here that drives the **real** `persistPorts`, and its
only guard was a single `beforeAll` line setting
`ARGENT_CHROMIUM_PORTS_FILE`. That line is pinned — deleting it fails 3
tests — but the write lands before the detection, and `persistPorts`
merges rather than replaces, so on a real machine the test ports would
be appended for `list-devices` to probe afterwards. It now scopes `HOME`
as well, so a lapsed redirect falls back somewhere disposable. Measured
with a pre-seeded `$HOME/.argent/chromium-cdp-ports.json` = `[9222]`:
deleting the redirect still fails 3 tests, and the seeded file comes
back **unchanged**.

## Verification

| mutation | before | after |
|---|---|---|
| delete the module mock | 9/9 pass, ports file written | fails |
| call-through `vi.fn(actual.trackChromiumPort)` | 9/9 pass, ports file
written | fails |
| swap `CHROMIUM_ELECTRON_CDP_TIMEOUT` at the throw site | 60/60 pass |
fails |
| nonexistent `appPath` in the catch-path test | — | fails
(`APP_PATH_INVALID`) |
| swap `failure_stage` at the throw site | — | fails |
| delete / call-through mock in `boot-electron-kill` | 15/15 pass, 11
ports written | 11 failed \| 4 passed |
| call-through mock in `boot-electron-ready-race` | 2/2 pass | 1 failed
\| 1 passed |

Full `packages/tool-server` in 6 shards: 4303 passed, 1 skipped, every
shard exit 0, and no `.argent` directory created under a redirected
`HOME`. `eslint` 0, `prettier --check` 0, `tsc --build` 0, `tsc -p
tsconfig.test.json` 0, `knip` 0 — exit codes read directly.
2026-08-21 12:00:50 +02:00

445 lines
16 KiB
TypeScript

/**
* Teardown-path tests for runner-booted Chromium: `killChromiumByPort` must
* prefer the ChildProcess handle retained at boot — whose exitCode/signalCode
* guard skips the delayed leader SIGKILL once the child has exited, while the
* group sweep escalates on a signal-0 liveness probe of the group — and only
* fall back to group signalling on the raw pid (group SIGTERM, then a
* probe-gated group SIGKILL) once the child has left the registry.
*/
import { describe, it, expect, vi, beforeEach, afterEach, beforeAll, afterAll } from "vitest";
import { EventEmitter } from "node:events";
import * as fs from "node:fs";
import * as http from "node:http";
import * as os from "node:os";
import * as path from "node:path";
import { scopeTempHome } from "./helpers/temp-home";
scopeTempHome("argent-boot-electron-kill-home-");
const spawnMock = vi.fn();
vi.mock("node:child_process", async () => {
const actual = await vi.importActual<typeof import("node:child_process")>("node:child_process");
return {
...actual,
spawn: (cmd: string, args: string[], opts: unknown) => spawnMock(cmd, args, opts),
};
});
// Keep trackChromiumPort from persisting booted ports to on-disk state.
vi.mock("../src/utils/chromium-discovery", () => ({
trackChromiumPort: vi.fn(),
untrackChromiumPort: vi.fn(),
}));
import {
bootElectronApp,
killChromiumByPort,
killChromiumByPortAndWait,
} from "../src/tools/devices/boot-electron";
const FAKE_PID = 4242;
interface FakeChild extends EventEmitter {
pid: number | undefined;
stderr: EventEmitter;
unref: () => void;
kill: ReturnType<typeof vi.fn>;
exitCode: number | null;
signalCode: NodeJS.Signals | null;
}
function makeFakeChild(): FakeChild {
const ee = new EventEmitter() as FakeChild;
ee.pid = FAKE_PID;
ee.stderr = new EventEmitter();
ee.unref = () => {};
ee.kill = vi.fn(() => true);
ee.exitCode = null;
ee.signalCode = null;
return ee;
}
let appDir: string;
beforeAll(() => {
// resolveLauncher() fs-checks the app path before spawn, so the test needs
// a real directory on disk. The spawn itself is mocked, so the contents
// don't matter — only the path's existence.
appDir = fs.mkdtempSync(path.join(os.tmpdir(), "argent-boot-electron-kill-test-"));
fs.writeFileSync(
path.join(appDir, "package.json"),
JSON.stringify({ name: "fake-electron-app", main: "main.js" })
);
fs.writeFileSync(path.join(appDir, "main.js"), "// fake\n");
});
afterAll(() => {
if (appDir) fs.rmSync(appDir, { recursive: true, force: true });
});
beforeEach(() => {
spawnMock.mockReset();
// Teardown sweeps the child's process group. The fake children carry a
// stand-in pid, so real signals must never escape the test; individual tests
// re-spy when they need to assert on the calls.
vi.spyOn(process, "kill").mockImplementation(() => true);
});
afterEach(() => {
vi.useRealTimers();
vi.restoreAllMocks();
});
/**
* Minimal CDP stub: only /json/version, which is all waitForCdpReady probes.
* Booting against it lands the fake child in the boot-electron handle
* registry, which is the state under test.
*/
async function bootFakeChild(): Promise<{ child: FakeChild; port: number; close: () => void }> {
const child = makeFakeChild();
spawnMock.mockReturnValue(child);
const srv = http.createServer((req, res) => {
res.setHeader("content-type", "application/json");
if (req.url === "/json/version") {
res.end(JSON.stringify({ "Browser": "Chrome/Test", "Protocol-Version": "1.3" }));
return;
}
res.statusCode = 404;
res.end();
});
await new Promise<void>((resolve) => srv.listen(0, "127.0.0.1", resolve));
const { port } = srv.address() as { port: number };
await bootElectronApp({ appPath: appDir, port, readyTimeoutMs: 5000 });
// Both guards are load-bearing: the mock above has to actually intercept
// trackChromiumPort — deleted or calling through, it is invisible — and the
// scoped HOME keeps the port every boot here persists out of the real file.
expect(fs.existsSync(path.join(os.homedir(), ".argent", "chromium-cdp-ports.json"))).toBe(false);
return { child, port, close: () => srv.close() };
}
describe("killChromiumByPort — handle path", () => {
it("kills through the retained handle: SIGTERM, then SIGKILL once the grace period elapses with the child still running", async () => {
const { child, port, close } = await bootFakeChild();
try {
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
// The handle path was taken (child.kill, not process.kill on a raw pid).
expect(child.kill).toHaveBeenCalledTimes(1);
expect(child.kill).toHaveBeenCalledWith("SIGTERM");
// Child ignores the SIGTERM (exitCode stays null) → escalate after 2s.
vi.advanceTimersByTime(2000);
expect(child.kill).toHaveBeenCalledTimes(2);
expect(child.kill).toHaveBeenLastCalledWith("SIGKILL");
} finally {
close();
}
});
it("skips the delayed SIGKILL when the child exits within the grace period (the recycled-pid guard)", async () => {
const { child, port, close } = await bootFakeChild();
try {
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
expect(child.kill).toHaveBeenCalledWith("SIGTERM");
// The SIGTERM lands: the child exits before the escalation timer fires.
child.exitCode = 0;
vi.advanceTimersByTime(2000);
expect(child.kill).toHaveBeenCalledTimes(1); // no SIGKILL
} finally {
close();
}
});
it("sends no group SIGTERM while the handle is live — the wrapper forwards it for a clean quit", async () => {
// A group SIGTERM here would hit the browser and every Chromium helper
// directly, bypassing Electron's quit sequence (before-quit/will-quit
// never run). The handle SIGTERM alone must carry the graceful request.
const { child, port, close } = await bootFakeChild();
try {
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
expect(child.kill).toHaveBeenCalledWith("SIGTERM");
expect(killSpy).not.toHaveBeenCalledWith(-FAKE_PID, "SIGTERM");
} finally {
close();
}
});
it("sends the group SIGTERM when the child already exited, so orphaned survivors get a graceful request", async () => {
// The boot-failure path's shape: the child died (or was never viable)
// before the kill, so child.kill reaches nothing and the group SIGTERM
// must carry the graceful request to any surviving helpers.
const { child, port, close } = await bootFakeChild();
try {
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
vi.useFakeTimers();
child.exitCode = 0;
killChromiumByPort(port, FAKE_PID);
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGTERM");
} finally {
close();
}
});
it("escalates the group to SIGKILL when anything in it outlives the grace period", async () => {
const { child, port, close } = await bootFakeChild();
try {
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
// The leader exits promptly; a helper does not. The old exit-status guard
// alone would skip the escalation and strand it.
child.exitCode = 0;
vi.advanceTimersByTime(2000);
expect(child.kill).toHaveBeenCalledTimes(1); // no SIGKILL on the leader
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, 0); // group liveness probe
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGKILL");
} finally {
close();
}
});
it("does not escalate a group that already emptied", async () => {
const { child, port, close } = await bootFakeChild();
try {
const killSpy = vi.spyOn(process, "kill").mockImplementation(((
_pid: number,
signal: NodeJS.Signals | 0
) => {
if (signal === 0) {
const err: NodeJS.ErrnoException = new Error("no such process");
err.code = "ESRCH";
throw err;
}
return true;
}) as typeof process.kill);
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
child.exitCode = 0;
vi.advanceTimersByTime(2000);
const signals = killSpy.mock.calls.map((c) => c[1]);
expect(signals).not.toContain("SIGKILL");
} finally {
close();
}
});
});
describe("killChromiumByPort — raw-pid fallback", () => {
it("falls back to group signalling after the child's exit evicted the handle, and the liveness probe suppresses the SIGKILL", async () => {
const { child, port, close } = await bootFakeChild();
try {
// Natural exit (e.g. the user closed the window): the cleanup listener
// must drop the handle from the registry.
child.emit("exit", 0, null);
const killSpy = vi
.spyOn(process, "kill")
.mockImplementation((_pid: number, signal?: string | number) => {
if (signal === 0) {
// By probe time the group is empty.
const err = new Error("kill ESRCH") as NodeJS.ErrnoException;
err.code = "ESRCH";
throw err;
}
return true;
});
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
// Handle gone → raw-pid fallback, never the stale ChildProcess.
expect(child.kill).not.toHaveBeenCalled();
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGTERM");
vi.advanceTimersByTime(2000);
// The 0-probe reported ESRCH → the SIGKILL must be suppressed so it
// cannot land on a recycled pgid.
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, 0);
const signals = killSpy.mock.calls.map((c) => c[1]);
expect(signals).not.toContain("SIGKILL");
} finally {
close();
}
});
it("escalates the fallback to a group SIGKILL when the group is still alive at probe time", () => {
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
vi.useFakeTimers();
// Port never registered → straight to the pid fallback.
killChromiumByPort(59999, FAKE_PID);
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGTERM");
vi.advanceTimersByTime(2000);
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, 0);
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGKILL");
});
it("still arms the group escalation when the leader pid is already gone but the group is not", async () => {
// The asymmetric leak: the wrapper died independently (external kill -9,
// OOM kill), evicting the handle, and its bare pid reports ESRCH — while
// the browser and helpers keep the group alive. A leader-only fallback
// would return early here and never signal any of them.
const { child, port, close } = await bootFakeChild();
try {
child.emit("exit", null, "SIGKILL");
const killSpy = vi.spyOn(process, "kill").mockImplementation(((
pid: number,
_signal: NodeJS.Signals | 0
) => {
if (pid === FAKE_PID) {
const err: NodeJS.ErrnoException = new Error("no such process");
err.code = "ESRCH";
throw err; // the leader is gone; only group members survive
}
return true;
}) as typeof process.kill);
vi.useFakeTimers();
killChromiumByPort(port, FAKE_PID);
expect(child.kill).not.toHaveBeenCalled();
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGTERM");
vi.advanceTimersByTime(2000);
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, 0);
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGKILL");
} finally {
close();
}
});
it("is a no-op when no handle is registered and no pid is provided", () => {
const killSpy = vi.spyOn(process, "kill").mockImplementation(() => true);
killChromiumByPort(58888);
expect(killSpy).not.toHaveBeenCalled();
});
});
/** When the faked process group starts reporting ESRCH, in the raw-pid poll tests. */
const GONE_AFTER_MS = 40;
describe("killChromiumByPortAndWait", () => {
it("resolves only once the child has actually exited", async () => {
const { child, port, close } = await bootFakeChild();
try {
let settled = false;
const waited = killChromiumByPortAndWait(port, FAKE_PID).then(() => {
settled = true;
});
expect(child.kill).toHaveBeenCalledWith("SIGTERM");
// The signal is out, but the process is still holding its lock. Flushed
// through a macrotask so a wait that never awaits the child at all shows
// up here as already-settled rather than passing on microtask timing.
await new Promise((r) => setTimeout(r, 20));
expect(settled).toBe(false);
child.exitCode = 0;
child.emit("exit", 0, null);
await waited;
expect(settled).toBe(true);
} finally {
close();
}
});
it("gives up after the timeout so a wedged process can't stall a run", async () => {
const { child, port, close } = await bootFakeChild();
try {
// Child never emits exit — the wait must still resolve.
await killChromiumByPortAndWait(port, FAKE_PID, 20);
expect(child.kill).toHaveBeenCalledWith("SIGTERM");
} finally {
close();
}
});
it("returns immediately when the child already exited", async () => {
const { child, port, close } = await bootFakeChild();
try {
child.exitCode = 0;
// No exit event is ever emitted here, so a wait that didn't notice the
// already-exited child would hang past the test timeout.
await killChromiumByPortAndWait(port, FAKE_PID, 30_000);
expect(child.kill).toHaveBeenCalledWith("SIGTERM");
} finally {
close();
}
});
it("polls the group when no handle is held, and stops once it reports ESRCH", async () => {
let alive = true;
const killSpy = vi.spyOn(process, "kill").mockImplementation(((
_pid: number,
signal: NodeJS.Signals | 0
) => {
if (signal === 0 && !alive) {
const err: NodeJS.ErrnoException = new Error("no such process");
err.code = "ESRCH";
throw err;
}
return true;
}) as typeof process.kill);
setTimeout(() => {
alive = false;
}, GONE_AFTER_MS).unref();
// Port never registered → the raw-pid path, which has no exit event to await.
const started = Date.now();
await killChromiumByPortAndWait(59998, FAKE_PID, 2000);
const elapsed = Date.now() - started;
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, "SIGTERM");
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, 0);
// Two-sided: it must not return while the group is still alive (the whole
// point — the replacement would race the lock), and must not burn the whole
// budget once it is gone (every handle-less retire would stall the flow).
expect(elapsed).toBeGreaterThanOrEqual(GONE_AFTER_MS);
expect(elapsed).toBeLessThan(1000);
});
it("does not return while the group is still alive, even though the leader itself is gone", async () => {
// The single-instance lock the wait guards is held by the browser — a
// group member — so leader liveness is the wrong signal: here the bare pid
// reports ESRCH from the start while the group lives on for a while.
let groupAlive = true;
const killSpy = vi.spyOn(process, "kill").mockImplementation(((
pid: number,
signal: NodeJS.Signals | 0
) => {
const err: NodeJS.ErrnoException = new Error("no such process");
err.code = "ESRCH";
if (pid === FAKE_PID) throw err; // the leader is already dead
if (signal === 0 && !groupAlive) throw err;
return true;
}) as typeof process.kill);
setTimeout(() => {
groupAlive = false;
}, GONE_AFTER_MS).unref();
const started = Date.now();
await killChromiumByPortAndWait(59997, FAKE_PID, 2000);
const elapsed = Date.now() - started;
expect(killSpy).toHaveBeenCalledWith(-FAKE_PID, 0);
expect(elapsed).toBeGreaterThanOrEqual(GONE_AFTER_MS);
expect(elapsed).toBeLessThan(1000);
});
});