fix(adbc): re-authenticate on Unauthenticated instead of failing permanently#73
Merged
Merged
Conversation
…anently The ADBC FlightSQL connection authenticates only once, when it is opened: the Basic-auth handshake yields a server-side session token that is then reused for every prepared statement on that connection. If the server invalidates that session (e.g. it expires after a period of inactivity), every subsequent prepared statement fails with Unauthenticated. Because SqlWithParams classified Unauthenticated as a permanent error, the connection never recovered on its own — the only remedy was to recreate the SpiceClient (restart the process). Detect authentication failures (adbc.StatusUnauthenticated / Unauthorized, with a message fallback) and, when one occurs, re-open the ADBC connection to perform a fresh handshake and retry the query once. Re-initialization is guarded by a mutex and a staleness check so concurrent callers re-open the connection at most once. The plain (non-parameterized) Sql/Query path is unaffected: it re-authenticates on every call, so it was already immune.
There was a problem hiding this comment.
Pull request overview
This PR improves resiliency of the parameterized ADBC FlightSQL query path (SqlWithParams/QueryWithParams) by detecting authentication/session-expiry failures, re-opening the long-lived ADBC connection, and retrying the query once so callers don’t need to recreate the SpiceClient.
Changes:
- Add auth-failure detection (
isADBCAuthError) and retry-once reauthentication behavior forSqlWithParams. - Refactor the transient retry/backoff logic into
execADBCWithBackoff(intended to preserve existing behavior for non-auth errors). - Add a unit test for auth-error classification (
TestIsADBCAuthError) and introduce anadbcMumutex to coordinate reinitialization.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| client.go | Adds an adbcMu mutex field intended to coordinate ADBC connection reinitialization. |
| adbc.go | Implements auth-error detection, reinit-on-auth-failure retry behavior, and refactors ADBC backoff logic. |
| adbc_reauth_test.go | Adds unit tests for isADBCAuthError behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…on-session-expiry # Conflicts: # adbc.go
- Fix the data race on c.adbcClient: capture the connection once under adbcMu via ensureADBC() and thread it through execADBCWithBackoff / queryADBCWithParams / bindParameters, so queries never re-read c.adbcClient while reinitADBC may be replacing it. reinitADBC returns the fresh connection. - isADBCAuthError now matches both adbc.Error and *adbc.Error before the message fallback (new isADBCAuthStatus helper). - reinitADBC logs a warning when closeADBC() fails instead of discarding it. - Add *adbc.Error (pointer) cases to TestIsADBCAuthError.
lukekim
approved these changes
Jul 13, 2026
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.
Problem
SqlWithParams/QueryWithParamsrun over the ADBC FlightSQL connection, which authenticates only once, when it is opened: the Basic-auth handshake (OptionKeyUsername/OptionKeyPassword) yields a server-side session token that is then reused for every prepared statement on that long-lived connection.If the server invalidates that session — most commonly when it expires after a period of inactivity — every subsequent prepared statement fails:
SqlWithParamsclassifiedUnauthenticatedas abackoff.Permanenterror, so the connection never recovered on its own. The only workaround was to recreate theSpiceClient(i.e. restart the consuming process). The credentials are still valid — it is the cached session that is stale.The plain (non-parameterized)
Sql/Querypath is unaffected because it re-runsAuthenticateBasicTokenon every call, so it was already immune to session expiry.Fix
errors.Asonadbc.Error(StatusUnauthenticated/StatusUnauthorized), with a message-substring fallback for cases where the typed error is not propagated.adbcMumutex plus a staleness check (the connection a caller observed failing), so when many goroutines hit the expired session simultaneously the connection is re-opened at most once and the rest reuse it.execADBCWithBackoff(no behavior change for non-auth errors).Tests
TestIsADBCAuthError(no server required) covering typed, wrapped, fallback, and non-auth cases.go build,go vet,gofmt -l, andgo test -raceon the new logic all pass.taxi_tripsdataset /:8090runtime) that require the CI spiced harness — they fail identically on cleantrunk, unrelated to this change.Release / downstream
Needs a tagged release (e.g.
v8.0.2) so consumers can pick it up; the immediate motivation is a downstream service whose long-lived control-plane ADBC connection went idle past the server's session TTL and then failed every parameterized query until restart.