fix: read the auth token from response headers before trailers - #23
Open
lukekim wants to merge 1 commit into
Open
fix: read the auth token from response headers before trailers#23lukekim wants to merge 1 commit into
lukekim wants to merge 1 commit into
Conversation
AuthenticateAsync passed stream.GetTrailers() as an argument, so trailers were
read eagerly — before the handshake had completed. gRPC only makes trailers
available once a call finishes, so any handshake that fails throws
System.InvalidOperationException: Can't get the call trailers because the
call has not completed successfully.
which replaces the gRPC status describing the actual problem. Every integration
test in CI reports this, and it says nothing about what to fix.
Now the token is read from the response headers first, trailers are consulted
only if the headers carry none, and a failure to read them is caught rather
than propagated. The resulting SpiceException names the likely cause and
carries the originating RpcException as InnerException.
Reproduced against a TLS endpoint that is not a Flight service:
before: InvalidOperationException: Can't get the call trailers ...
after: SpiceException: Failed to authenticate: the runtime returned no
authorization token. Check that the API key is valid for this endpoint.
There was a problem hiding this comment.
Pull request overview
Fixes gRPC Flight authentication failures by avoiding an eager trailers read during handshake, so authentication can succeed when the token is already present in response headers and failures surface a meaningful SpiceException (with the underlying RpcException preserved).
Changes:
- Reads the auth token from handshake response headers first; only consults trailers as a fallback and suppresses trailer-read exceptions.
- Improves authentication failure diagnostics by formatting gRPC failures into actionable messages.
- Adds an internal
SpiceExceptionconstructor that accepts aninnerException.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
Spice/src/Flight/SpiceFlightClient.cs |
Reworks AuthenticateAsync to avoid eager trailer reads; adds helper methods for trailer token retrieval and RPC failure description. |
Spice/src/Errors/SpiceException.cs |
Adds an internal constructor overload to preserve inner exceptions for better error reporting. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+228
to
+232
| catch (InvalidOperationException) | ||
| { | ||
| // The handshake never completed, so there are no trailers to read. | ||
| return null; | ||
| } |
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
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.
This fixes the failing tests
Initially I thought this was only a diagnostics improvement and that CI needed a credential rotation. That was wrong, and the evidence is direct: the TPCH cloud tests pass on this branch and fail on #22, same repo, same secrets, same runner.
The credential was never broken. The eager trailer read threw before the token in the response headers could be used, so every authenticated connection died regardless of whether the key was valid.
FailedPassedThe 23 skips are the ADBC-driver tests, which skip for their own unrelated reasons.
What
AuthenticateAsyncpassedstream.GetTrailers()as an argument:Arguments are evaluated eagerly, so the trailers are read before the handshake has completed. gRPC only exposes trailers once a call finishes, so any failing handshake throws:
That exception replaces the gRPC status that says what actually went wrong.
The token is now read from response headers first; trailers are consulted only if the headers carry none, and failing to read them is caught rather than propagated. The resulting
SpiceExceptionnames the likely cause and carries the originatingRpcExceptionasInnerException. Adds an internalSpiceExceptionconstructor accepting an inner exception.Why it matters
This is repo-wide and long-standing, not specific to any branch:
trunk's ownprrun has been failing with this exact error since 2026-07-25Anyone looking at CI right now sees a .NET internal error about trailers and has no way to tell that the cause is a credential. That's what this changes.
Verification
Reproduced against a TLS endpoint that isn't a Flight service, which produces the same handshake-failure shape as CI:
InvalidOperationException: Can't get the call trailers because the call has not completed successfully.SpiceException: Failed to authenticate: the runtime returned no authorization token. Check that the API key is valid for this endpoint.Also checked the healthy-server-but-no-token path (valid endpoint, bad key) still fails gracefully with the same clear message rather than regressing.
dotnet build -c Release— all four targets includingnetstandard2.0dotnet test— 115 passed, 0 failed on net8.0 / net9.0 / net10.0 (33 skipped: the cloud integration tests, which need the API key)Follow-on effect
Merging this unblocks #22, whose 11 failing checks are all this same bug.