[Pro] Cap the size of external source maps read by the node renderer - #4688
Conversation
External `.map` files were read with no size check. `MAX_INLINE_SOURCE_MAP_BYTES` (50MB) only guarded `data:` URL maps, so a large external map was read in full and retained for the life of each pooled VM. External maps now get the same 50MB ceiling as inline maps. The size comes from the `statSync` that `resolveReadableSourceMapPath` already performs for its `isFile()` check, so no extra syscall is added. An oversized map is skipped on both the async preload path and the synchronous lazy path, and warns once per map path — the async preload runs at every VM build and the sync path can run per error, so an unthrottled warning would flood the log on a rolling deploy. Behavior change: frames for a bundle whose external map exceeds the cap keep their bundled locations instead of remapping to original sources. This is a pre-read size gate, not a hard memory bound: a map that grows between the stat and the read is still read in full. Scope: implements only the narrow cap sanctioned in the issue discussion. It does not fix the issue's headline complaint that bundles which never error still retain their full map size. Removing that retention requires dropping the eager preload, which provably breaks generation isolation — the "same-path rebuild does not remap active old VM errors with the new source map" test overwrites the map on disk and then asserts an old in-flight error still remaps to the original source, so the preload retains the only surviving copy of that generation's map. A fingerprint can detect staleness but cannot recover destroyed bytes. That tradeoff is a product decision and stays parked. Partially addresses #4313. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe Node renderer caps external source-map reads at 50MB, skips oversized maps while preserving bundled stack locations, warns once per map path, and applies the behavior to synchronous and asynchronous loading. ChangesExternal source map size cap
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant Bundle
participant NodeRenderer
participant FileSystem
participant Logger
Bundle->>NodeRenderer: request stack remapping
NodeRenderer->>FileSystem: inspect external .map size
FileSystem-->>NodeRenderer: return map size
NodeRenderer->>Logger: warn once when map exceeds 50MB
NodeRenderer-->>Bundle: remap or preserve bundled locations
Possibly related issues
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR limits external source maps loaded by the Pro Node renderer. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (2): Last reviewed commit: "Do not let an oversized fallback map ret..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.github.com/in/867647?s=60&v=4){kind=small}
![chatgpt-codex-connector[bot]](https://avatars.github.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9f002782ef
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
ReviewScope matches the description: this is a narrow, well-justified cap-only change (external Overview
Minor observations (not blockers)
Neither of these affects correctness of the cap itself — both are steady-state efficiency/cleanliness nits. Nice, well-scoped change overall. |
![claude[bot]](https://avatars.github.com/in/1236702?s=60&v=4){kind=small}
Review SummaryThis is a well-scoped, well-documented change: it applies the existing 50MB inline-source-map cap ( Strengths
Minor observations (non-blocking)
No correctness, security, or performance red flags found. The path-containment/realpath checks are unchanged, and the size check sits before the file read on both paths as intended. |
size-limit report 📦
|
![coderabbitai[bot]](https://avatars.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/react-on-rails-pro-node-renderer/tests/vmSourceMapSupport.test.ts`:
- Around line 312-345: Update both oversized external-map tests in
packages/react-on-rails-pro-node-renderer/tests/vmSourceMapSupport.test.ts
(anchor lines 312-345 and sibling lines 347-381) to resolve
fs.realpathSync(mapPath) once and use that resolved path in the
readFileSyncSpy/readFileAsyncSpy not-to-have-been-called assertions; preserve
the existing 'utf8' argument and all other test behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 08004026-96cd-4e69-8178-984c46385f61
📒 Files selected for processing (3)
CHANGELOG.mdpackages/react-on-rails-pro-node-renderer/src/worker/vmSourceMapSupport.tspackages/react-on-rails-pro-node-renderer/tests/vmSourceMapSupport.test.ts
Review feedback from @greptile-apps (P1) and @chatgpt-codex-connector (P2) on #4688: returning `undefined` for an oversized map made it indistinguishable from "no map here", so `readSourceMapJsonForBundle`'s candidate loop fell through to the conventional `<bundle>.js.map` fallback. When that fallback is a different or stale map, frames were remapped to the WRONG sources rather than keeping their bundled locations as the cap advertises — worse than not remapping at all. Reproduced first: a bundle naming an oversized `explicit-oversized.js.map` with a stale `<bundle>.js.map` alongside it remapped to the stale map's source. `resolveSourceMapPathWithinSizeLimit` now returns a discriminated usable/oversized/unusable result, and the readers surface `oversized` distinctly. The candidate loops stop on it and return `null` ("found, permanently unusable") instead of `undefined` ("nothing here, may arrive later"). `preloadSourceMapJsonForBundle` maps that `null` to `retryMissingSourceMap: false`, which also closes the second half of the codex finding: an oversized preload left the miss retryable, so a later same-path map could remap an old VM generation and break generation isolation. Also from review: - Bound `warnedOversizedSourceMapPaths` as an insertion-ordered FIFO (256 entries). It previously grew one entry per distinct oversized map path for the process lifetime, which is ironic in a memory-retention PR (@claude). Evicting the oldest only risks a duplicate warning; clearing on unregister would re-warn on every VM rebuild and defeat the throttle. - Assert on the realpath in the read-spy assertions, since the readers read the realpath and macOS resolves /var -> /private/var, so a regression could have slipped past (@coderabbitai). - Fix "%d bytes exceeds" subject/verb agreement in the warning (@claude). Two tests added, both verified to fail against the previous commit: the stale-fallback repro and terminal-on-preload (an under-cap map appearing later must not remap that generation). 47/47 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The previous commit made oversized maps terminal only at preload. A registration
that was lazy and retryable (map absent at build) still ran the retry budget when
the map later turned up oversized — so if that same path subsequently became
under-cap, this VM generation could remap with the newer file. That is the same
wrong-remap class the previous commit fixed for preload, and it made the CHANGELOG
claim ("will not retry the map for that VM generation") false for the lazy path.
`loadSourceMapForBundle` now returns a discriminated loaded/terminal/missing result
instead of `LoadedSourceMap | null`, which was collapsing the reader's `null`
(oversized, terminal) and `undefined` (nothing yet, retryable) back together right
after the previous commit went to the trouble of separating them.
`sourceMapForRegistration` caches the miss immediately on `terminal` and keeps the
existing retry budget for `missing`.
Deliberately still retryable: JSON.parse failures on a partially-written map, so a
completed map is still picked up (covered by the existing partial-content tests),
and createSourceMap returning null on old Node, to avoid an unrelated behavior change.
Found by Codex (xhigh) reviewing the previous commit. Regression test added and
verified to fail against it: missing at preload, oversized on first error, under-cap
later, must keep the bundled location. 48/48 green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review (commit e2cf33a)Re-reviewed the latest revision. This is a well-scoped, well-tested change (external Two nits from earlier review rounds are now fixed:
One subtlety worth a second pair of eyes (not blocking): the "oversized" signal is now encoded two different ways depending on when it's discovered:
Still outstanding from a prior review round (minor, explicitly out of this PR's stated scope): No security or performance concerns beyond what's already documented in the PR description (the TOCTOU window between |
Review nit from @claude on e2cf33a: `loadSourceMapForBundle`'s ternary coerced a registration's own `sourceMapJson: null` ("confirmed no usable map") to `undefined` via `?? undefined`, so it resolved as `missing` rather than `terminal`. It still settled correctly, but only because `sourceMapForRegistration` separately checks `shouldRetryMissingSourceMap`, which is only false because `preloadSourceMapJsonForBundle` happens to always pair `sourceMapJson: null` with `retryMissingSourceMap: false`. That invariant is not type-enforced, so the correctness was resting on a coincidence. `null` on a registration now returns `{ status: 'terminal' }` explicitly, before the ternary. Same behavior today, no longer dependent on the pairing holding. Honest note on the accompanying test: it is a guard, not a regression reproducer. I verified it passes against the previous commit too, because the `??` coercion already prevented the read and the only difference was retry-counter churn, which is not observable through this API. It pins the invariant the reviewer noted was untested (`sourceMapJson: null` + `retryMissingSourceMap: true` must still never read the map) so a future refactor cannot quietly break it. 49/49 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
@claude — on the last outstanding point, the inline ( I checked whether inline oversized maps have the same defect the external ones did, since that would have changed the answer. They do not.
So the gap is observability only — an oversized inline map is correctly ignored, just silently. Adding a warning there means either warning on every Filing it as a follow-up rather than doing it here. Thanks for the thorough re-review — the state-machine trace across both paths is what surfaced the |
|
Claude finished @AbanoubGhadban's task in 1m 12s —— View job Tasks
VerificationTraced all four inline-map paths against
So the claim holds: an oversized inline map is already handled as a correct terminal One small correction to the writeup, immaterial to the conclusion: Deferring this as a follow-up rather than folding it into a cap-only PR on a |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 75d809fc39
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
|
Review Overview Code quality
Test coverage Security Minor observations (non-blocking, also left as inline comments)
Verdict |
Review finding from @chatgpt-codex-connector (P2) on 75d809f, reproduced before fixing. Making an oversized map terminal was too aggressive at the candidate-loop level: A bundle names `named.js.map`, which is missing at build (the supported late-arriving-map flow), while an oversized `<bundle>.js.map` sits at the conventional fallback path. The loop skipped the missing named map, hit the oversized fallback, and returned the terminal `null` — so `preloadSourceMapJsonForBundle` registered `sourceMapJson: null, retryMissingSourceMap: false` and the named map was never picked up when it arrived. The frame kept its bundled location forever. The two goals are distinct: an oversized candidate must stop *substitution* (never remap through a map the bundle does not name — the original P1), but it must not retire the *retry* for a higher-priority candidate that was merely missing and can still show up. The loops now track whether a higher-priority candidate was missing and return the retryable `undefined` in that case, keeping the terminal `null` for when the oversized map is itself the best candidate. Also documents two things @claude noted: - `MAX_EXTERNAL_SOURCE_MAP_BYTES` and `MAX_INLINE_SOURCE_MAP_BYTES` share a number but not a unit (bytes on disk vs UTF-16 string length), so the alias implies a parity that does not quite hold. Pre-existing; noted rather than changed. - A warned path is never re-armed, so a reused path whose map dips under the cap and grows back may not warn twice. The map is still skipped correctly; only the warning is suppressed. Regression test added, verified to fail against 75d809f. The existing oversized-terminal tests still pass, so this does not regress the original fix. 50/50 green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review: Cap the size of external source maps read by the node rendererOverview: Extends the existing 50MB inline-source-map cap to external CorrectnessI traced the tri-state logic (
All of this checks out — I didn't find a correctness bug in the branching logic, and it's consistent with the four scenarios called out in the test suite plus a few I traced manually (e.g., the FIFO warn-dedup eviction correctly evicts oldest-first via SecurityVerified the cap is the only path that reads external maps — Minor observations (non-blocking)
Test coverageThe 9 new tests are well-targeted: async preload path, sync lazy path, boundary equality, candidate-priority interactions (oversized named vs. stale fallback, oversized fallback vs. late-arriving named), terminal/no-retry behavior on both preload and lazy discovery, and warn-dedup. The "vacuity check" described in the PR body (tests fail when only the Overall: a tightly-scoped, well-tested fix with careful attention to the existing generation-isolation invariants. No blocking issues found. |
|
@coderabbitai — you were right about the CHANGELOG, fixed in 8c2910f. The entry claimed an oversized map "will not retry the map for that VM generation" unconditionally, which stopped being true in
This is the second time a reviewer caught this exact class of drift on this PR (@chatgpt-codex-connector caught the same overstatement about the lazy path earlier), which is a fair signal about my changelog discipline. On the Thanks for the exhaustive enumeration; it matches the one I ran independently, which is a useful cross-check on a state machine that has now had two real bugs in it. |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git log --oneline -5 --all | grep -i "8c2910f" || true
git show 8c2910f7 -- CHANGELOG.md || trueLength of output: 2647
Agreed on leaving the Nothing else outstanding from my side on this thread. |
ReviewSolid, well-scoped PR — the implementation matches its own extensive design notes and the test suite exercises every corner case called out in the description (terminal-vs-retryable, generation isolation, priority ordering, boundary at exactly the cap, warn-once). Code quality
Minor observations (non-blocking)
Correctness spot-checkTraced through the priority-fallback logic by hand for the tricky cases:
No functional bugs found. Test coverage is thorough and matches the documented scope (async preload path, sync lazy path, boundary, warn-once, terminal-not-retried, priority-fallback interactions). CHANGELOG entry follows existing formatting/conventions. |
Review round complete — all threads addressed and resolvedTwo of the findings were real bugs I had introduced. Both were reproduced with a failing test before any fix, and both regression tests were verified to fail against the commit that shipped the bug. Must-fix (both confirmed by repro, not taken on faith)1. Oversized map fell through to a stale fallback — P1, @greptile-apps + @chatgpt-codex-connector ( Fixed with a discriminated 2. My fix for #1 was too aggressive — P2, @chatgpt-codex-connector ( 3. Lazy path + registration state — @claude ( Also fixed
Declined, with reasons
One correction against my own favorI added a test for the Status
Scope unchanged: this remains the narrow cap @justin808 sanctioned. It still does not fix the issue's headline complaint (zero-error bundles retaining their map), which can't be fixed without breaking the generation-isolation guarantee test |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes.
|
|
Claude finished @AbanoubGhadban's task in 2m 6s —— View job Final verification complete
I re-read
I wasn't able to execute the Jest suite myself in this session (the sandbox declined the Assessment: the state machine holds together under inspection — the fixes for the two real bugs (stale-fallback fallthrough, and the fix-for-the-fix that restored late-arriving-map retries) are both present and match their descriptions, and the two open "declined" items (inline-map warning asymmetry, |
|
This is a clean re-review pass. The final state of the code is solid — the reported bugs are genuinely fixed and the logic is correct throughout. What I verified in the diff: The The The FIFO eviction on The One minor note (non-blocking): the 50/50 green, all threads resolved, scope maintained. This looks ready for merge pending maintainer sign-off on the product scope decision. |
No behavior change; 50/50 tests unchanged, including both bug-repro regression tests
and the generation-isolation test.
The cap landed as its own layer on top of path resolution, which left four encodings
of the same tri-state chained through six levels. `resolveReadableSourceMapPath` had
exactly one caller and returned `{sourceMapPath, sizeInBytes}` purely to hand the size
to that caller — a value with no other consumer.
Folding the cap into `resolveReadableSourceMapPath` (which already ran the `statSync`
for its own `isFile` check) deletes `resolveSourceMapPathWithinSizeLimit`, the
`ResolvedSourceMapCandidate` union, and that object shape. `SourceMapFileReadResult`
becomes `SourceMapCandidateResult`, shared by resolve and read since their state spaces
are identical. Net -26 lines.
The containment order is unchanged and still ends with the read: lexical containment ->
realpathSync -> realpath containment -> statSync/isFile -> size check -> read. The size
check only ever runs on a realpath already proven inside the real bundle directory, so
the symlink-swap protection and its codeql justification are untouched. Still one
`statSync` per candidate.
Also drops a provably dead clause in `sourceMapForRegistration`:
`(sourceMappingUrl === null && !retryMissingSourceMap)` implies `!shouldRetryMissingSourceMap`,
so it could never be the deciding condition. It predates this PR, but this PR already
rewrote that branch.
Corrects the warn-throttle comment. It claimed the sync path floods "per error", which is
false — terminal caching means that path is never re-entered. The real driver is that
preload re-checks on every VM build, so an app with more bundles than `maxVMPoolSize`
(default 2) rebuilds per request and would warn at request rate.
Kept deliberately, per review: the OVERSIZED_SOURCE_MAP symbol (it means "this candidate
is oversized", which is NOT the higher level's "resolution is terminal" — `[missing,
oversized]` returns the symbol at one level and a retryable undefined at the next;
collapsing them would recreate the conflation behind the original P1), and
`SourceMapLoadResult` / `higherPriorityCandidateMissing`, which are the minimal state
for those two fixes.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review: Cap the size of external source maps read by the node rendererOverview: this narrowly extends the existing 50MB inline-source-map cap to external Code quality
Minor observation (not blocking)
TestsCoverage is thorough — preload vs. lazy-sync paths, exact-boundary ( SecurityNo concerns — the size check runs only on an already realpath-validated, contained file, and the warning only logs the path/size server-side. PerformanceAs claimed: the cap reuses the existing Overall: solid, well-scoped, well-tested change. No blocking issues found. |
…ottle
The "warns once per map path" test was verified to be vacuous: with the dedup guard
neutralized it still passed. It called runInVM twice, but the first oversized detection
caches the terminal answer, so the second error never re-checks — exactly one warning
happened with or without dedup. It asserted nothing the preload test did not already
assert, and the throttle itself had no coverage.
Rewritten to exercise the repeat that actually matters: preload re-checks the map on
every buildVM, so the test builds, evicts via a second bundle at maxVMPoolSize 1, and
rebuilds. With the dedup removed it now fails (expected 1 warning, received 2) — the
check the old version passed.
Deleted `a registration with a confirmed no-map result never reads, even if marked
retryable`. It constructed a `sourceMapJson: null` + `retryMissingSourceMap: true`
registration that preload never emits, and the realistic version of that regression is
already covered by the preload-terminal test. It was also vacuous against the commit it
shipped in, which was reported on the PR.
Decoupled the warning assertions from the message wording. They pinned
`stringContaining('exceeds the')`, and this PR reworded that message — it survived by
luck. A warning naming the map is the requirement; its prose is not. They now assert the
data the warning carries (map path, size, limit) with `expect.any(String)` for the format.
The boundary test's negative assertion is matched by the map it names instead, since a
bare `expect.any(String)` there would have matched any 4-argument warning and weakened it.
Break-it verified, each reverted after: dedup removed -> the rewritten throttle test fails;
oversized-returns-undefined (the original P1) -> the fallthrough test fails with
RebuiltBoom.ts, the stale map's source; higherPriorityCandidateMissing never set -> the
late-arriving-map test fails. 49/49 green.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Review: Cap the size of external source maps read by the node rendererScope check: This matches the PR description exactly — a narrow, cap-only change to Code quality
BugsI didn't find a correctness bug. Specifically checked and found consistent:
SecurityThis is a net security improvement (bounds a previously-unbounded memory read from external PerformanceNegligible overhead: one integer comparison per candidate resolution, and the warn-dedup Test coverageGood — the four scenarios named in the PR description are all present, plus two extra (fallback-oversized-with-named-map-arriving-late, and warn-dedup-across-rebuilds) that specifically probe the trickier terminal/retryable interaction. I did not execute the suite in this environment (sandboxed), so I'm relying on the PR's stated 45/45 pass + vacuity-check results rather than reproducing them. Minor suggestion (non-blocking)Left as an inline comment: |
Review: Cap the size of external source maps read by the node rendererRead through Correctness
Minor / non-blocking observations
Security
Test coverageThe 8 new tests in the No blocking issues found. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b6b88663f1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Merge readiness (batch ror-c-20260716-1543-pro-node-residuals)Confidence note: merging under batch merge_authority Evidence at head
Follow-up commitments carried into the next batch PR: qualify the changelog memory-cap wording (pre-read gate), normalize the data:-branch tri-state, export the byte-cap constant for tests. [lane ror-c-4313-koa] |
|
Completed-batch audit: replay evidence follows. |
2 participants
Partially addresses #4313.
Scope — please read first
#4313 is labeled
parked/P3. In this comment @justin808 recommended no implementation PR for now, while allowing that a maintainer could "split a later narrow cap-only change with targeted source-map tests."This PR is exactly that narrow cap-only change, and nothing more. It deliberately does not fix the issue's headline complaint. See "What this does not fix" below — that part is a product decision, not an oversight.
Problem
MAX_INLINE_SOURCE_MAP_BYTES(50MB) is referenced only inparseDataUrlSourceMap, so it guardsdata:URL maps only.readSourceMapFileandreadSourceMapFileAsyncread external.mapfiles with no size check at all. A large external map is read in full and retained for the life of each pooled VM.Change
External maps get the same 50MB ceiling as inline maps.
statSyncthatresolveReadableSourceMapPathalready performs for itsisFile()check, so the cap costs no extra syscall.resolveReadableSourceMapPathnow returns{ sourceMapPath, sizeInBytes }, and a newresolveSourceMapPathWithinSizeLimitapplies the cap once for both the sync and async readers.buildVM) and the synchronous lazy path (used when a map lands after the bundle).Behavior change (intentional, called out in CHANGELOG)
Frames for a bundle whose external map exceeds 50MB keep their bundled locations instead of remapping to original sources, and the renderer logs a warning naming the map.
Honest limitation
This is a pre-read size gate, not a hard memory bound. A map that grows between the
statSyncand thereadFileSyncis still read in full. Stated in the code comment and CHANGELOG too — it should not be oversold as absolute protection.What this does not fix
The issue's headline complaint is that a bundle which never errors still retains its full map size for the life of the pooled VM. This PR does not address that, and the obvious fix — dropping the eager preload so the existing lazy path runs — is provably wrong:
The existing test
same-path rebuild does not remap active old VM errors with the new source mapoverwrites the.mapon disk with a rebuilt map, then asserts that an old in-flight error still remaps to the original source. So the preload is not a cache — it retains the only surviving copy of that generation's map. A fingerprint ({size, mtimeMs, ino}) can detect staleness but cannot recover bytes that no longer exist on disk; it would return a raw stack and fail that assertion.So removing the retention necessarily breaks a deliberately-encoded guarantee. The real tradeoff is:
That is a product call for @justin808, not one to make inside a cap PR. Left parked; I'll write it up on the issue.
Also deliberately excluded: drop-raw-JSON-after-parse (needs a "raw intentionally discarded" sentinel or a future parsed-map LRU would silently fall back to disk and reintroduce the stale-map risk the preload exists to prevent), the parsed-map LRU itself, and the
extractSourceMappingUrltail-scan.Testing
Four targeted tests added (
external source map size capdescribe block):readFile/readFileSyncasserted not called), warn logged.>/>=flip.Verification performed:
external .map file next to the bundle is used(asserts the preload path does no sync read) andsame-path rebuild does not remap active old VM errors with the new source map(generation isolation).srcchange reverted and the tests intact, tests 1, 2 and 4 fail. Test 3 passes either way by design — it is a boundary guard, not a reproducer.eslintclean;type-checkshows 4 pre-existing errors (opentelemetry version skew + an unbuilt workspace dep), identical with the change reverted, none invmSourceMapSupport.ts.Planning, implementation, and review were done in a loop with Codex CLI (GPT-5.2, xhigh reasoning) as an adversarial peer. Codex refuted my original fingerprint-based design by finding the
:895test — that refutation is why this PR is cap-only.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
.maploading in the Node renderer to 50 MB (matching inline limits). Oversized maps are skipped as terminal, no fallback/remap retries occur, and stack frames remain at bundled locations.Tests
Documentation
Completed-batch audit
Status: Follow-ups remain — see the durable receipt. Durable receipt.