Conversation
A refresh that failed left the credential on disk whatever the failure was. When the failure was invalid_grant — the grant expired, revoked, or already spent, as every session is when a password changes — the dead token stayed put and went back out on the next command, and the next. The token endpoint's rate limit counts refusals as well as successes and is keyed on the address, so this is self-inflicted: `hey watch` re-authenticates on every ActionCable dial and redials on a fifteen-second timeout, so one dead grant spends the whole hourly allowance in minutes and goes on spending each new one — including the allowance a fresh sign-in needs. The Omarchy bar plugin restarts `hey watch` after any non-auth exit, which kept that going indefinitely. Split the token endpoint's answers into the one that is a verdict on the grant and the rest that are not. invalid_grant on a 4xx clears the credential and reports an auth error, so the next command asks for a login instead of resending a token that will never be accepted. A transport failure, a 5xx, an unparseable body, a different OAuth error code, and a 429 all leave the credential alone — signing someone out over a blip is the worse failure. A 429 is transient, but asking again during one cannot get through and only spends what is left, so it also parks refreshes in this process until the limit can have cleared: Retry-After when the server sends one, fifteen minutes otherwise. While parked, a token that has not actually expired yet is used as-is rather than failing the command — the refresh window opens five minutes early, so it usually still works. Two processes can queue on the credential lock holding the same dead grant, so a re-read that finds the credential gone now stops rather than falling back to the copy it came in with: another process has just had that grant refused.
The CLI's auth strategy runs inside SDK calls, and the SDK returns whatever it hands back untouched. So a credential failure arrives at apierr.FromSDK already classified — but hey.AsError only recognizes the SDK's own error type, and read everything else as a generic API failure. An auth error came out of the envelope as "api" and exit 7, without the hint that says how to fix it. That matters most where the auth exit is what stops a loop. `hey watch` treats a read error as permanent only for usage and auth, and permanentReadError asked the same classifier, so a session the server had ended read as a retryable API error and the watch redialled every fifteen seconds. The Omarchy bar plugin restarts `hey watch` after any non-auth exit, so the wrong code kept it being restarted. Preserve an already-classified CLI error in both places, and report a missing or unusable credential as the auth failure it is rather than a bare error. The messages are unchanged.
|
🤖 All five review comments addressed in a22e869. Replies are on each thread. One change in that commit was not asked for, so flagging it here rather than burying it. The codex pass I run before pushing pointed out that the failed-delete path did not actually uphold the PR's own claim: when the store refused to delete a refused credential, I had it borrow the rate-limit hold, which only postponed the resend by fifteen minutes and did nothing for a fresh process. The refused token is now remembered for the life of the process and not sent again, which is what the delete would have guaranteed. CI is green. |
Review follow-up. errNotAuthenticated builds the message from its cause, so the four call sites that were each spelling out the same Sprintf no longer do. errRefreshHeld returns *apierr.Error like the others rather than a bare error. errRefusedGrant is the one place the invalid_grant sentence is written, with or without a cleanup failure to report. tokenEndpointError drops Description: nothing read it, and the server's error_description already reaches the reader through Body, which the message prints verbatim. Removing the field also removes an accident — a non-string error_description used to fail the whole unmarshal, leaving the code unread and a dead grant treated as transient. The verdict is the error code; what the server puts beside it does not change whether the grant is dead, and a test now pins that for a string, a number, an object and nothing at all. Deleting the credential is what makes a refused grant stop being sent. When the store will not let go of it the credential is still there for the next command to load, so the refused token is now remembered for the life of the process and not sent again. That replaces the rate-limit hold the failed delete used to borrow, which only postponed the resend by fifteen minutes. The transient-failure cases move into TestRefreshFailuresPreserveCredentials rather than sitting in a table of their own. Every status and body is still covered, the credential assertion now checks both tokens, and the rate-limited case additionally pins its classification.
a22e869 to
8527c57
Compare
robzolkos
left a comment
robzolkos
left a comment
There was a problem hiding this comment.
The core invalid_grant handling looks right: the credential is removed under the cross-process refresh lock, while transient failures preserve it. I found two issues that should be addressed in this PR:
-
Preserve classified errors in
hey auth refreshandhey auth token. Both commands currently replace every manager error withErrAuth. A 429 therefore becomescode: auth, exit 3, with “Run: hey auth login,” discarding the newrate_limitclassification and wait hint. Please preserve existing*apierr.Errorvalues and add a command-level 429 regression test. -
Clear the HTTP response cache after automatic credential invalidation. Explicit logout clears cached responses because cached mail must not outlive its credentials. The new
invalid_grantpath deletes the credential directly and bypasses that cleanup. Please connect successful automatic invalidation to the same best-effort cache clearing and test that transient refresh failures clear neither credentials nor cache.
I reproduced both behaviors at the current head: a 429 from hey auth refresh --json exits 3 as auth, and an invalid_grant deletes the credential while leaving a seeded cache body present.
I also noticed three bounded cases that do not prevent this PR from fixing the reported rate-limit loop. I’ll create separate cards for these and tackle them as follow-up work:
- After an established Action Cable connection drops, an auth failure from the reconnect header callback is retried indefinitely by
actioncable-go. The credential has already been deleted, so this does not resend the refresh token, but the watch can remain alive and disconnected instead of exiting with an auth envelope. - If the credential store remains readable but refuses deletion,
refusedRefreshTokensuppresses resubmission only in the current manager. A later process can load and submit the credential once more. - Credential loads do not distinguish absence from operational failures. A temporarily locked keyring is now classified as a permanent auth failure and can stop an established watch with misleading login guidance.
The bug
A refresh that failed left the credential on disk whatever the failure was. When the failure was
invalid_grant— the grant expired, revoked, or already spent, which is every session once a password changes — the dead token stayed put and went back out on the next command, and the next.That is self-inflicted rate limiting. The token endpoint's limit counts refusals as well as successes and is keyed on the address, so a client that keeps presenting a dead grant spends the allowance a fresh sign-in needs.
hey watchre-authenticates on every ActionCable dial and redials on a fifteen-second timeout, and the Omarchy bar plugin restarts it after any non-auth exit — so one password change put a customer in a loop that ran for twelve hours and left them unable to log back in.The split
The whole fix is deciding which refusals are a verdict on the grant and which are not. Dropping credentials on a transient failure would sign people out over a blip, which is worse than the bug.
Definitive — clears the credential:
invalid_granton a 4xx, and nothing else. RFC 6749 §5.2 gives it one meaning — the refresh token is expired, revoked, or already used — and it is the only answer that proves re-sending can never work. The status is checked alongside the code so a 5xx that happens to echo an error code reads as an origin failing, not a grant decision.Transient — credential untouched: transport failures (no answer at all, so no verdict), 5xx, an unparseable body, any other OAuth error code, and 429. An unrecognized failure is transient by construction: the code is only read out of a body that parses, so anything unfamiliar leaves it empty.
Retrying during a 429
A 429 is transient — the server declined to look at the credential, so it says nothing about it — but continuing to ask during one is the other half of the bug. Asking again inside a fixed window cannot get through and only spends what is left of the allowance.
So a 429 parks refreshes in this process until the limit can have cleared:
Retry-Afterwhen the server sends one (clamped to an hour, since that is the whole window), fifteen minutes otherwise. While parked, a token that has not actually expired is used as-is rather than failing the command — the refresh window opens five minutes early, so it is usually still good. Being rate-limited is not being logged out.The hold is per-process and deliberately not persisted. Once the credential is dropped on
invalid_grant, the restart path makes no network call at all, so the loop this fixes never reaches a 429; persisting a hold would mean writing new state onto a working credential to throttle a case the drop already closes.Getting the verdict out
The CLI's auth strategy runs inside SDK calls and the SDK returns its errors untouched, so a credential failure reached
apierr.FromSDKalready classified — andhey.AsErrorrecognizes only the SDK's own type, flattening it toapi/exit 7 without the login hint.permanentReadErrorasked the same classifier, so a session the server had ended read as retryable andhey watchredialled. Both now preserve an already-classified CLI error, and a missing credential is reported as the auth failure it is. Messages are unchanged.This is what makes the fix land: the restarted watch exits with the auth envelope, and the service stops restarting it.
Tests
Regression tests fail without the fix and pass with it — verified by reverting the behaviour while keeping the tests compiling:
TestARefusedGrantIsNeverSentTwice—refresh requests = 5, want 1without the fix. Five dials, five dead-token POSTs, five slots burned.TestRefreshForgetsAGrantTheServerRefused— credential cleared, error codedauth.TestRefreshStopsWhenAnotherProcessForgotTheCredential— the losing process on the lock does not send its stale copy.TestRateLimitedRefreshStopsAskingUntilTheLimitCanHaveCleared/...FallsBackToTheTokenItStillHolds/...HoldHonorsRetryAfter/...SurvivesABodyThatNeverArrives.TestFromSDKKeepsAnAlreadyClassifiedError,TestPermanentReadErrorRecognizesACLIAuthFailure.TestRefreshKeepsCredentialsTheServerNeverJudgedpasses both ways on purpose: it guards the transient side against a future fix that over-broadens the drop.Local:
fmt-check,vet,lint,test,race-test,tidy-check,check-surface,check-release-lockstep,coverage(85.2%, floor 70.8%),gitleaks— all green.ref: https://app.basecamp.com/2914079/buckets/27/card_tables/cards/10303250656