[PureGo] Preserve server rejection on handshake timeout - #636
[PureGo] Preserve server rejection on handshake timeout#636zlata-stefanovic-db wants to merge 6 commits into
Conversation
Prefer a real server rejection over deadline/cancel in rawStream handshake timeout path. Adds regression test for teardown-gated Recv returning Unauthenticated. Fixes #635 Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
Move the rejection-vs-deadline decision after the select so it no longer depends on which case wins the race, guarantee teardown whenever hctx has expired, and extract the classification as isTeardownArtifact. Add coverage for the teardown-artifact branches (gRPC and bare context cancellation, EOF), assert the rejection replaces DeadlineExceeded rather than merely being present, and consolidate the two near-identical blocking RPC fakes. Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
teodordelibasic-db
left a comment
There was a problem hiding this comment.
Pre-existing, unrelated to PR, at purego/internal/transport/transport_test.go:529:
The regression suite does not exercise the user-visible failure path where an auth rejection has already been captured, the open context expires, and HeadersProvider.Invalidate must still run. The new handshake tests cover arbitration with fakes, while this existing test covers only an immediate rejection without deadline pressure, so the AfterFunc bridge, gRPC wrapping, and invalidation are never exercised together.
Could we add a deterministic Conn.Open test with a client-stream interceptor that first captures the real RecvMsg result and then holds it until openCtx.Done() before returning it?
func (s *heldRecvErrorStream) RecvMsg(m any) error {
err := s.ClientStream.RecvMsg(m)
<-s.release // openCtx.Done()
return err
}With the in-memory server returning codes.Unauthenticated, this guarantees the trailer is captured before the deadline but reaches the handshake only after hctx has expired. The test can assert that Open returns codes.Unauthenticated, not context.DeadlineExceeded, and that Invalidate is called once.
| // error instead, and EOF carries no status at all. | ||
| func isTeardownArtifact(err error) bool { | ||
| switch status.Code(err) { | ||
| case codes.Canceled, codes.DeadlineExceeded: |
There was a problem hiding this comment.
codes.DeadlineExceeded is not a teardown artifact on the current Conn.Open path. streamCtx is built with context.WithoutCancel and context.WithCancel, so it has no deadline. Calling cancelStream makes grpc-go report codes.Canceled; a codes.DeadlineExceeded returned by Recv can therefore be a server status.
Treating that status as teardown noise replaces it with hctx.Err(). This also changes recovery behavior today: isRetryable treats the context error as terminal, while a gRPC status with the same code does not satisfy errors.Is(err, context.DeadlineExceeded) and remains retryable. Could we narrow the gRPC case to codes.Canceled and keep the bare context checks below it?
switch status.Code(err) {
case codes.Canceled:
return true
}The existing grpc deadline exceeded table case can then become a preservation case using serverErr := status.Error(codes.DeadlineExceeded, "server deadline") and asserting errors.Is(got, serverErr). That assertion deterministically fails with the current classifier and passes after the switch is narrowed.
There was a problem hiding this comment.
Implemented both, thanks for the detailed suggestions
Added a deterministic Conn.Open regression using a held RecvMsg interceptor, so we now cover the “auth rejection captured first, open deadline expires before return” race. The test asserts we return Unauthenticated (not DeadlineExceeded) and still call Invalidate once.
Also narrowed teardown-artifact gRPC classification to codes.Canceled only. codes.DeadlineExceeded is now preserved as a possible server status, and the tests were updated to assert that preservation path.
Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
Signed-off-by: Zlata Stefanovic <zlata.stefanovic@databricks.com>
Summary
raw_streamhandshake expiry path instead of always returninghctx.Err()hctx's state rather than on whichselectcase won, so a response and an expiry becoming ready together cannot make the error a coin flipteardownwheneverhctxhas expired, including when the recv branch observed the result firstisTeardownArtifact) so an RPC the SDK itself cancelled is not misread as a server rejectionWhy
When the server's rejection lands in the same window as the handshake deadline,
the status was discarded and
Openreturned a bareDeadlineExceeded. Twothings follow from losing the code:
isAuthRejection(err)is false, soHeadersProvider.Invalidateis nevercalled and rejected credentials stay cached.
isRetryableininternal/stream/supervisor.gotreatscontext.DeadlineExceededasterminal, so the supervisor breaks its recovery loop, fails the watermark,
and abandons every buffered record to
OnError. A permanent setup failure thata fresh token would fix instead kills the stream outright.
Note for anyone reading #635: its impact section predicts the opposite outcome —
that a
transport.IsTerminalStatusreturns false and the supervisor "retries forthe full recovery budget, resending records each time". That function does not
exist, and the stream layer does no gRPC-code classification at all. The real
behaviour is terminal-on-first-occurrence, which is worse than the issue
describes.
Why the exclusion list is necessary rather than a heuristic
An earlier revision of this PR tried to replace the shape-based exclusions with
ordering: trust any result reaped before
teardown(), on the theory that itmust predate the cancellation and therefore came from the server.
TestOpenHonorsCallerDeadlinerejects that.Conn.Openinstalls a secondcanceller —
context.AfterFunc(handshakeCtx, cancelStream)— which fires as soonas
handshakeCtxexpires, independently ofhandshake's ownteardown. Acodes.Canceledcan already be sitting indonebeforeteardownis called, soarrival order says nothing about provenance and the error's shape is the only
signal available.
Scope
This makes the reported cause faithful; it does not change the fact that an
expired
hctxalways fails the handshake, and it does not resurrect a readinessresponse that arrived too late (
Openalready fails that case via itspost-handshake bridge check).
It also does not address the related defect in #637: because
isRetryabletreatsany
context.DeadlineExceededas terminal, an internal setup budget expiring isitself misclassified as permanent. Fixing that would make this class of masking
self-healing — the next attempt would surface the real status and re-mint — which
is how the Rust core already behaves (
DeadlineExceededis absent fromUNRETRIABLE_STATUS_CODES). The two changes are complementary and independent.Fixes #635
Testing
gofmt -l .,go vet ./...go test -race -count=3 ./...frompurego/TestRawStreamHandshakeTimeoutPrefersServerRejection(rejection replacesDeadlineExceeded),TestRawStreamHandshakeTimeoutKeepsDeadlineForTeardownArtifacts(table over gRPC
Canceled/DeadlineExceeded, bare context errors, and EOF),TestRawStreamHandshakeTimeoutRejectionIsDeterministic(200 iterations acrossboth
selectinterleavings)