Skip to content

perf(server): bound ordinal recovery to selected messages - #27495

Open
Motaghi wants to merge 1 commit into
pingdotgg:mainfrom
Motaghi:perf/bounded-message-ordinals
Open

Motaghi wants to merge 1 commit into
pingdotgg:mainfrom
Motaghi:perf/bounded-message-ordinals

Conversation

@Motaghi

@Motaghi Motaghi commented Oct 30, 2026

Copy link
Copy Markdown

Problem

Bounded message-record reads load the requested messages, then recover ordering by grouping every user/assistant timeline item in the thread. A small MCP t3_thread_read page can therefore return thousands of unused ordinal groups. The message-ordinal query left open in #17038 still has this behavior.

Change

Restrict ordinal recovery to the messages actually loaded when message IDs, runs, or a role selection narrows the read. Use the existing JSON ID-list encoder and json_each, retaining MIN(ordinal) across duplicate timeline items. Unfiltered reads and role-only reads covering both user and assistant messages keep their existing query, avoiding extra membership work for title regeneration. No index, migration, DTO, or ordering change.

Scope and approval

This is a focused performance fix for an unnecessary aggregation in an already bounded read. It preserves the returned records and uses the existing filtering mechanism, so I am submitting it under the small, focused obvious-bug exception. No maintainer approval is claimed.

Related work: #17038 skipped empty-filter scans and explicitly left this ordinal lookup unresolved. Open #22354 proposes the message expression index; this PR adds no competing migration. With that index present, the existing query still reads all message groups, while this predicate enables a thread/message-ID lookup.

Verification

  • Extended the existing ProjectionStore SQL-bounded-history test. On the original production query, its bounded-group assertion fails; after the fix, selected reads return only the loaded messages' ordinal groups. It checks duplicate minimum ordinals, other-thread isolation, null/missing IDs, ID/role/run filtering, empty and singleton selection, and unchanged SQL for unfiltered and broad role-only reads.
  • Focused regression: 1 failed before, 1 passed after. Full ProjectionStore.test.ts: 44 passed. On the ordinal-only two-file tree at 97e35bcf23e4, vp run check:changed passed formatting, lint, Knip, GraphQL generation, three affected-package typechecks and 45 tests in two files. Missing workspace dependency links were restored with the frozen lockfile before the final checks. Before publication, a fresh ordinal-only worktree on main 32ecd71726fe passed the same checks: three affected-package typechecks and 45 tests in two files. The reviewed patch is unchanged.
  • Isolated, file-backed samples from four private technical sessions, using their exact timeline payloads and installed indexes. Node 26.12.1, SQLite 3.54.0, no ANALYZE; one warm-up per variant, then 15 alternating before/after executions. A 20-message selection produced identical selected ordinal rows in every sample.
Timeline items Ordinal groups, before → after Query median, before → after
866 71 → 20 0.398 → 0.388 ms
6,263 1,317 → 20 8.731 → 4.358 ms
12,595 4,417 → 20 24.129 → 10.197 ms
34,409 8,174 → 20 49.668 → 24.721 ms

With current indexes, both versions still scan the thread's history and use a temporary grouping B-tree. With only #22354's proposed message index added to the same isolated sample, the original query took 10.686 ms and the narrowed query 0.108 ms on the largest sample. The narrowed plan searches by thread plus message expression. That index is not included here.

These are SQL query timings, not a complete MCP request or a measured mobile speedup. Samples were copied in short read-only batches, not as one atomic snapshot. A second replay retained identical selected results; timings varied with cache/system conditions, so they are illustrative. Both raw runs are retained privately. No private session records are included. The patch has not been installed on the live server or tested on a phone.

Implemented with Claude Opus 5.5 in Claude Code and GPT-6.1-Sol in Codex, through T3 Code. Independent reviewers: GPT-6.1-Sol xhigh and Claude Opus 5.5 high.

@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Oct 30, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 30, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 5b4382e

Macroscope's review found this PR approvable — Ordinal recovery is narrowed to selected messages while broad reads keep their existing query. This read-only optimization introduces no data migration, product-default change, or material operational exposure.

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

@coderabbitai

coderabbitai Bot commented Oct 30, 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 32ecd71 and 5b4382e.

Files selected for processing: 2
  • apps/server/src/orchestration-v2/ProjectionStore.test.ts
  • apps/server/src/orchestration-v2/ProjectionStore.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

The message-order lookup now filters turn items to loaded message IDs for message-ID, run-ID, and partial-role reads. Tests check filtered and unfiltered query behavior, including empty selections and single-message reads.

Changes: Message ordinal lookup

Layer / File(s) Summary
Filter ordinal lookup by loaded messages
apps/server/src/orchestration-v2/ProjectionStore.ts, apps/server/src/orchestration-v2/ProjectionStore.test.ts
Filtered reads restrict ordinal lookup to loaded message IDs. Broad reads retain the unfiltered query. Tests cover message, run, and role filters, empty selections, and single-message reads.

Priority: Low

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

Change: Refactor

Suggested reviewers: juliusmarminge, t3dotgg

Merge Risk: Minimal · up to 5b438

Filtered reads limit ordinal aggregation while preserving tested message ordering. No merge-blocking issue was identified.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title is concise, specific, and accurately describes the main change: limiting ordinal recovery to selected messages.
Description check Passed The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the issue, implementation, scope rationale, related work, focused tests, performance evide…
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.
Full details: Description check

Explanation

The description includes complete Problem, Change, Scope and approval, and Verification sections. It explains the issue, implementation, scope rationale, related work, focused tests, performance evidence, limitations, and agent usage.

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.

This branch has not been deployed

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

Labels

size:XS 0-9 changed lines (additions + deletions). vouch:unvouched PR author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant