Repository navigation
Conversation
…orking connection The supervisor raced every connect attempt against one 15 s budget that also covered the first config snapshot. Once the session is synchronizing, the attempt now gets a separate 2 minute bound for the snapshot. Dead sockets still fail early through the socket open timeout and ping check. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
github-actions
Bot
added
vouch:unvouched
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configuration
📒 Files selected for processing (2)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review. 📝 WalkthroughWalkthroughThe supervisor now allows a synchronizing connection attempt additional time to receive its first config snapshot after the route-scaled setup timeout. Tests cover delayed readiness, expiration of the extended timeout, and a transport failure during synchronization. ChangesFirst config snapshot wait
Priority: ➖ Normal Estimated code review effort: 2 (Simple) | ~10 minutes Change: Bug fix · Severity of issue fixed: Medium Suggested reviewers: Merge Risk: ⚪ Minimal · up to Clients connecting to a server with a slow first config snapshot get up to two extra minutes before the attempt times out, instead of repeatedly dropping a working socket. No merge-blocking risk was found. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to Identity checks and connection cleanup remain intact. However, a failed route can leave the snapshot-wait state active during fallback preparation, allowing the extra two-minute wait after the original connection has already closed. The demonstrated impact is delayed connection recovery, not increased access or privileges. Retained concerns
Security review detailsSecurity Blast Radius
Security Findings and Attack Paths
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
juliusmarminge
added
the
macroscope-review
ChatGPT Codex Connector
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| if ("prepared" in progress) { | ||
| yield* SubscriptionRef.set(prepared, Option.some(progress.prepared)); | ||
| } | ||
| yield* Ref.set(synchronizing, progress.stage === "synchronizing"); |
There was a problem hiding this comment.
🟠 High connection/supervisor.ts:427
A setup attempt can wait an extra firstSnapshotTimeout and delay retry even though its current route is still preparing credentials. synchronizing remains true if an earlier route reached that stage and its session failed, because driver.ts reports preparing only once before trying routes; reset the flag when each route begins preparation.
🤖 Copy this AI Prompt to have your agent fix this:
In file @packages/client-runtime/src/connection/supervisor.ts around line 427:
A setup attempt can wait an extra `firstSnapshotTimeout` and delay retry even though its current route is still preparing credentials. `synchronizing` remains true if an earlier route reached that stage and its session failed, because `driver.ts` reports `preparing` only once before trying routes; reset the flag when each route begins preparation.
ApprovabilityVerdict: Would Approve Macroscope's review found this PR approvable — This is a focused client-runtime bug fix with targeted tests that preserves existing setup timeouts while allowing a live connection more time to receive its first configuration snapshot. A shared synchronizing flag can remain set after one route fails, potentially making a later credentials-preparation route wait an unnecessary two minutes. Not approved because:
Adjust the Minimum Blocking Severity for this repo — including turning it Off — in Settings. You can add or adjust custom eligibility rules. Learn more. |
2 participants
Problem
A client connecting to a server whose first config snapshot takes longer than 15 s never finishes connecting. The supervisor tears down the working socket at 15 s, waits 3s/4s/8s/16s, and starts
subscribeServerConfigfrom scratch, so the snapshot never gets to complete until the server warms up. The 15 s budget (CONNECTION_ESTABLISHMENT_TIMEOUTinpackages/client-runtime/src/connection/supervisor.ts) covers credentials, session creation, socket open and the first snapshot, becauseConnectionDriver.connectonly returns atsession.ready(packages/client-runtime/src/rpc/session.ts).Change
The supervisor keeps the 15 s (per route) establishment budget. If the attempt has reached the
synchronizingstage when that budget runs out, it now gets an additionalFIRST_SNAPSHOT_TIMEOUT(2 minutes) before it is recorded as a setup timeout. The supervisor tracks this with asynchronizingflag set from the driver's progress reports, so the driver,RpcSessionand the public types are untouched, as are the backoff policy and the failure shape.Liveness is not weakened: a socket that never opens still fails at the socket open timeout, and a socket that stops answering pings fails
session.readyvia the existing ping-timeout disconnect, which reconnects immediately. The 2 minute bound only applies to an open, live connection whose snapshot handler stays silent.Scope and approval
Fixes #14516. The issue is a triaged bug and the fix is confined to the shared supervisor timeout.
Verification
supervisor.test.ts, usingTestClock:mainand pass with the fix.vp test runinpackages/client-runtime: 128 files, 2091 tests pass.tsc --noEmit,vp lintandvp fmt --checkon the changed directory pass.🤖 Generated with Claude Code (Claude Sonnet 5.5)