Skip to content

Commit d818b10

Browse files
committed
Evict a stale direct-to-VM route before the error body is read
httpx reads the body of a non-streamed response inside send(), so a 401 or 403 whose body read fails surfaces as a connection error and never reaches the status-error path. The route eviction ran after that read, which left a dead JWT cached and wedged every later call for the session. Move eviction into an httpx response event hook, which runs once the status is known and before any body is read, for both the sync and async clients. `_should_retry` now only decides whether replaying the body on the control plane is safe.
1 parent 7d2a8f9 commit d818b10

3 files changed

Lines changed: 150 additions & 33 deletions

File tree

src/kernel/_client.py

Lines changed: 10 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,9 @@
4343
browser_routing_config_from_env,
4444
is_stale_direct_vm_auth_response,
4545
should_retry_stale_direct_vm_auth,
46+
install_stale_direct_vm_auth_eviction,
4647
maybe_evict_browser_route_from_response,
48+
install_async_stale_direct_vm_auth_eviction,
4749
maybe_populate_browser_route_cache_from_response,
4850
)
4951

@@ -204,6 +206,7 @@ def __init__(
204206
)
205207
self.browser_route_cache = _browser_route_cache or BrowserRouteCache()
206208
self._browser_routing = browser_routing_config_from_env()
209+
install_stale_direct_vm_auth_eviction(self._client, cache=self.browser_route_cache)
207210

208211
@cached_property
209212
def deployments(self) -> DeploymentsResource:
@@ -364,28 +367,15 @@ def _prepare_options(self, options: Any) -> Any:
364367
def _prepare_request(self, request: httpx.Request) -> None:
365368
strip_direct_vm_auth(request, cache=self.browser_route_cache)
366369

367-
def _evict_stale_direct_vm_route(self, response: httpx.Response) -> None:
368-
maybe_evict_browser_route_from_response(response, cache=self.browser_route_cache)
369-
370370
@override
371371
def _should_retry(self, response: httpx.Response) -> bool:
372372
if is_stale_direct_vm_auth_response(response):
373-
self._evict_stale_direct_vm_route(response)
374-
# The route is evicted either way; only retry when the body can be
375-
# rebuilt, otherwise the caller sees the original auth failure and a
376-
# later call goes to the control plane.
373+
# The route was already evicted by the response hook; retry only when
374+
# the body can be rebuilt, otherwise the caller sees the original auth
375+
# failure and a later call goes to the control plane.
377376
return should_retry_stale_direct_vm_auth(response)
378377
return super()._should_retry(response)
379378

380-
@override
381-
def _make_status_error_from_response(self, response: httpx.Response) -> APIStatusError:
382-
# `_should_retry` never runs when the request has no retries left, so this
383-
# is the only place a stale direct-to-VM route gets evicted before the
384-
# error surfaces to the caller.
385-
if is_stale_direct_vm_auth_response(response):
386-
self._evict_stale_direct_vm_route(response)
387-
return super()._make_status_error_from_response(response)
388-
389379
@override
390380
def _process_response(
391381
self,
@@ -602,6 +592,7 @@ def __init__(
602592
)
603593
self.browser_route_cache = _browser_route_cache or BrowserRouteCache()
604594
self._browser_routing = browser_routing_config_from_env()
595+
install_async_stale_direct_vm_auth_eviction(self._client, cache=self.browser_route_cache)
605596

606597
@cached_property
607598
def deployments(self) -> AsyncDeploymentsResource:
@@ -762,28 +753,15 @@ async def _prepare_options(self, options: Any) -> Any:
762753
async def _prepare_request(self, request: httpx.Request) -> None:
763754
strip_direct_vm_auth(request, cache=self.browser_route_cache)
764755

765-
def _evict_stale_direct_vm_route(self, response: httpx.Response) -> None:
766-
maybe_evict_browser_route_from_response(response, cache=self.browser_route_cache)
767-
768756
@override
769757
def _should_retry(self, response: httpx.Response) -> bool:
770758
if is_stale_direct_vm_auth_response(response):
771-
self._evict_stale_direct_vm_route(response)
772-
# The route is evicted either way; only retry when the body can be
773-
# rebuilt, otherwise the caller sees the original auth failure and a
774-
# later call goes to the control plane.
759+
# The route was already evicted by the response hook; retry only when
760+
# the body can be rebuilt, otherwise the caller sees the original auth
761+
# failure and a later call goes to the control plane.
775762
return should_retry_stale_direct_vm_auth(response)
776763
return super()._should_retry(response)
777764

778-
@override
779-
def _make_status_error_from_response(self, response: httpx.Response) -> APIStatusError:
780-
# `_should_retry` never runs when the request has no retries left, so this
781-
# is the only place a stale direct-to-VM route gets evicted before the
782-
# error surfaces to the caller.
783-
if is_stale_direct_vm_auth_response(response):
784-
self._evict_stale_direct_vm_route(response)
785-
return super()._make_status_error_from_response(response)
786-
787765
@override
788766
async def _process_response(
789767
self,

src/kernel/lib/browser_routing/routing.py

Lines changed: 45 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,9 @@ class BrowserRoutingConfig:
3232
subresources: tuple[str, ...] = field(default_factory=tuple)
3333

3434

35+
_EVICTION_HOOK_CACHE_ATTR = "_kernel_browser_route_cache"
36+
37+
3538
_BROWSER_ROUTE_CACHEABLE_PATH = re.compile(r"^/(?:v\d+/)?browsers(?:/[^/]+)?/?$")
3639
_BROWSER_DELETE_BY_ID_PATH = re.compile(r"^/(?:v\d+/)?browsers/([^/]+)/?$")
3740
_BROWSER_POOL_ACQUIRE_PATH = re.compile(r"^/(?:v\d+/)?browser_pools/[^/]+/acquire/?$")
@@ -198,6 +201,48 @@ def is_stale_direct_vm_auth_response(response: httpx.Response) -> bool:
198201
return bool(response.request.url.params.get("jwt"))
199202

200203

204+
def install_stale_direct_vm_auth_eviction(client: httpx.Client, *, cache: BrowserRouteCache) -> None:
205+
"""Evict stale direct-to-VM routes as soon as the response status is known.
206+
207+
httpx reads the body of a non-streamed response inside `send()`, so a caller
208+
that only inspects the returned response never learns the status of a 401/403
209+
whose body read fails — the read error surfaces from `send()` instead and the
210+
dead route would stay cached, wedging every later call for that session. A
211+
response event hook runs after the status is known and before any body is
212+
read, which keeps eviction independent of the body.
213+
"""
214+
hooks = client.event_hooks.setdefault("response", [])
215+
if _has_eviction_hook(hooks, cache):
216+
return
217+
218+
def evict(response: httpx.Response) -> None:
219+
if is_stale_direct_vm_auth_response(response):
220+
maybe_evict_browser_route_from_response(response, cache=cache)
221+
222+
setattr(evict, _EVICTION_HOOK_CACHE_ATTR, cache)
223+
hooks.append(evict)
224+
225+
226+
def install_async_stale_direct_vm_auth_eviction(client: httpx.AsyncClient, *, cache: BrowserRouteCache) -> None:
227+
"""Async counterpart of `install_stale_direct_vm_auth_eviction`."""
228+
hooks = client.event_hooks.setdefault("response", [])
229+
if _has_eviction_hook(hooks, cache):
230+
return
231+
232+
async def evict(response: httpx.Response) -> None:
233+
if is_stale_direct_vm_auth_response(response):
234+
maybe_evict_browser_route_from_response(response, cache=cache)
235+
236+
setattr(evict, _EVICTION_HOOK_CACHE_ATTR, cache)
237+
hooks.append(evict)
238+
239+
240+
def _has_eviction_hook(hooks: list[Any], cache: BrowserRouteCache) -> bool:
241+
# A copied client shares both the httpx client and the route cache, so the
242+
# hook is registered once per cache instead of once per client.
243+
return any(getattr(hook, _EVICTION_HOOK_CACHE_ATTR, None) is cache for hook in hooks)
244+
245+
201246
def should_retry_stale_direct_vm_auth(response: httpx.Response) -> bool:
202247
"""Whether a stale direct-to-VM auth failure can be retried on the control plane.
203248

tests/test_browser_routing.py

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,7 +3,7 @@
33
import io
44
import os
55
import asyncio
6-
from typing import Any, AsyncIterator, cast
6+
from typing import Any, Iterator, AsyncIterator, cast
77
from pathlib import Path
88
from typing_extensions import override
99

@@ -14,6 +14,7 @@
1414
from kernel import (
1515
Kernel,
1616
AsyncKernel,
17+
APIConnectionError,
1718
AuthenticationError,
1819
InternalServerError,
1920
PermissionDeniedError,
@@ -1259,3 +1260,96 @@ def test_indexed_multipart_body_flattens_only_given_values() -> None:
12591260
"files[1][dest_path]": "/tmp/two",
12601261
"flag": True,
12611262
}
1263+
1264+
1265+
class _FailingSyncStream(httpx.SyncByteStream):
1266+
"""A response body that fails while it is being read."""
1267+
1268+
@override
1269+
def __iter__(self) -> Iterator[bytes]:
1270+
raise httpx.ReadError("connection reset while reading the error body")
1271+
1272+
1273+
class _FailingAsyncStream(httpx.AsyncByteStream):
1274+
@override
1275+
async def __aiter__(self) -> AsyncIterator[bytes]:
1276+
raise httpx.ReadError("connection reset while reading the error body")
1277+
yield b"" # pragma: no cover - unreachable, keeps this an async generator
1278+
1279+
1280+
def test_stale_direct_vm_jwt_evicts_route_when_error_body_read_fails(
1281+
monkeypatch: pytest.MonkeyPatch,
1282+
) -> None:
1283+
monkeypatch.delenv("KERNEL_BROWSER_ROUTING_SUBRESOURCES", raising=False)
1284+
requests: list[httpx.Request] = []
1285+
1286+
def handle_request(request: httpx.Request) -> httpx.Response:
1287+
requests.append(request)
1288+
if "browser-session.test" in str(request.url):
1289+
return httpx.Response(401, stream=_FailingSyncStream(), headers={"content-type": "text/plain"})
1290+
return httpx.Response(200, content=b"png", headers={"content-type": "image/png"})
1291+
1292+
http_client = httpx.Client(transport=httpx.MockTransport(handle_request))
1293+
with Kernel(
1294+
base_url=base_url,
1295+
api_key=api_key,
1296+
max_retries=0,
1297+
http_client=http_client,
1298+
_strict_response_validation=True,
1299+
) as client:
1300+
_cache_browser(client)
1301+
with pytest.raises(APIConnectionError):
1302+
client.browsers.computer.capture_screenshot("sess-1")
1303+
# The status was known before the body read failed, so the dead route is gone.
1304+
assert client.browser_route_cache.get("sess-1") is None
1305+
1306+
client.browsers.computer.capture_screenshot("sess-1")
1307+
1308+
assert str(requests[0].url).startswith("http://browser-session.test/browser/kernel/computer/screenshot")
1309+
assert requests[1].url == httpx.URL(f"{base_url}/browsers/sess-1/computer/screenshot")
1310+
assert requests[1].url.params.get("jwt") is None
1311+
assert requests[1].headers.get("Authorization") == f"Bearer {api_key}"
1312+
1313+
1314+
@pytest.mark.asyncio
1315+
async def test_async_stale_direct_vm_jwt_evicts_route_when_error_body_read_fails(
1316+
monkeypatch: pytest.MonkeyPatch,
1317+
) -> None:
1318+
monkeypatch.delenv("KERNEL_BROWSER_ROUTING_SUBRESOURCES", raising=False)
1319+
requests: list[httpx.Request] = []
1320+
1321+
async def handle_request(request: httpx.Request) -> httpx.Response:
1322+
requests.append(request)
1323+
if "browser-session.test" in str(request.url):
1324+
return httpx.Response(403, stream=_FailingAsyncStream(), headers={"content-type": "text/plain"})
1325+
return httpx.Response(200, content=b"png", headers={"content-type": "image/png"})
1326+
1327+
http_client = httpx.AsyncClient(transport=httpx.MockTransport(handle_request))
1328+
async with AsyncKernel(
1329+
base_url=base_url,
1330+
api_key=api_key,
1331+
max_retries=0,
1332+
http_client=http_client,
1333+
_strict_response_validation=True,
1334+
) as client:
1335+
route = browser_route_from_browser(_fake_browser())
1336+
assert route is not None
1337+
client.browser_route_cache.set(route)
1338+
with pytest.raises(APIConnectionError):
1339+
await client.browsers.computer.capture_screenshot("sess-1")
1340+
assert client.browser_route_cache.get("sess-1") is None
1341+
1342+
await client.browsers.computer.capture_screenshot("sess-1")
1343+
1344+
assert str(requests[0].url).startswith("http://browser-session.test/browser/kernel/computer/screenshot")
1345+
assert requests[1].url == httpx.URL(f"{base_url}/browsers/sess-1/computer/screenshot")
1346+
assert requests[1].url.params.get("jwt") is None
1347+
assert requests[1].headers.get("Authorization") == f"Bearer {api_key}"
1348+
1349+
1350+
def test_copied_client_registers_one_route_eviction_hook() -> None:
1351+
with Kernel(base_url=base_url, api_key=api_key, _strict_response_validation=True) as client:
1352+
copied = client.copy(api_key="sk-456")
1353+
assert copied.browser_route_cache is client.browser_route_cache
1354+
hooks = client._client.event_hooks["response"] # pyright: ignore[reportPrivateUsage]
1355+
assert len(hooks) == 1

0 commit comments

Comments
 (0)