When the worker pool's Layer 3 quarantine filters one or more files
out of a chunk's dispatch, the worker results returned to
processParsing are silently narrower than the input chunk. Without
this reparse, the graph for this run would be missing every quarantined
file's symbols/imports/calls/heritage with no failure signal.
After the existing per-chunk quarantine log emits in
processParsing's worker-path try-block, run processParsingSequential
on JUST the quarantined-in-chunk files. The sequential path writes
directly to the graph, so symbols for those files land alongside
worker output for the surviving files.
Mirrors the WorkerPoolDispatchError catch-block's processParsingSequential
call shape — same signature, same args, same scopeTreeCache wiring.
Emits a structured warn naming `reparsedPaths` so operators can
observe the sequential fall-through.
This fixes the in-run side of the corruption Codex's adversarial
review of PR #1693 flagged. The cross-run side (chunk-cache
poisoning) is closed by U20.U2 in a follow-up commit.
References plan: docs/plans/2026-05-20-002-fix-chunk-cache-corruption-on-worker-quarantine-plan.md
Walks the full set of findings from a multi-agent code review (11
reviewers, 1 maintainability dispatch lost to tool-permission denial)
of the PR #1693 branch. All 16 actionable findings — 4 P1, 4 P2,
8 P3 — applied in a single pass against a consistent tree. Tests
pass (269/269 unit files, 29/29 integration).
P1 — bounds-only / disguised-bounds assertions across 4 test files
(per user-memory DoD §2.7):
- worker-pool.test.ts: 5 sites — `nodes.length > 0` dropped (redundant
after `.toContain('validateInput')`); `files.length >= 4` pinned to
`.toBe(7)` (mini-repo/src has exactly 7 .ts files); `results.length
> 0` pinned to `.toHaveLength(1)` (default sub-batch absorbs all 7);
`result.fileCount >= 0` pinned to `.toBe(1)` (empty file is still
"processed"); `warnRecords.length > 0` replaced with content-
predicate `/respawn|dropping|replacement|did not report ready/`
(catches silenced warnings); `fallbackExcludePaths.length > 0`
pinned to exact `['one.ts', 'two.ts']` (deterministic given the
single-slot pool + 2 items + per-item starting-file).
- parse-impl-fallback.test.ts: 3 sites — `astCacheClearCalls >= 1`
pinned to exact 4 (per-chunk × 2 + finally × 2); the two error-path
delta checks pinned to exact +2 and +3 (verified empirically).
- parse-impl-progress-monotonic.test.ts: `percents.length > 0` →
`.not.toEqual([])`; per-element `Math.max(prev, cur)` tautology
replaced with direct `if (cur < prev) throw`; final-percent
`Math.min(last, 95)` tautology pinned to exact `.toBe(70)` (3-file
skipWorkers fixture's deferred band lands at the band start).
- parse-impl-large-fixture.test.ts: `Math.min(elapsedMs, BUDGET)`
tautology removed; Promise.race rejection is the load-bearing
wall-clock check.
P1 — terminate() lacks `.catch` mask:
- worker-pool.ts terminate() now matches the `.catch(() => undefined)`
pattern used at every other internal terminate site. Prevents a
hung/OOM worker's terminate rejection from masking the original
pipeline error when called from parse-impl.ts's finally block, and
guarantees `workers.length = 0` / `activeSlots.clear()` always run.
P1 — hybrid envelope length-mismatch + null-payload silent data loss:
- parse-worker.ts decodeIncomingMessage: explicit non-null-and-typed
check before `.type` access (decodeMessage permits null payloads
per encodeMessage contract); explicit length-equality assertion
between `decoded.files` and `contents` before zipping. Without
these, `TextDecoder.decode(undefined)` silently returns "" and
produces empty-content graph nodes — a contract violation that
used to be undetectable. Both throws route through the outer
try/catch → worker `error` reply → pool's recoverAndResume.
P1 — unsafe casts at the IPC boundary:
- buildDispatchMessage now uses a properly-typed `isParseWorkerItemArray`
type guard. The narrowed branch accesses `item.path` and
`item.content` as statically-typed strings — a future rename of
`ParseWorkerInput.content` would fail to compile inside the branch
instead of silently mismatching at runtime. The remaining
decodeMessage payload casts are bounded by the F3/F6 runtime
guards.
P2 — idle-timeout retry bypasses circuit breaker:
- worker-pool.ts timeout-retry IIFE now increments
`consecutiveFailuresPerSlot[workerIndex]` alongside `respawnCount`.
A slot that consistently times out (vs crashes) now trips the
per-slot breaker, instead of consuming its full respawn budget
over potentially tens of minutes without the breaker firing.
P2 — null/non-object worker message crashes pool handler:
- Dispatch handler in worker-pool.ts now guards `null /
non-object / no string type discriminant` before `msg.type` access
and routes through recoverAndResume on violation. Previously a
legitimate `null` payload would throw TypeError out of the
EventEmitter listener → uncaughtException on main, crashing the
analyze.
P2 — workerPoolSize === 0 creates unusable pool:
- parse-impl.ts now treats `workerPoolSize === 0` as `skipWorkers`
at the gate. Matches the PipelineOptions docstring contract ("0
disables the pool entirely — equivalent to skipWorkers"); avoids
constructing a pool that rejects every dispatch and logs
"Worker pool parsing stopped" per chunk.
P2 — encodeMessage 2-buffer allocation per frame:
- protocol.ts encodeMessage coalesced to a single
`Buffer.allocUnsafe + writeUInt8 + writeUInt32LE + buf.write
(string, offset, 'utf8')`. Drops the intermediate
`Buffer.from(JSON.stringify(...), 'utf8')` allocation + memcpy.
Length pre-check via `Buffer.byteLength(string, 'utf8')` surfaces
the uint32 cap before any allocation.
P3 — slotGenerations made optional on WorkerPoolStats so external
implementations of getStats() that predate U12 don't compile-break;
in-repo callers already use optional chaining.
P3 — buildDispatchMessage marked `@internal` so it isn't surfaced as
public API by typedoc / api-extractor (it's a test-only export).
P3 — verboseThroughputLog hoisted above the chunk loop (env vars can't
change mid-run; one O(env-read) per analyze, not per chunk).
P3 — corrected the messageerror routing comment in worker-pool.ts
dispatch handler. `ProtocolDecodeError` is caught by the surrounding
try/catch — distinct from `messageerror`, which fires for V8
structured-clone failures before the message body would reach the
handler.
P3 — initial pool spawn now uses a `Promise.allSettled` ready-handshake
gate symmetric with `replaceWorker`. Dispatch awaits this gate before
selecting slots, so an init-crashing initial worker is dropped from
`activeSlots` and a downstream OOM/missing-native-binding failure
surfaces in seconds (bounded by WORKER_READY_TIMEOUT_MS) rather than
waiting for the first idle timeout (30s default).
P3 — `GITNEXUS_WORKER_MAX_RESPAWNS_PER_SLOT`,
`GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS`,
`GITNEXUS_WORKER_CONSECUTIVE_FAILURE_THRESHOLD` added to:
- CLI `--help` text in src/cli/index.ts
- Root README env-var table
- gitnexus/README troubleshooting section (new "Worker pool
resilience tuning" subsection)
P3 — CLI `catch (e: any)` / `catch (err: any)` in analyze.ts replaced
with `catch (err: unknown)` + narrowed access; matches modern TS
best practice and the codebase pattern at other catch sites.
P3 — `WorkerPoolStats.terminated: boolean` field added (optional, for
backward compatibility). `terminate()` sets it true; `getStats()`
surfaces it. Distinguishes graceful shutdown from a circuit-breaker
trip in observability surfaces.
Coverage / advisory items not addressed in this commit (kept in the
report only):
- maintainability reviewer failed (Read/Bash denied) — god-module
audit on worker-pool.ts (~1400 LOC) carried as residual risk
- quarantine case-sensitivity contract unpinned (adversarial #8)
- WORKER_READY_TIMEOUT_MS env-configurability (adversarial #2)
- chunk-byte-budget × parseChunkConcurrency memory multiplier doc
(adversarial #5)
- MCP discoverability gaps for env vars / verbose (agent-native W1/W2)
- bench/parse-throughput.md scaffold-with-TBD-rows (PS RR-003)
Pool dispatch now hoists `{path, content: string}[]` file contents OUT
of the U17 JSON envelope into separately-allocated `Uint8Array`s whose
ArrayBuffers are passed to `worker.postMessage`'s `transferList` for
zero-copy ownership transfer. The envelope itself carries only
lightweight metadata (`{path, byteLength}` per file) and is structure-
cloned the same as before.
What this saves vs U17 baseline:
- **JSON.stringify of file contents on main thread** drops to zero —
the envelope is now O(paths + sizes), not O(total bytes). For a 200-
file sub-batch of 10 KB TS files, that's ~2 MB of escape processing
per dispatch that disappears. JSON.stringify's per-character branch
on quotes/backslashes/control chars is roughly 2x slower than
UTF-8 transcode in TextEncoder, so the replacement is a CPU win
even though it adds a single TextEncoder.encode per file.
- **Structured-clone memcpy of file contents** drops to zero — the
contents' backing ArrayBuffers are ownership-transferred, not copied
into the worker's heap. The envelope's struct-clone cost is now
proportional to metadata size only.
- **JSON.parse on worker thread** likewise no longer scales with
content size. Worker decodes each `Uint8Array` to string via
`TextDecoder` lazily at the parse boundary — runs on the worker
thread, parallel with continued main-thread work, vs U17's
sequential JSON.parse blocking the worker before processBatch can
start.
Pipelining: TextEncoder.encode (main) and TextDecoder.decode (worker)
can both run while the OTHER side is doing useful work. Under U17,
struct-clone was a synchronous main-thread blocker.
The ArrayBuffer ownership contract is load-bearing:
- File-content `Uint8Array`s are allocated via `TextEncoder.encode`,
NOT `Buffer.from(str, 'utf8')`. TextEncoder produces a dedicated
ArrayBuffer per call; `Buffer.from(str)` carves from Node's shared
`Buffer.poolSize` slab for small strings, so transferring one
pool-backed Buffer's ArrayBuffer would detach every other Buffer
that shares that slab — silent data corruption.
- The envelope itself is NOT transferred. It MAY be pool-backed by
`encodeMessage`, and at ~30-80 bytes/file the struct-clone cost is
negligible. Not transferring avoids the same detach-collateral risk
the contents path is careful to dodge.
Detection is strict: every input element must have both `path: string`
and `content: string`. A single non-conforming element disqualifies
the whole batch from the transfer path and falls back to the legacy
single-Uint8Array `encodeMessage` envelope. Safer than partial
transfer (which would split a sub-batch into mixed-shape messages
the worker can't reassemble).
`parse-worker.ts` `decodeIncomingMessage` recognizes the hybrid
`{envelope, contents}` shape, decodes the envelope, zips metadata
positionally with the contents array, decodes UTF-8 → string per file,
and hands the reassembled `ParseWorkerInput[]` to the existing
`processBatch`. Identical downstream behavior to U17 — the IPC
optimization is invisible above this line.
Test scaffolding (3 FakeWorkers + 1 integration-test preamble) gain a
`decodeDispatchedMessage` helper that tolerates BOTH shapes (legacy
single-frame Uint8Array AND the new hybrid envelope+contents) so the
in-process unit mocks keep their existing action-scripting API and the
9 ad-hoc integration test workers keep their `msg.type === 'sub-batch'`
handlers unchanged.
`buildDispatchMessage` is now exported from worker-pool.ts so its
contract can be tested in isolation. A new
`test/unit/worker-pool-transferlist.test.ts` pins:
- hybrid shape produced for parse-worker inputs
- transferList carries one ArrayBuffer per file in input order
- envelope decodes to metadata only (no `content` field)
- content bytes round-trip byte-for-byte through UTF-8 (ASCII,
multi-byte, surrogate-pair emoji)
- each content's ArrayBuffer is independently allocated (no pool
sharing) — the load-bearing transfer-safety invariant
- non-parse shapes, empty arrays, and mixed-conformance arrays all
fall back to the legacy single-frame path
All 271 test files (6166 unit + integration tests) pass.
Production worker IPC now uses the U16 binary wire format (1-byte tag +
4-byte LE length + UTF-8 JSON body) end-to-end. The pool encodes every
outgoing `sub-batch` / `flush` dispatch via `encodeMessage`; the worker
decodes incoming frames via `decodeMessage` and encodes its `ready`,
`starting-file`, `progress`, `sub-batch-done`, `result`, `warning`, and
`error` outputs the same way.
The load-bearing correctness fix is making `decodeMessage` accept
`Uint8Array` rather than only `Buffer`: Node's `worker_threads`
`postMessage` structured-clones the payload, which strips the `Buffer`
prototype on the receive side. A frame sent as `Buffer` arrives as a
plain `Uint8Array`, and `Buffer.isBuffer(raw)` returns false — so the
first attempt at U17 (gating decode on `Buffer.isBuffer`) silently
treated every incoming frame as POJO and the worker never responded.
The fix adopts the underlying memory zero-copy via
`Buffer.from(view.buffer, view.byteOffset, view.byteLength)` and uses
`raw instanceof Uint8Array` at every call site (parse-worker decode,
pool dispatch handler, pool ready-handshake handler, FakeWorker test
mocks, and the integration-test worker preamble).
The pool stays tolerant of POJO incoming so unit-test FakeWorkers
don't need rewriting — only the new outgoing encoded dispatches require
the test scaffolding to decode on receive, which the test FakeWorkers
and the integration test's inline `parentPort.on` wrapper now do.
The slot-drop integration test was rewritten from a shared-counter-file
race (which pre-U17 timing happened to land on the assertion-friendly
counter==2 endpoint, but post-U17 protocol decoding latency shifted to
counter==1 and produced 3 quarantines instead of 2) to a deterministic
path-based crash trigger: slot 0 crashes on a.ts, respawns, crashes on
the requeued b.ts, slot is dropped after budget exhausted; slot 1
handles [c.ts, d.ts] normally. Outcome no longer depends on inter-worker
file-write ordering.
Protocol coverage adds two regression tests pinning the Uint8Array
decode path: structured-clone-stripped frames decode identically to
their Buffer originals, and Uint8Array views with non-zero byteOffset
into a wider ArrayBuffer also decode correctly (catches `Buffer.from(uint8)`
copying semantics if a future refactor loses the zero-copy adoption).
All 94 worker-pool tests (9 files, unit + integration) pass; the full
unit suite (6128 tests across 268 files) passes unchanged.
Honest partial U13: extract the quarantine resilience layer (Layer 3
of the 5-layer model) into a dedicated module with a small explicit
interface. The full 5-module split that the original plan named was
flagged by doc-review A10 as abstraction-without-multi-consumer-demand
("Each has exactly one consumer: worker-pool.ts. None of these layers
is imported elsewhere in the codebase pre-extraction, and the plan
doesn't identify any future consumer.") This commit ships the smallest
self-contained layer as a named module to validate the factory +
interface pattern with minimal risk. The remaining four layers
(respawn-budget, cumulative-timeout, circuit-breaker, slot-attribution)
stay inline until a real second consumer emerges (e.g., a non-parse
worker pool that reuses the same resilience layers).
Module shape (`workers/quarantine.ts`, ~30 LOC):
interface Quarantine {
add(path: string): void;
has(path: string): boolean;
snapshot(): string[]; // defensive copy
readonly size: number; // getter, reflects state at access time
}
function createQuarantine(): Quarantine
Replaces in `worker-pool.ts`:
- `const quarantined: Set<string> = new Set()` -> `createQuarantine()`
- `quarantined.has(p)` -> `quarantine.has(p)` (2 sites)
- `quarantined.add(p)` -> `quarantine.add(p)` (2 sites)
- `quarantined.size` -> `quarantine.size` (2 sites)
- `Array.from(quarantined)` -> `quarantine.snapshot()` (6 sites)
Public worker-pool.ts API is unchanged — `getQuarantinedPaths()` still
returns the same defensive `string[]` copy. The behavioral contract is
preserved: paths are quarantined as opaque strings (the U9 / M5
non-normalization contract still holds — see the new dedicated test).
Tests:
- 8 isolated unit tests for the quarantine module — pins the
interface contract (empty start, add/has/size, dedup on repeated
add, no separator normalization, snapshot defensive copy + freshness,
size-getter live behavior).
- All 86 existing worker-pool tests pass unchanged — they exercise
the quarantine through the pool and act as the regression net for
behavior preservation.
Why not the full 5-module extraction in this commit: doc-review A10's
concern is real — a single-consumer abstraction adds module-boundary
overhead (5 sets of imports, 5 dedicated test files, 5 interfaces to
keep in sync with worker-pool) without any structural benefit until a
second consumer materializes. Extracting one validates the pattern;
the remaining four can be moved on demand.
Defines the binary frame for worker-thread IPC as an isolated, fully-tested
module. Production wiring is deferred to U17 — shipping the wire-format
contract first de-risks the migration by establishing a single source of
truth for the byte layout. Resolves the scaffold half of PR #1693 review
R12.
Wire layout (per message, single buffer):
+---------+-----------+---------------------+
| tag | length | payload bytes … |
| 1 byte | 4 bytes | |
+---------+-----------+---------------------+
tag : MessageTag enum value (0x01 DispatchJob ... 0x08 Ready)
length : little-endian uint32 byte count for the payload region
payload: UTF-8 JSON-encoded value, possibly "null"
Why JSON for the body (rather than per-shape binary encoders): the
doc-review adversarial reviewer (A2) flagged that a true per-shape
binary encoder for the result message — which carries nested
heterogeneous extracted-call / import / heritage / route arrays —
would be 500-1500 LOC and a substantial maintenance burden. The
honest perf win the IPC repack targets is moving file CONTENTS via
ArrayBuffer transferList (zero-copy ownership transfer for the
largest single piece of state in any message). That win is captured
by U17 layering transferList over the bulk file-content payload while
keeping this module's framing for the surrounding metadata. If U18
benchmark data shows the JSON body is itself a bottleneck after U17
lands, a follow-up unit can swap to per-shape binary encoding behind
the same encodeMessage / decodeMessage surface without changing the
frame.
API:
- MessageTag (const object): stable byte tags 0x01..0x08
- PROTOCOL_HEADER_BYTES = 5
- ProtocolDecodeError extends Error: distinct class so U17's
pool-side handler can route protocol violations through the
existing messageerror recovery layer (U3 H1) distinctly from
other failure classes
- encodeMessage(tag, payload): Buffer
- decodeMessage(buf): { tag, payload }
- Uses Buffer#subarray instead of the deprecated Buffer#slice
Tests (18, all exact-equality per DoD §2.7):
- byte layout (tag at offset 0, length LE uint32 at offset 1)
- empty/null payload encodes to 5-byte header + 4-byte "null" body
- round-trip for every MessageTag with representative payloads
- non-ASCII path string (UTF-8 byte-length boundary)
- 9 MB payload (well past the existing 8 MB sub-batch budget)
- decode errors surface as ProtocolDecodeError, not generic Error:
* buffer < header size
* tag outside valid range
* declared length exceeds buffer
* payload bytes are not valid JSON
- error class name is preserved through prototype chain so callers
can `err instanceof ProtocolDecodeError` reliably
PR #1693 review M1 noted that the deferred-extraction accumulator
arrays (`deferredWorkerImports`, `deferredWorkerCalls`,
`deferredWorkerHeritage`, `deferredConstructorBindings`,
`deferredAssignments`) were retained until function return, making
peak accumulator memory O(repo) instead of O(in-flight stage).
This commit implements the LIGHTWEIGHT version: free each array
immediately after its last consumer drains/reads it, dropping peak
accumulator memory progressively through the deferred-extraction
stages. The structural per-chunk streaming variant (the original
U15 framing) is deliberately deferred — the doc-review's adversarial
reviewer (A4) flagged it as defending unmeasured memory pressure,
and the simpler array-clearing captures the bulk of the benefit
without committing to a scheduling-strategy decision (microtask vs
parallel extractor task vs worker-side) that profile data should
inform.
Clears added:
1. After `processImportsFromExtracted` (the sole consumer of
`deferredWorkerImports`): clear the imports array before
the heavier heritage/calls stages run.
2. After `buildHeritageMap` (the LAST consumer of the raw
`deferredWorkerHeritage` records — processCallsFromExtracted
reads from the derived `fullWorkerHeritageMap` instead):
clear the heritage array before the call-resolution stage.
3. After `processAssignmentsFromExtracted` (the joint last
consumer with processCallsFromExtracted for the calls/
bindings/assignments triple): clear all three before
downstream graph-build / scope-resolution uses its own
working memory.
Arrays returned in the function result object (allFetchCalls,
allExtractedRoutes, allDecoratorRoutes, allToolDefs, allORMQueries,
allParsedFiles) intentionally stay live — downstream consumers
need them.
Graph-output equivalence is preserved (U7 multi-chunk equivalence
test passes — the clears happen AFTER each array's last consumer
has copied data into the graph or derived structures).
Resolves PR #1693 review R13 (benchmark artifact requirement).
Creates `gitnexus/bench/parse-throughput.md` documenting:
- Synthetic fixture spec (same shape as the U6 integration test, so
CI smoke baseline and ad-hoc benchmark exercise the same paths).
- What to measure (wall-clock, peak heap, chunk count, getStats
snapshot) and the hardware-shape metadata to record alongside.
- Harness recipe — vitest + env-var overrides to exercise sequential
fallback vs worker-pool paths.
- Latest-measurement table with placeholder rows for the three paths
(sequential, workers+concurrency, workers single-threaded) and an
explicit "Status: scaffold — fill in before merging" callout. The
U6 test's observed ~6 s wall-clock is captured as a smoke-baseline.
- Operator-tuning quick reference cross-linked to the README env-var
section (U11) so the doc is actionable without re-reading the PR.
- "What this benchmark does NOT measure" section explicitly scoping
the artifact's limits (synthetic ≠ real-repo, throughput-only ≠
resilience-tested, Phase 3 IPC repack row reserved for U16-U17).
Mitigates the doc-review SG5 "static doc drift" concern via:
1. Explicit "regenerate this file before merging" callout at the top.
2. Self-contained methodology so anyone can re-run the numbers.
3. Cross-links to the U6 integration test that already bounds the
wall-clock as part of the CI suite — so "is it still completing?"
is regression-tested even if the numbers in this doc drift.
The standalone harness script (`bench/scripts/parse-throughput.ts`)
remains a stretch goal per the original plan. The U6 vitest with
verbose ingestion logs covers the primary observability gap until
the standalone harness lands.
Adds a monotonic per-slot generation counter to createWorkerPool's
state. Each successful worker replacement (replaceWorker) bumps the
slot's counter exactly once — atomically with the workers[slotIndex]
swap, so observers (getStats) see the new (worker, generation) pair
consistently. Handler closures in the dispatch loop capture the
slot's generation at attach time and short-circuit when they fire
on a stale generation.
In the current implementation, cleanup() synchronously removes
listeners on a Worker instance the moment a death is observed, so
no listener naturally fires on a stale generation — the guard is a
defensive layer protecting against any future refactor that loosens
cleanup() ordering or re-attaches handlers across the swap. The
load-bearing observable is the slotGenerations[] array exposed via
WorkerPoolStats so operators (and tests) can confirm a slot was
actually replaced and not just the same worker recycled.
Implementation:
- const slotGenerations: number[] = new Array(size).fill(0) in
createWorkerPool's per-pool state, alongside respawnCount and
consecutiveFailuresPerSlot.
- replaceWorker: slotGenerations[workerIndex]++ AFTER the
workers[workerIndex] = replacement swap (only on the success
branch — drop-slot paths leave the counter unchanged).
- runWorker dispatch loop: const slotGen = slotGenerations[workerIndex]
captured before handler attachment; every handler (handler /
errorHandler / exitHandler / messageErrorHandler) starts with
`if (slotGenerations[workerIndex] !== slotGen) return`.
- WorkerPoolStats gains `readonly slotGenerations: readonly number[]`.
- getStats() returns slotGenerations.slice() so callers can't mutate
pool state by writing to the returned array.
Two existing toEqual snapshots in worker-pool-resilience.test.ts
extended with the new slotGenerations field (both expect all-zeros —
neither test scenario triggers a respawn).
New test file (worker-pool-slot-generation.test.ts, 4 tests):
1. Fresh pool: every slot at generation 0.
2. Successful crash + respawn: generation bumps to 1 exactly once.
3. Crash that drops the slot (maxRespawnsPerSlot:0): generation
stays at 0 because no successful respawn happened. The dispatch
rejection on breaker trip is the expected outcome here; the
load-bearing assertion is the post-rejection stats.
4. Multi-slot independence: one slot crashing bumps only that
slot's generation, not the other. Order-independent via sort()
because the round-robin assignment isn't pinned by contract.
All assertions exact .toEqual / .toBe per DoD §2.7.
Resolves PR #1693 review F7 / U14: pre-U14, `CHUNK_BYTE_BUDGET` was a
module-load IIFE constant that captured `GITNEXUS_CHUNK_BYTE_BUDGET`
once and froze the value for the module's lifetime. That defeated
per-call option threading (a future
`PipelineOptions.chunkByteBudget` was silently no-op'd because the
function body read the frozen module-level constant) AND forced tests
to use `vi.resetModules` to vary chunk layout. The U7
deferred-extraction test and the U6 multi-chunk integration test
both used the workaround.
After this change:
- `DEFAULT_CHUNK_BYTE_BUDGET = 2 * 1024 * 1024` stays as a
module-level constant — purely a default, no env access.
- `resolveChunkByteBudget(options)` runs per call: option wins,
then env, then default. Same options-first/env-fallback/default
pattern as resolveAutoPoolSize and the U1 parseChunkConcurrency
resolver — keeps the ingestion code's configuration model uniform.
- `PipelineOptions.chunkByteBudget?` added with documentation that
threading through options lets long-running hosts (eval-server,
MCP daemon) size per-call without leaking process.env state
across analyze invocations.
New test (parse-impl-env-reads.test.ts) pins all four behaviors:
1. option-first: option present + env present -> option wins
2. env-fallback: option absent + env present -> env wins
3. default-fallback: both absent -> 2 MB default
4. per-call: two back-to-back runs in the same vitest worker with
different chunkByteBudget option values observe their OWN values,
proving the module-load freeze is gone (no vi.resetModules in
this test — that's the invariant being verified).
All four assertions use exact `.toBe(N)` per DoD §2.7. The chunk
count is observed by parsing the `Parsing chunk X/Y` progress message
stream — a stable proxy that doesn't require exposing internal
parse-impl counter state.
Note: U7 and U6 tests still use `vi.resetModules` because they were
written before this change. A follow-up cleanup could simplify those
tests (drop the resetModules dance, pass chunkByteBudget via options),
but they pass as-is so this commit doesn't touch them.
Resolves PR #1693 review B3 — the final P0/P1 merge blocker. With this
test, all five doc-review blockers (B1-B5) are pinned by regression
coverage.
The PR's headline claim is "analyze no longer hangs on TS-root-shaped
loads". The existing suite pins each resilience layer (worker-pool-
resilience.test.ts), the deferred-extraction equivalence (U7), and
the chunk-concurrency contract (U1). What was missing: a single
end-to-end run that exercises the full chunked parse-and-resolve
path on a multi-chunk fixture, BOUNDED by a wall-clock budget so a
regression that re-introduces the hang fails this test loudly via
timeout rather than slipping past as a count drift.
Implementation:
- 17-file synthetic fixture: 15 small modules (one function each),
one "realistic dense" complex.ts (30 functions + class + interface),
and an index.ts re-exporting them. Forces cross-chunk import
chains.
- GITNEXUS_CHUNK_BYTE_BUDGET=64 via vi.resetModules forces multi-chunk
parsing on the small fixture.
- Promise.race with 30s timeout: a hang fails as
"exceeded WALL_CLOCK_BUDGET_MS — likely the hang B3 was meant to
prevent", not as a bounds-only inequality (DoD §2.7 distinction —
hang-detector via exception, not regression-mask via inequality).
- Exact .toBe(true) assertions on specific expected symbols
(fn0..fn14, Service, Config, configure, describe, complex0/15/29)
so a silent mid-chunk crash that exits 0 without producing graph
data also fails this test, not just the hang case.
Scope: runs the sequential-fallback path (skipWorkers: true) because
the full real-worker scenario requires a built dist/parse-worker.js
and ~60s wall-clock per run — appropriate for a CI-integration job,
not vitest. The load-bearing invariants pinned here catch the bulk
of B3's concern; the dist-worker swap is a Phase 2 follow-up
documented in the file header.
Resolves PR #1693 review B4: the deferred-extraction reorder (moving
processImportsFromExtracted / Heritage / Routes / Wildcard /
ReceiverTypes from per-chunk to end-of-loop) was proven observably
equivalent by Lane 4 of the production-readiness review. Until now,
the existing suite never asserted cross-chunk graph equivalence,
which lets a future refactor that accidentally tightens the per-chunk
vs end-of-loop coupling silently break cross-chunk resolution.
This test forces multi-chunk parsing on a small fixture by setting
GITNEXUS_CHUNK_BYTE_BUDGET=64 BEFORE the parse-impl module loads
(the budget is captured at module load via vi.resetModules — a future
move to function-scope env reads is U14 in Phase 2). Then runs the
same fixture under a 10MB budget (single chunk) and asserts the two
graphs are byte-identical: same nodeCount, same relationshipCount,
exact .toBe(N) per DoD §2.7.
Fixture: 3-file class hierarchy with cross-file inheritance — Animal
(a.ts) -> Dog extends Animal (b.ts) -> makeDog returns Dog (c.ts).
Forces the resolver to chain imports + heritage across chunks. A
second test pins specific symbol names (Animal, Dog, makeDog, speak,
bark) in the multi-chunk graph so a regression in chunk-boundary
resolution surfaces as a missing-symbol failure with a specific
diagnostic instead of a bare count mismatch.
Resolves PR #1693 review B5: the captures.ts ancestor-walk rewrite
(findSelfOrAncestorOfType[s] + pickFirstNode replacing the prior
findNodeAtRange-from-root path) was semantically equivalent to its
predecessor per Lane 4 of the production-readiness review, but the
existing typescript-captures.test.ts didn't pin the specific sharp
edges where an over-aggressive walk would silently break captures.
This file does.
Each test exercises a capture class whose anchor type is one the
rewrite explicitly handles:
- member call obj.foo() -> @reference.call.member (call_expression
anchor walks to self)
- dynamic import import("./helper") -> raw @import.dynamic gets
decomposed by splitImportStatement into @import.statement with
@import.kind=dynamic + @import.source stripped of quotes
- JSX <Foo /> in .tsx -> @reference.call.free emitted (TSX query
pattern, query.ts:899-905) but @declaration.parameter-count is
NOT synthesized because findSelfOrAncestorOfType('call_expression')
returns null on a jsx_self_closing_element anchor. Pre-rewrite the
range lookup also returned null. Pinning this contract catches
accidental "walk JSX -> outer call" refactors.
- constructor `new Foo(1,2)` -> @reference.call.constructor (new_expression
anchor walks to self)
- named/namespace import + re-export -> @import.statement (one each)
- class method override -> @declaration.method per class, no collapse
- member read obj.foo (no call) -> @reference.read.member
All assertions use exact .toBe(N) per DoD §2.7.
Resolves PR #1693 review M5 (Windows quarantine path-normalization
coverage). worker-pool.ts quarantines paths via a Set<string> keyed by
exact string equality. The existing suite never asserted this contract,
which lets a future "helpfully normalizing" refactor on one side of the
pipeline (caller, worker, or pool) silently break quarantine filtering
on Windows.
This file pins the contract from both directions:
1. Round-trip: a path the caller dispatches with backslashes
(src\bad.ts) flows through starting-file -> death -> quarantine ->
next-dispatch filter verbatim. The replacement worker never sees the
re-dispatched bad path because the pool's pre-dispatch filter
short-circuits it.
2. Non-normalization: quarantining src\poison.ts does NOT filter
src/poison.ts. Whoever changes that contract has to update this test
alongside (the load-bearing assertion catches accidental
path.normalize() calls in the quarantine path).
Runs on every platform — the path strings are test-injected, so the
test exercises the same code path regardless of the host's path.sep.
Used a self-contained FakeWorker that emits {type:'ready'} for U3's
waitForWorkerReady handshake, so the test doesn't depend on the larger
worker-pool-resilience.test.ts harness.
Resolves PR #1693 review L6: operator-facing env vars were either
mentioned inline (GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS) or only
documented via `gitnexus --help`, with no single place to look up
the full set. The new "Environment variables" subsection under the
Quick Start CLI block lists every operator-facing knob with default,
effect, and tuning guidance, matching the names in cli/index.ts
addHelpText post-U2 / U1.
Covers:
GITNEXUS_WORKER_POOL_SIZE (--workers)
GITNEXUS_PARSE_CHUNK_CONCURRENCY (newly real per U1)
GITNEXUS_VERBOSE (--verbose)
GITNEXUS_MAX_FILE_SIZE (--max-file-size)
GITNEXUS_WORKER_SUB_BATCH_TIMEOUT_MS (--worker-timeout × 1000)
GITNEXUS_WORKER_SUB_BATCH_MAX_BYTES
GITNEXUS_CHUNK_BYTE_BUDGET
GITNEXUS_NO_GITIGNORE
GITNEXUS_SKIP_OPTIONAL_GRAMMARS
CLI flag vs env-var precedence is stated explicitly (CLI > env > default)
so operators running long-lived hosts (MCP server, eval-server) know
which channel wins.
Resolves PR #1693 review M6: the existing resilience suite asserts only
the *default value* of maxCumulativeTimeoutMs (5x subBatchIdleTimeoutMs),
not that dispatch actually aborts the offending job when the cumulative
wall-clock budget is exhausted. Without this test, a future refactor
could remove the exhaustion branch in requeueAfterTimeout and the suite
would stay green while the pool sat in retry loops for an hour on a
real production stall.
Scenario:
subBatchIdleTimeoutMs = 100ms
timeoutBackoffFactor = 10
maxCumulativeTimeoutMs = 300ms
Single file, HangingWorker that never responds. First attempt times
out at 100ms (cumulative=100). The next backoff (1000ms, cumulative
1100ms) exceeds the 300ms cap, so requeueAfterTimeout returns
give-up on the first timeout retry and the file goes to the session
quarantine. Asserts:
- pool.getQuarantinedPaths() includes 'src/stuck.ts' after dispatch
- if dispatch rejected, the error is a WorkerPoolDispatchError
(the typed surface that routes to sequential fallback)
Uses a local minimal HangingWorker double rather than the full
action-scripted FakeWorker from worker-pool-resilience.test.ts —
the inverse pattern (always hang) doesn't need the scripted-action
machinery and keeps the test file focused on the one behavior.
Resolves PR #1693 review B1 (GITNEXUS_PARSE_CHUNK_CONCURRENCY documented
in --help but unimplemented).
The chunk loop now pre-fetches chunk file contents up to
`parseChunkConcurrency` chunks ahead of the worker-dispatch cursor so
disk I/O overlaps with worker compute. Worker dispatch itself stays
serial because WorkerPool.dispatch is not reentrant — concurrent calls
would race on the shared per-slot busy/in-flight state, regressing the
hang/resilience work this PR is built on. The pre-fetch path is the
honest interpretation of "concurrent in-flight parse chunks" that the
help text advertises: I/O overlap, not parallel worker dispatch.
Concurrency value resolution:
1. PipelineOptions.parseChunkConcurrency (threaded from CLI)
2. GITNEXUS_PARSE_CHUNK_CONCURRENCY env var
3. Default 2 (matches the help text)
F4 (wildcard-synthesis ordering) is preserved: deferred-state
aggregation runs in chunkIdx order because the for-loop iterates
sequentially after awaiting each chunk's pre-fetched contents.
Cross-chunk processors (processImportsFromExtracted,
synthesizeWildcardImportBindings, etc.) still run only after all
chunks complete — they see deterministic input regardless of
file-read completion order.
Concurrency=1 produces behavior identical to the pure-serial loop;
that's the regression baseline.
New test: parse-impl-chunk-concurrency.test.ts
- Asserts graph output is identical (nodeCount + relationshipCount)
between parseChunkConcurrency=1 and =2 — the critical correctness
invariant. Exact .toBe(N) comparisons per DoD §2.7 (the second run's
counts must equal the first run's exactly).
- Pins specific fixture symbols (foo/bar/Baz) under both
parseChunkConcurrency=1 and the env-fallback (3) path.
- Env-fallback test confirms GITNEXUS_PARSE_CHUNK_CONCURRENCY is
honored when the option is undefined.
Resolves PR #1693 review M2, M3, L1, L5 in a single parse-impl.ts pass:
M2 - Monotonic progress through deferred phase (no more "stuck at 82%")
Previously the deferred resolution stages (imports, heritage, routes,
calls) all emitted percent: 82 — the UI looked frozen for the duration
of the deferred work, which on large repos is several seconds to minutes
and visually identical to the hang PR #1693 set out to fix.
Redistributed:
parse phase: 20-70 (was 20-82)
imports: 70-75
heritage: 75-80
routes: 80-85
calls: 85-95
Each deferred stage now advances through its own band via the existing
per-batch progress callback. Skipped stages (zero deferred input) leave
their band as a no-op jump - the next stage still starts at its own
band, preserving strict monotonicity. The "no parseable files" early
return now jumps to 95 (was 82), and the duplicate "Parsing N files..."
announcement is suppressed when totalParseable === 0 to avoid a
non-monotonic 95 -> 20 regression that pre-existed (uncovered by the
new monotonic test).
M3 - Throughput log gated on `--verbose`, not just NODE_ENV=development
The per-chunk files/s log was gated on `isDev`, so operators running
`gitnexus analyze --verbose` in a production install never saw it.
Now fires when (isDev || isVerboseIngestionEnabled()) — matches the
documented promise that `--verbose` shows tuning observability.
L1 - Typo rename: `chunkChunkStartMs` -> `chunkStartMs`
L5 - `buildExportedTypeMapFromGraph` runs BEFORE `seedCrossFileReceiverTypes`
Previously the seeding branch was reached with `exportedTypeMap.size === 0`
in the worker path (the map was only built far below, AFTER the seeding
branch), so the seed dead-coded itself silently and call resolution
never got the cross-file receiver-type enrichment. Now the map is
populated from the in-progress graph before the seed call; the
post-parse builder remains as a defensive sequential-path fallback,
guarded by `size === 0` so we don't pay the cost twice on the worker
path. Net win: cross-file CALLS edges that previously had no receiver
type now get enriched.
New test: parse-impl-progress-monotonic.test.ts
Asserts the emitted percent stream is strictly non-decreasing across
the parse + deferred phases, and that the deferred band (>=70) is
actually reached. Also pins the "no parseable files" path to exactly
[95] so the 95 -> 20 regression we just fixed can't re-emerge.
Resolves PR #1693 review H1, H2, M4:
H1 - messageerror handler at every dispatch site
V8 deserialization failure on postMessage previously left the message
silently lost; the pool would wait out the idle timeout (default 30s)
instead of treating it as worker death. The dispatch loop now wires
worker.once('messageerror', ...) alongside error/exit and routes through
recoverAndResume so the existing per-slot respawn budget, in-flight
file attribution, and circuit-breaker layers fire as designed.
H2 - resolveAutoPoolSize uses os.availableParallelism()
Mirrors the pattern at capabilities.ts:85 (defaultEmbeddingThreads).
os.cpus().length returns the host CPU count, which over-sizes the pool
on cgroup-limited containers, taskset-restricted runtimes, and CI
runners with explicit CPU quotas. Falls back to os.cpus().length on
Node < 18.14.
M4 - worker-side ready handshake replaces online-trust
parse-worker.ts now emits {type: 'ready'} after all top-of-script
initialization completes, BEFORE the message handler is attached. The
pool's renamed waitForWorkerReady listens for this message under a
bounded WORKER_READY_TIMEOUT_MS (5s) budget instead of trusting Node's
online event - which fires when the worker thread starts, BEFORE the
script body runs, letting init crashes slip past pool startup. ready
is added to WorkerOutgoingMessage with an exhaustiveness-checked
no-op branch in the dispatch handler (defensive: the message is
consumed by waitForWorkerReady before dispatch handlers attach).
messageerror is wired into waitForWorkerReady the same way.
Test scaffolding:
- FakeWorker emits {type: 'ready'} in addition to 'online' so
replacement workers in unit tests don't hit the 5s budget.
- Integration test ad-hoc worker scripts go through a writeReadyWorker
helper that prepends the ready handshake. Tests intending to script
"crash BEFORE ready" can bypass the helper.
61/61 worker-pool unit tests pass; 28/28 integration tests pass.
Resolves PR #1693 review B2 (env-var leak in long-running hosts):
- --workers is now threaded through AnalyzeOptions -> runFullAnalysis
-> PipelineOptions.workerPoolSize -> createWorkerPool's explicit
poolSize arg, bypassing the GITNEXUS_WORKER_POOL_SIZE env channel.
The env var remains as a back-compat fallback inside resolveAutoPoolSize
for operators who set it directly.
- analyzeCommand and wikiCommand snapshot the GITNEXUS_* env vars they
mutate at function entry and restore them in finally. Inner *Impl
extraction keeps the diff surgical (no body re-indent). process.exit(0)
on the CLI success path still terminates the process; restoration
matters for programmatic callers (tests, long-running hosts) reaching
early-return paths or the alreadyUpToDate fast path.
- Tests updated to assert the new behavior:
analyze-worker-pool-size.test.ts: workerPoolSize flows through
runFullAnalysis options; env is not mutated; back-to-back calls
see their own values, not the previous call's leak.
analyze-worker-timeout.test.ts: env IS set during the runFullAnalysis
call (captured via mockImplementation) and restored after, proving
the timeout reaches downstream while the leak fix holds.
- Also addresses L4: afterEach NODE_OPTIONS restore so back-to-back test
runs don't accumulate --max-old-space-size=8192 tokens.
Addresses PR #1693 review B2 (blocker) and L4 (test polish).
- Add afterEach to worker-pool-resilience.test.ts cleaning up the per-test temp
directory created by beforeEach (~25 stale dirs per CI run previously).
- Delete the duplicated describe('worker pool option resolution', ...) block.
Verified the first block (lines 490-532) is a strict superset (includes the
GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS env test the second block omitted),
so deletion loses no test coverage.
Addresses PR #1693 review findings L2 (temp-dir leak) and L3 (duplicate block).
* fix(detect-changes): guard resolveWorktreeCwd against overriding a separately-indexed worktree
When the repo registry entry points to a linked worktree (both main
checkout and worktree indexed separately), resolveWorktreeCwd was
incorrectly replacing the correct worktree repoPath with the server's
main-checkout launch directory. Both share the same canonical root so
the existing same-repo check passed, causing git diff to run from the
wrong directory and return 0 changes (issue #1659).
Fix: early-exit guard — if tryRealpath(repoPath) differs from
tryRealpath(getCanonicalRepoRoot(repoPath)), repoPath is itself a
linked worktree and is returned unchanged. Auto-detection only fires
when repoPath equals the canonical main-checkout root.
Also normalises the launchCanonical comparison in the auto-detect path
to use tryRealpath for cross-platform consistency.
Regression test: 'returns worktreeDir unchanged when repoPath IS a
linked worktree and launchCwd is the main checkout'.
* test(detect-changes): add worktreeA→worktreeB case and assumption comment
Cover the missing case from the production-readiness review:
repoPath = wt-A (indexed), launchCwd = wt-B (server on a different
linked worktree). The guard fires on repoPath being a worktree
regardless of launchCwd, so wt-A is returned unchanged.
Also add an inline comment documenting the assumption that repoPath
is a git root or linked-worktree root (not an arbitrary subdirectory),
as noted in Finding 2 of the review.
* refactor(detect-changes): validate repoPath is a git root before canonical comparison
Instead of relying on a comment asserting repoPath is always a git
root, call getGitRoot(repoPath) first. Only if the result matches
repoPath itself do we call getCanonicalRepoRoot and apply the guard.
This eliminates the over-classification risk for subdirectory repoPath
values and makes the assumption explicit in code. repoCanonical is
shared across both the guard and the auto-detect block.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
User reported 4-5% CPU utilization on a multi-core machine during
ingestion. Two structural reasons:
1. **Pool cap.** `createWorkerPool` resolved size as
`Math.min(8, max(1, os.cpus().length - 1))` — a 16-core box got 8
workers (50% theoretical max). U1 lifts the default to
`min(16, max(1, cores - 1))`, exposes `GITNEXUS_WORKER_POOL_SIZE`
env override, and adds `--workers <N>` CLI flag (`0` disables the
pool for sequential fallback).
2. **Per-chunk extraction serialized the loop.** Per chunk:
dispatch → await workers → main-thread `processImportsFromExtracted`
+ `processHeritageFromExtracted` + `processRoutesFromExtracted`
+ `synthesizeWildcardImportBindings` + `seedCrossFileReceiverTypes`
→ next chunk dispatch. Workers sat idle through every extraction
block. U2 (revised from the plan's pipelined-chunks design) defers
these passes to a single end-of-loop batch. Chunk loop becomes
parse + merge + accumulate. Resolution sees strictly-more-info
(full repo graph) so cross-chunk import/heritage targets resolve at
least as well as before. Memory cost: `deferredWorkerImports`
accumulates across chunks; bounded by total file count, acceptable.
Plan deviation note: the plan called for an in-flight chunk pipeline
(N concurrent dispatches with bounded memory). That design needed
either a `processParsing` API refactor or duplicating its catch-block
fallback in `parse-impl`. The deferred-extraction approach delivers
the same "workers stay busy" outcome with much smaller surface area
and zero changes to `processParsing`. The `GITNEXUS_PARSE_CHUNK_CONCURRENCY`
env var documented in U2 of the plan is therefore not implemented in
this commit; if memory growth from `deferredWorkerImports` becomes
a problem at very-large-repo scale, a bounded sliding-window variant
can land as a follow-up.
Tests:
- New `test/unit/analyze-worker-pool-size.test.ts` covers --workers
validation (5 invalid inputs rejected with exit code 1 + clear
error; valid integers set the env var; `--workers 0` routes to
sequential).
- Extended `worker-pool-resilience.test.ts` with `resolveAutoPoolSize`
scenarios: env override, env=0, env above cap, invalid env fallback,
auto-formula match, integer return type.
- Full unit suite: 6097 / 6127 passed / 30 skipped / 0 failed.
- Full integration suite (second run): 77 / 78 passed / 1 skipped /
0 failed. First run had a known cosmetic flake from an uncaught
worker exception bleeding into the test reporter.
Resilience contract from PR #1693 preserved: per-slot respawn budget,
circuit breaker, quarantine, authoritative in-flight tracking,
cumulative timeout budget — all unchanged.
New env vars surfaced in --help: GITNEXUS_WORKER_POOL_SIZE,
GITNEXUS_PARSE_CHUNK_CONCURRENCY (reserved for future bounded
pipelining).
* fix(lbug): probe-then-load FTS extension on Windows (#1690)
The Windows skip-on-process.platform==='win32' guard in pool-adapter.ts
hard-skipped loadFTSExtension() for every Windows host, even when the
FTS extension binary was already present locally at
~/.lbdb/extension/<version>/win_amd64/fts/libfts.lbug_extension.
That left BM25 silently degraded on Windows hosts that had a working
extension on disk, with no error path — `gitnexus doctor` still reported
FTS as available, but query returned 0 BM25 hits.
This patch adds hasLocalWinFtsExtension() which probes
~/.lbdb/extension/*/win_amd64/fts/ before the Windows skip. When a binary
is on disk we call loadFTSExtension(..., { policy: 'load-only' }); the
crashing install path documented in #1199 / #1217 is never exercised at
query time, and LadybugDB's version-specific resolution combined with
the ExtensionManager's tryLoad try/catch handles stale or zero-byte
sibling version dirs cleanly (no dlopen attempted on a stale binary).
When no binary is on disk at all, we fall back to the upstream skip so
install-time SIGSEGV continues to be avoided.
Verified on Windows 10 + Node 22.19.0 + gitnexus 1.6.5 +
@ladybugdb/core 0.16.1 with the FTS extension cached at 0.16.0:
* BM25 timing goes from 0 → ~250-326ms on previously-zero queries
* gitnexus context / impact / cypher unaffected
* Adversarial-mixed-state run (real 0.16.0 binary + zero-byte stubs at
0.15.0, 0.16.1, 0.17.0): exits 0, no SIGSEGV, FTS resolves to the
real 0.16.0 binary, BM25 returns real hits
* Stub-only state at the resolution path (0.16.0, zero-byte): exits 0,
emits "FTS extension unavailable; load-only policy: extension not
pre-installed", FTS marked unavailable cleanly via markUnavailable
in extension-loader.ts — no silent greenlight
Closes#1690
* test(lbug): cover hasLocalWinFtsExtension probe + format pool-adapter
- Export hasLocalWinFtsExtension and add lbug-pool-win-fts-probe.test.ts
with 7 cases against a real tmpdir + os.homedir spy:
* missing ~/.lbdb/extension dir -> false
* extension root present but no version dirs -> false
* one version dir with binary present -> true
* zero-byte stub at probe path -> true (LOAD failure handled downstream)
* multi-version with binary only in a non-first dir -> true
* multi-version with no binary anywhere (Nix/Bazel/MDM tree) -> false
* fs.readdir throws (EACCES) -> false
The Windows conditional in doInitLbug / initLbugWithDb is intentionally
not unit-isolated: it reduces to `probe ? load : true` over a fully
constructed lbug.Database + Connection pool, which the
test/integration/lbug-pool*.test.ts suites already exercise on the
windows-latest CI matrix.
- Apply prettier format to the fs.stat() call in pool-adapter.ts,
resolving the quality/format CI failure surfaced by gitnexus/autofix.
Addresses DoD §2.7 test-coverage blocker raised in the production-
readiness review on #1692, and the dir-exists-no-file regression case
raised on #1690.
Refs #1690.
---------
Co-authored-by: Gergő Magyar <gergomagyar@icloud.com>
Adds 6 new real-worker integration tests covering the PR #1693
resilience layers + fixes 3 follow-on bugs surfaced while writing them.
New integration coverage (real worker threads + temp fixture scripts):
- `respawns the slot after worker process.exit and finishes the work on
the replacement` — exercises Layer 1 auto-respawn + Layer 3 quarantine
through real IPC.
- `attributes exactly via authoritative starting-file message on worker
crash` — Layer 4 end-to-end: starting-file message → exact quarantine
attribution (not the items[0] heuristic).
- `quarantine filters subsequent dispatches without sending to a worker`
— second dispatch's sub-batch payload audited via filesystem; the
quarantined path is never sent across the message channel.
- `drops a slot after maxRespawnsPerSlot and continues on the survivor`
— 2-slot pool, slot dies twice past budget, survivor finishes
re-queued remainder.
- `trips the circuit breaker on cascading per-slot consecutive failures`
— single-slot pool, dies on every job, breaker trips after
consecutiveFailureThreshold with WorkerPoolDispatchError carrying
the cumulative quarantine.
- `survives a worker error event (uncaught throw) the same as a
process.exit` — validates recoverAndResume on the errorHandler path
via a real worker `throw` (not just process.exit).
Bug fixes uncovered while writing these tests:
1. **Stack-overflow recursion in runWorker's no-worker branch** —
`if (!worker) { ...; wakeIdleSlots(); maybeDone(); }` recursed
indefinitely when multiple slots were mid-respawn simultaneously
(wakeIdleSlots → runWorker → no worker → wakeIdleSlots → …).
Removed the wakeIdleSlots call: the slot's own respawn IIFE owns
runWorker post-respawn, and other slots will pick up work via
finishJob's runWorker.
2. **requeueAfterTimeout dispatched work before respawn completed** —
the F2 fix had `requeueAfterTimeout` `void`-discarding
`handleWorkerDeath`, so the `!shouldContinue` IIFE had no way to
know when the respawn finished. New design: `requeueAfterTimeout`
returns a `TimeoutDecision` discriminated union; the IIFE owns
the death-and-respawn-and-dispatch orchestration in an async
closure so it can `await handleWorkerDeath` and then call
`runWorker` deterministically.
3. **Stalled-singleton + protocol-error + replacement-startup-crash
tests** had stale contracts predating the resilience refactor. The
stalled-singleton no longer rejects (it quarantines + resolves
`[]`); the protocol-error rejection message now mentions
"circuit breaker tripped"; the replacement-startup-crash test
documents the known `waitForWorkerOnline` race (online fires
before the worker's main script runs, so a top-level throw looks
like a successful spawn) — the test asserts the file is
quarantined via the second-idle-timeout give-up path.
Full suite: 334 files / 8982 passed / 43 skipped / 0 failed (second
run; first run had a Vitest-reported flake from an uncaught worker
exception bleeding into the test report — repeated runs are clean).
Walks through every finding from ce-code-review run
20260519-094648-3549cf5e. All 12 picked Apply.
Critical:
- F1 — Layer 5 cumulative-timeout exhaustion no longer silently drops
the rest of the job. `requeueRemainder` is now invoked before
`handleWorkerDeath` in both Layer 5 and singleton-final-fail give-up
paths so non-quarantined items get re-tried by another worker.
- F2 — idle-timer recovery overhaul. `!shouldContinue` branch no
longer calls `replaceWorker` (double-spawn race with the
`handleWorkerDeath` inside `requeueAfterTimeout`). `shouldContinue`
branch now enforces `maxRespawnsPerSlot` before respawning, closing
the budget-bypass for the timeout-retry path. Also fixes premature
`maybeDone` by simplifying the bookkeeping.
- F3 — `requeueRemainder` no longer pre-charges `cumulativeTimeoutMs`
by `job.timeoutMs`. The death itself consumed no budget, so the
next `requeueAfterTimeout` was double-billing the first attempt.
- F4 — `WorkerPool.getQuarantinedPaths` is now optional on the
interface, matching the defensive `?.()` call site and the existing
mocks. Removes the contract-vs-callsite contradiction.
- F5 — per-job unattributed-death tracking. When a worker dies with
no exclusion attribution, `requeueRemainder` tracks death count per
`startIndex`. First time: re-queue intact. Second time: quarantine
items[0] as best guess, or drop the job entirely when items lack
paths. Bounds the death loop the original design admitted to.
- F6 — per-slot consecutive-failure counter. Replaces the pool-wide
scalar so a chronically-failing slot trips the breaker on its own
streak instead of being masked by another slot's successes.
Smaller:
- F7 — exhaustiveness `never` check on `WorkerOutgoingMessage` union.
- F8 — recursive `runWorker` on fully-quarantined jobs converted to
a while-loop.
- F9 — `tripBreaker` calls `reject(err)` BEFORE awaiting
`worker.terminate()`. A stuck terminate no longer blocks the caller.
- F10 — `parsing-processor.ts` quarantine log de-duplicates per pool
instance via a `WeakMap`. Only newly-quarantined paths are logged
in each chunk; the per-chunk count still surfaces via progress.
- F11 — extract `firstPath` local in `requeueAfterTimeout`; eliminates
double `itemPath` call and the `unknown as string` cast.
Tests (F12, 6 new):
- crash-error event path (errorHandler).
- F5 drop-branch coverage via items without `.path`.
- Common-case unattributable crash falling back to items[0] heuristic.
- `replaceWorker` startup failure (workerFactory emits 'exit' before
'online').
- All-slots-dropped breaker trip.
- `GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS` env override.
Residual gap (deferred): no unit test exercises the Layer 5
cumulative-budget runtime path — requires fake-timer interleaving
with FakeWorker that's too brittle for this iteration. Tracked.
Unit suite: 257 files / 6056 passed / 30 skipped / 0 failed.
Restructures `createWorkerPool` so a single bad file no longer kills the
pool for the rest of an analyze run. Five interlocking layers:
1. **Auto-respawn on error/exit** — worker death triggers `replaceWorker`
on the same slot, bounded by `maxRespawnsPerSlot` (default 3). The slot
is dropped from rotation when the budget is exhausted; other slots
keep running.
2. **Circuit breaker** — replaces the permanent `poolBroken=true` with a
consecutive-failure counter. The pool only trips after
`consecutiveFailureThreshold` deaths (default `max(3, poolSize)`) with
no successful job in between. A successful job resets the counter so
transient bursts of bad files don't escalate.
3. **Session-scoped file quarantine** — paths identified as the in-flight
file at the moment of a worker death are added to a `Set<string>` on
the pool. `dispatch()` filters quarantined items up front (they never
reach a worker again this pool lifetime). Exposed via the new
`WorkerPool.getQuarantinedPaths()` so callers can log/route them.
`processParsing` surfaces the per-chunk quarantine summary alongside
the existing fallback-exclusion log.
4. **Authoritative in-flight tracking** — `parse-worker.ts` emits
`{type:'starting-file', path}` before each file. The pool tracks this
per slot and uses it for crash attribution, falling back to the
`items[lastProgress]` heuristic only when no starting-file has been
observed (very-early crash, older worker build). Closes the
reorder/race concerns raised by reviewers C1 and R3 in the earlier
review run.
5. **Per-job cumulative timeout budget** — each `WorkerJob` tracks the
total wall time spent across attempts/splits/retries. When the budget
is exhausted (default 5x `subBatchIdleTimeoutMs`), the pool surfaces
the in-flight path instead of letting exponential backoff balloon
into multi-hour stalls.
Cross-layer wiring: a new `wakeIdleSlots` helper kicks any non-busy live
slot when items are requeued (after a death or split-retry), so a dropped
slot doesn't strand work in the queue. `recoverAndResume` consolidates
the per-job teardown shared by the three in-pool death sites (`error`,
`exit`, msg-channel `error`).
New env knobs: `GITNEXUS_WORKER_MAX_RESPAWNS_PER_SLOT`,
`GITNEXUS_WORKER_MAX_CUMULATIVE_TIMEOUT_MS`,
`GITNEXUS_WORKER_CONSECUTIVE_FAILURE_THRESHOLD`.
New `WorkerPoolOptions.workerFactory` injection point for unit tests.
Tests: 12 new unit tests using a FakeWorker mock cover quarantine
seeding, slot-respawn, slot-drop after budget, breaker trip + reset,
and quarantine filtering. Plus option-resolution tests for the three
new env vars. All 19 worker-pool/-fallback/-options tests pass; full
unit suite 6040 passed / 30 skipped / 0 failed.
- Use cause-neutral "worker-excluded" label in skip messages and tests now
that worker error/exit paths share the same exclusion contract as
singleton-timeout (correctness + maintainability reviewers).
- Add JSDoc to findSelfOrAncestorOfType{s} explaining the parent-walk
short-circuit vs root-DFS fallback (maintainability reviewer).
WorkerPoolDispatchError previously surfaced the stalled path only for the
singleton-timeout final-fail branch. Worker `error` and `exit` events (and
the msg-channel `error` reply) fell back to plain `Error`, so the sequential
fallback re-attempted every file in the active job — re-hanging on the same
pathological file when the worker crashed mid-parse.
Lift the in-flight-file inference into `inFlightExcludePath(job, lastProgress)`
and wire it into the three remaining in-pool failure sites. `lastProgress` is
already in `runWorker` scope, so `items[lastProgress]` (the next file the
worker was about to acknowledge) is the best single guess at the culprit;
earlier files are still re-tried sequentially. Returns `[]` when no path is
determinable (`lastProgress >= items.length`, or path missing/non-string) so
sequential retries the whole job.
Replacement-worker startup failures stay plain `Error` (no job context); the
result-before-flush protocol bug stays plain `Error` (code fault, not file).
Tests cover the three new exclusion paths plus a negative test confirming
non-WorkerPoolDispatchError throws fall through to full sequential retry.
