Skip to content

fix: bound aggregate code review diff memory across all files (APP-4827) - #13394

Draft
warp-agent-staging[bot] wants to merge 1 commit into
masterfrom
oz/bound-code-review-diff-aggregate-memory-app-4827
Draft

fix: bound aggregate code review diff memory across all files (APP-4827)#13394
warp-agent-staging[bot] wants to merge 1 commit into
masterfrom
oz/bound-code-review-diff-aggregate-memory-app-4827

Conversation

@warp-agent-staging

Copy link
Copy Markdown
Contributor

Summary

Bounds the aggregate memory the local code-review diff-state materializes when loading diffs for a repo. Addresses the dominant facet of the memory spike tracked in APP-4827 (Sentry 7259255054), complementing #13393 (which bounds a single file).

Root cause (from a symbolized heap profile)

The latest macOS event's heap profile sampled ~10.9 GB (internal footprint ~11.6 GB). I symbolized it against the release dSYM. The dominant allocation stacks are all in the code-review diff viewer:

  • ~2.6 GBGlobalBufferModel::populate_buffer_with_read_contentsBuffer::replace_allBufferSumTree::append_str (loading full file contents into editor buffers)
  • ~2.4 GBCodeEditorModel::rebuild_layout_and_refresh_diffStyledBufferBlocks build → ActiveTextBlock::push_str (styled/syntax layout per diff)
  • ~0.9 GBLocalDiffStateModel::load_diffs_for_current_repoget_file_diffparse_diff_hunks
  • ~0.2 GBCodeReviewView::build_view_state_for_file_diffs (a CodeEditorView + CommentEditor per file)

diff_state_against_head / diff_state_against_specific_branch enumerate all changed + untracked files (git status --untracked-files=all) and, for each, fully parse the diff and retain the base file content in an in-memory Vec<FileDiffAndContent>; the view then builds a CodeEditorModel + buffer per file. For a repo with a large untracked tree (e.g. a non-gitignored node_modules), this materializes tens of thousands of files at once — driving the multi-GB spike.

Fix

Add two aggregate caps in diff_size_limits.rs and enforce them in both enumeration loops:

  • MAX_TOTAL_DIFF_FILES = 2000 — max files whose full diff + content we materialize
  • MAX_TOTAL_DIFF_BYTES = 256 MB — max cumulative (parsed hunk text + base content) footprint

Once either cap is exceeded the loop stops materializing further files. The cheap aggregate metadata path (diff_metadata_against_specific_branch) is untouched, so the file-count/stat badges still reflect the true totals — only the fully-rendered editor state is bounded.

This is the "cap total processed files / cumulative diff bytes" follow-up explicitly called out in APP-4827, and it targets the ~5 GB editor-materialization facet that the per-file guard in #13393 does not cover.

Testing

  • I have manually tested my changes locally with ./script/run

Added unit tests for the new approx_file_diff_bytes helper (diff_size_limits_tests.rs) and a compile-time invariant that the aggregate byte budget exceeds the per-file limit. cargo check, cargo clippy -p warp --tests (0 warnings), and the new tests all pass.

Agent Mode

  • Warp Agent Mode - This PR was created via Warp's AI Agent Mode

CHANGELOG-BUG-FIX: Fixed a multi-GB memory spike when opening code review in repositories with very large untracked file trees (e.g. a non-gitignored node_modules).


Conversation: https://staging.warp.dev/conversation/90209902-7540-40fd-9fb4-da4a5dbd6820
Run: https://oz.staging.warp.dev/runs/019f3469-5051-7c7b-b2c5-59e25419b3de
Plans:

This PR was generated with Oz.

The local code-review diff-state enumerates every changed + untracked
file (`git status --untracked-files=all`) and, for each, fully parses the
diff into `DiffHunk`/`DiffLine` structs and retains the base file content,
accumulating all of it into an in-memory `Vec<FileDiffAndContent>`. The
code review view then builds a `CodeEditorModel` + buffer per file.

For a repo with a large untracked tree (e.g. a non-gitignored
`node_modules`), this materializes tens of thousands of files at once.
A symbolized macOS heap profile (Sentry 7259255054, ~10.9 GB sampled)
showed the dominant cost was exactly this: ~2.6 GB in
`populate_buffer_with_read_contents` (loading file contents into editor
buffers), ~2.4 GB rebuilding styled buffer blocks, and ~0.9 GB parsing
diff hunks.

`MAX_DIFF_SIZE` (added in #13393) bounds any single file, but not the
aggregate. This adds `MAX_TOTAL_DIFF_FILES` (2000) and
`MAX_TOTAL_DIFF_BYTES` (256MB) caps to both diff enumeration loops
(`diff_state_against_head` and `diff_state_against_specific_branch`):
once either is exceeded we stop materializing further files. The cheap
aggregate metadata path still counts every file, so the file-count badge
stays accurate.

Co-Authored-By: Oz <oz-agent@warp.dev>
@warp-agent-staging

Copy link
Copy Markdown
Contributor Author

Heads-up from a live recurrence: Sentry event 021d3fb1803e4e0da9c42572a04febd7 (18.9 GB live, stable v0.2026.07.29.09.05) has ~4.73 GB in parse_diff_hunks DiffLine/DiffHunk allocations aggregated across many under-MAX_DIFF_SIZE files — the facet this PR bounds.

approx_file_diff_bytes counts only line.text.len(), but each retained line also costs a 64-byte DiffLine plus its own String allocation. For short-line content (minified/generated files, the workload in these events) that undercounts real heap by roughly 80x, so a 256 MB accounted budget can still admit >10 GB. Adding a per-line overhead term to the estimate would make MAX_TOTAL_DIFF_BYTES bound what it's named for.

Context: #15225 bounds the per-subprocess capture on the same path, so the two are complementary — under-budget files still parse and aggregate-retain after it lands.

Responding as wilson: Open session · View factory task

warp-agent-staging Bot added a commit that referenced this pull request Aug 19, 2026
Root-cause analysis for this facet of APP-5462 concluded (a) breadth
within a single load, not (b) accumulation across repeated loads: no
aggregate cap existed on the number of files or total bytes
materialized in one diff_state_against_head/diff_state_against_specific_branch
call, so a repo with enough changed files -- each individually under
MAX_DIFF_SIZE -- can retain gigabytes in a single load. (b) was
specifically checked and ruled out: InternalDiffState/Diffs/GitDiffData
are replaced wholesale (and dropped) on every new load, Arc-shared
hunks are correctly refcounted with no extra deep clones,
GlobalBufferModel evicts buffers via weak handles once no editor
references them, and the per-file invalidation SyncQueue is bounded
(1024-slot broadcast buffer, cleared task map on cancel_all) rather
than accumulating.

Adds an aggregate materialization budget (MAX_TOTAL_DIFF_FILES,
MAX_TOTAL_DIFF_BYTES) enforced by a shared materialize_with_aggregate_cap
helper used by both diff_state_against_head and
diff_state_against_specific_branch. The per-file footprint estimate
(approx_file_diff_bytes) counts each DiffLine's own struct size
(via size_of, not a hardcoded guess) in addition to its text, fixing
an ~80x undercount in an earlier attempt (#13394) that summed only
line text length and could in practice admit multi-GB despite a
nominal 256MB budget.

Files beyond the budget are marked Unrenderable(DiffTooLarge) --
the same presentation a single oversized file already gets -- rather
than silently omitted from the list. Their line-count contributions to
the aggregate stats still come from the numstat already fetched for
binary detection, so the header stats stay accurate.

Co-Authored-By: Warp <agent@warp.dev>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant