Skip to content

Fix search performance regression and TUI race condition#262

Merged
wesm merged 5 commits intowesm:mainfrom
jesserobbins:fix-search-perf-and-race
Apr 15, 2026
Merged

Fix search performance regression and TUI race condition#262
wesm merged 5 commits intowesm:mainfrom
jesserobbins:fix-search-perf-and-race

Conversation

@jesserobbins
Copy link
Copy Markdown
Contributor

@jesserobbins jesserobbins commented Apr 13, 2026

Tested and ran into a bunch of performance & context issues that surfaced in the TUI. Original approach using ILIKE is better it seems like for large archives. (Note this is a resubmission of #256)

Changes

  • Reverts DuckDB text search from regexp_matches to ILIKE — regexp caused significant performance regression on Parquet scans. ILIKE is natively optimized by DuckDB for columnar data. FTS5 deep search path is unchanged.
  • Fixes a TUI race condition where pressing a (all messages) then quickly searching could cause the stale loadMessages response to overwrite search results. Search now invalidates pending loads via loadRequestID.
  • Fixes a placeholder/arg count drift in buildStatsSearchConditions — text terms emit 4 args each (subject + snippet + 2 sender) but the arg-slicing helper assumed 3. Extracted buildNonTextSearchConditions to eliminate the brittle offset math entirely. Regression tests lock the placeholder invariant across both search builders.

jesserobbins and others added 4 commits April 10, 2026 11:33
@jesserobbins @claude
Revert DuckDB search from regexp_matches to ILIKE for performance
01aaf38 
The regexp_matches approach caused significant performance regression
in TUI fast search (Parquet scans). ILIKE is natively optimized by
DuckDB for Parquet columnar scans. FTS5 deep search path remains
unchanged for body text accuracy.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@jesserobbins @claude
Fix search race condition: invalidate pending loadMessages on search
0981119 
When starting an inline search, increment loadRequestID to prevent
in-flight loadMessages responses from overwriting search results.
Previously, pressing (a) to view all messages then quickly searching
could cause the stale loadMessages response to replace search results
with unfiltered messages.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@jesserobbins @claude
Add tests for search race condition and ILIKE performance fix
d3811db 
- TestStaleLoadMessagesDoesNotOverwriteSearch: verifies that in-flight
  loadMessages responses (from sort toggle or "all messages" view) are
  ignored after a search starts, preventing stale results from
  overwriting search results
- TestSearchClearsStaleMessages: verifies messages are cleared
  immediately when search starts so stale results never display
- TestBuildSearchConditions_UsesILIKENotRegex: verifies the fast search
  path uses ILIKE (fast on Parquet) instead of regexp_matches (slow)
- TestBuildAggregateSearchConditions_UsesILIKENotRegex: same check for
  the aggregate search path
- Remove dead code: escapeRegex and startsWithWordChar (no longer used
  after reverting to ILIKE)

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@claude
Fix stats search arg/placeholder drift by extracting non-text helper
bf776fb 
buildStatsSearchConditions used to call buildAggregateSearchConditions
and slice off the text-term prefix using a hand-tracked
countArgsForTextTerms helper. Any drift between the helper and the
actual per-term arg count (subject + snippet + 2 sender = 4) would
cause aggregate stats queries to fail with unmatched placeholders when
a search mixed text terms with non-text operators like from:.

Extract the non-text filter portion of buildAggregateSearchConditions
into a new buildNonTextSearchConditions helper and call it directly
from buildStatsSearchConditions, eliminating the arg-slicing workaround
and the brittle countArgsForTextTerms constant entirely.

Add regression tests that lock the invariant in place by counting "?"
placeholders in the emitted WHERE conditions and comparing to len(args)
across a matrix of query shapes (text only, text+from, text+to+subject,
label view, date/size filters, etc.) for both builders.
@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 13, 2026

roborev: Combined Review (bf776fb)

Verdict: Changes are mostly sound, but there is one medium-severity UI regression to address before merging.

Medium

  • False "No results found" state during inline search
    • Location: internal/tui/model.go:1194 (affects internal/tui/view.go)
    • Finding: Clearing m.messages = nil when a search starts can cause the view to briefly render a misleading empty state. The empty-state logic shows "No results found" when len(m.messages) == 0 && !m.loading, but debounced inline search sets m.inlineSearchLoading = true while leaving m.loading = false. That means users can see a false no-results message while the inline search request is still in flight.
    • Suggested fix: In internal/tui/view.go, suppress the empty-state message while inline search is loading, e.g. require !m.inlineSearchLoading in the condition: 
      if len(m.messages) == 0 && !m.loading && !m.inlineSearchLoading

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@jesserobbins jesserobbins marked this pull request as ready for review April 13, 2026 16:17
@jesserobbins
Copy link
Copy Markdown
Contributor Author

jesserobbins commented Apr 13, 2026

roborev: Combined Review (bf776fb)

Verdict: Changes are mostly sound, but there is one medium-severity UI regression to address before merging.

Medium

  • False "No results found" state during inline search

    • Location: internal/tui/model.go:1194 (affects internal/tui/view.go)
    • Finding: Clearing m.messages = nil when a search starts can cause the view to briefly render a misleading empty state. The empty-state logic shows "No results found" when len(m.messages) == 0 && !m.loading, but debounced inline search sets m.inlineSearchLoading = true while leaving m.loading = false. That means users can see a false no-results message while the inline search request is still in flight.
    • Suggested fix: In internal/tui/view.go, suppress the empty-state message while inline search is loading, e.g. require !m.inlineSearchLoading in the condition: 
      if len(m.messages) == 0 && !m.loading && !m.inlineSearchLoading

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

I still disagree with this recommendation. It is better to show nothing than stale search. I discussed this in #256

@wesm @claude
Add query engine benchmark smoke tests with make bench target
74b9da6 
Benchmarks 6 hot-path operations (SearchFast, SearchFastWithStats,
Aggregate, GetTotalStats, ListMessages, SubAggregate) against a
100K-message Parquet dataset with 500 participants across 50 domains.
Data generated via DuckDB SQL for fast setup (~1s).

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>
@wesm
Copy link
Copy Markdown
Owner

wesm commented Apr 15, 2026

Confirmed. Adding a minimal benchmark so we can keep an eye on this

@roborev-ci
Copy link
Copy Markdown

roborev-ci bot commented Apr 15, 2026

roborev: Combined Review (74b9da6)

No Medium, High, or Critical issues found; the reviewed changes look clean.

All reviewers who reported findings agreed there were no issues requiring comment at Medium severity or above.

Synthesized from 3 reviews (agents: codex, gemini | types: default, security)

@wesm wesm merged commit 09d18df into wesm:main Apr 15, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@jesserobbins @wesm @claude