diff --git a/gitnexus-claude-plugin/hooks/gitnexus-hook.js b/gitnexus-claude-plugin/hooks/gitnexus-hook.js index 539ec931e..1c618d8aa 100644 --- a/gitnexus-claude-plugin/hooks/gitnexus-hook.js +++ b/gitnexus-claude-plugin/hooks/gitnexus-hook.js @@ -161,7 +161,7 @@ function extractPattern(toolName, toolInput) { } /** - * Concurrency guard for the augment subprocess. + * Concurrency guard for the augment subprocess (hard cap). * * Claude Code fires PreToolUse hooks per parallel tool call. With no cap, N * parallel Grep/Glob/Bash tool calls spawn N concurrent `gitnexus augment` @@ -171,8 +171,18 @@ function extractPattern(toolName, toolInput) { * silently above that. Augment is a best-effort enrichment; missing a few * fires under heavy parallel load is preferable to melting the box. * - * Implementation: lockfiles named `.lock` under `<.gitnexus>/.hook-locks/`. - * Stale entries are pruned by age (>30s mtime) or by pid-liveness check. + * Implementation: fixed-name slot files `slot-0.lock` ... `slot-N.lock` + * under `<.gitnexus>/.hook-locks/`. Each file is created with `wx` + * (O_CREAT|O_EXCL), which is atomic across processes at the OS level — + * exactly one process wins each slot. This is a HARD cap, not the + * count-then-claim soft cap that an earlier revision shipped (it had a + * TOCTOU window between readdirSync and the per-pid wx write). + * + * Owner identity is written into the slot file as the PID, used for + * stale-takeover when a hook crashes without releasing. PID-liveness is + * checked before age so a slow-but-alive hook is never wrongly evicted; + * the age window only kicks in to defend against PID reuse on a long- + * abandoned slot. */ const HOOK_LOCK_SUBDIR = '.hook-locks'; const HOOK_LOCK_MAX_INFLIGHT = 3; @@ -180,66 +190,83 @@ const HOOK_LOCK_STALE_MS = 30000; function acquireHookSlot(gitNexusDir) { const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR); - let entries; try { fs.mkdirSync(lockDir, { recursive: true }); - entries = fs.readdirSync(lockDir); } catch { + // Cannot create lock dir — fall through unguarded so the hook still works. return () => {}; } - const now = Date.now(); - let active = 0; - for (const entry of entries) { - if (!entry.endsWith('.lock')) continue; - const lockPath = path.join(lockDir, entry); - const pid = Number.parseInt(entry, 10); - let stale = false; - try { - const stat = fs.statSync(lockPath); - if (now - stat.mtimeMs > HOOK_LOCK_STALE_MS) stale = true; - } catch { - stale = true; - } - if (!stale && Number.isFinite(pid) && pid > 0) { + const myPidStr = String(process.pid); + + for (let slot = 0; slot < HOOK_LOCK_MAX_INFLIGHT; slot++) { + const slotPath = path.join(lockDir, `slot-${slot}.lock`); + for (let attempt = 0; attempt < 2; attempt++) { try { - process.kill(pid, 0); - active++; - continue; + fs.writeFileSync(slotPath, myPidStr, { flag: 'wx' }); + let released = false; + const release = () => { + if (released) return; + released = true; + try { + // Only unlink if we still own the slot. If we appeared stale and + // another hook took over, the file now belongs to it — leave alone. + const content = fs.readFileSync(slotPath, 'utf-8').trim(); + if (content === myPidStr) fs.unlinkSync(slotPath); + } catch { + /* already removed or unreadable */ + } + }; + process.on('exit', release); + return release; } catch { - stale = true; - } - } - if (stale) { - try { - fs.unlinkSync(lockPath); - } catch { - /* another hook beat us to it */ + // Slot exists. Decide whether to take it over. + let stat; + try { + stat = fs.statSync(slotPath); + } catch { + continue; // Vanished between EEXIST and stat — retry this slot. + } + let isLive = false; + try { + const ownerStr = fs.readFileSync(slotPath, 'utf-8').trim(); + if (ownerStr === '') { + // Owner created the file but hasn't written its PID yet. The + // wx open+write window is microseconds; give it the benefit + // of the doubt and treat as live. + isLive = true; + } else { + const owner = Number.parseInt(ownerStr, 10); + if (Number.isFinite(owner) && owner > 0) { + try { + process.kill(owner, 0); + isLive = true; + } catch { + /* ESRCH (or EPERM under cross-user) — treat as dead */ + } + } + } + } catch { + /* unreadable — treat as dead */ + } + // PID-liveness wins over age (avoids evicting a slow-but-alive hook). + // Age check is a safety net against PID reuse on long-abandoned slots: + // 30s >> the 7s augment timeout, so a healthy run never hits it. + if (isLive && Date.now() - stat.mtimeMs > HOOK_LOCK_STALE_MS) { + isLive = false; + } + if (isLive) break; // Try the next slot. + try { + fs.unlinkSync(slotPath); + } catch { + /* another hook beat us to it — retry will hit EEXIST */ + } + // Loop and retry this slot. } } } - if (active >= HOOK_LOCK_MAX_INFLIGHT) return null; - - const ownLock = path.join(lockDir, `${process.pid}.lock`); - try { - fs.writeFileSync(ownLock, '', { flag: 'wx' }); - } catch { - return () => {}; - } - - let released = false; - const release = () => { - if (released) return; - released = true; - try { - fs.unlinkSync(ownLock); - } catch { - /* pruned by another hook */ - } - }; - process.on('exit', release); - return release; + return null; } /** diff --git a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs index e6d64d29f..e7f758116 100644 --- a/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs +++ b/gitnexus-cursor-integration/hooks/gitnexus-hook.cjs @@ -193,7 +193,7 @@ function resolveCliPath() { } /** - * Concurrency guard for the augment subprocess. + * Concurrency guard for the augment subprocess (hard cap). * * Editors fire postToolUse hooks per parallel tool call. With no cap, N * parallel Grep/Read/Shell tool calls spawn N concurrent `gitnexus augment` @@ -201,8 +201,18 @@ function resolveCliPath() { * for several seconds. Issue #1486 reported 180+ piled-up processes and * load average > 100 on the Claude variant; the same shape applies here. * - * Implementation: lockfiles named `.lock` under `<.gitnexus>/.hook-locks/`. - * Stale entries are pruned by age (>30s mtime) or by pid-liveness check. + * Implementation: fixed-name slot files `slot-0.lock` ... `slot-N.lock` + * under `<.gitnexus>/.hook-locks/`. Each file is created with `wx` + * (O_CREAT|O_EXCL), which is atomic across processes at the OS level — + * exactly one process wins each slot. This is a HARD cap, not the + * count-then-claim soft cap that an earlier revision shipped (it had a + * TOCTOU window between readdirSync and the per-pid wx write). + * + * Owner identity is written into the slot file as the PID, used for + * stale-takeover when a hook crashes without releasing. PID-liveness is + * checked before age so a slow-but-alive hook is never wrongly evicted; + * the age window only kicks in to defend against PID reuse on a long- + * abandoned slot. */ const HOOK_LOCK_SUBDIR = '.hook-locks'; const HOOK_LOCK_MAX_INFLIGHT = 3; @@ -210,66 +220,83 @@ const HOOK_LOCK_STALE_MS = 30000; function acquireHookSlot(gitNexusDir) { const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR); - let entries; try { fs.mkdirSync(lockDir, { recursive: true }); - entries = fs.readdirSync(lockDir); } catch { + // Cannot create lock dir — fall through unguarded so the hook still works. return () => {}; } - const now = Date.now(); - let active = 0; - for (const entry of entries) { - if (!entry.endsWith('.lock')) continue; - const lockPath = path.join(lockDir, entry); - const pid = Number.parseInt(entry, 10); - let stale = false; - try { - const stat = fs.statSync(lockPath); - if (now - stat.mtimeMs > HOOK_LOCK_STALE_MS) stale = true; - } catch { - stale = true; - } - if (!stale && Number.isFinite(pid) && pid > 0) { + const myPidStr = String(process.pid); + + for (let slot = 0; slot < HOOK_LOCK_MAX_INFLIGHT; slot++) { + const slotPath = path.join(lockDir, `slot-${slot}.lock`); + for (let attempt = 0; attempt < 2; attempt++) { try { - process.kill(pid, 0); - active++; - continue; + fs.writeFileSync(slotPath, myPidStr, { flag: 'wx' }); + let released = false; + const release = () => { + if (released) return; + released = true; + try { + // Only unlink if we still own the slot. If we appeared stale and + // another hook took over, the file now belongs to it — leave alone. + const content = fs.readFileSync(slotPath, 'utf-8').trim(); + if (content === myPidStr) fs.unlinkSync(slotPath); + } catch { + /* already removed or unreadable */ + } + }; + process.on('exit', release); + return release; } catch { - stale = true; - } - } - if (stale) { - try { - fs.unlinkSync(lockPath); - } catch { - /* another hook beat us to it */ + // Slot exists. Decide whether to take it over. + let stat; + try { + stat = fs.statSync(slotPath); + } catch { + continue; // Vanished between EEXIST and stat — retry this slot. + } + let isLive = false; + try { + const ownerStr = fs.readFileSync(slotPath, 'utf-8').trim(); + if (ownerStr === '') { + // Owner created the file but hasn't written its PID yet. The + // wx open+write window is microseconds; give it the benefit + // of the doubt and treat as live. + isLive = true; + } else { + const owner = Number.parseInt(ownerStr, 10); + if (Number.isFinite(owner) && owner > 0) { + try { + process.kill(owner, 0); + isLive = true; + } catch { + /* ESRCH (or EPERM under cross-user) — treat as dead */ + } + } + } + } catch { + /* unreadable — treat as dead */ + } + // PID-liveness wins over age (avoids evicting a slow-but-alive hook). + // Age check is a safety net against PID reuse on long-abandoned slots: + // 30s >> the 7s augment timeout, so a healthy run never hits it. + if (isLive && Date.now() - stat.mtimeMs > HOOK_LOCK_STALE_MS) { + isLive = false; + } + if (isLive) break; // Try the next slot. + try { + fs.unlinkSync(slotPath); + } catch { + /* another hook beat us to it — retry will hit EEXIST */ + } + // Loop and retry this slot. } } } - if (active >= HOOK_LOCK_MAX_INFLIGHT) return null; - - const ownLock = path.join(lockDir, `${process.pid}.lock`); - try { - fs.writeFileSync(ownLock, '', { flag: 'wx' }); - } catch { - return () => {}; - } - - let released = false; - const release = () => { - if (released) return; - released = true; - try { - fs.unlinkSync(ownLock); - } catch { - /* pruned by another hook */ - } - }; - process.on('exit', release); - return release; + return null; } function runGitNexusCli(cliPath, args, cwd, timeout) { diff --git a/gitnexus/hooks/claude/gitnexus-hook.cjs b/gitnexus/hooks/claude/gitnexus-hook.cjs index b4276a4df..fca5a84cc 100755 --- a/gitnexus/hooks/claude/gitnexus-hook.cjs +++ b/gitnexus/hooks/claude/gitnexus-hook.cjs @@ -161,7 +161,7 @@ function extractPattern(toolName, toolInput) { } /** - * Concurrency guard for the augment subprocess. + * Concurrency guard for the augment subprocess (hard cap). * * Claude Code fires PreToolUse hooks per parallel tool call. With no cap, N * parallel Grep/Glob/Bash tool calls spawn N concurrent `gitnexus augment` @@ -171,8 +171,18 @@ function extractPattern(toolName, toolInput) { * silently above that. Augment is a best-effort enrichment; missing a few * fires under heavy parallel load is preferable to melting the box. * - * Implementation: lockfiles named `.lock` under `<.gitnexus>/.hook-locks/`. - * Stale entries are pruned by age (>30s mtime) or by pid-liveness check. + * Implementation: fixed-name slot files `slot-0.lock` ... `slot-N.lock` + * under `<.gitnexus>/.hook-locks/`. Each file is created with `wx` + * (O_CREAT|O_EXCL), which is atomic across processes at the OS level — + * exactly one process wins each slot. This is a HARD cap, not the + * count-then-claim soft cap that an earlier revision shipped (it had a + * TOCTOU window between readdirSync and the per-pid wx write). + * + * Owner identity is written into the slot file as the PID, used for + * stale-takeover when a hook crashes without releasing. PID-liveness is + * checked before age so a slow-but-alive hook is never wrongly evicted; + * the age window only kicks in to defend against PID reuse on a long- + * abandoned slot. */ const HOOK_LOCK_SUBDIR = '.hook-locks'; const HOOK_LOCK_MAX_INFLIGHT = 3; @@ -180,68 +190,83 @@ const HOOK_LOCK_STALE_MS = 30000; function acquireHookSlot(gitNexusDir) { const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR); - let entries; try { fs.mkdirSync(lockDir, { recursive: true }); - entries = fs.readdirSync(lockDir); } catch { - // Cannot read lock dir — fall through unguarded so the hook still works. + // Cannot create lock dir — fall through unguarded so the hook still works. return () => {}; } - const now = Date.now(); - let active = 0; - for (const entry of entries) { - if (!entry.endsWith('.lock')) continue; - const lockPath = path.join(lockDir, entry); - const pid = Number.parseInt(entry, 10); - let stale = false; - try { - const stat = fs.statSync(lockPath); - if (now - stat.mtimeMs > HOOK_LOCK_STALE_MS) stale = true; - } catch { - stale = true; - } - if (!stale && Number.isFinite(pid) && pid > 0) { + const myPidStr = String(process.pid); + + for (let slot = 0; slot < HOOK_LOCK_MAX_INFLIGHT; slot++) { + const slotPath = path.join(lockDir, `slot-${slot}.lock`); + for (let attempt = 0; attempt < 2; attempt++) { try { - process.kill(pid, 0); - active++; - continue; + fs.writeFileSync(slotPath, myPidStr, { flag: 'wx' }); + let released = false; + const release = () => { + if (released) return; + released = true; + try { + // Only unlink if we still own the slot. If we appeared stale and + // another hook took over, the file now belongs to it — leave alone. + const content = fs.readFileSync(slotPath, 'utf-8').trim(); + if (content === myPidStr) fs.unlinkSync(slotPath); + } catch { + /* already removed or unreadable */ + } + }; + process.on('exit', release); + return release; } catch { - stale = true; - } - } - if (stale) { - try { - fs.unlinkSync(lockPath); - } catch { - /* another hook beat us to it */ + // Slot exists. Decide whether to take it over. + let stat; + try { + stat = fs.statSync(slotPath); + } catch { + continue; // Vanished between EEXIST and stat — retry this slot. + } + let isLive = false; + try { + const ownerStr = fs.readFileSync(slotPath, 'utf-8').trim(); + if (ownerStr === '') { + // Owner created the file but hasn't written its PID yet. The + // wx open+write window is microseconds; give it the benefit + // of the doubt and treat as live. + isLive = true; + } else { + const owner = Number.parseInt(ownerStr, 10); + if (Number.isFinite(owner) && owner > 0) { + try { + process.kill(owner, 0); + isLive = true; + } catch { + /* ESRCH (or EPERM under cross-user) — treat as dead */ + } + } + } + } catch { + /* unreadable — treat as dead */ + } + // PID-liveness wins over age (avoids evicting a slow-but-alive hook). + // Age check is a safety net against PID reuse on long-abandoned slots: + // 30s >> the 7s augment timeout, so a healthy run never hits it. + if (isLive && Date.now() - stat.mtimeMs > HOOK_LOCK_STALE_MS) { + isLive = false; + } + if (isLive) break; // Try the next slot. + try { + fs.unlinkSync(slotPath); + } catch { + /* another hook beat us to it — retry will hit EEXIST */ + } + // Loop and retry this slot. } } } - if (active >= HOOK_LOCK_MAX_INFLIGHT) return null; - - const ownLock = path.join(lockDir, `${process.pid}.lock`); - try { - fs.writeFileSync(ownLock, '', { flag: 'wx' }); - } catch { - // Couldn't claim a slot — proceed unguarded rather than block legitimate work. - return () => {}; - } - - let released = false; - const release = () => { - if (released) return; - released = true; - try { - fs.unlinkSync(ownLock); - } catch { - /* pruned by another hook */ - } - }; - process.on('exit', release); - return release; + return null; } /** diff --git a/gitnexus/test/unit/cursor-hook.test.ts b/gitnexus/test/unit/cursor-hook.test.ts index f0d875dee..329095c92 100644 --- a/gitnexus/test/unit/cursor-hook.test.ts +++ b/gitnexus/test/unit/cursor-hook.test.ts @@ -60,16 +60,35 @@ function parseCursorOutput(stdout: string): { additional_context?: string } | nu // ─── Test fixtures ────────────────────────────────────────────────── let tmpDir: string; +// Separate fixture for the concurrency guard tests: this one has a real +// `.gitnexus/` so the hook reaches acquireHookSlot. The base tmpDir above +// deliberately has no .gitnexus so unrelated early-exit tests stay cheap. +let guardTmpDir: string; +let guardGitNexusDir: string; beforeAll(() => { tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-cursor-hook-test-')); spawnSync('git', ['init'], { cwd: tmpDir, stdio: 'pipe' }); spawnSync('git', ['config', 'user.email', 'test@test.com'], { cwd: tmpDir, stdio: 'pipe' }); spawnSync('git', ['config', 'user.name', 'Test'], { cwd: tmpDir, stdio: 'pipe' }); + + guardTmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-cursor-hook-guard-')); + guardGitNexusDir = path.join(guardTmpDir, '.gitnexus'); + fs.mkdirSync(guardGitNexusDir, { recursive: true }); + spawnSync('git', ['init'], { cwd: guardTmpDir, stdio: 'pipe' }); + spawnSync('git', ['config', 'user.email', 'test@test.com'], { + cwd: guardTmpDir, + stdio: 'pipe', + }); + spawnSync('git', ['config', 'user.name', 'Test'], { cwd: guardTmpDir, stdio: 'pipe' }); + fs.writeFileSync(path.join(guardTmpDir, 'dummy.txt'), 'hello'); + spawnSync('git', ['add', '.'], { cwd: guardTmpDir, stdio: 'pipe' }); + spawnSync('git', ['commit', '-m', 'init'], { cwd: guardTmpDir, stdio: 'pipe' }); }); afterAll(() => { fs.rmSync(tmpDir, { recursive: true, force: true }); + fs.rmSync(guardTmpDir, { recursive: true, force: true }); }); // ─── Manifest + hook file presence ─────────────────────────────────── @@ -375,6 +394,126 @@ describe('Cursor hook debug logging', () => { }); }); +// ─── Source code regression: concurrency guard (#1486) ───────────── + +describe('Cursor hook concurrency guard', () => { + const source = fs.readFileSync(CURSOR_HOOK, 'utf-8'); + + it('defines acquireHookSlot with MAX_INFLIGHT constant', () => { + expect(source).toContain('function acquireHookSlot'); + expect(source).toContain('HOOK_LOCK_MAX_INFLIGHT'); + }); + + it('calls acquireHookSlot in main() and releases via finally', () => { + // The Cursor hook uses a flat main() dispatcher rather than a separate + // handlePreToolUse — assert the guard call + finally release wiring is + // present so a future refactor cannot accidentally skip it. + expect(source).toContain('acquireHookSlot('); + expect(source).toMatch(/finally\s*\{[^}]*release\(\)/s); + }); + + it('uses atomic fixed-name slot files (hard cap, not soft TOCTOU cap)', () => { + expect(source).toMatch(/slot-\$\{slot\}\.lock|`slot-/); + const slotFn = source.slice( + source.indexOf('function acquireHookSlot'), + source.indexOf('function', source.indexOf('function acquireHookSlot') + 1), + ); + expect(slotFn).not.toContain('readdirSync'); + }); +}); + +// ─── Integration: concurrency guard skips when slots are full ────── + +describe('Cursor hook concurrency guard (integration)', () => { + it('exits silently when all MAX_INFLIGHT slots hold live pids', async () => { + const { spawn } = await import('child_process'); + const lockDir = path.join(guardGitNexusDir, '.hook-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + + const sleepers = [0, 1, 2].map(() => + spawn(process.execPath, ['-e', 'setTimeout(()=>{},60000)'], { + stdio: 'ignore', + detached: false, + }), + ); + const writtenLocks: string[] = []; + try { + for (let i = 0; i < sleepers.length; i++) { + const p = path.join(lockDir, `slot-${i}.lock`); + fs.writeFileSync(p, String(sleepers[i].pid)); + writtenLocks.push(p); + } + + const result = runHook(CURSOR_HOOK, { + tool_name: 'Grep', + tool_input: { query: 'validateUser' }, + cwd: guardTmpDir, + }); + + expect(result.stdout.trim()).toBe(''); + for (let i = 0; i < sleepers.length; i++) { + const p = path.join(lockDir, `slot-${i}.lock`); + expect(fs.existsSync(p)).toBe(true); + expect(fs.readFileSync(p, 'utf-8').trim()).toBe(String(sleepers[i].pid)); + } + } finally { + for (const child of sleepers) { + try { + child.kill(); + } catch { + /* ignore */ + } + } + for (const p of writtenLocks) { + try { + fs.unlinkSync(p); + } catch { + /* ignore */ + } + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); + + it('reclaims a slot held by a dead pid', () => { + const lockDir = path.join(guardGitNexusDir, '.hook-locks'); + fs.mkdirSync(lockDir, { recursive: true }); + const deadPid = 2_147_483_640; + const stalePath = path.join(lockDir, 'slot-0.lock'); + try { + fs.writeFileSync(stalePath, String(deadPid)); + expect(fs.readFileSync(stalePath, 'utf-8').trim()).toBe(String(deadPid)); + + runHook(CURSOR_HOOK, { + tool_name: 'Grep', + tool_input: { query: 'validateUser' }, + cwd: guardTmpDir, + }); + + // The hook reclaimed and then released slot-0 — either gone (released) + // or no longer owned by the dead pid. + if (fs.existsSync(stalePath)) { + expect(fs.readFileSync(stalePath, 'utf-8').trim()).not.toBe(String(deadPid)); + } + } finally { + try { + fs.unlinkSync(stalePath); + } catch { + /* already pruned */ + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); +}); + // ─── Documented contract behavior (extractPattern via the live hook) ─ describe('Shell quoted-pattern parser limitations (documented)', () => { diff --git a/gitnexus/test/unit/hooks.test.ts b/gitnexus/test/unit/hooks.test.ts index 9ae5124f5..c34667367 100644 --- a/gitnexus/test/unit/hooks.test.ts +++ b/gitnexus/test/unit/hooks.test.ts @@ -316,6 +316,21 @@ describe('PreToolUse concurrency guard', () => { expect(preBody).toContain('acquireHookSlot('); expect(preBody).toMatch(/release\(\)/); }); + + it(`${label} hook uses atomic fixed-name slot files (hard cap)`, () => { + // Regression for the TOCTOU soft-cap: an earlier revision counted + // entries then wrote a per-pid lock, which let simultaneous bursts + // exceed MAX_INFLIGHT. The hard-cap version writes to fixed-name + // slot-N.lock paths so O_CREAT|O_EXCL is atomic across processes. + const source = fs.readFileSync(hookPath, 'utf-8'); + expect(source).toMatch(/slot-\$\{slot\}\.lock|`slot-/); + // And no longer reads the lock dir to count active hooks. + const slotFn = source.slice( + source.indexOf('function acquireHookSlot'), + source.indexOf('function', source.indexOf('function acquireHookSlot') + 1), + ); + expect(slotFn).not.toContain('readdirSync'); + }); } }); @@ -326,7 +341,7 @@ describe('PreToolUse concurrency guard (integration)', () => { ['CJS', CJS_HOOK], ['Plugin', PLUGIN_HOOK], ] as const) { - it(`${label}: hook exits silently when MAX_INFLIGHT lockfiles for live pids exist`, async () => { + it(`${label}: hook exits silently when all MAX_INFLIGHT slots hold live pids`, async () => { const { spawn } = await import('child_process'); const lockDir = path.join(gitNexusDir, '.hook-locks'); fs.mkdirSync(lockDir, { recursive: true }); @@ -340,9 +355,10 @@ describe('PreToolUse concurrency guard (integration)', () => { ); const writtenLocks: string[] = []; try { - for (const child of sleepers) { - const p = path.join(lockDir, `${child.pid}.lock`); - fs.writeFileSync(p, ''); + for (let i = 0; i < sleepers.length; i++) { + // Slot files are named slot-N.lock; content is the owning PID. + const p = path.join(lockDir, `slot-${i}.lock`); + fs.writeFileSync(p, String(sleepers[i].pid)); writtenLocks.push(p); } @@ -354,9 +370,13 @@ describe('PreToolUse concurrency guard (integration)', () => { }); expect(result.stdout.trim()).toBe(''); - const afterFiles = fs.readdirSync(lockDir).filter((f) => f.endsWith('.lock')); - // Sentinel locks remain; the hook bailed before acquiring its own slot. - expect(afterFiles.length).toBe(3); + // Sentinel slot files survive; the hook bailed before claiming any of them. + for (let i = 0; i < sleepers.length; i++) { + const p = path.join(lockDir, `slot-${i}.lock`); + expect(fs.existsSync(p)).toBe(true); + // Owner unchanged. + expect(fs.readFileSync(p, 'utf-8').trim()).toBe(String(sleepers[i].pid)); + } } finally { for (const child of sleepers) { try { @@ -380,17 +400,17 @@ describe('PreToolUse concurrency guard (integration)', () => { } }); - it(`${label}: hook prunes stale lockfiles (dead pid)`, () => { + it(`${label}: hook reclaims a slot held by a dead pid`, () => { const lockDir = path.join(gitNexusDir, '.hook-locks'); fs.mkdirSync(lockDir, { recursive: true }); // PID 1 exists on every POSIX system (init); on Windows process.kill(1,0) // throws. Use a definitely-dead PID instead: a very large number unlikely // to be assigned. const deadPid = 2_147_483_640; - const stalePath = path.join(lockDir, `${deadPid}.lock`); + const stalePath = path.join(lockDir, 'slot-0.lock'); try { - fs.writeFileSync(stalePath, ''); - expect(fs.existsSync(stalePath)).toBe(true); + fs.writeFileSync(stalePath, String(deadPid)); + expect(fs.readFileSync(stalePath, 'utf-8').trim()).toBe(String(deadPid)); runHook(hookPath, { hook_event_name: 'PreToolUse', @@ -399,8 +419,11 @@ describe('PreToolUse concurrency guard (integration)', () => { cwd: tmpDir, }); - // After the hook runs, the dead-pid lockfile should be pruned. - expect(fs.existsSync(stalePath)).toBe(false); + // The hook reclaimed and then released slot-0 — either the file is + // gone (released) or its content is something other than the dead PID. + if (fs.existsSync(stalePath)) { + expect(fs.readFileSync(stalePath, 'utf-8').trim()).not.toBe(String(deadPid)); + } } finally { try { fs.unlinkSync(stalePath); @@ -414,6 +437,126 @@ describe('PreToolUse concurrency guard (integration)', () => { } } }); + + it(`${label}: hook does not exceed MAX_INFLIGHT under simultaneous bursts (hard cap)`, async () => { + // Spawn many hook processes concurrently and assert that at most + // MAX_INFLIGHT (3) slot files end up populated by live pids. The + // O_CREAT|O_EXCL slot scheme makes this a hard cap, not the soft cap + // that the count-then-claim approach gives. + const { spawn } = await import('child_process'); + const lockDir = path.join(gitNexusDir, '.hook-locks'); + // Clean any leftover slot files. + try { + for (const f of fs.readdirSync(lockDir)) fs.unlinkSync(path.join(lockDir, f)); + } catch { + /* dir may not exist yet */ + } + fs.mkdirSync(lockDir, { recursive: true }); + + // We use child workers that just claim a slot via the same algorithm + // and then sleep, so we can observe the on-disk state under contention + // without spawning the real gitnexus augment CLI. + const claimerScript = ` + const fs = require('fs'); const path = require('path'); + const lockDir = ${JSON.stringify(lockDir)}; + const MAX = 3; + const STALE = 30000; + const myPid = String(process.pid); + function tryAcquire() { + for (let slot = 0; slot < MAX; slot++) { + const p = path.join(lockDir, 'slot-' + slot + '.lock'); + for (let a = 0; a < 2; a++) { + try { fs.writeFileSync(p, myPid, { flag: 'wx' }); return p; } + catch { + let stat; try { stat = fs.statSync(p); } catch { continue; } + let live = false; + try { + const s = fs.readFileSync(p, 'utf-8').trim(); + if (s === '') live = true; + else { const o = Number.parseInt(s, 10); + if (Number.isFinite(o) && o > 0) { try { process.kill(o, 0); live = true; } catch {} } + } + } catch {} + if (live && Date.now() - stat.mtimeMs > STALE) live = false; + if (live) break; + try { fs.unlinkSync(p); } catch {} + } + } + } + return null; + } + const claimed = tryAcquire(); + if (claimed) { + process.stdout.write('CLAIMED:' + claimed + '\\n'); + setTimeout(() => {}, 5000); + } else { + process.stdout.write('SKIPPED\\n'); + } + `; + + const N = 10; + const claimers = Array.from({ length: N }, () => + spawn(process.execPath, ['-e', claimerScript], { + stdio: ['ignore', 'pipe', 'ignore'], + detached: false, + }), + ); + try { + // Wait until every claimer has printed its decision. + const decisions = await Promise.all( + claimers.map( + (c) => + new Promise((resolve) => { + let buf = ''; + c.stdout!.on('data', (d) => { + buf += d.toString(); + if (buf.includes('\n')) resolve(buf.split('\n')[0]); + }); + c.on('exit', () => resolve(buf.split('\n')[0] || 'EXIT')); + }), + ), + ); + const claimedCount = decisions.filter((d) => d.startsWith('CLAIMED:')).length; + const skippedCount = decisions.filter((d) => d === 'SKIPPED').length; + + // HARD CAP: never more than 3 winners, regardless of how many bursts. + expect(claimedCount).toBeLessThanOrEqual(3); + // And the remainder must have all explicitly skipped. + expect(claimedCount + skippedCount).toBe(N); + + // On-disk state matches. + const liveSlots = fs + .readdirSync(lockDir) + .filter((f) => /^slot-\d+\.lock$/.test(f)) + .filter((f) => { + try { + const o = Number.parseInt(fs.readFileSync(path.join(lockDir, f), 'utf-8').trim(), 10); + return Number.isFinite(o) && o > 0; + } catch { + return false; + } + }); + expect(liveSlots.length).toBeLessThanOrEqual(3); + } finally { + for (const c of claimers) { + try { + c.kill(); + } catch { + /* ignore */ + } + } + try { + for (const f of fs.readdirSync(lockDir)) fs.unlinkSync(path.join(lockDir, f)); + } catch { + /* ignore */ + } + try { + fs.rmdirSync(lockDir); + } catch { + /* ignore */ + } + } + }); } });