Repository navigation
Compute diagnostic positions incrementally to fix quadratic hang in Bun.JSONC.parse - #31273
Conversation
WalkthroughThis PR optimizes diagnostic message generation by introducing incremental line/column tracking. It adds ChangesIncremental line/column tracking for diagnostic performance
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
|
Updated 12:34 PM PT - May 23rd, 2026
✅ @robobun, your commit 5399754f4dddf0c632e4c566507a9f561fe398bf passed in 🧪 To try this PR locally: bunx bun-pr 31273That installs a local version of the PR into your bun-31273 --bun |
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
LGTM — focused perf fix with an opt-in trait hook, default behavior unchanged for JS/TOML lexers, and a solid regression test.
Extended reasoning...
Overview
This PR caps the number of diagnostics a single JSON-family parse will log at 25, fixing quadratic-time error reporting on pathological inputs (each logged message rescans the source for line/column). It touches four files: src/ast/lexer_log.rs adds an opt-in remaining_errors_mut() hook (default None) and gates the two provided error-logging methods on it; src/parsers/json_lexer.rs defines MAX_LOGGED_DIAGNOSTICS = 25 and opts the JSON lexer in via a new remaining_errors field; src/parsers/json.rs adds a parallel remaining_duplicate_key_warnings budget for the duplicate-key warning path; and test/js/bun/jsonc/jsonc.test.ts adds a 30s-timeout subprocess regression test for both the malformed-flood and duplicate-key-flood cases.
Security risks
None. This is purely a diagnostics-throttling change with no auth, crypto, or untrusted-input handling beyond what already existed. If anything it reduces a DoS-adjacent surface (a ~250 KB malformed Bun.JSONC.parse input previously hung for over a minute).
Level of scrutiny
Low-to-moderate. The shared LexerLog trait is touched, but the new hook defaults to None so the JS (src/js_parser/lexer.rs:430) and TOML (src/parsers/toml/lexer.rs:101) implementers are unaffected — only the JSON lexer opts in. The budget check uses checked_sub and sits after the existing prev_error_loc dedup, returning early on the same Ok(()) path callers already handle. The duplicate-key gate runs after dup_map.insert, so dedup state is still maintained and only the warning is suppressed. Parse results are unchanged; only diagnostic count is capped.
Other factors
No CODEOWNERS cover these paths. The bug-hunting system found no issues. The only user-visible behavior change is that Bun.JSONC.parse's AggregateError.errors is now bounded, which is the explicit intent and is exercised by the new test. The cap value of 25 is a minor UX constant, not a large-scale design decision.
|
@robobun terrible fix. Completely redo this PR. Simply make it faster instead. Do not solve quadratic behavior by limiting the count. Solve it by fixing the underlying cause. |
|
Found 1 issue this PR may fix:
🤖 Generated with Claude Code |
…ing per message Every logged diagnostic computed its line/column by walking the source from byte 0 (Source::init_error_position), so a parse that reports many diagnostics against one file paid O(messages x file size). The lenient JSONC/JSON parsers recover from errors and can report one per token, which made Bun.JSONC.parse quadratic on malformed input: a 250 KB fuzz input spent minutes inside Lexer::expected_string -> LexerLog::add_range_error computing positions. Duplicate-key warnings hit the same cost on valid input. Give Log a LineColumnTracker (the esbuild approach): a resumable scanner keyed to the current source that picks up from the previous diagnostic's offset instead of restarting, with the end-of-line offset cached per line. All Log message constructors route through it. Out-of-order offsets and offsets inside a multi-byte codepoint fall back to the one-off full scan, so results are identical to init_error_position, which is now expressed through the same shared scan helpers. No limits on diagnostic counts; N diagnostics over one source now cost O(file size + N) for position computation instead of O(N x file size).
95ac8e2 to
65393a1
Compare
robobun
changed the title
|
Redone without any limits, per the feedback: the quadratic came from every diagnostic recomputing its line/column by rescanning the source from byte 0, so the fix makes that computation incremental instead. |
Log is embedded by value in size-sensitive types (TaskError::Patching in the isolated installer), and the inline tracker grew it past clippy's large_enum_variant threshold there. Box the tracker and allocate it on the first located diagnostic instead; Log only grows by one pointer.
The tracker's source-identity check is pointer-based; drop it together with the messages in reset()/clear_and_free() so reused Logs never carry scan state across a reuse boundary.
Same invariant as reset()/clear_and_free(): every method that clears this log's messages also drops the pointer-keyed line/column scan cache.
There was a problem hiding this comment.
All my earlier nits are addressed and I found nothing further on 9247649, but this is a ~400-line rework of core logger infrastructure that was redone from scratch after maintainer feedback — worth a human sign-off on the new design.
Extended reasoning...
Overview
This PR replaces the O(offset)-per-diagnostic line/column scan in Source::init_error_position with an incremental LineColumnTracker owned by Log, and routes all ~20 Log::add_* message constructors through a new tracked_range_data helper. The existing scan loop is factored into shared helpers (ErrorPositionState::advance, scan_line_end) so the one-shot and incremental paths run identical byte-handling logic. Log gains a new lazily-boxed Option<Box<LineColumnTracker>> field that is dropped in reset/clear_and_free/append_to/append_to_with_recycled. Differential unit tests in src/ast/lib.rs and a kill-switch integration test in test/js/bun/jsonc/jsonc.test.ts are added.
Security risks
None identified. The change is internal to diagnostic-position computation; no new inputs are parsed, no I/O or auth surfaces are touched. The pointer-identity cache is keyed on usize-erased pointers (kept Send) and is never dereferenced — it only gates whether to resume vs. restart a scan over the current &Source slice, so there is no UAF or out-of-bounds risk. Worst-case misbehavior is a cosmetically wrong line/column, and the four-way ptr+len identity check plus the now-consistent cache-drop on every msg-clearing path makes even that effectively unreachable.
Level of scrutiny
High. src/ast/lib.rs is the shared logger used by every parser in Bun (JS/TS, JSON/JSONC, TOML, CSS, bundler, resolver), so a regression here would affect diagnostic output across the whole product. The change is not mechanical: it introduces pointer-identity-keyed cache state on a long-lived struct, a resumable scanner with a cached line_end invalidated on line-break crossings, and a fallback for out-of-order / mid-codepoint offsets. The PR was also completely rewritten after the maintainer rejected the original cap-based approach, and that maintainer has not yet reviewed the new version.
Other factors
On the positive side: the refactor is careful (the old init_error_position is now expressed through the same helpers, so there is exactly one copy of the `\r`/`\r\n`/U+2028/U+2029 logic), the differential tests exhaustively compare tracker vs. full-scan over forward/backward/strided/repeated offsets across a corpus including CRLF, lone CR, Unicode line separators, multi-byte and invalid UTF-8, and source-switching; and all three of my earlier nits (stale description, reset/clear_and_free not dropping the tracker, append_to* not dropping the tracker) have been resolved. I did not find any correctness issues in the final revision. The reason I'm not approving is scope and provenance, not a specific concern: a redesigned fix to core infrastructure that a maintainer explicitly asked to be redone should get that maintainer's eyes before merge.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@src/ast/lib.rs`:
- Around line 1571-1577: The field line_column_tracker on struct Log (and the
LineColumnTracker type) are internal implementation details and should not be
part of the crate's public API; change the visibility of line_column_tracker
from pub to pub(crate) or private and mark LineColumnTracker as pub(crate) or
private as well so external callers cannot construct or manipulate the cache
directly and must go through Log's public methods (reset/append); update any
internal references to use the tightened visibility and ensure tests or other
crate modules import the type with the new visibility.
In `@test/js/bun/jsonc/jsonc.test.ts`:
- Line 195: The test call that currently passes an explicit timeout (the
trailing "90_000" argument on the test(...) invocation in jsonc.test.ts) should
have that per-test timeout removed; edit the test(...) invocation that ends with
"}, 90_000);" to drop the numeric timeout so it becomes a plain test(callback)
call and rely on the subprocess/global timeout (timeout: 60_000) used for tests
under test/js/bun/**, keeping only explicit timeouts where RSS-leak exceptions
are required.
🪄 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: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: c4f7fe10-9a43-4978-8530-dd1390a11180
📒 Files selected for processing (2)
src/ast/lib.rstest/js/bun/jsonc/jsonc.test.ts
Problem
Fuzzing found a hang in the JSONC parser: a 250 KB malformed input keeps
Bun.JSONC.parsebusy for over a minute, sampled spinning inSource::init_error_position←Location::init_or_null←Log::add_formatted_msg←LexerLog::add_range_error←Lexer::expected_string.Synthetic reproduction (release build):
12.5 KB → 152 ms · 25 KB → 644 ms · 50 KB → 2.4 s — doubling the input quadruples the time.
The same blowup exists through duplicate-key warnings on valid input:
'{' + '"a":1,'.repeat(40_000) + '"a":1}'(240 KB) parses successfully but takes 11 s. The same code path parses tsconfig.json, package.json, and.jsonimports.Cause
Every logged diagnostic computes its line/column in
Source::init_error_position, which rescans the source from byte 0 — O(offset) per message. The lenient JSONC/JSON parsers recover from errors rather than bailing, so a desynced input produces a diagnostic for nearly every token: ~150k messages × O(130 KB average scan) ⇒ quadratic.Fix
Make position computation incremental instead of limiting anything — the
Lognow owns aLineColumnTracker(the esbuild approach):Logmessage constructors route theirLocationcomputation through the tracker (tracked_range_data), so lexer errors, parser errors, and warnings (including the duplicate-key path) all benefit — for any parser that logs throughLog, N diagnostics against one source now cost O(file size + N) instead of O(N × file size).Source::init_error_position. That function is now expressed through the same shared scan helpers (ErrorPositionState::advance,scan_line_end), so there is exactly one copy of the\r/\r\n/U+2028/U+2029 handling.No caps or truncation of diagnostics anywhere.
Measurements
Scaling is now linear (5× input ⇒ ~5× time); release builds are several times faster than the debug+ASAN column. The remaining per-message cost is formatting/allocating the messages themselves.
Tests
src/ast/lib.rsunit tests: differential tests drivingLineColumnTrackeragainst a from-scratchinit_error_positionover CRLF / lone\r/ U+2028/U+2029 / multi-byte UTF-8 / invalid UTF-8 / single-giant-line corpora with forward, repeated, backwards, and strided offsets, plus a tracker-reuse-across-sources test.test/js/bun/jsonc/jsonc.test.ts: new pathological-input test spawns a child with a kill switch (same pattern as the Bun.markdown bracket-flood test) that parses the 250 KB malformed flood (expects the usualAggregateError) and the 240 KB duplicate-key flood (expects the correct value). Fails without the fix (child killed), passes with it.