fix(extract): AST traversal stacks no longer outlive the file they walk

[shafty023]

Fixes #2010. Part of #1997; the retention half is #1925.

#2010 is the focused issue I split out of #1997 for exactly this defect, so the
closing keyword points there. #1997 itself stays open: its retention half is in
flight as #1925 and its underlying complaint, that the memory budget does not
bound the index, is not answered here.

Indexing a 14k-file TypeScript monorepo holds 13.3 GB resident at post_extract on Linux
aarch64, and about 3.1 GB of that is AST traversal scratch that nothing in the graph points at.
The diagnosis is in #1997 (comment) and this is the fix it asked about.

Why this change, and why now

Of the memory a 14k-file index retains, roughly half is traversal scratch that nothing in
the result ever points at, and the other half is the extraction output that later passes
read. This PR takes the first half with a contained change: one owner for the scratch, a
lifetime that is correct by construction, and signatures that make the old mistake a compile
error rather than a review item. It needs nothing from, and changes nothing in, the bounded
two-pass design in #1925 that takes the second half, so the two land independently and in
either order. The alternatives below were each measured or read out before this shape was
chosen; the numbers that decided it are in the section after.

The mechanism

ts_nstack_init cut its stacks from ctx->arena, which is result->arena. In the parallel path
that result is stored into ec->result_cache[file_idx] (pass_parallel.c:963) and freed only
after parallel_resolve and the infra passes (pipeline.c:1324), so a structure written for a
one-file lifetime is in fact held for the whole index, once per file, concurrently.
extract_node_stack.h said otherwise and was wrong: "freed when the arena is destroyed at end of
file extraction".

The largest instance is channels. CHAN_STACK_CAP is 4096 and sizeof(TSNode) is 32, so
scan_string_consts_js (extract_channels.c:104) and extract_channels_js (:376) take 128 KB
each, per JavaScript or TypeScript file. Across 12,673 such files that is 3,168 MB, and it
produced 23 Channel nodes out of 164,684.

CBMExtractCtx now carries a scratch arena, created and destroyed by the cbm_extract_file_ex
call that builds the context. Nothing else moves: ctx->arena still owns every string a
CBMFileResult points at, ctx->scratch owns only traversal stacks, which nothing points at.

Public surface

cbm.h gains one field, CBMExtractCtx.scratch, and extract_node_stack.h changes the three
ts_nstack_* signatures. Both are additive for anything that builds a context through
cbm_extract_file_ex; a context built by hand without a scratch keeps the old behaviour through
the fallback to ctx->arena. Nothing else on the public extraction surface moves.

Measured, Linux aarch64, gcc 13.3, static build, five runs per side

Full 13,999-file tree, CBM_PROFILE=1, a private CBM_CACHE_DIR per run, 18 workers, 24 GB
container.

metric	46ae198	this branch	delta
mem.collect post_extract rss_mb	13273 to 13323, mean 13293	6732 to 6778, mean 6752	-49.2%
mem.phase peak_mb	14807 to 14870, mean 14840	8552 to 8585, mean 8565	-42.3%
pipeline.done nodes	164684, all five	164684, all five	identical
pipeline.done edges	835869 to 835874	835867 to 835881	ranges overlap, run to run noise
parse_partial_count	280, all five	280, all five	identical
registry entries / defines / imports	787954 / 804999 / 100027	same	identical
pipeline.done elapsed_ms	29975 to 39357, mean 35651	26876 to 34384, mean 30502	mean -14.4%, ranges overlap

The wall clock is not a claim: the two ranges overlap, so all that is supportable is that there is
no regression. The memory ranges do not overlap, by a factor of two.

ObjectScript Studio Export, the one path that calls cbm_extract_file_ex more than once per
physical file, on a small fixture: nodes 18, edges 25, both sides, identical.

Scratch block size

512 KB, chosen from measurement rather than taste, and named
CBM_EXTRACT_SCRATCH_BLOCK in cbm.c.

initial block	files where arena_grow fired	post_extract rss_mb	elapsed_ms
256 KB	many, two channel stacks alone are exactly 262144 bytes	6896	37992
512 KB	1 of 12000	6765	29663
1024 KB	1 of 12000	6717	29358

512 KB and 1 MB grow equally rarely, so 512 KB wins on resident cost per worker. It is also
exactly MI_LARGE_MAX_OBJ_SIZE in the vendored mimalloc, MI_LARGE_PAGE_SIZE/8 = 524,288
(vendored/mimalloc/include/mimalloc/types.h:426, with MI_ENABLE_LARGE_PAGES defaulting to 1 at
:115 and not overridden by this build), so the block is still bin-allocated from a 4 MiB large
page. Growth is not free at that size for the same reason: arena_grow doubles to 1 MiB, which is
above that bound and therefore a singleton OS allocation. One file in twelve thousand pays it,
which is why the cost is accepted. The largest single file's scratch high water mark on this corpus
is 33.5 MB, now transient rather than retained.

The block is created before the quarantine short-circuit, not after. cbm_arena_init(&result->arena)
already sits ahead of that check, so this is consistent with what the path pays today, and the
wrapper is what keeps the change to one create and one destroy across the body's seven early
returns instead of eight destroy sites.

The mechanical part

165 call sites: ts_nstack_init 55, ts_nstack_push 70, ts_nstack_push_children 40, across
extract_imports.c 108, extract_channels.c 30, extract_defs.c 9, extract_type_refs.c 6,
extract_semantic.c 6, extract_type_assigns.c 3, extract_env_accesses.c 3.

It is not one uniform sed. 29 init sites passed ctx->arena and 26 passed a local alias, and
that alias is shared with result allocations: in parse_zig_imports
(extract_imports.c:1093) the same a feeds cbm_node_text, strip_quotes, path_last and
cbm_imports_push at :1101, :1106, :1108 and :1109. Rebinding it would have moved import names and module paths into memory that dies
with the call while the result kept pointing at them. Each site was edited on its own and no alias
was rebound. parse_lisp_imports used its alias only for the stack, so the alias goes too. Verify
the set with:

grep -rn 'ts_nstack_init' internal/cbm/extract_*.c
grep -rn 'ts_nstack_push' internal/cbm/extract_*.c

The signature change is what makes the mistake unrepresentable rather than merely avoided.
ts_nstack_init takes the context, so handing it ctx->arena is an incompatible pointer type;
push and push_children read the arena the stack recorded at init, so a stale argument is an
arity error. Both fail under -Werror.

Alternatives considered

Thread-local scratch arena, reset per file, with a depth guard. Rejected on three counts. This
codebase never uses thread-local storage for owned bulk memory; every existing _Thread_local is
a small cache, a guard or a log buffer, and every per-worker resource is a field in a context
struct. It would have needed cbm_arena_reset, which has no production caller and a documented
subtlety with grown block sizes (arena.c:213-217). And extraction threads are created per
cbm_parallel_for call and joined (worker_pool.c:71, 88) with no thread-exit hook in this
project's own code, so the arena would have leaked once per thread per dispatch and turned the
leak lane red.

Save and restore mark and release on the arena. Rejected because a per-call arena already
makes nesting correct by construction, and this would add a mark type and two functions to both
arena headers.

Per-walk arenas, one per traversal rather than one per file. Rejected on cost: 55 sites would
each need an explicit span, for the same bound.

Right-sizing the initial capacities alone. CHAN_STACK_CAP at 256 rather than 4096 would cut
those two sites from 3,168 MB to about 198 MB, which is real, but it tunes the constant while the
lifetime defect remains at all 55 sites, and growth is not free: 256 doubling to 4096 abandons
122,880 bytes, and one file here needs 33.5 MB of stacks. Worth doing separately, and the cap and
the scratch block size are coupled, which is another reason to change one at a time.

Routing to a scratch inside extract_node_stack.h while keeping the arena parameters.
Rejected because a parameter that is ignored lies about the API, and the alias hazard above would
stay a review obligation instead of a compile error.

Freeing the per-file result arena earlier. Rejected: the results are read by
parallel_resolve and the infra passes. That is the retention half #1925 addresses, and this
change is independent of it and does not touch that machinery.

Tests

Nothing in the suite asserted on result->arena size, block count or total_alloc after
cbm_extract_file, which is why this went unnoticed for the life of extract_node_stack.h.

• traversal_stack_not_in_result_arena_issue2010 (tests/test_extraction.c) is the byte budget. Extracting
  "export const x = 1;" as TypeScript charged 365,984 bytes to the result arena before and
  charges 87,456 after, a difference of 278,528, exactly the two 4096-entry channel walks plus the
  512-entry ES import walk at 32 bytes per TSNode. Of the 87,456 that remain, 7,680 is the defs
  item array at GROW_ARRAY's starting capacity of 32 times sizeof(CBMDefinition) 240, and the
  other 79,776 is everything else this file's extraction interns, none of it traversal scratch. The
  128 KB bound is therefore derived: above the new figure with room, a factor of four below the old
  one.
• extract_traversal_stacks_come_from_ctx_scratch_issue2010 (tests/test_mem.c) is the lifetime half. A
  byte budget alone would also be satisfied by shrinking CHAN_STACK_CAP, so this builds the
  extraction context directly over two arenas it owns, runs cbm_extract_channels, and asserts
  the scratch took at least the 262,144 bytes of the two walks while the result arena did not.
• extract_c_macro_hidden_call_survives_preprocessed_pass_issue2010 is not a scratch test.
  pp_ctx carries ctx->scratch so every context in the file is uniform, but nothing reads it
  there: pp_ctx reaches only cbm_extract_unified and cbm_run_c_lsp, and neither
  extract_unified.c nor anything under internal/cbm/lsp/ includes extract_node_stack.h, so no
  traversal stack is built on that path today. It guards the macro-expansion path itself, which
  extract_c_ifdef_split_brace_fn_recovered_issue961 and
  extract_cpp_preproc_signature_gap_issue946 reach but neither asserts a call that exists only
  after expansion.
• ObjectScript composite extraction is already covered by
  pipeline_objectscript_export_preserves_calls_sequential_parallel (tests/test_pipeline.c) and
  iris_export_xml_multi_class, so no new test is added. A per-call arena is correct there by
  construction, since each call owns and destroys its own.

scripts/test.sh: 7558 passed, 0 failed, 8 skipped, 139 suites.

Failure modes closed rather than added

A context built without a scratch, and a file whose scratch arena fails to allocate, both fall
back to ctx->arena, which is exactly the behaviour that shipped before this change. No path can
be handed a NULL arena and silently lose nodes, which matters because
ts_nstack_push already swallows an allocation failure (if (!new_items) return;), and silently
truncating a traversal is the failure class this file was created to fix in #199.

Two things a reviewer will ask

Size. 544 changed lines against the "under 500" guideline in CONTRIBUTING. 165 of them are a
one-line mechanical edit at every ts_nstack_* call site, which the two greps above re-derive; the
change itself is about 200 lines in cbm.c and three headers. I kept it as one PR because a
signature change and its call sites cannot land separately without breaking the build in between.
Happy to split differently if you would rather review it another way.

Why ts_nstack_init takes the context rather than an arena parameter. An arena parameter would
have kept extract_node_stack.h a leaf header, but it would also have kept the hazard: 26 of the
55 init sites pass a local a that is an alias of ctx->arena and is also the arena for the
result's own strings, so getting one site wrong is a use-after-free that survives into resolve
rather than a compile error. Taking the context makes that unrepresentable, at the cost of the
header including cbm.h. That include is acyclic and a no-op in all seven consumers, each of which
already has cbm.h as its first line, so nothing is reordered. If you prefer the leaf header, the
plain-parameter variant is a small change from here: ts_nstack_init(TSNodeStack *, CBMArena *, int)
with the same field and the same push signatures, and I will take the review burden on the 26
sites instead. Say which you want.

Separate observations, not fixed here

1. internal/cbm/arena.h and src/foundation/arena.h share the guard CBM_ARENA_H and declare
   the same struct, but the internal copy omits init_sized, calloc, reset and total, and
   disagrees on CBM_ARENA_DEFAULT_BLOCK_SIZE. -Isrc precedes -I internal/cbm
   (Makefile.cbm:53-55) and there is no src/arena.h, so a src/pipeline file writing
   #include "arena.h" also lands on the reduced copy, and internal/cbm/arena.c is in no
   *_SRCS list and is dead. This change works within that rather than untangling it: cbm.c
   includes foundation/arena.h ahead of cbm.h, the spelling src/pipeline/lsp_surface.h:24 and
   tests/test_mem.c:9 already use, and no header is edited. Happy to open an issue for the
   duplication itself.
2. arena_grow returns 0 at CBM_ARENA_MAX_BLOCKS 256, cbm_arena_alloc then returns NULL, and
   GROW_ARRAY does a bare return at cbm.c:111, silently dropping items with no log line and
   no has_error.
3. collect_children (lsp/ts_lsp.c:201) and cbm_lsp_collect_children
   (lsp/c_lsp.c, cs_lsp.c, go_lsp.c, eight call sites) allocate TSNode arrays from the
   result arena that are pure iteration scratch. They are not routed here because they sit behind
   LSP entry points taking a bare arena, which the cross-file resolve pass also calls with a
   different lifetime. Worth a follow-up.
4. Pre-existing and untouched, flagged because this diff renames the function: the extraction body
   is 475 lines against readability-function-size.LineThreshold 400. It was 471 at 46ae198, so
   the wrapper adds four lines to an already-over-threshold function rather than crossing it.

[DeusData]

Approved on merit. I traced the mechanism end to end rather than taking the numbers, and every step holds.

Verified

• CHAN_STACK_CAP = 4096 (extract_channels.c:35), sizeof(TSNode) 32 → 128 KB per stack, and ts_nstack_init is called twice per JS/TS file (:104 and :138). Your 3,168 MB across 12,673 files is arithmetic, not estimate.
• ts_nstack_init cuts from ctx->arena, which is result->arena.
• pass_parallel.c:964 stores that result into ec->result_cache[file_idx], and its own comment says why: "Cache result (arena + extracted data, no tree) for Phase 3B and Phase 4". The cache is then read by pass_calls.c:748, pass_definitions.c:682, pass_k8s.c:651 and pass_usages.c:344 — so the arena genuinely lives across the whole resolve phase, once per file, concurrently.

So the header comment at extract_node_stack.h:8 — "freed when the arena is destroyed at end of file extraction" — is simply false on the parallel path, and it is the reason this survived. A comment asserting a lifetime the code does not honour is worse than no comment, because every subsequent reader checks the claim instead of the call graph. Correcting it is as valuable as the fix.

3.1 GB of scratch to produce 23 Channel nodes out of 164,684 is the number that settles whether this is worth doing.

One thing you did not claim, which strengthens it

ts_nstack_push doubles on overflow and abandons the old block in the arena — its own comment says so. So a stack that grows once does not replace 128 KB, it retains 128 KB and 256 KB, all for the life of the index. The waste compounds on exactly the deep files most likely to need the growth.

On the shape of the fix

A separate ctx->scratch arena, created and destroyed by the cbm_extract_file_ex call that builds the context, is the right answer — and the reason is the ownership split, not the byte count. ctx->arena owns every string a CBMFileResult points at, so its long life is correct and must not change; ctx->scratch owns only traversal stacks, which nothing points at, so its short life is also correct. Two lifetimes, two arenas, each right by construction.

And making the signatures enforce it matters more than the change itself. Threading scratch through ts_nstack_init/ts_nstack_push turns "did this caller use the right arena?" from a review question into a compile error. A fix that relies on future reviewers noticing would regress the first time someone adds a traversal; this one cannot.

Scoping it to the traversal half and leaving the retention half to #1925 is right too, and I appreciate you saying plainly that #1997's underlying complaint — the memory budget does not bound the index — is not answered here. Splitting #2010 out so the closing keyword points at the defect actually fixed is the correct bookkeeping.

cbm.h gains 7 lines, so this touches the public extraction surface — additive and necessary for the scratch handle, but worth a line in the description saying so.

Before merge

CI is still running and our Actions pool has been backlogged all day (25 runs queued as I write), so expect a wait unrelated to this PR. Once green I will merge.

Thank you — the diagnosis, the arithmetic, and the "23 nodes for 3.1 GB" framing made this reviewable in one pass.

[github-actions]

Thanks for opening this — it has been seen, and it is queued.

This note is automated, but it is not a brush-off: it exists so you know where your PR stands instead of having to guess from silence.

Current review status: working through a backlog. 0.9.1-rc.1 is out, so the release freeze that held reviews is over — but it left a large queue of open pull requests behind it, and we are reading through them oldest-first. The background is in discussion #1144.

What that means for this PR, concretely:

• It will not be closed for inactivity. No stale bot touches pull requests here.
• It may still sit a while before a human reads it. That is on us, not on you.
• Older PRs are read first, so a recent one is not being skipped — it is behind a queue.

Things that will genuinely speed it up whenever review does happen:

• Keep it rebased on main — the tree is moving quickly right now, and a conflicting branch cannot be reviewed as the diff you intended.
• Get CI green, or say which failures you believe are pre-existing.
• Keep the change to one claim. Bundled features and refactors get split before they get merged, which costs you a round trip.
• Every commit needs a sign-off (git commit -s) — CI enforces DCO.

If this fixes a bug, a reproduction we can run is worth more than a description of the symptom.

Thanks for contributing, and sorry in advance for the wait.

[shafty023]

@DeusData Thanks for the fast and thorough read. Added a Public surface section to the description covering the cbm.h field and the ts_nstack signature changes, as asked. Your point about growth compounding on deep files is right, and it is the same reason the scratch block was sized from measured growth counts rather than the old caps. Happy to wait out the CI queue.