Skip to content

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

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

Fall back to the control plane on a stale session JWT#173
tnsardesai wants to merge 3 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 #170 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

  • jest tests/lib/browser-routing.test.ts
  • screenshot 401 on metro retries on the API origin with Authorization
  • cached route is evicted after the stale JWT

Note

Medium Risk
Touches request routing and JWT cache eviction for browser sessions. Incorrect eviction or retry could drop a valid route or loop auth failures, but the change is small and JWT-keyed.

Overview
Direct-to-VM browser requests that return 401/403 with a session jwt now drop that cached route and retry the original call on the control plane (with Authorization).

Eviction uses deleteIfJwt so only the JWT that failed is removed. A route that was refreshed concurrently is left in place. The failed VM response body is cancelled before the retry.

Reviewed by Cursor Bugbot for commit 31517bb. 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.

@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 b3195f0. Configure here.

Comment thread src/lib/browser-routing.ts
Leaving the stream open can stall undici connection reuse.
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 refreshes the cache with route B, and then request A receives its 401, A deletes the fresh route B. I reproduced this deterministically by replacing the cached route inside the VM handler before returning 401; the refreshed route is gone after fallback. Calls remain functional through the control plane, but direct routing stays disabled until another cache-populating request.

Please make eviction conditional on the current cached JWT/base URL still matching the route used for the failed request, while keeping the control-plane fallback independent of whether that conditional delete succeeds. A regression test should refresh the route while the stale request is in flight and assert the fresh route survives.

The current Python #157 implementation has the same race when a refreshed route retains the same base URL, so the equivalent conditional eviction should be applied there too.

The response-body cancellation fix otherwise looks good, and the routing tests pass.

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

The 401 response is discarded without cancelling its body. src/client.ts:653 does await Shims.CancelReadableStream(response.body) before retrying, for the same reason; without it undici won't release the connection until GC. One line before the fallback fetch:

if ((routed.status === 401 || routed.status === 403) && target.searchParams.get('jwt')) {
  await Shims.CancelReadableStream(routed.body);
  cache.delete(sessionId);
  return innerFetch(input, init);
}

Separately, the description says this is "the same fallback as kernel-python-sdk#157", but that branch is only the one-line allowlist change plus tests, with no 401/403 handling. As things stand Python ships direct computer/playwright routing with no stale-JWT recovery. Worth either adding it there or dropping the claim here.

Verified the fallback replays the request body and restores Authorization correctly, and that no routed endpoint declares 401 or 403 in the instance spec, so the trigger can't fire on an application-level auth failure.

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

Withdrawing my earlier review: I was reviewing b3195f0 and the body cancellation had already landed in 11f19da. That comment was obsolete when I posted it, sorry for the noise.

My parity note was also wrong. #157 has since picked up the fallback in f525a24, 3580671 and 728330d, so Python is no longer missing it. Disregard that paragraph.

On the open race, I reproduced it independently and it confirms: storing a refreshed route for the same session while the stale request is in flight leaves the cache empty after fallback, because cache.delete(sessionId) is unconditional.

after fallback: undefined     // expected jwt-FRESH

Worth noting for the Python side of the fix: its eviction resolves the session by matching the failed request URL against cached base_url values, so a refreshed route keeps matching precisely because the VM address doesn't change when only the JWT rotates. Conditioning on the JWT rather than the base URL is what makes it safe there.

A later 401 must not delete a route that was refreshed in flight.
@tnsardesai

Copy link
Copy Markdown
Contributor Author

fixed in 31517bb.

cache.deleteIfJwt(sessionId, failedJwt) only evicts if the cached jwt still matches the one that 401'd. a refresh to jwt-FRESH while the stale request is in flight is left alone. fallback to the API still always runs.

test: does not evict a refreshed route after a stale JWT 401

sorry for the earlier review ping — i was testing a workflow that requested reviews before bugbot / self-review landed. this commit is the actual fix.

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