Bound the connect dial and handshake separately from operations - #46
Conversation
A steady-state operation may legitimately wait a long time for a large or slow response; establishing a connection may not. The pool salvages a connection whose operation just failed by calling connect() through reconnect(), and behind a proxy that accepts instantly while its backend is unreachable, the dial always "succeeds" — the first SCRAM reply is what stalls. That salvage attempt then cost a second full receive timeout on top of the failure it was recovering from: a 10s operation deadline produced a measured 21.8s outage per dead pooled connection. The new connectTimeout bounds both the TCP dial and the handshake receives, and only those: the flag arming the shorter deadline is scoped to connect() with a finally, so a slow first real query never inherits it and a failed handshake never leaves it armed on a reused client. Defaults to $timeout, so existing callers keep their behaviour. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 31 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe client now accepts an optional connection timeout. Dialing and SCRAM authentication use this timeout, while normal receives use the standard timeout after handshake completion or failure. ChangesConnection timeout
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant Socket
participant SCRAM
Client->>Socket: Dial using connectTimeout
Client->>SCRAM: Authenticate with handshake timeout
SCRAM-->>Client: Authentication result
Client->>Socket: Receive using normal timeout
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryAdds a separate connection deadline and applies it across dialing and SCRAM authentication.
Confidence Score: 4/5The PR is not yet safe to merge because the synchronous receive path can still block beyond the configured connection deadline. The shared absolute deadline fixes phase-by-phase renewal, and coroutine receives receive the remaining budget directly. However, the synchronous path relies on an error-suppressed post-connect timeout update that the implementation itself acknowledges may not be honored; in that case, the blocking receive retains the longer steady-state timeout and the deadline cannot be rechecked until afterward. Files Needing Attention: src/Client.php Important Files Changed
Reviews (3): Last reviewed commit: "fix(client): bound the socket-level wait..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/Client.php`:
- Around line 307-320: Update the SCRAM handshake sequence around auth->start(),
query(), and auth->continue() so any exception, including decoded authentication
command errors, invalidates the transport before being rethrown; retain the
existing handshaking cleanup in finally. Add coverage for an ok: 0
authentication response that verifies the transport closes and a subsequent
connect() performs a new dial.
- Around line 294-295: Update the connection setup around Client::connect() so
it uses $this->timeout as the initial Swoole receive timeout, while retaining
$connectTimeout for the dial timeout if the API supports separate configuration.
After SCRAM completes successfully, update the client receive timeout to
$this->timeout using the existing receiveTimeout() flow.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9dbae8dc-3583-4927-814b-33258d0612ae
📒 Files selected for processing (2)
src/Client.phptests/ClientTest.php
Review findings on the connect deadline, both real. The dial and each SCRAM receive were each granted a fresh connectTimeout, so a phase that consumed most of its allowance left the next a full one — the complete attempt could take a multiple of the documented bound. The deadline is now absolute: set once before the dial, every handshake receive gets the remainder, and chunk-arrival renewals shrink toward it instead of extending past it. A failed SCRAM exchange also left the transport dialed and reporting connected, so a later connect() early-returned without ever completing authentication. The handshake now invalidates the socket on any failure before rethrowing: the next connect() dials fresh. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Both findings addressed in c023ef1:
|
Third finding on the same seam, and the root of all three: the deadline arithmetic in receive() means nothing while the blocking primitive answers only to the constructor-time steady-state timeout. A peer that accepts and then goes silent — a proxy fronting an unreachable backend, the exact case connect() is being bounded for — parks the first recv() for the full steady-state window before any deadline is rechecked. Every socket wait now takes the remaining budget of the loop it serves: per-call on the coroutine client, via a per-wait refresh of the client timeout option on the synchronous one. Where a synchronous build applies options only at connect(), the behaviour degrades to exactly what shipped before — the loop-level deadline still bounds the total — and where honoured, the wait matches the budget. The contract tests observe the socket layer directly: every handshake wait stays within the remaining connect budget, steady-state waits return to the full receive timeout, and the synchronous transport sees its option refreshed per wait. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
ef33138 addresses the root the three findings circled: the client has two timeout layers, and the fixes so far steered only the loop-level deadline while each blocking Contract tests observe the socket layer directly: handshake waits never exceed the remaining connect budget (verified red: |
What
Adds a
connectTimeoutconstructor parameter toClient: a deadline covering the TCP dial and the SCRAM handshake, separate from the steady-state receive$timeout. Defaults to$timeout, so existing callers keep their behaviour.Why
The two timeouts answer different questions. A steady-state operation may legitimately wait a long time for a large or slow response. Establishing a connection may not:
Poolssalvages a connection whose operation just failed by callingconnect()viareconnect(), and behind a proxy that accepts instantly while its backend is unreachable, the dial always "succeeds" — the first handshake reply is what stalls. That salvage attempt then costs a second full receive timeout on top of the failure it was recovering from.Measured in Appwrite Cloud CI (dedicated MongoDB behind the edge TCP proxy, during a resize): a 10s operation deadline produced 21.8s of API stall per dead pooled connection — the operation's 10s plus a 10s handshake wait inside recovery.
How
connect()dials withconnectTimeout ?? timeout.handshakingflag arms the same bound for the SCRAM receives, scoped withfinallyso a slow first real query never inherits it and a failed handshake never leaves it armed on a reused client.Tests
testConnectDialsAndHandshakesUnderTheConnectDeadline— the dial carries the connect deadline; the flag clears after a completed handshake.testHandshakeSilenceFailsAtTheConnectDeadline— a silent handshake fails at the connect deadline instead of the receive timeout (verified red: without the fix it waits the full receive timeout).Downstream: consumed by appwrite-labs/cloud#5047's stack to bound dedicated-database recovery.
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes