Bound auto-reload's background diff to a max content size

[warp-agent-staging]

Description

GlobalBufferModel::start_background_diff_parse (auto-reload path, behind FeatureFlag::IncrementalAutoReload, which is compiled into the default feature set for the app/warp crate — i.e. it is already on for stable users) unconditionally cloned the buffer's full current text (buffer.as_ref(ctx).text().into_string()) and the full new file content (new_content.to_string()) into owned Strings on every auto-reload event, with no size guard, before handing both to imara_diff's text_diff. For a large file this puts several multiples of the file's size live on the heap for a single auto-reload: one such clone alone was 7.52 GB (58%) of a 12.92 GB sampled heap in APP-5529 / Sentry.

Design choice: size bound over handle handoff. Buffer's internal representation is a rope of styled blocks, not a plain string, so .text() always has to materialize a fresh String for the diff — there's no existing cheaply-clonable text to hand off for the old side. The new content clone could in principle be avoided by changing FileModelEvent to carry an Arc<str>/owned String instead of a borrowed &str through populate_buffer_with_read_content, but that touches the ownership model across three call sites (FileLoaded, FileUpdated, discard_unsaved_changes) for a change whose worst case is already eliminated by the size bound below — out of scope for this fix per the linked ticket.

This bounds the diff path by content size: when either the buffer's current content or the new file content is above MAX_INCREMENTAL_DIFF_CONTENT_BYTES (32 MiB), populate_buffer_with_read_content skips start_background_diff_parse entirely and takes a synchronous replace_all fallback (apply_synchronous_reload) — the same one already used when IncrementalAutoReload is disabled. This also skips the diff's own imara_diff::Interner/InternedInput allocation (~0.72 GB in the same heap profile), since text_diff is simply never called for oversized content.

Revision (post-review): an adversarial review of the first version of this fix found the naive fallback silently dropped two behaviors the background-diff path provided, so apply_synchronous_reload now also:

• Preserves ServerLocal broadcast semantics. ServerModel only reacts to a failed BufferUpdatedFromFileEvent (to forward a conflict); a successful one is not handled at all. The generic fallback used to emit only that event, so a ServerLocal buffer's connected clients would never learn oversized content had changed. The fallback now mirrors apply_diff_result's ServerLocal branch: it bumps the sync clock's server_version and emits a full-replacement ServerLocalBufferUpdated (built the same way force_reload_server_local already builds one), instead of BufferUpdatedFromFileEvent.
• Aborts a stale pending diff before replacing. An under-limit update can leave pending_diff_parse set; a subsequent over-limit update used to leave it untouched, so the stale background task would keep its owned text/interner alive, then later fail its base-version check against content it no longer matches and emit a spurious BufferUpdatedFromFileEvent { success: false } (forwarded to ServerLocal clients as a conflict). apply_synchronous_reload now takes and aborts state.pending_diff_parse first, the same way start_background_diff_parse already does when a newer diff supersedes an older one.
• Also fixed a doc/code off-by-one: the threshold's doc comment said "at or above" but the check is strictly >; reworded to "above" to match.

The base_version staleness check and the ServerLocal byte→char conversion in apply_diff_result are otherwise untouched — they only run once a diff is actually produced.

32 MiB is chosen well above the size of source/config files the incremental path exists to serve (preserving undo history and anchors across an on-disk change), so only files far outside that use case fall back to a full replace. (The threshold value itself, as a UI-latency cutoff for running replace_all synchronously on the model/UI context, is a separate judgment call for the requester — not changed here.)

Fixes APP-5529. Distinct facets of the same Sentry issue (Buffer::replace_all, StyledBufferRun/invalidate_layout, etc.) are tracked separately in APP-5357/APP-5353/APP-4844/APP-5388/APP-5429/APP-5462/APP-5458/APP-4840/APP-5524/APP-5464 and are intentionally not touched here.

Linked Issue

• Filed by the Sentry memory-triage bot as APP-5529; no separate GitHub issue.

Testing

• cargo check -p warp --lib --features test-util and --features test-util,local_fs: both pass.
• cargo clippy -p warp --lib --features test-util --all-targets -- -D warnings: passes, no warnings.
• ./script/format: clean (no diffs after running).
• cargo test -p warp --lib --features test-util global_buffer_model_tests: all 8 tests pass, including four new regression tests:
  • auto_reload_under_size_limit_uses_background_diff: content under the limit still spawns the background diff (existing behavior preserved).
  • auto_reload_over_size_limit_falls_back_to_synchronous_replace: content over the limit takes the synchronous fallback instead, with no background diff spawned.
  • auto_reload_over_size_limit_broadcasts_for_server_local_buffer: a ServerLocal buffer's sync-clock server_version still bumps for over-limit content (regression test for the broadcast gap above).
  • auto_reload_over_size_limit_aborts_stale_pending_diff: an under-limit update followed by an over-limit one leaves no stale pending diff (regression test for the abort gap above).
  • The size threshold is a #[cfg(test)]-shrunk constant (4096 bytes) so these tests exercise the real boundary logic without materializing a 32 MiB buffer; the test/prod split technique mirrors the existing TEXT_FRAGMENT_SIZE split, though that precedent only justifies the technique, not the 32 MiB production value.
• Not independently verified: end-to-end behavior of a real multi-GB file under a running Warp client, or a real ServerLocal/remote-server round trip — no toolchain for that scale of manual reproduction in this environment. The size-guard and fallback logic are covered by the tests above.
• No UI change, so no visual proof is included.

CHANGELOG-BUG-FIX: Fixed a memory blowup on auto-reload of very large files with unsaved-change preservation enabled.

[warp-agent-staging]

This PR was generated with Warp.

Comment @warp-factory on this PR to send it follow-up work.

 

[warp-agent-staging]

Overview

Bounds auto-reload's background diff by content size so the unbounded full-content clones can no longer reach multi-GB, falling back to the existing synchronous replace_all above the limit. The memory fix itself is sound; one design question needs an owner's decision before this merges.

Concerns

• The 32 MiB cutoff is justified as a content-size bound but not as a latency bound: content just over it now runs replace_all synchronously on the model/UI context, trading the memory spike for a possible UI stall on exactly the largest files. That trade is probably right, but nobody has measured what the synchronous path costs at that size, and the TEXT_FRAGMENT_SIZE precedent cited in the doc comment only supports the test/prod split, not the production number. Either take a representative measurement at the boundary, or record an explicit decision that the stall is acceptable for auto-reload.

Verdict

Checks: build pass, tests pass, CI green, visual proof n/a

Found: 0 critical, 0 important, 0 suggestions, 0 nits, 1 question

Responding as wilson: Open session · View factory task

[warp-agent-staging]

What does the synchronous fallback cost at this boundary? The doc above argues 32 MiB sits well above the source/config files the incremental path serves, which justifies it as a memory bound. The open question is the other side: a 32 MiB replace_all on the model/UI context may stall the UI where the old path only spiked memory. A measurement at the boundary, or an explicit "acceptable for auto-reload" call, would settle it.