Uh oh!
There was an error while loading. Please reload this page.
perf(server): stop reading a thread's whole activity timeline per event - #6613
perf(server): stop reading a thread's whole activity timeline per event#6613patroza wants to merge 1 commit into
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ApprovabilityVerdict: Approved 845bcf4 Performance optimization that adds a filtered database query method to avoid loading entire thread activity timelines. The change is self-contained with comprehensive test coverage, and doesn't alter runtime behavior beyond improved efficiency. You can customize Macroscope's approvability policy. Learn more. |
`refreshThreadShellSummary` runs on every event in a thread and loads every activity row that thread has produced — payloads included — to compute one integer, `pendingUserInputCount`. Those payloads are the tool timeline, so the cost of each event scales with the thread's entire history. On a heavily-used instance the busiest thread carries 10,652 activity rows totalling 467 MB, and none of them are rows the count derives from: across that whole database, 5.01 GB of activity payloads reduce to 13 rows (10 KB) carrying a user-input request id. `derivePendingUserInputCountFromActivities` only reacts to three kinds — `user-input.requested`, `user-input.resolved` and `provider.user-input.respond.failed` — and skips everything else, so the read now filters on exactly those. Measured against that database, the heaviest thread goes from 349 ms and 467 MB to 6.9 ms and no rows, before the schema decode and sort that follow. The repository gains `listByThreadIdAndKinds`, which keeps the ordering and row decoding of `listByThreadId` and short-circuits an empty kind list without issuing a query. Co-authored-by: Patrick Roza <42661+patroza@users.noreply.github.com>
4d6d0bd to
845bcf4CompareDismissing prior approval to re-evaluate 845bcf4
## Summary The PR panel mapped every unclassified `gh` exit to **GitHub CLI command failed.**, so the real guest-wrapper reason never reached the UI. That is what `pingdotgg#6613` (`pingdotgg/t3code`) showed this time. The live t3vm image already has ops #80 (host-qualified `--repo`). The product `#373` shim fix is still on `fork/dev`. App-only mint dies with: ``` t3-github-app-token: app is not installed on pingdotgg/t3code (or repo does not exist) ``` The App is installed on `patroza` / `macs-holding` / `effect-app` / `aaaomega` — not `pingdotgg`. This change passes through guest wrapper lines (`t3-github-app-token:` / `gh-app-wrapper:`) as the command-failed detail, and keeps raw provider stderr off the VCS error message (tokens stay out of logs). Installing the App on `pingdotgg` (or using a user/SSH token for those reads) is still required for the panel to actually load that PR. ## Test plan - [x] `vp test run apps/server/src/vcs/VcsProcess.test.ts apps/server/src/sourceControl/GitHubCli.test.ts` - [ ] After deploy: open a PR the App is not installed on and confirm the panel shows `app is not installed on …` instead of the generic CLI line - [ ] Confirm a `patroza/t3code` PR still loads on t3vm Co-authored-by: T3 Code PR Stack <41898282+github-actions[bot]@users.noreply.github.com>
t3dotgg
commented
Sep 1, 2026
Note 🤖 GPT-5.6 Sol responding on behalf of Theo Closing this because its server optimization is already on #9032 records this PR in the related-work credits. |
The problem
refreshThreadShellSummaryloads every activity row a thread has ever produced — payloads included — to compute a single integer,pendingUserInputCount.Activity payloads are the tool timeline, so the cost of one refresh scales with the thread's entire history. Across the 402 threads on a heavily-used instance, comparing what the refresh reads against what the derivation needs:
The "needed" column is zero for 397 of 402 threads, and that is not a quirk of this dataset:
pendingUserInputCountderives only fromuser-input.requested,user-input.resolvedandprovider.user-input.respond.failed, which exist only when a provider stops mid-turn to ask the user something. Even on the 5 threads here that have had one, they account for 2–4 rows out of thousands — 1.3 KB out of 3.5 MB on the largest.So the read is not merely oversized on one unusual thread; it is reading the whole timeline to find a handful of rows that are usually not there at all.
This is already reported
refreshThreadShellSummaryand this exact reload.--max-old-space-size. At the cap the process sits in back-to-back full GCs and every connected client stalls until it is restarted.dainalso observed cross-thread head-of-line blocking: one busy thread delaying unrelated turns for minutes. That matches what this looks like from a client — everything slow, not one screen.How this relates to #5855 and #6608
Both of those reduce how often the refresh runs. This PR reduces what it costs when it runs. They compose; none of them replaces another:
Neither #5855 nor #6608 changes the read itself — both still call
projectionThreadActivityRepository.listByThreadId({ threadId })and pull the full payload set. With either merged, every lifecycle event (user-input.requested/resolved, approval events, proposed-plan upserts,thread.session-set,thread.turn-diff-completed) still pays the whole-thread read. On the instance above that is up to 467 MB per event, for one integer.Merging this alongside them means the refreshes that remain are also cheap. If the maintainers prefer #6608's shape, this still applies unchanged on top of it — the two touch different lines.
The change
derivePendingUserInputCountFromActivitiesonly reacts to three kinds —user-input.requested,user-input.resolvedandprovider.user-input.respond.failed— andcontinues past everything else. The read now filters on exactly those three, with the kinds declared next to the deriver so the two stay in step.Measured against the same database. The busiest thread, warm, before schema decode and the sort that follows:
And on the threads where the filtered read is not empty — the fix's worst case:
Deployed, the traced projection step moved:
94% of orchestration events there reach this path (
thread.activity-appendedalone), averaging ~356/hour and peaking at 2,613 in one hour.ProjectionThreadActivityRepositorygainslistByThreadIdAndKinds, which reuses the row decoding oflistByThreadId, keeps its exact ordering, and short-circuits an empty kind list without issuing a query.listByThreadIdis unchanged and still used everywhere else.No behaviour change: the rows removed from the read are ones the deriver already skipped.
Why it went unnoticed for so long
The code landed in #1973 (2026-04-13) and has not been touched in the 1,239 commits since. It only bites at the tail — on the instance measured, the median thread holds 306 activity rows (~2.8 MB), which is unnoticeable; the p99 holds 4,279 rows / 123 MB. It needs long-lived threads and a server process that stays up for days, which is not the common desktop profile.
Adjacent fixes have repeatedly addressed the same underlying volume on the serve path — #4622 pruning activity payloads over the wire, #4788 gzipping snapshots, #5482 dropping MCP tool results from thread payloads, #5147 bounding catch-up replay. This is the same volume problem on the projection path.
Tests
Five repository tests cover kind filtering, ordering parity with the unfiltered list, payload-decoding parity, thread isolation, and the empty-kinds short circuit.
One
ProjectionPipelinetest is added, because the existing suite could not catch the failure mode this change introduces. The suite assertedpendingUserInputCountonly where it settles back to0, so a filter that dropped every row would still have passed. The new test appends auser-input.requestedactivity alongside unrelated tool noise and asserts the count is 1.Verified by mutation: renaming the filtered kinds makes only the new test fail — the other 22 in that file still pass.
Equivalence was also checked against real data. Replaying the deriver over all 402 threads on the instance above, once from the full activity list and once from the filtered list, produced identical counts for every thread.
Not a UI change, so no screenshots.