Skip to content

fix(connections): avoid provider refresh storms on reconnect - #11456

Open
Bil0000 wants to merge 1 commit into
pingdotgg:mainfrom
Bil0000:fix/reconnect-provider-refresh
Open

fix(connections): avoid provider refresh storms on reconnect#11456
Bil0000 wants to merge 1 commit into
pingdotgg:mainfrom
Bil0000:fix/reconnect-provider-refresh

Conversation

@Bil0000

@Bil0000 Bil0000 commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Each reconnect started a full provider refresh, adding CPU load and repeated catalog updates. Slow but healthy setup could then exceed the 15-second deadline and start the same work again.

Serve cached provider status and use the existing background refreshes. Give complete connection setup 45 seconds; keep the existing socket-open and heartbeat limits. T3 Connect and the native launcher stay unchanged.

Verified: 368 focused tests pass, including 26-second direct/relay setup, reconnects, timeout cleanup, provider updates, and shell/thread sync. The new regression cases fail on main. Client/server typechecks, scoped lint, formatting, and CodeRabbit review pass.

A stalled setup can now wait up to 45 seconds before retrying. This does not fix Cloudflare network outages.

Model: GPT-6. Harness: Codex.

Summary by CodeRabbit

  • Bug Fixes
    • Extended the connection setup timeout from 15 to 45 seconds, improving support for slow connections and reconnects.
    • Prevented unnecessary provider refreshes when opening server configuration subscriptions.
    • Ensured multiple configuration subscriptions receive the expected snapshots consistently.

@github-actions github-actions Bot added vouch:trusted PR author is trusted by repo permissions or the VOUCHED list. size:XS 0-9 changed lines (additions + deletions). labels Sep 12, 2026
@macroscopeapp

macroscopeapp Bot commented Sep 12, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Not approved

Macroscope's review found this PR not approvable — This targeted reconnect fix removes provider probes from each server-config connection, but also raises the client’s default connection-establishment timeout from 15 to 45 seconds. That production default changes connection failure and retry timing and merits human review.

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

@coderabbitai

coderabbitai Bot commented Sep 12, 2026

Copy link
Copy Markdown

Review Change StackReview Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Advanced

Run ID: 7f254afa-07a9-4ff9-bdc3-661fb61cae2f

📥 Commits

Reviewing files that changed from the base of the PR and between c542b78 and fbdda7f.

📒 Files selected for processing (4)
  • apps/server/src/server.test.ts
  • apps/server/src/ws.ts
  • packages/client-runtime/src/connection/supervisor.test.ts
  • packages/client-runtime/src/connection/supervisor.ts
💤 Files with no reviewable changes (1)
  • apps/server/src/ws.ts

Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.


📝 Walkthrough

Walkthrough

The server subscription no longer triggers provider refreshes. The connection supervisor now allows 45 seconds for establishment, with tests covering slow setup, reconnects, and timeout behavior.

Changes

Server configuration subscription

Layer / File(s) Summary
Snapshot-based subscription flow
apps/server/src/ws.ts, apps/server/src/server.test.ts
The subscription handler no longer starts providerRegistry.refresh(). Tests verify that reconnecting subscriptions receive snapshots without provider refresh calls.

Connection establishment timeout

Layer / File(s) Summary
Extended establishment window and coverage
packages/client-runtime/src/connection/supervisor.ts, packages/client-runtime/src/connection/supervisor.test.ts
The establishment timeout increases from 15 to 45 seconds. Tests cover slow setup, reconnects, stalled relay connections, timeout release, and platform wakeups.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: t3dotgg

Merge Risk: ⚪ Minimal · up to fbdda

The subscription path now serves cached snapshots without triggering provider refreshes, and the longer establishment timeout is covered for slow setup, reconnect, timeout, relay, and wakeup behavior.

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly identifies the main change: preventing provider refresh storms during reconnects. It is concise and specific.
Description check ✅ Passed The description clearly explains what changed, why it changed, the timeout impact, scope boundaries, and verification results. It does not use the template headings or checklist, but it contains the r…
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check. Docstring coverage is scoped to functions touched by this diff. Analyzed 0 functions across 3…
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.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size:XS 0-9 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.

1 participant