Skip to content

A module graph too deep to link now raises an error the host can catch, instead of ending the process - #3415

Merged
lahma merged 2 commits into
sebastienros:mainfrom
lahma:fix/3401-module-graph-depth
Aug 26, 2026
Merged

lahma merged 2 commits into
sebastienros:mainfrom
lahma:fix/3401-module-graph-depth

Conversation

@lahma

@lahma lahma commented Aug 26, 2026

Copy link
Copy Markdown
Collaborator

CyclicModuleRecord.InnerModuleLinking and InnerModuleEvaluation descend once per module, the way the
spec writes them. The load phase does not — InnerModuleLoading drives a work queue — so the depth of a
graph an engine can load is Options.Modules.MaxModuleGraphDepth, while the depth it can link and
evaluate
is the calling thread's stack. Exceeding that is a native stack overflow: no exception, nothing
in a catch, no log, and the process gone. Nothing stood in the way — MaxModuleGraphDepth defaults to
int.MaxValue, constraints are not evaluated during linking, and StackOverflowGuard probes inside
ScriptFunction's four entry points, which these algorithms never reach.

That is what #3308 was: at roughly 700 bytes of stack per module, a 1,000-module import needs 640–768 KB
on net8.0 and 768–896 KB on net10.0, which is inside a factor of two of an ordinary thread's stack.
One CI leg died and four passed. A host whose whole reason for implementing IModuleLoader is to serve a
graph it did not author could be killed by one.

Both algorithms now probe for headroom before descending, and each fails in the shape its own caller needs.

JavaScriptException: Maximum call stack size exceeded while linking module '/base/968.js'

The two halves fail differently, and that is the whole design

Linking throws. Link's existing catch already walks the Tarjan stack returning every module on it
to unlinked, which is exactly the cleanup a failure here needs, so the probe raises a RangeError
through it and the engine is usable afterwards.

Evaluation returns a throw completion. Evaluate step 9.a is the only thing that marks the modules
still on the Tarjan stack evaluated-with-the-error, and only an abrupt completion returned to it reaches
that step. Measured against a build that throws instead: the first import fails the same way, and the
second import succeeds, handing the host a namespace for a graph not one body of which ever ran. A
half-evaluated graph that reports itself as fine is worse than the crash the probe replaced, so
GraphDepth_AnEvaluationTooDeepForTheStackLeavesTheGraphErrored pins the second import too.

One more decision worth naming: the probe is gated on Constraints.StackOverflowGuard alone and
deliberately does not stand down for MaxExecutionStackCount the way the script backstop does. That
exclusion exists because the two probes sit a few native frames apart on one call path and the more
specific request should win. The module pipeline is not on that path — MaxExecutionStackCount's lane is
TryEnterOnCurrentStack at the call expression, which nothing here reaches — so honouring the precedence
would leave an engine that asked for a call-depth limit with no probe at all for a deep import.

Tests

Both are in Jint.Tests.PublicInterface, and both were confirmed to end the test process against this
branch with only the engine change reverted — Test Run Aborted, with the stack walk in the fatal banner
naming the phase each one selects:

test what it drives reverted-engine outcome
GraphDepth_AnImportTooDeepForTheStackThrowsRatherThanEndingTheProcess a 1,000-module chain imported end to end Stack overflow. → InnerModuleLinking ×N
GraphDepth_AnEvaluationTooDeepForTheStackLeavesTheGraphErrored the same chain, pre-linked in slices so only evaluation has a thousand levels left to descend Stack overflow. → InnerModuleEvaluation ×N

Both run on a 256 KB thread — the floor a thread request is rounded up to on Windows — where a plain
import chain reaches about 200 modules before either phase runs out. The chain is 1,000, i.e. five
times
the depth that trips it. That margin is the point: the per-module cost differs by runtime,
architecture and operating system by tens of percent, and it was exactly a margin thinner than that which
let #3308 pass on four legs and end the process on the fifth. Nothing decided by codegen moves a factor of
five, and a test that could kill the runner if it regressed would be a bad test.

Measured trip depths on this branch (net8.0, x64, Release, Windows), by binary search:

thread stack deepest chain that imports first that does not
256 KB 155 156 (evaluating)
512 KB 475 476 (evaluating)
1 MB 1,000 1,001 (linking)

dotnet build -c Release clean. Jint.Tests 10,758 / Jint.Tests.PublicInterface 3,098 green on
net472, net8.0 and net10.0. Test262: 102,495 passed, 0 failed — the control that matters here,
since module code is heavily covered and the evaluation half hands back a completion where an exception
used to be impossible.

What this is not

It is a catchable failure, not a raised ceiling. How deep a graph an engine can import is still decided
by the thread's stack rather than by MaxModuleGraphDepth, so #3401 stays open for the real fix:
making the two algorithms iterative the way loading already is. Part of #3401.

Two findings from attempting it, for whoever picks it up:

  • The linking half converts mechanically; the evaluation half does not. InnerModuleLinking is
    Tarjan's SCC and becomes an explicit stack of (module, next-request, dfsIndex) frames with the
    post-child step (spec 9.c) run when a child frame pops. InnerModuleEvaluation has to reproduce
    [[PendingAsyncDependencies]], [[CycleRoot]], the agent-level async evaluation order, the
    defer-phase evaluationList, and abrupt propagation that returns past every ancestor frame at once —
    and ExecuteAsyncModule can re-enter and mutate module state from inside the loop.
  • Converting those two alone does not remove the cliff, it moves it. Three more depth-linear
    recursions live in the same pipeline: ResolveExport/GetExportedNames over export * from chains,
    and the deferred-import gatherers (GatherAsynchronousTransitiveDependencies, ReadyForSyncExecution).
    They are unprobed here on purpose, and safely so today only because each is bounded by the same graph
    depth the linking DFS has already walked with a larger frame — measured on a 256 KB stack, linking
    gives up at ~209 modules while a 200-deep export * from chain resolves. Make linking iterative and
    that argument evaporates: InitializeEnvironment would then run at depth ~0 with a full stack for
    ResolveExport to eat. So the iterative rewrite has to carry the probe down into those sites as part of
    the same change, which is why it is a design pass and not a patch.

lahma added 2 commits August 26, 2026 23:13
…he process

CyclicModuleRecord.InnerModuleLinking and InnerModuleEvaluation recurse once per
module, so the deepest graph an engine can import is decided by the calling
thread's stack rather than by anything the host configured - and exceeding it is
a native stack overflow, which .NET delivers to nobody.

Both now probe for headroom the way StackGuard's script backstop does, and the
two halves fail in the shape their caller needs: linking throws, and Link's
existing cleanup returns every module on the Tarjan stack to unlinked; evaluation
returns a throw completion, so Evaluate step 9.a marks them evaluated with that
error rather than stranding them in `evaluating` with a capability nothing will
settle.

Part of sebastienros#3401.
…t is still stack-bound

The README's recursion section, the migration document's section 4, and the
modules gotcha that until now recorded the crash as an open gap.

All three say the same two things: the failure is now a RangeError naming the
module, on an engine that goes on working; and this is a catchable failure rather
than a raised ceiling - how deep a graph an engine can import is still the calling
thread's stack, not MaxModuleGraphDepth.
@lahma
lahma merged commit f39f396 into sebastienros:main Aug 26, 2026
7 checks passed
lahma added a commit to lahma/jint that referenced this pull request Sep 1, 2026
…t the one asked for

Both new tests failed the linux legs with "found <null>": the 1,000-module chain on a 256 KB thread
request linked and evaluated successfully there, so they asserted an exception that legitimately never
happened.

The margin was over the wrong quantity. The frames agree across platforms almost exactly — binary search
on this branch puts the trip at 157 modules on a 256 KB Windows thread, where sebastienros#3415 measured 156 for main
— so the fixed chain length was five times a correctly measured depth. What does not carry across
platforms is the stack itself: `maxStackSize` is a request, and on that runner it bought enough for a
1,000-module walk. No constant chosen on one machine can be right about that on another.

So the tests now measure the machine instead of predicting it: the chain grows (2,000 … 32,000) until the
walk under test meets the probe, and the assertions are made on whichever length did. Whatever stack the
platform really handed over, doubling reaches the end of it; the cap is there so a regression is reported
rather than pursued forever, and covers a genuinely 16 MB stack — the largest this suite ever asks for —
with the escalation costing 6.5 s there and 0.3 s where the first length already trips.

Preparation moves off the small thread while it is at it. Only the walk under test needs that stack, and
where a platform's probe reserve is itself larger than it — a 64 KB page size makes the runtime's reserve
768 KB — preparing there would meet the probe during setup and report a failure about the wrong thing.

Unchanged against the reverted engine: both tests still end the test process, exit 127, `Stack overflow.`
with 433 x InnerModuleLinking and 328 x InnerModuleEvaluation on net10.0, "Process is terminated due to
StackOverflowException." on net472. Green on both with the fix, and the engine is untouched by this
commit.

Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
lahma added a commit that referenced this pull request Sep 1, 2026
…nstead of ending the process (#3415) (#3548)

* A module graph too deep to link raises an error the host can catch, instead of ending the process (#3415)

Backport of #3415 to 4.x.

`CyclicModule.InnerModuleLinking` and `InnerModuleEvaluation` descend once per module, the way the spec
writes them, so the depth of a graph an engine can link and evaluate is decided by the calling thread's
stack rather than by anything the host configured. Exceeding it is a native stack overflow: no exception,
nothing in a `catch`, no log, and the process gone. Nothing stood in the way — execution constraints are
not evaluated during linking, and `Constraints.StackOverflowGuard` probes inside `ScriptFunction`'s four
entry points, which these algorithms never reach.

Both algorithms now probe for headroom before descending, through the same
`Constraints.StackOverflowGuard` that already covers script recursion, and each fails in the shape its own
caller needs. Linking throws: `Link`'s existing `catch` walks the Tarjan stack returning every module on it
to `unlinked`, which is exactly the cleanup a failure here needs, so the engine is usable afterwards.
Evaluation returns a throw completion: `Evaluate` step 9.a is the only thing that marks the modules still
on the Tarjan stack evaluated-*with*-the-error, and only an abrupt completion returned to it reaches that
step. Measured on this branch against a build that throws instead, the first import fails the same way and
the second import succeeds, handing the host a namespace for a graph not one body of which ever ran.

The probe is gated on `Constraints.StackOverflowGuard` alone and deliberately does not stand down for
`MaxExecutionStackCount` the way the script backstop does: that limit's lane is `TryEnterOnCurrentStack`
at the call expression, which nothing in the module pipeline reaches, so honouring the precedence would
leave an engine that asked for a call-depth limit with no probe at all for a deep import.

Not ported from #3415, which carried them only as context for options this branch does not have: the
`docs/v5-migration.md` entry, the `Jint/Runtime/Modules/AGENTS.md` note, and the edits to
`HostModuleGraphSecurityTests` — that suite, `Options.Modules.MaxModuleCount` and `MaxModuleGraphDepth`
are all v5-only. No new option or public member is introduced here.

Unlike main, this branch's load phase recurses per module too, so the two tests drive their phase in
slices rather than importing a cold graph: the load recursion would otherwise meet the reserve first. That
third recursion stays unprobed — the load phase reports failure by rejecting its own promise, so a throw
out of it would be a different contract rather than the same one in another place.

Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

* Size the depth tests against the stack the platform actually gave, not the one asked for

Both new tests failed the linux legs with "found <null>": the 1,000-module chain on a 256 KB thread
request linked and evaluated successfully there, so they asserted an exception that legitimately never
happened.

The margin was over the wrong quantity. The frames agree across platforms almost exactly — binary search
on this branch puts the trip at 157 modules on a 256 KB Windows thread, where #3415 measured 156 for main
— so the fixed chain length was five times a correctly measured depth. What does not carry across
platforms is the stack itself: `maxStackSize` is a request, and on that runner it bought enough for a
1,000-module walk. No constant chosen on one machine can be right about that on another.

So the tests now measure the machine instead of predicting it: the chain grows (2,000 … 32,000) until the
walk under test meets the probe, and the assertions are made on whichever length did. Whatever stack the
platform really handed over, doubling reaches the end of it; the cap is there so a regression is reported
rather than pursued forever, and covers a genuinely 16 MB stack — the largest this suite ever asks for —
with the escalation costing 6.5 s there and 0.3 s where the first length already trips.

Preparation moves off the small thread while it is at it. Only the walk under test needs that stack, and
where a platform's probe reserve is itself larger than it — a 64 KB page size makes the runtime's reserve
768 KB — preparing there would meet the probe during setup and report a failure about the wrong thing.

Unchanged against the reverted engine: both tests still end the test process, exit 127, `Stack overflow.`
with 433 x InnerModuleLinking and 328 x InnerModuleEvaluation on net10.0, "Process is terminated due to
StackOverflowException." on net472. Green on both with the fix, and the engine is untouched by this
commit.

Claude-Session: https://claude.ai/code/session_014W5mbjGhyvgAS4pivXoc4S

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
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.

1 participant