[APP-4280] Async blocklist search#9618
Conversation
vkodithala
changed the title
| ```rust path=null start=null | ||
| if model.is_alt_screen_active() { /* alt-screen path, unchanged */ } | ||
| else if let Some(controller) = &mut self.async_find_controller { /* async path */ } | ||
| else { /* legacy sync path, unchanged */ } | ||
| ``` |
There was a problem hiding this comment.
From my understanding, there are some 3rd-party agents that do render in alt screens (OpenCode and Claude Code no-flicker mode, I think). It seems like we're running find on alt screens synchronously in all cases, maybe as a fast follow we can extend this logic to alt screens if not totally impossible?
There was a problem hiding this comment.
I think there's an adjacent issue, which is that we don't search above the current view for alt screens (but also probably fine to de-prio if it hasn't already been flagged)?
There was a problem hiding this comment.
alt screen is tiny by comparison - it never contains scrollback. we could potentially also do async scanning of the alt screen, but it would add complexity and probably zero meaningful benefit.
There was a problem hiding this comment.
Yeah, this was my understanding too. Appreciate the explanation!
| // For now, we do a full rescan when the query is refined. | ||
| // A future optimization could filter existing matches without rescanning, | ||
| // and update the query for pending queue items. | ||
|
|
||
| // Create new config. |
There was a problem hiding this comment.
Ack, can probably include in a fast follow
|
|
||
| `background_task::run_find_task_loop` (`background_task.rs (56-128)`) builds `RegexDFAs` once from the config, then: | ||
|
|
||
| - For `FullBlock { block_index }`: calls `scan_terminal_block_chunked`, which iterates the grid in the block-sort-direction-aware order and delegates each grid to `scan_grid_chunked`. |
There was a problem hiding this comment.
From my understanding, we match on chunks of arbitrary size 1k. Does this fail if a match falls on a chunk boundary? Should we instead end chunks on newlines (I don't think our DFA/Regex based find extends to multiple lines?)
There was a problem hiding this comment.
i believe this was something that the agent addressed and wrote a test to cover, but one of us would need to check again.
There was a problem hiding this comment.
Gotcha, I can take a look here. Thanks!
| ## Follow-ups | ||
|
|
||
| - True in-place query refinement (today `filter_results_for_refinement` rescans). The plumbing — `is_query_refinement`, `current_find_options`, the queue's `invalidate_block` — is already in place. | ||
| - Move AI block scanning off the main thread (currently still synchronous via `ScanAIBlock` round-trip). |
There was a problem hiding this comment.
Curious if there's any functional reason we don't also scan AIBlocks on background threads, like we do for terminal blocks? i.e., is this a design thing or just OOS?
There was a problem hiding this comment.
the terminal model is shareable across threads (it's in an Arc<Mutex<...>>), but AFIAK AI blocks are not - they're UI framework entities, and as such, can only be accessed from the main thread.
that said, each AI block tends to be quite small, in the scheme of things, so scanning a full one shouldn't take very long. (by comparison, a terminal block could be as many as 1M lines long.)
There was a problem hiding this comment.
Yeah, that makes sense. Appreciate the context.
|
Did an initial pass via back-filled tech/product specs, think this mostly makes sense. The intent seems pretty clear, which is that we're moving from a synchronous scan of each terminal/AI block in the blocklist to a mostly async scan, and wiring up everything necessary to support this. Noticed a few issues while dogfooding:
And have a few follow-up qs for ya @vorporeal (mostly non-blocking & out of curiosity). Hoping to wrap this up soon! |
yeah i'm not surprised there are some bugs there - correct match highlights were one piece that the agent consistently struggled with.
yep should definitely fix this one.
might relate to the two other findings (bugs in the match order/highlight index)
|
vkodithala
force-pushed
the
david/async-find
branch
2 times, most recently
from
bcf782a to
3d78487
Compare
Track which blocks have been fully scanned during the initial async find run. When invalidate_block is called during an ongoing scan: - If the block hasn't been scanned yet, skip it (the scan will handle it). - If the block was already scanned, do a synchronous full rescan without cancelling the ongoing scan. - If no scan is in progress, spawn a background task as before. Adds a BlockScanned message from the background task and a sync_rescan_full_block method for the during-scan case. Co-Authored-By: Oz <oz-agent@warp.dev>
Store the FindOptions in AsyncFindController alongside AsyncFindConfig so that active_find_options() can return a reference when async find is active. Previously it would return None during async find since the sync block_list_find_run is not populated. Co-Authored-By: Oz <oz-agent@warp.dev>
focused_terminal_match() previously sorted all HashMap keys and iterated through matches on every call. Since this is called per-visible-block per-frame via find_render_data_for_block, this was O(k log k + n) on every frame. Cache the result and recompute only when focused_match_index changes or when matches are added/removed/modified. Co-Authored-By: Oz <oz-agent@warp.dev>
vkodithala
force-pushed
the
david/async-find
branch
from
3d78487 to
1c981d9
Compare
|
I'm starting a first review of this pull request. You can view the conversation on Warp. I completed the review and no human review was requested for this pull request. Comment Powered by Oz |
![oz-for-oss[bot]](https://avatars.githubusercontent.com/in/3450139?s=60&v=4){kind=small}
There was a problem hiding this comment.
Overview
This PR moves terminal blocklist find to an async controller with background scanning, streamed results, dirty-range invalidation, and a new scanning indicator in the find bar. I found several correctness issues in the async result lifecycle and dirty-range bookkeeping that can leave stale highlights/counts or make navigation fail for rich-content matches.
Concerns
- Dirty row tracking only records the cursor row, so multi-row terminal mutations can leave stale async find results outside the rescanned range.
- Dirty-range merge coordinates use a truncation offset captured when work is enqueued rather than when the grid is scanned, which can splice replacements into the wrong absolute rows if scrollback truncates before the background task runs.
- The background task can emit
Donebased on a queue-empty snapshot taken before a long scan, so invalidations enqueued during that scan may still be pending while the UI reports completion. - Rich-content matches are counted in focus traversal but cannot be resolved as focused matches, so next/previous can land on an AI result that
scroll_to_matchcannot scroll to. - This is a user-facing behavior/UI change, but the PR description has no screenshot or short recording showing the scanning indicator and streaming highlights working end to end. Please attach visual evidence before merge.
Verdict
Found: 0 critical, 5 important, 0 suggestions
Request changes
Comment /oz-review on this pull request to retrigger a review (up to 3 times on the same pull request).
Powered by Oz
| *dirty_cells_range = range_start..range_end; | ||
|
|
||
| // Also update the find dirty rows range. | ||
| self.update_find_dirty_rows_range(cursor_point.row); |
There was a problem hiding this comment.
dirty_cells_range can cover multiple rows for clears, wrapping, insert/delete lines, or other ANSI mutations. Async find will rescan only the cursor row and can leave stale matches in the other changed rows; accumulate range_start.row..=range_end.row instead.
| ScanResultMode::DirtyRange { | ||
| num_lines_truncated, | ||
| } => { | ||
| let absolute_start = current_row as u64 + num_lines_truncated; |
There was a problem hiding this comment.
num_lines_truncated is captured when the dirty work is enqueued, but AbsoluteMatch::from_range uses the grid's truncation offset at scan time. If scrollback truncates again before this task runs, dirty_range no longer lines up with the returned matches and update_dirty_matches can splice the wrong rows; compute the absolute dirty bounds from the same locked grid snapshot used to build the matches.
| // The emptiness flag is checked atomically with the pop inside | ||
| // the queue lock, avoiding the TOCTOU race of a separate | ||
| // `is_empty()` call. | ||
| if queue_drained { |
There was a problem hiding this comment.
queue_drained is a snapshot from before the work item is processed. If invalidate_block enqueues more work while a large block scan is running, this still sends Done afterward, so the controller can show the final N/N state while new dirty/full-block work is pending; check drain state after processing or track in-flight work before emitting completion.
| } | ||
| } | ||
| } | ||
| BlockInfo::RichContent { view_id, .. } => { |
There was a problem hiding this comment.
BlockListMatch::RichContent lets focused_match_index land on an AI result while focused_block_list_match() returns None, so next/previous navigation and scroll-to-match fail for that result. Store enough rich-content match data to resolve the focused match, or exclude these from focus traversal until supported.
vkodithala
force-pushed
the
david/async-find
branch
from
7189363 to
b0c93c7
Compare
3 participants
Closes #9757.
Description (from @vorporeal):
Moves find in the blocklist off the main thread so that searching large terminal histories no longer causes UI freezes.
The synchronous path scans every block (and all rich-content/AI blocks) on the main thread whenever the query changes, a block updates, or the active block scrolls. For large histories this can stall the UI for noticeable amounts of time. This PR introduces an
AsyncFindControllerthat runs searches on a background task and streams results back to the main thread.Key pieces:
AsyncFindController(app/src/terminal/find/model/async_find.rs): controller owned byTerminalFindModel, hidden behind theAsyncFindfeature flag. Owns the current config, per-block results, focused match state, a shared work queue, and the spawned task handle. Implements the sameFindModelsurface (match_count,focused_match_index,focus_next_match, etc.) used by the existing sync path.FindWorkQueue(async_find/work_queue.rs): a shared queue ofScanFullBlock/ScanDirtyRange/ScanAIBlockitems. The background task awaits it via anevent_listener::Event, so there is no polling.invalidate_blockpushes dirty work to the front so newly-changed blocks are prioritized; pending full-block rescans for the same block are deduped.background_task(async_find/background_task.rs): the task loop pulls work items, buildsRegexDFAsonce per run, and streamsFindTaskMessageresults back over anasync_channel. Grid scanning is chunked (ROWS_PER_CHUNK,MAX_LOCK_DURATION_MS) so the terminal model lock is never held long enough to block the main thread, and the task yields between chunks viafutures_lite::yield_now. AI blocks are bounced back to the main thread viaScanAIBlocksince rich-content search runs there.AbsoluteMatch(absolute row indices fromAbsolutePoint). This means scrollback truncation does not require re-indexing existing results — truncated matches are filtered on read viaAbsoluteMatch::to_range. Dirty-range updates useBlockFindResults::update_dirty_matchesto splice replacements into the existing ordered vector.BlockFindRenderDataabstraction inapp/src/terminal/find/model.rsgivesblock_list_element.rsa single interface over both the sync and async paths, and pre-converts async absolute ranges to relativePointranges per grid.GridHandlernow accumulates afind_dirty_rows_rangeduring byte processing (take_find_dirty_rows_range) so we can hand the background task the exact rows that changed instead of rescanning whole blocks.rerun_find_on_active_gridand a newnotify_block_completedpath onTerminalFindModelfeed dirty ranges and block completions intoinvalidate_block. A throttled channel in the controller coalesces result bursts into at most oneFindEvent::RanFindper 50 ms, and a monotonicgenerationcounter guards against staleDonemessages ending a newer scan. The find bar renders a "Scanning..." /N+ ...indicator while the background task is still running.FeatureFlag::AsyncFindgates creation of the controller; the sync path is the default everywhere else. For now (while @vkodithala fixes up issues in "Work still needed"), we disable this path for all builds, including dogfood. Using async find is restricted to app runs with the feature flag:./script/run --features async_find.Work still needed
./script/runand select the second match from the bottom of the blocklist. As new results inevitably stream in, that match index isn't preserved (defaults to the most recent match). Regression from dev.Matches should be in ascending order after update_dirty_matches.Linked Issue
ready-to-specorready-to-implement.Testing
async_find_tests.rs(~800 lines, 18 tests) covering: parity with sync find, cancellation, streaming message handling, focus navigation / wraparound ordering for bothBlockSortDirections, dirty-range merging (prepend / append / replace-middle / clear / empty-existing), absolute-match truncation, andBlockFindResultsbookkeeping (total_match_count,remove_block).Agent Mode