Skip to content

perf(server): PR discovery no longer reads thread history - #25443

Open
only21mil wants to merge 2 commits into
pingdotgg:mainfrom
only21mil:fix/r081-perf-server-keep-pr-discovery-from-sca
Open

only21mil wants to merge 2 commits into
pingdotgg:mainfrom
only21mil:fix/r081-perf-server-keep-pr-discovery-from-sca

Conversation

@only21mil

@only21mil only21mil commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Pull request discovery runs at startup, on the one-minute sweep while a client is connected, and after each finished turn. Each run builds full thread shells. A shell read counts tool items, looks up run errors and pending work, and walks fork sources. Discovery uses none of that. It needs the project, branch, worktree, saved PR links, and settled or archived state.

On large installs, discovery is the longest database holder, and this read is the part of it that runs inside a transaction. In #18618, a 31 GB install with about 2,790 threads saw ThreadPullRequestServiceV2.synchronize run for a median of 5.5 s and up to 239.7 s. Its longest reader transaction ran 120.7 s, and a reader held that long keeps the WAL pinned (#23482).

Change

  • ProjectionStoreV2.getPullRequestDiscoverySnapshot reads thread rows and the thread event high-water mark in one transaction, then decodes them after the transaction ends. It touches no run, item, message, plan or provider table. The SQL and in-memory stores both implement it.
  • ThreadPullRequestServiceV2 uses it for the sweep and one-thread refreshes. Kept as before: active threads only, unsettled only unless a backfill is pending, archived threads still readable by id, startup backfill, and the thread.pull-request.sync sequence guard.
  • Legacy payloads that store a single linkedPullRequest still read as one link, as threadPullRequestsOf does for shells. Without that, discovery could replace a legacy link that shells treat as already linked.
  • A one-thread refresh used to read the thread's event sequence and then its shell in two separate reads. It now gets the thread event high-water mark from the same transaction as the row, the value the sweep already uses. The guard still rejects any thread event committed after the read, because that event's sequence is higher.

I passed on a metadata-only mode for getShellSnapshot: the shell builder joins history in its main query and fork walk, so a separate small read is simpler.

Scope and approval

Issue #18618. The triage reply on Julius's behalf (comment) kept it open as an enhancement: "A metadata-only read is a reasonable, cheap follow-up that fits alongside #14703." The large-install numbers came later in that thread.

Maintainers to confirm: whether a performance change of this size fits current intake. This version is server-only, with no contract or client changes.

The backfill window in #21620 applies unchanged, since it reads settledAt and updatedAt from the same thread rows. Only its test mock would move from getShellSnapshot to the new read.

Verification

  • vp test run on ThreadPullRequestService.test.ts, ProjectionSettlement.test.ts, ProviderTurnControlService.test.ts and ProjectionStore.test.ts: 64 passed.
  • New service test: real SQLite projection store, one branch thread, a thread event at sequence 42, and every projection table except threads dropped. Discovery must dispatch the PR sync with snapshotSequence: 42.
    • On the base commit, with the orchestrator shell reads routed to that store, it fails with no command dispatched.
    • On the base commit, with the tables kept, it passes. So the failure comes from the history reads.
  • New store test, SQL and memory: unsettled, active and archived scopes, plus legacy link normalization. With the normalization removed, both variants fail.
  • Equivalence and timing, with a throwaway script on an in-memory SQLite projection of 500 branch threads, each with one run and 100 completed tool items (mixed worktrees, legacy links, explicit links, settled and archived):
    • Discovery fields (id, project, branch, worktree, PR links, archived and settled state) match the shell reads exactly: 0 mismatches across both sweep scopes and all 500 one-thread reads.
    • Median of 9 warm reads on the same machine and run: sweep 15.79 ms with shells, 4.32 ms with the new read. One-thread refresh: 0.36 ms vs 0.03 ms.
  • vp run typecheck in apps/server: passes. vp fmt --check and vp lint on the changed files: clean.

Not checked:

  • A large real install. The fixture numbers above are the read alone, not GitHub calls or whole-app CPU.
  • UI. No screen changes, because discovery dispatches the same sync commands from the same fields.

Prior PRs

Generated with Claude Opus 5.5 in T3 Code Nightly

only21mil and others added 2 commits October 26, 2026 04:52
Discovery should find a branch pull request from thread rows alone.
On the base, the sweep builds thread shells, which read run and item
history, so it fails once those tables are gone.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
PR discovery built full thread shells, which count tool items, look
up run errors and pending work, and walk fork sources. It uses none
of that. It needs the thread's project, branch, worktree, saved PR
links and settled or archived state.

Add a projection read that returns the thread rows and the thread
event high-water mark in one transaction, then decodes after it
ends. Legacy single-link payloads still read as one link, as they do
in shells. The sweep keeps its active and unsettled filters, startup
backfill and sequence guard; a one-thread refresh still reads that
thread wherever it lives.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions github-actions Bot added vouch:unvouched PR author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Oct 26, 2026
@macroscopeapp

macroscopeapp Bot commented Oct 26, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Needs Human Approval

Macroscope's review found this PR requires human approval — This PR changes a high-frequency production discovery path and introduces a new projection-store snapshot implementation, including different transaction and sequence handling. The scope is focused and tested, but the runtime and shared-infrastructure impact warrants human review.

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

@coderabbitai

coderabbitai Bot commented Oct 26, 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 3433ba1 and f0f1ce4.

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

Pull-request discovery now reads thread snapshots from ProjectionStore. SQLite and in-memory stores provide matching selection behavior. ThreadPullRequestServiceV2 uses these snapshots to identify threads and dispatch pull-request sync commands.

Changes: Pull-request discovery

Layer / File(s) Summary
Discovery snapshot contract and reads
apps/server/src/orchestration-v2/ProjectionStore.ts, apps/server/src/orchestration-v2/ProjectionSettlement.test.ts
ProjectionStore adds a snapshot result containing the event sequence and thread rows. SQLite and in-memory implementations apply active-thread, unsettled, and specific-thread selection. Tests cover these scopes and legacy pull-request normalization.
Service snapshot integration and regression coverage
apps/server/src/orchestration-v2/ThreadPullRequestService.ts, apps/server/src/orchestration-v2/ThreadPullRequestService.test.ts, apps/server/src/orchestration-v2/ProviderTurnControlService.test.ts, apps/server/src/server.ts
ThreadPullRequestServiceV2 reads discovery snapshots from ProjectionStore, and the server provides that layer to the worker. Tests cover snapshot-based discovery, including a case where orchestration projection tables other than the thread table are absent.

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

Change: Refactor

Possibly related issues

  • pingdotgg/t3code#24660 — The change removes the full shell read from pull-request discovery, but does not add reader concurrency or change ensureTranscript caching.
  • pingdotgg/t3code#24895 — The service now uses getPullRequestDiscoverySnapshot to read thread metadata during discovery instead of the prior shell snapshot path.

Suggested reviewers: t3dotgg

Merge Risk: Minimal · up to f0f1c

PR discovery can use the metadata-only snapshots without an identified sync-sequence regression. No merge-blocking issue remains after normal checks.

Pre-merge checks: Passed 4
Check name Status Explanation
Title check Passed The title clearly and concisely describes the main performance change: PR discovery no longer reads full thread history.
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 includes all required sections and provides detailed problem context, implementation scope, verification results, limitations, and related issue information. The Scope and approval sec…
Full details: Description check

Explanation

The description includes all required sections and provides detailed problem context, implementation scope, verification results, limitations, and related issue information. The Scope and approval section cites issue #18618 and a triage comment, but it also states that maintainer confirmation is still pending.

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

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:M 30-99 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