fix(autoselect): demote proxies reset mid-stream#284
Conversation
noteIO discarded every errored read/write, so a proxy that established fine but was reset mid-stream hit no demotion path: the dial-error path never re-runs on an established conn, and Close stops the idle timer before fireStall can fire. Short URL-test probes kept it ranked healthy, so traffic kept routing through a dead tunnel. Classify errored data-plane I/O in noteIO and route real transport failures (non-EOF, non-timeout, non-cancel) through the existing onStall attribution via fireFailure, which skips the proven/lastWasWrite gates an explicit reset doesn't need and dispatches onStall off the I/O path.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds transport-error classification and a new atomic fireFailure path to the data-plane watchdog, distinguishing benign termination from real failures. The tests expand failure-attribution coverage and replace several fatal-style assertions with require-based checks. ChangesData-plane watchdog failure attribution
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Conn
participant noteIO
participant isDataPlaneFailure
participant fireFailure
participant onStall
Conn->>noteIO: IO result (err)
noteIO->>isDataPlaneFailure: classify(err)
isDataPlaneFailure-->>noteIO: benign or failure
alt non-benign failure
noteIO->>fireFailure: fireFailure()
fireFailure->>fireFailure: CAS stalled/fired
fireFailure->>onStall: invoke asynchronously
else benign (EOF, Canceled, timeout)
noteIO-->>Conn: ignore
end
Related Issues: None found in the provided context. Related PRs: None found in the provided context. Suggested labels: enhancement, tests Suggested reviewers: None determined from provided context. 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
This PR updates the MutableAutoSelect data-plane watchdog to more accurately attribute mid-stream transport failures (so only genuine failures demote an outbound) and expands the test suite to cover mid-stream error scenarios and related edge cases.
Changes:
- Added
isDataPlaneFailureandfireFailureto distinguish benign terminal conditions from real mid-stream transport failures and attribute failures immediately. - Refined
noteIOand stall attribution behavior to short-circuit once stalled and avoid misclassification. - Expanded/refactored tests to cover mid-stream resets, benign EOF/timeouts, post-close behavior, and single-attribution semantics.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| protocol/group/mutableautoselect_dataplane.go | Adds explicit mid-stream failure classification and immediate failure attribution path in the watchdog. |
| protocol/group/mutableautoselect_test.go | Adds new mid-stream failure/benign-condition tests and refactors assertions for consistency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
protocol/group/mutableautoselect_test.go (1)
965-1157: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid new failure-attribution coverage; one gap vs. stated classification.
The
failingConn/failingPacketConnhelpers and reset/EOF/timeout/post-close/single-fire tests correctly exercise the newfireFailurepath and its CAS semantics, matching thenoteIO/fireFailureimplementation frommutableautoselect_dataplane.go. However, both the PR summary and AI summary describecontext.Canceledas a benign terminal condition alongside EOF and timeouts, yet no test here verifies that a canceled-context error does not attribute a failure (onlyio.EOFat line 1088 and a synthetic timeout at line 1106 are covered).Consider adding a
TestDataPlaneStream_ContextCanceledDoesNotAttributemirroring the EOF/timeout tests, usingcontext.Canceled(orcontext.DeadlineExceeded, if that's separately classified) as the injected error.🤖 Prompt for 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. In `@protocol/group/mutableautoselect_test.go` around lines 965 - 1157, Add a new failure-attribution test in the same test block as TestDataPlaneStream_EOFDoesNotAttribute and TestDataPlaneStream_TimeoutDoesNotAttribute to cover the benign context-cancellation case. Use the existing failingConn helper and newDataPlaneStream, inject context.Canceled (or context.DeadlineExceeded if that is meant to be classified separately), then assert the read returns the error but the fireFailure callback is not invoked. Reference the same pattern used by TestDataPlaneStream_EOFDoesNotAttribute, TestDataPlaneStream_TimeoutDoesNotAttribute, and the noteIO/fireFailure behavior being validated here.
🤖 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.
Nitpick comments:
In `@protocol/group/mutableautoselect_test.go`:
- Around line 965-1157: Add a new failure-attribution test in the same test
block as TestDataPlaneStream_EOFDoesNotAttribute and
TestDataPlaneStream_TimeoutDoesNotAttribute to cover the benign
context-cancellation case. Use the existing failingConn helper and
newDataPlaneStream, inject context.Canceled (or context.DeadlineExceeded if that
is meant to be classified separately), then assert the read returns the error
but the fireFailure callback is not invoked. Reference the same pattern used by
TestDataPlaneStream_EOFDoesNotAttribute,
TestDataPlaneStream_TimeoutDoesNotAttribute, and the noteIO/fireFailure behavior
being validated here.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0e8b1694-9a90-4051-8c52-97a553d3fc33
📒 Files selected for processing (2)
protocol/group/mutableautoselect_dataplane.goprotocol/group/mutableautoselect_test.go
This pull request improves the robustness and clarity of failure detection and attribution in the data plane watchdog logic, and significantly enhances the test coverage for mid-stream errors and edge cases. The main changes include introducing explicit mid-stream transport failure attribution, updating the handling of benign terminal conditions, and refactoring tests for better reliability and coverage.
Data Plane Failure Detection and Attribution
isDataPlaneFailurefunction to distinguish between real transport failures (such as connection resets) and benign conditions (like EOF, context cancellation, or timeouts), ensuring only genuine failures demote the outbound connection.fireFailuremethod indataPlaneWatchdog, which immediately attributes a failure on explicit mid-stream transport errors, bypassing the proven/lastWasWrite gates and ensuring accurate and timely demotion.Improvements to Idle and Error Handling Logic
noteIOto use the new failure detection logic, ensuring that empty I/O and benign errors do not trigger failure attribution, and that attribution is short-circuited after the connection is already marked as stalled.fireStallto better explain its role in demoting connections that go idle mid-stream.Test Suite Enhancements
failingConnandfailingPacketConnfor simulating error scenarios.require.NoErrorandrequire.FailNowfor more consistent and reliable assertions, replacing manual error checks andt.Fatalcalls. [1] [2] [3] [4] [5] [6] [7] [8] [9]These changes collectively make the data plane watchdog more accurate in detecting real failures, prevent spurious demotions, and ensure that the test suite reliably covers a wide range of edge cases.
Summary by CodeRabbit
Bug Fixes
Tests