mirror of
https://github.com/software-mansion/argent.git
synced 2026-09-14 19:27:14 +08:00
filip131311
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.
64 lines
2.6 KiB
TypeScript
64 lines
2.6 KiB
TypeScript
import { afterEach, describe, expect, it } from "vitest";
|
|
import * as http from "node:http";
|
|
import { probeArgentToolServer } from "../src/utils/probe-argent-tool-server";
|
|
|
|
// Bind an ephemeral loopback server with a given handler; returns its port.
|
|
function listen(handler: http.RequestListener): Promise<{ port: number; server: http.Server }> {
|
|
return new Promise((resolve) => {
|
|
const server = http.createServer(handler);
|
|
server.listen(0, "127.0.0.1", () => {
|
|
const addr = server.address();
|
|
resolve({ port: typeof addr === "object" && addr ? addr.port : 0, server });
|
|
});
|
|
});
|
|
}
|
|
|
|
const servers: http.Server[] = [];
|
|
afterEach(async () => {
|
|
await Promise.all(servers.splice(0).map((s) => new Promise<void>((r) => s.close(() => r()))));
|
|
});
|
|
|
|
describe("probeArgentToolServer", () => {
|
|
it("returns true for an argent peer answering /tools with 200", async () => {
|
|
const { port, server } = await listen((_req, res) => {
|
|
res.writeHead(200, { "content-type": "application/json" });
|
|
res.end(JSON.stringify({ tools: [] }));
|
|
});
|
|
servers.push(server);
|
|
await expect(probeArgentToolServer("127.0.0.1", port)).resolves.toBe(true);
|
|
});
|
|
|
|
it("returns true for a token-protected peer answering /tools with 401", async () => {
|
|
const { port, server } = await listen((_req, res) => {
|
|
res.writeHead(401, { "content-type": "application/json" });
|
|
res.end(JSON.stringify({ error: "Missing or invalid Authorization header." }));
|
|
});
|
|
servers.push(server);
|
|
await expect(probeArgentToolServer("127.0.0.1", port)).resolves.toBe(true);
|
|
});
|
|
|
|
it("returns false for a foreign server answering with a non-argent status (500)", async () => {
|
|
const { port, server } = await listen((_req, res) => {
|
|
res.writeHead(500);
|
|
res.end("nope");
|
|
});
|
|
servers.push(server);
|
|
await expect(probeArgentToolServer("127.0.0.1", port)).resolves.toBe(false);
|
|
});
|
|
|
|
it("returns false when nothing is listening (connection refused)", async () => {
|
|
// Grab a port then immediately release it so the connection is refused.
|
|
const { port, server } = await listen((_req, res) => res.end());
|
|
await new Promise<void>((r) => server.close(() => r()));
|
|
await expect(probeArgentToolServer("127.0.0.1", port)).resolves.toBe(false);
|
|
});
|
|
|
|
it("returns false when the peer accepts but never responds (wedged) before the timeout", async () => {
|
|
const { port, server } = await listen(() => {
|
|
/* accept the request but never write a response → probe must time out */
|
|
});
|
|
servers.push(server);
|
|
await expect(probeArgentToolServer("127.0.0.1", port, 150)).resolves.toBe(false);
|
|
});
|
|
});
|