Skip to content

sdk/python: send the first frames with the agent upgrade request over plain HTTP - #324

Merged
CMGS merged 1 commit into
mainfrom
perf/py-pipelined-upgrade
Oct 5, 2026
Merged

CMGS merged 1 commit into
mainfrom
perf/py-pipelined-upgrade

Conversation

@CMGS

@CMGS CMGS commented Oct 5, 2026

Copy link
Copy Markdown
Contributor

What

This is the Python SDK counterpart of #322. Over plain HTTP, dial_agent used to write the upgrade request and wait for the 101 before the first frame went out. It now returns a Conn that holds the request. The first send writes the request and the first frame in one sendall, and the first recv parses the 101.

HTTPS keeps the old order, because a TLS edge may be a proxy that drops bytes sent ahead of the 101 (#322 has the probe: Go's httputil.ReverseProxy drops them, Caddy forwards them).

Details:

  • One parser handles the upgrade reply on both paths (_read_upgrade), moved out of the old inline loop.
  • The lazy 101 read is bounded by min(caller socket timeout, client.timeout). With no per-call deadline, that is the same client.timeout budget the old in-dial read had. A run deadline still cuts a pending upgrade through its watchdog.
  • If the connection closes before any upgrade reply, the error is now ProtocolError instead of APIError(0, ""), matching Go. This change lets a run deadline that cuts a pending upgrade surface as SandboxTimeout.
  • The test relay fakes set TCP_NODELAY, as sandboxd's Go sockets do. Without it, Nagle held a reply behind the 101 and the fake's reset close dropped it. One existing test failed 9 of 30 runs on Linux without it, and passed 100 of 100 with it.

Not ported: Go's pipelined proto probe. Python's first kept call still sends info, waits for its reply, and then sends the request. With this PR the upgrade and info leave together, so that first call takes 2 round trips instead of 3 (Go's takes 1). Porting the probe means _connect has to carry the request through the old-daemon redial, which touches run, _lease, _open_stream, and about 15 call sites. That is left for a follow-up.

Known edge, accepted: while the 101 is being read, the socket timeout is temporarily min(caller, client.timeout). A run(stdin=…) pump thread whose sendall starts inside that window inherits the bound. To fail, the guest must stop reading stdin for longer than client.timeout at exactly that point. I judged that contrived and left it.

Numbers

Bare-metal test host, real sandboxd, plain HTTP. Both SDK copies went through a byte-level TCP forwarder that adds a fixed 5 ms one-way delay (10 ms RTT). Runs were interleaved old/new/new/old/old/new, n=100 each, stat("/") p50:

mode old new
keep_alive=0 (every call dials) 21.24–21.29 ms 10.85–10.89 ms
first call on a fresh handle (proto probe) 32.39–32.62 ms 22.14–22.22 ms

On loopback, without the forwarder, the two are within noise (p50 313–356 µs for both), because one loopback round trip is tens of µs.

Acceptance against real sandboxd (plain HTTP)

Old and new behave identically:

  • run(["sh","-c","wc -c"], stdin=8 MiB) → exit 0, 8388608.
  • exec("echo","hi") → hi.
  • A wrong token raises APIError agent upgrade: {"error":"unknown sandbox"} (HTTP 404).

Tests and gates

  • New tests/test_upgrade.py: the fake server reads the request, waits for the frame, and only then sends the 101. There are two cases, keep-alive off (fs_mkdir) and the probe (info), plus a rejected-upgrade case and a test that a run deadline cuts an unanswered upgrade. On main, 3 of these 4 fail; with this PR, 4 of 4 pass.
  • ruff format --check, ruff check, mypy and pytest all pass: 228 passed on macOS py3.12, on the py3.9 floor, and in a linux/arm64 container.
  • e2e TestCaddyTLSCluster/Python (HTTPS path) and the whole e2e package pass.

After both this PR and #322 are merged, the docs/sandboxd-api.md sentence that #322 adds should say "the Go and Python SDKs".

… plain HTTP

A relay dial waited for the 101 before writing its first frame, one extra round trip on every dial: the proto probe, every call after the idle window, every call with keep-alive off. sandboxd replays bytes buffered behind the request, so over plain HTTP the request now leaves with the first frame and the 101 is read on the first recv, under the dial timeout or the caller's tighter socket timeout. HTTPS keeps the old order, since a TLS edge may be a proxy that drops early bytes. A connection that closes before the upgrade reply is now a ProtocolError, so a run deadline that cuts a pending upgrade surfaces as SandboxTimeout. The relay fakes set TCP_NODELAY as sandboxd does; without it Nagle held a reply behind the 101 and the fake's reset dropped it.
@CMGS

CMGS commented Oct 5, 2026

Copy link
Copy Markdown
Contributor Author

Review note (pre-101 sends with no socket timeout): checked, no change.

  • The pre-101 window is bounded by sandboxd. sandboxd does not read relay bytes until it hijacks, so a large write_file can sit in sendall while the node wakes the VM. That wait ends when sandboxd answers: restore runs under the engine's 2 min command timeout plus a 15 s probe. On success it sends the 101 and drains; on failure it closes, and sendall fails at once.

  • Only a hung sandboxd can block forever, and that is outside the deployment contract.

  • The old code differed only in giving up earlier. Its in-dial read gave up at client.timeout, where the new code completes once the wake does. After the 101, uploads were never bounded by a timeout in either version.

  • A rejected large upload keeps its error type. _upload recovers it: on a failed send it runs _expect(conn, "done"), which reads the reply and raises the typed APIError. Checked with a fake node that answers 404 and closes:

    upload size macOS linux/arm64
    1 KiB APIError agent upgrade: {"error":"unknown sandbox"} (HTTP 404) same
    64 MiB same same

@CMGS
CMGS merged commit 721d80b into main Oct 5, 2026
2 checks passed
@CMGS
CMGS deleted the perf/py-pipelined-upgrade branch October 5, 2026 09:58
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.

1 participant