Skip to content

Fall back to the control plane on a stale session JWT - #167

Open
tnsardesai wants to merge 4 commits into
mainfrom
hypeship/stale-jwt-fallback
Open

Fall back to the control plane on a stale session JWT#167
tnsardesai wants to merge 4 commits into
mainfrom
hypeship/stale-jwt-fallback

Conversation

@tnsardesai

@tnsardesai tnsardesai commented Aug 21, 2026

Copy link
Copy Markdown
Contributor

Summary

A direct-to-VM 401/403 with a session jwt query param now evicts the cached route and retries the original request against the API.

This is the same fallback as kernel-python-sdk#157. It did not land in #164 because that PR merged before this commit.

Retry keys off the request JWT, not a still-present cache entry, so concurrent 401s still fall back after the first eviction.

Test plan

  • go test ./lib/browserrouting
  • screenshot 401 on metro retries on the API origin with Authorization restored
  • cached route is evicted after the stale JWT

Note

Medium Risk
Touches request rewriting, auth header restoration, and retry of browser session traffic. Behavior is client-side routing with conservative no-retry when the body cannot be replayed.

Overview
Direct-to-VM requests that get 401/403 with a session jwt query param now retry against the original API origin instead of failing on a stale cached route.

DirectVMRoutingMiddleware restores the original URL, host, and Authorization header, strips jwt, evicts the cached session route, and reissues the request. Fallback is keyed off the request JWT (not a still-present cache entry), so concurrent 401s still retry after the first eviction. If the body cannot be rewound (GetBody missing or failing), the original auth response is returned unchanged.

Reviewed by Cursor Bugbot for commit c342a58. Bugbot is set up for automated code reviews on this repo. Configure here.

A direct-to-VM 401/403 with a jwt query param evicts the cached route
and retries the original request against the API.
Comment thread lib/browserrouting/route_cache.go
The first metro attempt consumes Body; GetBody restores it so
computer/playwright POSTs retry with the original payload.
Comment thread lib/browserrouting/route_cache.go Outdated
If GetBody is missing, return the original auth response still readable.

@cursor cursor Bot 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.

Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.

Fix All in Cursor

❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.

Reviewed by Cursor Bugbot for commit 82ad2ea. Configure here.

Comment thread lib/browserrouting/route_cache.go Outdated
sjmiller609
sjmiller609 previously approved these changes Aug 21, 2026

@rgarcia rgarcia left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

There is a cache invalidation race in the stale-JWT fallback.

cache.Delete(sessionID) unconditionally deletes whatever route is current when the stale request returns. If request A uses stale route A, another request stores refreshed route B, and then request A receives its 401, A deletes the fresh route B. I reproduced this deterministically by storing route B inside the VM handler before returning 401; cache.Load is empty after fallback. Calls remain functional through the control plane, but direct routing stays disabled until another cache-populating request.

Please add a compare-and-delete operation under the cache mutex that removes the entry only when its JWT/base URL still match the route used for the failed request. The control-plane fallback should still happen when the comparison loses, so concurrent stale 401s recover without deleting a newer route. Please add the corresponding race regression test.

There is also an unresolved BugBot finding on the latest commit: if GetBody() returns an error, the middleware returns the 401 response plus the rewind error without closing the response. requestconfig treats the error as decisive, masking the auth response and potentially leaking the connection. Returning the original readable 401 with a nil error would match the existing no-GetBody branch.

The normal rewind path and the full/race test suites otherwise look good.

A rewind error must not mask the auth response or leak its body.
Only mutate the request and close the 401 after rewind succeeds.

@Sayan- Sayan- left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

res.Body.Close() on the fallback path is unguarded, while sniffAndPopulateCache at line 250 of the same file checks res.Body == nil. Only reachable through a custom CustomHTTPDoer, so it's cheap insurance rather than a live bug.

Also worth mirroring the guard the outer retry loop has: shouldRetry in internal/requestconfig/requestconfig.go bails when req.Body != nil && req.GetBody == nil, because such a body can't be replayed. The middleware fallback has no equivalent check. It doesn't bite today, since every generated method sends a *bytes.Buffer and gets GetBody set at requestconfig.go:395-412, which is what lets net/http rewind across the second next(req). It would only matter for a caller-supplied raw io.Reader body, which lands in the default branch with GetBody nil.

Same note as the Node PR: the description says this matches kernel-python-sdk#157, but that branch has no 401/403 fallback, so Python is currently the only SDK routing computer and playwright direct without stale-JWT recovery.

Confirmed end to end that the fallback preserves the request body, restores Authorization, strips jwt, and evicts the route, and that falling back on 403 upgrades metro-api's plaintext "Pool session not leased" to the control plane's proper 400 envelope.

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.

4 participants