From ae93706b8df56625e874a220f11d1f1e1fe5ab9f Mon Sep 17 00:00:00 2001 From: Giulio Leone Date: Mon, 11 May 2026 17:19:59 +0200 Subject: [PATCH] fix(ingestion): optional per-parse timeout to prevent worker deadlocks MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds an opt-in `GITNEXUS_PARSE_TIMEOUT_MICROS` env var that wires `Parser.setTimeoutMicros` around every call routed through `parseSourceSafe`. When the limit is hit, the wrapper converts tree-sitter's `null` return into a catchable `ParseTimeoutError` — which the existing callers in `call-processor.ts` already handle by skipping the file. When unset (default) behaviour is identical to previous versions. Why --- On large repositories the worker-pool idle timeout (default 30s) can fire while a worker is blocked inside a sync `parser.parse()` on a pathological file. The replacement path then calls `worker.terminate()` while the native parser is still running; on macOS this races the tree-sitter binding and the process aborts with: libc++abi: terminating due to uncaught exception of type Napi::Error The crash is unrecoverable from JavaScript because the throw originates in C++ during teardown and bypasses `try/catch`. Setting a per-parse timeout slightly below the worker idle timeout lets tree-sitter abort cooperatively before the pool tries to terminate the worker, so the race window never opens. Reproduction ------------ npx gitnexus analyze # 30s default worker timeout # → "Worker N parse job idle timeout. Splitting into …" # → "libc++abi: terminating due to uncaught exception of type Napi::Error" Workaround that confirmed the trigger: npx gitnexus analyze --worker-timeout 300 # succeeds (~10× slower) Scope ----- This change removes the dominant trigger of the race but does NOT fix the underlying race in `worker-pool.replaceWorker`. A complete fix would still want to either (a) make the post-terminate path tolerant of in-flight native work, or (b) drive this same timeout automatically from `--worker-timeout`. Keeping the env var opt-in here so the change is minimal and bisectable; happy to follow up with the automatic wiring if the direction is acceptable. Tests ----- `test/unit/safe-parse.test.ts` gains three cases covering: - no-op behaviour when the env var is unset (backward compatibility), - `ParseTimeoutError` thrown when the limit is exceeded, - timeout is reset after each call so a reused parser is unaffected. --- gitnexus/src/core/tree-sitter/safe-parse.ts | 66 ++++++++++++++++++--- gitnexus/test/unit/safe-parse.test.ts | 35 ++++++++++- 2 files changed, 91 insertions(+), 10 deletions(-) diff --git a/gitnexus/src/core/tree-sitter/safe-parse.ts b/gitnexus/src/core/tree-sitter/safe-parse.ts index 1b3bc4fc5..e79217a3c 100644 --- a/gitnexus/src/core/tree-sitter/safe-parse.ts +++ b/gitnexus/src/core/tree-sitter/safe-parse.ts @@ -19,6 +19,51 @@ const SAFE_PARSE_CHUNK_CHARS = 16 * 1024; */ const DIRECT_PARSE_LIMIT_CHARS = 16 * 1024; +/** + * Optional per-parse timeout, opt-in via `GITNEXUS_PARSE_TIMEOUT_MICROS`. + * + * When unset (default) the parser has no timeout — behaviour is identical to + * previous versions. When set to a positive integer, `parser.setTimeoutMicros` + * is applied before each parse so tree-sitter aborts long-running parses + * cooperatively (returning `null` from `parse()`); this wrapper then converts + * the `null` into a catchable `ParseTimeoutError`, which existing callers in + * `call-processor.ts` already handle by skipping the file. + * + * Motivation: on large repos the worker-pool idle timeout can fire while + * tree-sitter is blocked inside a sync `parser.parse()` on a pathological + * file. The replacement path (`worker.terminate()`) then races the native + * parser and surfaces as `libc++abi: terminating due to uncaught exception + * of type Napi::Error`, killing the analysis run. Setting a per-parse + * timeout slightly below the worker idle timeout lets the parser abort + * cleanly before the pool tries to terminate it. + */ +const readParseTimeoutMicros = (): number => { + const raw = process.env.GITNEXUS_PARSE_TIMEOUT_MICROS; + if (!raw) return 0; + const value = Number(raw); + return Number.isFinite(value) && value > 0 ? Math.floor(value) : 0; +}; + +export class ParseTimeoutError extends Error { + constructor(timeoutMicros: number, sourceLength: number) { + super( + `tree-sitter parse exceeded GITNEXUS_PARSE_TIMEOUT_MICROS=${timeoutMicros} ` + + `(source length ${sourceLength} chars).`, + ); + this.name = 'ParseTimeoutError'; + } +} + +const applyAndClearTimeout = (parser: Parser, timeoutMicros: number, run: () => T): T => { + if (timeoutMicros <= 0) return run(); + parser.setTimeoutMicros(timeoutMicros); + try { + return run(); + } finally { + parser.setTimeoutMicros(0); + } +}; + /** * Parse `sourceText` safely on every platform. See {@link SAFE_PARSE_CHUNK_CHARS} * for the underlying tree-sitter binding bug this works around. @@ -29,12 +74,17 @@ export function parseSourceSafe( oldTree?: Parser.Tree, options?: Parser.Options, ): Parser.Tree { - if (sourceText.length <= DIRECT_PARSE_LIMIT_CHARS) { - return parser.parse(sourceText, oldTree, options); - } - const input: Parser.Input = (index) => { - if (index >= sourceText.length) return null; - return sourceText.slice(index, index + SAFE_PARSE_CHUNK_CHARS); - }; - return parser.parse(input, oldTree, options); + const timeoutMicros = readParseTimeoutMicros(); + const tree = applyAndClearTimeout(parser, timeoutMicros, () => { + if (sourceText.length <= DIRECT_PARSE_LIMIT_CHARS) { + return parser.parse(sourceText, oldTree, options); + } + const input: Parser.Input = (index) => { + if (index >= sourceText.length) return null; + return sourceText.slice(index, index + SAFE_PARSE_CHUNK_CHARS); + }; + return parser.parse(input, oldTree, options); + }); + if (!tree) throw new ParseTimeoutError(timeoutMicros, sourceText.length); + return tree; } diff --git a/gitnexus/test/unit/safe-parse.test.ts b/gitnexus/test/unit/safe-parse.test.ts index e536bbd40..b083e15a1 100644 --- a/gitnexus/test/unit/safe-parse.test.ts +++ b/gitnexus/test/unit/safe-parse.test.ts @@ -1,7 +1,7 @@ -import { describe, it, expect } from 'vitest'; +import { describe, it, expect, afterEach } from 'vitest'; import Parser from 'tree-sitter'; import Python from 'tree-sitter-python'; -import { parseSourceSafe } from '../../src/core/tree-sitter/safe-parse.js'; +import { parseSourceSafe, ParseTimeoutError } from '../../src/core/tree-sitter/safe-parse.js'; const makeParser = (): Parser => { const p = new Parser(); @@ -71,4 +71,35 @@ describe('parseSourceSafe', () => { expect(tree.rootNode.hasError).toBe(false); expect(tree.rootNode.endIndex).toBe(large.length); }); + + describe('GITNEXUS_PARSE_TIMEOUT_MICROS opt-in', () => { + const ORIGINAL = process.env.GITNEXUS_PARSE_TIMEOUT_MICROS; + afterEach(() => { + if (ORIGINAL === undefined) delete process.env.GITNEXUS_PARSE_TIMEOUT_MICROS; + else process.env.GITNEXUS_PARSE_TIMEOUT_MICROS = ORIGINAL; + }); + + it('is a no-op when the env var is unset (backward compatible)', () => { + delete process.env.GITNEXUS_PARSE_TIMEOUT_MICROS; + const tree = parseSourceSafe(makeParser(), 'x = 1\n'); + expect(tree.rootNode.hasError).toBe(false); + }); + + it('throws ParseTimeoutError when parsing exceeds the configured timeout', () => { + // 1 microsecond is below any real parse latency — tree-sitter aborts + // immediately and returns null, which the wrapper converts to a throw. + process.env.GITNEXUS_PARSE_TIMEOUT_MICROS = '1'; + const src = buildSource(64 * 1024); + expect(() => parseSourceSafe(makeParser(), src)).toThrowError(ParseTimeoutError); + }); + + it('resets the parser timeout after each call so reuse is unaffected', () => { + const parser = makeParser(); + process.env.GITNEXUS_PARSE_TIMEOUT_MICROS = '1'; + expect(() => parseSourceSafe(parser, buildSource(64 * 1024))).toThrow(); + delete process.env.GITNEXUS_PARSE_TIMEOUT_MICROS; + const tree = parseSourceSafe(parser, 'x = 1\n'); + expect(tree.rootNode.hasError).toBe(false); + }); + }); });