From 9fa44109b87adf389e23a77b26c721b40a79f963 Mon Sep 17 00:00:00 2001 From: Aamer Akhter Date: Mon, 5 Oct 2026 09:51:27 -0400 Subject: [PATCH] fix(cases): keep deleted workspaces deleted, cap pastCap, scope stalls to network mounts - applyWorkspaceHooks: an "unknown" probe that is not near a stalled path (refused by the stall cap, or an unexpected stat error) no longer reads as "go ahead". It checks existence with pathExistsForWrite first, so a deleted workspace is not recreated by the mkdir -p in ensureCodemanHooks. - pastCap gets a hard ceiling, PATH_PROBE_STALL_CEILING = UV_THREADPOOL_SIZE (default 4) minus one, so explicit requests against several dead paths can never take the last libuv worker. The bulk cap now defaults to one below the ceiling (2 with the default pool), leaving a slot for an explicit request. - A stall widens to its mount only for network and FUSE filesystem types read from /proc/self/mounts; on a local mount (a path typed under a local /home that reaches a NAS through a symlink) it narrows to the stalled path. - GET /api/cases/:name probes CLAUDE.md with pastCap, like the folder probe. - Comment in config/path-probe.ts describes the mount-scoped stall. --- src/config/path-probe.ts | 29 +++++-- src/hooks-config.ts | 19 +++-- src/utils/bounded-path-probe.ts | 64 +++++++++++----- src/web/routes/case-routes.ts | 3 +- test/bounded-path-probe.test.ts | 75 ++++++++++++++++++- test/routes/case-routes.test.ts | 2 + .../workspace-hooks-unreachable-mount.test.ts | 17 ++++- 7 files changed, 171 insertions(+), 38 deletions(-) diff --git a/src/config/path-probe.ts b/src/config/path-probe.ts index 3c11a82d..e69e9642 100644 --- a/src/config/path-probe.ts +++ b/src/config/path-probe.ts @@ -10,8 +10,8 @@ * * Both are env-overridable, in the same style as the other config modules. A slow * but healthy mount (an sshfs that needs a couple of seconds on first touch) may want - * a longer timeout; a server started with a larger `UV_THREADPOOL_SIZE` can afford a - * higher stall cap. + * a longer timeout. The stall limits follow `UV_THREADPOOL_SIZE` on their own, so a + * server started with a larger pool gets a higher ceiling without further setup. * * @module config/path-probe */ @@ -26,10 +26,23 @@ function envInt(name: string, fallback: number, min: number, max: number): numbe export const PATH_PROBE_TIMEOUT_MS = envInt('CODEMAN_PATH_PROBE_TIMEOUT_MS', 1_500, 100, 60_000); /** - * Timed-out probes allowed to stay pending before new probes are refused (answered - * "unknown" without a stat). This is a backstop, not the main defence: a stalled - * path already takes its neighbours (same parent directory) out of probing, so the - * cap only engages once three UNRELATED places have stopped answering. The default - * leaves one of libuv's default four workers free for the rest of the process. + * Hard ceiling on timed-out probes left pending, for every caller, `pastCap` ones + * included: the threadpool size minus one, so a dead mount can never take the last + * worker. libuv sizes the pool from `UV_THREADPOOL_SIZE` (4 when unset). A pool of + * one cannot keep a worker free at all, so the ceiling never drops below one. */ -export const MAX_STALLED_PATH_PROBES = envInt('CODEMAN_PATH_PROBE_MAX_STALLED', 3, 1, 64); +export const PATH_PROBE_STALL_CEILING = Math.max(1, (Number(process.env.UV_THREADPOOL_SIZE) || 4) - 1); + +/** + * Timed-out probes allowed to stay pending before new BULK probes are refused + * (answered "unknown" without a stat). This is a backstop, not the main defence: a + * stalled path on a network or FUSE mount already takes the rest of that mount out + * of probing (a stall anywhere else takes out only the stalled path), so the cap + * only engages once that many UNRELATED places have stopped answering. It defaults + * to one below {@link PATH_PROBE_STALL_CEILING} (2 with the default pool), leaving a + * slot a `pastCap` probe may still use, and is never allowed above the ceiling. + */ +export const MAX_STALLED_PATH_PROBES = Math.min( + PATH_PROBE_STALL_CEILING, + envInt('CODEMAN_PATH_PROBE_MAX_STALLED', Math.max(1, PATH_PROBE_STALL_CEILING - 1), 1, 64) +); diff --git a/src/hooks-config.ts b/src/hooks-config.ts index 33f9c618..5f9eabac 100644 --- a/src/hooks-config.ts +++ b/src/hooks-config.ts @@ -852,13 +852,20 @@ export async function refreshStaleCodemanHooks(casePath: string): Promise */ export async function applyWorkspaceHooks(workspace: string, install?: boolean): Promise { try { - const skip = await absentOrUnreachable(workspace); - if (skip === 'unreachable') { - console.warn( - `[hooks] ${workspace} is not responding (unreachable mount?); Codeman hooks not checked or installed` - ); + const state = await probePath(workspace); + if (state === 'absent') return; + if (state === 'unknown') { + if (isNearStalledPath(workspace)) { + console.warn( + `[hooks] ${workspace} is not responding (unreachable mount?); Codeman hooks not checked or installed` + ); + return; + } + // Any other "unknown" (the stall cap refused the probe, or the stat failed + // with something other than ENOENT) proves nothing about existence, and the + // install below would mkdir -p a deleted repo back into being: ask directly. + if (!(await pathExistsForWrite(workspace))) return; } - if (skip) return; const shouldInstall = install ?? (await readWorkspaceHooksEnabled()); await (shouldInstall ? ensureCodemanHooks(workspace) : refreshStaleCodemanHooks(workspace)); } catch { diff --git a/src/utils/bounded-path-probe.ts b/src/utils/bounded-path-probe.ts index 5b2e7e58..34f32adc 100644 --- a/src/utils/bounded-path-probe.ts +++ b/src/utils/bounded-path-probe.ts @@ -21,11 +21,14 @@ * - a path whose probe timed out is "stalled" until that stat finally settles. * Paths NEAR a stalled one are answered "unknown" without a new stat, so one * dead mount costs one worker, not one per case and file on it. "Near" means on - * the same mount: under the deepest mount point holding the stalled path, read - * from `/proc/self/mounts` (procfs, which never waits on the dead filesystem). - * Where that table is unavailable (not Linux), or the deepest mount is `/`, it - * narrows to the stalled path and everything under it. Unrelated paths are - * probed normally; + * the same mount when that mount is a network or FUSE filesystem (NFS, SMB, + * sshfs and the like): under the deepest mount point holding the stalled path, + * with its type, read from `/proc/self/mounts` (procfs, which never waits on the + * dead filesystem). Otherwise it narrows to the stalled path and everything under + * it: when the deepest mount is local (a path typed under a local `/home` can + * reach a NAS through a symlink, and must not take the rest of `/home` with it), + * is `/`, or the table is unavailable (not Linux). Unrelated paths are probed + * normally; * - once `MAX_STALLED_PATH_PROBES` stalled stats are pending, new probes are * refused process-wide (answered "unknown"), since each would risk another * worker. Probes merely in flight do not count, so concurrent healthy probes @@ -34,7 +37,9 @@ * probe is still bounded and still recorded as stalled if it hangs (so a dead * path costs at most one worker however often it is retried), but it is not * refused just because unrelated mounts are dead. Bulk scans (the case list) - * and per-spawn helpers keep the cap. + * and per-spawn helpers keep the cap. `pastCap` still stops at + * `PATH_PROBE_STALL_CEILING` (the threadpool size minus one), so explicit + * requests against several dead paths can never take the last worker. * * Both events are logged once (`console.warn`): a path's first stall, and the * cap engaging, so "my case vanished" and "hooks stopped firing" leave a trace. @@ -49,7 +54,7 @@ import { readFileSync } from 'node:fs'; import fs from 'node:fs/promises'; import { resolve, sep } from 'node:path'; -import { MAX_STALLED_PATH_PROBES, PATH_PROBE_TIMEOUT_MS } from '../config/path-probe.js'; +import { MAX_STALLED_PATH_PROBES, PATH_PROBE_STALL_CEILING, PATH_PROBE_TIMEOUT_MS } from '../config/path-probe.js'; /** What a probe could establish about a path. */ export type PathProbeState = 'present' | 'absent' | 'unknown'; @@ -75,29 +80,53 @@ function isWithin(path: string, root: string): boolean { return path.startsWith(root.endsWith(sep) ? root : root + sep); } -/** Deepest mount point holding `abs`, from the kernel's mount table; undefined when unreadable. */ -function mountPointOf(abs: string): string | undefined { +/** Filesystem types whose stall means the whole mount is gone (network and FUSE). */ +const REMOTE_FS_TYPES = new Set([ + 'nfs', + 'nfs4', + 'cifs', + 'smb3', + 'smbfs', + '9p', + 'ceph', + 'glusterfs', + 'afs', + 'lustre', + 'davfs', +]); + +function isRemoteFsType(fsType: string): boolean { + return REMOTE_FS_TYPES.has(fsType) || fsType.startsWith('fuse.'); +} + +/** Deepest mount holding `abs`, from the kernel's mount table; undefined when unreadable. */ +function mountOf(abs: string): { mountPoint: string; fsType: string } | undefined { let table: string; try { table = readFileSync('/proc/self/mounts', 'utf-8'); } catch { return undefined; } - let best: string | undefined; + let best: { mountPoint: string; fsType: string } | undefined; for (const line of table.split('\n')) { - const field = line.split(' ')[1]; - if (!field) continue; + const [, field, fsType] = line.split(' '); + if (!field || !fsType) continue; // The table octal-escapes space, tab, newline and backslash in mount points. const mountPoint = field.replace(/\\([0-7]{3})/g, (_m, oct: string) => String.fromCharCode(parseInt(oct, 8))); - if (isWithin(abs, mountPoint) && (!best || mountPoint.length > best.length)) best = mountPoint; + if (isWithin(abs, mountPoint) && (!best || mountPoint.length > best.mountPoint.length)) { + best = { mountPoint, fsType }; + } } return best; } -/** The subtree a stalled path takes down with it: its mount, else just itself (see the module comment). */ +/** + * The subtree a stalled path takes down with it (see the module comment): its + * mount when that is a network or FUSE filesystem, else just the path itself. + */ function stallScope(abs: string): string { - const mountPoint = mountPointOf(abs); - return mountPoint && mountPoint !== '/' ? mountPoint : abs; + const mount = mountOf(abs); + return mount && mount.mountPoint !== '/' && isRemoteFsType(mount.fsType) ? mount.mountPoint : abs; } /** @@ -130,7 +159,8 @@ export async function probePathKind(path: string, options: PathProbeOptions = {} let probe = inFlight.get(abs); if (!probe) { - if (stalled.size >= MAX_STALLED_PATH_PROBES && !options.pastCap) { + // pastCap lifts the bulk cap, never the ceiling that keeps one worker free. + if (stalled.size >= (options.pastCap ? PATH_PROBE_STALL_CEILING : MAX_STALLED_PATH_PROBES)) { if (!capWarned) { capWarned = true; console.warn( diff --git a/src/web/routes/case-routes.ts b/src/web/routes/case-routes.ts index 8b8483f1..3edabb0a 100644 --- a/src/web/routes/case-routes.ts +++ b/src/web/routes/case-routes.ts @@ -1665,7 +1665,8 @@ export function registerCaseRoutes(app: FastifyInstance, ctx: EventPort & Config return { name, path: casePath, - hasClaudeMd: await boundedPathExists(join(casePath, 'CLAUDE.md')), + // Probed like the folder above, or a healthy case reads as having no CLAUDE.md under the cap. + hasClaudeMd: (await probePath(join(casePath, 'CLAUDE.md'), { pastCap: true })) === 'present', ...(linked && { linked: true }), }; }); diff --git a/test/bounded-path-probe.test.ts b/test/bounded-path-probe.test.ts index 64b93f61..ff85bf12 100644 --- a/test/bounded-path-probe.test.ts +++ b/test/bounded-path-probe.test.ts @@ -38,7 +38,7 @@ vi.mock('node:fs', async (importOriginal) => { import fs from 'node:fs/promises'; import { boundedPathExists, isNearStalledPath, probePath, probePathKind } from '../src/utils/bounded-path-probe.js'; -import { MAX_STALLED_PATH_PROBES, PATH_PROBE_TIMEOUT_MS } from '../src/config/path-probe.js'; +import { MAX_STALLED_PATH_PROBES, PATH_PROBE_STALL_CEILING, PATH_PROBE_TIMEOUT_MS } from '../src/config/path-probe.js'; const stat = vi.mocked(fs.stat); const dirStats = { isDirectory: () => true } as never; @@ -143,10 +143,11 @@ describe('probePath', () => { expect(results).toEqual([true, true, true, true, true]); }); - it('still probes a healthy path as present while two unrelated paths are stalled', async () => { + it('still probes a healthy path as present while fewer unrelated paths are stalled than the cap', async () => { vi.useFakeTimers(); - hangOn(['/mnt/nas-a/project', '/mnt/nas-b/project']); - await stall(['/mnt/nas-a/project', '/mnt/nas-b/project']); + const dead = Array.from({ length: MAX_STALLED_PATH_PROBES - 1 }, (_, i) => `/mnt/nas-${i}/project`); + hangOn(dead); + await stall(dead); expect(await probePath('/home/user/codeman-cases/healthy')).toBe('present'); expect(await boundedPathExists('/home/user/codeman-cases/healthy/CLAUDE.md')).toBe(true); @@ -189,6 +190,41 @@ describe('probePath', () => { expect(await probePath('/home/user/codeman-cases/one')).toBe('present'); }); + it('narrows a stall on a local mount to the stalled path, even when that mount is not /', async () => { + // /home is its own local filesystem; ~/nas is a symlink to a network mount, so + // the stalled path is typed under /home. Only network and FUSE mounts widen. + vi.useFakeTimers(); + mounts.table = [mounts.default, '/dev/sdb1 /home ext4 rw,relatime 0 0', ''].join('\n'); + hangOn(['/home/user/nas/project']); + await stall(['/home/user/nas/project']); + stat.mockClear(); + + expect(await probePath('/home/user/nas/project/CLAUDE.md')).toBe('unknown'); + expect(isNearStalledPath('/home/user/codeman-cases/one')).toBe(false); + expect(await probePath('/home/user/codeman-cases/one')).toBe('present'); + expect(await probePath('/home/user/nas/other')).toBe('present'); + expect(stat).toHaveBeenCalledTimes(2); + }); + + it('widens a stall to the whole mount for network and FUSE filesystems', async () => { + vi.useFakeTimers(); + mounts.table = [ + mounts.default, + 'nas:/four /srv/nas4 nfs4 rw,hard 0 0', + 'user@host:/ /srv/sshfs fuse.sshfs rw 0 0', + '//nas/share /srv/smb cifs rw 0 0', + '', + ].join('\n'); + const dead = ['/srv/nas4/one', '/srv/sshfs/one']; + hangOn(dead); + await stall(dead); + + expect(isNearStalledPath('/srv/nas4/two')).toBe(true); + expect(isNearStalledPath('/srv/sshfs/two')).toBe(true); + expect(isNearStalledPath('/srv/smb/two')).toBe(false); + expect(isNearStalledPath('/srv/elsewhere')).toBe(false); + }); + it('narrows a stall to the stalled path when there is no mount table', async () => { vi.useFakeTimers(); mounts.table = null; @@ -235,6 +271,37 @@ describe('probePath', () => { expect(stat).toHaveBeenCalledTimes(2); }); + it('stops pastCap probes at the threadpool ceiling, answering unknown without a stat', async () => { + vi.useFakeTimers(); + // Fill the bulk cap, then let pastCap probes stall until the ceiling is reached. + const dead = Array.from({ length: PATH_PROBE_STALL_CEILING + 1 }, (_, i) => `/mnt/ceiling-${i}/case`); + hangOn(dead); + await stall(dead.slice(0, MAX_STALLED_PATH_PROBES)); + for (const path of dead.slice(MAX_STALLED_PATH_PROBES, PATH_PROBE_STALL_CEILING)) { + const hung = probePath(path, { pastCap: true }); + await vi.advanceTimersByTimeAsync(PATH_PROBE_TIMEOUT_MS); + expect(await hung).toBe('unknown'); + } + stat.mockClear(); + + // One worker must stay free: no new stat, even for an explicit request. + const refused = probePath(dead[PATH_PROBE_STALL_CEILING], { pastCap: true }); + await vi.advanceTimersByTimeAsync(PATH_PROBE_TIMEOUT_MS); + expect(await refused).toBe('unknown'); + expect(stat).not.toHaveBeenCalled(); + expect(await probePath('/healthy/explicit', { pastCap: true })).toBe('unknown'); + expect(stat).not.toHaveBeenCalled(); + + // Once one stalled stat settles, an explicit request is probed again. + releases.get(dead[0])!(); + await vi.advanceTimersByTimeAsync(0); + expect(await probePath('/healthy/explicit', { pastCap: true })).toBe('present'); + }); + + it('keeps the bulk cap below the ceiling, so a pastCap probe has room', () => { + expect(MAX_STALLED_PATH_PROBES).toBeLessThan(PATH_PROBE_STALL_CEILING); + }); + it('warns once when a path first stalls and once when the cap engages', async () => { vi.useFakeTimers(); const dead = Array.from({ length: MAX_STALLED_PATH_PROBES }, (_, i) => `/mnt/gone-${i}/case`); diff --git a/test/routes/case-routes.test.ts b/test/routes/case-routes.test.ts index 5a78272e..c0fdcce8 100644 --- a/test/routes/case-routes.test.ts +++ b/test/routes/case-routes.test.ts @@ -779,6 +779,8 @@ describe('case-routes', () => { expect(res.statusCode).toBe(200); expect(JSON.parse(res.body).data).toMatchObject({ name: 'healthy-local' }); expect(JSON.parse(res.body).data.unreachable).toBeUndefined(); + // Its CLAUDE.md is probed the same way as its folder, so it is not misreported missing. + expect(JSON.parse(res.body).data.hasClaudeMd).toBe(true); }); it('answers a local case it cannot read with a non-NOT_FOUND error', async () => { diff --git a/test/workspace-hooks-unreachable-mount.test.ts b/test/workspace-hooks-unreachable-mount.test.ts index a9da2b54..909f4625 100644 --- a/test/workspace-hooks-unreachable-mount.test.ts +++ b/test/workspace-hooks-unreachable-mount.test.ts @@ -79,8 +79,21 @@ describe('workspace helpers while other mounts are unreachable', () => { expect(readFileSync(settings, 'utf-8')).toContain('/api/hook-event'); }); - it('installs hooks in a healthy workspace while two unrelated paths are stalled', async () => { - await stallUnrelatedMounts(2); + it('keeps a deleted workspace deleted while the stall cap is engaged', async () => { + // The cap refuses the probe ("unknown" without a stat), which must not read as + // "go ahead": installing would mkdir -p the deleted repo back into existence. + await stallUnrelatedMounts(MAX_STALLED_PATH_PROBES); + const workspace = join(root, 'deleted-repo'); + expect(await probePath(workspace)).toBe('unknown'); + + await applyWorkspaceHooks(workspace, true); + await applyWorkspaceHooks(workspace, false); + + expect(existsSync(workspace)).toBe(false); + }); + + it('installs hooks in a healthy workspace while fewer unrelated paths are stalled than the cap', async () => { + await stallUnrelatedMounts(MAX_STALLED_PATH_PROBES - 1); const workspace = join(root, 'healthy-b'); mkdirSync(workspace);