Skip to content

Give the turn back when a strategy panics, take a provider that cannot hand over a token for the refresh failing, and tell a cut-off waiter its context ended - #212

Merged
robzolkos merged 1 commit into
mainfrom
feature/go-refresh-edges
Sep 16, 2026
Merged

robzolkos merged 1 commit into
mainfrom
feature/go-refresh-edges

Conversation

@robzolkos

@robzolkos robzolkos commented Sep 16, 2026

Copy link
Copy Markdown
Collaborator

Three edges of the refresh coordination #206 added, found by an independent review of that PR after it merged. The core generation logic, lock handling and ordering held up; these are the paths around it.

A strategy that panics kept the turn. A signing gave the turn back only when the strategy returned. The turn is shared by every client on the root, so after one panic every later signing and every refresh waited on it until its own context ended, and a request with no deadline waited for good. The turn is now given back on the way out, and the panic goes on to the caller unchanged. Kotlin's and Rust's locks already release on unwind; this brings Go in line.

A provider that cannot hand over a token was asked to refresh on top. Before a refresh is spent the provider is asked what it would sign with. When AccessToken failed, that was treated as no answer and Refresh was called straight after, so a provider whose own renewal inside AccessToken had just failed was made to try again at once. That failure is now the refresh's answer, shared with every request signed under the same credentials like any other failed refresh, as Kotlin (#198) and Rust (#211) take it.

A cut-off waiter was told it was refused. A request whose context ended while it waited on the refresh its 401 had joined was answered not renewed, which the hand-written send paths surfaced as "Authentication failed", reporting a deadline as an auth refusal. It now gets its context's error, as a request refused before signing already did, and the refresh carries on for the requests still waiting. The generated client's refresher hook only answers whether to resend, so on that path the 401 remains the answer; the godoc says so.

TokenRefresher godoc and go/README.md describe all three. New tests cover the panic releasing the turn and the failing provider; the cancelled-waiter test now expects the context error. The refresh tests pass 20 times under -race. Public Go API unchanged.


Summary by cubic

Fixes three edges of the refresh coordination in the Go SDK: a panicking strategy now releases the turn, a provider that can't hand over a token ends the refresh as failed, and a request cut off by its context gets its context's error instead of an auth refusal.

Details

  • The turn is released via a deferred call, so the panic still reaches the caller.
  • The provider's failure is treated as the refresh's answer and shared across all requests under the same credentials.
  • The cancelled waiter returns ctx.Err() and leaves the refresh running for other waiters.

Written for commit d2f360f. Summary will update on new commits.

Review in cubic

…t hand over a token for the refresh failing, and tell a cut-off waiter its context ended

A signing gave the turn back only when the strategy returned. One that panicked kept it,
and the turn is shared by every client on the root: every later signing and every
refresh waited on it until its own context ended, and a request with no deadline waited
for good. The turn is given back on the way out now, and the panic goes on to the caller
as it was.

Before a refresh is spent the provider is asked what it would sign with. A provider that
could not hand over any token was taken as having nothing to say, and the refresher was
asked straight after, so one whose own renewal inside AccessToken had just failed was
made to try again at once. That failure is the refresh's answer now, shared with every
request signed under the same credentials, as Kotlin and Rust take it.

A request whose context ended while it waited on the refresh its 401 had joined was
answered not renewed, and the hand-written send paths surfaced that as "Authentication
failed" — a deadline reported as a refusal. It gets its context's error now, as a request
refused before signing already did; the refresh goes on for the requests still waiting.
The generated client's refresher hook answers only whether to resend, so there the 401
is still the answer.
Copilot AI balanced review requested due to automatic review settings September 16, 2026 15:48

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

🟢 Approved

The implementation consistently addresses the three documented refresh-coordination edge cases with focused tests.

Pull request overview

Fixes three edge cases in Go credential-refresh coordination: panic-safe signing, provider token lookup failures, and cancellation-aware waiters.

Changes:

  • Releases the shared signing turn when an authentication strategy panics.
  • Treats AccessToken errors as refresh failures without calling Refresh again.
  • Returns context errors from hand-written request paths when refresh waiters are cancelled, with updated tests and documentation.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries
File Description
go/README.md Documents refresh cancellation and provider behavior.
go/pkg/hey/credential_refresh.go Makes signing panic-safe and propagates waiter context errors.
go/pkg/hey/credential_refresh_test.go Covers panic recovery, provider failure, and cancellation behavior.
go/pkg/hey/client.go Surfaces context errors and handles provider lookup failures.
go/pkg/hey/auth_strategy.go Updates TokenRefresher API documentation.
Review details
  • Files reviewed: 5/5 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@robzolkos
robzolkos merged commit d4a2eb5 into main Sep 16, 2026
32 checks passed
@robzolkos
robzolkos deleted the feature/go-refresh-edges branch September 16, 2026 16:41
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