Skip to content

Route computer and playwright calls directly to the VM - #157

Merged
tnsardesai merged 8 commits into
mainfrom
hypeship/tier1-direct-allowlist
Aug 24, 2026
Merged

Route computer and playwright calls directly to the VM#157
tnsardesai merged 8 commits into
mainfrom
hypeship/tier1-direct-allowlist

Conversation

@tnsardesai

@tnsardesaitnsardesai commented Aug 18, 2026

Copy link
Copy Markdown
Contributor

Summary

Default direct-to-VM routing now includes computer and playwright in addition to curl and telemetry/stream.

computer/screenshot and playwright/execute rewrite to {base_url}/... with the session JWT and drop Authorization. process/*, fs/*, and telemetry/events stay on the API origin.

KERNEL_BROWSER_ROUTING_SUBRESOURCES still overrides the default list. An empty value still disables routing.

Metro-api activity recording for /browser/kernel/* has been merged in kernel/kernel#3417. That was the hard gate for this allowlist flip. The kill switch is leaving base_url unset or setting KERNEL_BROWSER_ROUTING_SUBRESOURCES to empty.

Latency (eu-west)

Same hop skip as the Node/Go SDKs. Comparison script: https://gist.github.com/tnsardesai/6704dfc86e2c0785e03beaf274aa672d

Same eu-west session. Published SDK still hits api.onkernel.com; this allowlist rewrite hits metro-api.

creating eu-west browser…
session rao32cls5rhduxg9n1a732i5
region eu-west
base_url https://proxy.dub-unruffled-kowalevski.onkernel.com:8443/browser/kernel
cdp host proxy.dub-unruffled-kowalevski.onkernel.com:8443
rounds warmup=2 timed=8
control plane (@onkernel/sdk@0.93.0)
control-plane screenshot 498ms api.onkernel.com
control-plane screenshot 884ms api.onkernel.com
control-plane screenshot 647ms api.onkernel.com
control-plane screenshot 508ms api.onkernel.com
control-plane screenshot 614ms api.onkernel.com
control-plane screenshot 512ms api.onkernel.com
control-plane screenshot 509ms api.onkernel.com
control-plane screenshot 408ms api.onkernel.com
control-plane screenshot 622ms api.onkernel.com
control-plane screenshot 590ms api.onkernel.com
min 410ms p50 513ms p95 650ms max 650ms
direct to VM (kernel-node-sdk#hypeship/tier1-direct-allowlist)
direct-to-vm screenshot 648ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 623ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 321ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 335ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 332ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 329ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 345ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 344ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 339ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
direct-to-vm screenshot 341ms proxy.dub-unruffled-kowalevski.onkernel.com:8443
min 324ms p50 337ms p95 347ms max 347ms
p50 delta control-plane 513ms vs direct 337ms (-176ms)

Test plan

  • default allowlist includes curl, telemetry/stream, computer, playwright
  • computer screenshot and playwright execute rewrite to the VM and drop Authorization
  • process, fs, and telemetry/events stay on the API origin

Note

Medium Risk
Changes default request routing and retry/auth-cache behavior for browser subresources, including 401/403 JWT eviction. Misrouting or over-retry could send traffic to the wrong origin or drop a valid cached route.

Overview
Default direct-to-VM routing now includes computer and playwright (in addition to curl and telemetry/stream). Screenshot and Playwright execute go to the session VM with JWT and no Authorization; process, fs, and telemetry/events stay on the control plane. KERNEL_BROWSER_ROUTING_SUBRESOURCES still overrides or disables the list.

On 401/403 from a JWT-query VM request, the client evicts that JWT from the route cache (not a newer one) and retries so the next hop can hit the API origin.

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

Add computer and playwright to the default browser routing allowlist
so screenshot and execute traffic can skip the control plane once
metro-api records activity on the kernel proxy.
The default now includes computer and playwright, not just curl.
The asserted list can grow; the test name should not.
@tnsardesai
tnsardesai marked this pull request as ready for review August 20, 2026 23:06
sjmiller609
sjmiller609 previously approved these changes Aug 21, 2026
rgarcia
rgarcia previously approved these changes Aug 21, 2026

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

QA’d commit 1b28d62 across sync and async clients.

  • Browser-routing suite: 26 passed; PR CI is green.
  • Focused experiments confirmed computer/batch, clipboard, screenshot, and Playwright calls route directly with the session JWT and without Authorization.
  • process/*, fs/*, and telemetry/events retain API-origin routing and API authorization.
  • Missing base_url, custom allowlists, and the empty-value kill switch behave correctly.
  • Retry rewriting is idempotent and preserves request parameters.

Two non-blocking notes:

  1. The computer prefix routes all computer/* endpoints, while the description and tests focus on screenshot/execute. Please consider one nested computer endpoint test.
  2. A stale session JWT produces a direct-VM 401 with no automatic cache refresh or control-plane fallback. A follow-up could evict the route on 401/403 or document client.browsers.get(id) as the refresh path.

No blocking findings.

tnsardesaiand others added 2 commits August 21, 2026 10:35
A direct-to-VM 401/403 now evicts the cached route and retries the
original request against the API.
@tnsardesai

Copy link
Copy Markdown
ContributorAuthor

addressed the stale JWT note: a direct-VM 401/403 now evicts the cached route and retries the original request on the control plane. no get() refresh — if the jwt is stale the API path still works, and a later create/retrieve will recache.

@tnsardesai
tnsardesai dismissed stale reviews from sjmiller609 and rgarcia via f525a24August 21, 2026 18:04

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

Comment threadsrc/kernel/lib/browser_routing/routing.py Outdated
Lint failed on the stale-JWT retry change.
A metro 401/403 with a jwt query param is enough to fall back to the
control plane, so concurrent requests still retry after the first
eviction.
@tnsardesai

Copy link
Copy Markdown
ContributorAuthor

addressed the concurrent stale-JWT race: retry now keys off the request jwt query param, not a still-present cache entry. first eviction no longer blocks other in-flight 401/403s from falling back to the control plane. same fallback is now in node #170 and go #164.

@rgarciargarcia 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 stale-JWT fallback still has the cache invalidation race now covered in the Node and Go reviews.

_session_id_from_direct_vm_response() identifies the current cached route by base URL, then cache.delete(session_id) removes it without checking whether its JWT is the one that failed. If request A leaves with jwt-old, another request refreshes the same base URL to jwt-new, and A later returns 401, the fresh jwt-new route is deleted. I reproduced this against head 728330d: after replacing the cached route before processing the old 401, cache.get("sess-1") returns None.

The latest change correctly fixes the separate race where concurrent stale 401s stopped retrying after the first eviction, but it does not protect a newer cache entry from an older response.

Please make eviction conditional on the current route’s base URL and JWT matching the failed request, and add a regression test that refreshes the route while the stale request is in flight and asserts the fresh route survives. The existing routing suite passes, but does not cover this interleaving.

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

Copy link
Copy Markdown
ContributorAuthor

fixed in 111aa09.

eviction is now compare-and-delete on jwt: delete_if_jwt(session_id, failed_jwt). a 401 for token-abc no longer removes a cache entry that was refreshed to jwt-FRESH while that request was in flight. control-plane retry still happens either way.

test: test_stale_direct_vm_jwt_does_not_evict_refreshed_route

also ran this live against a real session before pushing — injected jwt-FRESH over the cached route, applied a stale 401, confirmed jwt-FRESH survived, then a real screenshot returned 200.

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.

@Sayan-Sayan- left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Verified at 111aa09. The race rgarcia flagged is fixed: delete_if_jwt refuses a mismatched JWT and still deletes a matching one, and with a refresh landing mid-flight the stale 401 leaves the fresh route alone (jwts sent to vm: ['jwt-STALE', 'jwt-FRESH'], fresh route intact). Suite green at 29, async path works.

Remaining issues are behavior drift from #173 (31517bb) and #167 (d5e0fc9). Same probes against all three heads:

 Node Go Python
retries disabled falls back falls back 401, no fallback, stale route stays cached
budget spent by prior 500 falls back falls back 401, no fallback, stale route stays cached
backoff added to fallback none none ~390ms
fallback destination control plane control plane whatever the cache says now
  1. The fallback is gated on the retry budget, with three consequences. It lives in _should_retry, which the base client only consults when remaining_retries > 0. With max_retries=0 or with_options(max_retries=0) the 401 surfaces raw. The two eviction sites are _should_retry and _process_response, and neither runs on a terminal 401, so the dead route stays cached and never self-heals (three successive screenshots all raised AuthenticationError with the stale route still present). And because the gate is the budget rather than the config, a transient 500 earlier in the same call reproduces both at default settings, so this is not limited to users who disable retries. Node and Go avoid all of this by falling back inside a single attempt.

  2. The fallback pays retry backoff, ~390ms measured from INITIAL_RETRY_DELAY plus jitter, against the 176ms this change is meant to save. Node and Go retry immediately.

  3. The two SDK families promise different destinations. Under the race, Node and Go both go VM then control plane; here the retry re-runs _prepare_options and re-routes to the VM on the refreshed JWT. Arguably better since it keeps the latency win, but worth making a decision rather than a side effect of hook placement.

Coverage: no nested computer/* test (nothing in the file matches clipboard, and computer/clipboard/read and write are the only three-segment tails that route direct), and the three new fallback tests are sync-only.

@sjmiller609
sjmiller609 removed their request for review August 24, 2026 13:56

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

Follow-up verified — approving.

Eviction is now conditional on both base-URL match (via _session_id_from_direct_vm_response) and JWT match (delete_if_jwt), closing the cache invalidation race from my last review; a refreshed route survives a stale-JWT 401 per the new regression test.

Ran tests/test_browser_routing.py at 111aa09 — 29 passed.

@tnsardesai
tnsardesai merged commit ec72bcd into mainAug 24, 2026
11 checks passed
Sign up for freeto 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

@tnsardesai@rgarcia@Sayan-@sjmiller609