Skip to content

fix(ingest): stop spreading a child subtree into push in walkFilesystem - #315

Open
datrixlab wants to merge 1 commit into
trailhq:mainfrom
datrixlab:fix/walk-filesystem-arg-cap
Open

datrixlab wants to merge 1 commit into
trailhq:mainfrom
datrixlab:fix/walk-filesystem-arg-cap

Conversation

@datrixlab

Copy link
Copy Markdown

What

walkFilesystem appends a child subtree with

out.push(...walkFilesystem(full, includes));

which makes the child array the argument list of push(). V8 caps that at roughly 128k, so a single unignored subtree with more paths than that throws RangeError: Maximum call stack size exceeded at trivial directory depth — the message is about the argument list, not recursion depth.

Appending in a loop removes the cap. Nothing else changes: same paths, same order.

Fixes #314

Why it only bites some repos

Only the git-less path reaches this. When listGitFiles returns null the walker collects every non-dot file first and filters by language later, so any large cache, browser profile or dataset directory in a repo that was never git init-ed is fatal — while the same tree under Git is fine because git ls-files respects .gitignore. That is what makes it look nondeterministic across a fleet, as the reporter described.

Tests

Two, deliberately split, because one of them cannot do the other's job:

  1. walkDir collects a large nested subtree outside Git without dropping paths — a wide, deep subtree outside Git comes back flat and complete. Runs on every platform, ~40ms. It pins the accumulation and would catch a botched loop, but it cannot reach the argument cap, so it passes on the old code too. I am flagging that rather than presenting it as proof.
  2. walkDir survives a subtree larger than V8's argument limit (#314) — the real reproduction, 150k files. On the previous code it fails with exactly Maximum call stack size exceeded; with this change it passes.

Verified both directions: reverting only src/ingest/fs.ts and re-running gives # pass 18 / # fail 1, and the one failure is test 2 with that message. With the fix, # pass 19 / # fail 0 in 6.9s.

The heavy test is skipped on Windows. CI runs windows-latest too, and creating 150k files there is far slower than the ~2.5s it takes on Linux. If you would rather not pay even the Linux cost in CI, say so and I will drop test 2 and keep test 1 — the fix stands on its own either way.

Measurements taken while writing this

  • On Node 22, a spread of 125k elements still succeeds; 150k throws. The reporter's ~65k figure is the commonly quoted number but the real boundary is higher, which is why the test uses 150k rather than something just over 65k.
  • --stack-size does not move that boundary. I tried 500/200/100 with 2k–10k elements: all fine. It is an argument-count limit, not stack depth — worth knowing if anyone later tries to make the test cheaper by shrinking the stack.
  • The other push(...) sites in src/graph/build.ts (nodes, rawEdges) accumulate per file rather than per subtree, so they do not reach the cap. Left untouched to keep this focused.

`out.push(...walkFilesystem(full, includes))` turns the child's array into
push()'s argument list, and V8 caps that at roughly 128k. A single
unignored subtree with more paths than that throws "Maximum call stack
size exceeded" at trivial directory depth — the message is about the
argument list, not recursion.

Only the git-less path reaches this: when listGitFiles returns null the
walker collects every non-dot file first and filters by language later,
so any large cache, browser profile or dataset directory in a repo that
was never `git init`-ed is fatal. Appending in a loop removes the cap
without changing what is collected or in what order.

Two tests, deliberately split:

- a cheap one that pins the accumulation itself (a wide, deep subtree
  outside Git comes back flat and complete). It runs everywhere and would
  catch a botched loop, but it cannot reach the argument cap.
- one that actually reproduces the crash with 150k files. It fails with
  "Maximum call stack size exceeded" on the previous code and passes with
  this one. CI also runs windows-latest, where creating that many files
  is far slower, so it is skipped there.

Measured while writing this: on Node 22 a spread of 125k elements still
succeeds and 150k throws, and `--stack-size` does not move the boundary —
it is an argument-count limit, not stack depth.
@trailhq-graft

trailhq-graft Bot commented Sep 9, 2026

Copy link
Copy Markdown

🌱 graft blast radius

1 area changed → 2 areas can be affected. 4 dependent symbols, depth 2.
Tests: no test reaches Filesystem Traversal.
Tag: @anirudhkumar-nanonets — 3 of 3 areas · @shhdwi — Dependency Graph · @yescine — Filesystem Traversal

flowchart TB
  A0(("Dependency Graph<br/>3 symbols"))
  A1(("Context Discovery<br/>1 symbol"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Dependency Graph 3 src/graph/build.ts:L151-L410 buildGraph — calls, depth 2 Filesystem Traversal
Context Discovery 1 src/context/build.ts:L109-L122 listContextFiles — calls, depth 2 Filesystem Traversal
Who knows this code — 4 people across 3 areas
Area Who knows it
Filesystem Traversal · changed @anirudhkumar-nanonets — 3 commits, last 1mo ago · @yescine — 2 commits, last 29d ago
Dependency Graph · affected @anirudhkumar-nanonets — 17 commits, last 20d ago · @shhdwi — 9 commits, last 27d ago
Context Discovery · affected @anirudhkumar-nanonets — 4 commits, last 20d ago · @Frankie-Xu — 2 commits, last 9d ago

Ownership is git history over each area's own files, weighted towards recent work (120-day half-life). Merge commits and bots are dropped, and you are dropped from your own PR. A name with no @ has no GitHub handle in its commit email — tag them by hand, or add a .mailmap entry. A suggestion from history, not a CODEOWNERS rule.

All 4 dependent symbols, grouped by area

Dependency Graph — 3 symbols in 3 files

  • src/graph/build.ts:L151-L410 — buildGraph (calls, depth 2)
  • src/graph/scopes.ts:L133-L272 — discoverScopes (calls, depth 2)
  • src/graph/source-files.ts:L64-L85 — listSourceFiles (calls, depth 2)

Context Discovery — 1 symbol in 1 file

  • src/context/build.ts:L109-L122 — listContextFiles (calls, depth 2)
Test signal per changed area — 1 ✗

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • Filesystem Traversal — 0 of 1 reached · no test file reaches it
    • not reached: walkFilesystem

graft blast · origin/main...HEAD · depth 2 · 2 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

walkFilesystem crashes with "Maximum call stack size exceeded" on large unignored subtrees (spread-push at fs.js:290, non-git repos)

1 participant