fix(hooks): fail closed when lock dir cannot be created

Previously the mkdirSync catch in acquireHookSlot returned `() => {}`
(a truthy no-op). The caller checks `if (!release) return;` to skip
augment when the guard can't be established — but a truthy no-op
slipped through that check and let augment spawn unguarded. On a
cross-user shared `.gitnexus/` or read-only filesystem, N concurrent
hooks would each take that branch and reintroduce the #1486 fan-out
the guard exists to prevent.

Return `null` instead so the caller's `if (!release) return;` skips
augment cleanly. Augment is best-effort enrichment — skipping it when
the guard fails is strictly safer than running unguarded.

Also clarify the stale-slot comment: PID-liveness wins for slots
younger than HOOK_LOCK_STALE_MS, but age is the final arbiter beyond
30s (PID-reuse defense). The previous wording said "PID-liveness wins
over age" without qualifying it, which contradicted the >30s branch.

Add source-level regression tests in hooks.test.ts and
cursor-hook.test.ts asserting acquireHookSlot returns null (not
() => {}) on lock-dir failure. Note in the Cursor test file that the
10-spawner burst test is not duplicated because the algorithm is
byte-for-byte identical to the CJS hook and already covered there.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
abhigyanpatwari 2026-05-12 23:13:44 +05:30
parent bff714dbd1
commit 482ce52458
5 changed files with 71 additions and 15 deletions

View file

@ -193,8 +193,11 @@ function acquireHookSlot(gitNexusDir) {
try {
fs.mkdirSync(lockDir, { recursive: true });
} catch {
// Cannot create lock dir — fall through unguarded so the hook still works.
return () => {};
// Cannot create lock dir (read-only fs, cross-user perm denial, out of
// inodes, etc.) — fail closed by returning null. Caller skips augment.
// Fail-open here would let N concurrent hooks all proceed unguarded and
// reintroduce the #1486 fan-out the guard exists to prevent.
return null;
}
const myPidStr = String(process.pid);
@ -269,9 +272,11 @@ function acquireHookSlot(gitNexusDir) {
/* already closed */
}
}
// 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.
// For slots younger than HOOK_LOCK_STALE_MS, PID-liveness wins —
// a slow-but-alive hook is never wrongly evicted. For older slots,
// age is the final arbiter as a defense against PID reuse on long-
// abandoned slots. 30s >> the 7s augment timeout, so a healthy run
// never crosses this threshold.
if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) {
isLive = false;
}

View file

@ -223,8 +223,11 @@ function acquireHookSlot(gitNexusDir) {
try {
fs.mkdirSync(lockDir, { recursive: true });
} catch {
// Cannot create lock dir — fall through unguarded so the hook still works.
return () => {};
// Cannot create lock dir (read-only fs, cross-user perm denial, out of
// inodes, etc.) — fail closed by returning null. Caller skips augment.
// Fail-open here would let N concurrent hooks all proceed unguarded and
// reintroduce the #1486 fan-out the guard exists to prevent.
return null;
}
const myPidStr = String(process.pid);
@ -299,9 +302,11 @@ function acquireHookSlot(gitNexusDir) {
/* already closed */
}
}
// 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.
// For slots younger than HOOK_LOCK_STALE_MS, PID-liveness wins —
// a slow-but-alive hook is never wrongly evicted. For older slots,
// age is the final arbiter as a defense against PID reuse on long-
// abandoned slots. 30s >> the 7s augment timeout, so a healthy run
// never crosses this threshold.
if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) {
isLive = false;
}

View file

@ -193,8 +193,11 @@ function acquireHookSlot(gitNexusDir) {
try {
fs.mkdirSync(lockDir, { recursive: true });
} catch {
// Cannot create lock dir — fall through unguarded so the hook still works.
return () => {};
// Cannot create lock dir (read-only fs, cross-user perm denial, out of
// inodes, etc.) — fail closed by returning null. Caller skips augment.
// Fail-open here would let N concurrent hooks all proceed unguarded and
// reintroduce the #1486 fan-out the guard exists to prevent.
return null;
}
const myPidStr = String(process.pid);
@ -269,9 +272,11 @@ function acquireHookSlot(gitNexusDir) {
/* already closed */
}
}
// 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.
// For slots younger than HOOK_LOCK_STALE_MS, PID-liveness wins —
// a slow-but-alive hook is never wrongly evicted. For older slots,
// age is the final arbiter as a defense against PID reuse on long-
// abandoned slots. 30s >> the 7s augment timeout, so a healthy run
// never crosses this threshold.
if (isLive && Date.now() - mtimeMs > HOOK_LOCK_STALE_MS) {
isLive = false;
}

View file

@ -420,6 +420,29 @@ describe('Cursor hook concurrency guard', () => {
);
expect(slotFn).not.toContain('readdirSync');
});
it('fails closed when lock dir cannot be created', () => {
// Regression: see hooks.test.ts. The mkdirSync catch must return null
// (skip augment) rather than `() => {}` (proceed unguarded), so that
// a read-only or cross-user `.gitnexus/` cannot reintroduce #1486.
const slotFn = source.slice(
source.indexOf('function acquireHookSlot'),
source.indexOf('function', source.indexOf('function acquireHookSlot') + 1),
);
const mkdirCatch = slotFn.slice(
slotFn.indexOf('fs.mkdirSync(lockDir'),
slotFn.indexOf('const myPidStr'),
);
expect(mkdirCatch).toContain('return null');
expect(mkdirCatch).not.toMatch(/return\s*\(\s*\)\s*=>\s*\{\s*\}/);
});
// Note: the 10-concurrent-spawner burst test that validates `wx`
// (O_CREAT|O_EXCL) under simultaneous contention lives in
// hooks.test.ts. The Cursor hook uses byte-for-byte the same
// acquireHookSlot, so duplicating the burst test here would only test
// the OS primitive, not Cursor-specific wiring. The source-level checks
// above guarantee the Cursor hook keeps calling that same algorithm.
});
// ─── Integration: concurrency guard skips when slots are full ──────

View file

@ -331,6 +331,24 @@ describe('PreToolUse concurrency guard', () => {
);
expect(slotFn).not.toContain('readdirSync');
});
it(`${label} hook fails closed when lock dir cannot be created`, () => {
// Regression: an earlier revision returned `() => {}` (truthy no-op) on
// mkdirSync failure, which left callers — `if (!release) return;` — to
// proceed unguarded and reintroduce the #1486 fan-out on read-only or
// cross-user `.gitnexus/` setups. The guard must fail closed (null).
const source = fs.readFileSync(hookPath, 'utf-8');
const slotFn = source.slice(
source.indexOf('function acquireHookSlot'),
source.indexOf('function', source.indexOf('function acquireHookSlot') + 1),
);
const mkdirCatch = slotFn.slice(
slotFn.indexOf('fs.mkdirSync(lockDir'),
slotFn.indexOf('const myPidStr'),
);
expect(mkdirCatch).toContain('return null');
expect(mkdirCatch).not.toMatch(/return\s*\(\s*\)\s*=>\s*\{\s*\}/);
});
}
});