fix: fail fast on Mongo socket receive timeouts#42
Conversation
Stop retrying recv() false up to 10k times after the socket timeout already elapsed, which could hang Appwrite requests past the HTTP client's 120s limit. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Warning Review limit reached
Next review available in: 42 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 Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesSocket timeout handling
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Client
participant SwooleClient
participant Scheduler
Client->>SwooleClient: recv()
SwooleClient-->>Client: data or transient empty read
Client->>Client: Check wall-clock deadline
Client->>Scheduler: Sleep briefly
Scheduler-->>Client: Resume before deadline
Client->>SwooleClient: recv() again
SwooleClient-->>Client: Data or receive error
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 SummaryThis PR replaces a 10,000-attempt retry loop in
Confidence Score: 5/5Safe to merge. The core logic change is correct: recv()===false now throws immediately, the idle deadline resets on each real chunk (so large responses complete), and the new constructor parameter is backward-compatible. The fix eliminates the root cause of the multi-minute hang correctly. The deadline-reset-on-chunk approach properly handles large multi-chunk responses without imposing a hard wall-clock budget from the first byte, addressing the concern raised in the previous review round. The configurable timeout addition is non-breaking. No data-correctness or connection-state bugs were introduced. No files require special attention; the two suggestions are purely cosmetic/diagnostic improvements. Important Files Changed
Reviews (3): Last reviewed commit: "refactor: rename receiveTimeout property..." | Re-trigger Greptile |
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 464-487: Update the timeout exceptions in the receive handling
logic of Client to pass MongoDB’s standard socket timeout code 11601 as the
constructor’s code argument. Apply this to the deadline check and both
false/empty recv timeout branches, preserving their existing messages and
conditions.
- Around line 434-438: Update send() to use the original non-zero error code
when the retry result is zero by applying a falsey fallback instead of
null-coalescing. Ensure reconnect send failures use error code 9001 and
receive() timeout exceptions use 11601, preserving correct isNetworkError() and
isTimeoutError() classification.
🪄 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
Run ID: 37258707-e024-4a48-ba60-12b9c18f4bb9
📒 Files selected for processing (1)
src/Client.php
Reset the receive deadline on each chunk so large transfers are not cut off mid-stream, expose setTimeout/getTimeout, and use Mongo socket codes so isTimeoutError()/isNetworkError() classify these failures. Co-authored-by: Cursor <cursoragent@cursor.com>
Replace setTimeout/getTimeout with an optional trailing $timeout constructor argument (default 30s) so callers configure it at connect time. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
What does this PR do?
Fails fast when a Swoole Mongo socket
recv()times out, instead of retrying up to 10 000 times and turning a single ~30s socket timeout into a multi-minute hang.This was found while investigating Appwrite CI flake
DocumentsDBCustomServerTest::testTimeout(create of ~12MB documents under load). Appwrite never got an HTTP response; the e2e client hit:Appwrite logs during the hang showed the library spinning / then failing late:
Root cause
Client::receive()treatedrecv() === false(and empty string) as “try again” with adaptive sleep, up tomaxAttempts = 10000. The Swoole client already has'timeout' => 30, so each failedrecv()can wait ~30s. Retrying that path multiplies into hangs that exceed Appwrite’s HTTP client 120s curl timeout — the API never responds.Relevant before behavior:
Fix
recv() === false→ throw immediately (socket wait already elapsed; do not multiply it).$receiveTimeout(30s), shared with the Swoole'timeout'option.''+ nonzeroerrCode→ treat as timeout (some Swoole builds return empty string on timeout instead offalse).errCode === 0still yield briefly until the deadline.errCodeafter reconnection failure.Relevant after behavior:
Under load, Appwrite now surfaces fail-fast errors like:
instead of silently blocking until the HTTP client gives up.
How we reproduced (Appwrite + this driver)
Local OrbStack stack, CPU-capped to force Mongo contention:
Stress results
0 bytes received)recv falsefail-fast only)errCode+ send errCode)500 ≠ 408What this fixes: the library no longer multiplies one socket timeout into a multi-minute receive loop. Failures show up as Mongo
Receive timeout (errCode=11)within ~one socket timeout budget.What this does not fully eliminate: under extreme 0.5 CPU + concurrent 12MB creates, some requests can still starve past curl’s 120s (worker/CPU starvation) or return truncated bodies. That class of flake still wants a lighter Appwrite
testTimeoutsetup and/or mapping Mongo receive timeouts → HTTP 408 on the Appwrite side (DATABASE_TIMEOUT).Alone / unloaded,
testTimeoutcompletes in ~10–14s.Test plan
utopia-php/mongounit/integration suiteClient.phpcopied into running containerReceive timeout ... (errCode=11)after patchDATABASE_TIMEOUT(408)DatabasesBase::testTimeoutdocument payload / countNotes / BC
timeout).maxTimeMSvia the database adapter; this PR only fixes the wire-protocol receive loop, not querymaxTimeMSsemantics.Summary by CodeRabbit