fix(worker): analyze native worker aborts

[ChamHerry]

Summary

• Retire timed-out parse workers instead of immediately terminating a thread that may still be inside native tree-sitter parser state.
• Skip generated Monaco editor worker bundles (monaco-workers) by default, while keeping .gitnexusignore negation overrides available for explicit opt-in.
• Add regression coverage for timeout retirement and Monaco-worker ignore behavior.

Root cause

gitnexus analyze could abort with libc++abi: terminating due to uncaught exception of type Napi::Error when parsing generated Monaco worker bundles. Retrying/terminating worker threads while native parser frames were still active could make the abort process-fatal.

Validation

• npm test — 9432 passed, 1 skipped
• npx tsc --noEmit
• npm run build
• npx prettier --check src/config/ignore-service.ts src/core/ingestion/workers/worker-pool.ts test/unit/ignore-service.test.ts test/unit/worker-pool-timeout-retire.test.ts
• npx eslint src/config/ignore-service.ts src/core/ingestion/workers/worker-pool.ts test/unit/ignore-service.test.ts test/unit/worker-pool-timeout-retire.test.ts — 0 errors; existing warnings remain in test/unit/ignore-service.test.ts
• Live repro: node gitnexus/dist/cli/index.js analyze --index-only --workers 4 --worker-timeout 5 /Users/wangxc/Code/keep succeeded with Repository indexed successfully
• gitnexus detect_changes --scope staged showed high risk due worker-pool handler flows; changed files were limited to the intended fix/test scope.

Notes

Local unrelated working-tree changes were intentionally excluded from this PR: .mcp.json deletion and CLAUDE.md modification.

[vercel]

Someone is attempting to deploy a commit to the NexusCore Team on Vercel.

A member of the Team first needs to authorize it.

[ChamHerry]

CI Gate is currently blocked because tests / packaged install smoke (windows-latest) was cancelled during npm install -g; other quality/typecheck/lint/tests/scope-parity checks passed. I tried to rerun the failed jobs but GitHub requires repository admin rights for this run. Could a maintainer please rerun the failed CI job?

[github-actions]

Claude finished @magyargergo's task in 7m 40s —— View job

---

Production-Readiness Review — PR #1833 fix(worker): analyze native worker aborts

---

Review bar for this PR

The PR must actually prevent process-fatal libc++abi: terminating due to uncaught exception of type Napi::Error without introducing new hangs, leaks, silent partial indexing, or ignore-rule regressions. Tests must fail against the broken old behavior. CI must be green (or failures must be unambiguously infra noise). The Definition of Done in AGENTS.md and the PR-specific DoD from the reviewer prompt are the minimum bar.

---

Problem being solved

gitnexus analyze could abort the entire Node process with Napi::Error when a timed-out parse worker was terminated while native tree-sitter parser frames were still active on its stack. Issues #1665 and #1304 document the symptom. The PR addresses this via two mechanisms: (1) retire rather than immediately terminate timed-out workers, waiting for a safe JS-visible signal before calling terminate(); (2) skip monaco-workers directories by default since they contain large generated Monaco editor bundles whose file names don't match existing extension filters.

---

Current PR state

• Head SHA: a497311 — confirmed to match the current working tree (git rev-parse HEAD → a49731176d8efe31a548b84d7a5782dffe9396b4)
• Changed files: 4 (worker-pool.ts +84/-14, ignore-service.ts +1/-8, ignore-service.test.ts +13/-0, worker-pool-timeout-retire.test.ts +93/-0) — matches PR metadata
• Diff size: +191/-22 — matches PR metadata

---

Merge status and mergeability

• CI — "CI" workflow: cancelled (run 26441561533). Per PR comment from ChamHerry, the tests / packaged install smoke (windows-latest) job was cancelled during npm install -g, not due to a test failure. The cancellation happened before tests ran. Windows packaged install smoke result is unknown.
• All other CI checks (11/12): passed — Gitleaks, Dependency Review, PR Autofix, CodeQL, Docker Build & Push, PR Description Check, PR Conventional Labeler × multiple runs.
• Vercel authorization warning: bot noise; not a required CI gate.
• Mergeability conflicts: none visible.
• Unresolved review threads: none at review time.

The missing Windows packaged install smoke result is unresolved. It must be rerun before merge — a maintainer needs to do this since ChamHerry lacks admin rights on this run.

---

Repository history considered

• Napi::Error #1665: Target symptom — Napi::Error abort on repos with tensorflow lite micro / executorch. Logs show 16,882 files, termination during chunk 11 after ~35s, worker idle timeout then native abort. The Monaco bundle angle in this PR is one documented path; whether Napi::Error #1665 involved Monaco bundles specifically is not established. The timeout-retirement fix is the mechanism that addresses the root cause regardless of which file triggered the native stall.
• Error when parsing #1304: Native abort after worker idle timeout on a very large C# class — same root cause (terminate during native parse). This PR's retirement path directly addresses this.
• [bug] Worker pool split-and-retry path silently exits without writing index on default 30s timeout (1.6.4-rc.41) #1278/fix: prevent premature pool resolution in worker split-and-retry path #1321: Split-and-retry could exit with code 0 and no index written due to false idle state in accounting. This PR modifies the timeout path; regression analysis below.
• [test] Worker pool retry test flakes under load — 150ms idle timeout too tight #1323/fix(test): widen worker pool retry timeout to prevent CI flake (#1323) #1354: Prior worker-pool retry tests flaked under CI load with tight timing. The new test uses 20ms with a fake EventEmitter (no real worker startup cost).
• GitNexus analyze gets stuck at 49% during “Resolving calls”. #1741/feat(progress): add per-language progress reporting to scope-resolution phase #1813: Large-repo analyze progress work. This PR touches the same timeout/accounting path but doesn't touch progress reporting directly.

---

Branch hygiene assessment

Clean. One focused commit, four files in the worker-pool/ignore/test scope. No lockfile churn, no generated artifacts, no merge-from-main commits, no unrelated changes. The commit message is conventional and matches the diff. Local unrelated changes (.mcp.json, CLAUDE.md) explicitly excluded and confirmed absent from the diff.

Hidden Unicode audit: grep -P scan for bidi controls (U+202A–202E, U+2066–2069, U+200B–200D, U+FEFF) across all four changed files — none found. git diff --check — clean.

---

Understanding of the change

Lane 1 — Worker-pool timeout retirement (worker-pool.ts)

Pre-PR timeout path (both give-up and retry branches): called removeWorkerFromSlot(workerIndex, 'terminate', reason) → await existing.terminate() while the worker might still be inside tree-sitter native code.

Post-PR: both the give-up path (via handleWorkerDeath(..., 'retire')) and the idle-timeout retry paths now call removeWorkerFromSlot(workerIndex, 'retire', reason).

removeWorkerFromSlot (lines 799–812):

const existing = workers[workerIndex];
workers[workerIndex] = undefined;
if (!existing) return;
if (mode === 'retire') {
  retireWorkerAfterTimeout(existing, workerIndex, reason);
  return;
}
await existing.terminate().catch(() => undefined);

retireWorkerAfterTimeout (lines 761–797): adds the worker to retiredWorkers, installs message/error/exit/messageerror listeners, calls unref(), and only calls terminate() after receiving sub-batch-done, result, or error (the "worker returned to JS" signals) or after messageerror.

Normal dispatch listeners (handler, errorHandler, exitHandler, messageErrorHandler) are removed via cleanup() before removeWorkerFromSlot is called in all timeout paths (line 1258), so there are no listener races.

Lane 2 — Monaco-workers ignore (ignore-service.ts)

monaco-workers added to DEFAULT_IGNORE_LIST at line 90. shouldIgnorePath checks every path segment, so public/monaco-workers/json.worker.js is caught at the monaco-workers segment. isHardcodedIgnoredDirectory uses DEFAULT_IGNORE_LIST.has(name), so directory traversal pruning also fires. The createIgnoreFilter's negation path (from #771) takes precedence: hasExplicitUnignore is checked before the hardcoded list, so !public/monaco-workers/ in .gitnexusignore re-enables traversal and indexing.

---

Findings

---

F1 — pool.terminate() does not clean up retired workers [MEDIUM / Confirmed]

Risk: retiredWorkers is a Set<Worker> at the pool closure scope (line 601). pool.terminate() (lines 1547–1558) iterates workers[] and activeSlots but never touches retiredWorkers. A worker retired due to a native stall that never emits sub-batch-done/result/error/exit will remain in retiredWorkers indefinitely, with active event listeners installed on it. Since unref() is called, the stalled thread does not prevent process exit — for short-lived CLI analyze runs this is acceptable. However:

• The DoD for this PR explicitly requires: "Pool termination still cleans up active workers and does not leave retired native workers unbounded across repeated analyzes." This requirement is violated.
• tripBreaker() (lines 855–865) similarly terminates workers[] but never touches retiredWorkers.
• In MCP server or programmatic long-running contexts where createWorkerPool is called repeatedly, retired-but-stalled workers accumulate across pool lifetimes.

Evidence checked: grep retiredWorkers worker-pool.ts — only three references: declaration (line 601), retiredWorkers.add(worker) in retireWorkerAfterTimeout (line 766), retiredWorkers.delete(worker) in cleanupRetired (line 772). terminate() at lines 1547–1558 — no reference to retiredWorkers. tripBreaker() at lines 855–865 — no reference to retiredWorkers.

Recommended fix:

// In terminate():
await Promise.all([
  ...workers.map((w) => w?.terminate().catch(() => undefined)),
  ...[...retiredWorkers].map((w) => w.terminate().catch(() => undefined)),
]);
retiredWorkers.clear();

And similarly in tripBreaker(). A retire-grace-period upper bound (e.g., a long-lived setTimeout that force-terminates after N seconds) would be belt-and-suspenders for the truly-stuck case.

Blocks merge: yes — confirmed DoD violation. Fix this →

---

F2 — Test does not cover retired-worker cleanup on pool.terminate() [MEDIUM / Confirmed]

Risk: worker-pool-timeout-retire.test.ts asserts (line 88) that terminateCalls === 0 after the timeout retirement path. This correctly proves no immediate terminate. But the test calls pool.terminate() in the finally block (line 90) and does not assert what happens to the retired worker instance after that. Since pool.terminate() currently does not touch retiredWorkers, terminateCalls remains 0 after pool.terminate() — meaning the test passes even with the F1 leak. The test cannot detect the DoD violation in F1.

The test also does not cover the "retired worker eventually terminated after sub-batch-done" lifecycle: there is no case in the fake worker where worker 0 emits sub-batch-done after a delay, followed by an assertion that terminateCalls === 1.

Evidence checked: worker-pool-timeout-retire.test.ts lines 80–93 — the only assertions after pool.terminate() are on results, instances.length, unrefCalls, and terminateCalls. No assertion that retiredWorkers is empty or that terminate() was called on retired workers during pool shutdown.

Recommended fix: Add a test case where pool.terminate() is called while a worker is in the retired set and assert that terminateCalls becomes 1 (i.e., the retired worker was terminated by pool.terminate()). Add a second case where a retired worker emits sub-batch-done asynchronously and assert it is then terminated.

Blocks merge: yes — the test that should prove the DoD cannot detect a known DoD violation in the current code.

---

F3 — Accidental immediate termination: NOT CONFIRMED [Severity: N/A]

Risk (as described in reviewer prompt): removeWorkerFromSlot might call existing?.terminate() before checking mode === 'retire', defeating the fix.

Evidence checked: Lines 799–812 of worker-pool.ts:

const existing = workers[workerIndex];
workers[workerIndex] = undefined;
if (!existing) return;
if (mode === 'retire') {
  retireWorkerAfterTimeout(existing, workerIndex, reason);
  return;           // ← returns here, never reaches terminate()
}
await existing.terminate().catch(() => undefined);

The mode check is correctly placed before terminate(). For retire mode, terminate() is never called. This specific risk is not present in the current code.

Blocks merge: no.

---

F4 — Listener races (retired vs active handlers): NOT CONFIRMED [Severity: N/A]

Risk (as described in reviewer prompt): Retired worker could emit messages that reach normal dispatch handlers and corrupt result state.

Evidence checked: The idle timer callback (lines 1255–1381) calls settled = true; cleanup() before any removeWorkerFromSlot call. cleanup() (lines 1215–1221) removes handler, errorHandler, exitHandler, messageErrorHandler from the worker. Only after cleanup does retireWorkerAfterTimeout install its own isolated onRetiredMessage / onRetiredError / onRetiredExit / onRetiredMessageError listeners. The retired listeners do not call job result callbacks, do not update busySlots/activeWorkers/inFlightProgress, and do not resolve/reject the dispatch promise. No listener races present.

Blocks merge: no.

---

F5 — Worker-pool accounting (#1278/#1321 regression): NOT CONFIRMED [Severity: N/A]

Risk: Missing activeWorkers-- / busySlots.delete() could cause hangs or early resolve.

Evidence checked:

• Give-up path (lines 1267–1281): IIFE opens with activeWorkers--; busySlots.delete(workerIndex) before awaiting handleWorkerDeath. Subsequent runWorker/wakeIdleSlots/maybeDone calls are only reached if not stopped.
• Retry path (lines 1296–1379): activeWorkers--; busySlots.delete(workerIndex) in a finally block (lines 1362–1363), ensuring decrement happens even if replaceWorker throws or tripBreaker fires. The two paths are mutually exclusive (give-up returns early, retry doesn't reach give-up code).
• maybeDone (lines 867–876): guards on jobs.length === 0 && activeWorkers === 0. During either IIFE, activeWorkers is still > 0 (decremented at end), preventing premature resolve.
• #1278/#1321 fix path: no regression visible. The accounting discipline mirrors the pre-PR structure for non-timeout deaths.

Blocks merge: no.

---

F6 — Monaco ignore applies to any directory segment named "monaco-workers" [LOW / Confirmed]

Risk: Any user-authored directory named monaco-workers anywhere in the repo will be silently ignored by default, not just generated Monaco bundles. The check (DEFAULT_IGNORE_LIST.has(part) in shouldIgnorePath and DEFAULT_IGNORE_LIST.has(p.name) in childrenIgnored) matches the bare directory name.

Evidence checked: shouldIgnorePath (lines 285–339): path split on /, then for-each against DEFAULT_IGNORE_LIST. A file at src/monaco-workers.ts is safe (segment is monaco-workers.ts, not monaco-workers). A directory at services/monaco-workers/ is caught. The negation override works correctly for users who want to opt in.

Assessment: This is an accepted product tradeoff with the same precedent as generated, dist, coverage, __tests__, etc. — directories where the heuristic value outweighs the false-positive risk. The PR description documents the rationale. Negation override is tested and functional. This does not block merge but should be documented in a release note.

Blocks merge: no.

---

F7 — .gitnexusignore negation override: NOT CONFIRMED incomplete [Severity: N/A]

Risk (as described in reviewer prompt): Negation might work for ignored() but not childrenIgnored().

Evidence checked: createIgnoreFilter (lines 435–484): both ignored() and childrenIgnored() call hasExplicitUnignore(ig, rel) before checking DEFAULT_IGNORE_LIST. The test at lines 327–332 asserts filter.childrenIgnored(mkPath('public/monaco-workers')) returns false and filter.ignored(mkPath('public/monaco-workers/json.worker.js')) returns false under !public/monaco-workers/. Both pass. The existing !__tests__/ tests (lines 287–293) also cover childrenIgnored.

Blocks merge: no.

---

F8 — Test timer fragility (20ms): LOW / Acceptable

Risk: subBatchIdleTimeoutMs: 20 could flake under CI load.

Evidence checked: Worker 0 (TimeoutThenHealthyWorker.id === 0) uses queueMicrotask for the ready handshake but returns immediately on sub-batch without posting any message. There is no real Worker startup involved — the factory injects a pure EventEmitter. The 20ms setTimeout races against queueMicrotask, which always resolves within the same microtask checkpoint, long before 20ms. The timing dependency is between a setTimeout(20ms) and microtask-delivered messages, which is well-established behavior on all platforms. Prior flakes (widened in #1354 to 500ms) involved real worker thread startup latency, which is absent here.

Blocks merge: no.

---

PR-specific assessment sections

A. Native worker timeout retirement

The core fix is correctly implemented. removeWorkerFromSlot correctly branches on mode === 'retire' before calling terminate(). retireWorkerAfterTimeout correctly installs isolated retired-lifecycle listeners before unref(). terminateWhenBackInJs() fires only on sub-batch-done, result, error (safe JS-return signals), or messageerror. A retired worker that never returns to JS remains unref'd and does not prevent process exit — acceptable for CLI, but pool.terminate() must be patched to clean it up (F1).

B. Worker-pool accounting and lifecycle

No double-decrements, no hangs, no early resolve found. The give-up and retry paths are mutually exclusive and each handle activeWorkers/busySlots before any downstream runWorker/wakeIdleSlots/maybeDone call. #1278/#1321 accounting discipline is preserved. The generation counter (U12, slotGenerations) is correctly incremented atomically with the worker slot swap in replaceWorker (line 842).

C. Monaco-worker ignore behavior

DEFAULT_IGNORE_LIST addition flows through both shouldIgnorePath (file-level) and isHardcodedIgnoredDirectory / childrenIgnored (directory-traversal pruning). Case sensitivity: the DEFAULT_IGNORE_LIST.has(part) check is case-sensitive; monaco-workers matches exactly. Windows path normalization is done via replace(/\\/g, '/') before splitting. Negation works for both ignored and childrenIgnored via hasExplicitUnignore.

D. Tests and validation

The new unit test correctly asserts terminateCalls === 0 immediately after timeout retirement — this WOULD FAIL against the old code (which called terminate() unconditionally). However, as noted in F2, it does not cover the pool.terminate() cleanup path and cannot detect the F1 DoD violation. The Monaco negation test (lines 327–332) covers both childrenIgnored and ignored. The isHardcodedIgnoredDirectory test (line 225) adds the monaco-workers entry.

E. Cross-domain risk

Worker-pool lane: the core retirement fix is correct; the pool.terminate() cleanup omission is a confirmed DoD violation that must be fixed before merge. Ignore-service lane: correct and well-tested, no regressions.

---

Back-and-forth avoided by verifying

• Suspected immediate-terminate-before-retire (reviewer prompt risk Add support for Ollama as a local inference backend #1): inspected removeWorkerFromSlot directly — not present.
• Suspected listener races (reviewer prompt risk readme #3): confirmed cleanup() fires before retirement in all timeout paths.
• Suspected accounting regression ([bug] Worker pool split-and-retry path silently exits without writing index on default 30s timeout (1.6.4-rc.41) #1278/fix: prevent premature pool resolution in worker split-and-retry path #1321): traced give-up IIFE and retry IIFE exhaustively — no regression.
• hasExplicitUnignore behavior under !public/monaco-workers/: confirmed ig.test('public/monaco-workers/').unignored returns true and flows correctly through childrenIgnored.

---

Open questions that remain only if unavoidable

1. Is GitNexus analyze used in long-running MCP server contexts where the same pool object persists across multiple analyzes? If yes, F1 is more urgent (workers accumulate across pool lifetimes). If no (new pool per analyze run, process-exit-per-CLI-session), unref alone may be sufficient in practice — but the DoD still requires terminate() to clean up retired workers.

2. Windows packaged install smoke test: infrastructure cancellation, not a code failure — but the result is unknown and must be rerun by a maintainer before merge.

---

Final verdict

not production-ready

The core retirement mechanism — avoiding immediate terminate() on timed-out workers — is correctly implemented and the test correctly validates the no-immediate-terminate invariant. However, two confirmed DoD violations block merge: (1) pool.terminate() does not clean up retiredWorkers, directly violating the stated DoD requirement that "Pool termination still cleans up active workers and does not leave retired native workers unbounded"; (2) the unit test cannot detect this omission and would pass unchanged even if retired workers were leaked on shutdown. Additionally, the "CI" workflow run (which includes the Windows packaged install smoke test) was cancelled at the infrastructure level and must be rerun by a maintainer — the Windows result is unknown and this native-worker fix is explicitly untested on non-macOS platforms. Fix F1 (patch terminate() and tripBreaker() to also terminate and clear retiredWorkers), add the corresponding test coverage for F2, rerun the cancelled CI job, and this PR will be in good shape.

---

[magyargergo]

@ChamHerry could you please look into claude's findings?

[ChamHerry]

Addressed Claude findings F1/F2 in commit 6248aec.

Changes:

• Retired parse workers are now tracked as lifecycle records with idempotent cleanup/termination.
• pool.terminate() now awaits cleanup for both live slot workers and retired workers.
• tripBreaker() now fire-and-forgets cleanup for both live slot workers and retired workers while preserving the existing non-blocking rejection behavior.
• Timeout retry/give-up still uses retire mode and does not immediately terminate timed-out native parser workers during normal recovery.
• Added tests for no-immediate-terminate + shutdown cleanup, safe JS-visible return cleanup, and circuit-breaker cleanup.

Validation:

• npx vitest run test/unit/worker-pool-timeout-retire.test.ts — passed, 3 tests
• npx vitest run test/unit/worker-pool-timeout-retire.test.ts test/unit/worker-pool-resilience.test.ts test/unit/worker-pool-cumulative-timeout.test.ts test/unit/worker-pool-slot-generation.test.ts — passed, 34 tests
• npx tsc --noEmit — passed
• npm run build — passed
• npx prettier --check src/core/ingestion/workers/worker-pool.ts test/unit/worker-pool-timeout-retire.test.ts ../docs/todo/pr-1833-retired-worker-cleanup-plan.md — passed
• npx eslint src/core/ingestion/workers/worker-pool.ts test/unit/worker-pool-timeout-retire.test.ts — passed
• gitnexus detect_changes --scope staged — medium risk; affected worker handler/error/exit/messageerror cleanup flows as expected

Note: I also ran the full npm test suite twice. Both runs had exactly one unrelated test/unit/hooks.test.ts parseHookOutput-null failure, and each failed hook test passed when rerun in isolation. All other full-suite tests passed in those runs.

[github-actions]

CI Report

✅ All checks passed

Pipeline Status

Stage	Status	Details
✅ Typecheck	success	tsc --noEmit
✅ Tests	success	unit tests, 3 platforms
✅ E2E	success	gitnexus-web changes only

Test Results

Tests	Passed	Failed	Skipped	Duration
9921	9917	0	4	607s

✅ All 9917 tests passed

4 test(s) skipped — expand for details

• PHP pipeline benchmark > scales with file count (workers enabled)
• Ruby pipeline benchmark > scales with file count (workers enabled)
• Rust pipeline benchmark > scales with file count (workers enabled)
• buildTypeEnv > known limitations (documented skip tests) > Ruby block parameter: users.each { |user| } — closure param inference, different feature

Code Coverage

Tests

Metric	Coverage	Covered	Base	Delta	Status
Statements	79.71%	34508/43289	N/A%	—	🟢 ███████████████░░░░░
Branches	68.22%	22016/32270	N/A%	—	🟢 █████████████░░░░░░░
Functions	84.76%	3572/4214	N/A%	—	🟢 ████████████████░░░░
Lines	83.2%	31095/37373	N/A%	—	🟢 ████████████████░░░░

---

_{📋 View full run · Generated by CI}

[magyargergo]

I need to merge #1838 and update this PR afterward