Repository navigation
fix(lsp): eliminate multi-GB memory blowup (50GB RSS reports) - #3938
Merged
Merged
Conversation
Manual repro tests for the 50GB LSP investigation: simulate didChange full-text edit churn and identifier-rename churn against a ProjectDatabase while printing RSS. Run with: cargo test -p baml_tests --test memory_leak_audit -- --ignored --nocapture Co-Authored-By: Claude Fable 5 <[email protected]>
…re discovery Workspace discovery walked the entire workspace with no pruning and ran the lenient project-root resolver on every entry, so under any path containing "baml_language" every fixture/stdlib .baml file became its own LSP project (own salsa DB + BexEngine + $init execution). Discovery also crawled target/, node_modules/, etc., stalling VS Code shutdown. - Discovery now only accepts real markers (baml.toml / baml_src) via get_marked_baml_project_root; the standalone-.baml internal-dev fallback remains for direct file operations (didOpen/didChange). - The crawl uses ignore::WalkBuilder on native (like ruff), so .gitignore prunes generated dirs organically, with a fixed-name backstop (hidden dirs, node_modules, target, dist) for non-git workspaces; WASM uses a read_dir recursion with the same backstop. - Markers are matched on directories only, instead of running an ancestor resolution per walked file. Co-Authored-By: Claude Fable 5 <[email protected]>
The in-memory run store grew without bound: profile_events was append-only and fully cloned on EVERY ingested event, with every rooted run's call tree rebuilt from scratch per event; terminal runs (holding full LLM payloads) were never evicted; the history store kept per-run bookkeeping (open writer fd, claimed-record set) forever. With profiling default-on, a long LSP session leaked multi-GB. - ingest_profile_event/attach_root_trace no longer clone the event log (split borrows) and only recompute runs rooted in the event's (process_euid, engine_id) — an event can never affect other engines. - Engine closure now fans out to observers: the run store drops the closed engine's events after a final recompute, and the history store releases boundary state (falling back to disk scans for open/read); the history router also drops the engine's buffered records. - RunRetentionPolicy.max_terminal_runs / terminal_ttl_ms are enforced on completion; the native LSP caps terminal runs at 100. Evicted runs rehydrate from disk history on Snapshot (same fallback OpenHistory already had). WASM keeps unlimited retention (no disk history there). - profile_events gets a 100k FIFO backstop, matching the history router's bound; the history router's component lookup no longer clones every buffered envelope per query. Co-Authored-By: Claude Fable 5 <[email protected]>
Every successful compile eagerly built a fully-inlined control-flow graph for EVERY function and retained 8 generations of those snapshots. The inliner copies the callee graph at every call site (only cycles are guarded), so snapshot size grows as fan_out^depth: measured ~25MB per generation on a depth-10/fan-3 chain and ~220MB at depth 12 — tens of GB steady-state on workflow-heavy projects, rebuilt per keystroke. The only consumer is the run graph-overlay resolver, which needs ONE function's spans per run, pinned to the run's launch generation. - control_flow_graph_for_generation now builds that one graph on demand (while its generation is still current), cached in a small per-project LRU keyed by (generation, function). Run launches pre-warm it, so overlay spans stay resolvable after later recompiles. - capture_cfg_snapshot / RetainedCfgSnapshots are gone; update_bex does no CFG work at all. - ast_control_flow_graph memoizes callee expansion per top-level build (build cost was also exponential) and caps inlined output at 5k nodes; calls beyond the budget render as plain call nodes. Co-Authored-By: Claude Fable 5 <[email protected]>
…e didChange Every didChange synchronously re-applied EVERY open buffer to salsa (dirtying all their query chains), ran full bytecode generation, built a fresh BexEngine (executing $init), and spawned test collection — all on the stdin thread, per keystroke, with the results discarded by the next keystroke. - didChange now re-applies only the document that changed (the playground editor path likewise); a refresh mode without a named file keeps re-applying all open buffers. - Diagnostics still compute and publish synchronously per change, but the heavy tail (bytecode + BexEngine + test collection) runs on a background task after a 300ms quiet window, coalesced per project via an epoch counter and off the async workers via spawn_blocking. Once the engine is current the project update is re-announced and tests collected, mirroring the old synchronous tail. WASM keeps the synchronous rebuild (no timer wired there). Co-Authored-By: Claude Fable 5 <[email protected]>
Salsa never frees inputs, so every delete/recreate cycle of a path (branch switches, codegen rewriting .baml files, editors that replace on save) minted a fresh immortal SourceFile — each retaining its full text plus the last memo of every per-file query (~30-100x source size). remove_file now empties the input's text (releasing the string and letting downstream memos collapse) and parks the handle in a tombstone map; add_or_update_file revives it for the same path instead of creating a new input. Input growth is now bounded by distinct paths ever seen, not by churn count. Also adds a delete/recreate churn scenario to the manual memory-audit harness. Co-Authored-By: Claude Fable 5 <[email protected]>
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
⏭️ Performance benchmarks were skippedPerf benchmarks (CodSpeed) are opt-in on pull requests — they no longer run on every push. They always run automatically after merge to To run them on this PR, do any of the following, then push a commit (or re-run CI):
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe PR adds terminal-run retention and history rehydration, engine-closed lifecycle notifications, CFG expansion limits and tombstoned file revival, Arc-backed run-scoped CFG caching with refresh/rebuild changes, strict project discovery, and an ignored memory audit test. ChangesCore behavior changes
Estimated code review effort: 4 (Complex) | ~75 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Binary size checks passed✅ 7 passed
Generated by |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
baml_language/crates/bex_events/src/history/mod.rs (1)
442-490: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease no-root boundaries on completion
engine_closed()only matchesroot_trace: Some(..), so runs that finish beforebroadcast_root_trace_attachment()never get marked for cleanup. ThoseBoundaryStates keep theirBoundaryWriterand open file descriptors alive until process exit. Remove them incomplete()once the record is flushed, even whenroot_traceisNone.🤖 Prompt for 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. In `@baml_language/crates/bex_events/src/history/mod.rs` around lines 442 - 490, The cleanup logic in complete() only drops BoundaryState after engine_closed is set, but no-root runs can complete before broadcast_root_trace_attachment() and never get marked for removal. Update the complete() flow in RunHistory so that once the RunCompletedRecord is written and flushed, the boundary is removed from inner.boundaries even when state.root_trace is None, while still preserving the existing durability and profile_write_error checks.
🧹 Nitpick comments (2)
baml_language/crates/baml_tests/tests/memory_leak_audit.rs (1)
57-182: 📐 Maintainability & Code Quality | 🔵 TrivialConsider unit tests per coding guidelines; remember to run
cargo test --lib.Per coding guidelines, prefer Rust unit tests over integration tests where possible. These memory audit tests exercise
baml_project::ProjectDatabase's public API and could alternatively live as#[cfg(test)]unit tests insidebaml_project. That said, as manual#[ignore]'d audit harnesses that needlibcand process-level RSS measurement, integration tests are a reasonable choice — just flagging the guideline.Also, per coding guidelines, run
cargo test --libif you changed any Rust code.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible" and "Always run
cargo test --libif you changed any Rust code".🤖 Prompt for 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. In `@baml_language/crates/baml_tests/tests/memory_leak_audit.rs` around lines 57 - 182, The three memory audit cases are currently implemented as an integration test harness even though they primarily exercise ProjectDatabase behavior; move these checks into a #[cfg(test)] unit test module inside the baml_project crate if possible, so they live closer to the public API they validate. Keep the #[ignore] manual audit behavior and the RSS/libc helpers if needed, but prefer unit-test placement for editing_session_memory_growth, delete_recreate_churn, and editing_session_rename_churn. After moving or updating the Rust test code, run cargo test --lib as required by the coding guidelines.Source: Coding guidelines
baml_language/crates/baml_lsp_server/src/playground_server.rs (1)
1973-1980: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate history-rehydration logic between
OpenHistoryandSnapshot.Both branches implement the identical "in-memory miss →
history_store.open()→insert_replayed_run()→ use cloned snapshot" fallback. Consider extracting a small helper (e.g.fn snapshot_or_rehydrate(state: &WsState, boundary_id) -> io::Result<Run>) shared by both call sites.♻️ Proposed helper extraction
+fn snapshot_or_rehydrate(state: &WsState, boundary_id: BoundaryId) -> io::Result<Run> { + match state.run_store.snapshot(boundary_id) { + Some(snapshot) => Ok(snapshot), + None => state.history_store.open(boundary_id).map(|run| { + let snapshot = run.clone(); + let _ = state.run_store.insert_replayed_run(run); + snapshot + }), + } +}Then both
OpenHistoryandSnapshothandlers callsnapshot_or_rehydrate(state, boundary_id).Also applies to: 2018-2028
🤖 Prompt for 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. In `@baml_language/crates/baml_lsp_server/src/playground_server.rs` around lines 1973 - 1980, The `OpenHistory` and `Snapshot` paths in `playground_server.rs` duplicate the same cache-miss rehydration flow. Extract the shared “run_store miss → history_store.open() → insert_replayed_run() → cloned snapshot” logic into a small helper like `snapshot_or_rehydrate` on `WsState` (or nearby), and update both handlers to call it so the fallback behavior lives in one place.
🤖 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 `@baml_language/crates/baml_project/src/db.rs`:
- Around line 616-635: The memoization in ast_control_flow_graph_impl is reusing
a callee graph across different expansion contexts, which can cause later call
sites to miss inlining when the graph was first built under recursion. Update
the caching logic around ctx.cache in db.rs so cached entries are either scoped
by the active expansion context (for example, include the current expansion
chain in the key) or skip caching graphs built while ctx.expanding is active;
keep the recursion guard with ctx.expanding and preserve correct reuse only for
context-safe builds.
In `@baml_language/crates/baml_tests/tests/memory_leak_audit.rs`:
- Around line 9-24: The rss_mb helper assumes libc::getrusage always succeeds,
but it calls usage.assume_init() unconditionally in the unsafe block. Update
rss_mb to check the return value from getrusage before assuming initialization,
and only read ru_maxrss when the call succeeds; keep the change localized to the
rss_mb function so the unsafe usage is provably sound.
---
Outside diff comments:
In `@baml_language/crates/bex_events/src/history/mod.rs`:
- Around line 442-490: The cleanup logic in complete() only drops BoundaryState
after engine_closed is set, but no-root runs can complete before
broadcast_root_trace_attachment() and never get marked for removal. Update the
complete() flow in RunHistory so that once the RunCompletedRecord is written and
flushed, the boundary is removed from inner.boundaries even when
state.root_trace is None, while still preserving the existing durability and
profile_write_error checks.
---
Nitpick comments:
In `@baml_language/crates/baml_lsp_server/src/playground_server.rs`:
- Around line 1973-1980: The `OpenHistory` and `Snapshot` paths in
`playground_server.rs` duplicate the same cache-miss rehydration flow. Extract
the shared “run_store miss → history_store.open() → insert_replayed_run() →
cloned snapshot” logic into a small helper like `snapshot_or_rehydrate` on
`WsState` (or nearby), and update both handlers to call it so the fallback
behavior lives in one place.
In `@baml_language/crates/baml_tests/tests/memory_leak_audit.rs`:
- Around line 57-182: The three memory audit cases are currently implemented as
an integration test harness even though they primarily exercise ProjectDatabase
behavior; move these checks into a #[cfg(test)] unit test module inside the
baml_project crate if possible, so they live closer to the public API they
validate. Keep the #[ignore] manual audit behavior and the RSS/libc helpers if
needed, but prefer unit-test placement for editing_session_memory_growth,
delete_recreate_churn, and editing_session_rename_churn. After moving or
updating the Rust test code, run cargo test --lib as required by the coding
guidelines.
🪄 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: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 9b61b394-47bc-4f1b-8642-a5a56f0a7e75
⛔ Files ignored due to path filters (1)
baml_language/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (16)
baml_language/Cargo.tomlbaml_language/crates/baml_lsp_server/src/lib.rsbaml_language/crates/baml_lsp_server/src/playground_runs.rsbaml_language/crates/baml_lsp_server/src/playground_server.rsbaml_language/crates/baml_project/src/db.rsbaml_language/crates/baml_tests/Cargo.tomlbaml_language/crates/baml_tests/tests/memory_leak_audit.rsbaml_language/crates/bex_events/src/history/mod.rsbaml_language/crates/bex_events/src/history/router.rsbaml_language/crates/bex_events/src/prof/consumer.rsbaml_language/crates/bex_events/src/run.rsbaml_language/crates/bex_project/Cargo.tomlbaml_language/crates/bex_project/src/bex_lsp/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/notification.rsbaml_language/crates/bex_project/src/project.rs
aaronvg
enabled auto-merge
July 8, 2026 00:43
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
baml_language/crates/bex_project/src/fs.rs (1)
160-163: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the Windows path normalization.
The fix correctly replaces backslashes with forward slashes before the VFS
join, but there's no test covering this behavior. Per coding guidelines, prefer Rust unit tests for changed Rust code. A test verifying thatget_path_from_pathproduces a valid VFS path with forward slashes on Windows would prevent regressions.Also, remember to run
cargo test --libafter this change, as required by the coding guidelines.As per coding guidelines: "Prefer writing Rust unit tests over integration tests where possible" and "Always run
cargo test --libif you changed any Rust code".🧪 Suggested unit test
#[cfg(test)] mod tests { use super::*; #[test] fn test_get_path_from_path_normalizes_backslashes() { let fs = BamlVFS::new(std::sync::Arc::new(Box::new(vfs::PhysicalFS::new("/")))); // On Windows, a path with backslashes should be normalized to forward slashes let raw = std::path::PathBuf::from(if cfg!(windows) { "C:\\Users\\foo\\bar.baml" } else { "/Users/foo/bar.baml" }); let result = fs.get_path_from_path(&raw, "test").unwrap(); assert!(result.as_str().contains("foo/bar.baml")); assert!(!result.as_str().contains('\\')); } }🤖 Prompt for 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. In `@baml_language/crates/bex_project/src/fs.rs` around lines 160 - 163, The Windows path normalization change in get_path_from_path should be covered by a Rust unit test so regressions are caught. Add a test in the BamlVFS test module that exercises get_path_from_path with a Windows-style PathBuf containing backslashes and asserts the returned VFS path uses forward slashes and does not contain backslashes. Keep the test close to the existing path-joining logic so it clearly validates the #[cfg(windows)] normalization branch, and remember to run cargo test --lib after updating the Rust code.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@baml_language/crates/bex_project/src/fs.rs`:
- Around line 160-163: The Windows path normalization change in
get_path_from_path should be covered by a Rust unit test so regressions are
caught. Add a test in the BamlVFS test module that exercises get_path_from_path
with a Windows-style PathBuf containing backslashes and asserts the returned VFS
path uses forward slashes and does not contain backslashes. Keep the test close
to the existing path-joining logic so it clearly validates the #[cfg(windows)]
normalization branch, and remember to run cargo test --lib after updating the
Rust code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 05eaa357-b321-49d7-9852-b90f88fd7e78
📒 Files selected for processing (4)
baml_language/crates/baml_project/src/db.rsbaml_language/crates/baml_tests/tests/memory_leak_audit.rsbaml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rsbaml_language/crates/bex_project/src/fs.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- baml_language/crates/baml_tests/tests/memory_leak_audit.rs
- baml_language/crates/baml_project/src/db.rs
- baml_language/crates/bex_project/src/bex_lsp/multi_project/mod.rs
rossirpaulo
added a commit
that referenced
this pull request
Jul 8, 2026
Conflict resolution: canary #3938 replaced the eager per-compile CFG snapshot (capture_cfg_snapshot / RetainedCfgSnapshots) with a lazy per-run RunCfgCache, deleting the function our P1.4b fix had switched to a schema-free listing. Took canary's side — the lazy rewrite supersedes P1.4b entirely (no function listing happens on build at all) — and removed the now-caller-less list_playground_function_names helper from baml_project. Verified post-merge: cargo check/test on baml_project, bex_project, bridge_wasm; pnpm typecheck + tests in pkg-proto (after regenerating the renamed baml_bridge protos), pkg-playground, app-vscode-webview. Co-Authored-By: Claude Fable 5 <[email protected]>
This was referenced Jul 10, 2026
This was referenced Jul 30, 2026
This was referenced Aug 13, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
Users regularly saw
baml-cliLSP sessions reach 50GB RSS, even "passively" (no code running, no active typing). VS Code shutdown also hung on large workspaces.How we identified each cause, and what we changed
flowchart LR S["50GB RSS reports<br/>(grows even passively)"] S --> C1 S --> C2 S --> C3 S --> C4 S --> C5 subgraph Causes["Root causes (how found)"] C1["<b>1. Accidental projects</b><br/>discovery promoted every stray .baml<br/>under a baml_language path to a full<br/>project (salsa DB + engine + $init each)<br/><i>found: code audit + 180-file synthetic<br/>workspace peaked ~423MB before</i>"] C2["<b>2. Exponential CFG snapshots</b><br/>fully-inlined graph for EVERY function,<br/>EVERY compile, 8 generations retained;<br/>inliner copies callee per call site<br/><i>found: live LSP driven over stdio;<br/>depth-10 chain = 25MB/edit,<br/>depth-12 = 220MB/edit (9x = 3^2,<br/>exponential confirmed)</i>"] C3["<b>3. Unbounded run store, O(n^2) ingest</b><br/>profile_events append-only and cloned<br/>whole on EVERY event; terminal runs<br/>(with full LLM payloads) never evicted;<br/>profiling is default-on<br/><i>found: code audit of bex_events;<br/>every recompile's $init feeds it,<br/>hence passive growth</i>"] C4["<b>4. Per-keystroke engine churn</b><br/>every didChange re-applied ALL open<br/>buffers + full bytecode + new engine<br/>+ $init + test collection, synchronously<br/><i>found: CPU sampling (1:34 CPU per<br/>50s wall) + code trace</i>"] C5["<b>5. Immortal salsa inputs</b><br/>delete/recreate cycles (branch switch,<br/>codegen) minted a new input per cycle,<br/>each keeping text + all per-file memos<br/><i>found: salsa audit; core salsa itself<br/>converges (harness committed)</i>"] end C1 --> F1["<b>Fix (96e49fd0a)</b><br/>marker-only discovery (baml.toml /<br/>baml_src) + ruff-style gitignore-aware<br/>crawl via the ignore crate"] C2 --> F2["<b>Fix (152e5b438)</b><br/>snapshot store deleted; ONE graph built<br/>lazily per run at launch (64-entry LRU);<br/>expansion memoized + 5k node cap"] C3 --> F3["<b>Fix (e94c7a25c)</b><br/>recompute scoped to the event's engine,<br/>no clones; cap 100 terminal runs (native,<br/>disk rehydrate); engine-close purge;<br/>100k event backstop"] C4 --> F4["<b>Fix (079f612d3)</b><br/>didChange applies only the changed file;<br/>engine rebuild debounced 300ms onto a<br/>background task (diagnostics still sync)"] C5 --> F5["<b>Fix (4e77bce2e)</b><br/>removed files park as emptied tombstones<br/>and revive on re-add"]Salsa itself was ruled out empirically: editing churn plateaus (~80-160MB). The harness proving this is committed as ignored tests (
crates/baml_tests/tests/memory_leak_audit.rs, 959e344).Measured results (debug build, LSP driven over stdio with real edits)
.bamlfiles +target/junkEnd-user consequences: what changes, what is no longer possible
The fixes cap several things that were previously unbounded. Honest list of what a user can notice:
.bamlfiles sitting in the workspace (under abaml_languagepath).bamlfile as the workspace root also still worksnode_modules/targetdirectoryNot changed: diagnostics latency and correctness, formatting, hover/completion/definition, test running, run history on disk (nothing is deleted from disk, ever).
Validation
bex_events,bex_project,baml_lsp_server,baml_project(new tests for discovery filtering, retention/eviction, engine-scoped recompute, CFG cap + memoization equivalence, tombstone revival)bridge_wasmcompiles for wasm32; WASM keeps prior behavior (sync rebuild, no eviction)Repro/measurement scripts and the full investigation narrative are in the session notes; the committed
memory_leak_audit.rsignored tests cover the salsa-side regressions (cargo test -p baml_tests --test memory_leak_audit -- --ignored --nocapture).🤖 Generated with Claude Code
Summary by CodeRabbit
.gitignore-aware scanning.