mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
7f85f996aa
## Summary Fixes **bucket B** of the 0.16.0 crash telemetry — the `toolserver_bind` / `EADDRINUSE` restart loop. It's the largest crash **volume** (~817 events, one machine alone = 785) even though it's a narrow user set (10). Companion to #531 (which fixed the widespread multi-user bucket A). ## Root cause (reproduced live) The main HTTP listener's `server.on("error")` handler routed **every** bind error through `crashShutdown` (exit 1). When a second tool-server starts on an already-owned port, that exit just feeds a supervisor respawn → re-bind → crash, forever. The telemetry signature was puzzling: all these crashes are `crash_phase=serving` with ~1s uptime and **0 tool calls** — i.e. the listener *bound successfully* and then died. I reproduced it: on macOS, a **second tool-server process's** `listen()` momentarily **succeeds** (the `listening` callback runs → `listening=true` → phase `serving`), and **then** the server emits an asynchronous `EADDRINUSE`. So the redundant instance binds, immediately errors, crashes, and the supervisor loops it (~1s each, 785× on one machine). Live A/B on the compiled `dist` (isolated port, telemetry disabled), server A up then a second server B on the same port: ``` buggy (current 0.16.0): B: "Tools server listening ..." → "Failed to bind ... EADDRINUSE" → exit 1 (→ respawn loop) fixed: B: "Tools server listening ..." → "deferring to it (redundant instance)" → exit 0 ``` A survives in both. ## Fix Handle `EADDRINUSE` specifically instead of crash-looping: - Probe the port for a healthy argent tool-server (`GET /tools` → 200, or 401 when a token is configured). - **If a healthy peer answers**, this instance is redundant → shut down **cleanly** (`shutdown(0)`) and defer to it. This removes it from the `reason:"crash"` restart-loop population entirely. - **If nothing healthy owns the port** (a foreign holder, or a wedged peer that doesn't answer within the probe timeout) → still crash, so a genuinely stuck port stays visible. - `EACCES` and all other server errors crash exactly as before. It also corrects the now-false comment claiming these crashes are phased `startup` ("the listen callback never ran"). Telemetry and the live repro prove the bind succeeds first, so `listening=true` and the phase is genuinely `serving`. ## Testing - **New unit tests** for `probeArgentToolServer`: peer answering 200 or 401 → `true`; foreign 500, connection-refused, and wedged/timeout → `false`. - Full tool-server suite **2731 green**; `tsc` + `eslint` clean. - **Live A/B** above, on the built dist. ## Note on scope This is a robustness fix that breaks the loop for the redundant-instance case (the dominant one). The precise reason macOS lets a second process's bind succeed-then-error asynchronously (vs. failing immediately, as same-process contention does) is a kernel/loopback timing detail; the fix does not depend on it — any `EADDRINUSE`, immediate or deferred, now defers-or-crashes deterministically rather than hot-looping.
221 lines
7.8 KiB
TypeScript
221 lines
7.8 KiB
TypeScript
import { describe, expect, it, vi, beforeEach, afterEach } from "vitest";
|
|
import { FAILURE_CODES } from "@argent/registry";
|
|
|
|
const telemetryMock = vi.hoisted(() => ({
|
|
init: vi.fn(),
|
|
attachRegistryTelemetry: vi.fn(() => ({
|
|
detach: vi.fn(),
|
|
recordInvocation: vi.fn(),
|
|
getTotalToolCalls: vi.fn(() => 0),
|
|
})),
|
|
track: vi.fn(),
|
|
shutdown: vi.fn().mockResolvedValue(undefined),
|
|
warmTelemetryIdentity: vi.fn().mockResolvedValue(undefined),
|
|
}));
|
|
|
|
const registryMock = vi.hoisted(() => ({
|
|
dispose: vi.fn().mockResolvedValue(undefined),
|
|
}));
|
|
|
|
// The probe is mocked so no test ever issues a real network request — the
|
|
// un-mocked probe would GET http://127.0.0.1:3001/tools, and the outcome (and
|
|
// so the test verdict) would depend on whatever happens to be listening on the
|
|
// developer's real default tool-server port when the suite runs.
|
|
const probeMock = vi.hoisted(() => ({
|
|
probeArgentToolServer: vi.fn<(host: string, port: number) => Promise<boolean>>(),
|
|
}));
|
|
|
|
// A minimal stand-in for the http.Server returned by app.listen(): just enough
|
|
// of the surface index.ts touches — `.on("error")` to register the handler, a
|
|
// manual `.emit()` to fire it, and the `.address()`/`.close()` shutdown() calls.
|
|
// (Hand-rolled rather than a real EventEmitter because vi.hoisted runs before
|
|
// module imports resolve.) `listen()` deliberately never invokes its success
|
|
// callback — that models a bind that fails, so `listening` stays false and the
|
|
// crash is phased as "startup".
|
|
const serverMock = vi.hoisted(() => {
|
|
const listeners = new Map<string, Array<(...args: unknown[]) => void>>();
|
|
return {
|
|
on(event: string, cb: (...args: unknown[]) => void) {
|
|
const list = listeners.get(event) ?? [];
|
|
list.push(cb);
|
|
listeners.set(event, list);
|
|
return this;
|
|
},
|
|
listenerCount(event: string) {
|
|
return listeners.get(event)?.length ?? 0;
|
|
},
|
|
emit(event: string, ...args: unknown[]) {
|
|
for (const cb of listeners.get(event) ?? []) cb(...args);
|
|
},
|
|
// Each test re-imports index.ts and calls start() again; without this the
|
|
// previous test's bind-error handler (a stale closure) would fire too.
|
|
reset() {
|
|
listeners.clear();
|
|
},
|
|
address: () => ({ port: 3001 }),
|
|
close: (cb: () => void) => cb(),
|
|
};
|
|
});
|
|
|
|
const httpHandleMock = vi.hoisted(() => ({
|
|
dispose: vi.fn(),
|
|
attachChromiumWebsockets: vi.fn(),
|
|
app: {
|
|
listen: vi.fn(() => serverMock),
|
|
},
|
|
}));
|
|
|
|
const updateCheckerMock = vi.hoisted(() => ({
|
|
dispose: vi.fn(),
|
|
}));
|
|
|
|
const watcherMock = vi.hoisted(() => ({
|
|
stop: vi.fn(),
|
|
}));
|
|
|
|
// Keep the REAL describeCrash so the bind-error handler produces genuine
|
|
// anonymous diagnostics (error_name / error_syscall / crash_fingerprint); stub
|
|
// the rest for assertion.
|
|
vi.mock("@argent/telemetry", async (importOriginal) => {
|
|
const actual = await importOriginal<typeof import("@argent/telemetry")>();
|
|
return { ...telemetryMock, describeCrash: actual.describeCrash };
|
|
});
|
|
vi.mock("@argent/registry", async (importOriginal) => {
|
|
const actual = await importOriginal<typeof import("@argent/registry")>();
|
|
return { ...actual, attachRegistryLogger: vi.fn() };
|
|
});
|
|
vi.mock("../src/utils/setup-registry", () => ({
|
|
createRegistry: vi.fn(() => registryMock),
|
|
}));
|
|
vi.mock("../src/utils/probe-argent-tool-server", () => probeMock);
|
|
vi.mock("../src/http", () => ({
|
|
createHttpApp: vi.fn(() => httpHandleMock),
|
|
}));
|
|
vi.mock("../src/utils/update-checker", () => ({
|
|
startUpdateChecker: vi.fn(() => updateCheckerMock),
|
|
}));
|
|
// Unlike the startup-failure test, the readiness gate RESOLVES here so control
|
|
// reaches app.listen() and the bind-error handler is what fires.
|
|
vi.mock("../src/utils/simulator-watcher", () => ({
|
|
startSimulatorWatcher: vi.fn(() => ({
|
|
stop: watcherMock.stop,
|
|
ready: Promise.resolve(),
|
|
})),
|
|
}));
|
|
|
|
// Boot a fresh start() (fresh module state + a newly registered bind-error
|
|
// handler) and fire a bind error at it once the handler is attached.
|
|
async function startAndEmitBindError(err: NodeJS.ErrnoException): Promise<void> {
|
|
const { start } = await import("../src/index");
|
|
start();
|
|
await vi.waitFor(() => {
|
|
expect(serverMock.listenerCount("error")).toBeGreaterThan(0);
|
|
});
|
|
serverMock.emit("error", err);
|
|
}
|
|
|
|
describe("tool-server bind-failure telemetry", () => {
|
|
let exitSpy: ReturnType<typeof vi.spyOn>;
|
|
|
|
beforeEach(() => {
|
|
vi.clearAllMocks();
|
|
vi.resetModules();
|
|
serverMock.reset();
|
|
exitSpy = vi.spyOn(process, "exit").mockImplementation(() => undefined as never);
|
|
});
|
|
|
|
afterEach(() => {
|
|
exitSpy.mockRestore();
|
|
});
|
|
|
|
it("captures the syscall on a startup-phase crash when nothing healthy owns the port", async () => {
|
|
// EADDRINUSE, but the probe finds no healthy argent peer (foreign holder,
|
|
// wedged server, or race already resolved) → still a genuine crash.
|
|
probeMock.probeArgentToolServer.mockResolvedValue(false);
|
|
|
|
await startAndEmitBindError(
|
|
Object.assign(new Error("listen EADDRINUSE: address already in use 127.0.0.1:3001"), {
|
|
code: "EADDRINUSE",
|
|
syscall: "listen",
|
|
})
|
|
);
|
|
|
|
await vi.waitFor(() => {
|
|
expect(telemetryMock.shutdown).toHaveBeenCalledWith(1500);
|
|
});
|
|
|
|
expect(probeMock.probeArgentToolServer).toHaveBeenCalledWith("127.0.0.1", 3001);
|
|
|
|
// The bind failure routes through crashShutdown: reason:"crash", phased as
|
|
// "startup" (the listen callback never ran), with the syscall captured as
|
|
// error_syscall and the failure signal carried through. error_name is "Error"
|
|
// and the de-identified stack yields a 16-hex fingerprint.
|
|
expect(telemetryMock.track).toHaveBeenCalledWith("toolserver:stop", {
|
|
reason: "crash",
|
|
uptime_ms: expect.any(Number),
|
|
total_tool_calls: 0,
|
|
error_code: FAILURE_CODES.ARGENT_UNCLASSIFIED_FAILURE,
|
|
failure_stage: "toolserver_bind",
|
|
failure_area: "tool_server",
|
|
error_kind: "crash",
|
|
error_name: "Error",
|
|
error_syscall: "EADDRINUSE",
|
|
crash_fingerprint: expect.stringMatching(/^[0-9a-f]{16}$/),
|
|
crash_phase: "startup",
|
|
});
|
|
expect(exitSpy).toHaveBeenCalledWith(1);
|
|
});
|
|
|
|
it("defers cleanly (exit 0, reason 'deferred', no crash fields) when a healthy argent peer owns the port", async () => {
|
|
probeMock.probeArgentToolServer.mockResolvedValue(true);
|
|
|
|
await startAndEmitBindError(
|
|
Object.assign(new Error("listen EADDRINUSE: address already in use 127.0.0.1:3001"), {
|
|
code: "EADDRINUSE",
|
|
syscall: "listen",
|
|
})
|
|
);
|
|
|
|
await vi.waitFor(() => {
|
|
expect(telemetryMock.shutdown).toHaveBeenCalledWith(1500);
|
|
});
|
|
|
|
expect(probeMock.probeArgentToolServer).toHaveBeenCalledWith("127.0.0.1", 3001);
|
|
|
|
// The redundant instance leaves the crash population entirely: a clean stop
|
|
// with its own distinct reason (not "signal", so a supervisor relaunch loop
|
|
// over deferrals stays identifiable), no failure signal, no crash
|
|
// diagnostics — asserted via exact object equality.
|
|
expect(telemetryMock.track).toHaveBeenCalledWith("toolserver:stop", {
|
|
reason: "deferred",
|
|
uptime_ms: expect.any(Number),
|
|
total_tool_calls: 0,
|
|
});
|
|
expect(exitSpy).toHaveBeenCalledWith(0);
|
|
expect(exitSpy).not.toHaveBeenCalledWith(1);
|
|
});
|
|
|
|
it("crashes without probing on a non-EADDRINUSE bind error", async () => {
|
|
await startAndEmitBindError(
|
|
Object.assign(new Error("listen EACCES: permission denied 127.0.0.1:3001"), {
|
|
code: "EACCES",
|
|
syscall: "listen",
|
|
})
|
|
);
|
|
|
|
await vi.waitFor(() => {
|
|
expect(telemetryMock.shutdown).toHaveBeenCalledWith(1500);
|
|
});
|
|
|
|
expect(probeMock.probeArgentToolServer).not.toHaveBeenCalled();
|
|
expect(telemetryMock.track).toHaveBeenCalledWith(
|
|
"toolserver:stop",
|
|
expect.objectContaining({
|
|
reason: "crash",
|
|
error_syscall: "EACCES",
|
|
})
|
|
);
|
|
expect(exitSpy).toHaveBeenCalledWith(1);
|
|
});
|
|
});
|