2 Commits

Author	SHA1	Message	Date
filip131311	7f85f996aa	fix(tool-server): defer to an existing server on EADDRINUSE instead of crash-looping (#533) ... ## 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.	2026-07-22 16:56:54 +02:00
filip131311	2196cc18f0	feat(telemetry): anonymous crash diagnostics on toolserver:stop (#494) ... ## Why PostHog shows **~9,785 `toolserver:stop` crashes over 90 days**, but every one collapses into a single bucket — `error_kind=crash`, `error_code=TOOLSERVER_UNCAUGHT_EXCEPTION`. We know *that* the tool server died on a top-level throw, never *which* throw. The population is also two very different things we can't tell apart today: - **~99%** crash at **<5s uptime with 0 tool calls** — startup faults, and heavily concentrated per-user (top 10 users ≈ 31% of all crashes; one user logged 1,027), i.e. **boot → crash → relaunch loops**. - **~1,000** crash after **>10 min** of real serving — genuine runtime bugs. Without the actual exception, all 9,785 rows are unrankable and unfixable. ## What Four **crash-only, provably-anonymous** fields on `toolserver:stop`, enforced by the existing coded-allowlist sanitizer so **no free text can ever reach PostHog**: | field | value | why | |---|---|---| | `error_name` | error class name (`TypeError`, …) | "what kind" | | `error_syscall` | Node's `err.code` (`EADDRINUSE`, `ECONNREFUSED`, …) | the key discriminator for the startup-loop population (port/socket conflicts) | | `crash_fingerprint` | 16-hex hash of the **de-identified** top stack frames | clusters the same crash across users; input carries no identity, output is opaque | | `crash_phase` | `"startup"` \| `"serving"` | splits the two populations directly | Turns 9,785 opaque rows into a handful of rankable clusters. ## Anonymity (the hard requirement) - **No error message, no raw stack.** Those routinely embed `/Users/<name>/…`, URLs, argv, hostnames, interpolated values — the highest-PII surface and impossible to reliably scrub. The fingerprint replaces the message for clustering; `error_name` + `error_syscall` give the "what kind" without free text. - **Fingerprint input is de-identified before hashing** (defense-in-depth on top of the hash): first-party frames → basename only (drops the home-dir/project prefix), `node_modules` frames → package-relative tail (drops the user prefix; also collapses the pnpm `.pnpm/pkg@ver/` layout), `node:` internals kept verbatim, column dropped. So the *same crash on two machines produces the same fingerprint*, and no username/path survives even into the hash input. - **Sanitizer is the final gate.** `error_name` `^[A-Za-z][A-Za-z0-9_]{0,63}$`, `error_syscall` `^E[A-Z0-9_]{1,31}$`, `crash_fingerprint` `^[0-9a-f]{16}$`, `crash_phase` a 2-value enum. Any path/message/lowercase/oversize value is dropped before it can leave the machine. ## Scope & safety (no leaking crashes out of scope) - Fields are populated **only** in `crashShutdown` (which forces `reason="crash"`), so clean idle/signal stops carry none. Verified by the existing startup-telemetry test (the pre-listener watcher-rejection crash path stays the bare `{reason,uptime_ms,total_tool_calls}` shape). - The four keys exist only on `ToolserverStopProps` and only in the `toolserver:stop` allowlist entry — they can't ride any other event. - `describeCrash` is **total**: every field extraction is isolated, so a hostile throwing getter on `.name`/`.code`/`.stack` yields the phase (and whatever else is readable) instead of turning crash reporting into a second crash. The crash/exit guarantee — re-entrancy guard, force-exit timer, `finalExitCode` escalation — is untouched. ## Tests - New `crash-diagnostics.test.ts`: class-name/constructor-name/primitive extraction, syscall capture, cross-user determinism (incl. **Windows** drive-letter paths and **pnpm** layout), node_modules tail, line-sensitivity, no-stack omission, hostile-getter resilience, and "never emits a message/stack" confinement. - `sanitize.test.ts`: crash fields pass when well-formed; dropped for message-as-name, path/lowercase syscall, wrong-length/uppercase fingerprint, unknown phase; `ERR_*` accepted. - Full telemetry suite (278) + tool-server startup test green; lint/prettier clean. ## Reviewed Ran an adversarial review pass (anonymity leak-hunt, determinism, crash-path safety, scope). Ship-ready; all findings were Low/Nit and the actionable ones (hostile-getter hardening, Windows/pnpm test coverage, `error_syscall` doc clarity) are folded in. ### Deliberately deferred (documented, not blockers) - Sanitizer doesn't *cross-gate* the crash fields on `reason==="crash"` — the per-field validator design has no sibling access; coupling is enforced in emit code. The fields are non-identifying regardless, so a hypothetical stray `crash_phase` on an idle stop is a data nit, not a leak. - First-party frames reduce to basename; if `dist` ever ships unbundled with duplicate basenames, two sites at the same line could collide. Non-issue for the bundled tool-server.	2026-07-13 22:16:33 +02:00