fix(extraction): give parse workers a native stack the kernel walkers can't overflow - #1582
Open
apollo600 wants to merge 1 commit into
Open
fix(extraction): give parse workers a native stack the kernel walkers can't overflow#1582apollo600 wants to merge 1 commit into
apollo600 wants to merge 1 commit into
Conversation
… can't overflow A C/C++ file with thousands of nested braces segfaulted the whole `codegraph index` process. The kernel walkers recurse once per parse-tree level (~450 bytes of native stack per level for c/cpp), and parse workers ran on Node's default 4MB worker stack, so deep nesting overflowed it. The overflow is a SIGSEGV inside the addon rather than a JS exception, so neither parse-worker's catch nor the kernel's per-file `defer:` fallback could see it — and since worker threads share the process, it took the CLI down with no diagnostic and no index. Repro'd on llvm/llvm-project, where `clang/test/Parser/parser_overflow.c` nests 16,384 braces: SIGSEGV at 4MB and 6MB of worker stack, parses cleanly at 8MB. The pool now pins `resourceLimits.stackSizeMb` to 16MB, which is reserved lazily and so costs nothing on files of ordinary depth. This raises the cliff rather than removing it. The complete fix is a depth cap in the kernel walkers that raises `defer:`, landing such a file on the wasm extractor — which parses this input correctly today, and is why `CODEGRAPH_KERNEL=0` was a working workaround. The regression test runs the parse in a child process on purpose: if this regresses, the parse segfaults, and a segfault in a worker thread would take the test runner down instead of reporting a failure. Fixes colbymchenry#1581.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1581.
What was happening
codegraph indexon a repo containing a deeply nested C/C++ file died withSegmentation fault (core dumped)— the whole CLI process, no error message, no index. Reproduced onllvm/llvm-project, where the trigger isclang/test/Parser/parser_overflow.c(16,384 nested braces).ccpp::visit_noderecurses once per parse-tree level, ~450 bytes of native stack per level. Parse workers were created asnew Worker(scriptPath), i.e. on Node's default 4MB worker stack (the main thread gets 8MB), so ~9k levels exhausted it. The overflow is a SIGSEGV inside the addon and never becomes a JS exception, so:parse-worker.ts's catch (which handlesmemory access out of bounds/out of memoryby exiting so the pool respawns) never runs;tryKernelExtractRaw's per-filedefer:fallback to the wasm extractor never runs;codegraphprocess dies.Measured, with the worker stack as the only variable:
parser_overflow.cNesting depth at 4MB: 8,000 levels ok, 10,000 levels SIGSEGV.
Same file on the wasm path parses fine —
CODEGRAPH_KERNEL=0indexed all 31,608 files of llvm-project in 1m48s. With the kernel on and only that one file excluded: 1m24s, and the node/edge counts match the wasm run except for that file's own 2 nodes.What this changes
ParseWorkerPoolpinsresourceLimits: { stackSizeMb: 16 }. A thread stack is reserved lazily, page by page, so this costs nothing on files of ordinary depth.I want to be explicit that this raises the cliff rather than removing it. The complete fix is a depth cap in the kernel walkers that raises
defer:, which routes the file to the wasm extractor that already handles it.A third thing worth considering separately: when the pool sees a worker exit with a parse in flight, naming that file in the error would turn this class of bug from a bisect into a one-line diagnosis.
Tests
New
__tests__/parse-worker-stack.test.ts:resourceLimitsactually reaches the worker thread, and the partialresourceLimitsdoesn't silently cap the V8 heap (parse workers legitimately reach ~1.4GB RSS on a large index);.nodeis staged, matchingkernel-scaffold.test.ts.Both worker arms run in a child process deliberately: on a regression the parse segfaults, and a segfault in a worker thread would take the test runner down with it instead of reporting a failure. Verified by setting the constant back to 4 — the suite fails cleanly with
expected 'SIGSEGV' to be nulland the runner survives.npx tsc --noEmitclean;__tests__/parse-pool.test.ts(17 tests) still passes.