Repository navigation
fix(host-service): refresh stale auth token on remote workspace ops - #4106
Merged
Merged
Conversation
Workspace create/delete on remote hosts surfaced "Failed to register host: Not authenticated" once the host-service's session token aged out. Two layered causes, both inside the auth provider: 1. JwtApiAuthProvider held the session token captured at child-process spawn time as a `private readonly` field, so re-logins in the desktop renderer (which rewrite auth-token.enc) were never observed. 2. The cached JWT had no error-driven invalidation — once the cloud started rejecting it (clock skew, JWKS rotation, mid-flight session revoke) every retry served the same stale value. Provider now takes a `getSessionToken: () => Promise<string>` loader that's consulted on each cache miss, so token rotations are picked up without restarting the host-service. Adds `invalidateCache()` and a one-shot retry-on-UNAUTHORIZED tRPC link in `createApiClient` that drops the cache and retries when the cloud rejects. Desktop entry's loader reads `auth-token.enc` from disk; standalone `serve.ts` keeps the env-var path. Verified end-to-end against prod api.superset.sh: poisoning the cached JWT triggers a real 401, the link invalidates and re-mints, and the request succeeds.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR implements automatic retry on 401 unauthorized responses by refactoring authentication providers to support dynamic token retrieval and adding a cache invalidation mechanism. A new retry link intercepts 401 errors, clears cached credentials, and retries the request once. ChangesAuthentication & Retry Flow
Sequence DiagramsequenceDiagram
participant Client as TRPC Client
participant RetryLink as Retry Link
participant AuthProvider as Auth Provider
participant API as API Server
Client->>RetryLink: Make request
RetryLink->>API: Forward request (with cached JWT)
API-->>RetryLink: 401 Unauthorized
RetryLink->>AuthProvider: invalidateCache()
AuthProvider->>AuthProvider: Clear JWT cache
RetryLink->>AuthProvider: getHeaders()
AuthProvider->>AuthProvider: getSessionToken() [fresh]
AuthProvider-->>RetryLink: New JWT
RetryLink->>API: Retry request (with fresh JWT)
API-->>RetryLink: 200 Success
RetryLink-->>Client: Forward response
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
Contributor
🧹 Preview Cleanup CompleteThe following preview resources have been cleaned up:
Thank you for your contribution! 🎉 |
Contributor
Greptile SummaryThis PR fixes stale-auth failures on remote workspace operations by making
Confidence Score: 4/5The retry and cache-invalidation mechanism is correct end-to-end; the fix addresses the stated stale-token failure cleanly. Two non-blocking issues exist: the OAuth JWT passthrough path never populates cachedJwt so the desktop loader incurs a disk read on every getJwt() call rather than only on cache misses, and the retry link reassigns subscription without an explicit prior unsubscribe, which is safe today but leaves a subtle timing edge under concurrent cancellation. Neither blocks correctness of the main fix. JwtAuthProvider.ts (JWT passthrough caching) and createApiClient.ts (subscription teardown in retry link) warrant a second look.
|
| Filename | Overview |
|---|---|
| packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.ts | Refactored to accept a lazy getSessionToken loader; adds invalidateCache(). JWT passthrough path (looksLikeJwt) now skips populating cachedJwt, causing a disk read per API call for OAuth JWT users. |
| packages/host-service/src/api/createApiClient/createApiClient.ts | Adds a one-shot retryOnUnauthorizedLink that calls invalidateCache() and retries on 401; subscription variable is reassigned on retry without explicit prior unsubscribe. |
| apps/desktop/src/main/host-service/index.ts | Wires getSessionToken to read auth-token.enc from disk on each cache miss, with env.AUTH_TOKEN as a fallback. |
| packages/host-service/src/providers/auth/types.ts | Adds invalidateCache() to the ApiAuthProvider interface with clear JSDoc. |
| packages/host-service/src/providers/auth/DeviceKeyAuthProvider/DeviceKeyAuthProvider.ts | Adds no-op invalidateCache() to satisfy the updated interface; correct for device keys that do not expire. |
| packages/host-service/src/serve.ts | Updates standalone serve.ts to use the new options-object constructor with a trivial getSessionToken that wraps the env var. |
Sequence Diagram
sequenceDiagram
participant C as Caller
participant R as retryOnUnauthorizedLink
participant H as httpBatchLink
participant P as JwtApiAuthProvider
participant D as disk (auth-token.enc)
participant API as Cloud API
C->>R: op (e.g. host.list)
R->>H: next(op) [attempt 1]
H->>P: getHeaders()
P->>D: getSessionToken() [cache miss]
D-->>P: stale session token
P->>API: POST /api/auth/token
API-->>P: JWT (now stale)
P-->>H: Authorization: Bearer stale-jwt
H->>API: tRPC request
API-->>H: 401 UNAUTHORIZED
H-->>R: error(UNAUTHORIZED)
R->>P: invalidateCache()
R->>H: next(op) [attempt 2 - retry]
H->>P: getHeaders()
P->>D: getSessionToken() [cache cleared]
D-->>P: fresh session token
P->>API: POST /api/auth/token
API-->>P: new JWT
P-->>H: Authorization: Bearer fresh-jwt
H->>API: tRPC request
API-->>H: 200 OK
H-->>R: value
R-->>C: value
Prompt To Fix All With AI
Fix the following 2 code review issues. Work through them one at a time, proposing concise fixes.
---
### Issue 1 of 2
packages/host-service/src/api/createApiClient/createApiClient.ts:29-42
**Previous subscription not unsubscribed before retry reassignment**
When a 401 fires, the error handler reassigns `subscription` via `start()` without first calling `subscription?.unsubscribe()` on the still-live (erroring) reference. In tRPC's observable model an errored subscription is terminal and won't emit further, so no events leak — but if the outer teardown (`() => subscription?.unsubscribe()`) is invoked by the tRPC runtime at the same instant the error handler is mid-execution (e.g. request cancellation racing with a 401 response), the cleanup function would already point to the old subscription, leaving the newly started retry un-cancellable. Explicitly unsubscribing the previous handle before `start()` removes the ambiguity.
### Issue 2 of 2
packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.ts:58-60
**OAuth JWT passthrough never populates `cachedJwt`**
When `sessionToken` passes `looksLikeJwt`, the value is returned immediately without assigning `this.cachedJwt`. Because `cachedJwt` stays `null`, the top-of-function cache check never hits for OAuth JWT users, so `getSessionToken()` (a disk read in the desktop entry) is called on every `getJwt()` invocation. Previously, `this.sessionToken` was an in-memory field. For a low-frequency desktop tool this is unlikely to surface, but populating `cachedJwt` on this path would restore parity while still allowing `invalidateCache()` to force a disk re-read when a re-login is detected.
Reviews (1): Last reviewed commit: "fix(host-service): refresh stale auth to..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+29
to
+42
| const start = () => { | ||
| subscription = next(op).subscribe({ | ||
| next: (value) => observer.next(value), | ||
| error: (err) => { | ||
| if (!attempted && isUnauthorizedError(err)) { | ||
| attempted = true; | ||
| authProvider.invalidateCache(); | ||
| start(); | ||
| return; | ||
| } | ||
| observer.error(err); | ||
| }, | ||
| complete: () => observer.complete(), | ||
| }); |
Contributor
There was a problem hiding this comment.
When a 401 fires, the error handler reassigns subscription via start() without first calling subscription?.unsubscribe() on the still-live (erroring) reference. In tRPC's observable model an errored subscription is terminal and won't emit further, so no events leak — but if the outer teardown (() => subscription?.unsubscribe()) is invoked by the tRPC runtime at the same instant the error handler is mid-execution (e.g. request cancellation racing with a 401 response), the cleanup function would already point to the old subscription, leaving the newly started retry un-cancellable. Explicitly unsubscribing the previous handle before start() removes the ambiguity.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/api/createApiClient/createApiClient.ts
Line: 29-42
Comment:
**Previous subscription not unsubscribed before retry reassignment**
When a 401 fires, the error handler reassigns `subscription` via `start()` without first calling `subscription?.unsubscribe()` on the still-live (erroring) reference. In tRPC's observable model an errored subscription is terminal and won't emit further, so no events leak — but if the outer teardown (`() => subscription?.unsubscribe()`) is invoked by the tRPC runtime at the same instant the error handler is mid-execution (e.g. request cancellation racing with a 401 response), the cleanup function would already point to the old subscription, leaving the newly started retry un-cancellable. Explicitly unsubscribing the previous handle before `start()` removes the ambiguity.
How can I resolve this? If you propose a fix, please make it concise.
Comment on lines
+58
to
60
| if (looksLikeJwt(sessionToken)) { | ||
| return sessionToken; | ||
| } |
Contributor
There was a problem hiding this comment.
cachedJwt
When sessionToken passes looksLikeJwt, the value is returned immediately without assigning this.cachedJwt. Because cachedJwt stays null, the top-of-function cache check never hits for OAuth JWT users, so getSessionToken() (a disk read in the desktop entry) is called on every getJwt() invocation. Previously, this.sessionToken was an in-memory field. For a low-frequency desktop tool this is unlikely to surface, but populating cachedJwt on this path would restore parity while still allowing invalidateCache() to force a disk re-read when a re-login is detected.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/host-service/src/providers/auth/JwtAuthProvider/JwtAuthProvider.ts
Line: 58-60
Comment:
**OAuth JWT passthrough never populates `cachedJwt`**
When `sessionToken` passes `looksLikeJwt`, the value is returned immediately without assigning `this.cachedJwt`. Because `cachedJwt` stays `null`, the top-of-function cache check never hits for OAuth JWT users, so `getSessionToken()` (a disk read in the desktop entry) is called on every `getJwt()` invocation. Previously, `this.sessionToken` was an in-memory field. For a low-frequency desktop tool this is unlikely to surface, but populating `cachedJwt` on this path would restore parity while still allowing `invalidateCache()` to force a disk re-read when a re-login is detected.
How can I resolve this? If you propose a fix, please make it concise.
saddlepaddle
added a commit
that referenced
this pull request
May 5, 2026
Changes since v0.2.8: - `superset agents list` (and demoted "presets" to a UI-only concept). CLI now reads canonical agents from host-service. (#4097) - Host-service: refresh stale OAuth access tokens on remote workspace ops instead of failing the request. (#4106) - Host-service: workspace.create now adopts an existing worktree at any path, not just the canonical one. (#4096) - Host-service: AI workspace naming times out after 5s and falls back to a deterministic name. (#4102) Push cli-v0.2.9 after this lands to fire the release pipeline.
saddlepaddle
added a commit
that referenced
this pull request
May 6, 2026
…4106) Workspace create/delete on remote hosts surfaced "Failed to register host: Not authenticated" once the host-service's session token aged out. Two layered causes, both inside the auth provider: 1. JwtApiAuthProvider held the session token captured at child-process spawn time as a `private readonly` field, so re-logins in the desktop renderer (which rewrite auth-token.enc) were never observed. 2. The cached JWT had no error-driven invalidation — once the cloud started rejecting it (clock skew, JWKS rotation, mid-flight session revoke) every retry served the same stale value. Provider now takes a `getSessionToken: () => Promise<string>` loader that's consulted on each cache miss, so token rotations are picked up without restarting the host-service. Adds `invalidateCache()` and a one-shot retry-on-UNAUTHORIZED tRPC link in `createApiClient` that drops the cache and retries when the cloud rejects. Desktop entry's loader reads `auth-token.enc` from disk; standalone `serve.ts` keeps the env-var path. Verified end-to-end against prod api.superset.sh: poisoning the cached JWT triggers a real 401, the link invalidates and re-mints, and the request succeeds.
saddlepaddle
added a commit
that referenced
this pull request
May 6, 2026
Changes since v0.2.8: - `superset agents list` (and demoted "presets" to a UI-only concept). CLI now reads canonical agents from host-service. (#4097) - Host-service: refresh stale OAuth access tokens on remote workspace ops instead of failing the request. (#4106) - Host-service: workspace.create now adopts an existing worktree at any path, not just the canonical one. (#4096) - Host-service: AI workspace naming times out after 5s and falls back to a deterministic name. (#4102) Push cli-v0.2.9 after this lands to fire the release pipeline.
MocA-Love
added a commit
to MocA-Love/superset
that referenced
this pull request
May 25, 2026
This branch was previously deployed
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.
Summary
JwtApiAuthProvidercaptured the token at child-process spawn time and never observed re-logins in the desktop renderer (which rewriteauth-token.enc). The cached JWT also had no 401-driven invalidation, so once the cloud started rejecting it (clock skew, JWKS rotation, mid-flight session revoke) every retry served the same stale value.getSessionToken: () => Promise<string>loader consulted on each cache miss, exposesinvalidateCache(), andcreateApiClientwrapshttpBatchLinkwith a one-shot retry-on-UNAUTHORIZED link that drops the cache and retries.auth-token.encfrom disk; standaloneserve.tskeeps the env-var path.Test plan
Verified end-to-end against
https://api.superset.shwith the user's real auth token:invalidateCache().invalidateCachecauses the next mint to use the new value.UNAUTHORIZED: retries exactly once, callsinvalidateCache.host.listcall, received a real 401 from the cloud, link invalidated and re-minted, request returned the user's actual hosts.Out of scope (follow-ups)
Summary by cubic
Fixes remote workspace create/delete failures caused by stale auth tokens by loading fresh session tokens and retrying once on 401. This makes
@superset/host-servicerecover automatically from expired or rotated credentials.JwtApiAuthProvidernow acceptsgetSessionTokento fetch a fresh token on cache miss and exposesinvalidateCache().createApiClientthat drops the JWT cache and retries, wrappinghttpBatchLink.auth-token.encfor token minting;serve.tscontinues using the env var path.Written for commit b8ad7c1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes