fix(auth): refresh an expiring token before spending it, not after the 401 - #205
Merged
RXWatcher merged 12 commits intoAug 11, 2026
Merged
Conversation
…e 401 Every TokenManager has recorded an access-token deadline at save time since the beginning — `TokenManagerImpl.tokenExpiry`, `EncryptedTokenManagerImpl`'s `tokenExpiryEpochMs`, `TemporaryAuthScope.expiresAtEpochMs` — and no caller has ever read any of them. Expiry was therefore only ever discovered by a 401: the first request past the deadline was sent with a token the server was always going to reject, and the interceptor then refreshed and retried it. The result is correct and invisible, which is why it survived. It is not free. On a live device `/api/v1/home/sections` was 309 × 200 against 42 × 401, every 401 immediately followed by `token refresh required / started / succeeded` — 12% of home loads paying two round trips for one, on the screen whose latency the viewer actually sees. `ws-ticket` shows the same shape at 17/4. `TokenManager` gains `accessTokenExpiresWithin(marginMs)`, defaulting to false so that a manager which cannot answer keeps today's reactive behaviour exactly rather than guessing — a wrong "yes" would spend a refresh token on every request. Both real implementations answer from the deadline they already store, and both exclude the identity they do not own: the in-memory one declines for a temporary overlay it tracks no expiry for, and the Android one answers from the overlay's own deadline rather than falling through to the saved account's. The plugin's 401 path is unchanged. Its refresh body is lifted verbatim into `refreshScopeOnce()` and called from both paths, so the new one cannot drift from it — every guard in there (mid-flight server switch, a sign-out landing while the round trip is open, a dead temporary credential generation) exists because it was needed once, and a second copy would be a second place to forget one. The proactive path is deliberately narrow: authenticated requests on the active scope only, never the auth endpoints themselves, never a pinned outbox op. Verified: :shared:testDebugUnitTest 1015 tests and :android-shared:testDebugUnitTest 1101 tests, both 0 failures — including the existing SiloAuthPluginRefreshFailureTest and SiloAuthPluginPinTest, which are what prove the 401 path still behaves as it did. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 7 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe PR adds access-token lifetime tracking and proactive refresh logic. It centralizes refresh outcomes and concurrency checks, excludes authentication from public endpoints, introduces typed unavailable-auth failures, and retries downloads after retriable authentication failures. ChangesAuthentication refresh and retry flow
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant HttpClient
participant AuthInterceptorImpl
participant TokenManagerImpl
participant AuthServer
HttpClient->>AuthInterceptorImpl: Send authenticated request
AuthInterceptorImpl->>TokenManagerImpl: Check token lifetime
AuthInterceptorImpl->>AuthServer: Refresh near-expiry token
AuthServer-->>AuthInterceptorImpl: Return refresh outcome
AuthInterceptorImpl->>HttpClient: Send request with current credentials
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…udiated bearer Adversarial review of the proactive-refresh change found four hazards, none of which the original tests could have caught. 1. The 60s margin was fixed, so a server issuing tokens shorter than the margin was inside the refresh window from the instant it issued one: every request refreshed and every refresh rotated the refresh token. `shouldRefreshProactively` now clamps the margin to half the token's own lifetime, so a 30s token refreshes at its half-life instead of on every call. Lifetime is recorded and persisted alongside the expiry. 2. A rejected proactive refresh tore the session down and then sent the original request anyway, still carrying the old bearer. The access token often has time left, so the server could honour a write for a session the client had already ended. `refreshScopeOnce` now reports a `RefreshOutcome` and the proactive path strips the repudiated header instead of spending it. 3. The expiry question was answered by whichever identity was installed at check time while the refresh spent the scope captured earlier, so an overlay beginning or ending in between charged one identity's rejection against the other. The generation is now read first and must match the request's scope, and must still match after the check. 4. TemporaryAuthScope.expiresAtEpochMs is the SESSION deadline, but the scoped save overwrote it with the access-token expiry and the new expiry probe read it as one — a four-hour session read as a four-hour token, and every refresh silently extended the guest session. Access-token expiry and lifetime are now their own fields, null until a refresh reports them. Tests: the half-life clamp and both hazards, each mutation-checked (removing the clamp fails the storm tests; removing the header strip fails the repudiation test). shared 1015 -> 1024, all green; android-shared 1101 and androidTvApp 976 unchanged. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Second review round against the previous commit closed all four original findings but raised three against the fix itself. Stripping only the bearer still SENT the request. An optionally-authenticated endpoint would accept the anonymous remainder, so a repudiated write could still land — and the profile headers were left attached besides. Drop every credential header and fail the call with `silo_auth_credentials_repudiated`, matching what a pinned request with no usable token already does. The test now uses a write against a deliberately permissive endpoint and asserts the call never reaches the server, rather than asserting a header was absent from a request that still went out. An unknown lifetime now stays reactive rather than falling back to the full margin. Credentials stored before this field existed load without one, and because a 5xx refresh never persists a lifetime, the old fallback would retry a proactive refresh on every request for the length of an outage. They migrate on the next successful issuance. An already-expired token stays due either way — there is nothing left to conserve. Temporary playback keeps its access-token lifetime: DeviceLoginPollResponse does carry expires_in, so the previous comment claiming otherwise was wrong and the overlay was needlessly reactive-only until its first 401. shared 1024, android-shared 1101, androidTvApp 976, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…c reads Third review round closed all three of the previous round's findings and raised two more against the throw itself. A repudiated session mid-download was permanently failing the download. DownloadWorker calls Ktor directly and classifies 401 as retriable, but the new exception fell into its generic `catch (e: Throwable)`, which deletes the partial and marks the download Failed — potentially gigabytes discarded because a refresh token expired. The classification is now a named predicate, `downloadAuthFailureIsRetriable`, so it is unit-testable without a worker harness, and it is deliberately narrower than the IllegalStateException that SiloAuthUnavailableException extends: widening it that far would make a genuine 404 retry forever. Failing the request also punished endpoints that never needed the bearer. /health and the relative setup/signup-status calls do not opt out of auth, so the header is merely attached globally; with a live access token they would have answered fine. The rule now matches the harm: credentials always come off, and only unsafe methods are blocked. An anonymous write could be accepted as anonymous; an anonymous read either works or 401s exactly as before. Both sentinels are now a typed SiloAuthUnavailableException rather than a bare IllegalStateException carrying a magic string, so callers can classify them without matching on message text. Also confirmed by review rather than assumed: the throw happens inside Send before proceed(), so no engine connection is ever acquired and the refresh mutex has already unwound; pinned outbox calls never enter the proactive path; and safeApiCall maps both sentinels to ApiResult.NetworkError. shared 1025, android-shared 1103, androidTvApp 976, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…diation rule
Fourth review round closed both previous findings but rejected the method-based
fallback, correctly.
Two things I asserted were wrong. "Safe methods do not change server state" is
false in this codebase: GET /downloads/{id}/file moves the download to
completed server-side, and GET /admin/stats?refresh=true forces a recompute.
And an anonymous GET is not "a 401 exactly as before" — an optionally
authenticated read can return GUEST data with a 200 that callers accept and
cache while sessionExpired is signing the user out. That is a worse outcome
than the 401 it replaced.
So the rule goes back to strict: a repudiated session sends nothing, whatever
the method. The public endpoints that motivated the exception are fixed at the
root instead — /health and the RELATIVE setup/signup-status calls now
skipSiloAuth(), matching their explicit-server twins which already did. Opted
out, they never carry a bearer, never enter the proactive path, and cannot be
failed by a dead session elsewhere. This also restores the download GET to
throwing, so downloadAuthFailureIsRetriable covers a path production actually
takes rather than documenting a dead one.
Separately: a transient proactive failure (5xx, gateway, dropped connection) is
now RefreshOutcome.FailedTransient and suppresses the reactive retry for that
request. One request was producing two refresh attempts against a refresh
service already failing, and concurrent traffic amplified the outage.
shared 1027, android-shared 1103, androidTvApp 976, androidApp 591, all green.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… request Round 5 closed the previous four findings and left one real defect, in the suppression added by the last commit. Returning early on a transient proactive failure skipped more than the network call: it skipped refreshScopeOnce's double-check. So if request A's proactive refresh failed transiently while request B's succeeded and installed a new token, A surfaced a stale 401 even though working credentials were already sitting there — and recovering needed no network call at all, just the check that was being bypassed. The suppression is now a parameter on refreshScopeOnce rather than an early return at the call site. Everything before the network POST still runs, including the already-rotated check; only the POST itself is skipped. Test covers the concurrent-rotation case and is mutation-checked: restoring the early return fails it. The single-request cost stands as intended — one genuinely expired token surfaces a 401 after a transient failure and recovers on a later request, which is the point of not hammering a failing refresh service. shared 1028, android-shared 1103, androidTvApp 976, androidApp 591, all green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
refreshScopeOnce's KDoc still documented a boolean return, and the CredentialsDead branch said 'a 401 is the honest answer' when it throws without sending anything. Also documents allowNetworkRefresh. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt (1)
273-276: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winRethrow
CancellationExceptionand record the swallowed cause.
catch (e: Throwable)also catchesCancellationException. If the caller's coroutine is cancelled while the refresh POST is in flight, this returnsFailedTransientinstead of propagating cancellation. TheSendhook then continues and issues the original request on a cancelled coroutine. The capturedeis also discarded, which is what detekt reports.♻️ Proposed fix
- } catch (e: Throwable) { - diagnosticsObserver.safeAuthRefresh("failed") - RefreshOutcome.FailedTransient - } + } catch (e: CancellationException) { + throw e + } catch (e: Throwable) { + diagnosticsObserver.safeAuthRefresh("failed: ${e::class.simpleName}") + RefreshOutcome.FailedTransient + }Add
import kotlinx.coroutines.CancellationException.Note: the pinned path at Line 477 has the same shape. Fix both or neither, to keep one behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt` around lines 273 - 276, Update both refresh exception handlers in AuthInterceptorImpl, including the pinned-path handler, to rethrow CancellationException before converting other failures to RefreshOutcome.FailedTransient. Preserve safeAuthRefresh("failed") for non-cancellation errors and record the caught exception through the existing diagnostics mechanism instead of discarding e.Source: Linters/SAST tools
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@shared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.kt`:
- Around line 273-276: Update both refresh exception handlers in
AuthInterceptorImpl, including the pinned-path handler, to rethrow
CancellationException before converting other failures to
RefreshOutcome.FailedTransient. Preserve safeAuthRefresh("failed") for
non-cancellation errors and record the caught exception through the existing
diagnostics mechanism instead of discarding e.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: ef12e208-73bf-4cf7-be45-fe679668dd0e
📒 Files selected for processing (14)
android-shared/src/androidMain/kotlin/org/siloserver/silo/common/downloads/DownloadWorker.ktandroid-shared/src/androidUnitTest/kotlin/org/siloserver/silo/common/downloads/DownloadWorkerHttpStatusTest.ktandroidTvApp/src/androidMain/kotlin/org/siloserver/silo/tv/cast/RemotePlaybackIdentityManager.ktshared/src/androidMain/kotlin/org/siloserver/silo/network/EncryptedTokenManagerImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/AuthInterceptorImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/ProactiveRefreshPolicy.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/SiloAuthUnavailableException.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManager.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/TokenManagerImpl.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/api/AuthApi.ktshared/src/commonMain/kotlin/org/siloserver/silo/network/api/HealthApi.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/ProactiveRefreshPolicyTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginProactiveRefreshHazardTest.ktshared/src/commonTest/kotlin/org/siloserver/silo/network/SiloAuthPluginProactiveRefreshTest.kt
Post-merge review against the new main. Silo-Server#192 added BrandingApi as the PRIMARY source of a server's display name, replacing checkHealth() in the same AuthRepository path — but it does not skipSiloAuth(), so it carries a bearer it never needed. Before this stack that was harmless. It is not harmless now: a repudiated session makes the proactive path throw before branding reaches the server, safeApiCall turns that into a NetworkError, and AuthRepository falls back to health — restoring the compatibility-backed name that Silo-Server#192 exists to stop using. Opening the TV server list is enough to trigger it. The regression is introduced by this stack, so it belongs in this stack rather than a follow-up. Opted out exactly like its sibling. Test drives both probes through a repudiated session and asserts neither fails and neither carries a bearer; mutation-checked by removing the opt-out. shared 1067, android-shared 1134, androidTvApp 993, androidApp 606, green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
RXWatcher
force-pushed
the
fix/auth-refresh-before-expiry
branch
from
August 11, 2026 04:37
ea98c4a to
9f37044
Compare
Pre-merge gate against the new main found a race that breaks this stack's central invariant. A request captures its bearer before it waits on the refresh mutex. A concurrent sign-out, server switch, or repudiation in that window all make refreshScopeOnce return NotAttempted — which says only "no refresh happened", not "the scope is still alive". The proactive path read that as permission to continue and sent the request with the bearer it had captured, so an invalidated credential could still be spent after another request had already torn the session down. Rather than enumerate every outcome that can mean a dead scope, the caller now checks the invariant directly at the only point it matters: immediately before proceed(). If nothing is installed, or the active server has changed, the request is dropped and fails like any other repudiated one. If the credentials were merely rotated by another coroutine, the request spends the token that is actually installed instead of the stale capture. Applied to every non-Refreshed outcome, including FailedTransient — that case is meant to spend the existing credentials, but only if they still exist. Tests cover both halves and are mutation-checked: neutering the guard fails both. shared 1069, android-shared 1138, androidTvApp 993, androidApp 606. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
1 participant
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.
The bug
Every
TokenManagerhas recorded an access-token deadline at save time since the beginning —TokenManagerImpl.tokenExpiry,EncryptedTokenManagerImpl.tokenExpiryEpochMs,TemporaryAuthScope.expiresAtEpochMs— and no caller has ever read any of them. Expiry was only ever discovered by a 401: the first request past the deadline is sent with a token the server was always going to reject, and the interceptor then refreshes and retries it.The outcome is correct and invisible, which is why it survived. It is not free.
Measured on a live device:
GET /api/v1/home/sectionsPOST /api/v1/events/ws-ticketEvery one of those 401s is immediately followed by
token refresh required → started → succeeded. That is 12% of home loads paying two round trips for one, on the screen whose latency the viewer actually sees.I checked the obvious alternative explanation and it does not hold:
EncryptedTokenManagerImpl.initdoesrunBlocking { reloadCacheLocked() }, so the manager is never observable without its cache and this is not a cold-start race. It is ordinary expiry.The change
TokenManagergains:Defaulting to false so a manager that cannot answer keeps today's reactive behaviour exactly rather than guessing — a wrong "yes" would spend a refresh token on every request. Both real implementations answer from the deadline they already store, and both exclude the identity they do not own: the in-memory one declines for a temporary overlay it tracks no expiry for; the Android one answers from the overlay's own deadline rather than falling through to the saved account's.
The 401 path is unchanged. Its refresh body is lifted verbatim into
refreshScopeOnce()and called from both paths, so the new path cannot drift from it — every guard in there (mid-flight server switch, a sign-out landing while the round trip is open, a dead temporary credential generation) exists because it was needed once, and a second copy would be a second place to forget one.The proactive path is deliberately narrow: authenticated requests on the active scope only, never the auth endpoints themselves (refreshing before a login is meaningless and before a refresh is recursive), never a pinned outbox op. Margin is 60s — wide enough for the round trip plus clock skew, narrow enough not to dominate a short token lifetime.
Verification
:shared:testDebugUnitTest— 1015 tests, 0 failures:android-shared:testDebugUnitTest— 1101 tests, 0 failuresIncluding the pre-existing
SiloAuthPluginRefreshFailureTestandSiloAuthPluginPinTest, which are what prove the 401 path still behaves as it did. NewSiloAuthPluginProactiveRefreshTestdrives the realTokenManagerImpl(not a fake) and asserts the refresh precedes the call, that a healthy token costs one round trip, and that a signed-out client refreshes nothing.Summary by CodeRabbit
New Features
Bug Fixes
Tests