From 00b935abe6e2f5d131dfa4ea51ececca1a5409be Mon Sep 17 00:00:00 2001 From: Saqeb Akhter Date: Thu, 1 Oct 2026 13:58:55 -0400 Subject: [PATCH] fix(cases): bound linked-workspace path probes so an unreachable mount cannot freeze the server A linked case can live on a network mount. When that mount goes away, a hard mount makes stat() wait indefinitely, and the existsSync() probes in the case routes and the workspace hook/statusline helpers ran on the event loop, so a single GET /api/cases (or a session create in that workspace) froze the whole web server until the mount came back. Add boundedPathExists() (src/utils/bounded-path-probe.ts): an async stat that answers "absent" after 1.5 s, shares one in-flight probe per path, remembers a timed-out path until its stat finally settles, and refuses to start new probes while two stalled ones still hold libuv threadpool workers. Route the read-side probes in case-routes.ts and hooks-config.ts through it. The settings writers in hooks-config.ts use an async lstat that treats only ENOENT as missing, so an unreachable workspace is never mistaken for an empty one and has its settings recreated. --- src/hooks-config.ts | 42 +++++++++---- src/utils/bounded-path-probe.ts | 82 +++++++++++++++++++++++++ src/web/routes/case-routes.ts | 26 ++++---- test/bounded-path-probe.test.ts | 103 ++++++++++++++++++++++++++++++++ test/routes/case-routes.test.ts | 44 ++++++++++++++ 5 files changed, 274 insertions(+), 23 deletions(-) create mode 100644 src/utils/bounded-path-probe.ts create mode 100644 test/bounded-path-probe.test.ts diff --git a/src/hooks-config.ts b/src/hooks-config.ts index fda526c7..568d1c27 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -30,7 +30,6 @@ */ import { randomBytes } from 'node:crypto'; -import { existsSync } from 'node:fs'; import { readFile, writeFile, mkdir, lstat, readdir, realpath, rename, unlink, rmdir, chmod } from 'node:fs/promises'; import { homedir } from 'node:os'; import { join, dirname } from 'node:path'; @@ -40,6 +39,25 @@ import type { HookEventType } from './types.js'; import { HOOK_TIMEOUT_SECONDS } from './config/auth-config.js'; import { dataPath } from './config/instance.js'; import { readJsonConfig, SETTINGS_PATH } from './web/route-helpers.js'; +import { boundedPathExists } from './utils/bounded-path-probe.js'; + +/** + * Existence check for a WRITER. Unlike `boundedPathExists`, which answers + * "absent" for a path it could not reach in time, this tells "missing" apart + * from "unreachable": only ENOENT reads as absent, anything else throws, so a + * stalled or unreadable workspace can never be mistaken for an empty one and + * have its settings recreated over the top. It is async, so a dead mount ties + * up a threadpool worker rather than the event loop. + */ +async function pathExistsForWrite(path: string): Promise { + try { + await lstat(path); + return true; + } catch (err) { + if ((err as NodeJS.ErrnoException).code === 'ENOENT') return false; + throw err; + } +} /** * Serializes read-modify-write access to a `settings.local.json` path. Every @@ -558,7 +576,7 @@ export async function stripCaseEnvKeys(casePath: string, keysToRemove: readonly if (keysToRemove.length === 0) return; await withSafeSettingsWrite(casePath, 'env-key removal', async (_claudeDir, settingsPath) => { - if (!existsSync(settingsPath)) return; + if (!(await boundedPathExists(settingsPath))) return; let existing: Record; try { @@ -590,7 +608,7 @@ export async function stripCaseEnvKeys(casePath: string, keysToRemove: readonly */ export async function updateCaseEnvVars(casePath: string, envVars: Record): Promise { await withSafeSettingsWrite(casePath, 'env vars', async (claudeDir, settingsPath) => { - if (!existsSync(claudeDir)) { + if (!(await pathExistsForWrite(claudeDir))) { await mkdir(claudeDir, { recursive: true }); } @@ -621,7 +639,7 @@ export async function updateCaseEnvVars(casePath: string, envVars: Record { await withSafeSettingsWrite(casePath, 'model', async (claudeDir, settingsPath) => { - if (!existsSync(claudeDir)) { + if (!(await pathExistsForWrite(claudeDir))) { await mkdir(claudeDir, { recursive: true }); } @@ -650,7 +668,7 @@ export async function updateCaseModel(casePath: string, model: string | null): P */ export async function writeHooksConfig(casePath: string): Promise { await withSafeSettingsWrite(casePath, 'hooks', async (claudeDir, settingsPath) => { - if (!existsSync(claudeDir)) { + if (!(await pathExistsForWrite(claudeDir))) { await mkdir(claudeDir, { recursive: true }); } @@ -698,7 +716,7 @@ export async function writeHooksConfig(casePath: string): Promise { */ export async function ensureCodemanHooks(casePath: string): Promise { await withSafeSettingsWrite(casePath, 'hooks (ensure)', async (claudeDir, settingsPath) => { - if (!existsSync(claudeDir)) { + if (!(await pathExistsForWrite(claudeDir))) { await mkdir(claudeDir, { recursive: true }); } @@ -738,7 +756,7 @@ export async function ensureCodemanHooks(casePath: string): Promise { * when the hooks aren't ours, so it is cheap enough to call on every Claude spawn. */ export async function refreshStaleCodemanHooks(casePath: string): Promise { - if (!existsSync(join(casePath, '.claude', 'settings.local.json'))) return; + if (!(await boundedPathExists(join(casePath, '.claude', 'settings.local.json')))) return; await withSafeSettingsWrite(casePath, 'hooks (refresh)', async (_claudeDir, settingsPath) => { let existing: Record; try { @@ -820,7 +838,7 @@ export async function refreshStaleCodemanHooks(casePath: string): Promise */ export async function applyWorkspaceHooks(workspace: string, install?: boolean): Promise { try { - if (!existsSync(workspace)) return; + if (!(await boundedPathExists(workspace))) return; const shouldInstall = install ?? (await readWorkspaceHooksEnabled()); await (shouldInstall ? ensureCodemanHooks(workspace) : refreshStaleCodemanHooks(workspace)); } catch { @@ -883,7 +901,7 @@ export function generateStatusLineCommand(): string { export async function applyStatusLineConfig(casePath: string, enabled: boolean): Promise { await withSafeSettingsWrite(casePath, 'statusLine', async (claudeDir, settingsPath) => { let existing: Record = {}; - if (existsSync(settingsPath)) { + if (await pathExistsForWrite(settingsPath)) { try { existing = JSON.parse(await readFile(settingsPath, 'utf-8')); } catch { @@ -898,7 +916,7 @@ export async function applyStatusLineConfig(casePath: string, enabled: boolean): const desired = generateStatusLineCommand(); if (isOurs && current?.command === desired) return; // already current — skip rewrite if (current && !isOurs) return; // user has their OWN statusLine — never clobber it - if (!existsSync(claudeDir)) await mkdir(claudeDir, { recursive: true }); + if (!(await pathExistsForWrite(claudeDir))) await mkdir(claudeDir, { recursive: true }); existing.statusLine = { type: 'command', command: desired }; // add, or update an out-of-date ours } else { if (!isOurs) return; // nothing of ours to remove (leave a user's own statusLine alone) @@ -957,7 +975,7 @@ function statusLineExporterScriptContent(): string { } async function readStatusLineCommandFromFile(settingsPath: string): Promise { - if (!existsSync(settingsPath)) return undefined; + if (!(await boundedPathExists(settingsPath))) return undefined; try { const parsed = JSON.parse(await readFile(settingsPath, 'utf-8')); const current = parsed.statusLine as { command?: unknown } | undefined; @@ -1103,7 +1121,7 @@ export async function resolveStatusLineCliCommand( ): Promise { const settingsPath = join(casePath, '.claude', 'settings.local.json'); let userHasOwnStatusLine = false; - if (existsSync(settingsPath)) { + if (await boundedPathExists(settingsPath)) { try { const existing = JSON.parse(await readFile(settingsPath, 'utf-8')); const current = existing.statusLine as { command?: unknown } | undefined; diff --git a/src/utils/bounded-path-probe.ts b/src/utils/bounded-path-probe.ts new file mode 100644 index 00000000..28928b0c --- /dev/null +++ b/src/utils/bounded-path-probe.ts @@ -0,0 +1,82 @@ +/** + * @fileoverview Bounded existence probe for user-chosen paths. + * + * A linked case can live on a network mount (NFS, SMB, sshfs). When that mount + * goes unreachable, a hard mount makes `stat()` wait forever. A synchronous + * probe (`existsSync`) on such a path blocks the event loop and freezes the + * whole web server; even an async `stat()` never settles and permanently holds + * one of libuv's few threadpool workers, which every other `fs`, `dns.lookup` + * and `crypto` call in the process shares. + * + * `boundedPathExists()` therefore: + * - probes asynchronously and answers `false` after `PROBE_TIMEOUT_MS`, so a + * request never waits on a dead mount for longer than that; + * - shares one in-flight probe per path, and keeps answering `false` for a path + * whose probe timed out until that probe finally settles (so a dead path is + * not re-probed on every request, and is re-probed once the mount recovers); + * - stops starting new probes once `MAX_STALLED_PROBES` timed-out probes are + * still pending, so stalled stats cannot drain the threadpool. Probes that are + * merely in flight do not count, so concurrent healthy probes never get a + * false negative. + * + * Like `existsSync`, it follows symlinks and reports any error as "absent". It + * is meant for READ decisions (is it there, show it or not). A writer that must + * tell "missing" apart from "unreachable" should not treat its `false` as + * permission to create or overwrite anything. + * + * @module utils/bounded-path-probe + */ + +import fs from 'node:fs/promises'; + +/** How long a caller waits for one probe before treating the path as absent. */ +export const PROBE_TIMEOUT_MS = 1_500; +/** Timed-out probes allowed to remain pending before new probes are refused. */ +export const MAX_STALLED_PROBES = 2; + +const inFlight = new Map>(); +const stalled = new Set(); + +async function statExists(path: string): Promise { + try { + await fs.stat(path); + return true; + } catch { + return false; + } +} + +/** + * Resolve whether `path` exists without letting an unresponsive filesystem + * block the caller for longer than `PROBE_TIMEOUT_MS`. + */ +export async function boundedPathExists(path: string): Promise { + if (stalled.has(path)) return false; + + let probe = inFlight.get(path); + if (!probe) { + if (stalled.size >= MAX_STALLED_PROBES) return false; + probe = statExists(path); + inFlight.set(path, probe); + void probe.finally(() => { + inFlight.delete(path); + stalled.delete(path); + }); + } + + let timer: ReturnType | undefined; + try { + return await Promise.race([ + probe, + new Promise((resolve) => { + timer = setTimeout(() => { + if (inFlight.get(path) === probe) stalled.add(path); + resolve(false); + }, PROBE_TIMEOUT_MS); + timer.unref?.(); + }), + ]); + } finally { + if (timer) clearTimeout(timer); + } +} diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index b4802344..7aed82ac 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -50,6 +50,7 @@ import { } from '../../git-clone.js'; import type { GitRemoteProbe, GitUrlParse } from '../../git-clone.js'; import { generateClaudeMd } from '../../templates/claude-md.js'; +import { boundedPathExists } from '../../utils/bounded-path-probe.js'; import { readAgentCaseMarker, type AgentCaseMarker } from '../../agent-case-marker.js'; import { settingsWriteBlocker, writeHooksConfig } from '../../hooks-config.js'; import { @@ -162,8 +163,11 @@ function gitDiagnosticLine(stderr: string): string { * hooks, which run on the user's machine when a session starts in the case, so * the clone response says so out loud instead of silently merging into them. */ -function repoShipsClaudeSettings(casePath: string): boolean { - return ['settings.json', 'settings.local.json'].some((file) => existsSync(join(casePath, '.claude', file))); +async function repoShipsClaudeSettings(casePath: string): Promise { + for (const file of ['settings.json', 'settings.local.json']) { + if (await boundedPathExists(join(casePath, '.claude', file))) return true; + } + return false; } /** @@ -266,7 +270,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config cases.push({ name: e.name, path: casePath, - hasClaudeMd: existsSync(join(casePath, 'CLAUDE.md')), + hasClaudeMd: await boundedPathExists(join(casePath, 'CLAUDE.md')), location: 'local', ...(marker ? { agentCreated: agentCreatedInfo(marker) } : {}), }); @@ -281,11 +285,11 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config const existingNames = new Set(cases.map((c) => c.name)); if (admin) { for (const [name, path] of Object.entries(linkedCases)) { - if (!existingNames.has(name) && SAFE_CASE_NAME.test(name) && existsSync(path)) { + if (!existingNames.has(name) && SAFE_CASE_NAME.test(name) && (await boundedPathExists(path))) { cases.push({ name, path, - hasClaudeMd: existsSync(join(path, 'CLAUDE.md')), + hasClaudeMd: await boundedPathExists(join(path, 'CLAUDE.md')), linked: true, location: 'linked-local', }); @@ -333,7 +337,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config const dockerCaseInfo: CaseInfo = { name: dockerCase.name, path: dockerDisplayPath({ container, path: dockerCase.hostWorkspacePath }), - hasClaudeMd: existsSync(join(dockerCase.hostWorkspacePath, 'CLAUDE.md')), + hasClaudeMd: await boundedPathExists(join(dockerCase.hostWorkspacePath, 'CLAUDE.md')), location: 'docker', docker: { hostId: host.id, @@ -615,7 +619,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config } else { warnings.push('Kept the repository’s own CLAUDE.md.'); } - if (repoShipsClaudeSettings(casePath)) { + if (await repoShipsClaudeSettings(casePath)) { warnings.push( 'This repository ships its own .claude/settings files. Codeman merged its hooks alongside them without removing anything — review them before starting a session, since repo-supplied hooks run on this machine.' ); @@ -1618,7 +1622,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config return { name, path: dockerDisplayPath({ container, path: dockerCase.hostWorkspacePath }), - hasClaudeMd: existsSync(join(dockerCase.hostWorkspacePath, 'CLAUDE.md')), + hasClaudeMd: await boundedPathExists(join(dockerCase.hostWorkspacePath, 'CLAUDE.md')), location: 'docker', docker: { hostId: host.id, @@ -1634,7 +1638,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config const casePath = await resolveCasePath(name, getAuthUser(req)); - if (!existsSync(casePath)) { + if (!(await boundedPathExists(casePath))) { return createErrorResponse(ApiErrorCode.NOT_FOUND, 'Case not found'); } @@ -1642,7 +1646,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config return { name, path: casePath, - hasClaudeMd: existsSync(join(casePath, 'CLAUDE.md')), + hasClaudeMd: await boundedPathExists(join(casePath, 'CLAUDE.md')), ...(linked && { linked: true }), }; }); @@ -1660,7 +1664,7 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config const fixPlanPath = join(casePath, '@fix_plan.md'); - if (!existsSync(fixPlanPath)) { + if (!(await boundedPathExists(fixPlanPath))) { return { exists: false, content: null, todos: [] }; } diff --git a/test/bounded-path-probe.test.ts b/test/bounded-path-probe.test.ts new file mode 100644 index 00000000..c44e46bf --- /dev/null +++ b/test/bounded-path-probe.test.ts @@ -0,0 +1,103 @@ +/** + * @fileoverview Tests for boundedPathExists (src/utils/bounded-path-probe.ts): + * a stat() that never settles (an unreachable hard network mount) must not hold + * the caller past the timeout, must not be re-issued while it is still pending, + * and must not let stalled probes pile up in libuv's shared threadpool. + */ +import { afterEach, describe, expect, it, vi } from 'vitest'; + +vi.mock('node:fs/promises', () => ({ + default: { stat: vi.fn() }, +})); + +import fs from 'node:fs/promises'; +import { boundedPathExists, PROBE_TIMEOUT_MS } from '../src/utils/bounded-path-probe.js'; + +const stat = vi.mocked(fs.stat); + +/** + * Make the first stat() of each given path hang until released (the mount is + * down); every later stat, and every other path, answers "exists". + */ +function hangOn(paths: string[]): Map void> { + const releases = new Map void>(); + stat.mockImplementation((path) => { + if (!paths.includes(String(path)) || releases.has(String(path))) return Promise.resolve({} as never); + return new Promise((resolve) => { + releases.set(String(path), () => resolve({} as never)); + }); + }); + return releases; +} + +afterEach(() => { + vi.useRealTimers(); + stat.mockReset(); +}); + +describe('boundedPathExists', () => { + it('reports an existing path as present and a missing one as absent', async () => { + stat.mockImplementation(async (path) => { + if (String(path) === '/present') return {} as never; + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); + expect(await boundedPathExists('/present')).toBe(true); + expect(await boundedPathExists('/missing')).toBe(false); + }); + + it('answers false after the timeout when stat never settles, and does not re-probe until it does', async () => { + vi.useFakeTimers(); + const releases = hangOn(['/mnt/stalled/case']); + + const result = boundedPathExists('/mnt/stalled/case'); + await vi.advanceTimersByTimeAsync(PROBE_TIMEOUT_MS); + expect(await result).toBe(false); + + // A second caller gets the cached verdict immediately, without another stat. + expect(await boundedPathExists('/mnt/stalled/case')).toBe(false); + expect(stat).toHaveBeenCalledTimes(1); + + // Once the mount answers, the path is probed afresh. + releases.get('/mnt/stalled/case')!(); + await vi.advanceTimersByTimeAsync(0); + expect(await boundedPathExists('/mnt/stalled/case')).toBe(true); + expect(stat).toHaveBeenCalledTimes(2); + }); + + it('shares one in-flight stat between concurrent callers of the same path', async () => { + const releases = hangOn(['/slow']); + const a = boundedPathExists('/slow'); + const b = boundedPathExists('/slow'); + expect(stat).toHaveBeenCalledTimes(1); + releases.get('/slow')!(); + expect(await a).toBe(true); + expect(await b).toBe(true); + }); + + it('does not give concurrent healthy probes a false negative', async () => { + stat.mockImplementation(async () => ({}) as never); + const results = await Promise.all(['/a', '/b', '/c', '/d', '/e'].map((p) => boundedPathExists(p))); + expect(results).toEqual([true, true, true, true, true]); + }); + + it('stops issuing new stats once stalled probes would tie up the threadpool', async () => { + vi.useFakeTimers(); + const releases = hangOn(['/mnt/stalled/one', '/mnt/stalled/two']); + + const first = boundedPathExists('/mnt/stalled/one'); + const second = boundedPathExists('/mnt/stalled/two'); + await vi.advanceTimersByTimeAsync(PROBE_TIMEOUT_MS); + expect(await first).toBe(false); + expect(await second).toBe(false); + + // Both slots are held by stats that never returned: refuse a third. + expect(await boundedPathExists('/healthy/three')).toBe(false); + expect(stat).toHaveBeenCalledTimes(2); + + // Once the stalled stats settle, probing resumes normally. + releases.forEach((release) => release()); + await vi.advanceTimersByTimeAsync(0); + expect(await boundedPathExists('/healthy/three')).toBe(true); + expect(stat).toHaveBeenCalledTimes(3); + }); +}); diff --git a/test/routes/case-routes.test.ts b/test/routes/case-routes.test.ts index 56a89a2f..4315e0a8 100644 --- a/test/routes/case-routes.test.ts +++ b/test/routes/case-routes.test.ts @@ -35,6 +35,7 @@ vi.mock('node:fs', async (importOriginal) => { vi.mock('node:fs/promises', () => ({ default: { + stat: vi.fn(), readdir: vi.fn(async () => []), readFile: vi.fn(async () => { const err = new Error('ENOENT') as NodeJS.ErrnoException; @@ -74,6 +75,7 @@ const mockedReaddirSync = vi.mocked(readdirSync); const mockedReaddir = vi.mocked(fs.readdir); const mockedReadFile = vi.mocked(fs.readFile); const mockedWriteFile = vi.mocked(fs.writeFile); +const mockedStat = vi.mocked(fs.stat); const mockedCheckRemoteTmux = vi.mocked(checkRemoteTmuxAvailable); interface CaseRouteHarness { @@ -127,6 +129,12 @@ describe('case-routes', () => { // Default: existsSync returns false, readFile throws ENOENT mockedExistsSync.mockReturnValue(false); mockedReadFile.mockRejectedValue(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); + // Async stat (the bounded path probe) follows the mocked existsSync, so a + // test that sets up a path's presence via existsSync drives both the same way. + mockedStat.mockImplementation(async (path) => { + if (mockedExistsSync(path)) return { isDirectory: () => true } as never; + throw Object.assign(new Error('ENOENT'), { code: 'ENOENT' }); + }); }); afterEach(async () => { @@ -210,6 +218,42 @@ describe('case-routes', () => { // Should have both regular and linked cases expect(body.data.length).toBeGreaterThanOrEqual(1); }); + + it('still answers promptly when a linked case sits on an unreachable mount', async () => { + // A hard network mount that went away: a synchronous probe blocks the + // thread (simulated by a busy-wait), and an async stat never settles. + const stalledPath = '/mnt/unreachable/linked-nfs'; + const BLOCK_MS = 4_000; + mockedReaddir.mockResolvedValue([] as never); + mockedReadFile.mockResolvedValueOnce(JSON.stringify({ 'linked-nfs': stalledPath }) as never); + mockedExistsSync.mockImplementation((p) => { + if (String(p) !== stalledPath) return false; + const until = Date.now() + BLOCK_MS; + while (Date.now() < until) { + // spin: the event loop is frozen for as long as the mount does not answer + } + return true; + }); + let release: (() => void) | undefined; + mockedStat.mockImplementation((p) => { + if (String(p) !== stalledPath) { + return Promise.reject(Object.assign(new Error('ENOENT'), { code: 'ENOENT' })); + } + return new Promise((resolve) => { + release = () => resolve({ isDirectory: () => true } as never); + }); + }); + + const started = Date.now(); + const res = await harness.app.inject({ method: 'GET', url: '/api/cases' }); + const elapsed = Date.now() - started; + release?.(); + + expect(res.statusCode).toBe(200); + expect(elapsed).toBeLessThan(BLOCK_MS - 1_000); + // The unreachable case is left out rather than holding the list hostage. + expect(JSON.parse(res.body).data).toEqual([]); + }); }); describe('remote host and remote case routes', () => {