service: report the first CTP connection error, not the last - #38722
Open
antiguru wants to merge 1 commit into
Open
service: report the first CTP connection error, not the last#38722antiguru wants to merge 1 commit into
antiguru wants to merge 1 commit into
Conversation
A CTP connection's send and receive tasks both reported their errors into a
single `watch` channel, which keeps only the latest write. When the send task
hit its idle deadline it dropped its write half, the peer observed the resulting
shutdown and closed the connection, and the receive task's "unexpected end of
file" then overwrote the timeout that had caused it. Both raw errors were logged
at `debug!`, so at the default log level the actual cause appeared nowhere and
the controllers surfaced the consequence as `replica task failed: unexpected end
of file`. That reads as the replica having gone away when the controller had in
fact stopped writing to it.
Replace the raw `watch` pair with an `ErrorTx`/`ErrorRx` pair over
`Option<String>`. `ErrorTx::report` fills the slot only while it is empty, so
the first error wins and later consequential errors are discarded. `ErrorRx`
still falls back to a generic "connection closed" error when both tasks shut
down without reporting anything.
Errors now name the failing direction, as `send error: {e}` or
`recv error: {e}`. First-wins on its own would have surfaced a bare `timed out`,
which still does not say whether the local endpoint stopped writing or the peer
stopped answering, and that ambiguity is what made the reported connection loss
hard to attribute. Both task errors move from `debug!` to `warn!`, matching the
level the compute and storage controllers already use for `replica task failed`,
so a warning-filtered query shows the cause and its consequence together. These
paths only run on unexpected breaks: a deliberate teardown drops the connection,
which aborts both tasks before either can report.
New unit tests in `src/service/src/transport/tests.rs` cover the error channel
directly: first-wins, first-wins against an error reported after collection, the
no-error fallback, and waiting for an error that has not been reported yet. In
`src/service/tests/transport.rs`, `test_server_error` now waits for the send task
to fail as well before asserting again, pinning first-wins end to end, and the
expected error strings pick up the new direction prefix.
Closes: CPU-249
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
A CTP connection's send and receive tasks both reported their errors into a single
watchchannel, which keeps only the latest write. When the send task hit its idle deadline it dropped its write half, the peer observed the resulting shutdown and closed the connection, and the receive task's "unexpected end of file" then overwrote the timeout that had caused it. Both raw errors were logged atdebug!, so at the default log level the actual cause appeared nowhere and the controllers surfaced the consequence asreplica task failed: unexpected end of file. That reads as the replica having gone away when the controller had in fact stopped writing to it.Replace the raw
watchpair with anErrorTx/ErrorRxpair overOption<String>.ErrorTx::reportfills the slot only while it is empty, so the first error wins and later consequential errors are discarded.ErrorRxstill falls back to a generic "connection closed" error when both tasks shut down without reporting anything.Errors now name the failing direction, as
send error: {e}orrecv error: {e}. First-wins on its own would have surfaced a baretimed out, which still does not say whether the local endpoint stopped writing or the peer stopped answering, and that ambiguity is what made the reported connection loss hard to attribute. Both task errors move fromdebug!towarn!, matching the level the compute and storage controllers already use forreplica task failed, so a warning-filtered query shows the cause and its consequence together. These paths only run on unexpected breaks: a deliberate teardown drops the connection, which aborts both tasks before either can report.New unit tests in
src/service/src/transport/tests.rscover the error channel directly: first-wins, first-wins against an error reported after collection, the no-error fallback, and waiting for an error that has not been reported yet. Insrc/service/tests/transport.rs,test_server_errornow waits for the send task to fail as well before asserting again, pinning first-wins end to end, and the expected error strings pick up the new direction prefix.Closes: CPU-249
Posted by Claude Code.
🤖 Generated with Claude Code