Count the 401 refresh replay against the attempt budget, and amend SPEC §4 to match - #571
Conversation
…EC §4 to match
`max_retries: 0` documented one attempt and sent two. A refreshable 401 put a
second request on the wire from inside the single-request primitive, governed
by its own counter rather than the caller's budget — so the promise §14 makes
for downloads ("disabling retry yields exactly ONE hop-1 attempt") was false in
Python and Ruby, and the total-attempt semantics #461 settled leaked.
Counting the replay was tried once on #563 and reverted, correctly: on its own
it regresses SPEC §4, which says refresh is attempted once PER REQUEST, tracked
"with a boolean rather than a counter". Gated only by "have we already
refreshed", `max_retries: 1` refreshed the token and then rethrew the stale 401
without ever sending the refreshed request — for every GET, not just downloads.
That was worse than the bug.
So this lands both halves together, which is what makes it a decision rather
than a drift. The replay counts against the budget, AND §4 is amended to
attempt the refresh only when another attempt remains. The gate is checked
BEFORE refresh() rather than after: rotating a token the SDK has no budget left
to use burns it for nothing and hands the caller the stale 401 anyway, so
declining to refresh is both cheaper and easier to state. This supersedes §4's
unqualified per-request reading and closes #565.
The consequence is stated plainly in §4 rather than left to be discovered: with
a budget of one attempt, a refreshable 401 is NOT replayed and surfaces as
auth_required. Callers who want the replay must leave an attempt for it.
Scope is Python (sync and async) and Ruby — the two SDKs whose transports carry
a 401 replay under a governed budget. Go's main GET loop already counts it: a
401 with a successful refresh returns a retryable error that the loop re-issues
as the next attempt, subject to MaxRetries. Its hand-written mutation path
replays outside any budget, but mutations have no transient-retry budget to
draw from — that is the documented divergence in §7, and §4's new gate says
explicitly that it binds where a total-attempt budget governs the path.
TypeScript, Kotlin and Swift have no 401 replay in the transport at all.
Direct single-request callers keep the in-primitive replay. Python's and Ruby's
mutations bypass the retry loop entirely, so without the seam they would lose
401 refresh outright; `refresh_replay:` marks which side owns it. In Ruby that
also stops handle_error from rotating credentials as a side effect of
classifying an error — the refresh now happens where the budget is known.
Red-proofed against merged main on the plain GET path as well as the download
hop, since the every-GET case is exactly what the earlier revert protected:
tests/test_http.py::TestRefreshReplayAttemptBudget::test_no_budget_means_one_request_and_no_refresh
E AssertionError: assert 2 == 1
tests/test_http.py::TestRefreshReplayAttemptBudget::test_replay_and_transient_retries_share_one_budget
tests/test_download.py::TestHop1Retry::test_refresh_replay_is_not_attempted_without_budget
and in Ruby:
The request GET https://3.basecampapi.com/test.json was expected to execute 1 time but it executed 2 times
The request GET https://3.basecampapi.com/test.json was expected to execute 2 times but it executed 3 times
The request GET .../download/file.txt was expected to execute 1 time but it executed 2 times
The middle one is the load-bearing case: at `max_retries: 2` the replay used to
ride outside the cap for a total of three requests. Positive tests pin that the
refreshed request is actually SENT rather than refreshed-then-discarded, and
that mutations keep their replay.
jeremy
added
bug
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c2a2aed778
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…int still retries An exception raised inside an `except` suite is not offered to that `try`'s sibling handlers — same in Ruby for `rescue`. Calling refresh() from inside the AuthError handler therefore put the token endpoint outside the retry loop's reach: a NetworkError from a timing-out refresh escaped the whole loop and ended the request with budget still unspent. Before this branch the replay lived in the single-request primitive, where that failure landed in the transient handler and retried, so this was a regression introduced by moving it. Both loops now follow the shape the Swift and Kotlin download loops already use (#517): the handlers only CLASSIFY the attempt, and every side effect runs at the loop tail, outside any handler. A refresh that raises is classified as a transient failure of this attempt and retries under the same budget; a refresh that returns false surfaces the original 401; a refresh that succeeds replays immediately with no backoff, since the token is fresh and the server never asked us to wait. Red-proofed against the previous commit in both languages — the refresh's NetworkError propagated raw out of `client.get(...)` rather than being retried: E basecamp.errors.NetworkError: token endpoint timed out FAILED tests/test_http.py::TestRefreshReplayAttemptBudget::test_refresh_network_failure_still_retries_under_the_budget
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5d44c0f59f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
SPEC §4 tracks refresh with an "attempted" boolean — the algorithm's own
condition is "refresh has not yet been attempted for this request" — but the
flag was set only after a refresh returned true. A refresh that RAISES left it
false, so the next 401 in the same request called the provider again.
Reachable as soon as the token endpoint is flaky: attempt 1 401s, refresh
throws a NetworkError, the attempt retries under the budget, attempt 2 401s
with the same unchanged token, and refresh fires a second time. Beyond
violating the at-most-once rule, that is unsafe with a rotating refresh token —
if the first call reached the server and rotated before its response was lost,
the second spends a credential that is already dead.
All three paths now mark the attempt before invoking the provider. Red-proofed
against the previous commit, with a provider that always throws and a server
that always 401s:
> assert provider.refreshes == 1
E assert 2 == 1
The existing network-failure tests missed this because their second request
returns 200, so no second 401 ever arrives to trigger the second refresh.
|
Merging on green CI with a reviewer status note, per the repo's documented fallback when the bots do not deliver at the final head. Convergence check at Both threads are Codex P2 findings, both real, both fixed and answered with red proofs — the Reviewer coverage. Copilot errored on all three rounds ("Copilot encountered an error and was unable to review this pull request"). Codex reviewed Verification at the merged head.
Conformance: Go 135/0/2, Kotlin 136/0/1, TypeScript 157 passed / 2 skipped, Ruby 126/0/11, Python 137/0/0. CI is CLEAN on every required check. |
Closes #565.
max_retries: 0documents exactly one attempt and sends two. A refreshable 401 puts a second request on the wire from inside Python's_single_requestand Ruby'ssingle_request, governed by its own_retry_count/retry_countrather than the caller's budget. SPEC §14 makes the same promise for the download hop — "disabling retry yields exactly one hop-1 attempt" — so the contradiction was written down in two places and shipping in both.Why this is a decision, not a re-application
Counting the replay was tried once on #563 (
b9662222d) and reverted (aca2c481b). The revert was right. On its own, counting regresses SPEC §4, which says refresh is attempted once per request, tracked "with a boolean (e.g.refresh_attempted) rather than a counter". Gated only by "have we already refreshed",max_retries: 1refreshed the token and then rethrew the stale 401 without ever sending the refreshed request — for every GET in Python and Ruby, not just downloads. That is worse than the bug it fixed, and #565 was filed rather than smuggling the decision in.The resolution needs both halves in the same change:
The second is what stops the first from regressing §4. With it, there is no state in which the SDK refreshes and then discards the result.
The SPEC §4 wording
Step 2 gains a clause, and the section gains three paragraphs:
§14 needs no change: its per-SDK budget table already reads "
max_retriesas total attempts, floored at one (max_retries: 0still sends one attempt)". It is simply true now.Scope: which SDKs actually have this
Checked all six rather than assuming.
_single_request, own countersingle_request, own counterMaxRetriesdoRequestURL's non-GET arm replays outside any budgetDirect single-request callers keep the in-primitive replay. Python's and Ruby's mutations bypass the retry loop entirely, so without a seam they would lose 401 refresh outright; a
refresh_replay:parameter marks which side owns it. In Ruby that also stopshandle_errorfrom rotating credentials as a side effect of classifying an error — the refresh now happens where the budget is known.Red proofs
Against merged
main, withgit show origin/main:<path>swapped in. Both the plain GET path and the download hop, because the every-GET case is exactly what the earlier revert protected — it is covered by a test here rather than by absence.Python —
uv run pytest tests/test_http.py -k RefreshReplay tests/test_download.py -k refresh_replayagainst pristine_http.py:Ruby — against pristine
http.rb:test_401_refresh_replay_shares_the_budget_with_transient_retries— "expected 2, executed 3" — is the load-bearing one: atmax_retries: 2a 401 (attempt 1, refresh) followed by a 503 should exhaust the cap at two requests, and the replay used to ride outside it for three.The remaining new tests pass against
mainby construction and pin the ceiling rather than the floor: that the refreshed request is actually sent (a 200 body, not a rethrown 401), that only one refresh happens per request, and that mutations keep their in-primitive replay.Review round
Codex P2 — a refresh network failure escaped the retry loop. Real, and a regression this branch introduced: an exception raised inside an
exceptsuite is not offered to thattry's sibling handlers (Ruby'srescuebehaves the same), so callingrefresh()from theAuthErrorhandler put the token endpoint outside the loop's reach. A timing-out refresh ended the request with budget unspent, where before the move it landed in the transient handler and retried.All three paths now use the directive shape the Swift and Kotlin download loops adopted in #517: the handlers only classify the attempt, and every side effect runs at the loop tail, outside any handler. A refresh that raises is a transient failure of that attempt and retries under the same budget; one that returns false surfaces the original 401; one that succeeds replays immediately with no backoff, since the token is fresh and the server never asked us to wait. Red-proofed in Python and Ruby against the previous commit — the
NetworkErrorpropagated raw out ofclient.get(...)with two attempts still available.Verification
make checkgreen end to end (REAL_EXIT=0, grep-checked — one run reported "All checks passed!" mid-stream while exiting 2 on a laterruff formatstep):okall packagesConformance: Go 135/0/2, Kotlin 136/0/1, TypeScript 157 passed / 2 skipped, Ruby 126/0/11, Python 137/0/0.
One flake seen locally and not reproducible:
tests/client.test.ts > request timeout > propagates a caller-supplied signal through the combined signalfailed once in a localmake checkand passed 3/3 on re-run. This branch changes no TypeScript files at all (git diff origin/main...HEAD -- typescript/is empty), and the TypeScript CI job is green. Cited numbers are the CI job's.One aside for whoever hits it next:
make go-checkfirst failed here with 557 phantom issues in../../validation-errors/go/pkg/generated/client.gen.go— a golangci-lint cache shared across worktrees.golangci-lint cache cleancleared it; 0 issues after.