Skip to content

fix(client-runtime): stop query polling after the last reader leaves - #25266

Open
saphid wants to merge 4 commits into
pingdotgg:mainfrom
saphid:fix/idle-query-polling-20261026
Open

saphid wants to merge 4 commits into
pingdotgg:mainfrom
saphid:fix/idle-query-polling-20261026

Conversation

@saphid

@saphid saphid commented Oct 26, 2026 •

Copy link
Copy Markdown
Contributor

Problem

Shared query caches retain recurring polling after the final reader leaves. For example, navigating away from a mobile PR detail can leave linked-thread refreshes running during the five-minute cache lifetime, although the screen has no reader.

Change

Keep cached answers and signal revisions together, while disposable outer wrappers own mount revalidation and recurring polling. Preserve synchronous cached reads on reopen, configured TTLs, visible intervals, manual refresh, final answers and session cancellation. Distinguish a source rebuild during a read from a stale cached answer so synchronous signal/session refreshes execute once on remount. An already-started request may still finish while idle under its existing TTL policy.

Scope and approval

Upstream maintainer approval of direction and scope is pending; this PR is not ready to merge. Macroscope reconfirmed “Needs Human Approval” at 9906abb because this changes shared cache, invalidation, revalidation and polling ownership, and adds a line-level globalDate diagnostic suppression. No explicit maintainer approval link is available. Green CI and verification comments do not supply that approval.

The recurring subscription belongs to the shared query factory; a mobile focus callback cannot release it. The same ownership correction therefore serves mobile, web and desktop query consumers. No UI, setting, RPC contract or transport change is included. Audit tracker: #189.

Verification

Integrated upstream dac8b11. At the current head:

  • Independent review passed CI=true vp test run packages/client-runtime/src/state/runtime.test.ts packages/client-runtime/src/state/assets.test.ts packages/client-runtime/src/state/pullRequests.test.ts: 107 tests passed, with no actionable source findings.
  • CI=true vp run check:changed dac8b11ddc25bd5da62a367fdb6c3de777ae2986 passed after the integration commit.
  • All eight required checks pass; the latest notification reports all 33 checks passed. GitHub reports CLEAN/MERGEABLE. CodeRabbit found no actionable source issues; its remaining warning requests the maintainer approval above.

Real query-factory/AtomRegistry regressions use controlled timers: after final-reader removal, the quiet-signal baseline executes three extra query operations over three intervals; the candidate executes zero. Coverage includes interval-only and asset queries, multiple readers, cached reopen/revalidation, manual refresh, final results, expiry, and signal/session replacement. These are fixture query-operation counts, not wire-traffic or energy measurements.

Historical evidence: the earlier iOS navigation/display smoke check preserved PR-detail back/reopen behavior, but was not repeated after this integration. Maria’s verification report describes real web request-count and reopen/manual-refresh checks at the prior a64ca2a head; those observations and media were not independently replayed here. Current-head native iOS/Android, Electron, and remote/relay/tunnel compatibility remain unverified. The supported device host is unavailable. Physical battery impact and Hermes performance are unmeasured.

Implementation and independent review: separate GPT-6.1 Sol/high sessions through Codex in T3 Code.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or 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 — The PR changes shared client-runtime cache, invalidation, revalidation, and polling ownership used by multiple query families, making its production blast radius broader than a small bug fix. It also adds a line-level static-analysis diagnostic suppression in the runtime implementation.

No code changes detected at 9906abb. Prior analysis still applies.

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 →

Important

Review skipped

Review was skipped as selected files did not have any reviewable changes.

Run configuration
Commits

Reviewing files that changed from the base of the PR and between 9906abb and ae264c9.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • Trigger review

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 a64ca2a and 9906abb.

Files selected for processing: 2
  • packages/client-runtime/src/state/runtime.test.ts
  • packages/client-runtime/src/state/runtime.ts

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

Walkthrough

The runtime separates cached query state from live revalidation. Mount reads can refresh stale or unsuccessful non-final results. Periodic and signal-triggered refreshes follow reader and result state. Tests cover cache retention and expiry, polling, final results, and session replacement.

Changes: Query Refresh and Cache Lifecycle

Layer / File(s) Summary
Cached query and signal refresh
packages/client-runtime/src/state/runtime.ts, packages/client-runtime/src/state/runtime.test.ts
The cached query retains idle TTL and applies refresh-trigger handling. Signal changes refresh only when a prior non-final result exists. Tests cover idle signal retention and cache expiry.
Live revalidation and polling
packages/client-runtime/src/state/runtime.ts, packages/client-runtime/src/state/runtime.test.ts
A live wrapper revalidates stale or unsuccessful non-final results, and periodic refresh runs while readers are mounted. Tests cover revalidation, polling, final results, and session replacement.

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

Change: Bug fix

Suggested reviewers: t3dotgg, juliusmarminge

Merge Risk: Minimal · previous review 9906a

Visible polling still reaches the query, and no actionable merge-blocking issue remains after normal checks.

Pre-merge checks: Passed 3 · Failed 1

Failed Failed checks: 1 warning

Check name Status Explanation Resolution
Description check Warning The description includes the required Problem, Change, Scope and approval, and Verification sections. It provides detailed behavior, scope, tests, results, and unverified areas. However, the Scope and… Obtain explicit maintainer approval for the shared cache, invalidation, revalidation, polling ownership, and diagnostic suppression changes. Add a link to the approval discussion and the approval comment to the Scope and approval section be…
Passed Passed checks: 3 passed
Check name Status Explanation
Title check Passed The title clearly and concisely describes the primary change: stopping query polling after the final reader leaves.
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.
Full details: Description check

Explanation

The description includes the required Problem, Change, Scope and approval, and Verification sections. It provides detailed behavior, scope, tests, results, and unverified areas. However, the Scope and approval section explicitly states that upstream maintainer approval is pending and provides no approval comment or qualifying exemption.

Resolution

Obtain explicit maintainer approval for the shared cache, invalidation, revalidation, polling ownership, and diagnostic suppression changes. Add a link to the approval discussion and the approval comment to the Scope and approval section before merging.

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

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

thanks @saphid. separating the cached answer and signal revision from the live polling subscription fixes the ownership bug while preserving cached reopen behavior. i kept your implementation and all regression coverage, and merged current main (323d9611ff49) without rebasing. head: a64ca2ae9f55423eb78d6f104f51119082bf8a5a.

two independent shared-query reviews found no correctness blockers or safe substantial simplification. the build counter prevents duplicate synchronous revalidation; replacing the tracked cache with stock swr would retain source subscriptions. the diff remains two files, +438/-24, with no changes to your runtime patch.

verification:

  • reproduced the bug on current main with the existing-file regression cases before restoring the fix; all 47 runtime tests then passed.
  • blacksmith vp run check:changed passed format, lint, both knip passes, seven affected-package typechecks, and all 47 tests.
  • real web client, system chrome, live git.cafe provider: open this pr, leave via back for 30 seconds, then reopen. websocket requests for pullRequests.linkedThreads continued three times offscreen on main and zero times with the fix. both polled while visible; the fixed query refreshed and resumed polling on reopen. this is request-count verification, not a battery or latency benchmark.
  • all 29 ci checks are green. the automatic review found no actionable findings; its latest pass skipped the unchanged source. both independent source reviews approve this head.

unverified: native ios/android, electron, remote/relay/tunnel runtime, physical battery impact. an already-started request may complete under the retained cache lifetime. maintainer approval of the shared-query lifetime change remains for maria's review, as the approvability comment requests.

matching full-window captures show the same live pr, with no intended appearance change.

before: current main displaying pr 25266

after: the fixed client displaying the same pr

fixed client: leave pr detail and reopen it

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

Verified exact head a64ca2ae9f55423eb78d6f104f51119082bf8a5a as-is on a disposable Linux VM. No source changes or pushes were needed.

Started github/main (d2429f733c) in a separate worktree, stopped it, then started this exact PR head. Both used vp i, a background vp run dev, isolated state, repository Playwright and system Google Chrome. Added a real git project through the UI and completed real Codex gpt-6.1-sol turns through the web composer on both versions. No mocks, fixtures, demo data, or accelerated timers.

Observed actual pullRequests.linkedThreads WebSocket requests for this live GitCafe PR:

  • Main: 3 requests during 31 seconds visible; 3 more during 31 seconds after closing the PR panel.
  • PR: 3 requests during 31 seconds visible; 0 during 31 seconds after closing.
  • A second same-client close/reopen cycle also produced 0 idle requests. Clicking the sidebar PR badge reopened the panel, sent one immediate refresh and resumed polling 10 seconds later.
  • Nearby behavior: manual PR refresh worked; linking the real Codex thread to this PR updated the sidebar badge and the actual linked-threads RPC response. The same sidebar link reopened the PR successfully.

These are real request counts, not battery or latency measurements. The recording preserves the full idle interval and shows elapsed time.

Blacksmith verification on this head passed: 132 focused tests across runtime, assets, pullRequests, fileMetadata, threadFamilies and olderSettledThreads; vp run check:changed 323d9611ff49521357fdc205f667614602ec46b6 passed format, lint, both knip passes, seven package typechecks and 47 runtime tests. CI reports 29/29 successful checks.

Command and request transcript

before: main displaying live PR 25266

after: exact PR head displaying the same live PR

exact PR head: close for 31 seconds, reopen through the sidebar, and resume polling

Not verified

Native iOS/Android, Electron, remote/relay/tunnel connections, providers other than Codex, physical battery impact and Hermes performance were not exercised on this Linux web-client pass. Already-started requests may still finish under the existing cache policy. The separate maintainer scope approval mentioned in the PR description remains pending.

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:trusted PR author is trusted by repo permissions or the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants