mirror of
https://github.com/abhigyanpatwari/GitNexus.git
synced 2026-10-03 02:21:44 +00:00
fix(ingestion): optional per-parse timeout to prevent worker deadlocks
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.
This commit is contained in:
parent
fdf1effb2a
commit
ae93706b8d
2 changed files with 91 additions and 10 deletions
|
|
@ -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 = <T>(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;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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);
|
||||
});
|
||||
});
|
||||
});
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue