fix(host-service): record the descriptor table when git cannot be spawned (#6828)

* fix(host-service): record the descriptor table when git cannot be spawned

git.getStatus polls every couple of seconds, and on two machines the spawn
of the git subprocess started failing with EBADF before git ran. Once a
machine enters that state it never leaves it: 14.7k events across seven
days from two machines, the single largest error source in this service.

The captured stack is entirely inside simple-git's executor, and nothing
in the event says why the descriptor was bad. Attach the process's open
descriptor count and RLIMIT_NOFILE soft limit to spawn-syscall failures on
this path, so the next occurrence separates exhaustion (count at the
limit — we leak) from corruption (count nowhere near it).

Classification is unchanged: these keep reporting as 500s.

Refs HOST-SERVICE-4E
Refs HOST-SERVICE-1R

Claude-Session: https://claude.ai/code/session_019K18zjoDbeUSqdmxtm9GgR

* test(host-service): accept an unlimited descriptor cap in the diagnostics test

The soft limit is typed `number | string | undefined` because a container with
no cap reports it as "unlimited", but the test asserted `toBeGreaterThan(0)`,
which that value fails. Flagged by CodeRabbit.

Relaxed only that assertion, and only to what the type actually promises. The
descriptor count stays strict: both shipped platforms always let a process list
its own table, so anything but a positive number there means the counter has
stopped working, and weakening both would leave the test asserting almost
nothing.

Claude-Session: https://claude.ai/code/session_013MCFhBn5QrGso7qtPidC9N

Author: saddlepaddle

packages/host-service/src/trpc/error-diagnostics.ts

@@ -0,0 +1,36 @@
+/**
+ * A side channel for state a throw site measured at the moment of failure and
+ * the Sentry middleware should report alongside the event.
+ *
+ * The middleware decides what to report from the status code alone and never
+ * reads error text; this is how a throw site hands it evidence without
+ * changing the error, its message, or its classification. Attaching is not a
+ * capture — an error carrying diagnostics is reported exactly when it would
+ * have been anyway.
+ *
+ * Keyed by a module-private symbol: the error travels on to tRPC's error
+ * formatter and out to the client, and only enumerable string keys go with it.
+ */
+const DIAGNOSTICS = Symbol("errorDiagnostics");
+
+/** Undefined values are kept: "we looked and could not read it" is a fact
+ * worth seeing in the event, and it is not the same as never having looked. */
+export type ErrorDiagnostics = Record<string, number | string | undefined>;
+
+export function attachErrorDiagnostics(
+	error: unknown,
+	diagnostics: ErrorDiagnostics,
+): void {
+	if (!(error instanceof Error)) return;
+	(error as unknown as Record<symbol, ErrorDiagnostics>)[DIAGNOSTICS] =
+		diagnostics;
+}
+
+export function readErrorDiagnostics(
+	error: unknown,
+): ErrorDiagnostics | undefined {
+	if (!(error instanceof Error)) return undefined;
+	return (error as unknown as Record<symbol, ErrorDiagnostics | undefined>)[
+		DIAGNOSTICS
+	];
+}

packages/host-service/src/trpc/index.ts

@@ -2,6 +2,7 @@ import * as Sentry from "@sentry/node";
 import { initTRPC, TRPCError } from "@trpc/server";
 import superjson from "superjson";
 import type { HostServiceContext } from "../types";
+import { readErrorDiagnostics } from "./error-diagnostics";
 import {
 	type DeleteInProgressCause,
 	isDeleteInProgressCause,
@@ -88,6 +89,9 @@ const sentryMiddleware = t.middleware(async ({ next, path, type }) => {
 			},
 			extra: {
 				trpc_message: error.message,
+				// State a throw site measured at the moment of failure. Reporting
+				// is unchanged: this only fills in an event that is already going.
+				...readErrorDiagnostics(originalError),
 			},
 		});
 	}

packages/host-service/src/trpc/router/git/git.ts

@@ -45,6 +45,7 @@ import {
 	REVIEW_THREADS_QUERY,
 } from "./utils/graphql";
 import { resolveWorktreePath } from "./utils/resolve-worktree";
+import { attachSpawnFailureDiagnostics } from "./utils/spawn-failure-diagnostics";
 
 // Front-door cap for commit-file diffs. Statuses are admitted by
 // gitStatusRefreshLimiter; without a cap here, a burst of distinct-commit
@@ -243,6 +244,10 @@ export const gitRouter = router({
 				// worktree can vanish between resolveWorktreePath's existsSync
 				// check and the git spawn.
 				rethrowEnvironmentalGitError(error);
+				// A spawn that never produced a process reports with no
+				// first-party frame and no reason; record the descriptor table
+				// while we are still standing in the failure.
+				attachSpawnFailureDiagnostics(error);
 				throw error;
 			}
 		}),

packages/host-service/src/trpc/router/git/utils/spawn-failure-diagnostics.test.ts

@@ -0,0 +1,89 @@
+import { describe, expect, test } from "bun:test";
+import { readErrorDiagnostics } from "../../../error-diagnostics";
+import { attachSpawnFailureDiagnostics } from "./spawn-failure-diagnostics";
+
+function diagnose(message: string) {
+	const error = new Error(message);
+	attachSpawnFailureDiagnostics(error);
+	return { error, diagnostics: readErrorDiagnostics(error) };
+}
+
+describe("attachSpawnFailureDiagnostics", () => {
+	test("spawn refused outright → descriptor table recorded", () => {
+		// Verbatim from HOST-SERVICE-4E / HOST-SERVICE-1R: child_process throws
+		// synchronously for EBADF, and simple-git keeps only String(err).
+		const { diagnostics } = diagnose("Error: spawn EBADF");
+		expect(diagnostics).toBeDefined();
+		// The count stays strict: both shipped platforms always let a process
+		// list its own descriptor table, so anything but a positive number here
+		// means the counter stopped working.
+		expect(diagnostics?.open_file_descriptors).toBeGreaterThan(0);
+		// The limit is the one that genuinely varies — a container with no cap
+		// reports it as "unlimited", which is a fact worth recording, not a
+		// failure. Anything else would be.
+		const limit = diagnostics?.file_descriptor_soft_limit;
+		expect(
+			limit === "unlimited" || (typeof limit === "number" && limit > 0),
+		).toBe(true);
+	});
+
+	test("spawn refused via the child's error event → recorded too", () => {
+		// EACCES/EAGAIN/EMFILE/ENFILE/ENOENT reach the child's error event
+		// instead of throwing, and simple-git puts err.stack on stderr. First
+		// line verbatim from the same trpc_path (releases 1.22.0 and 1.23.0);
+		// frames reproduced locally against a git binary that does not exist.
+		// EMFILE arrives this way, so the exhaustion case must not be excluded
+		// by the shape the message happens to take.
+		const { diagnostics } = diagnose(
+			"Error: spawn git EAGAIN\n" +
+				"    at ChildProcess._handle.onexit (node:internal/child_process:285:19)\n" +
+				"    at onErrorNT (node:internal/child_process:483:16)\n" +
+				"    at process.processTicksAndRejections (node:internal/process/task_queues:90:21)",
+		);
+		expect(diagnostics).toBeDefined();
+		expect(diagnostics?.open_file_descriptors).toBeGreaterThan(0);
+	});
+
+	test("git ran and exited non-zero → nothing attached", () => {
+		// Real traffic from git.getStatus in the same week as the spawn groups.
+		// git started, did its work and refused: the descriptor table says
+		// nothing about a damaged object store, and attaching it here would
+		// turn the diagnostics into noise on the most common failures we see.
+		expect(diagnose("fatal: bad object HEAD\n").diagnostics).toBeUndefined();
+		expect(
+			diagnose(
+				"error: file .git/objects/pack/pack-816a419ea2792b300adb04c1f8bc739065981ebe.pack is far too short to be a packfile\n",
+			).diagnostics,
+		).toBeUndefined();
+		expect(
+			diagnose(
+				"fatal: not a git repository (or any of the parent directories): .git\n",
+			).diagnostics,
+		).toBeUndefined();
+	});
+
+	test("a spawn failure inside git's output is not our spawn failing", () => {
+		// The over-match this branch has to refuse: a clean filter that is
+		// itself a Node program crashes on its own spawn, and git relays the
+		// crash dump. Our spawn succeeded — git ran, and its stderr is
+		// quoting someone else's failure. Modelled on the filter failures in
+		// classify-git-error.test.ts, which are real traffic from this path.
+		expect(
+			diagnose(
+				"node:internal/child_process:421\n" +
+					"    throw errnoException(err, 'spawn');\n" +
+					"    ^\n\n" +
+					"Error: spawn image-optimiser ENOENT\n" +
+					"    at ChildProcess.spawn (node:internal/child_process:421:11)\n" +
+					"error: external filter 'media-filter' failed\n" +
+					"fatal: assets/img/hash.png: clean filter 'media' failed\n",
+			).diagnostics,
+		).toBeUndefined();
+	});
+
+	test("leaves the error itself alone", () => {
+		const { error } = diagnose("Error: spawn EBADF");
+		expect(error.message).toBe("Error: spawn EBADF");
+		expect(Object.keys(error)).toEqual([]);
+	});
+});

packages/host-service/src/trpc/router/git/utils/spawn-failure-diagnostics.ts

@@ -0,0 +1,94 @@
+import { readdirSync } from "node:fs";
+import { attachErrorDiagnostics } from "../../../error-diagnostics";
+
+// Node's own text for a `spawn` that never produced a process. `errnoException`
+// builds it, so the syscall name and the errno are the whole line: `spawn
+// EBADF` when child_process throws synchronously, `spawn git EAGAIN` when it
+// defers to the child's error event (EACCES/EAGAIN/EMFILE/ENFILE/ENOENT take
+// that route). simple-git surfaces the first as `String(err)` and the second as
+// `err.stack` — one line, or that line followed by ` at ` frames.
+//
+// This is matched on text rather than on `err.code` because the errno is gone
+// as structure long before this seam: simple-git's onFatalException replaces
+// the SystemError with `new GitError(task, String(e))`, keeping only the
+// sentence, and the worker boundary then keeps only name/message/stack/code.
+// The syscall name and errno in that sentence are what survive.
+const SPAWN_SYSCALL_FAILURE_PATTERN = /^(?:Error: )?spawn (?:.+ )?E[A-Z0-9]+$/;
+
+/**
+ * Whether a git failure is the spawn syscall failing rather than git running
+ * and exiting non-zero.
+ *
+ * Anchored to the start of the message, which is what separates our own spawn
+ * from one mentioned inside git's output: when the spawn fails there is no
+ * process and so no git stderr, and the error text is the whole message. A
+ * hook or filter that is itself a Node program can print the same sentence,
+ * but it arrives mid-stream, behind the program's own output and ahead of the
+ * `fatal:` line git adds.
+ */
+export function isSpawnSyscallFailure(message: string): boolean {
+	const firstLine = message.split("\n", 1)[0] ?? "";
+	return SPAWN_SYSCALL_FAILURE_PATTERN.test(firstLine);
+}
+
+/**
+ * How many descriptors this process holds. `/proc/self/fd` on Linux, `/dev/fd`
+ * on macOS; both list the caller's own table, and worker threads share it with
+ * the main thread, so this is the table the failed spawn drew from. Counts the
+ * descriptor the listing itself holds.
+ */
+function countOpenFileDescriptors(): number | undefined {
+	const dir = process.platform === "linux" ? "/proc/self/fd" : "/dev/fd";
+	try {
+		return readdirSync(dir).length;
+	} catch {
+		return undefined;
+	}
+}
+
+// RLIMIT_NOFILE is inherited at exec and nothing in this process changes it,
+// so it is read once: process.report.getReport() is the only core API that
+// exposes it and it costs ~12ms, which this path would otherwise pay on every
+// poll — the failure repeats every couple of seconds for hours.
+let softLimit: number | string | undefined;
+let softLimitRead = false;
+
+function fileDescriptorSoftLimit(): number | string | undefined {
+	if (softLimitRead) return softLimit;
+	softLimitRead = true;
+	try {
+		const report = process.report?.getReport() as
+			| { userLimits?: { open_files?: { soft?: unknown } } }
+			| undefined;
+		const soft = report?.userLimits?.open_files?.soft;
+		// `soft` is a number, or "unlimited" where the platform reports no cap.
+		if (typeof soft === "number" || typeof soft === "string") softLimit = soft;
+	} catch {
+		// Diagnostics must never replace the failure they describe.
+	}
+	return softLimit;
+}
+
+/**
+ * Record the descriptor table on a git failure that never got as far as
+ * running git.
+ *
+ * These failures report with no first-party frame — the captured stack is
+ * entirely simple-git's executor — and nothing in the event says why the
+ * spawn was refused. The count against the soft limit separates the two
+ * candidates: at the limit is exhaustion, and the leak is ours to find;
+ * nowhere near it is a descriptor that went bad while we still held it. See
+ * HOST-SERVICE-4E and HOST-SERVICE-1R, where a machine enters this state and
+ * every subsequent poll fails the same way for hours.
+ *
+ * No-op for anything else, and no-op on the error itself: the message,
+ * classification and 500 are exactly what they were.
+ */
+export function attachSpawnFailureDiagnostics(error: unknown): void {
+	if (!(error instanceof Error)) return;
+	if (!isSpawnSyscallFailure(error.message)) return;
+	attachErrorDiagnostics(error, {
+		open_file_descriptors: countOpenFileDescriptors(),
+		file_descriptor_soft_limit: fileDescriptorSoftLimit(),
+	});
+}