Sign up
View stargazersSign up

fix(server): PR sync stops reading missing pull requests every minute#17800

open7
main
GitHub fork: fix/pr-sync-not-found-backoff
opened24d agobyAabiassi
2 files changed
View on GitHubTwo-way sync enabledPR last imported 7d ago

Missing pull requests were read on every one-minute sync sweep, including links without snapshots on settled threads. This consumes quota without making progress. Fixes #17799.

A not-found response now starts the existing fifteen-minute retry cadence for active threads. Settled links no longer trigger periodic reads. Sibling syncs, link updates, and pending stack retries respect that backoff; an explicit reader refresh still reads immediately. A successful response restores normal polling. Current main's missing-merge-time recovery and refresh-generation handling are retained.

Maintainer takeover of @abiassi's focused fix in #17800. Contributor history is preserved, with main merged rather than rebased. The original implementation and tests were rebuilt against current main; coverage was consolidated into four new tests and an extension to the settled-thread test.

Verification on headless Linux:

  • Main baseline: 20 tests pass in PullRequestSyncReactor.test.ts.
  • Rebuilt regression suite on unchanged main: 19 pass, 4 fail. The failures show 16 settled-link reads instead of zero, 30 missing-link reads instead of two before backoff expires, event-triggered extra reads, and stack retries bypassing backoff.
  • With the fix: all 24 tests pass in that same file. The real reactor is exercised with injected host responses and TestClock, including fifteen-minute retries, recovery, explicit refresh, and both link event paths.
  • Server-only typecheck, targeted lint, formatting, and whitespace checks pass. Lint retains two existing unused-parameter warnings in the test harness.
  • Independent Opus/Sol full-diff review round 1 found no blockers. Macroscope subsequently found that changing the representative project could inherit its predecessor’s missing pause. A new regression failed before the correction, and the missing state now resets when the reading project changes. All 24 tests and the server typecheck pass. Both reviewers found no blockers in full-diff round 2 at b41599d. Current-head checks that ran pass, including Macroscope correctness and approvability; convention and label-definition jobs skipped. Macroscope marks its earlier finding obsolete. The PR remains unmerged for maintainer review.

A PR that initially returns not-found can stay without a snapshot for fifteen minutes; explicit reader refresh still bypasses this. Settled-only links also stop background stack retries until explicitly refreshed.

No visual UI changes; screenshots do not prove read cadence. No live GitHub quota was exhausted and no production server was used. Live-provider behavior, macOS, Windows, iOS, Android, and Electron desktop were not exercised in this takeover. No wire contracts or client entry points change.

Tests: Codex GPT-6-Astra (high). Implementation: Codex GPT-6.1-Sol (high). Review: Claude Opus 5.5 and Codex GPT-6-Sol. Maintainer integration: GPT-6.1-Sol through the Codex harness in T3 Code.

Checks

22 successful · 15 skipped
All checks passed
b41599d

Added 2 commits

  • fix(server): PR sync stops reading missing pull requests every minute58fd275
  • test(server): show PR sync re-reading pull requests the host cannot findf3821b7
MmacroscopeappBot24d ago
Synced from GitHub

Approvability

Verdict: Approved at b41599d

Macroscope's review found this PR approvable — This is a contained server-side bug fix that throttles repeated not-found pull-request reads while preserving explicit refresh behavior and cross-project synchronization. The accompanying tests cover the new cadence, recovery, event handling, and retry behavior without changing schemas, defaults, or deployment configuration.

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

CcoderabbitaiBot24d ago
Synced from GitHub

Review in Change Stack →

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: 572b4aaf-c4f1-4f57-b2b8-111ba0f52c2e
📥 Commits

Reviewing files that changed from the base of the PR and between 58fd2755d198096b02b42149e2dfe80f6010f156 and a294da720e52e36826302f9d563b50c5f0073692.

📒 Files selected for processing: 1
  • apps/server/src/orchestration-v2/PullRequestSyncReactor.test.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 pull-request sync reactor now bases automatic reads on thread and pull-request state. It tracks pull requests that return not-found, retries them on a 15-minute cadence, and limits event-triggered refreshes for known-missing links.

Changes: Pull-request sync polling

Layer / File(s)Summary
Due checks and missing-request cadence
apps/server/src/orchestration-v2/PullRequestSyncReactor.ts, apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts		Automatic sync eligibility now considers active links and their snapshots. Missing pull requests use a 15-minute cadence. A not-found summary records the missing key and advances its sync timestamp; a successful summary clears the marker. Tests cover host failures, missing-request retries, and stack-read failures.
Event-triggered refreshes
apps/server/src/orchestration-v2/PullRequestSyncReactor.ts, apps/server/src/orchestration-v2/PullRequestSyncReactor.test.ts		A synced-thread event requests a refresh for an unsynced link only when its key is not marked missing. Tests cover settled-thread links, sibling-link syncs, and explicit state-change requests.

Priority: ➖ Normal

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

Change: Bug fix · Severity of issue fixed: Medium

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to a294d

The change reduces repeated reads of pull requests that return not-found and keeps the 15-minute retry for unsettled threads. No concrete merge-blocking risk was identified.

Security architecture risk: 🔵 Low · up to a294d

Background reads are reduced without adding access paths or permissions, and reader-triggered refresh remains available. The main uncertainty is retry behavior across restarts and projects using different credentials.

Retained concerns No architecture-level concerns identified.

Security review details

Security Blast Radius

  • inferred — The changed behavior affects refresh timing for projected links sharing a pull-request identity. It does not add a caller, enlarge provider-read authority, or introduce a new event ingress in the compared changes.

Trust Boundaries and Controls

  • observed — Link references still pass through existing project and repository routing before provider reads, and credential-aware service wrapping remains unchanged. The new scheduling decisions do not replace these controls.

Resilience and Maintainability Implications

  • observed — Known-missing links no longer generate immediate refreshes merely because another link synced. Reader-triggered refresh remains available, while existing project/host rate-limit pauses continue to gate reads.
Pre-merge checks: Passed 4
Check nameStatusExplanation
Linked Issues checkPassedIssue #17799 is an active direct link. isDue now filters settled threads before no-snapshot checks. A not-found summary records missing, advances lastSyncedAt, and clears the pending request. …
Out of Scope Changes checkPassedThe diff changes only PullRequestSyncReactor.ts and its tests. The reactor changes directly implement issue #17799. The tests provide regression coverage for the required polling behavior. No unrela…
Title checkPassedThe title clearly describes the primary fix: preventing missing pull requests from being read on every one-minute sync.
Description checkPassedThe description explains the problem, the implementation, scope and approval context, verification results, limitations, and test or review evidence. It does not use the template headings, but it cont…
Full details: Linked Issues check

Explanation

Issue #17799 is an active direct link. isDue now filters settled threads before no-snapshot checks. A not-found summary records missing, advances lastSyncedAt, and clears the pending request. Unsettled links retry at the 15-minute cadence. Successful reads clear missing. Automatic stack retries and sibling-link syncs do not bypass the cadence. Explicit reader requests remain due immediately. The tests cover these cases, including settled links without snapshots and reader refresh.

Full details: Out of Scope Changes check

Explanation

The diff changes only PullRequestSyncReactor.ts and its tests. The reactor changes directly implement issue #17799. The tests provide regression coverage for the required polling behavior. No unrelated setting, contract, UI, or product behavior change is shown.

Full details: Description check

Explanation

The description explains the problem, the implementation, scope and approval context, verification results, limitations, and test or review evidence. It does not use the template headings, but it contains the required information and is sufficiently complete.

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

Added 1 commit

  • Merge branch 'main' into fix/pr-sync-not-found-backoffa294da7
JJulius Marminge15d ago

Taking over this one for batch pull-request-09. I’ll keep the focused backoff behavior, reduce the regression coverage to the cases that matter, merge current main, and verify the link-update and explicit-refresh paths before publishing. This remains unmerged.

Added 3 commits

  • chore: merge main before rebuilding PR sync backoff38a83fb
  • test(server): cover missing PR cadence and refresh triggers67a7e77
  • fix(server): back off missing pull request sync readsb8f7c4e
apps/server/src/orchestration-v2/PullRequestSyncReactor.tsline 299Outdated · b8f7c4eOpen thread
……@@ -287,7 +292,19 @@ export const make = Effect.gen(function* () {
296+ Effect.tapError((error) =>
297+ error.reason === "not-found"
298+ ? Effect.sync(() => {
299+ missing.add(key);
MmacroscopeappBoton line 29915d ago

🟡 Medium orchestration-v2/PullRequestSyncReactor.ts:299

A not-found from one project's credentials delays syncing the same PR through another project's credentials for the 15-minute backoff, even after the first project's link is removed. missing is keyed only by threadPullRequestKeyOf, while syncGroup reads through the first entry's projectId, so isDue treats the other project's link as missing too. Scope missing by project as well as PR key, or clear it when the representative project changes.

No longer relevant as of b41599d4928fff7cf4852c5ca68f54f49d97a968

JJulius Marminge15d ago

Fixed in b41599d. Missing state now retains the reading project and drops both its mark and cadence timestamp when the representative project changes. The event path also permits that changed representative to read immediately. A focused regression failed with only project A read before the fix; all 24 reactor tests now pass, with server typecheck and targeted lint/format passing.

Added 1 commit

  • fix(server): clear missing PR backoff when reading project changesb41599d