morluto/rea

Fork 0

mirror of https://github.com/morluto/rea.git synced 2026-11-05 06:45:22 +00:00

[PR #4997] [CLOSED] fix(javascript): name the exhausted heap and max_heap_mb in heap failures #6718

New issue

Closed

opened 2026-11-01 21:42:31 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-11-01 21:42:31 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/4997
Author: @Iyanuoluwa007
Created: 10/18/2026
Status: ❌ Closed

Base: main ← Head: fix/js-heap-exhaustion-hint

📝 Commits (2)

1068654 fix(javascript): name the exhausted heap and max_heap_mb in heap failures
e6f3539 test(javascript): report the static phase from the heap-exhaustion child

📊 Changes

3 files changed (+70 additions, -5 deletions)

View changed files

📝 src/domain/javascript/javascriptAnalysisResourceControls.ts (+8 -2)
📝 src/javascript/analysis/JavaScriptAnalysisWorker.ts (+13 -3)
📝 tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts (+49 -0)

📄 Description

Summary

Refs #4987 (its "side note"). When the owned JavaScript analyzer runs out of V8 heap, the per-file limitation now names the exhausted heap size and the analysis phase, and points to max_heap_mb while it can still be raised.

Before:

Owned JavaScript worker exhausted its heap while analyzing out/main/index.js during static

After (default 1024 MiB):

Owned JavaScript worker exhausted its 1024 MiB heap while analyzing out/main/index.js in the static phase; retry with a larger max_heap_mb (at most 16384)

Problem and expected behavior

In #4987 the reporter's run exited 0 with main_handlers: 0, and the only explanation was the limitation above. They noted it "looks cut off" (during static is the phase name) and "doesn't mention --max-heap-mb"; they found the flag only by reading --help, and the same input then reported 791 handlers with --max-heap-mb 6144. reported_limits already carries worker_old_space_mb, but the human-readable limitation that agents and users read did not say which budget ran out or how to raise it.

Change and scope

src/javascript/analysis/JavaScriptAnalysisWorker.ts: the V8 heap-exhaustion branch (stderr reports heap out of memory / heap limit / allocation failed) builds its reason through #heapExhaustedReason. It names this.limits.heapMb, says "in the phase", and appends ; retry with a larger max_heap_mb (at most 16384) only when the heap is below the maximum.
The process-memory-limit branch (#memoryFailure, used for Job Object limits) is unchanged: raising max_heap_mb is not the right advice there.
src/domain/javascript/javascriptAnalysisResourceControls.ts: JAVASCRIPT_ANALYSIS_HEAP_MB_BOUNDS replaces the 128/16_384 literals repeated in the max_heap_mb schema and the worker constructor.

max_heap_mb is the shared CLI/MCP input name; the CLI spells it --max-heap-mb.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScript analysis worker resource failure reason.
Contribution scope fit and linked maintainer decision: not applicable.
CLI and MCP/tool-catalog contract: max_heap_mb bounds unchanged; only the resource-constraint reason text changes.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: limitation text only; reported_limits unchanged.
Target selection, authority, upstream behavior, and cleanup details: not applicable.
Generated metadata, package, or installation impact: none.
Documentation or usability only; no target interaction.

Evidence and regression coverage

Tests added: tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts "names the exhausted %i MiB heap and how to raise it". A real owned child prints V8's FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory and exits 134. At 128 MiB the reason includes the max_heap_mb hint; at 16384 MiB it names the heap without a hint.
Review follow-up: the first version of the fixture never reported a phase, so both new cases failed on Linux (confirmed by @CassieFox23 on Node 22/24/26 and by Greptile). The child now sends { kind: "phase", id, phase: "static" } for the request before the simulated crash (a6b095992). Branch rebased onto main b4938aed5.
Remaining proof gaps: this worker test file cannot run on my Windows checkout, because owned worker processes need the packaged Windows native addon. The existing tests in the file fail there in the same way. It runs in the Linux CI test shards. I checked separately that the fixture's generated child program compiles and that the child prints the V8 line and exits 134. No real heap exhaustion of a large bundle was reproduced.
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

npx vitest run src/javascript/analysis src/domain/javascript: 90 files, 2411 tests passed.
npx tsc --noEmit: exit 0. npm run check:fast (typecheck, lint, pre-push hook): passed.
New worker test: not runnable on this Windows host (see above); relies on CI.

Compatibility, safety, and release

Breaking changes or migration steps: none; callers matching the old reason text exactly will see the new wording.
Real Hopper/Ghidra, browser, or OS coverage: none.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none.

Author readiness

Human owner: @Iyanuoluwa007

I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work.
I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps.
I declare this PR ready for initial maintainer review.

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/4997 **Author:** [@Iyanuoluwa007](https://github.com/Iyanuoluwa007) **Created:** 10/18/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/js-heap-exhaustion-hint` --- ### 📝 Commits (2) - [`1068654`](https://github.com/morluto/rea/commit/10686544abebd85c49cd35a5ff77ccedfd9c0228) fix(javascript): name the exhausted heap and max_heap_mb in heap failures - [`e6f3539`](https://github.com/morluto/rea/commit/e6f3539f1a001f3a1f8dfb89eb32c4789df8bbf8) test(javascript): report the static phase from the heap-exhaustion child ### 📊 Changes **3 files changed** (+70 additions, -5 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/javascript/javascriptAnalysisResourceControls.ts` (+8 -2) 📝 `src/javascript/analysis/JavaScriptAnalysisWorker.ts` (+13 -3) 📝 `tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts` (+49 -0) </details> ### 📄 Description ## Summary Refs #4987 (its "side note"). When the owned JavaScript analyzer runs out of V8 heap, the per-file limitation now names the exhausted heap size and the analysis phase, and points to `max_heap_mb` while it can still be raised. Before: ``` Owned JavaScript worker exhausted its heap while analyzing out/main/index.js during static ``` After (default 1024 MiB): ``` Owned JavaScript worker exhausted its 1024 MiB heap while analyzing out/main/index.js in the static phase; retry with a larger max_heap_mb (at most 16384) ``` ## Problem and expected behavior In #4987 the reporter's run exited 0 with `main_handlers: 0`, and the only explanation was the limitation above. They noted it "looks cut off" (`during static` is the phase name) and "doesn't mention `--max-heap-mb`"; they found the flag only by reading `--help`, and the same input then reported 791 handlers with `--max-heap-mb 6144`. `reported_limits` already carries `worker_old_space_mb`, but the human-readable limitation that agents and users read did not say which budget ran out or how to raise it. ## Change and scope - `src/javascript/analysis/JavaScriptAnalysisWorker.ts`: the V8 heap-exhaustion branch (stderr reports `heap out of memory` / `heap limit` / `allocation failed`) builds its reason through `#heapExhaustedReason`. It names `this.limits.heapMb`, says "in the <phase> phase", and appends `; retry with a larger max_heap_mb (at most 16384)` only when the heap is below the maximum. - The process-memory-limit branch (`#memoryFailure`, used for Job Object limits) is unchanged: raising `max_heap_mb` is not the right advice there. - `src/domain/javascript/javascriptAnalysisResourceControls.ts`: `JAVASCRIPT_ANALYSIS_HEAP_MB_BOUNDS` replaces the `128`/`16_384` literals repeated in the `max_heap_mb` schema and the worker constructor. `max_heap_mb` is the shared CLI/MCP input name; the CLI spells it `--max-heap-mb`. ## Contract and boundary impact - Semantic owner and earliest changed stage: JavaScript analysis worker resource failure reason. - Contribution scope fit and linked maintainer decision: not applicable. - CLI and MCP/tool-catalog contract: `max_heap_mb` bounds unchanged; only the resource-constraint reason text changes. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: limitation text only; `reported_limits` unchanged. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata, package, or installation impact: none. - [x] Documentation or usability only; no target interaction. ## Evidence and regression coverage - Tests added: `tests/boundary/javascript/javascriptSourceAnalysisWorker.test.ts` "names the exhausted %i MiB heap and how to raise it". A real owned child prints V8's `FATAL ERROR: Reached heap limit Allocation failed - JavaScript heap out of memory` and exits 134. At 128 MiB the reason includes the `max_heap_mb` hint; at 16384 MiB it names the heap without a hint. - Review follow-up: the first version of the fixture never reported a phase, so both new cases failed on Linux (confirmed by @CassieFox23 on Node 22/24/26 and by Greptile). The child now sends `{ kind: "phase", id, phase: "static" }` for the request before the simulated crash (a6b095992). Branch rebased onto main b4938aed5. - Remaining proof gaps: this worker test file cannot run on my Windows checkout, because owned worker processes need the packaged Windows native addon. The existing tests in the file fail there in the same way. It runs in the Linux CI test shards. I checked separately that the fixture's generated child program compiles and that the child prints the V8 line and exits 134. No real heap exhaustion of a large bundle was reproduced. - [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 - `npx vitest run src/javascript/analysis src/domain/javascript`: 90 files, 2411 tests passed. - `npx tsc --noEmit`: exit 0. `npm run check:fast` (typecheck, lint, pre-push hook): passed. - New worker test: not runnable on this Windows host (see above); relies on CI. ## Compatibility, safety, and release - Breaking changes or migration steps: none; callers matching the old reason text exactly will see the new wording. - Real Hopper/Ghidra, browser, or OS coverage: none. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. ## Author readiness Human owner: @Iyanuoluwa007 - [x] I reviewed the final diff and relevant existing code, understand the change and its edge cases, and take responsibility for the complete PR, including AI-assisted work. - [x] I checked the evidence above, confirmed it supports the current change, and recorded meaningful validation gaps. - [x] I declare this PR ready for initial maintainer review. ## 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-11-01 21:42:31 +00:00

closed this issue
added the
pull-request
label

No milestone

No project

No assignees

1 participant

Notifications

Due date

The due date is invalid or out of range. Please use the format "yyyy-mm-dd".

No due date set.

Dependencies

No dependencies set.

Reference

morluto/rea#6718

No description provided.

Rows
Columns