Skip to content

Route browser fs and logs endpoints directly to the VM - #164

Open
tnsardesai wants to merge 4 commits into
mainfrom
hypeship/direct-vm-fs-logs
Open

Route browser fs and logs endpoints directly to the VM#164
tnsardesai wants to merge 4 commits into
mainfrom
hypeship/direct-vm-fs-logs

Conversation

@tnsardesai

@tnsardesai tnsardesai commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds fs and logs/stream to the default KERNEL_BROWSER_ROUTING_SUBRESOURCES prefixes, so every /browsers/{id}/fs/* operation (JSON, binary read/write, multipart upload, watch SSE) and /browsers/{id}/logs/stream goes straight to the browser VM through the existing route cache (base_url + JWT) instead of the control plane.

Browser lifecycle/metadata, extensions, replays, telemetry/events, and anything else under logs/ stay on the control plane. KERNEL_BROWSER_ROUTING_SUBRESOURCES still overrides the list, and an empty value still disables routing entirely.

Two Python-only fixes were needed before fs could route directly:

Indexed multipart array names, scoped to fs.upload. Array entries were serialized as repeated files[][dest_path] / files[][file] parts, which cannot be paired back up by the remote filesystem. fs.upload now flattens its own body with indexed names and asks extract_files for matching file-part names, producing files[0][dest_path] / files[0][file]. The client's generic multipart array encoding is untouched, so load_extensions, deployments.create, extensions.create, fs.upload_zip and any future multipart array keep their existing wire format.

Body replay safety on a stale-JWT fallback. A stale direct-to-VM 401/403 is retried by rebuilding the request from the original options, so it is only safe when the body can be serialized again byte for byte. The client now:

  • evicts the stale route from an httpx response event hook, which runs once the status is known and before any body is read. Eviction used to depend on the retry path or the status-error path, so max_retries=0 left a dead route cached — and because httpx reads a non-streamed body inside send(), a 401/403 whose body read fails surfaces as a connection error that never reaches the status-error path at all. The hook is prepended, so a caller-supplied response hook that reads a failing body or raises cannot pre-empt it, and it is registered once per route cache, so a copied client does not stack hooks;
  • retries only when the body is demonstrably replayable — buffered bytes, or a multipart body whose file fields are in-memory or can actually be rewound. seekable() is not treated as proof: a wrapper can report True and still raise from seek(), which would have rendered the fallback body as an empty part, so the rewind httpx would perform is attempted (and the position restored) before classifying the body as replayable.

Streamed bodies (a file object or iterator passed to fs.write_file) surface the original auth error instead of sending a truncated request; the caller's next call uses the control plane. Sync and async clients both changed.

Tests

tests/test_browser_routing.py:

  • default prefix list and segment-boundary matching include fs and logs/stream, and still exclude telemetry/events, fsx/..., logs, logs/history, logstream, extensions, replays (asserted both at the matcher and through rewrite_direct_vm_options)
  • fs JSON (list_files, move), binary read, binary write_file, indexed multipart upload, fs/watch/{id}/events SSE and logs/stream SSE all route to base_url with ?jwt=, no API-key Authorization, and preserved query strings
  • async logs/stream cancellation reaches the transport
  • stale-JWT fallback: buffered binary body and seekable multipart upload are replayed byte for byte on the control plane; a streamed body (sync iterator, async generator) and a multipart field that lies about seekable() each make exactly one VM call, no control-plane fallback, surface the original auth error, and still evict the route (sync + async)
  • max_retries=0 401/403 for buffered and streamed bodies: original error raised, cache empty, next call goes to the control plane with bearer auth and no jwt
  • max_retries=0 direct 401 (sync) and 403 (async) whose body stream raises mid-read: APIConnectionError surfaces, the route is still evicted, and the next call reaches the control plane with bearer auth and no jwt
  • with a caller-supplied response hook already registered on a custom httpx client, a direct 401 (sync) / 403 (async) whose body read fails from inside that hook still evicts the route: the eviction hook runs first, the caller hook still executes, and the next call reaches the control plane with bearer auth and no jwt
  • a copied client registers exactly one eviction hook, ordered ahead of caller hooks, and shares the route cache
  • direct_vm_request_body_is_replayable classification for empty, buffered, streamed, seekable-multipart, lying-seekable(), no-seekable()-with-failing-seek(), and closed-file bodies
  • load_extensions still sends extensions[][name] / extensions[][zip_file], and the generic multipart array path still sends array[], pinning that the indexed encoding is scoped to fs.upload
  • indexed_multipart_body skips omitted values
  • env override excluding fs/logs, and empty env disabling routing

Each new stale-JWT/eviction test was confirmed to fail against the corresponding pre-fix implementation.

Ran locally: full pytest (705 passed, 2952 skipped), pydantic v1 session (692 passed), ruff check . clean, mypy . clean, pyright 0 errors. ruff format --check flags src/kernel/app_framework.py and tests/test_client.py, both untouched by this branch and already unformatted on main.

Live validation

Ran against staging with real headless browsers: fs.write_file, fs.read_file (binary), fs.list_files, fs.upload (bytes and BytesIO entries), fs.upload_zip, fs.download_dir_zip, fs.create_directory, fs.file_info, fs.set_file_permissions, fs.delete_directory, fs.watch.start/stop and logs.stream all hit https://<browser-host>/browser/kernel/...?jwt=... with no API-key header and returned the expected data. Uploaded files read back with the correct per-entry contents, which is what the indexed multipart names fix. telemetry/events and the browser delete stayed on the control plane. Re-verified after narrowing the multipart change and switching to logs/stream.


Note

Medium Risk
Changes default request routing and auth/retry behavior for fs and logs traffic; incorrect replay or eviction logic could wedge sessions or corrupt fallback uploads, though behavior is heavily tested.

Overview
Adds fs and logs/stream to the default direct-to-VM routing allowlist so browser filesystem calls (JSON, binary, multipart upload, watch SSE) and live log streaming use the cached VM base_url + JWT instead of the control plane. Extensions, replays, telemetry/events, and other logs/* paths stay on the API origin; KERNEL_BROWSER_ROUTING_SUBRESOURCES still overrides or disables routing.

fs.upload now sends indexed multipart field names (files[0][dest_path] / files[0][file]) via indexed_multipart_body and extract_files(..., array_format="indices"); other multipart endpoints keep files[] / extensions[] encoding.

Stale direct-to-VM 401/403: sync/async clients register a prepended httpx response hook that evicts the cached route as soon as status is known (including when error body reads fail or max_retries=0). _should_retry only retries when direct_vm_request_body_is_replayable says the body can be rebuilt (buffered bytes or rewindable multipart); streamed or non-rewindable uploads get the original auth error with the route still cleared for the next call.

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

Add fs and logs to the default direct-to-VM subresource prefixes so
filesystem operations and log streaming use the cached browser base_url
and JWT instead of the control plane.

Serialize multipart array entries with indexed names so a file part stays
associated with the sibling fields of its array entry, which repeated
`files[][file]` names cannot express.

Only retry a stale direct-to-VM auth failure on the control plane when the
request body can be rebuilt byte for byte; a streamed body is consumed by
the direct attempt, so retrying would send a truncated body. The stale
route is evicted either way.
Scope the indexed multipart array names to fs.upload instead of changing
the client's generic array encoding: the endpoint now flattens its own body
with indexed names and asks extract_files for matching file part names, so
load_extensions and any other multipart array keep their existing wire
format.

Prove a multipart file field can be rewound before treating a stale-JWT
failure as retryable. A wrapper can report seekable() while seek() raises,
which rendered the fallback body as an empty part.

Evict a stale direct-to-VM route from the terminal error path too. Retry
eligibility is only consulted when retries remain, so with max_retries=0 a
VM 401/403 previously left the dead route cached and wedged later calls.

Route only logs/stream rather than the whole logs subresource.
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.
A response hook registered by the caller runs in registration order, so one
that reads a failing 401/403 body or raises would skip the eviction hook
appended after it and leave the stale route cached. Prepend the hook in both
the sync and async installers; registration stays once per route cache.
@tnsardesai
tnsardesai force-pushed the hypeship/direct-vm-fs-logs branch from d1e21ed to 41ebeda Compare September 2, 2026 21:12
@tnsardesai
tnsardesai marked this pull request as ready for review September 2, 2026 21:18
@tnsardesai
tnsardesai requested review from Sayan- and rgarcia September 2, 2026 21:19

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

reviewed alongside the go (#174) and node (#178) siblings, including a control-plane vs kernel-images surface comparison. all 16 fs/* and logs/stream endpoints match on params, body fields, 2xx codes and content types, and metro-api's /browser/kernel/* handler already wakes standby VMs and records session activity for direct requests, so the routing change itself looks safe.

the response-hook placement is sound: it fires inside send() before the body read, so the 401-with-failing-body case evicts, and it runs before _should_retry so the rebuilt request misses the cache. the replayable check matches httpx behavior (FileField.render_data seeks to 0 on each render, so probing seek(0) is the right test), and write_file with a file object correctly lands in the non-replayable branch. the body-read-failure and hook-ordering tests are the ones that justify the design.

Questions

  • src/kernel/resources/browsers/fs/fs.py:514 — this is a generated file; is the array_format="indices" / indexed_multipart_body change captured as stainless custom code so it survives the next regen?
  • src/kernel/resources/browsers/fs/fs.py:514 — main emits files[][dest_path], which the kernel-images upload parser rejects (empty index fails Atoi) and the control plane forwards untouched. was fs.upload already failing via the control plane? if so this is a user-visible bug fix worth calling out in release notes

Nits

  • src/kernel/lib/browser_routing/routing.py:204 — docstring should mention the hook is inserted into a caller-supplied http_client's event_hooks
  • tests/test_browser_routing.py — control-plane mocks return 204 for fs/upload / fs/write_file; production returns 201

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

Approving. Verified against a mock control plane and VM over real HTTP: fs/* and logs/stream route to the VM with ?jwt= and no Authorization, telemetry/events and replays stay on the control plane with bearer auth, indexed multipart names arrive as files[0][dest_path] / files[0][file], and the stale-JWT path replays a buffered body byte for byte while a streamed body surfaces the original AuthenticationError with the route evicted.

  • p2: with max_retries=0 there is no transparent fallback even for a buffered body. The direct 401 surfaces as AuthenticationError and recovery depends on the eviction, so the caller's next call reaches the control plane. Node and Go fall back inline regardless of retry config. Measured buffered, iterator and file-object bodies at max_retries=0 and 2.

  • p2: the old files[][dest_path] encoding was rejected outright rather than mis-paired. The in-VM handler parses the index with strconv.Atoi, which fails on an empty index and returns 400 invalid form field, so fs.upload was failing for every call including single-file ones. Worth reflecting in the release note.

  • p2, pre-existing and out of scope: the new load_extensions assertion pins extensions[][name], which the control-plane rewriter regex does not match and the in-VM parser rejects.

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.

3 participants