Bound aggregate code review diff materialization (APP-5462)

[warp-agent-staging]

Description

This is the aggregate-materialization facet of APP-5462: repeated Sentry heap-profile events dominated by get_file_diff / parse_diff_hunks allocation, at 7.9–18.9 GB across several builds, that a per-file capture bound alone doesn't explain (a single under-cap file tops out around ~12MB typical / ~105MB pathological).

(a) vs (b): which one this is, and the evidence

The brief asked me to settle whether this is (a) breadth within one load (many under-cap files materialized in a single diff_state_against_head/diff_state_against_specific_branch call, with no aggregate cap) or (b) accumulation across repeated loads (stale state from earlier loads never released). It's (a); I specifically checked for (b) and found no evidence of it:

• LocalDiffStateModel's InternalDiffState::Loaded(Diffs) is replaced wholesale on every new load (self.state = InternalDiffState::Loaded(...)), which drops the previous GitDiffData and, with it, the last owning references to its Arc<Vec<DiffHunk>> hunks.
• The emitted NewDiffsComputed event wraps the load's GitDiffWithBaseContent in a fresh Arc (moved out of the same DiffsWithBaseContent, not an extra clone of the model's retained copy); subscribers borrow it synchronously and don't retain the Arc, so it drops once dispatch finishes.
• code_review_view.rs's invalidate_all (full reload) replaces CodeReviewViewState::Loaded(LoadedState { file_states, .. }) wholesale, and its single-file-invalidation path (update_from_single_file_diff_result) updates matching entries in place (by index) rather than appending — no growth from repeated per-file updates.
• GlobalBufferModel (the shared editor-buffer registry) tracks buffers via WeakModelHandle, and remove_deallocated_buffers (called after every diff update) evicts any buffer whose weak handle no longer upgrades — it doesn't accumulate indefinitely as editors come and go.
• The per-file invalidation SyncQueue uses a bounded 1024-slot broadcast channel, and cancel_all() clears its task map on every full reload — it doesn't grow unbounded either.
• I ran all 7 code-review integration tests under Xvfb (see Testing below), several of which perform an initial load + a mutation-triggered reload of the same model instance — the closest thing to a live repeated-load stress test — with no leak-shaped failures.

No code path retains diff data past the load that produced it. Given that, and that nothing previously bounded the number of files or total bytes materialized in a single load, (a) fully explains the observed multi-GB spikes: a repo with enough changed files — each individually under MAX_DIFF_SIZE — can already blow past several GB in one diff_state_against_head call (e.g. a large rebase, a repo-wide reformat, or diffing against a long-diverged branch).

The fix

Adds an aggregate materialization budget, adopting the shape of the dormant #13394 (MAX_TOTAL_DIFF_FILES / MAX_TOTAL_DIFF_BYTES) but superseding its footprint estimate, which was the reason it could still admit multi-GB despite a nominal 256MB budget:

• approx_file_diff_bytes(hunks, content_at_head) counts each DiffLine's own struct size (size_of::<DiffLine>(), not a hardcoded guess — stays correct if the struct changes) in addition to its text length (floored at 8 bytes per line, a typical minimum allocator size class). fix: bound aggregate code review diff memory across all files (APP-4827) #13394's version summed only line.text.len(), undercounting real retained memory by roughly the fixed per-line overhead — for the dense, short-line diffs that are typical, that's close to two orders of magnitude, which is how a "256MB" budget could in practice admit >10GB.
• Honest framing, not oversold: the estimate is documented as a lower bound, not a full accounting — it still omits each line's own String heap allocation and allocator size-class rounding beyond the 8-byte floor, Vec capacity slack, and DiffHunk's own header fields. Those add a roughly constant-factor fudge on top (an adversarial review recomputed this at ~1.1x for the pathological short-line case), not the ~80x the original text-only version was off by.
• materialize_with_aggregate_cap: a shared helper (used by both diff_state_against_head and diff_state_against_specific_branch) that fetches and parses each file's diff in order, but stops calling the expensive per-file fetch (the actual git subprocess calls + hunk parsing) as soon as either MAX_TOTAL_DIFF_FILES (2,000) or MAX_TOTAL_DIFF_BYTES (256MB) is reached.
• MAX_TOTAL_DIFF_FILES = 2_000 (unchanged from fix: bound aggregate code review diff memory across all files (APP-4827) #13394 — a secondary safety net against pathological file counts independent of byte size, e.g. a mass-rename).
• MAX_TOTAL_DIFF_BYTES = 256MB — re-derived, not blindly reused: it's the same nominal number as fix: bound aggregate code review diff memory across all files (APP-4827) #13394, but now means something real (a documented lower bound, not an ~80x-off proxy). 256MB is generous for legitimate large diffs (hundreds to thousands of typically-sized files, or ~20 MAX_DIFF_SIZE-sized files) while still bounding the worst case to a small, constant multiple of what a single load should reasonably need.

What the user sees at the limit

Files beyond the budget get the same presentation a single oversized file already gets: is_binary: false, empty hunks, size: DiffSize::Unrenderable(UnrenderableReason::DiffTooLarge) → "Diff is too large to render". They are not silently dropped from the list — every changed file still appears, just with this per-file placeholder instead of a rendered diff once the aggregate budget is exhausted. Their additions/deletions still come from git diff --numstat (already fetched for binary detection) when it has an entry, or a cheap on-disk line count for untracked files (which --numstat never reports — this was a review finding, see below), so the header's total-additions/total-deletions summary stays accurate.

The requester will be asked whether budget-skipped files deserve their own UnrenderableReason distinct from a single oversized file's "Diff is too large to render" (they can currently show that copy even when the file itself is tiny and was only skipped because earlier files in the list exhausted the budget). I'm holding that copy change pending an answer; it's a small follow-up on this branch if the answer changes it.

Known gaps (please read before merging)

1. One-file overshoot is a soft ceiling, not a hard one. The budget is checked before fetching a file and materialized_bytes is only updated after one is fully materialized, so a file already in flight when the check runs can still land. MAX_TOTAL_DIFF_BYTES can therefore overshoot by up to one file's own footprint — bounded by MAX_DIFF_SIZE-derived worst cases (~105MB pathological), not by the 256MB budget itself — and, until APP-5464 bounds base content too, by that same file's uncapped git show content on top. Documented on materialize_with_aggregate_cap's doc comment.
2. Per-file invalidation bypasses the aggregate cap entirely — a real gap, deliberately left open. retrieve_diff_state → file_diff_for_path (the path a single file's filesystem-change invalidation takes) does not go through materialize_with_aggregate_cap at all. After a capped full load, invalidating a placeholder file — or any newly-changed path — fully materializes it with no aggregate budget check, so a burst of filesystem invalidations can walk the aggregate back up past the cap over a session. This is a second door into the same room the full-load cap closes, not the cross-load accumulation (b) I ruled out above (that would be stale data never released; this is fresh per-file data the cap simply never sees).
   I did not attempt a contained fix here because I don't think one exists: FileInvalidationTask (in file_invalidation_queue.rs) carries only (file, repo_path, mode, merge_base) and runs on a generic SyncQueue background executor with no reference to the model's state, and the model itself currently discards its aggregate totals once a full load returns — nothing persists between calls for a per-file check to consult. A correct fix needs new persistent state: a per-path byte-contribution map on the model, reset at each full load, consulted (and updated) by both the full load and every per-file invalidation, with product decisions about what happens when a user's own active edit would push over budget (materialize anyway and evict something else? show the placeholder for the file they're actively editing?). That's a redesign, not a check moved earlier, so I'm flagging it here as a known, deliberate gap rather than attempting something fragile under this PR's scope. Happy to pick up a follow-up once its shape (including the copy question above) is decided.

Relationship to other work

• Supersedes fix: bound aggregate code review diff memory across all files (APP-4827) #13394's approach for the footprint estimate and budget derivation (same nominal constants, corrected accounting, honestly documented as a lower bound). fix: bound aggregate code review diff memory across all files (APP-4827) #13394 is a dormant, unreviewed, now-conflicting draft; I did not push to its branch — this is a fresh PR off current master.
• Independent of, and complementary to, #15225 (the per-subprocess stdout/stderr capture bound). That PR bounds a single file's subprocess capture; this PR bounds the aggregate across all files in one load. Both are needed: Bound git diff subprocess stdout capture (APP-5462) #15225 stops one huge file from itself blowing up; this PR stops many small-under-cap files from blowing up in aggregate.
• Landing order: Bound git diff subprocess stdout capture (APP-5462) #15225 is out of draft, green on full CI, and awaiting human review — it should land first. Bound aggregate code review diff materialization (APP-5462) #15307 and Bound git diff subprocess stdout capture (APP-5462) #15225 conflict textually in diff_size_limits.rs and in local.rs's imports (both add constants/helpers in the same spots), but are semantically independent; I'll rebase this PR onto Bound git diff subprocess stdout capture (APP-5462) #15225 once it merges. No redesign expected from that rebase — both sets of constants/helpers coexist, get_file_diff keeps using the capped runner from Bound git diff subprocess stdout capture (APP-5462) #15225, and materialize_with_aggregate_cap stays as-is.
• Learned from Bound git diff subprocess stdout capture (APP-5462) #15225's CI SIGABRT (a stack-overflow from a large stack buffer embedded in a deeply-nested future) that a real UI integration-test run can catch what unit tests miss. Before finalizing this PR, I ran the actual code-review integration test suite under Xvfb (not just unit tests) — see Testing.

Linked Issue

APP-5462 (Linear) — filed by the Sentry memory-triage bot; no corresponding GitHub issue.

Testing

• app/src/code_review/diff_size_limits_tests.rs: approx_bytes_counts_line_struct_overhead_not_just_text and approx_bytes_scales_with_line_count_for_short_lines guard against the ~80x-undercount regression this PR fixes; approx_bytes_floors_empty_line_text_at_minimum_allocation covers the 8-byte floor added in this revision; plus baseline coverage (approx_bytes_empty_diff_no_content_is_zero, approx_bytes_counts_only_content_when_no_hunks, approx_bytes_sums_hunk_line_text_and_content).
• app/src/code_review/diff_state/local_tests.rs:
  • materialize_with_aggregate_cap_stops_fetching_once_file_cap_is_reached / ..._byte_cap_is_reached: prove the expensive per-file fetch callback is not invoked at all for files beyond the cap (counting actual calls), not just that the returned shape looks truncated — the same rigor as the read_capped probe test from the Bound git diff subprocess stdout capture (APP-5462) #15225 revision.
  • materialize_with_aggregate_cap_uses_numstat_for_skipped_files_totals: proves skipped files' additions/deletions come from numstat when available.
  • materialize_with_aggregate_cap_counts_untracked_skipped_file_lines_from_disk (new this revision): proves a skipped untracked file's line count comes from an on-disk count when numstat has no entry for it — the review finding that the numstat-only path silently zeroed these out, which is exactly the tail of the APP-4827 node_modules shape that motivated the cap.
  • unrenderable_file_diff_matches_the_per_file_over_cap_presentation: proves the placeholder shape matches the existing per-file DiffTooLarge presentation exactly.
• cargo test -p warp --features local_fs,local_tty --lib code_review: 143 passed.
• cargo fmt -p warp -- --check and cargo clippy -p warp --features local_fs,local_tty --lib --tests -- -D warnings: clean.
• Real UI integration tests under Xvfb (WARPUI_USE_REAL_DISPLAY_IN_INTEGRATION_TESTS=1 xvfb-run -a cargo nextest run -p integration -E "test(/test_code_review/)" --run-ignored all): all 7 code-review integration tests pass, including the initial-load-plus-reload scenarios that exercise the same model instance across multiple loads. Re-ran after this revision's changes.

Out of scope

• get_file_content_at_head's unbounded git show HEAD:<path> capture is the same class of exposure but tracked separately as APP-5464; not touched here.
• The per-subprocess stdout/stderr capture bound is Bound git diff subprocess stdout capture (APP-5462) #15225, not duplicated here.
• The per-file-invalidation aggregate-cap gap described above under "Known gaps."

Plans:

• Conversation
• Run

Co-Authored-By: Warp agent@warp.dev

[warp-agent-staging]

Overview

Bounds the aggregate diff materialization that dominates occurrences 2-4 on APP-5462, and does so soundly — the per-line footprint estimate closes the ~80x undercount that made #13394's budget ineffective. Two decisions in it are product calls rather than engineering ones, so they are yours; the unambiguous findings are already going back to the author.

Concerns

• Budget-skipped files are labeled "Diff is too large to render." A file skipped because its siblings exhausted the budget may itself be tiny, so the message points the user at the wrong cause. The options are a distinct UnrenderableReason (e.g. "Diff hidden to limit memory") or a conscious decision to reuse DiffTooLarge and say so in the body. Nothing is silently omitted either way — skipped files stay visible in the list.
• The 256 MB budget covers hunk and base-content structures, not the whole code-review footprint. Eager per-file editor buffers for every materialized file are outside it, as is the uncapped git show HEAD:<path> base read tracked by APP-5464. Real DiffLine heap under a 256 MB accounted budget lands around 0.32 GB, plus one full under-MAX_DIFF_SIZE file of overshoot by design. Worth confirming that ceiling is the one you want, and whether editor creation should be gated too.
• Landing order. This conflicts textually with #15225 in diff_size_limits.rs and local.rs imports while being semantically complementary. #15225 is green and merge-ready, so it should land first and this rebases onto it.

Verdict

Checks: build unverified (reviewer's sandbox OOM-killed rustc; author reports 141 unit tests plus 7 Xvfb integration tests passing), tests unverified independently, CI n/a while draft, visual proof n/a for a memory change.

Found: 0 critical, 3 important, 3 suggestions, 1 nit. The important ones are with the author; the two decisions above need you.

Responding as wilson: Open session · View factory task