Skip to content

fix(server): a finished turn reads its pull request from GitHub once - #33762

Closed
Silthus wants to merge 3 commits into
pingdotgg:mainfrom
Silthus:fix/turn-end-one-pr-lookup
Closed

Silthus wants to merge 3 commits into
pingdotgg:mainfrom
Silthus:fix/turn-end-one-pr-lookup

Conversation

@Silthus

@Silthus Silthus commented Nov 10, 2026 •

Copy link
Copy Markdown

Problem

One finished agent turn reads its branch's pull request from GitHub up to four times, about a second apart, for the same branch and head. A rollback reads it once per stale checkpoint. Every T3 server a user runs shares their per-user GitHub budget with their own gh and every agent, so servers with many short turns burn through it faster.

Closes #33744

Change

Checkpoint capture commits a turn end as one batch: up to three checkpoint.captured events (thread start, baseline, the run's own) and the run's terminal run.updated. ThreadPullRequestService queued a refresh: true lookup for each of them, and each refresh skips the branch lookup cache, so each one was its own GraphQL call.

Now the terminal run.updated is the only finished-turn trigger, checked with the shared isTerminalRunStatus. That adds rolled_back, which rollback commits together with its stale checkpoints, so a rollback still refreshes once. checkpoint.captured no longer forces a lookup. The one-minute sweep and the cache lengths are unchanged.

Commit GitHub reads before After
A first turn (thread start, baseline, run checkpoint, run completed) 4 1
A follow-up turn (run checkpoint, run completed) 2 1
Rolling back one turn with two stale checkpoints 2 1

A rollback reads once per rolled-back run, because rollback commits one terminal run.updated per run. Rollbacks are rare and user-started, so they get no extra coalescing. A stopped turn still reads twice: its run.updated (cancelled or interrupted) lands once when it stops and again when its checkpoint is captured.

Scope and approval

A very small, focused fix of an obvious bug: one finished turn should read its pull request once. It changes which orchestration events trigger PR discovery in one service, plus its tests. It leaves the deliberate one-minute sweep and PR cache lengths from #21372 alone. The larger share of background PR reads on servers with many subagent threads is #19657, which #19665 addresses; this PR doesn't overlap with it.

Merge risk: low, two-way door. Capture and rollback commit every checkpoint together with its run's terminal update, so no turn end loses its refresh; the worst case is a badge that waits for the one-minute sweep.

Verification

CI passed on e1dc23c and is pending on 1df3b55, which changes only the test file.

Live run. I ran Replay (dev) turns against an isolated dev server (vp run dev --no-seed, its own .t3) on a thread in a worktree on this PR's branch. Then I counted listPullRequestsByHead GraphQL calls under ThreadPullRequestServiceV2.synchronize in server.trace.ndjson after each turn ended:

Same follow-up turn on the same thread listPullRequestsByHead after the turn ends
Base 4c36119 2 (+18.2s, +19.3s; the turn ended at +12.2s)
This PR 1 (+27.5s; the turn ended at +13.6s)

On this PR, the thread's first turn also read once after it ended, from Orchestrator.handleRunUpdate. The three reads during thread creation and worktree setup, before the turn starts, come from thread.created and thread.metadata-updated. This PR leaves them alone.

At 1df3b55 I then ran one more turn and rolled it back on the same thread, counting GraphQL calls between the domain events in the dev database:

Commit on this PR listPullRequestsByHead
Turn end: two checkpoint.captured, then run.updated completed 1
Rollback: stale checkpoint.captured, then run.updated rolled_back 1

The rollback kept the files ("Revert and keep changes"), because a shared worktree refuses a file restore. Its thread.metadata-updated pass was answered from the lookup cache without a request.

Tests.

  • New test, reads the branch pull request from the host once for $commit in ThreadPullRequestService.test.ts. It runs the real ThreadPullRequestService, GitManager, source control registry and GitHub driver over a temp git repository whose origin names GitHub. Only the network and the gh CLI are fakes: an in-memory GitHub answers the GraphQL lookups and records every request it receives. The test delivers a finished turn's commit and a rollback's commit after the startup sweep has drained, and asserts that GitHub received one request and the thread stays linked to the pull request. On main GitHub receives 4 and 2 requests; with the fix, 1 each.
  • The two existing tests that used checkpoint.captured as the turn-end trigger now use the terminal run.updated.
  • vp test run src/orchestration-v2/ThreadPullRequestService.test.ts: 13 passed.
  • vp run check:changed: format, lint, knip, graphql, typecheck (3 packages) and tests pass.

Decisions

Question Answer Who Where
Which events refresh a finished turn's pull request? Only the terminal run.updated. This replaces the per-thread coalescing proposed in #33744, which depended on worker scheduling 🤖 Agent comment
Add ETag conditional requests to the repeated check reads? No, they already send If-None-Match 🤖 Agent comment
Also cut the settlement sweep's pull request reads here? No, #19665 owns them 🤖 Agent comment
Which seam does the test drive? Requests an in-memory GitHub receives, through the real GitManager, registry and GitHub driver. This replaces counting lookups at a stubbed GitManager 🤖 Agent comment

Involvement: no grilling rounds, a design review and three code review rounds by GPT-6.1 Sol, live-run proof captured.

Created with Claude Opus 5.5 in Claude Code, running in T3 Code.

🤖 Generated with Claude Code

Checkpoint capture commits a turn end as one batch: up to three
checkpoint.captured events and the run's terminal run.updated. Thread PR
discovery forced a fresh GitHub lookup for each of them, so one turn end
cost up to four GraphQL calls for the same branch and head. A rollback
cost one per stale checkpoint.

The terminal run update is now the only finished-turn trigger. It also
covers rollbacks through rolled_back, which they commit with their
checkpoints.

Closes pingdotgg#33744

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:S 10-29 changed lines (additions + deletions). labels Nov 10, 2026
@Silthus

Silthus commented Nov 10, 2026 •

Copy link
Copy Markdown
Author

Note

🤖 Claude Opus 5.5 responding on behalf of Michael

Decisions made while building this PR:

Silthus and others added 2 commits November 10, 2026 09:22
The turn-end test now drives the real GitManager over a local repository and
counts the pull request list requests a fake GitHub host receives, then checks
the thread stays linked to the discovered pull request.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
The regression test now runs the real GitHub driver and source control registry
behind the real GitManager. Only the network and the gh CLI are fakes: an
in-memory GitHub answers each GraphQL request and records it. On main a finished
turn sends GitHub 4 requests and a rollback 2; with the fix each sends 1.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@Silthus Silthus closed this Nov 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:S 10-29 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.

[Bug]: One finished turn reads its branch's pull request from GitHub up to four times

1 participant