fix(client): code the pre-send failures so callers can replay them - #47
Conversation
Connection validation runs ahead of the send, so both failures it raises leave the command unsent and nothing applied - the one class of failure a caller may safely replay. Both carried code 0, making that legible only in the message text, and a caller that classifies on text turns a permanent failure into an advertised-retryable one the moment a message quotes a caller-chosen value. A cloud connection pool consequently could not tell a replayable refusal from a post-send timeout of unknown outcome, and surfaced the former to clients as a 500 during a backing resize. Both now carry MongoDB's own HostUnreachable, with isUnsentError() to read it. The messages are unchanged, so the client's own internal reconnect check that matches on them still behaves identically. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 46 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 (4)
📝 WalkthroughWalkthroughThe change adds ChangesUnsent error classification
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: 🚥 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 SummaryThe PR now distinguishes failures raised before command transmission using an
Confidence Score: 5/5The PR appears safe to merge. No blocking failure remains; both previously reported issues are resolved by the type-based classification. Important Files Changed
Reviews (2): Last reviewed commit: "fix(client): carry the pre-send failures..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
Adds an explicit, code-based way to classify “pre-send” connection validation failures as replayable (unsent), so callers can safely distinguish them from post-send network/timeouts without matching on error message text.
Changes:
- Introduces
Exception::HOST_UNREACHABLE(MongoDB error code 6) andException::isUnsentError()for identifying replayable pre-send failures. - Updates
Client::validateConnection()to throw connection-validation exceptions withHOST_UNREACHABLEinstead of the default code0. - Adds
tests/UnsentErrorTest.phpto verify unsent vs timeout vs uncoded error classification.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/UnsentErrorTest.php | Adds unit tests asserting unsent errors are distinguishable by code and timeouts are not misclassified. |
| src/Exception.php | Adds HOST_UNREACHABLE and isUnsentError() to expose unsent/replayable failures via code. |
| src/Client.php | Tags pre-send connection validation throws with HOST_UNREACHABLE for reliable downstream classification. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| * safely - a message quotes caller-chosen values, a code cannot be | ||
| * spelled by a caller. | ||
| */ | ||
| public const int HOST_UNREACHABLE = 6; |
| /** | ||
| * Check if this is a timeout error. | ||
| * | ||
| * @return bool | ||
| */ |
![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 2181-2190: Update the failed reconnect-dial exception raised by
connect() and propagated through validateConnection() to use
Exception::HOST_UNREACHABLE, so unsent commands are recognized by
isUnsentError(). Add a transport test covering a failed reconnect and asserting
isUnsentError() returns true.
In `@src/Exception.php`:
- Around line 125-127: Update isUnsentError() and the related Exception
construction flow so unsent status is tracked by explicit local pre-send state
rather than inferred from code HOST_UNREACHABLE. Set that state only when the
client fails before sending, while preserving HOST_UNREACHABLE for
connection-validation classification; ensure fromResponse() and
Client::parseResponse() leave response-originated code 6 marked as sent/unknown,
and add coverage for that case.
🪄 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: fce2e465-315d-4f40-b890-ada9b051e52f
📒 Files selected for processing (3)
src/Client.phpsrc/Exception.phptests/UnsentErrorTest.php
Review found the first attempt unsound. Connection validation and the dial before it run ahead of every send, so a failure raised there leaves the command unsent and nothing applied - but a numeric code cannot say so. A post-send error response is parsed into the same Exception holding the SERVER's code, HostUnreachable included, so any caller keying on the code would replay an operation that was already transmitted and could apply it twice. The typed class constant also broke the PHP 8.0-8.2 this package still supports. The signal is now a type: UnsentException, raised only by the client and only before it has sent anything, answering isUnsentError() true where the base answers false. It extends Exception, so every existing catch behaves identically, and the messages are unchanged so the client's own internal reconnect check still matches. The failed reconnect dial throws it too - review caught that it escapes validateConnection while the command is just as unsent. Covered: an unsent failure is distinguishable by type and still caught as the package exception, a post-send timeout is never unsent, and a server response reporting HostUnreachable is never unsent. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
All four findings were right, and the second one invalidated the premise of the first attempt. Reworked in 232d355. Error code cannot prove unsent state — correct, and this was the important one. Typed constant breaks supported PHP — correct; the constant is gone entirely rather than untyped, since the type now carries the meaning. Code failed reconnect dials as unsent errors (CodeRabbit) — correct. The failed dial at
|
Connection validation runs ahead of the send, so both failures it raises leave the command unsent and nothing applied — the one class of failure a caller may safely replay.
Both carried code
0, making that distinction legible only in the message text. A caller that classifies on text turns a permanent failure into an advertised-retryable one the moment a message quotes a caller-chosen value, so callers correctly refuse to do it. The consequence downstream: a cloud connection pool could not tell a replayable pre-send refusal from a post-send receive timeout of unknown outcome, and surfaced the former to clients as a 500 during a dedicated-backing resize.Both throws now carry MongoDB's own
HostUnreachable(6), withisUnsentError()to read it. The messages are unchanged, so the client's own internal reconnect check that matches on'Connection to MongoDB has been lost'behaves identically.Covered by
tests/UnsentErrorTest.php: an unsent failure is distinguishable by code, a post-send timeout is never reported as unsent, and an uncoded failure is not assumed unsent.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes