morluto/rea

Fork 0

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

[PR #3550] [CLOSED] fix(javascript): keep parent budget exhaustion from advising more memory #5816

New issue

Closed

opened 2026-10-31 01:09:39 +00:00 by bsakaguchi · 0 comments

bsakaguchi commented

2026-10-31 01:09:39 +00:00

Owner

📋 Pull Request Information

Original PR: https://github.com/morluto/rea/pull/3550
Author: @Antruly
Created: 10/14/2026
Status: ❌ Closed

Base: main ← Head: fix/js-parent-budget-remediation-invariant

📝 Commits (8)

170a3ef fix(javascript): advise the parent result budget after a static checkpoint
5ea5a67 fix(javascript): give the parent budget remediation one owner
9e73670 test(javascript): drive the static-checkpoint parent budget through the CLI
f200310 test(javascript): keep the trigger fixture inside the worker's fact budget
5dfa560 Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant
b6abd86 Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant
5406ecc Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant
7cde632 Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant

📊 Changes

5 files changed (+155 additions, -17 deletions)

View changed files

📝 src/domain/analysisErrorPresentation.ts (+23 -13)
📝 src/domain/analysisErrorProjection.test.ts (+61 -0)
📝 src/domain/javascript/javascriptAnalysisResourceControls.ts (+10 -0)
📝 src/javascript/analysis/JavaScriptAnalysisWorker.ts (+6 -4)
➕ tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts (+55 -0)

📄 Description

Summary

analyze_javascript_application stops the remaining sources when the parent
result budget is exhausted while retaining a static checkpoint. That error told
the user to "Retry with sufficient memory", which cannot help, because the limit
belongs to the parent result budget. Fixes #3538.

Problem and expected behavior

JavaScriptAnalysisWorker raised this AnalysisResourceConstraintError with
parent_result_budget_exhausted: true but without a remediationAction, so
analysisErrorPresentation fell back to the generic memory advice. Its three
sibling parent-budget paths (module facts, semantic graph, expanded application
relationships) already state the opposite: the limit belongs to the parent
result budget and a larger worker heap does not raise it.

Reported (base revision):

code: resource_constraint
message: The parent's result capacity was exhausted after retaining static facts for app.asar/node_modules/onnxruntime-web/dist/ort.all.mjs; remaining sources were not analyzed
remediation: Review the reported worker memory limits and available host memory. Retry with sufficient memory or a smaller artifact; REA retains tighter inherited limits.

After this change:

remediation: Use the retained static facts or analyze fewer source files. This limit belongs to the parent result budget; increasing the worker heap alone does not increase it.

Consolidation with #3545

Draft #3545, opened by the reporter, changes the same worker error to the same
sentence. That overlap is real, and it is not surprising: #3538 quotes the
sibling remediation verbatim as the expected text, so there is essentially one
sentence that satisfies the request, and any fix converges on it. This PR began
from the same issue and landed on the same line independently.

morluto compared both against main at fc712b3 and asked for the two sets of
protection to be consolidated before either PR is retired: this PR's shared
remediation owner and presentation fallback, and #3545's constrained-heap public
CLI trigger. 9e73670 takes that trigger into this branch, so this PR now
carries both, and #3545 can close once it does.

The test comes from #3545 (b2aa24d), the file being the only thing that
commit needed from it. Rinkia ran it against 5ea5a676 before it landed here
and reported it passing. #3538 stays open until this fix is delivered.
greptile then reviewed 9e73670 and found the fixture itself at fault, which
it was; f200310 reduces it so that the parent result budget is the only limit
the test exercises. See Review follow-up for the measurements.

I kept the worker line rather than delete it, because the throw site is where the
inconsistency with its three sibling paths is visible, and it is the line a
future reader goes looking for. The fallback below is not an exact substitute:
it composes the same clause, but its leading sentence says "the retained facts",
because a producer that states nothing has not said the facts are static, so
deleting the worker line would lose that word. An earlier draft of this paragraph
claimed the two were equivalent; see Review follow-up.

What this PR adds beyond the sentence is the second question: where the guarantee
should live so that the next producer of this flag cannot make the same mistake.

Change and scope

src/javascript/analysis/JavaScriptAnalysisWorker.ts: pass
{ remediationAction } to the static-checkpoint parent-budget error. The
clause stating the invariant now comes from a shared constant; the leading
sentence stays at the site, because what was retained differs per path.
src/domain/javascript/javascriptAnalysisResourceControls.ts: own that clause
as JAVASCRIPT_PARENT_RESULT_BUDGET_REMEDIATION. Two sibling paths in the
worker already stated the clause verbatim, and this PR was about to add a third
at the throw site plus a fourth in the fallback, of a sentence that must not
drift; the four now compose one string, and no user-visible text changes. A
fifth path states the same fact inside a sentence of its own.
src/domain/analysisErrorPresentation.ts: when a memory resource
constraint reports parent_result_budget_exhausted: true and carries no
explicit remediation, return the parent budget remediation instead of the
generic memory advice. An explicit remediationAction still takes
precedence; a constraint of another kind keeps the advice for its own resource,
because the clause names the worker heap and does not describe those limits;
genuine worker-heap limits keep the existing memory advice.
src/domain/analysisErrorProjection.test.ts: four deterministic unit tests
(see evidence below). They construct the error directly, so they need no heap
window and no worker process.
tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts:
the end-to-end trigger, taken from #3545. It drives the public CLI under a
256 MiB parent heap and asserts that the run reports
parent_result_budget_exhausted with the parent budget remediation. The first
source is 5,000 lines rather than 30,000, so that the worker's own
complete-fact budget is not also in play; see Review follow-up.

Why the fallback sits in presentation selection

#3538 asks, optionally, for a test asserting that every error reporting
parent_result_budget_exhausted: true carries a parent-budget remediation, so
that new paths cannot regress to the generic memory advice. I did not manage to
write that test against the throw site. Reaching it needs a parent heap window
and an owned worker process, so an acceptance test can only cover the one path
it drives, and a unit test cannot construct that state at all — it would only
restate the one string.

Where the wrong advice is actually chosen is narrower and easier to pin:
analysisErrorRemediationAction sees a memory constraint with no explicit
remediation and falls through to the generic branch. Making the flag the worker
already reports an input to that decision turns the invariant into something a
plain object can check, and it holds for memory producers that have not been
written yet. A constraint of another kind is not described by that clause, so
the flag does not redirect it; see Review follow-up.

For this one path the worker line and the fallback are therefore redundant with
each other. I kept both because the explicit line states the intent where the
error is built, and the fallback keeps the invariant true where a producer does
not state it. That is a preference, not a requirement, and I am happy to be
told otherwise.

Review follow-up

rahul05ranjan reviewed 170a3ef and raised two non-blocking nits. Both were
checked against the code and hold, so 5ea5a67 addresses them.

The two sentences have drifted by one word. The clause already had two
byte-identical copies on main rather than one, and this PR was adding a third
and a fourth: the root cause is that one user-visible sentence had no owner,
which is the same question this PR asks about the invariant. The structural fix
is the shared constant, composed at all four sites; each site keeps its own
leading sentence, so nothing the caller sees changes.
The fallback is not scoped to memory. Confirmed: a constraint of another
kind reporting the flag was answered with advice about the worker heap. The
clause names the worker heap, so its applicability is the memory constraint;
the fallback now requires that kind, and a constraint of another kind keeps the
advice for its own resource rather than being aligned with a limit the clause
does not describe. A test pins that path
(keeps a constraint's own advice when another resource reports the flag),
and it passes on origin/main too, so it pins the scoping rather than the
fix.

greptile re-reviewed 5ea5a67 at a 5/5 confidence score with no inline
findings, then reviewed 9e73670 and raised one P1 against the new fixture.
It holds, and f200310 addresses it: greptile re-reviewed f200310 at 5/5 with
no new findings, and resolved its inline thread. It reviewed the merged head
5dfa560 at 5/5 as well, with no inline findings.

Fixture exceeds the checkpoint limit (greptile, P1 on 9e73670). Confirmed,
and it measures exactly: the 30,000-line first source's complete static facts
are 9,824,883 bytes against the worker's 8 MiB complete-fact budget, so
JavaScriptAnalysisWorkerServer raises a transport constraint before writing
the checkpoint, and the parent never retains that source's facts. A 4096 MiB
parent heap shows the outcome plainly: the run completes with one finding and
a per-file observation failure for a-large.mjs carrying
resource: "transport" and "JavaScript static checkpoint exceeds its
complete-fact byte budget".

Two details differ from the finding as written, and the second is worse than
the finding says. The test does not fail while parsing: the transport failure
is recorded per file, the analysis continues, and the run's final error is
still the flagged parent-capacity error, which is why the file passed here.
What is worse is that the over-limit checkpoint was load-bearing for the
assertion. With a first source inside the budget, the same 256 MiB heap stops
the run on a-large.mjs — measured, not inferred — so the original fixture
passed only because the transport limit removed the first source from the
parent. That is two limits in one test, asserting the parent result budget.

f200310 reduces the first source to 5,000 lines, whose complete static facts
are 1,613,878 bytes, 19% of the budget, and asserts the source the message
names now. The parent result budget is the only limit left in the test, and
the constrained-heap trigger #3545 supplied is otherwise kept.

Remaining duplication: the expanded-application-relationships path states
increasing the worker heap alone does not increase it inside a sentence of its
own (JavaScriptAnalysisWorker, when returning complete static facts for a file
node). It is not byte-identical to the clause, and making it compose the constant
would change that path's user-visible text, so I left it in place. Its wording
can still drift from the constant, and that is the one copy worth a follow-up if
a maintainer wants it folded in.

Intentionally not included: budget sizing, ordering app-owned sources before
node_modules, partial-observation payload size, and --source-path hints in
the remediation text (all context-only in #3538).

Follow the contribution scope.
Scope decision: not applicable (ordinary fix of user-facing remediation within
existing boundaries).

Branch update

The head was 204 commits behind main, so the tree under review was not the
one a merge would produce. 5dfa560 merges main @ 663d07f02 into the
branch. Added and removed lines in the PR diff are unchanged by it — the same
five files, the same 155 additions and 17 deletions — and the commits greptile
reviewed stay in history, so the tree a reviewer checks out is the tree that was
measured. Both reproductions were repeated on it; see Validation performed.

GitHub Actions has now run on this revision. Until 2026-10-15 20:42 UTC the
workflow run was held at completed / action_required, where GitHub parks runs
of fork PRs until a maintainer approves them; once approved it executed to
completion and every selected lane passed (run
38795669711):
Classify changes, Static checks, Docs Generation, Build package,
Source tests, Build bundled Windows x64 native controls, all five
Test shard jobs, binary_layout, recorded_crash,
javascript_recovery, web_network, the four sqlite-runtime versions,
Native package baseline (linux x64), Portable tests, and CI required.
The lanes that were not selected for this change (nativeaot, website,
ghidra_bridge, evm, browser, web_runtime, web_source_map,
Real Apple artifact CLI/MCP E2E, Reconstruction readiness,
Source-built conformance fixtures, Reverse tools quality,
Windows capability and package lane) were skipped by change classification,
as on main.

Branch updates while awaiting review (`b6abd867`, `5406eccf`)

main moved to 1b56237c, and the branch became unmergeable again
(mergeable_state: dirty). One file conflicted,
src/domain/analysisErrorPresentation.ts, and in one hunk: this branch
restructures the AnalysisResourceConstraintError branch of
analysisErrorRemediationAction to return an explicit remediationAction
and to add the exhausted-parent-budget clause, while main appended its new
AnalysisTimeoutError handling directly after that same branch. The two
changes are independent, so the resolution keeps this branch's block and
appends main's; nothing was adjudicated. b6abd867 is that merge.

The resolution is checkable rather than asserted: against origin/main, the
merged file's added and removed lines are identical to this branch's own
additions and removals against its merge base, so neither side lost a change.
The PR diff is still the same five files at the same 155 additions and 17
deletions as before the merge.

main also changed the error algebra this branch sits on, without conflicting:
AnalysisResourceConstraintError gained an optional retryable option, the
projection's retryability rule became error.retryable ?? error.resource === "capacity", and analysisErrorBase gained cleanup-attribution ownership. The
parent-budget constraint is resource: "memory", so it stays non-retryable
under the new rule, the new option is optional, so this branch's constructions
and assertions are unaffected, and main did not touch remediation selection
for resource constraints.

main then landed a themed run of error-algebra and cleanup-ownership commits
(roughly one every five minutes), so the branch went unmergeable again. Waiting
for it to settle produced 5406eccf, which merges main @ 150e996a. The only
conflict was again analysisErrorPresentation.ts, and again only textual: this
branch adds its remediation-clause import where main rewrote the
analysisErrorBase import to bring in resolvedAnalysisError. Both are kept.

main's function-body change in that file merged automatically:
analysisErrorRemediationAction, analysisErrorCategory, and
analysisErrorUserMessage now start by resolving the error. The parent-budget
clause reads the resolved error, which is the same view the other presentation
functions got, and the focused runs below exercise that path.

The same check holds for 5406eccf: against origin/main, the merged file's
added and removed lines match this branch's own additions and removals against
its merge base, and the PR diff is unchanged at five files, 155 additions and
17 deletions.

Contract and boundary impact

Semantic owner and earliest changed stage: JavaScriptAnalysisWorker error
construction; analysisErrorPresentation remediation selection.
Contribution scope fit and linked maintainer decision: not applicable; bug
fix for #3538.
CLI and MCP/tool-catalog contract: unchanged shape; only remediation.action
text changes, for this one error path.
Provider, bridge, target-format, or platform compatibility: none.
Evidence, artifact, provenance, or reconstruction contract: none. Code,
category, message and reported_limits are unchanged, so the partial
observation and its Evidence are unaffected.
Target selection, authority, upstream behavior, and cleanup details: not
applicable.
Generated metadata (docs/public/product-catalog.json), package, or
installation impact: none.

Expected effects (check known effects; use documentation-only on its own).
Unchecked items do not establish absence; leave uncertainties visible.

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/analysisErrorProjection.test.ts — advises the parent result budget when it is exhausted, keeps an explicit remediation when the parent budget is exhausted, keeps the worker memory advice without parent exhaustion, keeps a constraint's own advice when another resource reports the flag; and
tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts —
explains parent budget exhaustion after a static checkpoint, taken from
#3545 with the first source reduced to 5,000 lines.
Complete-fact budget measurements, taken from the worker's own schema parse of
each fixture's a-large.mjs facts: 9,824,883 bytes for 30,000 lines (117% of
the 8 MiB limit, so no checkpoint is written and the source fails as
resource: "transport"), and 1,613,878 bytes for 5,000 lines (19%).
Base reproduction, presentation: with both changed source files restored to
origin/main @ 1438422df and the new tests kept, npx vitest run src/domain/analysisErrorProjection.test.ts fails exactly one test —
advises the parent result budget when it is exhausted, with
expected 'Review the reported worker memory lim…' to contain 'parent result budget'. That is the string reported in #3538. The three companion tests pass
on the base revision, so they are not restating the fix: one pins explicit
remediationAction precedence, one keeps the generic memory advice for limits a
larger worker heap really does raise, and one keeps a non-memory constraint's
own advice when it reports the flag.
Base reproduction, end to end: with the same two files restored to
origin/main, npm run test:focused -- tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts
reaches the path through the real CLI and fails the same way,
expected 'Review the reported worker memory lim…' to contain 'parent result budget'. So the flag, the message, and the reported limits were already right
on main; the remediation was the only thing wrong, which is the conclusion the
unit-level reproduction reaches as well.
User-visible CLI/MCP output: remediation text shown above.
Remaining proof gaps: what drives the end-to-end test is the parent's
heap-derived result budget, not the fixture's size. Measured on Linux x64 /
Node 24.21.0, with either fixture: a 192 MiB or 384 MiB parent heap completes
the run, and 224, 256, 288 and 320 MiB stop it on the parent result budget.
256 MiB sits inside that window, and the test is skipped on Bun as its sibling
is. No other platform was run, so the window's width elsewhere is unmeasured;
the finding that led to f200310 was found by reading the fixture rather than
by a failure, and the window is the part of this test most likely to need
adjustment if another platform disagrees. Everything below was verified on
Linux x64 / Node 24.21.0 only.

For evidence-bearing changes:

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/domain/analysisErrorProjection.test.ts — 57 passed.
npx vitest run src/domain/analysisErrorProjection.test.ts with both changed
source files restored to origin/main — 1 failed (the invariant test), 56
passed; the failure is the reported Review the reported worker memory lim…
advice.
npx vitest run --project domain — 184 files passed, 3032 passed, 3 skipped.
npm run test:focused -- tests/acceptance/applications/javascriptCheckpointReserveCli.test.ts
— 3 passed, driving the sibling parent-budget path through the real CLI under a
448 MiB and a 640 MiB parent heap and asserting that the remediation names the
parent result budget. That is also what shows the shared clause still reaches a
real caller.
npm run test:focused -- tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts
— 1 passed (3.0 s), driving the static-checkpoint parent budget through the real
CLI under a 256 MiB parent heap.
the same command with the two changed source files restored to origin/main
— 1 failed, on expected 'Review the reported worker memory lim…' to contain 'parent result budget', with the message assertion naming a-large.mjs
passing, so the run reaches the parent result budget and not the transport
limit.
the fixture under a 4096 MiB parent heap, before and after the reduction — the
reduced fixture completes with both checkpoints transferred, so no
complete-fact budget is reached.
the fixture under 192, 224, 256, 288, 320 and 384 MiB parent heaps — 192 and
384 complete; the rest stop on the parent result budget naming the first
source.
after merging main @ 663d07f02 (5dfa560), on a tree rebuilt with
npm ci and npm run build:cached: npm run test:focused -- src/domain/analysisErrorProjection.test.ts tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts —
59 passed (58 unit, 1 CLI at 2.6 s).
the same command with the three changed source files restored to the merged
origin/main — 2 failed, 57 passed: the unit invariant case and the CLI case,
both on the reported Review the reported worker memory lim… advice. The unit
file holds one more case than the independent reproduction in this thread saw
(58 against 57), because main added a browser-budget case to it in
57a23f0.
after the merge: npm run check:fast — exit 0 (typecheck, lint), and
npx oxfmt --check on the five changed files — clean.
npx tsc --noEmit — exit 0.
npx oxfmt --check . — all files use the correct format.
npm run lint — exit 0, module boundaries checked in 1885 source files,
including the new domain/analysisErrorPresentation →
domain/javascript/javascriptAnalysisResourceControls dependency.
after merging main @ 1b56237c (b6abd867), with the conflict above
resolved: npm run build:cached — 3 successful; npm run check:fast — 2
successful (typecheck, lint); npx oxfmt --check . — clean over 3814 files;
npm run docs:check — passed; turbo run lint:built knip — 4 successful.
npm run test:focused -- src/domain/analysisErrorProjection.test.ts src/domain/providerCleanupError.test.ts on b6abd867 — 63 passed (58 in the
projection file, including the four parent-budget cases this branch adds, plus
the 5 cleanup-attribution cases main adds).
npm run test:focused -- tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts on
b6abd867 — 1 passed (3.0 s).
after merging main @ 150e996a (5406eccf), with the import conflict
above resolved: npm run build:cached — 3 successful; npm run check:fast —
2 successful (typecheck, lint); npx oxfmt --check . — clean over 3836 files;
npm run docs:check — passed; turbo run lint:built knip — 4 successful; the
two focused runs above repeated on that revision — 63 and 1 passed.
not run on b6abd867 or 5406eccf: npm run check:pr (the full
test:run, python tests, jscpd, scan:todos) and real-provider
verification. Local Node was 24.21.0 against the pinned 24.18.0.

Compatibility, safety, and release

Breaking changes or migration steps: none.
Real Hopper/Ghidra, browser, or OS coverage: not applicable; JavaScript
analysis only, Linux x64. No provider, process, or native path is touched.
Package or release metadata impact: none.
Security, privacy, process, or containment review: none. Text-only change to
one error's remediation plus the fallback that selects it; no new process,
file, or network behavior. No secrets in the diff.

Author readiness

Human owner: @Antruly

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/3550 **Author:** [@Antruly](https://github.com/Antruly) **Created:** 10/14/2026 **Status:** ❌ Closed **Base:** `main` ← **Head:** `fix/js-parent-budget-remediation-invariant` --- ### 📝 Commits (8) - [`170a3ef`](https://github.com/morluto/rea/commit/170a3ef9de486625bce9933f31b73135f4e01080) fix(javascript): advise the parent result budget after a static checkpoint - [`5ea5a67`](https://github.com/morluto/rea/commit/5ea5a676d41c488a8f4db33e1f342827915e5136) fix(javascript): give the parent budget remediation one owner - [`9e73670`](https://github.com/morluto/rea/commit/9e73670f75647f3d5568acbd1768f102ac0742ea) test(javascript): drive the static-checkpoint parent budget through the CLI - [`f200310`](https://github.com/morluto/rea/commit/f200310dbe6222802cc4ac71b07f8f8d59b34edf) test(javascript): keep the trigger fixture inside the worker's fact budget - [`5dfa560`](https://github.com/morluto/rea/commit/5dfa5600585c3e19cb660e5dcae36aa5b3bd2c08) Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant - [`b6abd86`](https://github.com/morluto/rea/commit/b6abd8676ae972ad50b886a41f6d0a450d963bfd) Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant - [`5406ecc`](https://github.com/morluto/rea/commit/5406eccf88ba11e2bfe9ba9c9d60fd77ebb71c29) Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant - [`7cde632`](https://github.com/morluto/rea/commit/7cde632b49f18d8683111a23251fbc8eb31b63ad) Merge remote-tracking branch 'origin/main' into fix/js-parent-budget-remediation-invariant ### 📊 Changes **5 files changed** (+155 additions, -17 deletions) <details> <summary>View changed files</summary> 📝 `src/domain/analysisErrorPresentation.ts` (+23 -13) 📝 `src/domain/analysisErrorProjection.test.ts` (+61 -0) 📝 `src/domain/javascript/javascriptAnalysisResourceControls.ts` (+10 -0) 📝 `src/javascript/analysis/JavaScriptAnalysisWorker.ts` (+6 -4) ➕ `tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts` (+55 -0) </details> ### 📄 Description ## Summary `analyze_javascript_application` stops the remaining sources when the parent result budget is exhausted while retaining a static checkpoint. That error told the user to "Retry with sufficient memory", which cannot help, because the limit belongs to the parent result budget. Fixes #3538. ## Problem and expected behavior `JavaScriptAnalysisWorker` raised this `AnalysisResourceConstraintError` with `parent_result_budget_exhausted: true` but without a `remediationAction`, so `analysisErrorPresentation` fell back to the generic memory advice. Its three sibling parent-budget paths (module facts, semantic graph, expanded application relationships) already state the opposite: the limit belongs to the parent result budget and a larger worker heap does not raise it. Reported (base revision): ```text code: resource_constraint message: The parent's result capacity was exhausted after retaining static facts for app.asar/node_modules/onnxruntime-web/dist/ort.all.mjs; remaining sources were not analyzed remediation: Review the reported worker memory limits and available host memory. Retry with sufficient memory or a smaller artifact; REA retains tighter inherited limits. ``` After this change: ```text remediation: Use the retained static facts or analyze fewer source files. This limit belongs to the parent result budget; increasing the worker heap alone does not increase it. ``` ## Consolidation with #3545 Draft #3545, opened by the reporter, changes the same worker error to the same sentence. That overlap is real, and it is not surprising: #3538 quotes the sibling remediation verbatim as the expected text, so there is essentially one sentence that satisfies the request, and any fix converges on it. This PR began from the same issue and landed on the same line independently. morluto compared both against main at `fc712b3` and asked for the two sets of protection to be consolidated before either PR is retired: this PR's shared remediation owner and presentation fallback, and #3545's constrained-heap public CLI trigger. `9e73670` takes that trigger into this branch, so this PR now carries both, and #3545 can close once it does. The test comes from #3545 (`b2aa24d`), the file being the only thing that commit needed from it. Rinkia ran it against `5ea5a676` before it landed here and reported it passing. #3538 stays open until this fix is delivered. greptile then reviewed `9e73670` and found the fixture itself at fault, which it was; `f200310` reduces it so that the parent result budget is the only limit the test exercises. See *Review follow-up* for the measurements. I kept the worker line rather than delete it, because the throw site is where the inconsistency with its three sibling paths is visible, and it is the line a future reader goes looking for. The fallback below is not an exact substitute: it composes the same clause, but its leading sentence says "the retained facts", because a producer that states nothing has not said the facts are static, so deleting the worker line would lose that word. An earlier draft of this paragraph claimed the two were equivalent; see *Review follow-up*. What this PR adds beyond the sentence is the second question: where the guarantee should live so that the next producer of this flag cannot make the same mistake. ## Change and scope - `src/javascript/analysis/JavaScriptAnalysisWorker.ts`: pass `{ remediationAction }` to the static-checkpoint parent-budget error. The clause stating the invariant now comes from a shared constant; the leading sentence stays at the site, because what was retained differs per path. - `src/domain/javascript/javascriptAnalysisResourceControls.ts`: own that clause as `JAVASCRIPT_PARENT_RESULT_BUDGET_REMEDIATION`. Two sibling paths in the worker already stated the clause verbatim, and this PR was about to add a third at the throw site plus a fourth in the fallback, of a sentence that must not drift; the four now compose one string, and no user-visible text changes. A fifth path states the same fact inside a sentence of its own. - `src/domain/analysisErrorPresentation.ts`: when a `memory` resource constraint reports `parent_result_budget_exhausted: true` and carries no explicit remediation, return the parent budget remediation instead of the generic memory advice. An explicit `remediationAction` still takes precedence; a constraint of another kind keeps the advice for its own resource, because the clause names the worker heap and does not describe those limits; genuine worker-heap limits keep the existing memory advice. - `src/domain/analysisErrorProjection.test.ts`: four deterministic unit tests (see evidence below). They construct the error directly, so they need no heap window and no worker process. - `tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts`: the end-to-end trigger, taken from #3545. It drives the public CLI under a 256 MiB parent heap and asserts that the run reports `parent_result_budget_exhausted` with the parent budget remediation. The first source is 5,000 lines rather than 30,000, so that the worker's own complete-fact budget is not also in play; see *Review follow-up*. ### Why the fallback sits in presentation selection #3538 asks, optionally, for a test asserting that *every* error reporting `parent_result_budget_exhausted: true` carries a parent-budget remediation, so that new paths cannot regress to the generic memory advice. I did not manage to write that test against the throw site. Reaching it needs a parent heap window and an owned worker process, so an acceptance test can only cover the one path it drives, and a unit test cannot construct that state at all — it would only restate the one string. Where the wrong advice is actually chosen is narrower and easier to pin: `analysisErrorRemediationAction` sees a `memory` constraint with no explicit remediation and falls through to the generic branch. Making the flag the worker already reports an input to that decision turns the invariant into something a plain object can check, and it holds for memory producers that have not been written yet. A constraint of another kind is not described by that clause, so the flag does not redirect it; see *Review follow-up*. For this one path the worker line and the fallback are therefore redundant with each other. I kept both because the explicit line states the intent where the error is built, and the fallback keeps the invariant true where a producer does not state it. That is a preference, not a requirement, and I am happy to be told otherwise. ### Review follow-up rahul05ranjan reviewed `170a3ef` and raised two non-blocking nits. Both were checked against the code and hold, so `5ea5a67` addresses them. - *The two sentences have drifted by one word.* The clause already had two byte-identical copies on main rather than one, and this PR was adding a third and a fourth: the root cause is that one user-visible sentence had no owner, which is the same question this PR asks about the invariant. The structural fix is the shared constant, composed at all four sites; each site keeps its own leading sentence, so nothing the caller sees changes. - *The fallback is not scoped to `memory`.* Confirmed: a constraint of another kind reporting the flag was answered with advice about the worker heap. The clause names the worker heap, so its applicability is the memory constraint; the fallback now requires that kind, and a constraint of another kind keeps the advice for its own resource rather than being aligned with a limit the clause does not describe. A test pins that path (`keeps a constraint's own advice when another resource reports the flag`), and it passes on `origin/main` too, so it pins the scoping rather than the fix. greptile re-reviewed `5ea5a67` at a 5/5 confidence score with no inline findings, then reviewed `9e73670` and raised one P1 against the new fixture. It holds, and `f200310` addresses it: greptile re-reviewed `f200310` at 5/5 with no new findings, and resolved its inline thread. It reviewed the merged head `5dfa560` at 5/5 as well, with no inline findings. - *Fixture exceeds the checkpoint limit* (greptile, P1 on `9e73670`). Confirmed, and it measures exactly: the 30,000-line first source's complete static facts are 9,824,883 bytes against the worker's 8 MiB complete-fact budget, so `JavaScriptAnalysisWorkerServer` raises a transport constraint before writing the checkpoint, and the parent never retains that source's facts. A 4096 MiB parent heap shows the outcome plainly: the run completes with one finding and a per-file observation failure for `a-large.mjs` carrying `resource: "transport"` and "JavaScript static checkpoint exceeds its complete-fact byte budget". Two details differ from the finding as written, and the second is worse than the finding says. The test does not fail while parsing: the transport failure is recorded per file, the analysis continues, and the run's final error is still the flagged parent-capacity error, which is why the file passed here. What is worse is that the over-limit checkpoint was load-bearing for the assertion. With a first source inside the budget, the same 256 MiB heap stops the run on `a-large.mjs` — measured, not inferred — so the original fixture passed only because the transport limit removed the first source from the parent. That is two limits in one test, asserting the parent result budget. `f200310` reduces the first source to 5,000 lines, whose complete static facts are 1,613,878 bytes, 19% of the budget, and asserts the source the message names now. The parent result budget is the only limit left in the test, and the constrained-heap trigger #3545 supplied is otherwise kept. Remaining duplication: the expanded-application-relationships path states `increasing the worker heap alone does not increase it` inside a sentence of its own (`JavaScriptAnalysisWorker`, when returning complete static facts for a file node). It is not byte-identical to the clause, and making it compose the constant would change that path's user-visible text, so I left it in place. Its wording can still drift from the constant, and that is the one copy worth a follow-up if a maintainer wants it folded in. Intentionally not included: budget sizing, ordering app-owned sources before `node_modules`, partial-observation payload size, and `--source-path` hints in the remediation text (all context-only in #3538). Follow the [contribution scope](https://github.com/morluto/rea/blob/main/CONTRIBUTING.md#contribution-scope). Scope decision: not applicable (ordinary fix of user-facing remediation within existing boundaries). ### Branch update The head was 204 commits behind `main`, so the tree under review was not the one a merge would produce. `5dfa560` merges `main` @ `663d07f02` into the branch. Added and removed lines in the PR diff are unchanged by it — the same five files, the same 155 additions and 17 deletions — and the commits greptile reviewed stay in history, so the tree a reviewer checks out is the tree that was measured. Both reproductions were repeated on it; see *Validation performed*. GitHub Actions has now run on this revision. Until 2026-10-15 20:42 UTC the workflow run was held at `completed / action_required`, where GitHub parks runs of fork PRs until a maintainer approves them; once approved it executed to completion and every selected lane passed (run [38795669711](https://github.com/morluto/rea/actions/runs/38795669711)): `Classify changes`, `Static checks`, `Docs Generation`, `Build package`, `Source tests`, `Build bundled Windows x64 native controls`, all five `Test shard` jobs, `binary_layout`, `recorded_crash`, `javascript_recovery`, `web_network`, the four `sqlite-runtime` versions, `Native package baseline (linux x64)`, `Portable tests`, and `CI required`. The lanes that were not selected for this change (`nativeaot`, `website`, `ghidra_bridge`, `evm`, `browser`, `web_runtime`, `web_source_map`, `Real Apple artifact CLI/MCP E2E`, `Reconstruction readiness`, `Source-built conformance fixtures`, `Reverse tools quality`, `Windows capability and package lane`) were skipped by change classification, as on `main`. ### Branch updates while awaiting review (`b6abd867`, `5406eccf`) `main` moved to `1b56237c`, and the branch became unmergeable again (`mergeable_state: dirty`). One file conflicted, `src/domain/analysisErrorPresentation.ts`, and in one hunk: this branch restructures the `AnalysisResourceConstraintError` branch of `analysisErrorRemediationAction` to return an explicit `remediationAction` and to add the exhausted-parent-budget clause, while `main` appended its new `AnalysisTimeoutError` handling directly after that same branch. The two changes are independent, so the resolution keeps this branch's block and appends `main`'s; nothing was adjudicated. `b6abd867` is that merge. The resolution is checkable rather than asserted: against `origin/main`, the merged file's added and removed lines are identical to this branch's own additions and removals against its merge base, so neither side lost a change. The PR diff is still the same five files at the same **155 additions and 17 deletions** as before the merge. `main` also changed the error algebra this branch sits on, without conflicting: `AnalysisResourceConstraintError` gained an optional `retryable` option, the projection's retryability rule became `error.retryable ?? error.resource === "capacity"`, and `analysisErrorBase` gained cleanup-attribution ownership. The parent-budget constraint is `resource: "memory"`, so it stays non-retryable under the new rule, the new option is optional, so this branch's constructions and assertions are unaffected, and `main` did not touch remediation selection for resource constraints. `main` then landed a themed run of error-algebra and cleanup-ownership commits (roughly one every five minutes), so the branch went unmergeable again. Waiting for it to settle produced `5406eccf`, which merges `main` @ `150e996a`. The only conflict was again `analysisErrorPresentation.ts`, and again only textual: this branch adds its remediation-clause import where `main` rewrote the `analysisErrorBase` import to bring in `resolvedAnalysisError`. Both are kept. `main`'s function-body change in that file merged automatically: `analysisErrorRemediationAction`, `analysisErrorCategory`, and `analysisErrorUserMessage` now start by resolving the error. The parent-budget clause reads the resolved error, which is the same view the other presentation functions got, and the focused runs below exercise that path. The same check holds for `5406eccf`: against `origin/main`, the merged file's added and removed lines match this branch's own additions and removals against its merge base, and the PR diff is unchanged at five files, **155 additions and 17 deletions**. ## Contract and boundary impact - Semantic owner and earliest changed stage: `JavaScriptAnalysisWorker` error construction; `analysisErrorPresentation` remediation selection. - Contribution scope fit and linked maintainer decision: not applicable; bug fix for #3538. - CLI and MCP/tool-catalog contract: unchanged shape; only `remediation.action` text changes, for this one error path. - Provider, bridge, target-format, or platform compatibility: none. - Evidence, artifact, provenance, or reconstruction contract: none. Code, category, message and `reported_limits` are unchanged, so the partial observation and its Evidence are unaffected. - Target selection, authority, upstream behavior, and cleanup details: not applicable. - Generated metadata (`docs/public/product-catalog.json`), package, or installation impact: none. Expected effects (check known effects; use documentation-only on its own). Unchecked items do not establish absence; leave uncertainties visible. - [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/analysisErrorProjection.test.ts` — `advises the parent result budget when it is exhausted`, `keeps an explicit remediation when the parent budget is exhausted`, `keeps the worker memory advice without parent exhaustion`, `keeps a constraint's own advice when another resource reports the flag`; and `tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts` — `explains parent budget exhaustion after a static checkpoint`, taken from #3545 with the first source reduced to 5,000 lines. - Complete-fact budget measurements, taken from the worker's own schema parse of each fixture's `a-large.mjs` facts: 9,824,883 bytes for 30,000 lines (117% of the 8 MiB limit, so no checkpoint is written and the source fails as `resource: "transport"`), and 1,613,878 bytes for 5,000 lines (19%). - Base reproduction, presentation: with both changed source files restored to `origin/main` @ `1438422df` and the new tests kept, `npx vitest run src/domain/analysisErrorProjection.test.ts` fails exactly one test — `advises the parent result budget when it is exhausted`, with `expected 'Review the reported worker memory lim…' to contain 'parent result budget'`. That is the string reported in #3538. The three companion tests pass on the base revision, so they are not restating the fix: one pins explicit `remediationAction` precedence, one keeps the generic memory advice for limits a larger worker heap really does raise, and one keeps a non-memory constraint's own advice when it reports the flag. - Base reproduction, end to end: with the same two files restored to `origin/main`, `npm run test:focused -- tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts` reaches the path through the real CLI and fails the same way, `expected 'Review the reported worker memory lim…' to contain 'parent result budget'`. So the flag, the message, and the reported limits were already right on main; the remediation was the only thing wrong, which is the conclusion the unit-level reproduction reaches as well. - User-visible CLI/MCP output: remediation text shown above. - Remaining proof gaps: what drives the end-to-end test is the parent's heap-derived result budget, not the fixture's size. Measured on Linux x64 / Node 24.21.0, with either fixture: a 192 MiB or 384 MiB parent heap completes the run, and 224, 256, 288 and 320 MiB stop it on the parent result budget. 256 MiB sits inside that window, and the test is skipped on Bun as its sibling is. No other platform was run, so the window's width elsewhere is unmeasured; the finding that led to `f200310` was found by reading the fixture rather than by a failure, and the window is the part of this test most likely to need adjustment if another platform disagrees. Everything below was verified on Linux x64 / Node 24.21.0 only. For evidence-bearing changes: - [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/domain/analysisErrorProjection.test.ts` — 57 passed. - `npx vitest run src/domain/analysisErrorProjection.test.ts` with both changed source files restored to `origin/main` — 1 failed (the invariant test), 56 passed; the failure is the reported `Review the reported worker memory lim…` advice. - `npx vitest run --project domain` — 184 files passed, 3032 passed, 3 skipped. - `npm run test:focused -- tests/acceptance/applications/javascriptCheckpointReserveCli.test.ts` — 3 passed, driving the sibling parent-budget path through the real CLI under a 448 MiB and a 640 MiB parent heap and asserting that the remediation names the parent result budget. That is also what shows the shared clause still reaches a real caller. - `npm run test:focused -- tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts` — 1 passed (3.0 s), driving the static-checkpoint parent budget through the real CLI under a 256 MiB parent heap. - the same command with the two changed source files restored to `origin/main` — 1 failed, on `expected 'Review the reported worker memory lim…' to contain 'parent result budget'`, with the message assertion naming `a-large.mjs` passing, so the run reaches the parent result budget and not the transport limit. - the fixture under a 4096 MiB parent heap, before and after the reduction — the reduced fixture completes with both checkpoints transferred, so no complete-fact budget is reached. - the fixture under 192, 224, 256, 288, 320 and 384 MiB parent heaps — 192 and 384 complete; the rest stop on the parent result budget naming the first source. - after merging `main` @ `663d07f02` (`5dfa560`), on a tree rebuilt with `npm ci` and `npm run build:cached`: `npm run test:focused -- src/domain/analysisErrorProjection.test.ts tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts` — 59 passed (58 unit, 1 CLI at 2.6 s). - the same command with the three changed source files restored to the merged `origin/main` — 2 failed, 57 passed: the unit invariant case and the CLI case, both on the reported `Review the reported worker memory lim…` advice. The unit file holds one more case than the independent reproduction in this thread saw (58 against 57), because `main` added a browser-budget case to it in `57a23f0`. - after the merge: `npm run check:fast` — exit 0 (typecheck, lint), and `npx oxfmt --check` on the five changed files — clean. - `npx tsc --noEmit` — exit 0. - `npx oxfmt --check .` — all files use the correct format. - `npm run lint` — exit 0, module boundaries checked in 1885 source files, including the new `domain/analysisErrorPresentation` → `domain/javascript/javascriptAnalysisResourceControls` dependency. - after merging `main` @ `1b56237c` (`b6abd867`), with the conflict above resolved: `npm run build:cached` — 3 successful; `npm run check:fast` — 2 successful (typecheck, lint); `npx oxfmt --check .` — clean over 3814 files; `npm run docs:check` — passed; `turbo run lint:built knip` — 4 successful. - `npm run test:focused -- src/domain/analysisErrorProjection.test.ts src/domain/providerCleanupError.test.ts` on `b6abd867` — 63 passed (58 in the projection file, including the four parent-budget cases this branch adds, plus the 5 cleanup-attribution cases `main` adds). - `npm run test:focused -- tests/acceptance/applications/javascriptStaticCheckpointBudgetCli.test.ts` on `b6abd867` — 1 passed (3.0 s). - after merging `main` @ `150e996a` (`5406eccf`), with the import conflict above resolved: `npm run build:cached` — 3 successful; `npm run check:fast` — 2 successful (typecheck, lint); `npx oxfmt --check .` — clean over 3836 files; `npm run docs:check` — passed; `turbo run lint:built knip` — 4 successful; the two focused runs above repeated on that revision — 63 and 1 passed. - not run on `b6abd867` or `5406eccf`: `npm run check:pr` (the full `test:run`, python tests, `jscpd`, `scan:todos`) and real-provider verification. Local Node was 24.21.0 against the pinned 24.18.0. ## Compatibility, safety, and release - Breaking changes or migration steps: none. - Real Hopper/Ghidra, browser, or OS coverage: not applicable; JavaScript analysis only, Linux x64. No provider, process, or native path is touched. - Package or release metadata impact: none. - Security, privacy, process, or containment review: none. Text-only change to one error's remediation plus the fallback that selects it; no new process, file, or network behavior. No secrets in the diff. ## Author readiness Human owner: @Antruly - [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-10-31 01:09:39 +00:00

closed this issue
added the
pull-request
label

bsakaguchi referenced this issue

2026-11-01 21:45:15 +00:00

[PR #5816] [MERGED] fix(hopper): keep an observed launcher exit over a later cancellation #7082

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#5816

No description provided.

Rows
Columns