mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-09-25 01:01:28 +00:00
fix(hooks): make augment concurrency cap a hard cap via atomic slot files
Address Claude's review of #1510. The original count-then-claim guard had a TOCTOU window: N hooks could each read `active < MAX_INFLIGHT` between readdirSync and the per-pid `wx` write and all proceed, briefly exceeding the cap. The PR title's "cap" language overstated this. Replace with fixed-name `slot-0.lock` ... `slot-N.lock` under `.hook-locks/`. `O_CREAT|O_EXCL` on a fixed path is OS-atomic — exactly one process wins each slot, so the cap is hard regardless of burst arrival timing. Each slot file contains the owning PID so stale-takeover still works when a hook crashes without releasing. PID liveness is checked before age (Claude's Finding 3): a slow-but-alive hook is never wrongly evicted. The 30s age window only kicks in to defend against PID reuse on a long-abandoned slot, well above the 7s augment timeout so a healthy run never hits it. Also adds the missing concurrency-guard tests to cursor-hook.test.ts (Claude's Finding 2): source-level wiring + dead-PID reclaim + 3-slots-full bail. Previously only the CJS and Plugin variants had test coverage for the guard; the Cursor variant was validated only by code inspection. Tests: 5726 passing, +9 from baseline (1 hard-cap burst test + 4 source regressions in hooks.test.ts; 3 source + 2 integration in cursor-hook.test.ts). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
6a34622c38
commit
c0baba86ce
5 changed files with 529 additions and 168 deletions
|
|
@ -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 `<pid>.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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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 `<pid>.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) {
|
||||
|
|
|
|||
|
|
@ -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 `<pid>.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;
|
||||
}
|
||||
|
||||
/**
|
||||
|
|
|
|||
|
|
@ -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)', () => {
|
||||
|
|
|
|||
|
|
@ -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<string>((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 */
|
||||
}
|
||||
}
|
||||
});
|
||||
}
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue