Bound BufferingRepositorySubscriber's debounce buffer (APP-4829) - #15343
Bound BufferingRepositorySubscriber's debounce buffer (APP-4829)#15343warp-agent-staging[bot] wants to merge 4 commits into
Conversation
Under sustained filesystem churn (a large git checkout, npm install, etc.), BufferingRepositorySubscriber::on_files_updated coalesced every incoming RepositoryUpdate into BufferState.pending with no maximum size or age. As long as updates kept arriving, the debounce flusher never observed a quiet period, so the pending HashSets/HashMap grew without bound, driving multi-gigabyte heap spikes (Sentry issue 7259255054). Force an early flush of the pending buffer once its combined entry count (added + modified + deleted + moved) reaches 10,000, independent of the debounce timer. The forced flush drains the same coalesced buffer merge_repository_updates already maintains and hands it to the inner subscriber exactly like the debounced flush does, so updates are only ever split into more, smaller batches -- never dropped or reordered. The debounce flusher, once spawned, keeps draining whatever accumulates between forced flushes, so pending is never left non-empty without a flusher scheduled against it. Adds a test-only with_max_pending_entries constructor so tests can exercise the bound without buffering 10,000 real entries, plus regression tests proving a flush is forced before the debounce timer fires and that no updates are lost across repeated forced flushes.
|
This PR was generated with Warp. Comment |
Review found two correctness gaps in the previous revision: 1. The threshold was checked only after merging the whole incoming RepositoryUpdate, so a single large filesystem-watcher batch (e.g. from a huge git checkout) could still balloon `pending` past the configured bound before anything drained it. 2. Forced-flush batches were dispatched fire-and-forget, so a fast producer could submit a new batch before the previous one's async delivery finished. The production consumer (RepoOutlines) silently drops updates that arrive while a previous recomputation is still in flight, so overlapping/rapid forced flushes could lose updates. Fix 1: merge each incoming update into `pending` one entry at a time, in the same phase order `merge_repository_updates` already applies (moves, adds, modifies, deletes, then flags), checking the bound after every entry. This keeps `pending` bounded even when a single incoming update by itself exceeds the limit. Fix 2: route every flush (forced or debounced) through a delivery queue drained strictly one batch at a time, so `inner.on_files_updated` never has two batches in flight and always applies them in order. Fix 2 also reaches into RepoOutlines (the only production consumer): it now retains and applies updates that arrive while a recomputation is Pending instead of dropping them, closing the remaining loss scenario a slow recomputation could otherwise still hit downstream of the channel. Also: rewrote the losslessness test to actually let the debounce elapse and account for every update (it previously left a partial batch unverified), added a regression test proving a single oversized update is split into bounded batches, added a slow-consumer regression test proving serialized, in-order, lossless delivery across forced and debounced flushes, and removed a redundant doc comment.
There was a problem hiding this comment.
Overview
This bounds BufferState.pending under sustained churn by merging incoming updates entry-by-entry and dispatching batches through a serialized delivery queue, and stops RepoOutlines discarding updates that arrive mid-recomputation. Posting findings rather than a verdict: the change is blocked on a design decision that needs a human, because the memory bound it adds is undone by the two unbounded queues it introduces.
Concerns
- The unbounded growth relocates rather than disappears.
BufferState::delivery_queueandOutlineState::queued_updatesare unboundedVecDeque<RepositoryUpdate>s, and under the exact churn APP-4829 describes, batches are produced faster thanOutline::updatecan parse them, so a backlog of full batches accumulates wherependingused to. This is the decision point: the consumer is genuinely slower than the producer, so memory cannot be bounded while keeping per-path fidelity for an arbitrarily large change set — either the queued work is coalesced into a bounded representation, or the pipeline needs an explicit bounded dirty-state/rescan protocol, and that second option changes observable outline behavior enough that it should not be chosen without a human. - Two mechanical findings are gated on that decision and will be fixed once it is made, not left behind.
on_unsubscribeaborts only the debounce flusher, so a queued delivery keeps its memory alive and can still fire after unsubscribe, and a never-resolving future leavesdeliveringstuck true. Separately, the newRepoOutlinesretain-and-drain path has no direct coverage — the added test's mock resolves on full processing, whereas the realOutlineRepositorySubscriberresolves on channel enqueue, so the two-stage path that actually prevents the loss is untested. - Coalescing across a forced cut is lossless but not identical. An add and a later delete of the same path now land in separate batches instead of cancelling, and a move followed by deletion of its target emits move+delete rather than a delete of the source. The end state is the same and the current production consumer is filesystem-only outline watching, so this is noted rather than blocking — but it is inherent to early flushing and worth knowing before another subscriber is added.
Verdict
Checks: build pass, tests pass (repo_metadata 144 passed, 2 skipped), CI green, visual proof n/a. The warp app-crate check could not be independently confirmed — it was killed on memory in the review sandbox, which is an environment limit and not a source diagnostic.
Found: 1 critical, 2 important, 0 suggestions, 0 nits
Responding as wilson: Open session · View factory task
Second review round found that the previous revision fixed the single-input peak and correctly serialized delivery, but relocated the unbounded accumulation into two new queues (BufferState's delivery_queue and OutlineState's queued_updates), each storing one full RepositoryUpdate per batch that formed while the consumer was busy -- worse than the original bug, since the same path could now be stored once per batch. Replace both queues with a single coalescing accumulator: - BufferState::next_delivery: Option<RepositoryUpdate>, merged via merge_repository_updates instead of pushed. on_files_updated now interleaves per-entry merging with immediate delivery attempts: the first batch to cross the bound while nothing is in flight is dispatched immediately (bounded); anything that forms while a delivery is already running is merged into next_delivery. - OutlineState::pending_update: RepositoryUpdate, merged via a new public RepositoryUpdate::merge() (repo_metadata crate) instead of queued, and applied as one follow-up recomputation. This bounds the backlog to O(distinct un-applied paths) instead of O(events x paths-per-event) -- the minimum retainable without dropping anything, though not an absolute cap under a sustained, never-catching-up consumer. The PR description is updated to state that honestly rather than overclaiming a hard cap. Also fixes a real leak: on_unsubscribe only aborted the debounce flusher, leaving any pending/in-flight delivery live and able to call the (already unsubscribed) inner subscriber. It now marks the buffer inactive and clears pending/next_delivery; advance_delivery's completion callback becomes a no-op once inactive. Adds: - unsubscribe_cancels_pending_delivery_and_releases_the_backlog: proves an in-flight delivery still completes but a backlogged one is released, not delivered late. - concurrent_update_while_pending_is_merged_not_dropped_or_double_recomputed (app/src/ai/outline/native_tests.rs): drives a real RepoOutlines recomputation via ai::index::build_outline against a temp dir, submits a second update while the first is Pending, and asserts it merges into the accumulator instead of being dropped or triggering a second overlapping recomputation. Relaxes the two existing repo_metadata tests that asserted every batch stays within the bound: with coalescing, only the very first batch formed while idle is guaranteed bounded; everything else is guaranteed lossless and duplicate-free but may exceed the bound in size when the consumer is slow. Both now loop until every fed update is accounted for instead of asserting a fixed batch count/size.
Third review round: the coalescing accumulators, unsubscribe lifecycle, public RepositoryUpdate::merge wrapper, and the outline test all held up. One item reversed a prior scope call: the moves loop's `return` instead of `continue` (when a move's source collapses into an already-recorded add or modify) was previously judged a narrow, pre-existing issue unrelated to this change. hand_off's coalescing now reachably calls merge_repository_updates with real, multi-entry batches on exactly the path sustained churn exercises (merging a bounded batch into the backlog while a delivery is in flight), so the bug is now live on the path this PR's losslessness claim depends on. Fixed by changing both `return`s to `continue`s. Added: - Two direct unit tests (merge_repository_updates_applies_every_entry _when_a_move_collapses_into_added / _into_modified) pinning both branches. Verified both fail without the fix and pass with it. - coalescing_a_move_that_collapses_into_an_existing_add_still_applies _later_entries: an end-to-end regression through the actual reachable path (accumulate a backlog while a delivery is in flight, then coalesce a batch whose move collides with the backlog). Verified it fails without the fix and passes with it. - collect_until_seen test helper: the two loop-until-every-update- seen tests now use a deterministic receive timeout instead of an unbounded await, so a real regression fails fast with the missing paths and batch count instead of hanging until nextest's slow-timeout. - Expanded RepositoryUpdate::merge's doc comment: it's not a generic union, it applies debounce-style normalization, and merging out of chronological order can produce a result that doesn't correspond to any real sequence of filesystem events. Re-ran cargo nextest run -p warp (full app unit test suite, excluding integration tests) explicitly to confirm no OOM: 6479 tests run in 80s, 6476 passed. The 3 failures (server::server_api::ai::tests:: ambient_agent_headers_for_task_overrides_existing_cloud_agent_header, terminal::input::tests::test_histignorespace_support_in_zsh, terminal::input::decorations::tests:: test_decorations_with_multibyte_chars) are pre-existing and unrelated: confirmed they fail identically with this branch's repo_metadata/outline changes reverted, in files this PR never touches.
|
Fresh recurrence while this sat unmerged: Sentry event 60991a92, 2026-08-22, 9.8 GB sampled — 60% at the Responding as wilson: Open session · View factory task |


Description
Fixes a memory-usage bug:
BufferingRepositorySubscriber::on_files_updated(crates/repo_metadata/src/repository.rs) coalesces every incoming filesystem-watcherRepositoryUpdateintoBufferState.pendingviamerge_repository_updates, but the debounce flush loop had no maximum buffer size or age. Under sustained filesystem churn (a largegit checkout,npm install, or a very active watched tree), the flusher never observes a quiet period, sopending.added/modified/deleted(HashSet<TargetFile>) andpending.moved(HashMap<TargetFile, TargetFile>) grow unbounded before ever being flushed. This is the dominant contributor in a freshly symbolicated macOS heap profile (Sentry event4836b25da6904993af8aec96dfa45b5don issue 7259255054, 10.63 GB sampled, 95.2% attributed to this call path), and is fix #1 from APP-4829.This PR only implements fix #1 (bounding the debounce buffer). The other two facets described in APP-4829 are out of scope here: the AI codebase-index path-set clone was already fixed by #13413, and the file-tree-store clone is tracked separately by APP-4822.
Fix
BufferingRepositorySubscribernow bounds the debounce buffer with these cooperating pieces:RepositoryUpdateis merged intopendingone entry at a time, in the same phase ordermerge_repository_updatesalready applies (moves, adds, modifies, deletes, then the boolean flags). The bound is checked after every single entry, sopendingnever grows pastmax_pending_entries(currently 10,000) even when one incoming update by itself is far larger than the bound (e.g. a single very large batch from a huge checkout).inner.on_files_updatedis only ever called for one batch at a time; a completion callback drains whatever's next before starting the next delivery.pendinginto a queue, and duplicate the same paths across multiple stored batches), they are merged (viaRepositoryUpdate::merge, a new public wrapper around the samemerge_repository_updates) into a single accumulator (BufferState::next_delivery). Delivered as one batch once the in-flight delivery finishes.RepoOutlines(app/src/ai/outline/native.rs) used to silently discard a filesystem update that arrived while a previous outline recomputation was stillPending. It now merges such updates into a single accumulator (OutlineState::pending_update, viaRepositoryUpdate::merge) and applies it as one follow-up recomputation once the in-flight one completes.on_unsubscribenow marks the buffer inactive and drops bothpendingand any coalesced backlog; an already-in-flight delivery's completion becomes a no-op instead of continuing to callinner(and keeping the buffer's memory alive) after the subscription has ended.merge_repository_updatesthat coalescing made reachable. The moves loop usedreturninstead ofcontinuewhen a move's source collapsed into an already-recorded add or modify, which would abandon every remaining move, add, modify, delete, and flag in that same call. Coalescing now callsmerge_repository_updateswith real, multi-entry batches on the path sustained churn actually exercises (merging a newly-bounded batch into the backlog while a delivery is in flight), so this was no longer a theoretical, narrow-window pre-existing issue -- it was a live way for the losslessness this PR promises to be violated. Fixed by changing bothreturns tocontinues.What this bound does and does not guarantee (please read before assuming more than the code delivers): the per-entry merge plus coalescing collapses the backlog from O(events × paths-per-event) down to O(distinct un-applied paths) -- the minimum that can be retained without dropping anything. This is not an absolute cap. If the consumer never catches up during one sustained burst (the pathological case the ticket describes), the coalesced accumulator can still grow to approach the number of distinct changed paths in that burst, same as
pendingbriefly can before its first bounded batch is cut. Only the first batch formed while nothing is in flight is bounded to exactlymax_pending_entries; everything that arrives while that batch is being delivered is coalesced into one (unbounded-in-size, bounded-in-content-by-distinct-paths) backlog. A hard cap on that backlog would require dropping per-path fidelity -- an explicit dirty-state/rescan protocol -- which changes observable outline behavior and is explicitly out of scope for this PR; it should be a separate, human-reviewed follow-up if the O(distinct paths) behavior still isn't good enough in practice.Why 10,000 for the one guaranteed-bounded batch: it keeps that first batch's content to a few tens of MB (each buffered entry clones a
PathBuf), while staying large enough that ordinary bursts (tens-to-thousands of files) still coalesce normally through the debounce.Accepted behavior change, not a defect: splitting a debounce window into more, earlier flushes means an add-then-delete (or move-then-delete-of-target) of the same path that straddles a forced-flush boundary no longer cancels out within one batch the way it would have if the whole window had coalesced together. The end state converges to the same thing; a subscriber just sees slightly more intermediate churn. This is inherent to forcing early flushes and isn't fixed here.
Linked Issue
This is tracked in Linear, not a GitHub issue: APP-4829.
Testing
forced_flush_drains_pending_before_debounce_timer_fires: with an hours-long debounce and a smallmax_pending_entriesoverride, feeds exactly enough updates to cross the threshold and asserts the flush is observed -- proving it came from the forced path, not the debounce.single_incoming_update_exceeding_the_bound_is_delivered_without_loss: feeds a singleRepositoryUpdatefar larger than the configured bound in onenotify_subscribercall; asserts the very first delivered batch is exactly bounded, more than one batch is produced, and the union of every batch covers every entry exactly once with no duplicates. Uses a deterministic receive timeout so a regression fails fast instead of hanging until nextest's slow-timeout.forced_and_debounced_flushes_apply_every_update_exactly_once_with_a_slow_consumer: a slow mock consumer (that itself asserts it's never invoked again before its previous call's future resolved) drains forced and debounce-triggered batches; asserts the union of everything delivered accounts for every fed update exactly once, with no duplicates, and that nothing else arrives afterward. Same deterministic timeout.unsubscribe_cancels_pending_delivery_and_releases_the_backlog: unsubscribes while one batch is in flight and a second is only backlogged; asserts the in-flight batch still completes normally but the backlogged one is never delivered late.merge_repository_updates_applies_every_entry_when_a_move_collapses_into_added/..._into_modified: direct unit tests pinning thereturn-vs-continuefix for both collapse branches -- confirmed these fail without the fix and pass with it.coalescing_a_move_that_collapses_into_an_existing_add_still_applies_later_entries: an end-to-end regression through the actual reachable path -- accumulates a backlog while a delivery is in flight, then coalesces a second batch whose move collides with the backlog's content, and asserts everything after that move in the second batch is still applied. Confirmed this fails without the fix and passes with it.concurrent_update_while_pending_is_merged_not_dropped_or_double_recomputed(inapp/src/ai/outline/native_tests.rs): drives a realRepoOutlinesrecomputation (viaai::index::build_outlineagainst a temp directory) and submits a second update while the first is stillPending; asserts it's merged into the accumulator rather than dropped or starting a second overlapping recomputation, then drives to completion and asserts both changes are reflected in the final outline's file count.cargo nextest run -p repo_metadata(148 passed, 2 skipped),cargo nextest run -p warp -E 'test(outline)'(the new outline test passes, confirmed not OOM-killed),cargo clippy -p repo_metadata --all-targets --tests -- -D warnings,cargo clippy -p warp --all-targets --tests -- -D warnings,cargo test -p repo_metadata --doc,cargo check -p warp --tests, and./script/format --checkall pass, with no OOM observed running any of these in this environment.Not manually tested with
./script/run: this is a background buffering/consumption behavior with no UI surface, exercised entirely by the unit tests above.The linked issue is labeled
ready-to-specorready-to-implement. (N/A -- tracked in Linear, not a GitHub issue)Where appropriate, screenshots or a short video of the implementation are included below (especially for user-visible or UI changes). (N/A -- no UI surface)
I have manually tested my changes locally with
./script/run(N/A -- see Testing above)Agent Mode
CHANGELOG-BUG-FIX: Fixed unbounded memory growth in the repository file watcher's debounce buffer under sustained filesystem churn (e.g. a large git checkout or npm install).