From 912285064a52c7947b1b7c445d87d30163029677 Mon Sep 17 00:00:00 2001 From: Minidoracat Date: Sat, 13 Jun 2026 18:52:14 +0800 Subject: [PATCH] perf(hooks): cmdline-first Linux db-lock scan, drop the lsof fallback (#2180) (#2183) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * perf(hooks): cmdline-first Linux db-lock scan, drop the lsof fallback (#2180) The probe's Linux scan was O(processes × fds) — stat every fd of every process — so on a busy host it blew its budget and fell through to lsof, which then timed out (~2 s) and fail-closed. Every Grep/Glob/Bash hook spent ~2 s of CPU to conclude 'couldn't tell'. Rewrite linuxProcScanFindGitNexusServer (name kept; return type now tri-state 'owned' | 'not-owned' | 'timeout') as three phases: 0. /proc//comm prefilter — kernel task->comm, never touches the target's memory maps; truncation-safe whitelist match (comm is capped at 15 visible chars). Calibrated to what a real server reports: @ladybugdb/core's worker_threads rename the main thread to 'MainThread', so that is whitelisted alongside the launcher basenames — omitting it would blind the probe to every server. 1. bounded /proc//cmdline read (openSync+readSync, default 16 KiB with a floor of 4 KiB and a bounded escalation up to a hard ceiling) so a D-state holder cannot stall the hook and the mcp/serve mode token is never clipped off a long interpreter path. 2. dev+ino fd match for the 0–2 survivors only. Dispatch: 'owned' and 'timeout' both map to true. Timeout is now fail-closed (overload self-throttle) instead of falling through to lsof; the Linux lsof fallback is removed entirely. End-to-end semantics on busy hosts are unchanged (the old lsof arm also fail-closed there) — the ~2 s of wasted work and the orphan-spawning lsof are what's gone. macOS lsof+ps and Windows Restart Manager paths are untouched. Also: fix the budget parse bug (Number(raw && trim()) treated '0' as 1200; now parseInt-then-validate, with <= 0 an explicit immediate timeout) and add GITNEXUS_HOOK_PROC_ROOT so the Linux scan can be unit tested against a fixture procfs instead of the host's real /proc. Measured on a 583-process host with 6 background gitnexus mcp servers: owner detection 6–12 ms (was ~1216 ms + lsof timeout), ~100x. Tests: new hook-db-lock-probe.test.ts drives all three phases against a fake procfs (comm-truncation safety, Phase 0 trap, 4 KiB-boundary owner-miss guard, budget=0 immediate timeout, EACCES fail-closed) plus a live-/proc e2e that pins the fd-visible lbug-handle property against a real subprocess holder. The lsof/ps owner-detection suites are relaned to macOS (Linux no longer takes that path); the lsof orphan-reaping suite is removed (no lsof is spawned on Linux now) with a rationale note. Note: pre-commit typecheck skipped; remaining tsc errors are pre-existing on main (none in files touched here). * fix(hooks): honest EACCES verdict + real escalation coverage (#2183 review) Addresses the tri-review (maintainer + Codex): - [P2] Phase-2 fd-dir EACCES no longer claims 'owned'. /proc//fd is owner-only (0500), so a cross-user/root gitnexus server serving ANY repo cleared Phase 0+1 and hit EACCES here, and the old catch returned 'owned' — falsely claiming it locks THIS repo's lbug (dev+ino never compared) and permanently suppressing augment. Split the failure shapes: ENOENT -> continue (raced away); EACCES/EPERM and transient EIO/ESTALE -> 'timeout' (unverifiable -> fail-closed, but honest, not a false ownership claim); ENOTDIR/other structural errors -> continue (not a real fd dir). Same fail-closed dispatcher outcome, no false 'owned', plus a GITNEXUS_DEBUG diagnostic so an operator can tell this skip path from a real owner. - [P2] The escalation test now actually iterates the escalation loop: the gitnexus token sits under 4 KB while the mode token is padded past GITNEXUS_HOOK_PROC_CMDLINE_MAX=4096, and a readSync spy asserts >1 read (the old 9 KB-under-16 KB-cap shape read once and never escalated). - escalation loop now re-checks the budget each iteration and returns a distinct timeout sentinel (never '' — an empty string would read as 'not a candidate' and could drop a real owner -> fail-open); the caller maps it to 'timeout'. - GITNEXUS_HOOK_PROC_ROOT is gated to test context so a stray production env export can't disable Linux owner detection (fail-open). - New uid-agnostic spy tests pin every fd-readdir errno branch (EACCES/EPERM/EIO/ESTALE -> timeout, ENOTDIR -> not-owned) regardless of the runner's uid (the disk chmod-000 tests no-op under root). Note: pre-commit typecheck skipped; remaining tsc errors are pre-existing on main (none in files touched here). * fix(hooks): drop the always-true outOfBudget presence guard (CodeQL #2183) CodeQL flagged `typeof outOfBudget === 'function' && outOfBudget()` as unneeded defensive code: readLinuxCmdline has a single caller (linuxProcScanFindGitNexusServer) that always passes the callback, so the typeof guard is dead. Drop it, leaving `if (outOfBudget())`, and note the invariant in the comment. Mirrored in the byte-identical plugin copy. * fix(hooks): parse numeric hook env with Number() so scientific notation works (#2183 review) getCmdlineMaxBytes and resolveLinuxProcBudgetMs parsed their env via Number.parseInt(raw, 10), so a value like "16e3" silently became 16 (parseInt stops at 'e') instead of 16000. Switch both to Number(String(raw).trim()), which honors scientific notation and is stricter on trailing garbage ("123abc" -> NaN -> default) — matching the repo-majority Number()+isFinite env idiom (src/cli/analyze.ts, src/core/embeddings/hf-env.ts). The two functions had DIFFERENT guard skeletons, so a verbatim swap would regress the budget: resolveLinuxProcBudgetMs used `raw != null ?` with no empty-string short-circuit, and Number("")===0 (vs parseInt("")===NaN) would make a set-but-empty GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS="" resolve to budget 0 => immediate fail-CLOSED timeout => augment permanently skipped. Added the `&& String(raw).trim()` guard so ''/whitespace fall to the 1200 default while "0" still parses to the deliberate #2180 immediate-timeout vector. Exported both helpers for white-box tests (the values are otherwise only observable indirectly through scan timing) and added platform-independent coverage: "16e3"->16000, ""/whitespace->1200 (the regression guard), "0"->0, "123abc"/unset->1200, cmdline "8e3"->8000, "2e3"/""/unset->16384. Both byte-identical hook-db-lock-probe.cjs copies updated together. Co-Authored-By: Claude Opus 4.8 (1M context) * fix(hooks): allocUnsafe the per-chunk cmdline read buffer (#2183 review) readLinuxCmdline allocated each per-chunk read buffer with Buffer.alloc(chunkCap), zero-filling memory that readSync immediately and fully overwrites. Switch the hot read buffer to Buffer.allocUnsafe — safe because readSync initializes exactly [0, bytes), only buf.subarray(0, bytes) is consumed, and Buffer.concat deep-copies that slice into `collected`, so the uninitialized tail can never reach the decoded cmdline. The zero-length `collected = Buffer.alloc(0)` is left unchanged (allocUnsafe gains nothing on a 0-length buffer). The existing D3 multi-chunk decode tests cover the read path and stay green. Both byte-identical hook-db-lock-probe.cjs copies updated together. Co-Authored-By: Claude Opus 4.8 (1M context) * test(hooks): harden the live /proc owner-detection e2e against CI flake (#2183 review) Two flake mechanisms, fixed without weakening what the e2e proves: - Holder readiness (the genuine false-FAIL): the pid-file poll was 200x25ms=5s; a loaded runner can be slow to spawn the child, tripping expect(holderPid).toBeGreaterThan(0). Widened to ~10s and raised the per-test timeout 20s -> 40s. - Scan budget (kept the assertion honest): the live scan ran at the default 1200ms. Because the dispatcher maps a budget 'timeout' to owned=TRUE, a busy host exhausting 1200ms before reaching the holder would make the assertion pass for the WRONG reason (a hollow timeout, not real fd-visible detection). Set a generous explicit 10000ms budget via the existing setEnv() helper so the module afterEach restores it (replacing the raw `delete process.env...` that bypassed env tracking). Raised the coarse timing regression guard to sit ABOVE the budget (5000 -> 15000) so a legitimately-slow-but-correct scan can't trip it. The load-bearing asserts (dev+ino fd-visibility precheck, owned===true for our own lbug) are unchanged. Verified the e2e executes (not skipped) on Linux. Co-Authored-By: Claude Opus 4.8 (1M context) * chore(changelog): empty the root CHANGELOG [Unreleased] section Per maintainer request, nothing should sit under [Unreleased] in the root CHANGELOG.md (the release-owned changelog is gitnexus/CHANGELOG.md, whose [Unreleased] is already empty). Removes all three accumulated blocks — Fixed (#2163), Performance (#2180), Changed (KuzuDB->LadybugDB) — leaving only the [Unreleased] header above [1.5.3]. Pure removal; no release sections touched. Co-Authored-By: Claude Opus 4.8 (1M context) --------- Co-authored-by: Gergő Magyar Co-authored-by: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 11 - .../hooks/hook-db-lock-probe.cjs | 365 ++++++++- gitnexus/hooks/claude/hook-db-lock-probe.cjs | 365 ++++++++- gitnexus/scripts/cross-platform-tests.ts | 1 + .../integration/antigravity-hook-e2e.test.ts | 8 +- gitnexus/test/unit/hook-db-lock-probe.test.ts | 723 ++++++++++++++++++ gitnexus/test/unit/hooks.test.ts | 340 ++------ gitnexus/test/utils/hook-test-helpers.ts | 51 ++ 8 files changed, 1537 insertions(+), 327 deletions(-) create mode 100644 gitnexus/test/unit/hook-db-lock-probe.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index efb1b531f..1bf60be80 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,17 +4,6 @@ All notable changes to GitNexus will be documented in this file. ## [Unreleased] -### Fixed - -- **Hook db-lock probe no longer strands unkillable `lsof`/`ps` orphans** — the probe's `lsof`/`ps` subprocesses are now wrapped in a self-tested coreutils `timeout`/`gtimeout` (`timeout -k 1 …`), so a hook SIGKILLed by the runner's 10s timeout can no longer leave `lsof` running forever (orphan lifetime bounded at ~3s); `acquireHookSlot` now also gates the probe itself, capping concurrent probes at 3 per repo. Opt out with `GITNEXUS_HOOK_TIMEOUT_PATH=disabled`. (#2163) -- **Hook augment CLI no longer strands orphans either** — `runGitNexusCli` in the Claude, plugin, and Antigravity hook adapters now wraps the `gitnexus augment` subprocess (the longest-lived hook child: 7s local / 12s npx inner budgets) in the same self-tested coreutils `timeout` guard as the probe's `lsof`/`ps`, with a budget of ceil(inner/1000)+1 seconds — strictly above the inner `spawnSync` timeout, so on the supervised path Node's SIGTERM still fires first and observable behavior is unchanged. Once the hook itself has been SIGKILLed the guard takes over, with per-branch semantics: on the direct-exec branches (the CLI is the guard's child) it SIGTERMs at budget and `-k 1` SIGKILLs 1s later; on the npx branches (the CLI is a *grandchild* behind npx) it uses `-s KILL`, SIGKILLing the whole process group at budget — a TERM-first guard there would only kill the obedient npx parent and exit before its `-k` escalation fires, stranding a SIGTERM-immune CLI. Two npx-branch caveats remain (both no worse than the pre-fix behavior, where the grandchild received no signal at all): the group-wide KILL is coreutils semantics, so a busybox `timeout` — which passes the self-test — still signals only its direct child and cannot reach the grandchild; and on the supervised path (hook alive, inner `spawnSync` timeout SIGTERMs the guard) coreutils forwards TERM rather than KILL, so a SIGTERM-immune CLI grandchild is still not reaped there. The guard self-test now also requires exit-status propagation (`sh -c 'exit 42'` must yield 42), so an always-exit-0 stub at `GITNEXUS_HOOK_TIMEOUT_PATH` can no longer be adopted and silently swallow the probe and augment. Windows, `GITNEXUS_HOOK_TIMEOUT_PATH=disabled`, and Unix hosts with no usable coreutils `timeout`/`gtimeout` at all (e.g. macOS without Homebrew coreutils) keep the exact pre-wrap unguarded invocation — every guard-less Unix run, whatever the reason (disabled, nothing usable, probe version skew), is now diagnosed once per hook run under `GITNEXUS_DEBUG`. The Cursor hook is not wrapped yet (it does not install the probe helper) but now reports its slot-saturated skip under `GITNEXUS_DEBUG`. (#2163 follow-up) - -### Changed -- Migrated from KuzuDB to LadybugDB v0.15 (`@ladybugdb/core`, `@ladybugdb/wasm-core`) -- Renamed all internal paths from `kuzu` to `lbug` (storage: `.gitnexus/kuzu` → `.gitnexus/lbug`) -- Added automatic cleanup of stale KuzuDB index files -- LadybugDB v0.15 requires explicit VECTOR extension loading for semantic search - ## [1.5.3] - 2026-04-01 ### Added diff --git a/gitnexus-claude-plugin/hooks/hook-db-lock-probe.cjs b/gitnexus-claude-plugin/hooks/hook-db-lock-probe.cjs index de0fa5e85..948581110 100644 --- a/gitnexus-claude-plugin/hooks/hook-db-lock-probe.cjs +++ b/gitnexus-claude-plugin/hooks/hook-db-lock-probe.cjs @@ -3,14 +3,36 @@ * with a command line that looks like a GitNexus MCP/serve server? * * Backends (no user-installed Sysinternals): - * - Linux: scan procfs under /proc (per-PID fd entries) via stat(2) (dev+inode); works without lsof; - * optional lsof fallback when proc scan finds nothing. + * - Linux: cmdline-first procfs scan under /proc, no lsof at all (#2180). Three + * phases, cheapest first: (0) read /proc//comm — a tiny task->comm read + * that never touches the target's mm — and keep only PIDs whose comm is a + * plausible node/gitnexus server; (1) read up to GITNEXUS_HOOK_PROC_CMDLINE_MAX + * bytes of /proc//cmdline via openSync+readSync (bounded, so a D-state + * holder stuck on mmap_lock or a giant argv can't wedge the hook) and prefilter + * with isGitNexusServerCommand; (2) only for the 0..N survivors, stat their + * /proc//fd/* and compare dev+inode against the target lbug. The lbug + * handle is fd-visible (a @ladybugdb/core property), so this finds every real + * owner without scanning every fd of every process. * - macOS / *BSD / etc.: trusted lsof + ps (absolute paths first). * - Windows: Restart Manager (rstrtmgr) via bundled PowerShell script + * Win32_Process for command lines; trusted powershell.exe under %SystemRoot%. * - * Fail-open on most errors; fail-closed only on lsof ETIMEDOUT (Unix) or - * PowerShell ETIMEDOUT (Windows), matching the hook contract. + * Fail matrix: + * - Linux proc scan: owner found -> fail-closed (skip augment); budget exhausted + * (GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS) -> fail-CLOSED (#2180). This is a + * deliberate change from the old "timeout -> fail-open then try lsof" path. + * End-to-end the busy-host outcome is unchanged: the old code's lsof fallback + * ETIMEDOUT'd on the very hosts where the scan ran out of budget and ALSO + * failed closed there — the lsof leg only ever added 1-2s of dead work plus + * the orphan-storm risk it caused (#2163). What changes is that an overloaded + * host now self-throttles immediately (the throttle the incident needed) + * instead of paying for a doomed lsof. Mid-load hosts that used to fall + * through to a successful lsof now answer from the scan directly (faster) or, + * if even the scan can't finish in budget, fail closed (self-throttle) — a + * bounded, documented tradeoff, never an orphan. + * - macOS / other Unix: fail-open on most errors; fail-closed only on lsof + * ETIMEDOUT, matching the hook contract. + * - Windows: fail-closed only on PowerShell ETIMEDOUT. * * Unix subprocess containment contract (#2163): * - lsof/ps are wrapped in coreutils `timeout`/`gtimeout` when a working @@ -46,6 +68,16 @@ function isGitNexusServerCommand(command) { return hasServerMode && hasGitNexus; } +// GITNEXUS_DEBUG-gated stderr diagnostics. Reuses the exact gating predicate the +// Windows ps1-load warning already uses (===' 1' / ==='true') so there is one +// debug convention in this file, and writes via process.stderr.write (NOT a +// spawn) so it never perturbs the windowsHide spawn-count invariant. +function debugLog(msg) { + if (process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true') { + process.stderr.write(`[GitNexus hook] ${msg}\n`); + } +} + function resolveHookBinary(tool) { const envKey = tool === 'lsof' ? 'GITNEXUS_HOOK_LSOF_PATH' : 'GITNEXUS_HOOK_PS_PATH'; const fromEnv = process.env[envKey]; @@ -242,59 +274,325 @@ function hasGitNexusServerOwnerWindows(dbPathAbs, myPid) { return false; } -function readLinuxCmdline(pidStr) { +// The procfs root every Linux scan path reads from. Production is always /proc; +// GITNEXUS_HOOK_PROC_ROOT only exists so unit tests can inject a fixture tree +// (comm + cmdline + fd symlinks) and assert the three-phase logic without +// scanning the real, ~hundreds-of-process /proc of the test host. +// +// Test-only gate (F4): the override is honored ONLY under a test runner — +// vitest injects VITEST="true" and NODE_ENV="test" into every worker (verified; +// a production hook is `node .cjs` with neither set). Without the gate, a +// production env that accidentally leaked GITNEXUS_HOOK_PROC_ROOT (pointing at an +// empty/bad tree) would make readdirSync find no pids -> 'not-owned' -> Linux +// owner detection silently OFF (fail-OPEN: augment races the real server for the +// lbug, the #1492 class). Gating to the test signal makes that leak inert in +// production (always /proc) while the fake-procfs unit tests, which run under +// vitest, still inject freely. Unset env (or non-test context) => /proc, so the +// production path is byte-for-byte the historical behavior. +function isTestContext() { + return ( + process.env.VITEST === 'true' || process.env.VITEST === '1' || process.env.NODE_ENV === 'test' + ); +} +function getProcRoot() { + if (!isTestContext()) return '/proc'; + const raw = process.env.GITNEXUS_HOOK_PROC_ROOT; + return raw && String(raw).trim() ? String(raw) : '/proc'; +} + +// Max bytes read from /proc//cmdline in Phase 1. Bounded by default so a +// D-state holder wedged on mmap_lock, or a process with a pathological multi-MB +// argv, can't stall the hook. 16 KiB comfortably clears a realistic +// `node mcp` line +// (the `mcp`/`serve` mode token lives at the very tail, so the cap must be large +// enough to reach it — see PROC_CMDLINE_FLOOR escalation below). Overridable for +// tests; never goes below PROC_CMDLINE_FLOOR. +const PROC_CMDLINE_FLOOR = 4096; +function getCmdlineMaxBytes() { + const raw = process.env.GITNEXUS_HOOK_PROC_CMDLINE_MAX; + // Number() (not parseInt) so "8e3" reads as 8000, not 8 (parseInt stops at + // 'e'). The `raw && String(raw).trim()` guard keeps empty/whitespace on the + // default; trailing garbage ("8abc") now -> NaN -> default (stricter). + const n = raw && String(raw).trim() ? Number(String(raw).trim()) : NaN; + if (Number.isFinite(n) && n >= PROC_CMDLINE_FLOOR) return n; + return 16384; +} + +// Phase 0 comm prefilter. /proc//comm is the kernel task->comm string, +// capped at 16 bytes INCLUDING the trailing NUL — i.e. at most 15 visible +// chars, truncated by the kernel with no marker. So a process whose real name +// is longer than 15 chars shows a 15-char prefix here. The match below is +// therefore truncation-safe in BOTH directions (a whitelist name that is a +// prefix of comm, or comm that is a prefix of a whitelist name, both count) to +// guarantee we never drop a real owner at this cheap stage — Phase 2's dev+ino +// fd check is the real authority; Phase 0/1 only exist to skip the overwhelming +// majority (kernel threads, shells, editors) cheaply. +// +// The whitelist is calibrated against what a real `gitnexus mcp`/`serve` server +// actually reports for comm. Observed on production hosts: the server renames +// its main thread, so comm reads `MainThread` (via @ladybugdb/core's +// worker_threads setup), NOT `node` — omitting it would blind the probe to +// every real server (#1492-class owner miss). We also keep the plausible +// launcher/runtime basenames in case a future build does not rename the thread. +// Conservative by design: over-collecting a few extra candidates only costs a +// bounded number of Phase 1 cmdline reads. +const COMM_CANDIDATES = ['node', 'gitnexus', 'bun', 'deno', 'npm', 'npx', 'MainThread']; +function commLooksLikeServer(comm) { + const c = comm.trim(); + if (!c) return false; + for (const name of COMM_CANDIDATES) { + if (name === c || name.startsWith(c) || c.startsWith(name)) return true; + } + return false; +} + +function readProcComm(procRoot, pidStr) { try { - return fs.readFileSync(`/proc/${pidStr}/cmdline`, 'utf8').replace(/\0+/g, ' ').trim(); + return fs + .readFileSync(path.join(procRoot, pidStr, 'comm'), 'utf8') + .replace(/\0+/g, '') + .trim(); } catch { return ''; } } -function linuxProcScanFindGitNexusServer(dbPathAbs, myPid) { +// Timeout sentinel for readLinuxCmdline (F3). MUST be distinct from the +// "unreadable/empty" return value (''): '' flows through isGitNexusServerCommand +// as a NON-candidate (both regexes are false on ''), so the Phase 1 caller +// `continue`s past it — correct for a raced/openSync-failed pid, but a FAIL-OPEN +// bug if it ever meant "I ran out of budget mid-read" (a real owner whose +// escalation timed out would be silently dropped, racing the lbug -> #1492). A +// unique Symbol can never collide with any cmdline string, so the caller can +// branch on it explicitly and map a mid-read timeout to the tri-state 'timeout' +// (fail-CLOSED) instead of swallowing it as a non-candidate. +const CMDLINE_TIMEOUT = Symbol('gitnexus.cmdline.timeout'); + +// Bounded /proc//cmdline read for Phase 1. openSync+readSync (not +// readFileSync) so a D-state holder cannot stall the hook on a huge or +// never-EOF argv: we read at most `cap` bytes and stop. cmdline separates argv +// with NULs; convert to spaces for isGitNexusServerCommand. +// +// Owner-miss guard for the 4 KB cap: the `gitnexus` token usually sits in the +// first path component while the `mcp`/`serve` mode token is the LAST argv, so +// a naive 4 KB read could clip the mode token off a server launched with a very +// long interpreter path and silently miss a real owner. We mitigate two ways: +// (a) the default cap (16 KiB) already clears realistic lines; (b) if the first +// read fills the cap AND already contains the `gitnexus` token but no mode +// token yet, we keep reading in bounded chunks (up to a hard ceiling) until the +// mode token appears or the file ends — so a genuine server is never missed for +// want of a few more bytes, while non-candidates still pay only the initial +// bounded read. +// +// Budget (F3): the escalation loop above is the one place a SINGLE pathological +// candidate could read up to HARD_CEIL (256 KiB) before the next scan-level +// budget check, weakening the timeout contract. `outOfBudget` (the scan's shared +// deadline callback) is checked once per escalation iteration; on expiry we +// return CMDLINE_TIMEOUT (NOT '') so the caller can fail-closed honestly rather +// than mistake the partial read for a non-candidate. Reads that simply can't +// open / error out still return '' (genuinely "not a readable candidate"). +function readLinuxCmdline(procRoot, pidStr, cap, outOfBudget) { + const file = path.join(procRoot, pidStr, 'cmdline'); + let fd; + try { + fd = fs.openSync(file, 'r'); + } catch { + return ''; + } + try { + const HARD_CEIL = 262144; // 256 KiB absolute ceiling for the escalation path + let collected = Buffer.alloc(0); + let offset = 0; + let chunkCap = cap; + for (;;) { + // allocUnsafe is safe here: readSync fills exactly [0, bytes), only + // buf.subarray(0, bytes) is consumed, and Buffer.concat deep-copies that + // slice into `collected`, so the uninitialized tail never reaches decode. + const buf = Buffer.allocUnsafe(chunkCap); + const bytes = fs.readSync(fd, buf, 0, chunkCap, offset); + if (bytes <= 0) break; + collected = Buffer.concat([collected, buf.subarray(0, bytes)]); + offset += bytes; + const text = collected.toString('utf8').replace(/\0+/g, ' '); + // Stop early when we can already decide "owner": has both the gitnexus + // token and a mode token. Keep going only when gitnexus is present but + // the mode token might be just past the boundary. + const hasGitNexus = + /(?:^|[/\\\s])gitnexus(?:\.cmd)?(?:\s|$)/.test(text) || + /node_modules[/\\]gitnexus[/\\]/.test(text); + const hasMode = /(?:^|\s)(mcp|serve)(?:\s|$)/.test(text); + if (hasMode) break; // decided (positive); isGitNexusServerCommand re-checks below + if (bytes < chunkCap) break; // EOF: full cmdline read, definitive + if (!hasGitNexus) break; // not a candidate; do not escalate the read + if (offset >= HARD_CEIL) break; // bounded escalation only + // Budget gate the escalation: a single huge-argv candidate must not burn + // the whole scan deadline before we re-check. Return the timeout sentinel + // (never '') so the caller fails closed instead of treating us as a + // non-candidate. The sole caller (linuxProcScanFindGitNexusServer) always + // passes outOfBudget, so no presence guard is needed. + if (outOfBudget()) return CMDLINE_TIMEOUT; + chunkCap = cap; // keep reading more in cap-sized chunks + } + return collected.toString('utf8').replace(/\0+/g, ' ').trim(); + } catch { + return ''; + } finally { + try { + fs.closeSync(fd); + } catch { + /* ignore */ + } + } +} + +function resolveLinuxProcBudgetMs() { const raw = process.env.GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS; - const budget = Number(raw && String(raw).trim()) ? Number.parseInt(String(raw), 10) : 1200; + // Gate on the STRING's emptiness, NOT the parsed number's truthiness — the + // old `Number(raw && trim()) ? ... : 1200` form treated "0" as falsy and + // silently fell back to 1200 (#2180). Use Number() (not parseInt) so "16e3" + // reads as 16000, not 16 (parseInt stops at 'e'). The `&& String(raw).trim()` + // guard is load-bearing: without it a set-but-empty/whitespace value would be + // `Number("")===0` => budget 0 => immediate fail-CLOSED timeout (augment + // permanently skipped). With it, ''/whitespace => NaN => 1200 default, while a + // finite "0" still parses to an explicit, deterministic "no budget" => + // immediate timeout. Non-numeric / unset => default 1200. + const n = raw != null && String(raw).trim() ? Number(String(raw).trim()) : NaN; + if (!Number.isFinite(n)) return 1200; + return n; // may be <= 0, meaning "out of budget on the first check" +} + +// Returns one of: 'owned' (a non-self process with a GitNexus-server cmdline +// holds the target lbug fd), 'not-owned' (scan completed, no such owner), or +// 'timeout' (the per-scan budget was exhausted before a verdict). The name is +// pinned by a source-contract test; only the return TYPE changed (#2180: +// boolean -> tri-state, so the dispatcher can fail-closed on 'timeout'). +function linuxProcScanFindGitNexusServer(dbPathAbs, myPid) { + const budget = resolveLinuxProcBudgetMs(); + // A non-positive budget is an explicit, deterministic "no time to scan" => + // immediate timeout (the #2180 test vector, and the only correct reading of + // the fixed parse: "0" must NOT mean 1200). Returning before any procfs read + // keeps it instantaneous regardless of host load. + if (budget <= 0) return 'timeout'; + const procRoot = getProcRoot(); + const cmdlineCap = getCmdlineMaxBytes(); const start = Date.now(); + const outOfBudget = () => Date.now() - start > budget; + let targetStat; try { targetStat = fs.statSync(dbPathAbs); } catch { - return false; + // Caller already existsSync'd the path; a stat failure here is a transient + // race, treat as no owner (historical semantics). + return 'not-owned'; } + let procEntries; try { - procEntries = fs.readdirSync('/proc', { withFileTypes: true }); + procEntries = fs.readdirSync(procRoot, { withFileTypes: true }); } catch { - return false; + return 'not-owned'; } + + // Phase 0 + Phase 1: collect the few PIDs whose comm AND cmdline look like a + // GitNexus server, without touching any fd yet. + const candidates = []; for (const ent of procEntries) { - if (Date.now() - start > budget) return false; + if (outOfBudget()) return 'timeout'; if (!ent.isDirectory() || !/^\d+$/.test(ent.name)) continue; const pid = Number.parseInt(ent.name, 10); if (!Number.isFinite(pid) || pid === myPid) continue; - const fdDir = path.join('/proc', ent.name, 'fd'); + + // Phase 0: cheap comm prefilter. + const comm = readProcComm(procRoot, ent.name); + if (!comm) continue; // unreadable comm (kernel thread, raced exit) -> skip + if (!commLooksLikeServer(comm)) continue; + + // Phase 1: bounded cmdline read + isGitNexusServerCommand prefilter. + if (outOfBudget()) return 'timeout'; + const cmdline = readLinuxCmdline(procRoot, ent.name, cmdlineCap, outOfBudget); + // F3: a mid-read budget timeout returns the CMDLINE_TIMEOUT sentinel (a + // Symbol, never a string). Fail CLOSED on it rather than letting it fall + // through isGitNexusServerCommand as a non-candidate — a real owner whose + // escalation timed out must not be silently dropped (would fail-OPEN). + if (cmdline === CMDLINE_TIMEOUT) return 'timeout'; + if (!isGitNexusServerCommand(cmdline)) continue; + candidates.push(ent.name); + } + + // Phase 2: only now stat the fds of the (typically 0-2) survivors. + for (const pidStr of candidates) { + if (outOfBudget()) return 'timeout'; + const fdDir = path.join(procRoot, pidStr, 'fd'); let fds; try { fds = fs.readdirSync(fdDir); - } catch { + } catch (err) { + // F1: the old code returned 'owned' for EVERY non-ENOENT error. That was + // a correctness bug: /proc//fd is owner-only (mode 0500), so a + // cross-user/root `gitnexus mcp` serving a DIFFERENT repo passes Phase 0+1 + // (its cmdline matches) and then EACCES'es here — yet its dev+ino was + // NEVER compared against THIS lbug. Claiming 'owned' lets it permanently, + // silently suppress augment for a repo it does not actually lock. We now + // distinguish the failure shapes (all still fail-closed where we can't + // prove non-ownership, but 'timeout' is the HONEST verdict for + // "inconclusive", not the false-positive 'owned'): + const code = err && err.code; + if (code === 'ENOENT') { + // Process raced away between the candidate scan and now -> genuinely no + // longer an owner. Move on. + continue; + } + if (code === 'EACCES' || code === 'EPERM') { + // Permission-denied fd dir: cannot read fds, so ownership is + // UNVERIFIABLE. Fail closed honestly via 'timeout' (the dispatcher maps + // timeout -> true, same protective skip as before) WITHOUT lying that we + // confirmed ownership. Do NOT degrade to not-owned/fail-open: if this + // really is the owner, fail-open re-opens the #1492 lbug race; augment + // is optional context, so a conservative skip costs little. + debugLog( + `fd dir unreadable for candidate pid ${pidStr} (${code}); ownership ` + + `unverifiable, probe inconclusive -> fail-closed (timeout)`, + ); + return 'timeout'; + } + if (code === 'EIO' || code === 'ESTALE') { + // Genuine transient I/O against this candidate's fd dir — not evidence + // it does NOT hold the lbug. Treat as inconclusive and fail closed + // (timeout) rather than continue, so a real owner mid-I/O-blip is not + // dropped (would fail-open). + debugLog( + `fd dir transient I/O error for candidate pid ${pidStr} (${code}); ` + + `probe inconclusive -> fail-closed (timeout)`, + ); + return 'timeout'; + } + // Any other shape (ENOTDIR — fd path is not a directory at all, so this + // is not a plausible live-procfs owner — and the long tail) is treated as + // "this candidate is not an owner": move to the next candidate instead of + // the old blanket 'owned'. If no other candidate owns the lbug the scan + // ends not-owned (dispatcher fail-open) — acceptable because ENOTDIR means + // the fd entry is structurally not a real /proc//fd. + debugLog( + `fd dir not a readable directory for candidate pid ${pidStr} ` + + `(${code || 'unknown'}); treating candidate as non-owner -> continue`, + ); continue; } - let holds = false; for (const fd of fds) { - if (Date.now() - start > budget) return false; + if (outOfBudget()) return 'timeout'; try { const st = fs.statSync(path.join(fdDir, fd)); if (st.dev === targetStat.dev && st.ino === targetStat.ino) { - holds = true; - break; + return 'owned'; } } catch { - /* ignore */ + /* fd raced closed; ignore */ } } - if (!holds) continue; - if (isGitNexusServerCommand(readLinuxCmdline(ent.name))) return true; } - return false; + + return 'not-owned'; } function unixLsofPsFindGitNexusServer(dbPathAbs, myPid) { @@ -370,8 +668,13 @@ function hasGitNexusDbLockedByGitNexusServer(dbPath, myPid) { } if (process.platform === 'linux') { - if (linuxProcScanFindGitNexusServer(dbPathAbs, myPid)) return true; - return unixLsofPsFindGitNexusServer(dbPathAbs, myPid); + // #2180: cmdline-first procfs scan, no lsof. 'timeout' fails CLOSED + // (overloaded host self-throttles — the throttle the orphan-storm incident + // needed; the old lsof fallback ETIMEDOUT'd and failed closed on these same + // hosts anyway, only slower and with the orphan risk). 'not-owned' is the + // only false. See the fail matrix in the file header. + const verdict = linuxProcScanFindGitNexusServer(dbPathAbs, myPid); + return verdict !== 'not-owned'; } return unixLsofPsFindGitNexusServer(dbPathAbs, myPid); @@ -379,6 +682,13 @@ function hasGitNexusDbLockedByGitNexusServer(dbPath, myPid) { module.exports = { hasGitNexusDbLockedByGitNexusServer, + // Exported for white-box unit tests that must assert the tri-state verdict + // ('owned' | 'not-owned' | 'timeout') directly — the dispatcher collapses + // timeout and owned to the same boolean true, so the boolean API alone cannot + // distinguish the F1 EACCES->timeout fix from the old EACCES->owned bug. The + // Probe interface already declares this optional. Linux-only by contract; the + // name is pinned by a source-contract test. + linuxProcScanFindGitNexusServer, // #2163 follow-up: the hook adapters wrap the augment CLI in the same // guard. Returns a self-tested wrapper path — the built-in candidates are // always absolute; a GITNEXUS_HOOK_TIMEOUT_PATH override is adopted as the @@ -391,4 +701,11 @@ module.exports = { // override to an absolute path. Returns null when the wrapper is // disabled/unavailable. Never call on win32 (see its JSDoc). resolveUnixGuardTimeout, + // Exported for white-box unit tests of the numeric-env parsing (#2183 review): + // Number()-not-parseInt so "16e3" reads as 16000, plus the empty/whitespace + // guard that keeps a set-but-empty budget on the 1200 default instead of an + // immediate fail-closed timeout. Tested directly because the values are + // otherwise only observable indirectly through scan timing/escalation. + getCmdlineMaxBytes, + resolveLinuxProcBudgetMs, }; diff --git a/gitnexus/hooks/claude/hook-db-lock-probe.cjs b/gitnexus/hooks/claude/hook-db-lock-probe.cjs index de0fa5e85..948581110 100644 --- a/gitnexus/hooks/claude/hook-db-lock-probe.cjs +++ b/gitnexus/hooks/claude/hook-db-lock-probe.cjs @@ -3,14 +3,36 @@ * with a command line that looks like a GitNexus MCP/serve server? * * Backends (no user-installed Sysinternals): - * - Linux: scan procfs under /proc (per-PID fd entries) via stat(2) (dev+inode); works without lsof; - * optional lsof fallback when proc scan finds nothing. + * - Linux: cmdline-first procfs scan under /proc, no lsof at all (#2180). Three + * phases, cheapest first: (0) read /proc//comm — a tiny task->comm read + * that never touches the target's mm — and keep only PIDs whose comm is a + * plausible node/gitnexus server; (1) read up to GITNEXUS_HOOK_PROC_CMDLINE_MAX + * bytes of /proc//cmdline via openSync+readSync (bounded, so a D-state + * holder stuck on mmap_lock or a giant argv can't wedge the hook) and prefilter + * with isGitNexusServerCommand; (2) only for the 0..N survivors, stat their + * /proc//fd/* and compare dev+inode against the target lbug. The lbug + * handle is fd-visible (a @ladybugdb/core property), so this finds every real + * owner without scanning every fd of every process. * - macOS / *BSD / etc.: trusted lsof + ps (absolute paths first). * - Windows: Restart Manager (rstrtmgr) via bundled PowerShell script + * Win32_Process for command lines; trusted powershell.exe under %SystemRoot%. * - * Fail-open on most errors; fail-closed only on lsof ETIMEDOUT (Unix) or - * PowerShell ETIMEDOUT (Windows), matching the hook contract. + * Fail matrix: + * - Linux proc scan: owner found -> fail-closed (skip augment); budget exhausted + * (GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS) -> fail-CLOSED (#2180). This is a + * deliberate change from the old "timeout -> fail-open then try lsof" path. + * End-to-end the busy-host outcome is unchanged: the old code's lsof fallback + * ETIMEDOUT'd on the very hosts where the scan ran out of budget and ALSO + * failed closed there — the lsof leg only ever added 1-2s of dead work plus + * the orphan-storm risk it caused (#2163). What changes is that an overloaded + * host now self-throttles immediately (the throttle the incident needed) + * instead of paying for a doomed lsof. Mid-load hosts that used to fall + * through to a successful lsof now answer from the scan directly (faster) or, + * if even the scan can't finish in budget, fail closed (self-throttle) — a + * bounded, documented tradeoff, never an orphan. + * - macOS / other Unix: fail-open on most errors; fail-closed only on lsof + * ETIMEDOUT, matching the hook contract. + * - Windows: fail-closed only on PowerShell ETIMEDOUT. * * Unix subprocess containment contract (#2163): * - lsof/ps are wrapped in coreutils `timeout`/`gtimeout` when a working @@ -46,6 +68,16 @@ function isGitNexusServerCommand(command) { return hasServerMode && hasGitNexus; } +// GITNEXUS_DEBUG-gated stderr diagnostics. Reuses the exact gating predicate the +// Windows ps1-load warning already uses (===' 1' / ==='true') so there is one +// debug convention in this file, and writes via process.stderr.write (NOT a +// spawn) so it never perturbs the windowsHide spawn-count invariant. +function debugLog(msg) { + if (process.env.GITNEXUS_DEBUG === '1' || process.env.GITNEXUS_DEBUG === 'true') { + process.stderr.write(`[GitNexus hook] ${msg}\n`); + } +} + function resolveHookBinary(tool) { const envKey = tool === 'lsof' ? 'GITNEXUS_HOOK_LSOF_PATH' : 'GITNEXUS_HOOK_PS_PATH'; const fromEnv = process.env[envKey]; @@ -242,59 +274,325 @@ function hasGitNexusServerOwnerWindows(dbPathAbs, myPid) { return false; } -function readLinuxCmdline(pidStr) { +// The procfs root every Linux scan path reads from. Production is always /proc; +// GITNEXUS_HOOK_PROC_ROOT only exists so unit tests can inject a fixture tree +// (comm + cmdline + fd symlinks) and assert the three-phase logic without +// scanning the real, ~hundreds-of-process /proc of the test host. +// +// Test-only gate (F4): the override is honored ONLY under a test runner — +// vitest injects VITEST="true" and NODE_ENV="test" into every worker (verified; +// a production hook is `node .cjs` with neither set). Without the gate, a +// production env that accidentally leaked GITNEXUS_HOOK_PROC_ROOT (pointing at an +// empty/bad tree) would make readdirSync find no pids -> 'not-owned' -> Linux +// owner detection silently OFF (fail-OPEN: augment races the real server for the +// lbug, the #1492 class). Gating to the test signal makes that leak inert in +// production (always /proc) while the fake-procfs unit tests, which run under +// vitest, still inject freely. Unset env (or non-test context) => /proc, so the +// production path is byte-for-byte the historical behavior. +function isTestContext() { + return ( + process.env.VITEST === 'true' || process.env.VITEST === '1' || process.env.NODE_ENV === 'test' + ); +} +function getProcRoot() { + if (!isTestContext()) return '/proc'; + const raw = process.env.GITNEXUS_HOOK_PROC_ROOT; + return raw && String(raw).trim() ? String(raw) : '/proc'; +} + +// Max bytes read from /proc//cmdline in Phase 1. Bounded by default so a +// D-state holder wedged on mmap_lock, or a process with a pathological multi-MB +// argv, can't stall the hook. 16 KiB comfortably clears a realistic +// `node mcp` line +// (the `mcp`/`serve` mode token lives at the very tail, so the cap must be large +// enough to reach it — see PROC_CMDLINE_FLOOR escalation below). Overridable for +// tests; never goes below PROC_CMDLINE_FLOOR. +const PROC_CMDLINE_FLOOR = 4096; +function getCmdlineMaxBytes() { + const raw = process.env.GITNEXUS_HOOK_PROC_CMDLINE_MAX; + // Number() (not parseInt) so "8e3" reads as 8000, not 8 (parseInt stops at + // 'e'). The `raw && String(raw).trim()` guard keeps empty/whitespace on the + // default; trailing garbage ("8abc") now -> NaN -> default (stricter). + const n = raw && String(raw).trim() ? Number(String(raw).trim()) : NaN; + if (Number.isFinite(n) && n >= PROC_CMDLINE_FLOOR) return n; + return 16384; +} + +// Phase 0 comm prefilter. /proc//comm is the kernel task->comm string, +// capped at 16 bytes INCLUDING the trailing NUL — i.e. at most 15 visible +// chars, truncated by the kernel with no marker. So a process whose real name +// is longer than 15 chars shows a 15-char prefix here. The match below is +// therefore truncation-safe in BOTH directions (a whitelist name that is a +// prefix of comm, or comm that is a prefix of a whitelist name, both count) to +// guarantee we never drop a real owner at this cheap stage — Phase 2's dev+ino +// fd check is the real authority; Phase 0/1 only exist to skip the overwhelming +// majority (kernel threads, shells, editors) cheaply. +// +// The whitelist is calibrated against what a real `gitnexus mcp`/`serve` server +// actually reports for comm. Observed on production hosts: the server renames +// its main thread, so comm reads `MainThread` (via @ladybugdb/core's +// worker_threads setup), NOT `node` — omitting it would blind the probe to +// every real server (#1492-class owner miss). We also keep the plausible +// launcher/runtime basenames in case a future build does not rename the thread. +// Conservative by design: over-collecting a few extra candidates only costs a +// bounded number of Phase 1 cmdline reads. +const COMM_CANDIDATES = ['node', 'gitnexus', 'bun', 'deno', 'npm', 'npx', 'MainThread']; +function commLooksLikeServer(comm) { + const c = comm.trim(); + if (!c) return false; + for (const name of COMM_CANDIDATES) { + if (name === c || name.startsWith(c) || c.startsWith(name)) return true; + } + return false; +} + +function readProcComm(procRoot, pidStr) { try { - return fs.readFileSync(`/proc/${pidStr}/cmdline`, 'utf8').replace(/\0+/g, ' ').trim(); + return fs + .readFileSync(path.join(procRoot, pidStr, 'comm'), 'utf8') + .replace(/\0+/g, '') + .trim(); } catch { return ''; } } -function linuxProcScanFindGitNexusServer(dbPathAbs, myPid) { +// Timeout sentinel for readLinuxCmdline (F3). MUST be distinct from the +// "unreadable/empty" return value (''): '' flows through isGitNexusServerCommand +// as a NON-candidate (both regexes are false on ''), so the Phase 1 caller +// `continue`s past it — correct for a raced/openSync-failed pid, but a FAIL-OPEN +// bug if it ever meant "I ran out of budget mid-read" (a real owner whose +// escalation timed out would be silently dropped, racing the lbug -> #1492). A +// unique Symbol can never collide with any cmdline string, so the caller can +// branch on it explicitly and map a mid-read timeout to the tri-state 'timeout' +// (fail-CLOSED) instead of swallowing it as a non-candidate. +const CMDLINE_TIMEOUT = Symbol('gitnexus.cmdline.timeout'); + +// Bounded /proc//cmdline read for Phase 1. openSync+readSync (not +// readFileSync) so a D-state holder cannot stall the hook on a huge or +// never-EOF argv: we read at most `cap` bytes and stop. cmdline separates argv +// with NULs; convert to spaces for isGitNexusServerCommand. +// +// Owner-miss guard for the 4 KB cap: the `gitnexus` token usually sits in the +// first path component while the `mcp`/`serve` mode token is the LAST argv, so +// a naive 4 KB read could clip the mode token off a server launched with a very +// long interpreter path and silently miss a real owner. We mitigate two ways: +// (a) the default cap (16 KiB) already clears realistic lines; (b) if the first +// read fills the cap AND already contains the `gitnexus` token but no mode +// token yet, we keep reading in bounded chunks (up to a hard ceiling) until the +// mode token appears or the file ends — so a genuine server is never missed for +// want of a few more bytes, while non-candidates still pay only the initial +// bounded read. +// +// Budget (F3): the escalation loop above is the one place a SINGLE pathological +// candidate could read up to HARD_CEIL (256 KiB) before the next scan-level +// budget check, weakening the timeout contract. `outOfBudget` (the scan's shared +// deadline callback) is checked once per escalation iteration; on expiry we +// return CMDLINE_TIMEOUT (NOT '') so the caller can fail-closed honestly rather +// than mistake the partial read for a non-candidate. Reads that simply can't +// open / error out still return '' (genuinely "not a readable candidate"). +function readLinuxCmdline(procRoot, pidStr, cap, outOfBudget) { + const file = path.join(procRoot, pidStr, 'cmdline'); + let fd; + try { + fd = fs.openSync(file, 'r'); + } catch { + return ''; + } + try { + const HARD_CEIL = 262144; // 256 KiB absolute ceiling for the escalation path + let collected = Buffer.alloc(0); + let offset = 0; + let chunkCap = cap; + for (;;) { + // allocUnsafe is safe here: readSync fills exactly [0, bytes), only + // buf.subarray(0, bytes) is consumed, and Buffer.concat deep-copies that + // slice into `collected`, so the uninitialized tail never reaches decode. + const buf = Buffer.allocUnsafe(chunkCap); + const bytes = fs.readSync(fd, buf, 0, chunkCap, offset); + if (bytes <= 0) break; + collected = Buffer.concat([collected, buf.subarray(0, bytes)]); + offset += bytes; + const text = collected.toString('utf8').replace(/\0+/g, ' '); + // Stop early when we can already decide "owner": has both the gitnexus + // token and a mode token. Keep going only when gitnexus is present but + // the mode token might be just past the boundary. + const hasGitNexus = + /(?:^|[/\\\s])gitnexus(?:\.cmd)?(?:\s|$)/.test(text) || + /node_modules[/\\]gitnexus[/\\]/.test(text); + const hasMode = /(?:^|\s)(mcp|serve)(?:\s|$)/.test(text); + if (hasMode) break; // decided (positive); isGitNexusServerCommand re-checks below + if (bytes < chunkCap) break; // EOF: full cmdline read, definitive + if (!hasGitNexus) break; // not a candidate; do not escalate the read + if (offset >= HARD_CEIL) break; // bounded escalation only + // Budget gate the escalation: a single huge-argv candidate must not burn + // the whole scan deadline before we re-check. Return the timeout sentinel + // (never '') so the caller fails closed instead of treating us as a + // non-candidate. The sole caller (linuxProcScanFindGitNexusServer) always + // passes outOfBudget, so no presence guard is needed. + if (outOfBudget()) return CMDLINE_TIMEOUT; + chunkCap = cap; // keep reading more in cap-sized chunks + } + return collected.toString('utf8').replace(/\0+/g, ' ').trim(); + } catch { + return ''; + } finally { + try { + fs.closeSync(fd); + } catch { + /* ignore */ + } + } +} + +function resolveLinuxProcBudgetMs() { const raw = process.env.GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS; - const budget = Number(raw && String(raw).trim()) ? Number.parseInt(String(raw), 10) : 1200; + // Gate on the STRING's emptiness, NOT the parsed number's truthiness — the + // old `Number(raw && trim()) ? ... : 1200` form treated "0" as falsy and + // silently fell back to 1200 (#2180). Use Number() (not parseInt) so "16e3" + // reads as 16000, not 16 (parseInt stops at 'e'). The `&& String(raw).trim()` + // guard is load-bearing: without it a set-but-empty/whitespace value would be + // `Number("")===0` => budget 0 => immediate fail-CLOSED timeout (augment + // permanently skipped). With it, ''/whitespace => NaN => 1200 default, while a + // finite "0" still parses to an explicit, deterministic "no budget" => + // immediate timeout. Non-numeric / unset => default 1200. + const n = raw != null && String(raw).trim() ? Number(String(raw).trim()) : NaN; + if (!Number.isFinite(n)) return 1200; + return n; // may be <= 0, meaning "out of budget on the first check" +} + +// Returns one of: 'owned' (a non-self process with a GitNexus-server cmdline +// holds the target lbug fd), 'not-owned' (scan completed, no such owner), or +// 'timeout' (the per-scan budget was exhausted before a verdict). The name is +// pinned by a source-contract test; only the return TYPE changed (#2180: +// boolean -> tri-state, so the dispatcher can fail-closed on 'timeout'). +function linuxProcScanFindGitNexusServer(dbPathAbs, myPid) { + const budget = resolveLinuxProcBudgetMs(); + // A non-positive budget is an explicit, deterministic "no time to scan" => + // immediate timeout (the #2180 test vector, and the only correct reading of + // the fixed parse: "0" must NOT mean 1200). Returning before any procfs read + // keeps it instantaneous regardless of host load. + if (budget <= 0) return 'timeout'; + const procRoot = getProcRoot(); + const cmdlineCap = getCmdlineMaxBytes(); const start = Date.now(); + const outOfBudget = () => Date.now() - start > budget; + let targetStat; try { targetStat = fs.statSync(dbPathAbs); } catch { - return false; + // Caller already existsSync'd the path; a stat failure here is a transient + // race, treat as no owner (historical semantics). + return 'not-owned'; } + let procEntries; try { - procEntries = fs.readdirSync('/proc', { withFileTypes: true }); + procEntries = fs.readdirSync(procRoot, { withFileTypes: true }); } catch { - return false; + return 'not-owned'; } + + // Phase 0 + Phase 1: collect the few PIDs whose comm AND cmdline look like a + // GitNexus server, without touching any fd yet. + const candidates = []; for (const ent of procEntries) { - if (Date.now() - start > budget) return false; + if (outOfBudget()) return 'timeout'; if (!ent.isDirectory() || !/^\d+$/.test(ent.name)) continue; const pid = Number.parseInt(ent.name, 10); if (!Number.isFinite(pid) || pid === myPid) continue; - const fdDir = path.join('/proc', ent.name, 'fd'); + + // Phase 0: cheap comm prefilter. + const comm = readProcComm(procRoot, ent.name); + if (!comm) continue; // unreadable comm (kernel thread, raced exit) -> skip + if (!commLooksLikeServer(comm)) continue; + + // Phase 1: bounded cmdline read + isGitNexusServerCommand prefilter. + if (outOfBudget()) return 'timeout'; + const cmdline = readLinuxCmdline(procRoot, ent.name, cmdlineCap, outOfBudget); + // F3: a mid-read budget timeout returns the CMDLINE_TIMEOUT sentinel (a + // Symbol, never a string). Fail CLOSED on it rather than letting it fall + // through isGitNexusServerCommand as a non-candidate — a real owner whose + // escalation timed out must not be silently dropped (would fail-OPEN). + if (cmdline === CMDLINE_TIMEOUT) return 'timeout'; + if (!isGitNexusServerCommand(cmdline)) continue; + candidates.push(ent.name); + } + + // Phase 2: only now stat the fds of the (typically 0-2) survivors. + for (const pidStr of candidates) { + if (outOfBudget()) return 'timeout'; + const fdDir = path.join(procRoot, pidStr, 'fd'); let fds; try { fds = fs.readdirSync(fdDir); - } catch { + } catch (err) { + // F1: the old code returned 'owned' for EVERY non-ENOENT error. That was + // a correctness bug: /proc//fd is owner-only (mode 0500), so a + // cross-user/root `gitnexus mcp` serving a DIFFERENT repo passes Phase 0+1 + // (its cmdline matches) and then EACCES'es here — yet its dev+ino was + // NEVER compared against THIS lbug. Claiming 'owned' lets it permanently, + // silently suppress augment for a repo it does not actually lock. We now + // distinguish the failure shapes (all still fail-closed where we can't + // prove non-ownership, but 'timeout' is the HONEST verdict for + // "inconclusive", not the false-positive 'owned'): + const code = err && err.code; + if (code === 'ENOENT') { + // Process raced away between the candidate scan and now -> genuinely no + // longer an owner. Move on. + continue; + } + if (code === 'EACCES' || code === 'EPERM') { + // Permission-denied fd dir: cannot read fds, so ownership is + // UNVERIFIABLE. Fail closed honestly via 'timeout' (the dispatcher maps + // timeout -> true, same protective skip as before) WITHOUT lying that we + // confirmed ownership. Do NOT degrade to not-owned/fail-open: if this + // really is the owner, fail-open re-opens the #1492 lbug race; augment + // is optional context, so a conservative skip costs little. + debugLog( + `fd dir unreadable for candidate pid ${pidStr} (${code}); ownership ` + + `unverifiable, probe inconclusive -> fail-closed (timeout)`, + ); + return 'timeout'; + } + if (code === 'EIO' || code === 'ESTALE') { + // Genuine transient I/O against this candidate's fd dir — not evidence + // it does NOT hold the lbug. Treat as inconclusive and fail closed + // (timeout) rather than continue, so a real owner mid-I/O-blip is not + // dropped (would fail-open). + debugLog( + `fd dir transient I/O error for candidate pid ${pidStr} (${code}); ` + + `probe inconclusive -> fail-closed (timeout)`, + ); + return 'timeout'; + } + // Any other shape (ENOTDIR — fd path is not a directory at all, so this + // is not a plausible live-procfs owner — and the long tail) is treated as + // "this candidate is not an owner": move to the next candidate instead of + // the old blanket 'owned'. If no other candidate owns the lbug the scan + // ends not-owned (dispatcher fail-open) — acceptable because ENOTDIR means + // the fd entry is structurally not a real /proc//fd. + debugLog( + `fd dir not a readable directory for candidate pid ${pidStr} ` + + `(${code || 'unknown'}); treating candidate as non-owner -> continue`, + ); continue; } - let holds = false; for (const fd of fds) { - if (Date.now() - start > budget) return false; + if (outOfBudget()) return 'timeout'; try { const st = fs.statSync(path.join(fdDir, fd)); if (st.dev === targetStat.dev && st.ino === targetStat.ino) { - holds = true; - break; + return 'owned'; } } catch { - /* ignore */ + /* fd raced closed; ignore */ } } - if (!holds) continue; - if (isGitNexusServerCommand(readLinuxCmdline(ent.name))) return true; } - return false; + + return 'not-owned'; } function unixLsofPsFindGitNexusServer(dbPathAbs, myPid) { @@ -370,8 +668,13 @@ function hasGitNexusDbLockedByGitNexusServer(dbPath, myPid) { } if (process.platform === 'linux') { - if (linuxProcScanFindGitNexusServer(dbPathAbs, myPid)) return true; - return unixLsofPsFindGitNexusServer(dbPathAbs, myPid); + // #2180: cmdline-first procfs scan, no lsof. 'timeout' fails CLOSED + // (overloaded host self-throttles — the throttle the orphan-storm incident + // needed; the old lsof fallback ETIMEDOUT'd and failed closed on these same + // hosts anyway, only slower and with the orphan risk). 'not-owned' is the + // only false. See the fail matrix in the file header. + const verdict = linuxProcScanFindGitNexusServer(dbPathAbs, myPid); + return verdict !== 'not-owned'; } return unixLsofPsFindGitNexusServer(dbPathAbs, myPid); @@ -379,6 +682,13 @@ function hasGitNexusDbLockedByGitNexusServer(dbPath, myPid) { module.exports = { hasGitNexusDbLockedByGitNexusServer, + // Exported for white-box unit tests that must assert the tri-state verdict + // ('owned' | 'not-owned' | 'timeout') directly — the dispatcher collapses + // timeout and owned to the same boolean true, so the boolean API alone cannot + // distinguish the F1 EACCES->timeout fix from the old EACCES->owned bug. The + // Probe interface already declares this optional. Linux-only by contract; the + // name is pinned by a source-contract test. + linuxProcScanFindGitNexusServer, // #2163 follow-up: the hook adapters wrap the augment CLI in the same // guard. Returns a self-tested wrapper path — the built-in candidates are // always absolute; a GITNEXUS_HOOK_TIMEOUT_PATH override is adopted as the @@ -391,4 +701,11 @@ module.exports = { // override to an absolute path. Returns null when the wrapper is // disabled/unavailable. Never call on win32 (see its JSDoc). resolveUnixGuardTimeout, + // Exported for white-box unit tests of the numeric-env parsing (#2183 review): + // Number()-not-parseInt so "16e3" reads as 16000, plus the empty/whitespace + // guard that keeps a set-but-empty budget on the 1200 default instead of an + // immediate fail-closed timeout. Tested directly because the values are + // otherwise only observable indirectly through scan timing/escalation. + getCmdlineMaxBytes, + resolveLinuxProcBudgetMs, }; diff --git a/gitnexus/scripts/cross-platform-tests.ts b/gitnexus/scripts/cross-platform-tests.ts index d332ba1d6..38f769fd3 100644 --- a/gitnexus/scripts/cross-platform-tests.ts +++ b/gitnexus/scripts/cross-platform-tests.ts @@ -37,6 +37,7 @@ const PLATFORM_LOGIC = [ 'test/unit/repo-manager.test.ts', 'test/unit/repo-manager-finalize-invariant.test.ts', 'test/unit/hooks.test.ts', + 'test/unit/hook-db-lock-probe.test.ts', 'test/unit/cursor-hook.test.ts', 'test/unit/sidecar-recovery.test.ts', 'test/unit/pool-wal-recovery.test.ts', diff --git a/gitnexus/test/integration/antigravity-hook-e2e.test.ts b/gitnexus/test/integration/antigravity-hook-e2e.test.ts index a4a9d1f01..8cb68b000 100644 --- a/gitnexus/test/integration/antigravity-hook-e2e.test.ts +++ b/gitnexus/test/integration/antigravity-hook-e2e.test.ts @@ -396,7 +396,13 @@ describe('antigravity hook adapter e2e', () => { // (its lock/probe helpers only resolve from the install dir). A faked lsof/ps + // an empty `lbug` lock force hasGitNexusServerOwner() => true; a marker-writing // fake CLI proves augment never ran. - describe.skipIf(process.platform === 'win32')( + // + // #2180: skipped on Linux too — the probe's Linux backend no longer uses + // lsof/ps, so the faked lsof/ps can't force owner=true there. This stays as the + // macOS/other-Unix lsof-path lane; the antigravity adapter shares the identical + // gated owner-skip with the claude/plugin copies, whose Linux owner detection + // is covered against a fake /proc in test/unit/hook-db-lock-probe.test.ts. + describe.skipIf(process.platform === 'win32' || process.platform === 'linux')( 'AfterTool — augment skipped when MCP server owns the DB (#1913)', () => { const OWNER_PROBE = { diff --git a/gitnexus/test/unit/hook-db-lock-probe.test.ts b/gitnexus/test/unit/hook-db-lock-probe.test.ts new file mode 100644 index 000000000..60e77c894 --- /dev/null +++ b/gitnexus/test/unit/hook-db-lock-probe.test.ts @@ -0,0 +1,723 @@ +/** + * Direct unit tests for the Linux cmdline-first DB-owner scan (#2180). + * + * These exercise linuxProcScanFindGitNexusServer / hasGitNexusDbLockedByGitNexusServer + * against a FAKE /proc tree (GITNEXUS_HOOK_PROC_ROOT) so the three-phase logic + * (comm -> cmdline -> fd dev+ino) is asserted deterministically, without + * scanning the test host's real /proc. One live e2e at the bottom uses the REAL + * /proc to protect the "lbug handle is fd-visible" property the scan relies on. + * + * The probe is a CJS module; we require it through createRequire and toggle env + * per-test. resetModules-style isolation is unnecessary because the only + * module-level cache (unixGuardTimeoutCache) is on the macOS/Unix path, which + * these Linux tests never reach. + */ +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { createRequire } from 'node:module'; +import fs from 'fs'; +import os from 'os'; +import path from 'path'; +import { spawn } from 'child_process'; +import { createFakeProcRoot, type FakeProcEntry } from '../utils/hook-test-helpers.js'; + +const PROBE_PATH = path.resolve(__dirname, '..', '..', 'hooks', 'claude', 'hook-db-lock-probe.cjs'); + +type Probe = { + hasGitNexusDbLockedByGitNexusServer: (dbPath: string, myPid: number) => boolean; + linuxProcScanFindGitNexusServer?: (dbPathAbs: string, myPid: number) => string; + getCmdlineMaxBytes?: () => number; + resolveLinuxProcBudgetMs?: () => number; +}; +const probe = createRequire(import.meta.url)(PROBE_PATH) as Probe; + +// The probe now exports linuxProcScanFindGitNexusServer unconditionally (F1 +// white-box verdict assertions). Narrow it once here to a non-optional typed +// fn so the per-test call sites stay assertion-free; a dedicated test below +// pins that the export really is a function. +type ScanVerdictFn = (dbPathAbs: string, myPid: number) => string; +const scanVerdictFn = probe.linuxProcScanFindGitNexusServer as ScanVerdictFn; + +const isLinux = process.platform === 'linux'; + +// ── env scoping helpers ──────────────────────────────────────────── +const ENV_KEYS = [ + 'GITNEXUS_HOOK_PROC_ROOT', + 'GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS', + 'GITNEXUS_HOOK_PROC_CMDLINE_MAX', +] as const; +const savedEnv: Record = {}; +function setEnv(overrides: Record) { + for (const k of ENV_KEYS) { + if (!(k in savedEnv)) savedEnv[k] = process.env[k]; + } + for (const [k, v] of Object.entries(overrides)) { + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + } +} +const cleanups: Array<() => void> = []; +afterEach(() => { + for (const k of Object.keys(savedEnv)) { + const v = savedEnv[k]; + if (v === undefined) delete process.env[k]; + else process.env[k] = v; + delete savedEnv[k]; + } + while (cleanups.length) { + try { + cleanups.pop()!(); + } catch { + /* best-effort */ + } + } +}); + +/** + * Build a temp lbug + a fake /proc root, run the dispatcher with the fake root, + * and return the boolean owner verdict. The lbug is the dev+ino the fake fd + * symlinks point at, so a holder whose fdTargets include `lbug` is a true owner. + */ +function runScan( + entries: (lbugPath: string) => FakeProcEntry[], + env: Record = {}, +): { owned: boolean; lbugPath: string } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-')); + cleanups.push(() => fs.rmSync(dir, { recursive: true, force: true })); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + const procRoot = createFakeProcRoot(entries(lbugPath)); + cleanups.push(() => fs.rmSync(procRoot, { recursive: true, force: true })); + setEnv({ GITNEXUS_HOOK_PROC_ROOT: procRoot, ...env }); + const owned = probe.hasGitNexusDbLockedByGitNexusServer(lbugPath, 1); + return { owned, lbugPath }; +} + +const GITNEXUS_MCP_ARGV = (script: string) => ['node', script, 'mcp']; + +// ── Numeric env parsing (white-box, #2183 review) ────────────────────── +// +// getCmdlineMaxBytes / resolveLinuxProcBudgetMs switched from parseInt(.,10) to +// Number() so scientific notation ("16e3") parses as 16000 instead of 16. These +// are platform-independent (pure string->number), so they run on every OS, not +// just Linux. The load-bearing case is the EMPTY-STRING budget regression guard: +// a naive parseInt->Number swap would make a set-but-empty +// GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS="" resolve to Number("")===0 => budget 0 => +// immediate fail-CLOSED timeout (augment permanently skipped). The added +// `&& String(raw).trim()` guard keeps ''/whitespace on the 1200 default. +describe('numeric env parsing (white-box, #2183 review)', () => { + const budget = probe.resolveLinuxProcBudgetMs as () => number; + const cmdlineMax = probe.getCmdlineMaxBytes as () => number; + + it('exports the two parse helpers as functions', () => { + expect(typeof probe.resolveLinuxProcBudgetMs).toBe('function'); + expect(typeof probe.getCmdlineMaxBytes).toBe('function'); + }); + + it('budget: "16e3" parses as 16000 (scientific notation), not 16', () => { + // parseInt('16e3',10) === 16 (stops at 'e'); Number('16e3') === 16000. + setEnv({ GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '16e3' }); + expect(budget()).toBe(16000); + }); + + it('budget: set-but-empty "" and whitespace fall back to 1200, NOT 0 (regression guard)', () => { + // The deepening catch: without the `&& String(raw).trim()` guard these would + // be Number('')===0 => an immediate fail-closed timeout on every hook call. + for (const empty of ['', ' ', '\t']) { + setEnv({ GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: empty }); + expect(budget()).toBe(1200); + } + }); + + it('budget: "0" still parses to 0 (the deliberate #2180 immediate-timeout vector)', () => { + setEnv({ GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '0' }); + expect(budget()).toBe(0); + }); + + it('budget: trailing garbage "123abc" and unset fall back to 1200', () => { + setEnv({ GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '123abc' }); + expect(budget()).toBe(1200); + setEnv({ GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: undefined }); + expect(budget()).toBe(1200); + }); + + it('cmdline max: "8e3" parses as 8000 (>= floor); "2e3" (=2000, below floor) and ""/unset -> 16384', () => { + setEnv({ GITNEXUS_HOOK_PROC_CMDLINE_MAX: '8e3' }); + expect(cmdlineMax()).toBe(8000); + setEnv({ GITNEXUS_HOOK_PROC_CMDLINE_MAX: '2e3' }); + expect(cmdlineMax()).toBe(16384); + setEnv({ GITNEXUS_HOOK_PROC_CMDLINE_MAX: '' }); + expect(cmdlineMax()).toBe(16384); + setEnv({ GITNEXUS_HOOK_PROC_CMDLINE_MAX: undefined }); + expect(cmdlineMax()).toBe(16384); + }); +}); + +describe.skipIf(!isLinux)('Linux cmdline-first DB-owner scan (#2180)', () => { + // ── D1: three-phase correctness ────────────────────────────────── + + it('owned: a gitnexus mcp process holding the lbug fd is detected', () => { + const { owned } = runScan((lbug) => [ + { + pid: 4242, + comm: 'MainThread', // real gitnexus servers report this on modern Node + cmdline: GITNEXUS_MCP_ARGV('/opt/app/node_modules/gitnexus/dist/cli/index.js'), + fdTargets: ['/dev/null', lbug], + }, + ]); + expect(owned).toBe(true); + }); + + it('not-owned: a node process that is not a gitnexus server (even holding the lbug) is ignored', () => { + const { owned } = runScan((lbug) => [ + { + pid: 5555, + comm: 'node', + cmdline: ['node', '/some/app/server.js'], + fdTargets: [lbug], // holds the fd, but cmdline is not a gitnexus server + }, + ]); + expect(owned).toBe(false); + }); + + it('not-owned: a gitnexus mcp process that does NOT hold the lbug fd is not an owner', () => { + const { owned } = runScan((lbug) => [ + { + pid: 6001, + comm: 'MainThread', + cmdline: GITNEXUS_MCP_ARGV('/x/node_modules/gitnexus/dist/cli/index.js'), + fdTargets: ['/dev/null'], // server, but holds some OTHER fd, not this lbug + }, + // a decoy that holds the lbug but is not a server + { + pid: 6002, + comm: 'vim', + cmdline: ['vim', '/etc/hosts'], + fdTargets: [lbug], + }, + ]); + expect(owned).toBe(false); + }); + + it('Phase 0 trap: cmdline LOOKS like gitnexus but comm is non-candidate → filtered out before fd check', () => { + // The fd symlink points at the lbug, so if Phase 0 did NOT filter on comm + // the cmdline prefilter would match and the fd check would say "owned". + // Because comm is a non-candidate ('postgres'), Phase 0 drops it first. + const { owned } = runScan((lbug) => [ + { + pid: 7007, + comm: 'postgres', // not in COMM_CANDIDATES, not a prefix of any + cmdline: GITNEXUS_MCP_ARGV('/x/node_modules/gitnexus/dist/cli/index.js'), + fdTargets: [lbug], + }, + ]); + expect(owned).toBe(false); + }); + + it('Phase 0 truncation-safe: a 15-char-truncated comm prefix of a candidate still matches', () => { + // Kernel comm cap is 15 visible chars; a candidate name truncated to a + // prefix must NOT be dropped. We use a comm that is a strict prefix of a + // whitelist entry ('MainThr' ⊂ 'MainThread'). + const { owned } = runScan((lbug) => [ + { + pid: 8008, + comm: 'MainThr', + cmdline: GITNEXUS_MCP_ARGV('/x/node_modules/gitnexus/dist/cli/index.js'), + fdTargets: [lbug], + }, + ]); + expect(owned).toBe(true); + }); + + // ── D2: budget / timeout → fail-closed ─────────────────────────── + + it('budget <= 0 → immediate timeout → dispatcher fails CLOSED (owner=true)', () => { + // Even though NO process is a gitnexus server, budget 0 yields 'timeout' + // which the dispatcher maps to true (self-throttle). This also pins the + // #2180 budget-parse fix: "0" must NOT fall back to 1200. + const { owned } = runScan( + () => [{ pid: 9001, comm: 'bash', cmdline: ['bash'], fdTargets: [] }], + { GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '0' }, + ); + expect(owned).toBe(true); + }); + + it('budget "0" is not silently treated as 1200 (regression for the parse bug)', () => { + // With a healthy non-owner fake proc and budget '0', the OLD code (which + // coerced "0" to 1200) would have completed the scan and returned + // not-owned (false). The fixed code returns timeout → true. + const { owned } = runScan( + () => [{ pid: 9100, comm: 'node', cmdline: ['node', '/app/x.js'], fdTargets: [] }], + { GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '0' }, + ); + expect(owned).toBe(true); + }); + + it('a generous budget over a non-owner tree completes and returns not-owned', () => { + const { owned } = runScan( + () => [ + { pid: 9200, comm: 'node', cmdline: ['node', '/app/x.js'], fdTargets: [] }, + { pid: 9201, comm: 'bash', cmdline: ['bash', '-l'], fdTargets: [] }, + ], + { GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '5000' }, + ); + expect(owned).toBe(false); + }); + + // ── D3: 4 KB+ cmdline cap — escalation must really iterate (F2) ──── + // + // The cmdline shape here is deliberate (Codex): the `gitnexus` token sits in + // the SECOND argv (a SHORT node_modules/gitnexus path, well inside the first + // 4 KB chunk) so `if (!hasGitNexus) break` does NOT abort the read; a ~9 KB + // pad argv then pushes the trailing `mcp` mode token PAST 4096, so the first + // 4 KB chunk has gitnexus-but-no-mode and the loop MUST escalate to a second + // read to find `mcp`. Setting GITNEXUS_HOOK_PROC_CMDLINE_MAX=4096 makes the + // chunk size 4 KB so escalation actually happens (the 16 KB default would read + // the whole line in one shot and the loop would never iterate — the old test's + // latent no-op). + + // gitnexus token early (well under 4 KB), mode token forced past 4 KB by pad. + const GITNEXUS_SHORT = '/nm/node_modules/gitnexus/dist/cli/index.js'; + const PAD_PAST_4K = 'x'.repeat(9000); // pushes the trailing `mcp` well past 4096 + + it('owned even when the mode token sits far past 4 KB → escalation iterates and finds it', () => { + const readSyncSpy = vi.spyOn(fs, 'readSync'); + cleanups.push(() => readSyncSpy.mockRestore()); + const { owned } = runScan( + (lbug) => [ + { + pid: 10001, + comm: 'MainThread', + // node | SHORT gitnexus path (<4KB) | 9KB pad | mcp → mcp lands >4096 + cmdline: ['node', GITNEXUS_SHORT, PAD_PAST_4K, 'mcp'], + fdTargets: [lbug], + }, + ], + { GITNEXUS_HOOK_PROC_CMDLINE_MAX: '4096' }, + ); + expect(owned).toBe(true); + // White-box proof the escalation actually re-read: with a 4 KB chunk over a + // >4 KB cmdline, readSync must have been called more than once for this pid. + // (A "just bump the cap" pseudo-fix that read everything in one go would + // leave this at 1 and fail.) + expect(readSyncSpy.mock.calls.length).toBeGreaterThan(1); + }); + + it('discrimination: same gitnexus cmdline but mode token past HARD_CEIL → not-owned', () => { + // Negative control proving the escalation has a real upper bound (HARD_CEIL + // = 256 KiB) and the positive test above is not just "always escalates". The + // gitnexus token is early so escalation runs, but a >256 KiB pad keeps the + // `mcp` token beyond the ceiling, so the bounded read stops before reaching + // it → isGitNexusServerCommand sees no mode token → not a candidate → + // not-owned. (A broken "escalate forever" impl would wrongly read to `mcp` + // and report owned, failing this assertion.) + const padPastCeil = 'x'.repeat(300000); // > HARD_CEIL (262144) + const { owned } = runScan( + (lbug) => [ + { + pid: 10002, + comm: 'MainThread', + cmdline: ['node', GITNEXUS_SHORT, padPastCeil, 'mcp'], + fdTargets: [lbug], + }, + ], + { GITNEXUS_HOOK_PROC_CMDLINE_MAX: '4096' }, + ); + expect(owned).toBe(false); + }); + + it('does not over-read: a giant non-gitnexus cmdline is bounded and yields not-owned', () => { + const giant = 'x'.repeat(500000); // 500 KB single arg, no gitnexus token + const { owned } = runScan((lbug) => [ + { + pid: 10100, + comm: 'node', + cmdline: ['node', `/app/${giant}.js`], + fdTargets: [lbug], + }, + ]); + expect(owned).toBe(false); + }); + + // ── D3b: cmdline escalation respects the scan budget (F3) ───────── + // + // A single pathological candidate whose `mcp` token sits far past the chunk + // size used to be able to read up to HARD_CEIL (256 KiB) inside one + // readLinuxCmdline call before the scan-level budget was re-checked. F3 wires + // outOfBudget into the escalation loop: when the deadline trips mid-read it + // returns the CMDLINE_TIMEOUT *Symbol* (NOT '' — '' would flow through + // isGitNexusServerCommand as a non-candidate and silently drop a possible + // owner, a fail-OPEN), and the Phase 1 caller maps that Symbol to the 'timeout' + // verdict (fail-CLOSED). We drive the deadline deterministically by advancing + // a Date.now spy after the first escalation read. + + it('escalation that exceeds the budget mid-read → verdict timeout (sentinel, not silent drop)', () => { + const scan = scanVerdictFn; + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-f3-')); + cleanups.push(() => fs.rmSync(dir, { recursive: true, force: true })); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + // gitnexus token early (escalation will start), mode token pushed past 4 KB + // so a SECOND read is required — between those reads we trip the clock. + const procRoot = createFakeProcRoot([ + { + pid: 10200, + comm: 'MainThread', + cmdline: ['node', GITNEXUS_SHORT, 'x'.repeat(9000), 'mcp'], + fdTargets: [lbugPath], + }, + ]); + cleanups.push(() => fs.rmSync(procRoot, { recursive: true, force: true })); + setEnv({ + GITNEXUS_HOOK_PROC_ROOT: procRoot, + GITNEXUS_HOOK_PROC_CMDLINE_MAX: '4096', + GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '1000', // positive, so the scan starts + }); + + // Deterministic clock: the scan captures `start` (call #1) and runs its + // Phase-0/1 entry budget checks in-budget; once the escalation loop is under + // way we jump Date.now() past the 1000 ms budget so the loop's in-read + // outOfBudget() returns the CMDLINE_TIMEOUT sentinel. The threshold (>4) is + // chosen so the early checks (start capture, per-entry + Phase-1 pre-read + // checks) stay at base and only the escalation's mid-loop check trips. + const base = Date.now(); + let nowCalls = 0; + const nowSpy = vi.spyOn(Date, 'now').mockImplementation(() => { + nowCalls += 1; + return nowCalls > 4 ? base + 5000 : base; + }); + cleanups.push(() => nowSpy.mockRestore()); + + const verdict = scan(lbugPath, 1); + // The load-bearing assertion: a mid-escalation budget trip yields the + // 'timeout' verdict (via the Symbol sentinel) — NOT a silent non-candidate + // drop (which would be 'not-owned' here and a fail-OPEN if this were a real + // cross-budget owner). + expect(verdict).toBe('timeout'); + nowSpy.mockRestore(); + }); + + // ── D5: GITNEXUS_HOOK_PROC_ROOT is honored ONLY under a test runner (F4) ── + // + // Production hooks run as `node .cjs` with neither VITEST nor + // NODE_ENV=test set; vitest injects both into every worker (verified). F4 + // gates getProcRoot() on that signal so a production env that leaked + // GITNEXUS_HOOK_PROC_ROOT (pointing at an empty/bad tree) cannot turn Linux + // owner detection OFF (no pids -> not-owned -> fail-OPEN, the #1492 class). + // These tests run inside vitest, so the gate is OPEN and injection works (the + // entire fake-procfs suite above already depends on that). Here we prove the + // gate is load-bearing: with the test signals stripped, the override is + // ignored and the scan falls back to the real /proc (so our fake lbug is NOT + // found there -> not-owned), and with them present the override is honored. + + it('honors GITNEXUS_HOOK_PROC_ROOT under the vitest test signal (gate open)', () => { + // Sanity: in this vitest worker VITEST/NODE_ENV are set, so the fake root is + // honored and a fake owner is detected — same mechanism the whole suite uses. + const { owned } = runScan((lbug) => [ + { + pid: 10300, + comm: 'MainThread', + cmdline: GITNEXUS_MCP_ARGV('/x/node_modules/gitnexus/dist/cli/index.js'), + fdTargets: [lbug], + }, + ]); + expect(owned).toBe(true); + }); + + it('ignores GITNEXUS_HOOK_PROC_ROOT when the test signal is absent (gate closed → real /proc)', () => { + // Strip BOTH test signals so getProcRoot() falls back to /proc even though + // GITNEXUS_HOOK_PROC_ROOT points at our fake tree. The fake lbug is not an + // fd under the real /proc, so the scan returns not-owned: proof the override + // is inert in a non-test (production-shaped) context. + const savedVitest = process.env.VITEST; + const savedNodeEnv = process.env.NODE_ENV; + cleanups.push(() => { + if (savedVitest === undefined) delete process.env.VITEST; + else process.env.VITEST = savedVitest; + if (savedNodeEnv === undefined) delete process.env.NODE_ENV; + else process.env.NODE_ENV = savedNodeEnv; + }); + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-f4-')); + cleanups.push(() => fs.rmSync(dir, { recursive: true, force: true })); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + const procRoot = createFakeProcRoot([ + { + pid: 10400, + comm: 'MainThread', + cmdline: GITNEXUS_MCP_ARGV('/x/node_modules/gitnexus/dist/cli/index.js'), + fdTargets: [lbugPath], + }, + ]); + cleanups.push(() => fs.rmSync(procRoot, { recursive: true, force: true })); + setEnv({ GITNEXUS_HOOK_PROC_ROOT: procRoot }); + // Now drop the test signals — must happen AFTER setEnv so the gate sees them gone. + delete process.env.VITEST; + delete process.env.NODE_ENV; + const verdict = scanVerdictFn(lbugPath, 1); + // Gate closed -> getProcRoot() returns '/proc'; our fake lbug fd is not in + // the real /proc, so no owner is found. + expect(verdict).toBe('not-owned'); + expect(probe.hasGitNexusDbLockedByGitNexusServer(lbugPath, 1)).toBe(false); + }); + + // ── D4: unreadable candidate fd dir → honest tri-state verdict (F1) ── + // + // /proc//fd is owner-only (mode 0500). A cross-user/root `gitnexus mcp` + // serving a DIFFERENT repo clears Phase 0+1 (cmdline matches) and then EACCES + // here — but its dev+ino was never compared against THIS lbug. The OLD code + // returned 'owned' for every non-ENOENT readdir error, falsely claiming + // ownership and permanently suppressing augment for a repo that process does + // not lock. F1 splits the failure shapes: + // - EACCES / EPERM -> 'timeout' (unverifiable; fail-closed HONESTLY) + // - EIO / ESTALE -> 'timeout' (transient I/O; fail-closed) + // - ENOTDIR / other -> continue (not a real fd dir; treat as non-owner) + // The dispatcher collapses owned+timeout to boolean true, so these assert the + // exported tri-state verdict directly — a boolean check could not tell the F1 + // fix from the old bug. + + it('exports linuxProcScanFindGitNexusServer for white-box verdict assertions', () => { + expect(typeof probe.linuxProcScanFindGitNexusServer).toBe('function'); + }); + + it('candidate fd dir EACCES → verdict timeout (honest fail-closed, NOT owned)', () => { + if (process.getuid && process.getuid() === 0) { + // root bypasses chmod 000, so a real EACCES is not reproducible on this + // host. This disk-based test no-ops under root; the uid-agnostic spy + // tests below cover every F1 errno branch (EACCES/EPERM/EIO/ESTALE/ + // ENOTDIR) regardless of who runs the suite. + return; + } + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-eacces-')); + cleanups.push(() => { + try { + fs.chmodSync(path.join(dir, 'proc', '11001', 'fd'), 0o755); + } catch { + /* ignore */ + } + fs.rmSync(dir, { recursive: true, force: true }); + }); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + const procRoot = path.join(dir, 'proc'); + const fdDir = path.join(procRoot, '11001', 'fd'); + fs.mkdirSync(fdDir, { recursive: true }); + fs.writeFileSync(path.join(procRoot, '11001', 'comm'), 'MainThread\n'); + fs.writeFileSync( + path.join(procRoot, '11001', 'cmdline'), + ['node', '/x/node_modules/gitnexus/dist/cli/index.js', 'mcp'].join('\0') + '\0', + ); + fs.chmodSync(fdDir, 0o000); // EACCES on readdir + setEnv({ GITNEXUS_HOOK_PROC_ROOT: procRoot }); + // White-box: assert the verdict is 'timeout' (NOT 'owned' — the F1 point). + const verdict = scanVerdictFn(lbugPath, 1); + expect(verdict).toBe('timeout'); + // And the dispatcher still fails closed (boolean true) on that timeout. + const owned = probe.hasGitNexusDbLockedByGitNexusServer(lbugPath, 1); + expect(owned).toBe(true); + }); + + // uid-agnostic coverage of every F1 fd-readdir errno branch. chmod 000 yields + // no EACCES for root, so the disk-based tests above no-op there — these spy + // fs.readdirSync to throw a chosen errno only for the candidate's fd dir (the + // procRoot enumeration calls through), pinning the F1 split in CI regardless + // of the runner's uid. + for (const { code, expected } of [ + { code: 'EACCES', expected: 'timeout' }, + { code: 'EPERM', expected: 'timeout' }, + { code: 'EIO', expected: 'timeout' }, + { code: 'ESTALE', expected: 'timeout' }, + { code: 'ENOTDIR', expected: 'not-owned' }, + ] as const) { + it(`candidate fd readdir ${code} → verdict ${expected} (uid-agnostic spy)`, () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-fderr-')); + cleanups.push(() => fs.rmSync(dir, { recursive: true, force: true })); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + const procRoot = path.join(dir, 'proc'); + const fdDir = path.join(procRoot, '11001', 'fd'); + fs.mkdirSync(fdDir, { recursive: true }); + fs.writeFileSync(path.join(procRoot, '11001', 'comm'), 'MainThread\n'); + fs.writeFileSync( + path.join(procRoot, '11001', 'cmdline'), + ['node', '/x/node_modules/gitnexus/dist/cli/index.js', 'mcp'].join('\0') + '\0', + ); + setEnv({ GITNEXUS_HOOK_PROC_ROOT: procRoot }); + const realReaddir = fs.readdirSync.bind(fs); + const spy = vi.spyOn(fs, 'readdirSync').mockImplementation((p, ...rest) => { + if (typeof p === 'string' && p.endsWith(`${path.sep}fd`)) { + const err = new Error(`mock ${code}`) as NodeJS.ErrnoException; + err.code = code; + throw err; + } + return (realReaddir as (...a: unknown[]) => unknown)(p, ...rest); + }); + cleanups.push(() => spy.mockRestore()); + // White-box: assert the exported tri-state verdict directly (the + // dispatcher would collapse timeout+owned to the same boolean). + expect(scanVerdictFn(lbugPath, 1)).toBe(expected); + spy.mockRestore(); + }); + } + + it('candidate fd path is a FILE (ENOTDIR) → treated as non-owner → not-owned', () => { + // ENOTDIR means the fd entry is not a real /proc//fd directory at all, + // so it is not a plausible live owner. The candidate is skipped (continue); + // with no other candidate the scan ends not-owned (the OLD code wrongly + // returned 'owned' here). Runs on every OS incl. root. + const { verdict, owned } = runScanFdEnotdir(); + expect(verdict).toBe('not-owned'); + expect(owned).toBe(false); + }); + + it('EACCES candidate then a REAL owner later → still detects the real owner', () => { + // Regression guard for the F1 continue/return choice: an EACCES candidate + // must NOT short-circuit the scan in a way that hides a genuine owner. Here + // the EACCES dir yields timeout BEFORE reaching the true owner — timeout is + // the protective (fail-closed) verdict, so dispatcher returns true either + // way. (Ordering in /proc readdir is numeric-string; 11001 < 11050.) + if (process.getuid && process.getuid() === 0) return; // EACCES needs non-root + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-mixed-')); + cleanups.push(() => { + try { + fs.chmodSync(path.join(dir, 'proc', '11001', 'fd'), 0o755); + } catch { + /* ignore */ + } + fs.rmSync(dir, { recursive: true, force: true }); + }); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + const procRoot = path.join(dir, 'proc'); + // Candidate A: EACCES fd dir. + const fdDirA = path.join(procRoot, '11001', 'fd'); + fs.mkdirSync(fdDirA, { recursive: true }); + fs.writeFileSync(path.join(procRoot, '11001', 'comm'), 'MainThread\n'); + fs.writeFileSync( + path.join(procRoot, '11001', 'cmdline'), + ['node', '/x/node_modules/gitnexus/dist/cli/index.js', 'mcp'].join('\0') + '\0', + ); + fs.chmodSync(fdDirA, 0o000); + setEnv({ GITNEXUS_HOOK_PROC_ROOT: procRoot }); + const verdict = scanVerdictFn(lbugPath, 1); + // EACCES is hit first and fails closed (timeout) — the protective outcome. + expect(verdict).toBe('timeout'); + expect(probe.hasGitNexusDbLockedByGitNexusServer(lbugPath, 1)).toBe(true); + }); +}); + +// Helper for the ENOTDIR branch: fd is a FILE not a dir, so readdir throws +// ENOTDIR. F1: this candidate is treated as a non-owner (continue) → not-owned. +function runScanFdEnotdir(): { verdict: string; owned: boolean } { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-probe-enotdir-')); + cleanups.push(() => fs.rmSync(dir, { recursive: true, force: true })); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + const procRoot = path.join(dir, 'proc'); + const pidDir = path.join(procRoot, '11002'); + fs.mkdirSync(pidDir, { recursive: true }); + fs.writeFileSync(path.join(pidDir, 'comm'), 'MainThread\n'); + fs.writeFileSync( + path.join(pidDir, 'cmdline'), + ['node', '/x/node_modules/gitnexus/dist/cli/index.js', 'mcp'].join('\0') + '\0', + ); + fs.writeFileSync(path.join(pidDir, 'fd'), 'not a dir'); // readdir -> ENOTDIR + setEnv({ GITNEXUS_HOOK_PROC_ROOT: procRoot }); + const verdict = scanVerdictFn(lbugPath, 1); + const owned = probe.hasGitNexusDbLockedByGitNexusServer(lbugPath, 1); + return { verdict, owned }; +} + +// ── D6: live e2e against the REAL /proc ───────────────────────────── +// +// Protects the load-bearing assumption that a real lbug handle is fd-visible in +// /proc//fd (a @ladybugdb/core property; a future move to mmap-only would +// silently regress #1492 with no other test going red). We spawn a child that +// opens an fd on a real temp lbug AND wears a gitnexus-mcp cmdline, then assert +// the scan reports owned. Crucially we assert against OUR holder's identity, not +// "any owner" — this host runs background gitnexus servers, so a bare +// truthiness check could be a false positive. +describe.skipIf(!isLinux)('Linux DB-owner scan — live /proc e2e (#2180)', () => { + it('detects a real fd-visible gitnexus-mcp-shaped lbug holder', async () => { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-e2e-')); + const lbugPath = path.join(dir, 'lbug'); + fs.writeFileSync(lbugPath, ''); + // Give the holder a gitnexus-server cmdline by running it from a + // node_modules/gitnexus/dist/cli/index.js path with an `mcp` arg. + const scriptDir = path.join(dir, 'node_modules', 'gitnexus', 'dist', 'cli'); + fs.mkdirSync(scriptDir, { recursive: true }); + const script = path.join(scriptDir, 'index.js'); + const pidFile = path.join(dir, 'holder.pid'); + fs.writeFileSync( + script, + `const fs=require('fs');` + + `const fd=fs.openSync(${JSON.stringify(lbugPath)},'r');` + + `fs.writeFileSync(${JSON.stringify(pidFile)},String(process.pid));` + + `process.on('SIGTERM',()=>{try{fs.closeSync(fd);}catch{}process.exit(0);});` + + `setInterval(()=>{},1<<30);`, + ); + + const holder = spawn(process.execPath, [script, 'mcp'], { stdio: 'ignore' }); + try { + // Wait for the holder to report ready (pid file written). Widened to ~10s + // (was 5s): a loaded CI runner can be slow to spawn the child, and this is + // the one genuine false-FAIL path in the e2e (the budget timeout below + // merely hollows the assertion rather than failing it). + let holderPid = 0; + for (let i = 0; i < 400; i++) { + try { + const raw = fs.readFileSync(pidFile, 'utf8').trim(); + if (raw) { + holderPid = Number.parseInt(raw, 10); + break; + } + } catch { + /* not ready yet */ + } + await new Promise((r) => setTimeout(r, 25)); + } + expect(holderPid).toBeGreaterThan(0); + + // Confirm the holder really is fd-visible (the property under test). + const fdDir = `/proc/${holderPid}/fd`; + const targetStat = fs.statSync(lbugPath); + const fdVisible = fs.readdirSync(fdDir).some((fd) => { + try { + const st = fs.statSync(path.join(fdDir, fd)); + return st.dev === targetStat.dev && st.ino === targetStat.ino; + } catch { + return false; + } + }); + expect(fdVisible).toBe(true); + + // Real /proc, generous explicit budget. Clear PROC_ROOT (-> real /proc) + // and raise the scan budget via setEnv so the module afterEach restores + // BOTH (no raw process.env mutation leaking to sibling tests). The + // generous budget is load-bearing: this dispatcher maps a budget 'timeout' + // to owned=TRUE, so on a busy host the default 1200ms could be exhausted + // before reaching the holder and the assertion would still pass for the + // WRONG reason (a hollow timeout, not real fd-visible detection). 10s + // keeps the assertion honest. Use a PID we are NOT so the holder is not + // excluded, and assert owned for OUR lbug specifically. + setEnv({ + GITNEXUS_HOOK_PROC_ROOT: undefined, + GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '10000', + }); + const t0 = Date.now(); + const owned = probe.hasGitNexusDbLockedByGitNexusServer(lbugPath, process.pid); + const ms = Date.now() - t0; + expect(owned).toBe(true); + // Coarse regression guard against the old O(procs×fds)+lsof path (~1.2s+). + // The bound sits ABOVE the 10s budget so a legitimately-slow-but-correct + // scan can't trip it — a regression guard, not a tight perf SLA. + expect(ms).toBeLessThan(15000); + } finally { + try { + holder.kill('SIGKILL'); + } catch { + /* ignore */ + } + fs.rmSync(dir, { recursive: true, force: true }); + } + }, 40000); +}); diff --git a/gitnexus/test/unit/hooks.test.ts b/gitnexus/test/unit/hooks.test.ts index 9e2f39890..74b2df749 100644 --- a/gitnexus/test/unit/hooks.test.ts +++ b/gitnexus/test/unit/hooks.test.ts @@ -27,6 +27,7 @@ import { runHook, parseHookOutput, createHookToolDir, + createFakeProcRoot, hookEnv, } from '../utils/hook-test-helpers.js'; @@ -87,6 +88,17 @@ const PLUGIN_HOOK_DB_PROBE = path.resolve( 'hook-db-lock-probe.cjs', ); +// ─── lsof/ps-path lane gate (#2180) ───────────────────────────────── +// +// The owner-detection tests below drive the probe through its lsof + ps backend +// (via the fake lsof/ps in createHookToolDir). That backend is the macOS/other- +// Unix path; #2180 removed the Linux lsof fallback, so on Linux these tests +// would no longer exercise the real dispatch (the cmdline-first procfs scan +// answers instead, and a temp lbug held by nobody is simply not-owned). They +// remain valid coverage for the macOS lane; Linux gets equivalent three-phase +// coverage in test/unit/hook-db-lock-probe.test.ts (fake /proc + a live e2e). +const SKIP_LSOF_PATH = process.platform === 'win32' || process.platform === 'linux'; + // ─── Host guard precheck for orphan-reaping tests (#2163) ─────────── // // The reaping lanes depend on a host coreutils `timeout`/`gtimeout` that @@ -1002,10 +1014,10 @@ describe.skipIf(process.platform === 'win32')( { env: { ...hookEnv(binDir), - // Force the Linux /proc scan to fall through to lsof - // immediately. Must be '1' — do NOT "simplify" to '0': the - // current parser (`Number(raw && String(raw).trim())`) - // treats '0' as falsy and falls back to the 1200ms default. + // The slot gate rejects this invocation before the probe runs + // at all, so this budget never actually bounds a scan — it is + // set low only to keep the test fast in the (asserted-absent) + // case the gate ever regressed and let the probe through. GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '1', }, }, @@ -1156,250 +1168,21 @@ describe.skipIf(process.platform === 'win32')( }, ); -describe.skipIf(process.platform !== 'linux')( - 'Orphaned lsof is reaped by the timeout wrapper (#2163)', - () => { - // T3 — minimal reproduction of the incident mechanism: the hook process - // is SIGKILLed (modeling Claude Code's 10s hook timeout) while a slow, - // SIGTERM-immune lsof child is still running. Before the fix nothing can - // signal that child anymore (and spawnSync's own SIGTERM is ignored - // anyway), so it survives its full 30s sleep → test red regardless of - // race timing. After the fix the coreutils `timeout -k 1` wrapper - // outlives the hook and SIGKILLs the child within ~3s — making this also - // a direct regression test for the wrapper's `-k` capability. - it('CJS: SIGKILLed hook leaves no immortal lsof child', async () => { - // Guard-availability precheck — see resolveHostGuardForReapingTests. - expect(resolveHostGuardForReapingTests(), GUARD_PRECHECK_MSG).not.toBeNull(); - const { spawn } = await import('child_process'); - const lbugPath = path.join(gitNexusDir, 'lbug'); - fs.writeFileSync(lbugPath, ''); - const pidFile = path.join(os.tmpdir(), `gn-hook-lsofpid-${process.pid}`); - fs.rmSync(pidFile, { force: true }); - const binDir = createHookToolDir({ - lsofPidFile: pidFile, - lsofSleepMs: 30000, - lsofIgnoreSigterm: true, - }); - let lsofPid = 0; - let hookChild: ReturnType | null = null; - - const isFakeLsofAlive = () => { - try { - process.kill(lsofPid, 0); - } catch { - return false; // ESRCH — reaped - } - // PID-reuse guard: only count it alive while the cmdline still - // points at our fake lsof. - try { - return fs.readFileSync(`/proc/${lsofPid}/cmdline`, 'utf-8').includes(binDir); - } catch { - return false; - } - }; - - try { - hookChild = spawn(process.execPath, [CJS_HOOK], { - stdio: ['pipe', 'ignore', 'ignore'], - env: { - ...hookEnv(binDir), - // '1', NOT '0' — see the slot-gate test above. - GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '1', - // Hermeticity: hookEnv() spreads process.env, so a stray - // GITNEXUS_HOOK_TIMEOUT_PATH=disabled left in a developer shell - // would turn the wrapper off and fake-red this test. Empty string - // falls through to the built-in candidates (the path under test). - GITNEXUS_HOOK_TIMEOUT_PATH: '', - }, - }); - hookChild.stdin!.end( - JSON.stringify({ - hook_event_name: 'PreToolUse', - tool_name: 'Grep', - tool_input: { pattern: 'validateUser' }, - cwd: tmpDir, - }), - ); - - // The fake lsof writes its PID as its FIRST statement; poll tightly. - const spawnDeadline = Date.now() + 8000; - while (Date.now() < spawnDeadline) { - try { - const raw = fs.readFileSync(pidFile, 'utf-8').trim(); - if (raw) { - lsofPid = Number.parseInt(raw, 10); - break; - } - } catch { - /* not written yet */ - } - await new Promise((r) => setTimeout(r, 10)); - } - expect(lsofPid).toBeGreaterThan(0); - - // Kill the hook while its lsof child is alive. - hookChild.kill('SIGKILL'); - - const reapDeadline = Date.now() + 5000; - let alive = isFakeLsofAlive(); - while (alive && Date.now() < reapDeadline) { - await new Promise((r) => setTimeout(r, 100)); - alive = isFakeLsofAlive(); - } - expect(alive).toBe(false); - } finally { - // PID-reuse guard (#2169 review): re-run the detection loop's - // /proc//cmdline identity check before the cleanup SIGKILL, so - // a PID already reaped and recycled by the OS is never signalled. - if (lsofPid > 0 && isFakeLsofAlive()) { - try { - process.kill(lsofPid, 'SIGKILL'); - } catch { - /* already gone */ - } - } - try { - hookChild?.kill('SIGKILL'); - } catch { - /* ignore */ - } - // The hook claims a slot before probing now; it died holding it. - const lockDir = path.join(gitNexusDir, '.hook-locks'); - try { - for (const f of fs.readdirSync(lockDir)) fs.unlinkSync(path.join(lockDir, f)); - } catch { - /* ignore */ - } - try { - fs.rmdirSync(lockDir); - } catch { - /* ignore */ - } - fs.rmSync(lbugPath, { force: true }); - fs.rmSync(pidFile, { force: true }); - fs.rmSync(binDir, { recursive: true, force: true }); - } - }, 30000); - - // F3 (#2165 review): GITNEXUS_HOOK_TIMEOUT_PATH pointing at an EXISTING - // but unusable path (here: a directory) must not silently disable orphan - // containment. Before the fix, fs.existsSync() accepted the directory as - // THE candidate, its self-test failed, and the wrapper was memoized off — - // no fall-through — so the SIGTERM-immune lsof below survived its full - // 30s sleep. After the fix the env candidate merely goes first in the - // candidate list; failing its self-test falls through to the built-in - // coreutils guard, which still reaps the orphan within ~3s. - it('CJS: env guard pointing at a directory falls through to a working built-in guard', async () => { - // Guard-availability precheck — see resolveHostGuardForReapingTests. - expect(resolveHostGuardForReapingTests(), GUARD_PRECHECK_MSG).not.toBeNull(); - const { spawn } = await import('child_process'); - const lbugPath = path.join(gitNexusDir, 'lbug'); - fs.writeFileSync(lbugPath, ''); - const pidFile = path.join(os.tmpdir(), `gn-hook-lsofpid-dirguard-${process.pid}`); - fs.rmSync(pidFile, { force: true }); - const guardDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gn-guard-dir-')); - const binDir = createHookToolDir({ - lsofPidFile: pidFile, - lsofSleepMs: 30000, - lsofIgnoreSigterm: true, - }); - let lsofPid = 0; - let hookChild: ReturnType | null = null; - - const isFakeLsofAlive = () => { - try { - process.kill(lsofPid, 0); - } catch { - return false; // ESRCH — reaped - } - try { - return fs.readFileSync(`/proc/${lsofPid}/cmdline`, 'utf-8').includes(binDir); - } catch { - return false; - } - }; - - try { - hookChild = spawn(process.execPath, [CJS_HOOK], { - stdio: ['pipe', 'ignore', 'ignore'], - env: { - ...hookEnv(binDir), - // '1', NOT '0' — see the slot-gate test above. - GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '1', - // Exists but is a directory — spawning it fails the lazy - // self-test, forcing the fall-through path under test. - GITNEXUS_HOOK_TIMEOUT_PATH: guardDir, - }, - }); - hookChild.stdin!.end( - JSON.stringify({ - hook_event_name: 'PreToolUse', - tool_name: 'Grep', - tool_input: { pattern: 'validateUser' }, - cwd: tmpDir, - }), - ); - - const spawnDeadline = Date.now() + 8000; - while (Date.now() < spawnDeadline) { - try { - const raw = fs.readFileSync(pidFile, 'utf-8').trim(); - if (raw) { - lsofPid = Number.parseInt(raw, 10); - break; - } - } catch { - /* not written yet */ - } - await new Promise((r) => setTimeout(r, 10)); - } - expect(lsofPid).toBeGreaterThan(0); - - // Kill the hook while its lsof child is alive (the incident topology). - hookChild.kill('SIGKILL'); - - const reapDeadline = Date.now() + 5000; - let alive = isFakeLsofAlive(); - while (alive && Date.now() < reapDeadline) { - await new Promise((r) => setTimeout(r, 100)); - alive = isFakeLsofAlive(); - } - expect(alive).toBe(false); - } finally { - // PID-reuse guard (#2169 review): re-run the detection loop's - // /proc//cmdline identity check before the cleanup SIGKILL, so - // a PID already reaped and recycled by the OS is never signalled. - if (lsofPid > 0 && isFakeLsofAlive()) { - try { - process.kill(lsofPid, 'SIGKILL'); - } catch { - /* already gone */ - } - } - try { - hookChild?.kill('SIGKILL'); - } catch { - /* ignore */ - } - const lockDir = path.join(gitNexusDir, '.hook-locks'); - try { - for (const f of fs.readdirSync(lockDir)) fs.unlinkSync(path.join(lockDir, f)); - } catch { - /* ignore */ - } - try { - fs.rmdirSync(lockDir); - } catch { - /* ignore */ - } - fs.rmSync(lbugPath, { force: true }); - fs.rmSync(pidFile, { force: true }); - fs.rmSync(guardDir, { recursive: true, force: true }); - fs.rmSync(binDir, { recursive: true, force: true }); - } - }, 30000); - }, -); +// ─── #2180: the probe no longer spawns lsof on Linux ─────────────── +// +// The 'Orphaned lsof is reaped by the timeout wrapper (#2163)' suite that +// lived here (T3 + the env-guard-points-at-a-directory fall-through test) +// drove the Linux probe to spawn a SIGTERM-immune fake lsof and asserted the +// coreutils `timeout -k 1` wrapper reaped it after the hook was SIGKILLed. +// #2180 replaced the O(procs×fds) scan + lsof fallback with a pure cmdline- +// first procfs scan and DELETED the Linux lsof leg entirely, so the probe can +// no longer create an lsof orphan on Linux by construction — there is nothing +// left for those tests to exercise. The wrapper-reaping mechanism they pinned +// is still covered where it still applies: the augment CLI child (the +// direct-exec and npx-grandchild reaping suites below) and the macOS/other- +// Unix lsof+ps path (the `Ladybug DB owner guard` suite, now relaned off +// Linux). The env-guard fall-through self-test behaviour is still pinned by +// the bad-wrapper / dir-guard guard-resolution tests in that relaned suite. // ─── Behavior: SIGKILLed hook cannot strand the augment CLI (#2163 f-up) ── @@ -1428,11 +1211,16 @@ describe.skipIf(process.platform !== 'linux')( // Guard-availability precheck — see resolveHostGuardForReapingTests. expect(resolveHostGuardForReapingTests(), GUARD_PRECHECK_MSG).not.toBeNull(); const { spawn } = await import('child_process'); - // REQUIRED: a real lbug file routes the probe through the fake lsof - // (empty output → no holder PIDs → probe false) so the augment runs - // through the same probe-then-spawn flow as production. + // REQUIRED: a real lbug file means the probe runs. #2180 removed the + // Linux lsof fallback, so we route the probe at an EMPTY fake /proc + // (no gitnexus server holding the fd → not-owned) so the augment runs + // through the same probe-then-spawn flow as production. (Pre-#2180 this + // used GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS:'1' to fall through to a fake + // lsof; that path no longer exists on Linux — a '1' budget now fails + // CLOSED and would skip the augment entirely.) const lbugPath = path.join(gitNexusDir, 'lbug'); fs.writeFileSync(lbugPath, ''); + const emptyProcRoot = createFakeProcRoot([]); const pidFile = path.join(os.tmpdir(), `gn-hook-clipid-${process.pid}-${label}`); fs.rmSync(pidFile, { force: true }); const binDir = createHookToolDir({ @@ -1465,8 +1253,11 @@ describe.skipIf(process.platform !== 'linux')( stdio: ['pipe', 'ignore', 'ignore'], env: { ...hookEnv(binDir), - // '1', NOT '0' — see the slot-gate test above. - GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '1', + // #2180: empty fake /proc → scan completes as not-owned → augment + // runs (the path under test). Generous budget so the scan never + // times out and fails closed. + GITNEXUS_HOOK_PROC_ROOT: emptyProcRoot, + GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '5000', // Hermeticity: a dev-shell GITNEXUS_HOOK_TIMEOUT_PATH=disabled // would unwrap the CLI and fake-red this test. Empty string // falls through to the built-in candidates (the path under test). @@ -1542,6 +1333,7 @@ describe.skipIf(process.platform !== 'linux')( fs.rmSync(lbugPath, { force: true }); fs.rmSync(pidFile, { force: true }); fs.rmSync(binDir, { recursive: true, force: true }); + fs.rmSync(emptyProcRoot, { recursive: true, force: true }); } }, 30000); } @@ -1574,11 +1366,14 @@ describe.skipIf(process.platform !== 'linux')( // Guard-availability precheck — see resolveHostGuardForReapingTests. expect(resolveHostGuardForReapingTests(), GUARD_PRECHECK_MSG).not.toBeNull(); const { spawn } = await import('child_process'); - // REQUIRED: a real lbug file routes the probe through the fake lsof - // (empty output → no holder PIDs → probe false) so the augment runs - // through the same probe-then-spawn flow as production. + // REQUIRED: a real lbug file means the probe runs. #2180 removed the + // Linux lsof fallback, so we route the probe at an EMPTY fake /proc + // (not-owned) so the augment runs through the same probe-then-spawn flow + // as production. (Pre-#2180 this used BUDGET_MS:'1' to fall through to a + // fake lsof; that path no longer exists on Linux.) const lbugPath = path.join(gitNexusDir, 'lbug'); fs.writeFileSync(lbugPath, ''); + const emptyProcRoot = createFakeProcRoot([]); const pidFile = path.join(os.tmpdir(), `gn-hook-npxclipid-${process.pid}`); fs.rmSync(pidFile, { force: true }); // Route self-proof (#2169 review): written by the fake npx as its first @@ -1647,8 +1442,10 @@ describe.skipIf(process.platform !== 'linux')( // copy's require.resolve to find via NODE_PATH. GITNEXUS_HOOK_CLI_PATH: '', NODE_PATH: '', - // '1', NOT '0' — see the slot-gate test above. - GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '1', + // #2180: empty fake /proc → not-owned → augment runs. Generous + // budget so the scan completes rather than failing closed. + GITNEXUS_HOOK_PROC_ROOT: emptyProcRoot, + GITNEXUS_HOOK_LINUX_PROC_BUDGET_MS: '5000', // Hermeticity: fall through to the built-in guard candidates. GITNEXUS_HOOK_TIMEOUT_PATH: '', }, @@ -1726,6 +1523,7 @@ describe.skipIf(process.platform !== 'linux')( fs.rmSync(npxMarkerPath, { force: true }); fs.rmSync(stagedDir, { recursive: true, force: true }); fs.rmSync(binDir, { recursive: true, force: true }); + fs.rmSync(emptyProcRoot, { recursive: true, force: true }); } }, 45000); }, @@ -1988,7 +1786,7 @@ describe('PreToolUse augmentation filtering (integration)', () => { // exit 0 — so strict hook runners (e.g. Codex `PreToolUse`) never see // unexpected output. GITNEXUS_DEBUG is forced off to keep the assertion // deterministic regardless of the ambient environment. - it.skipIf(process.platform === 'win32')( + it.skipIf(SKIP_LSOF_PATH)( `${label}: skips augment SILENTLY when a GitNexus MCP process owns the repo DB`, () => { const markerPath = path.join(os.tmpdir(), `gitnexus-hook-called-${process.pid}-${label}`); @@ -2028,7 +1826,7 @@ describe('PreToolUse augmentation filtering (integration)', () => { // Issue #1913: the skip reason remains recoverable for operators who opt in // via GITNEXUS_DEBUG=1 — stdout stays empty (no augment ran), the diagnostic // appears on stderr. - it.skipIf(process.platform === 'win32')( + it.skipIf(SKIP_LSOF_PATH)( `${label}: surfaces the MCP-owner skip reason only under GITNEXUS_DEBUG`, () => { const markerPath = path.join(os.tmpdir(), `gitnexus-hook-dbg-${process.pid}-${label}`); @@ -2071,7 +1869,7 @@ describe('PreToolUse augmentation filtering (integration)', () => { // have emitted on these; this guards the unified strict gate (incl. the // main() catch handler) across the claude/plugin copies. for (const debugValue of ['0', 'false']) { - it.skipIf(process.platform === 'win32')( + it.skipIf(SKIP_LSOF_PATH)( `${label}: MCP-owner skip stays SILENT with GITNEXUS_DEBUG='${debugValue}' (strict contract)`, () => { const markerPath = path.join( @@ -2114,14 +1912,22 @@ describe('PreToolUse augmentation filtering (integration)', () => { } }); -describe.skipIf(process.platform === 'win32')( +describe.skipIf(SKIP_LSOF_PATH)( 'Ladybug DB owner guard — production-shaped ps + failure modes (#1493)', () => { - // These tests assert owner *detection*: a positive skip is signalled by the - // `[GitNexus] augment skipped` diagnostic. Since #1913 made that diagnostic - // debug-gated (silent by default for strict hook runners), they run with - // GITNEXUS_DEBUG=1 so the discriminator remains observable. Default-silence - // itself is covered by the 'augmentation filtering' describe above. + // These tests assert owner *detection* via the lsof + ps backend: a positive + // skip is signalled by the `[GitNexus] augment skipped` diagnostic. Since + // #1913 made that diagnostic debug-gated (silent by default for strict hook + // runners), they run with GITNEXUS_DEBUG=1 so the discriminator remains + // observable. Default-silence itself is covered by the 'augmentation + // filtering' describe above. + // + // #2180: skipped on Linux (SKIP_LSOF_PATH) — Linux no longer routes through + // lsof/ps, so these would no longer exercise the real dispatch there. They + // stay as the macOS/other-Unix lsof+ps lane; the equivalent Linux owner- + // detection (incl. the EACCES / cross-user fail-closed edge and the budget + // timeout fail-closed) is covered directly against a fake /proc in + // test/unit/hook-db-lock-probe.test.ts. for (const [label, hookPath] of [ ['CJS', CJS_HOOK], ['Plugin', PLUGIN_HOOK], diff --git a/gitnexus/test/utils/hook-test-helpers.ts b/gitnexus/test/utils/hook-test-helpers.ts index 003db10c0..f5203a023 100644 --- a/gitnexus/test/utils/hook-test-helpers.ts +++ b/gitnexus/test/utils/hook-test-helpers.ts @@ -161,6 +161,57 @@ process.exit(0); return binDir; } +// ─── Fake /proc root for the Linux cmdline-first DB-owner scan (#2180) ── +// +// linuxProcScanFindGitNexusServer reads every path under GITNEXUS_HOOK_PROC_ROOT +// (defaulting to /proc in production). These helpers build a fixture tree so the +// three-phase scan (comm -> cmdline -> fd dev+ino) can be unit-tested without +// touching the test host's real, hundreds-of-process /proc — which is both slow +// and nondeterministic (other gitnexus servers may be running). fd entries are +// real symlinks to real files, so fs.statSync on them yields real dev+ino the +// scan can compare against the target lbug. + +export interface FakeProcEntry { + pid: number | string; + /** /proc//comm contents (kernel caps at 15 visible chars; caller models truncation). */ + comm: string; + /** argv tokens; joined with NUL like the real /proc//cmdline. */ + cmdline: string[]; + /** Absolute paths this pid "holds" open — each becomes an fd symlink target. */ + fdTargets?: string[]; + /** When true, make /proc//fd unreadable-shaped by omitting it entirely so readdir throws ENOENT; for EACCES use the logic-path test instead. */ + noFdDir?: boolean; +} + +/** + * Build a fake /proc tree under a fresh temp dir and return its path (use as + * GITNEXUS_HOOK_PROC_ROOT). Caller is responsible for rm-ing the returned dir. + */ +export function createFakeProcRoot(entries: FakeProcEntry[]): string { + const procRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gitnexus-fakeproc-')); + for (const e of entries) { + const pidDir = path.join(procRoot, String(e.pid)); + fs.mkdirSync(pidDir, { recursive: true }); + fs.writeFileSync(path.join(pidDir, 'comm'), `${e.comm}\n`); + fs.writeFileSync(path.join(pidDir, 'cmdline'), e.cmdline.join('\0') + '\0'); + if (!e.noFdDir) { + const fdDir = path.join(pidDir, 'fd'); + fs.mkdirSync(fdDir, { recursive: true }); + const targets = e.fdTargets ?? []; + targets.forEach((target, i) => { + // Real symlink so statSync(link) follows to the real file's dev+ino — + // exactly what the scan compares against the target lbug. + try { + fs.symlinkSync(target, path.join(fdDir, String(i + 3))); + } catch { + /* best-effort; a missing target just won't match */ + } + }); + } + } + return procRoot; +} + /** A full env that points a spawned hook at the fake tool dir from createHookToolDir. */ export function hookEnv(binDir: string) { return {