Skip to content

drpc: classify connection-teardown errors at the source#76

Open
shubhamdhama wants to merge 2 commits into
cockroachdb:mainfrom
shubhamdhama:improve-torpc-error
Open

drpc: classify connection-teardown errors at the source#76
shubhamdhama wants to merge 2 commits into
cockroachdb:mainfrom
shubhamdhama:improve-torpc-error

Conversation

@shubhamdhama

@shubhamdhama shubhamdhama commented Jul 2, 2026

Copy link
Copy Markdown

The idea across this PR is simple: classify an error where it happens, and
only translate it at the edge.

drpc.ToRPCErr is the translator from drpc errors to gRPC status codes, for
callers that work in terms of codes (for example
grpcutil.IsClosedConnection). It only recognizes errors that were already
tagged with a class it knows about: ConnectionError and ClosedError map to
codes.Unavailable (matched even through wrapping), the context and EOF
sentinels map to their own codes (matched by identity), and everything else
becomes codes.Unknown. The boundary has no way to recover intent that was
never attached to the error, so anything that can tear a connection down has to
classify its own error at the source. The edge only translates.

A few teardown paths did not do this and reached the boundary as
codes.Unknown, which defeated the connection-liveness checks. We saw it as a
TestDrain flake under DRPC stream multiplexing: the server kept reporting
"not yet refusing RPC" because the teardown error carried Unknown instead of
Unavailable. With multiplexing a single long-lived connection is reused, so
both the write side and the lifecycle path get exercised in practice.

This PR fixes both I/O directions and the lifecycle path, so that Unknown is
left for genuine faults.

drpcwire: classify write failures as ConnectionError
When MuxWriter's flush to the wire failed, it stored and passed along the raw
*net.OpError (EPIPE), which fell through ToRPCErr to codes.Unknown. Now
the failed write is wrapped in drpc.ConnectionError at its source in
MuxWriter.run, mirroring the read path, so the existing ConnectionError
branch maps it to codes.Unavailable. The underlying error stays in the chain.

drpcmanager: classify teardown errors at the source, by cause
Manager.Close(), which showed up as "manager closed: Close called", and an
abrupt read-side EOF were both unclassified. They are now classified by cause,
never with a catch-all. A Close() means the connection is gone, so it becomes
a ClosedError. A read EOF means the peer hung up, which is a ClosedError for
a client and a context.Canceled for a server, matching how gRPC treats a
client hang-up. Every other reader error is left transparent, so a real
protocol or internal fault still surfaces as Unknown rather than being
mistaken for a retryable connection loss. terminate() now just fans the one
already-classified cause out to everyone, which also fixes a raw-error leak in
NewServerStream. This commit drops the now-dead isConnectionReset.

The pattern is documented where the next person will run into it: ToRPCErr
(the translator and the source-classification rule), the error-class
definitions in drpc.go, and readError in the manager.

Comment thread rpc_util.go Outdated
// do not recognize.
func isTransportError(err error) bool {
var opErr *net.OpError
return errors.As(err, &opErr) || errors.Is(err, net.ErrClosed)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this should be a catch-all ("gRPC maps every transport failure"), since the net package has other kinds of errors as well. I haven't checked the gRPC code though.

	var netErr net.Error
	if errors.As(err, &netErr) {
		return true
	}

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed the whole way of detecting the connection failure. Right at the source we will appropriately classify the failure.

@shubhamdhama shubhamdhama force-pushed the improve-torpc-error branch from 5d6ae53 to b633dc9 Compare July 4, 2026 10:00
@shubhamdhama shubhamdhama changed the title rpc_util: map transport failures to codes.Unavailable drpcwire: classify write failures as ConnectionError Jul 4, 2026
drpc.ToRPCErr maps drpc's connection-teardown errors (ConnectionError,
ClosedError) to codes.Unavailable so callers can uniformly detect a dead
connection. The read path wraps teardown errors accordingly
(drpcwire.Reader.read, drpcmanager.Manager.terminate), but the mux write
path did not: when MuxWriter's flush to the wire failed it stored and
propagated the raw *net.OpError, which fell through ToRPCErr to
codes.Unknown.

With stream multiplexing a single long-lived connection is reused, so the
client writes before the read side observes the close and the write hits
EPIPE ("broken pipe") first. Connection-liveness checks keyed on
Unavailable (and the retry and circuit-breaker logic built on them) then
fail to recognize the write-side teardown.

Wrap the failed write in drpc.ConnectionError at its source in
MuxWriter.run, symmetric with the read path, so ToRPCErr's existing
ConnectionError branch maps it to codes.Unavailable. The underlying error
is preserved in the chain. This keeps the classification with the code
that operated the transport rather than re-deriving it from net error
types at the gRPC boundary.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
@shubhamdhama shubhamdhama force-pushed the improve-torpc-error branch from b633dc9 to 46e2e3d Compare July 4, 2026 10:07
@shubhamdhama shubhamdhama changed the title drpcwire: classify write failures as ConnectionError drpc: classify connection-teardown errors at the source Jul 6, 2026
The pattern here is simple: classify errors where they happen, and only
translate them at the edge.

drpc.ToRPCErr is just a translator from drpc errors to gRPC status codes.
It only recognizes errors that were already tagged with a class it knows
about: ConnectionError and ClosedError map to Unavailable (matched even
through wrapping), the context and EOF sentinels map to their own codes
(matched by identity), and everything else becomes Unknown. The boundary
has no way to recover intent that was never attached to the error, so any
subsystem that can tear a connection down has to classify its own error at
the source. The edge only translates.

The manager left two teardown causes unclassified, so they reached the
boundary as Unknown and defeated liveness checks like
grpcutil.IsClosedConnection. We saw this as a TestDrain flake under DRPC
stream multiplexing: the server kept reporting "not yet refusing RPC"
because the teardown error carried Unknown instead of Unavailable. The two
causes were a deliberate Manager.Close(), which showed up as "manager
closed: Close called", and an abrupt read-side EOF.

The fix classifies each one by its cause, never with a catch-all:

  - Close() means the connection is gone, so it terminates with a
    ClosedError (Unavailable) and keeps the "Close called" cause underneath.
  - A read EOF means the peer hung up. For a client the connection is gone
    (ClosedError, Unavailable); for a server a client hang-up is really a
    canceled RPC (context.Canceled), which is how gRPC behaves too.
  - Every other reader error is only wrapped in managerClosed, which is
    transparent to ToRPCErr. An error the reader already classified as a
    ConnectionError still maps to Unavailable, while a protocol or internal
    fault correctly stays Unknown instead of being mistaken for a retryable
    connection loss.

With this, terminate() no longer derives anything on its own. It just fans
the single, already-classified cause out to everyone: the writer, the
in-flight streams, and any stream created after teardown. Because the cause
now lives in the term signal already classified, NewServerStream, which
returns term.Err() directly, is fixed for free. It used to leak the raw
cause.

One caveat worth calling out: never bury a sentinel like io.EOF or
context.Canceled inside an opaque wrapper such as managerClosed. ToRPCErr
matches sentinels by identity, so it would miss them through the wrapper.
Connection classes are matched through wrapping, so wrapping those is fine.

This also removes isConnectionReset. The reader already classifies every
non-EOF I/O error, including connection resets, as ConnectionError, which
ToRPCErr maps to Unavailable, so the reset-sniffing was dead code.

The pattern is now documented where the next person will run into it:
ToRPCErr (the translator and the source-classification contract), the
error-class definitions in drpc.go (what the edge understands, and liveness
vs. fault), and readError in the manager.

This is the read and lifecycle half of the fix. The write half landed in
"drpcwire: classify write failures as ConnectionError", where MuxWriter now
tags a failed write as ConnectionError at the source, mirroring the reader.
Together, both I/O directions and the lifecycle path classify at the source,
so ToRPCErr's Unknown fallback is left for genuine faults.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants