Skip to content

perf(server): keep message bodies out of search ranking sorts - #23921

Closed
UtkarshUsername wants to merge 1 commit into
pingdotgg:mainfrom
UtkarshUsername:perf/server-thread-search-ranking
Closed

UtkarshUsername wants to merge 1 commit into
pingdotgg:mainfrom
UtkarshUsername:perf/server-thread-search-ranking

Conversation

@UtkarshUsername

@UtkarshUsername UtkarshUsername commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Thread search carries every matching message body through ranking, causing large temporary sorts and repeated JSON extraction in bigger histories.

Change

Rank and limit message IDs and metadata first, then fetch the returned bodies through the existing primary-key index. Preserve matching, ranking, snippets, and result order.

Scope and approval

Submitted under the exception for a small, focused fix to an obvious defect: thread search unnecessarily carries full message bodies through ranking, causing large temporary writes on matching histories. This changes only the existing query and its tests, preserving matching, ranking, snippets, and result order. No schema, contract, or product behavior changes.

Verification

Service latency when every message matches, limit 50:

Messages × characters/body Before After Less time
10,000 × 512 74.0 ms 40.7 ms 45%
50,000 × 512 431.7 ms 234.6 ms 46%
10,000 × 8,192 708.1 ms 137.6 ms 81%
  • Fixture with long messages: logical writes fell from approximately 167 MB/search to zero in separate Windows process-counter probes. Peak RSS stayed similar; native SQLite allocations were not sized. Logical transfers are not physical disk bytes.

  • Real service on Node 26.12.1/SQLite 3.54.0, synthetic histories with 100 active threads, five isolated candidate trials and paired baseline controls. Maximum A/A difference for these rows: 11.4%. Timings exclude network/UI. Sparse/no-match searches and the smallest fixture with one short message per thread stayed within noise; substring scanning remains.
    Benchmarks and tests cover final commit 046f086; the committed query matches the measured source.

  • vp test run apps/server/src/orchestration-v2/ThreadSearch.test.ts: four passed, including ranking/ties, limits, literal wildcards, long snippets, eligibility, and reader worker concurrency. Another 9,600 generated SQLite comparisons matched the baseline.

  • vp run check:changed upstream/main: passed format, lint, knip, GraphQL generation, affected-package typechecks, and adjacent tests.

Implemented with GPT-6.1 Sol (high) through Codex in T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 23, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 23, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 046f086

Macroscope's review found this PR approvable — This is a focused optimization of the existing thread-search query: ranking and limiting now operate on metadata and IDs before retrieving the selected message bodies. The accompanying tests cover ordering, limits, literal matching, and snippets, with no schema, API, default, or deployment changes.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Oct 23, 2026 •

Copy link
Copy Markdown
Contributor

Review in Change Stack →

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
📥 Commits

Reviewing files that changed from the base of the PR and between 18e2742 and 046f086.

📒 Files selected for processing: 2
  • apps/server/src/orchestration-v2/ThreadSearch.test.ts
  • apps/server/src/orchestration-v2/ThreadSearch.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

📝 Walkthrough

Thread search now ranks matching message metadata per thread, applies the result limit before retrieving selected message text, and includes tests for match ranking, literal wildcard characters, and snippet length.

Changes: Thread search

Layer / File(s) Summary
Rank thread matches and retrieve selected text
apps/server/src/orchestration-v2/ThreadSearch.ts, apps/server/src/orchestration-v2/ThreadSearch.test.ts
The query ranks matching messages per thread, selects one message per thread, and applies the limit before retrieving text. Tests check message and thread ordering, tie-breaking, literal matching, and snippet length.

Priority: ➖ Normal

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Refactor

Suggested reviewers: maria-rcks

Merge Risk: ⚪ Minimal · up to 046f0

The search change has no identified merge-blocking risk. It is ready for normal merge checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title "perf(server): keep message bodies out of search ranking sorts" follows conventional commit format and names the main change: thread search ranks and limits message IDs and metadata before l…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Description check Passed The description follows the template's main sections (Problem, Change, Scope and approval, Verification) and gives a clear problem statement, a concrete change, a scope justification under the small-f…
Full details: Title check

Explanation

The title "perf(server): keep message bodies out of search ranking sorts" follows conventional commit format and names the main change: thread search ranks and limits message IDs and metadata before loading message bodies. The changeset matches this: ThreadSearch.ts now ranks per-thread metadata, selects one message per thread, then fetches text for the selected messages, and ThreadSearch.test.ts adds tests for ranking, limits, and snippets.

Full details: Description check

Explanation

The description follows the template's main sections (Problem, Change, Scope and approval, Verification) and gives a clear problem statement, a concrete change, a scope justification under the small-fix exception, and detailed verification with benchmark tables, test results, and a check run. Remaining gaps are minor: the Scope section does not link a triaged issue or maintainer approval comment, and the description relies on a small-fix exemption that the reviewer must judge. The author also names the model and harness used, as the template asks. Overall the description is mostly complete.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Comment @coderabbitai help to get the list of available commands.

@UtkarshUsername

Copy link
Copy Markdown
Contributor Author

Superseded by #24343.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:M 30-99 changed lines (additions + deletions). vouch:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant