mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-08-28 05:25:25 +00:00
* fix(hooks): cap concurrent augment subprocesses to prevent runaway process spawn (#1486) When Claude Code fires PreToolUse hooks for parallel Grep/Glob/Bash tool calls, each invocation spawned its own `gitnexus augment` subprocess — a Node + LadybugDB cold start that holds resources for several seconds. Under heavy parallel search load (issue #1486: 180+ piled-up processes, load avg > 100), these accumulated faster than they completed because nothing capped concurrent in-flight augments. Add a lockfile-based concurrency guard under `<.gitnexus>/.hook-locks/`: each running hook claims a `<pid>.lock`, the guard counts live PIDs and prunes stale entries (>30s mtime or pid no longer alive), and bails silently when MAX_INFLIGHT (3) is reached. Augment is best-effort enrichment — missing a few fires under burst load is preferable to melting the system. Applied to all three hook variants that spawn augment: - gitnexus/hooks/claude/gitnexus-hook.cjs (npm-installed Claude hook) - gitnexus-claude-plugin/hooks/gitnexus-hook.js (plugin Claude hook) - gitnexus-cursor-integration/hooks/gitnexus-hook.cjs (Cursor hook) Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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> * fix(hooks): inspect slot mtime + content via single fd (codeql TOCTOU) CodeQL flagged the stale-takeover path in acquireHookSlot as a potential filesystem race (js/file-system-race): statSync(slotPath) followed by readFileSync(slotPath) gives a TOCTOU window where the file could be swapped between the metadata check and the content read. Replace the two separate path-based calls with a single openSync + fstatSync + readSync + closeSync sequence. Both mtime and owner PID now come from the same file descriptor, so the operations are atomic on one inode. No behavioral change beyond closing the race. Applied to all three hook variants (CJS, Plugin, Cursor). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(hooks): distinguish EPERM from ESRCH in PID liveness check Cursor Bugbot caught a contradiction with the stated design: the bare `catch` after `process.kill(owner, 0)` was treating EPERM (process exists but owned by another user) the same as ESRCH (process gone), which would evict a live slot whenever the lock dir straddled user boundaries. Inspect the error code: ESRCH → dead, evict; EPERM → still alive, keep the slot; anything else → assume alive (be conservative under unexpected failure rather than over-evict). Applied to all three hook variants. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * 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> * refactor(hooks): extract lock guard into helper modules Agent-Logs-Url: https://github.com/abhigyanpatwari/GitNexus/sessions/04dd20c5-28fd-433a-83cf-ad83fd03fb32 --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com> Co-authored-by: Gergő Magyar <gergomagyar@icloud.com> Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com>
119 lines
4.1 KiB
JavaScript
119 lines
4.1 KiB
JavaScript
const fs = require('fs');
|
|
const path = require('path');
|
|
|
|
const HOOK_LOCK_SUBDIR = '.hook-locks';
|
|
const HOOK_LOCK_MAX_INFLIGHT = 3;
|
|
const HOOK_LOCK_STALE_MS = 30000;
|
|
|
|
function acquireHookSlot(gitNexusDir) {
|
|
const lockDir = path.join(gitNexusDir, HOOK_LOCK_SUBDIR);
|
|
try {
|
|
fs.mkdirSync(lockDir, { recursive: true });
|
|
} catch {
|
|
// 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);
|
|
|
|
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 {
|
|
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 {
|
|
// Slot exists. Decide whether to take it over.
|
|
// Open once and inspect mtime + content via the same fd so there's
|
|
// no TOCTOU between the metadata check and the content read
|
|
// (codeql js/file-system-race).
|
|
let fd;
|
|
try {
|
|
fd = fs.openSync(slotPath, 'r');
|
|
} catch {
|
|
continue; // Vanished between EEXIST and open — retry this slot.
|
|
}
|
|
let isLive = false;
|
|
let mtimeMs = Date.now();
|
|
try {
|
|
mtimeMs = fs.fstatSync(fd).mtimeMs;
|
|
const buf = Buffer.alloc(32);
|
|
const n = fs.readSync(fd, buf, 0, 32, 0);
|
|
const ownerStr = buf.slice(0, n).toString('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 (e) {
|
|
// ESRCH = process gone → treat as dead. EPERM = process exists
|
|
// but owned by another user (cross-user lock dir) → still alive,
|
|
// keep the slot. Anything else: be conservative, assume alive.
|
|
if (e && e.code === 'ESRCH') {
|
|
isLive = false;
|
|
} else {
|
|
isLive = true;
|
|
}
|
|
}
|
|
}
|
|
}
|
|
} catch {
|
|
/* unreadable — treat as dead */
|
|
} finally {
|
|
try {
|
|
fs.closeSync(fd);
|
|
} catch {
|
|
/* already closed */
|
|
}
|
|
}
|
|
// 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;
|
|
}
|
|
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.
|
|
}
|
|
}
|
|
}
|
|
|
|
return null;
|
|
}
|
|
|
|
module.exports = {
|
|
HOOK_LOCK_SUBDIR,
|
|
HOOK_LOCK_MAX_INFLIGHT,
|
|
HOOK_LOCK_STALE_MS,
|
|
acquireHookSlot,
|
|
};
|