[PR #3035] [CLOSED] fix(javascript): stop overflowing the stack on large module factories #5571

New issue

Closed

opened 2026-10-31 01:07:41 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:07:41 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3035
Author: @LeonBytes
Created: 10/13/2026
Status: ❌ Closed

Base: main ← Head: fix/symbolic-module-completion-stack-overflow

📝 Commits (1)

0105d2d fix(javascript): walk module factories without spreading child nodes

📊 Changes

2 files changed (+70 additions, -15 deletions)

View changed files

📝 src/domain/javascript/javascriptSymbolicBindings.test.ts (+54 -0)
📝 src/domain/javascript/javascriptSymbolicBindings.ts (+16 -15)

📄 Description

Summary

Analysis fails for a whole file when a loaded module's factory contains a very large array literal. #moduleCompletionPoint did pending.push(...childNodes(node)), and spreading a million children overflows the call stack. The worker reports Maximum call stack size exceeded as a memory resource_constraint and suggests retrying with more memory, which doesn't help.

This swaps the hand-written walk for the existing traverseJavaScriptAst. No issue exists for this; I found it while reading the code.

Problem and expected behavior

A load()-ed module whose factory holds a huge array literal should be analyzed like any other module. With rea analyze-javascript-application <dir> --json:

1,000 and 200,000 elements: fine (the worker runs with --stack_size=2048).
1,000,000 elements (3.5 MB main.js): analysis_failure, code resource_constraint, resource memory, reason main.js: Maximum call stack size exceeded.

The graph for main.js goes from one unknown node plus the asset node (6 nodes overall) to 4 module nodes, 1 chunk node and the asset node (18 overall). I didn't measure the exact element count where it starts failing; it depends on stack size and host.

Fixture generator

// node make-fixture.mjs <dir> 1000000
import { mkdirSync, writeFileSync } from "node:fs";
const [dir, n] = [process.argv[2], Number(process.argv[3])];
mkdirSync(dir, { recursive: true });
writeFileSync(`${dir}/package.json`, '{"name":"demo","version":"1.0.0","main":"main.js"}');
const data = Array.from({ length: n }, (_, i) => i % 251).join(",");
writeFileSync(`${dir}/main.js`, `(() => {
  const modules = {
    4(module) { module.exports = require("electron"); },
    8(module, exports, load) {
      const electron = load(4);
      const TABLE = [${data}];
      exports.sender = function(channel) { return (...args) => electron.ipcRenderer.send(channel, ...args); };
    },
    12(module, exports, load) {
      const helpers = load(8);
      const api = { copy: helpers.sender("test:copy") };
      require("electron").contextBridge.exposeInMainWorld("api", api);
    }
  };
  function load(id) { const m = {exports:{}}; modules[id](m,m.exports,load); return m.exports; }
  load(12);
})();
`);

Change and scope

#moduleCompletionPoint now calls traverseJavaScriptAst(factory.body, { skip, enter }). The body root is still always entered, nested functions and classes are still skipped, and the earliest ReturnStatement is still picked by start. The unused childNodes import is removed. Since childNodes is documented as the single place that expands AST children, this removes a duplicate walker instead of adding a helper.

I read the other push(...) spreads in src/domain/javascript (javascriptAstFingerprint.ts, javascriptSemanticObjectReferences.ts) and left them alone: their sizes come from one node's tokens or one class's fields, not from a source-sized child list. That is from reading, not testing.

Not touched: the failure classification. A stack overflow shows up as a memory resource_constraint, which is misleading, but that is your call.

Contract and boundary impact

Semantic owner and earliest changed stage: symbolic bindings in src/domain/javascript (Electron wrapped call paths).
Contribution scope fit and linked maintainer decision: fix inside existing boundaries; not applicable.
CLI and MCP/tool-catalog contract: unchanged. For the affected input, a per-file failure becomes the normal graph.
Provider, bridge, target-format, or platform compatibility: none; static analysis only.
Evidence, artifact, provenance, or reconstruction contract: unchanged.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Read existing files or captured data.
Launch or attach to a process.
Inject instrumentation or run scripts/code.
Change process memory, registers, files, or configuration.
Contact network services.
Recover protected code or work around protections.
Documentation or usability only; no target interaction.
Not sure yet; investigation or maintainer guidance needed.

Evidence and regression coverage

Tests added or updated: src/domain/javascript/javascriptSymbolicBindings.test.ts, "completes a loaded module at its earliest return past a very large array literal". It replaces the elements of a parsed array with 1,000,000 synthetic numeric literals (faster than parsing a 3.5 MB source) and checks the loaded namespace completes at the factory's earliest bare return. The size matters: 200,000 elements didn't overflow in the test worker either.
Base reproduction: with only the source change stashed, the file shows 27 tests | 1 failed (the new one); with the change all 27 pass.
User-visible CLI/MCP output: see the numbers above.
Remaining proof gaps: no real-provider, Windows or macOS run; exact stack threshold not measured.
Observed, derived, and inferred claims remain distinguishable.
Artifact identity, source provenance, and failed attempts remain preserved.
Unsupported, incomplete, unavailable, or uncertain outcomes remain visible.

Validation performed

Linux, Node v22.22.0 (.nvmrc pins 24.18.0), npm 10.9.4 (packageManager is npm@11.16.0), npm ci --ignore-scripts.

npx tsc --noEmit: exit 0
npx oxfmt --check and npx oxlint on the two changed files: format passes; no new lint warnings
npm run verify:module-boundaries: pass
npx knip: exit 0
npx vitest run --project domain --project services src/domain/javascript src/application/javascript: 69 files, 1558 tests passed
On the base revision, before the change: domain, services, composition, conformance and evaluation projects: 285 files, 4077 passed, 4 skipped
npm run build:cached and node scripts/rea.mjs analyze-javascript-application <fixture> --json on the 1,000,000-element fixture: fails before the change, no analysis_failure after it

Not run: npm run check, npm run check:pr, npm run docs:check, the boundary, cli-boundary, mcp-boundary, acceptance and process projects, and the real-provider lanes.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; not run.
Package or release metadata impact: none.
Security, privacy, process, or containment review: no new I/O, process or network behavior; only how a parsed AST is walked changes.

Before merge

This PR addresses a concrete problem or an agreed enhancement.
Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff.
The PR has one focused outcome and the title follows type(scope): outcome.
Related issue is linked, or the reason for not linking one is stated above.
Tests cover changed observable behavior and meaningful failure paths.
Owning docs, contracts, and generated metadata are updated where needed.
User-visible CLI/MCP changes include representative output.
I checked the final diff for secrets, unrelated cleanup, and unsupported claims.

_{🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.}

## 📋 Pull Request Information **Original PR:** https://github.com/morluto/rea/pull/3035 **Author:** [@LeonBytes](https://github.com/LeonBytes) **Created:** 10/13/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/symbolic-module-completion-stack-overflow` --- ### 📝 Commits (1) - [`0105d2d`](https://github.com/morluto/rea/commit/0105d2d38d90ef39054e2e73cb1b660cf557869c) fix(javascript): walk module factories without spreading child nodes ### 📊 Changes **2 files changed** (+70 additions, -15 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptSymbolicBindings.test.ts` (+54 -0) 📝 `src/domain/javascript/javascriptSymbolicBindings.ts` (+16 -15) </details> ### 📄 Description ## Summary Analysis fails for a whole file when a loaded module's factory contains a very large array literal. `#moduleCompletionPoint` did `pending.push(...childNodes(node))`, and spreading a million children overflows the call stack. The worker reports `Maximum call stack size exceeded` as a memory `resource_constraint` and suggests retrying with more memory, which doesn't help. This swaps the hand-written walk for the existing `traverseJavaScriptAst`. No issue exists for this; I found it while reading the code. ## Problem and expected behavior A `load()`-ed module whose factory holds a huge array literal should be analyzed like any other module. With `rea analyze-javascript-application <dir> --json`: - 1,000 and 200,000 elements: fine (the worker runs with `--stack_size=2048`). - 1,000,000 elements (3.5 MB `main.js`): `analysis_failure`, code `resource_constraint`, resource `memory`, reason `main.js: Maximum call stack size exceeded`. The graph for `main.js` goes from one `unknown` node plus the asset node (6 nodes overall) to 4 module nodes, 1 chunk node and the asset node (18 overall). I didn't measure the exact element count where it starts failing; it depends on stack size and host. <details> <summary>Fixture generator</summary> ```js // node make-fixture.mjs <dir> 1000000 import { mkdirSync, writeFileSync } from "node:fs"; const [dir, n] = [process.argv[2], Number(process.argv[3])]; mkdirSync(dir, { recursive: true }); writeFileSync(`${dir}/package.json`, '{"name":"demo","version":"1.0.0","main":"main.js"}'); const data = Array.from({ length: n }, (_, i) => i % 251).join(","); writeFileSync(`${dir}/main.js`, `(() => { const modules = { 4(module) { module.exports = require("electron"); }, 8(module, exports, load) { const electron = load(4); const TABLE = [${data}]; exports.sender = function(channel) { return (...args) => electron.ipcRenderer.send(channel, ...args); }; }, 12(module, exports, load) { const helpers = load(8); const api = { copy: helpers.sender("test:copy") }; require("electron").contextBridge.exposeInMainWorld("api", api); } }; function load(id) { const m = {exports:{}}; modules[id](m,m.exports,load); return m.exports; } load(12); })(); `); ``` </details> ## Change and scope `#moduleCompletionPoint` now calls `traverseJavaScriptAst(factory.body, { skip, enter })`. The body root is still always entered, nested functions and classes are still skipped, and the earliest `ReturnStatement` is still picked by `start`. The unused `childNodes` import is removed. Since `childNodes` is documented as the single place that expands AST children, this removes a duplicate walker instead of adding a helper. I read the other `push(...)` spreads in `src/domain/javascript` (`javascriptAstFingerprint.ts`, `javascriptSemanticObjectReferences.ts`) and left them alone: their sizes come from one node's tokens or one class's fields, not from a source-sized child list. That is from reading, not testing. Not touched: the failure classification. A stack overflow shows up as a memory `resource_constraint`, which is misleading, but that is your call. ## Contract and boundary impact - Semantic owner and earliest changed stage: symbolic bindings in `src/domain/javascript` (Electron wrapped call paths). - Contribution scope fit and linked maintainer decision: fix inside existing boundaries; not applicable. - CLI and MCP/tool-catalog contract: unchanged. For the affected input, a per-file failure becomes the normal graph. - Provider, bridge, target-format, or platform compatibility: none; static analysis only. - Evidence, artifact, provenance, or reconstruction contract: unchanged. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Read existing files or captured data. - [ ] Launch or attach to a process. - [ ] Inject instrumentation or run scripts/code. - [ ] Change process memory, registers, files, or configuration. - [ ] Contact network services. - [ ] Recover protected code or work around protections. - [ ] Documentation or usability only; no target interaction. - [ ] Not sure yet; investigation or maintainer guidance needed. ## Evidence and regression coverage - Tests added or updated: `src/domain/javascript/javascriptSymbolicBindings.test.ts`, "completes a loaded module at its earliest return past a very large array literal". It replaces the elements of a parsed array with 1,000,000 synthetic numeric literals (faster than parsing a 3.5 MB source) and checks the loaded namespace completes at the factory's earliest bare `return`. The size matters: 200,000 elements didn't overflow in the test worker either. - Base reproduction: with only the source change stashed, the file shows `27 tests | 1 failed` (the new one); with the change all 27 pass. - User-visible CLI/MCP output: see the numbers above. - Remaining proof gaps: no real-provider, Windows or macOS run; exact stack threshold not measured. - [x] Observed, derived, and inferred claims remain distinguishable. - [x] Artifact identity, source provenance, and failed attempts remain preserved. - [x] Unsupported, incomplete, unavailable, or uncertain outcomes remain visible. ## Validation performed Linux, Node v22.22.0 (`.nvmrc` pins 24.18.0), npm 10.9.4 (`packageManager` is npm@11.16.0), `npm ci --ignore-scripts`. - `npx tsc --noEmit`: exit 0 - `npx oxfmt --check` and `npx oxlint` on the two changed files: format passes; no new lint warnings - `npm run verify:module-boundaries`: pass - `npx knip`: exit 0 - `npx vitest run --project domain --project services src/domain/javascript src/application/javascript`: 69 files, 1558 tests passed - On the base revision, before the change: domain, services, composition, conformance and evaluation projects: 285 files, 4077 passed, 4 skipped - `npm run build:cached` and `node scripts/rea.mjs analyze-javascript-application <fixture> --json` on the 1,000,000-element fixture: fails before the change, no `analysis_failure` after it Not run: `npm run check`, `npm run check:pr`, `npm run docs:check`, the boundary, cli-boundary, mcp-boundary, acceptance and process projects, and the real-provider lanes. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; not run. - Package or release metadata impact: none. - Security, privacy, process, or containment review: no new I/O, process or network behavior; only how a parsed AST is walked changes. ## Before merge - [x] This PR addresses a concrete problem or an agreed enhancement. - [x] Material scope and authority questions are resolved, and any required maintainer scope decision covers the final diff. - [x] The PR has one focused outcome and the title follows `type(scope): outcome`. - [x] Related issue is linked, or the reason for not linking one is stated above. - [x] Tests cover changed observable behavior and meaningful failure paths. - [x] Owning docs, contracts, and generated metadata are updated where needed. - [x] User-visible CLI/MCP changes include representative output. - [x] I checked the final diff for secrets, unrelated cleanup, and unsupported claims. --- <sub>🔄 This issue represents a GitHub Pull Request. It cannot be merged through Gitea due to API limitations.</sub>

bsakaguchi

2026-10-31 01:07:41 +00:00