Repository navigation
sdk/python: keep the client timeout off a guest stream once it is up - #178
Merged
Merged
Conversation
dial_agent set the client timeout on the socket at connect and never cleared it, so every read on that connection inherited it: a port stream or an exec that stayed quiet longer than the timeout died with a stream timeout, which surfaced as a dropped connection to whatever was proxied through it. The Go SDK bounds only the dial and the upgrade; this does the same, and the documented contract now says so. The regression test drives dial_port against a fake agent that answers the handshake, stays silent for three timeouts, then sends data: it fails with StreamTimeout on the previous code and passes now.
Its only producer was the socket timeout on a guest stream, and the previous commit stopped setting one; the class, its export, the read branch that raised it, and the one catch remain as dead code otherwise.
CMGS
added a commit
that referenced
this pull request
Sep 14, 2026
* wire: scan past a tag's siblings without recursing skipValue recursed once per nesting level, so a guest that put a deeply nested value ahead of the type key could grow a host goroutine stack by megabytes per frame. The scan is now a flat loop with a small depth cap; wire values nest two levels at most. * mcp: keep exec output when the call fails, cap it, and reject a negative ttl A cut-off or dropped exec answered with the bare error and threw away the output the command had produced; the agent now gets the collected stdout and stderr next to the error field. Each stream keeps its first 1 MiB and flags truncated past that, so a firehose cannot grow the process. A negative ttl_seconds used to reach the node, which clamps it to its own 5-minute default; it is now a bad-argument error. * sdk/go: release the relay when a watch ends on its own drain returned on the sandbox's error frame or a dropped connection without running the dial's cleanup, so the relay connection and its context hook lived until the caller's ctx ended; only Close released them. * sdk/python: enforce the exec cutoff inside the guest Since the stream socket lost its timeout (#178), the openai adapter's wait_for could cancel the awaiting coroutine while the worker thread stayed blocked on the relay, and the langchain tool's 5-minute promise had nothing behind it. Both now run the command under timeout(1) -s KILL, which kills the process group at the deadline; the adapter maps exit 124 to the TimeoutError the Agents SDK documents, the tool reports it as exit code 124. * config: keep an explicit warm 0 under warm_max; refuse a wildcard advertise_addr on a mesh warm defaulted through cmp.Or, so a pool written as warm 0 with a warm_max started at 4 instead of empty; the default now applies only when neither is set. advertise_addr defaulted to the listen address even when that was a wildcard, and a mesh node gossiped 0.0.0.0 or an empty host as its owner address to peers; a mesh config now fails to load until it names a routable host. * server: stop a preview forward after one hop, keep encoded slashes, end WriteTo cleanly A preview token names its owner by advertise_addr; after that address changed, the node forwarded the request to its old name, which routed back to itself and forwarded again until the connections ran out. The forward now marks the request and a marked request that still misses answers 502. proxyLocal rewrote only URL.Path and left the inbound RawPath behind, so a %2F in the guest path reached the guest decoded. auditTee.WriteTo returned io.EOF where io.WriterTo promises nil, and collectExec now accumulates into builders instead of copying the whole output once more into strings. * review: take the pool-key digest off the warm claim path; small simplifications handleClaim hashed the pool key on every claim although only the redirect and error paths read it; the digest is now computed where it is used, and writeResult logs the key itself. Also: the orphan-snapshot filter is a slices.DeleteFunc, confirmGone names its logger once, SetPools and the egress dialer wrap their causes with %w, ClusterDigest reuses DecodedKey instead of decoding the cluster key again, portconn's lone constant uses the bare form, and redirectFallback is a plain loop without the retryAny predicate that existed only for it. * silkd: bound the find read on the handle, fail a session without entropy, latch a present lane verdict scan_file checked the size through metadata and then read the file unbounded, so a file growing between the two calls was read whole; the read now stops at FIND_MAX_FILE on the same handle. rand_token fell back to a per-process counter when no entropy source answered, which made the session marker forgeable by the shell's own commands; session.create now fails instead. routes_directly re-read /etc/silkd-lane on every exec of a direct-lane guest; the host writes the file once per claim, so a present verdict is latched and only its absence is re-read. * review: silkd and boot-init simplifications fs.replace expands capture groups through a counting Replacer instead of allocating a String per match; the pty pre_exec registration moves into sysutil so every unsafe block lives in one module; tree's stderr cap is a file-level constant; Table::get is crate-visible; boot-init renders its marks into one String, trims the MAC in place, and formats the device path only for a serial that matched. Doc lines trimmed to one fact. * review: comment register and budget sweep; adapter method order Inline comments open lowercase in the STE register (61 lines); godocs that restated a short body are gone (checkpointURL, probeMAC, guardsEgressLane, validateArchiveWindow, synced, goldenCAMatches, two desktopsmoke steps) and multi-sentence comments in the wire package and the Go SDK are one line each. The openai adapter lists its public methods before the SDK hooks; two Python **fields parameters are typed; three test fakes close their socket, narrow a suppress, and report the observed op to the main thread instead of asserting inside it. * pool: refuse capture verbs on an archived claim; keep rollbacks and egress doors off a released claim Checkpoint, Promote and Fork of an archived claim ran the engine with an empty VM name and Hibernate journaled a bogus intent onto it; they now answer ErrArchived (409) and Hibernate is the no-op it already is for a hibernated claim. archive() and commitWake re-set the record on a failed commit even when a concurrent Release had deleted it, which resurrected the claim in claims.json; the rollback is now gated on the claim still being live. armEgressProxy closes the listener it displaces, and resyncEgress leaves hibernated and archived claims to their wake, so a guarded claim woken after a restart no longer leaks its startup door; finalizeBatch disarms a claim released in its own arm window. Also: an oversized request frame records as op oversized instead of escaping the audit journal; the pool passes no CA to a proxy whose pool never intercepts (one Transport clone and a leaf map less per claim); Sandboxes sorts outside m.mu; idleEnabled/archiveEnabled are atomics (read off m.mu by the sweeps); ClaimWarm kicks the refill before it answers ErrNoWarm; sweepExpiredCheckpoints owns its TTL guard; DeleteCheckpoint asks archiveCkPinned; quiesce gives the fallback sync its own budget; a persisted pool that fails validation names pools.json. * egress: keep the upstream connection for a closing guest; keep a clamped leaf cached r.Clone carried the guest's Connection: close into the upstream request, so an HTTP/1.0 guest or one sending the header paid a fresh upstream connect (and TLS handshake on the intercept path) per request; the relay clears out.Close like httputil.ReverseProxy does. A leaf whose end is clamped to the intermediate's own could never be renewed, so once the intermediate entered its last 24 h every intercepted session re-signed; such a leaf stays cached. * server: serve a held claim under a stale owner name; omit an unspecified owner address A preview token names its owner by the advertise address at mint time; a node that holds the claim now serves it locally whatever that string says, and the one-hop marker stays as the backstop. The raw path suffix is derived by segments, so a %70 spelling of the prefix keeps %2F too. A claim response and /owner no longer emit a wildcard advertise address (:7777, 0.0.0.0) as owner_addr: an empty owner makes both SDKs reuse the address they dialed, which is what a default single-node config needs. The audit tee records an oversized first line as such instead of dropping it; writeResult keeps a string id (the template name), and ErrArchived maps to 409. * mcp: bound read_file to a regular file under the exec cap read_file buffered whatever the guest streamed, so a device or a multi-gigabyte file grew the MCP process the way exec used to; it now stats first and refuses anything but a regular file up to 1 MiB with a pointer to exec head/tail. The tool tests share one fake node and a reply decoder. * sdk/go: release the relay when a pty exits Pty.drain returned on the exit or error frame without the dial's cleanup, the same leak Watcher had; the drain now owns it. * sdk/python: a wall-clock timeout on run and exec Sandbox.run and exec take timeout, a wall clock over the dial and the run: a watchdog cuts the connection at the deadline, which makes silkd kill the command, and TimeoutError is raised; the dial itself is bounded by the remaining time. The openai adapter passes its per-call timeout straight through, and the langchain tool carries one deadline across the lazy claim, the dial and the command, so neither depends on timeout(1) in the guest image or on exit code 124. * silkd: bound the replace read through one handle fs.replace still sized the file by path and then read it whole by path; both verbs now share read_bounded, which opens once, reserves the size it measured and stops at FIND_MAX_FILE. * review: simplify-lens follow-ups in silkd and boot-init Marks.render goes back to the iterator form (a once-per-boot path with at most seven marks), and routes_directly latches through Option::inspect. * docs: align the pages with the code README lists the boot-init and shell workflows; index drops the network adb path the android flavor never had; the desktop pages name the whole app set; performance records the refill-time door bind; sdk-python states the peer-heal TTL requirement and the run/exec timeout; the adapter, langchain, mcp and API pages describe the exec timeout, the capped exec result, the bounded read_file, the archived 409 and the oversized audit record. * sdk/go: stream a file into a writer; keep reading after a failed stdin close ReadFileTo hands each data frame to an io.Writer and stops when the writer refuses, so a caller can bound a read without buffering it. Run sent stdin_close synchronously and returned its send error before reading anything, so a guest that had already answered and closed cost the caller the exit code it had sent; the frames are read regardless. * mcp: bound read_file by the bytes it reads The stat-then-read guard was not a bound: a file can grow or be swapped between the two RPCs. read_file now streams through the capped writer and stops the read at 1 MiB with the same pointer to exec head/tail. * review: gofumpt the mcp tools table inside its var block * pool: check for an archived claim under the transition lock The ErrArchived prechecks in Checkpoint, Promote and Fork ran before sb.Transition was taken, and archive() clears VMName under that lock, so a capture racing an archive could still snapshot an empty VM name. The check now lives in sourceSnap, which every capture path calls under the lock; the prechecks are gone. * sdk/langchain: refuse the command when the claim used the whole call budget A lazy claim that consumed the five minutes handed the command a one-second timeout; the tool now reports the cut-off instead. * silkd: read only a regular file fs_read opened whatever the path named: a device streamed forever and a FIFO without a writer parked a blocking-pool thread in open for the life of the daemon. A path that is not a regular file now answers bad_request before the open. * mcp: refuse a non-regular file before streaming it read_file stats first and rejects a directory, device or FIFO with the kind in the message; the streamed 1 MiB cap stays the size bound. * pool: read the VM name for the fork and checkpoint usage events under the transition lock archive() clears VMName under sb.Transition; the two events read it after their capture released the lock. * review: lint follow-ups The MCP read test reuses the hijack's err instead of shadowing it; the pool's lockedVMName sits below the Manager method set. * silkd: check the file kind on the opened descriptor A path could be swapped for a FIFO between the stat and the open, and a writerless FIFO still parked a blocking-pool thread there. fs_read now opens with O_NONBLOCK and fstats the descriptor it will stream, so the kind it checks is the kind it reads; the test adds a FIFO to the directory and /dev/null cases. * egress: track both halves of a tunnel so Close ends it Close closed only the guest side of a CONNECT or SOCKS5 tunnel. When the upstream stayed silent after the half-close, the copy goroutine, the upstream socket and the sandbox hold outlived the proxy for the life of the daemon. * pool: drop the consumed snapshot when a release beats the wake to the finish A wake whose commit landed before a concurrent Release removed the claim returned without dropping the hibernate image it had just resumed from; Release saw an empty HibernateSnap and left it too, so the image stayed on disk until the next restart's Reconcile. No cheap test: the window sits between the commit and the release check with no seam to hold it open. * pool: evict a template's record lock with the record DeleteTemplate released its lock with recDone, so the per-id mutex stayed in recLocks after the record was gone, and resolveGolden created one for every key it looked up and did not find. The checkpoint paths already evict on delete and gate the lookup before locking. * docs: align the remaining pages with the code Egress: the audit record's field names, the SSRF guard's NAT64 handling, which lane a tenant policy may run on, the CONNECT rule wording, the archive wording and the internal-range file. Security and silkd pages name the SOCKS5 door and every not_found case. API: PUT /v1/pools 400 cases, the probe replay window, archive-backed checkpoints, usage-event tenant fields. Cluster gossip digest and epoch rule; the claims-journal write lock. SDK pages: which node verbs need the root token, the idle sweep's live-connection hold, spawn bad_request kinds, the checkpoint delete peer drop (and the Go godoc's stale TTL clause), the Python lookup, proxy_port and adapter state; langchain install and write contract; MCP defaults, per-call cap and child lifetimes. README: sh-lint target and the arm64 kernel; desktop, browser, benchmarks and image READMEs match their Dockerfiles and scripts. * docs: tighten four sentences from the last review round The idle-sweep sentence promises only what the sweep's re-check observes; a bare egress rule reads as admitting CONNECT; the checkpoint-delete paragraphs and the Go godoc carry the one reachable zero-TTL case (a node later run with healing off); the PUT /v1/pools 400 list names negative archive durations. * pool: take the release re-check off the claim path finalizeBatch re-took m.mu after arming every guarded claim to catch a release inside the arm window. Only a root client that lists and deletes the id within that sub-millisecond window can reach it, and the residue is one door listener and one nft table until the next restart; the wake paths keep the check, where the claimant holds the token and the window is hundreds of milliseconds. * review: drop the no-op Fetch release and the dead pty exit guard Both store backends returned func() {} from Fetch and every caller composed it into a no-op; the read pin is the pool's record lock. The pty finish guard tested a state only finish sets, from two mutually exclusive call sites. The other cut-list entries stay: the sweep skeletons save under ten lines for a generic helper, the two SDK options are published API, and the silkd helper the ledger named does not exist. * pool: write the archive delete marker outside the manager mutex releaseResolved ran MkdirAll and WriteFile under m.mu for an archived claim; reapOnce already writes the same marker after unlocking. The marker is written first and the claim re-checked under the lock, looping if the archive checkpoint changed meanwhile, so a wake or archive racing the release still ends with a marker for the checkpoint the release removes. * docs: a bare egress rule admits the SOCKS5 door The opt-in sentence said every rule without CONNECT is rejected; a rule with an empty methods list admits it, as Validate checks. * sdk/python: enforce run deadline through upgrade and send * sdk/python: wrap the exec send call The previous commit left the call one column past the 120-character limit, which fails ruff format and E501 in CI.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
dial_agentset the clienttimeouton the socket at connect and nevercleared it, so every read on that connection inherited it. A port stream
or an exec that stayed quiet for longer than the timeout died with a
stream timeout, which reached whatever was proxied through it as a dropped
connection: an OSWorld evaluator running a three-minute command over
proxy_portsawRemoteDisconnectedat exactly 120 s, the default.The Go SDK bounds only the dial and the upgrade and leaves the stream to
the caller's context. This does the same: once the upgrade answers 101 the
socket goes back to blocking mode, so
timeoutbounds every control-planerequest and the data-plane dial and upgrade, and a guest stream then lives
until the guest ends it. The documented contract now says so.
With no timeout on a stream nothing raises
StreamTimeoutany more; thesecond commit cuts the class, its export, the read branch that raised it
and the one catch, and its entry in the error list.
Verification
The regression test drives
dial_portagainst a fake agent that answersthe handshake, stays silent for three timeouts, then sends data. Against
origin/mainit fails withStreamTimeout(on macOS and on Linux); withthe change it passes, and a second test pins that the dial itself is still
bounded.
Gates, run the way CI runs them (
pip install -eall three SDKs into athrowaway venv, Python 3.12, Linux):
ruff format --check .andruff check .are clean at the repositoryroot.
Follow-up
The OSWorld cocoon provider currently works around this by constructing
its
Clientwith a 900 s timeout; once this lands it can drop back to thedefault.