Sign up
View stargazersSign up

perf(server): use indexed lookups for windowed thread snapshots#21193

closed8
main
GitHub fork: perf/long-thread-indexes
opened10d agobyAAlyx-ML
7 files changed
View on GitHubTwo-way sync enabledPR last imported 5d ago

Windowed thread snapshots still traversed completed run, attempt, message, provider-turn, and checkpoint histories. On long threads, a small requested window could therefore do work proportional to the whole history.

The queries now give SQLite independent indexed live/cohort branches while preserving thread guards, ordering, duplicate elimination, and retained node ancestry. Migration 65 adds seven indexes for those lookups and delegated-completion recovery. Existing migration 64 on main is preserved.

Original contribution by @Alyx-ML in #21193; contributor commits retained and current main merged normally.

Verification: 51 → 53 focused tests across ProjectionStore, migration installation, canonical-link upgrades, and preview upgrades. The canonical-link upgrade expectation now includes migration 65, correcting the Server 7 CI failure. An expanded migration compatibility run passes 62 tests across six files. Main fails the two query-plan regressions; the semantic case passes on both revisions. Tests cover live/cohort state, thread isolation, fork retention, ancestry, ordering, and delegated delivery. Server typecheck and touched-file lint/format checks pass.

A repeat headless benchmark calls the real ProjectionStoreV2 service against identical synthetic 10,000-turn databases with 2 KiB message/item text and a 76-row window. After five warmups, 21 serial calls measured 52.1 ms → 15.8 ms median (3.3×). Snapshots were byte-identical (809,500 bytes). Delegated recovery measured 9.90 ms → 0.07 ms. Individually removing each new snapshot index made its paired service measurement slower. Reproduction results and limitations.

Migration review: fresh/upgrade/preview/retry tests pass. Building the seven indexes on the populated synthetic schema-64 database took 38 ms in memory and added about 1.14 MiB; malformed legacy JSON was tolerated. Index construction scans existing tables and can hold a writer lock. Large on-disk upgrade duration was not verified. Installations that already ran the unmerged fork's conflicting migration 64 need separate ledger repair.

These are synthetic in-memory service measurements on a shared Linux host, with no SQLite statistics. They exclude transport, disk/fsync, app startup, and rendering. The original 5.6 s → 150 ms UI claim was not reproduced. No visual behavior changed, so screenshots are not applicable. Packaged Electron desktop, macOS, Windows, iOS, and Android were not verified.

Macroscope's existing approvability review requires human review of the production query and startup-migration impact. This preparation does not waive that gate.

Implementation: Codex gpt-6.1-sol; tests: Codex gpt-6-astra; adversarial reviews: Claude claude-opus-5-5 and Codex gpt-6-sol, through T3 Code.

Checks

20 successful · 7 skipped
All checks passed
184fc44

Added 1 commit

  • perf(server): long chats open in 150 ms instead of 5.6 sb83d5a0
MmacroscopeappBot10d ago
Synced from GitHub

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR rewrites existing production thread-snapshot queries and adds seven database indexes through a startup migration, changing query plans and database maintenance costs across several projection tables. The optimization is focused and backward-compatible in shape, but its production runtime and schema impact warrant human review.

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

CcoderabbitaiBot10d ago
Synced from GitHub

Review in Change Stack →

Warning

Review limit reached

Only developers with an assigned seat can use this organization's usage-based review budget, and seats here are assigned manually. Ask an admin to assign a seat, or change the review continuation mode in Billing.

Next included review available in 8 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 10 included reviews currently available.

Learn how review limits work.

Review configuration:

⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: 1aa0b186-adeb-4773-a6a1-bc489f6b29fe
📥 Commits

Reviewing files that changed from the base of the PR and between b83d5a006239ed89885acc8e7ed901571ad6e0d8 and 184fc44a7c9dd34f282b5a879fa7078076ba7b4d.

📒 Files selected for processing: 7
  • apps/server/src/orchestration-v2/ProjectionStore.test.ts
  • apps/server/src/orchestration-v2/ProjectionStore.ts
  • apps/server/src/persistence/Migrations.ts
  • apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts
  • apps/server/src/persistence/Migrations/064_CanonicalizeThreadPullRequestLinks.test.ts
  • apps/server/src/persistence/Migrations/066_LongThreadIndexes.ts
  • apps/server/src/persistence/reconcileV2PreviewMigration.test.ts

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration
  • Configuration used: Path: .coderabbit.config.ts
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: f4370de4-30c9-463f-b1a2-3a4b8e0608a5
📥 Commits

Reviewing files that changed from the base of the PR and between 9c161462c94f7d9d6d9169da5c841457cbadf213 and b83d5a006239ed89885acc8e7ed901571ad6e0d8.

📒 Files selected for processing: 5
  • apps/server/src/orchestration-v2/ProjectionStore.ts
  • apps/server/src/persistence/Migrations.ts
  • apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts
  • apps/server/src/persistence/Migrations/064_LongThreadIndexes.ts
  • apps/server/src/persistence/reconcileV2PreviewMigration.test.ts

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

📝 Walkthrough

Migration 64 adds indexes for projection records and is added to the migration registry and migration tests. Projection queries split status and ID predicates into separate OR arms, using unary plus on selected ID predicates. Existing selection and filtering conditions remain unchanged.

Changes: Projection Query Indexing

Layer / File(s)Summary
Add and register projection indexes
apps/server/src/persistence/Migrations/064_LongThreadIndexes.ts, apps/server/src/persistence/Migrations.ts, apps/server/src/persistence/Migrations/055_OrchestrationV2.test.ts, apps/server/src/persistence/reconcileV2PreviewMigration.test.ts		Migration 64 adds a partial index for delegated-completion runs and indexes for live run attempts, provider turns, and checkpoints, plus provider-turn and checkpoint lookup indexes. The registry and migration tests include migration 64.
Separate projection query predicates
apps/server/src/orchestration-v2/ProjectionStore.ts		Retained-node and windowed queries separate status and ID lookups into OR arms. Selected ID predicates use unary plus. A comment associates the delegated-completion query with the migration 64 index.

Priority: ➖ Normal

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge

Merge Risk: ⚪ Minimal · up to b83d5

The inspected projection lookups use indexes, and no issue remains that needs correction before merge.

Security architecture risk: 🔵 Low · up to b83d5

The database change is additive, and the changed reads retain their thread filters. No new access or privilege expansion was identified. Recovery after an interrupted upgrade or concurrent startup remains incompletely verified.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • observed — Index construction operates across the qualifying projection rows in a database, rather than one requested chat. The changed snapshot reads remain filtered to their supplied thread ID, bounding their data-selection scope separately from the database-wide migration work.

Trust Boundaries and Controls

  • observed — The rewritten SQL continues to bind thread and cohort values rather than concatenate them into SQL text. In particular, both message-selection arms retain the thread predicate, and the active-run subquery independently filters by that thread.

Resilience and Maintainability Implications

  • observed — The unchanged SQLite client acquires a transaction-scoped semaphore permit with a release finalizer and configures writable transactions to begin with BEGIN IMMEDIATE. These are relevant serialization controls, but do not establish where the unavailable migrator places its ledger checks and index statements within transaction boundaries.
Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check nameStatusExplanationResolution
Description checkWarningThe description covers the problem, change, and verification in detail. However, it does not provide the required explicit maintainer approval or a clear approval exemption, and it repeatedly refers t…Correct the migration number to 64 throughout the description. Add the triaged issue or discussion link with the explicit maintainer approval comment, or explain why this focused fix qualifies for an exemption. Align the reported verificati…
Passed Passed checks: 3 passed
Check nameStatusExplanation
Linked Issues checkPassedCheck skipped because no linked issues were found for this pull request.
Out of Scope Changes checkPassedCheck skipped because no linked issues were found for this pull request.
Title checkPassedThe title clearly and concisely describes the main change: using indexed lookups for windowed thread snapshots.
Full details: Description check

Explanation

The description covers the problem, change, and verification in detail. However, it does not provide the required explicit maintainer approval or a clear approval exemption, and it repeatedly refers to migration 65 while the changes add migration 64.

Resolution

Correct the migration number to 64 throughout the description. Add the triaged issue or discussion link with the explicit maintainer approval comment, or explain why this focused fix qualifies for an exemption. Align the reported verification details with the actual migration and test changes.

✨ 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.

JJulius Marminge10d ago

Taking over this one as maintainer. I’ll keep the change focused on indexed snapshot queries, renumber the migration against current main, and verify query behavior and measured performance before updating this branch.

Added 3 commits

  • Merge remote-tracking branch 'origin/main' into perf/long-thread-indexescca04fe
  • perf(server): verify indexed long-thread snapshot readse5d01a0
  • test(server): include snapshot indexes in migration upgrade expectation7771689
JJulius Marminge10d ago

The description warning about migration numbering refers to the old contributor head. Current head 7771689 preserves main’s 064_CanonicalizeThreadPullRequestLinks and adds 065_LongThreadIndexes; the body correctly says migration 65. Current-head CI, including the corrected canonical-link upgrade expectation, passes. Human review of the production SQL and startup migration remains required; no approval exemption is claimed.

Added 4 commits

  • Merge remote-tracking branch 'origin/main' into pr-2119318377f8
  • Merge remote-tracking branch 'origin/main' into pr-211939145c6f
  • test(server): count the renumbered long-thread index migration24d1697
  • Merge remote-tracking branch 'origin/main' into pr-21193184fc44