review: whole-repo round on main 893bf20 - #216
Merged
Merged
Conversation
Delete keeps meta.json until the exports are gone, so a failed export cleanup leaves a record with a meta and no export. The dir backend answers store.ErrNotFound for that record and resolveGolden cold-boots; s3 answered a plain error and every claim of the template key failed until someone retried the delete. All four Fetch callers already map ErrNotFound to their unknown-record outcome.
toolLogs collected the guest's replay in a bare strings.Builder. silkd bounds a log ring at 256 KiB, so only a replaced agent streams more, but that agent runs in the guest and the cap on exec and read_file exists for the same reason: guest-fed output must not grow this process.
handleExec appended the newline before it handed the frame to Audit, so an exec frame of exactly AuditLineCap bytes was journaled as oversized while the relay tap, which strips the delimiter, records the same frame in full.
NewManager dropped the Metas error: the template set stayed empty for the life of the process and the node gossiped no promoted template, with nothing in the log. Warn and carry on, as the startup archive listing in reconcile already does.
DialPort, Watch and OpenPty closed the connection when the guest answered the open with an error frame; every other verb parks it through lease.done. silkd returns to its serve loop after each of those error frames, so a DialPort poll on a port that is not listening yet paid a dial, an upgrade and a TLS handshake per attempt.
…Error The stdio pump and the data drain indexed frame["data"] and frame["code"] directly, so a guest that omits the field raised a bare KeyError outside the SandboxError hierarchy callers catch. Every sibling reader already goes through _need.
…closes close() from another thread does not wake a blocked accept(2) on Linux, so every proxy_port call left its accept thread parked for the life of the process; macOS does wake it, which kept the suite green. The loop now accepts under a poll timeout and leaves on the closed socket's OSError. Measured in python:3.11-slim and python:3.9-slim: five closed listeners left 5 threads before, 0 after.
_sandbox() built a fresh Client and Sandbox for every operation, so the SDK's keep-alive pool never served a second call: each operation paid a relay upgrade and an info probe, then parked its connection for the 30 s keep-alive window under its own timer, holding the sandbox's idle clock. The session state is set once at construction; delete releases through the same handle, which drains its pool.
The claim deadline from #212 reached sandbox_exec only. write_file, read_file and list_dir claimed with no deadline, so a run whose first tool call is a file write still walked a black-holed redirect at one client timeout per hop. The toolkit's sandbox() now defaults the claim deadline to CALL_TIMEOUT from the call.
handle_tool_error=True makes LangChain catch ToolException only, and the toolkit raised one for a missing read_file path alone. Every other failure — write_file into a missing directory, an expired claim, a guest error — escaped tool.invoke() and ended the agent run. _tool now maps SandboxError to ToolException in one place, which also replaces read_file's special case. docs/langchain.md states this and the claim budget every tool's first call runs under.
… config release.yml built the amd64 silkd asset without --locked while the arm64 leg and every image build use it, so one release could ship two dependency sets. sandboxd.yml did not trigger on Makefile or .golangci.yml, the two files that define its lint and test commands. The golangci-lint install ran under the host's go: on a host older than go.mod's directive the fresh linter refuses the config, so the install now pins GOTOOLCHAIN to that directive.
…the pty symbol The android flavor's SILKD_IMAGE default stayed at 0.1.0 through twelve releases; CI overrides it, a local build does not. The kernel's post-olddefconfig assertion list did not cover UNIX98_PTYS, which silkd's pty RPC needs and which sits under EXPERT with VT and LEGACY_PTYS off.
The egress, intercept and socks scripts waited for a warm pool and fell through on timeout, so the smoke then passed at the cold tier; a kit without jq takes exactly that path. They now exit the way sandboxd-e2e.sh does. meshsmoke printed PASS with both owner assertions skipped when -peer was empty; it now requires the flag.
…d of helpers runBounded's godoc sat on destroyAll (/code comment budget: a comment describes the declaration it sits on). Template.__init__ lacked its return annotation (/code-py item 4). The langchain suite's last test followed its helpers.
docs/sdk.md and docs/sdk-python.md listed four of the eight SandboxSummary fields as if complete; both now point at GET /v1/sandboxes, which owns the list. docs/egress.md gave 64 idle upstream connections per sandbox; an intercepting pool's proxy holds a second transport of the same size.
The Python twin of the Go SDK fix: _open_stream closed the connection on a guest error frame while _lease parks it, so a dial_port poll on a port that is not listening yet re-dialed and re-upgraded on every attempt.
The target is ceil(rate x lead x safety). The lead is measured only by a refill and a refill needs a target above zero, so a pool configured warm 0 with a warm_max stayed at zero under any load: every claim took the provision path. With no measured lead, demand now asks for one VM, whose refill measures it. The ceiling of any positive rate is 1 and the rate only decays exponentially, so an empty-floor pool would then hold one idle VM until the float underflowed, about twelve hours; under one arrival per decay period now counts as silence. Both branches are the identity for a floor of one or more.
…rges it The purge arm of the reap journaled archive_delete and nothing else, so the claim never got the release or reap that docs/sandboxd-api.md folds compute seconds over, and reaps_total missed it. Release of an archived claim already emits archive_delete then release; the purge now emits archive_delete then reap.
A daemon killed between the journaled hibernate intent and the usage event restarts, adopts the claim as hibernated, and later wakes it: the journal then held a wake with no hibernate and the whole hibernated span folded into billable compute. settlePendingSnap already journals the adoption it makes in-process; reconcile now does the same, after it drops the manager mutex.
…d it Fetch ran populate inside singleflight.Do under the first caller's request context. When that client hung up the download was cancelled and discarded, and every concurrent brancher of the same checkpoint got its context canceled back as a 500. A download that outlasts the client timeout never completed at all: each retry restarted from zero and died at the same point. The flight now runs detached under its own 30 minute budget, the shape healCheckpoint already uses.
sweepStoreGenerations was the one long task Run still called inline in its select; the TTL sweep, the archive-delete retries and every engine fan-out already run in their own goroutines. On a shared FUSE checkpoint root the hourly walk held refill, reap and the claim path's refill kick for its whole duration.
docs/deploy.md promises that stopping sandboxd leaves VMs alive, but the unit left KillMode at control-group, so the promise held only for VMMs that cocoon had moved into a scope of their own. A host survey found one VMM, up for days, still inside the sandboxd service cgroup, where a systemctl stop would have killed it. KillMode=process signals the daemon alone, as libvirtd, containerd and dockerd do.
Both adapters sent no ttl_seconds, so the node applied its 5 minute default and nothing renews a lease: a run that thought for five minutes between two tool calls lost its sandbox, and every later call failed for the rest of the run. The MCP server already defaults to an hour for this reason (mcp/tools.go:113). An explicit ttl_seconds is unchanged.
silkd.md, sdk.md, sdk-python.md: a dropped connection kills a foreground exec (only detached processes and sessions survive); exec with a session uses argv alone, sends no started frame and merges stderr; a command's environment is cleared to PATH, TERM and the lane's proxy variables; an exited detached process leaves the table after 5 minutes; after the child exits an undrained client has 2 seconds before the tail is dropped; one oversized match ends a find; replace is atomic per file, not per list. sdk-python.md: only run and exec take a timeout, unlike the Go SDK's contexts; Template.new takes no claim_ref. mcp.md: logs forgets an exited process after 5 minutes. sandboxd-api.md, security.md: preview has no operator path, unlike fork, promote and checkpoint; an archived claim kept forever has no lease to clamp a preview URL to. deploy.md: a volume claim refused at max_claims answers 429 with no redirect. egress.md: an absolute-form https request on the forward door injects without intercept; an egress pool's warm VMs and golden build sit on the bridge unlocked until claim.
The generation-sweep test's stallingSweepStore landed between stallingMetasStore and its Metas method (/code item 1: a type is immediately followed by its complete method set).
dcbfd6f returned the floor whenever the decayed rate fell under one arrival per decay period, and claimed that was the identity for a floor of one or more. It is not: warm 1, warm_max 3, 0.01/s and a two minute lead target 3, and the cutoff made it 1. The cutoff and the unmeasured lead bootstrap now only ever turn a dynamic target of 1 into 0 or of 0 into 1, which max(floor, ...) absorbs for any floor of one or more.
bf38fd9 detached the flight but kept the synchronous Do, so a caller whose client hung up stayed blocked, holding its record read lock, until the download ended or its 30 minute budget ran out. DoChan plus a select on the caller's context is the shape healCheckpoint uses.
Moving the sweep off the housekeeping loop let an hourly tick start a second walk while one on a wedged mount was still blocked. Every other sweep this loop starts is single-flight through an atomic flag.
Contributor
Author
Hardware results (bare-metal testbed, isolated engine root, guest images
|
| leg | main 893bf20 |
PR 904d046 |
|---|---|---|
| sandboxd-e2e | PASS 22.1 s | PASS 23.2 s |
| egress (none lane) | PASS 3.9 s | PASS 3.9 s |
| egress (egress lane) | PASS 7.7 s | PASS 7.3 s |
| socks | PASS 14.2 s | PASS 13.8 s |
| intercept | PASS 3.8 s | PASS 3.9 s |
| archive | PASS 41.4 s | PASS 41.4 s |
| desktop | PASS 52.2 s | PASS 51.5 s |
Warm claims in the sandboxd leg: 1.4 / 0.3 / 0.4 ms on main, 1.2 / 0.5 / 0.4 ms on the PR — three smoke readings per arm, not a benchmark. The PR adds no work to the claim path. No sbx- VM was left behind by either run.
A pool configured warm: 0, warm_max: 8 under load
Four parallel claim → exec → release loops, 160 claims at about 12 per second, then silence; GET /v1/info and the claim-tier counters sampled through the run.
| sample | main: warm / target | main: warm-tier claims | PR: warm / target | PR: warm-tier claims |
|---|---|---|---|---|
| load + 2 s | 0 / 0 | 0 of 26 | 0 / 1 (1 refilling) | 6 of 30 |
| load + 6 s | 0 / 0 | 0 of 73 | 0 / 3 (3 refilling) | 54 of 96 |
| load + 10 s | 0 / 0 | 0 of 120 | 3 / 3 | 116 of 160 |
| load done | 0 / 0 | 0 of 160 | 3 / 3 | 116 of 160 |
| silent + 30 s | 0 / 0 | — | 3 / 2 | — |
| silent + 120 s | 0 / 0 | — | 1 / 1 | — |
| silent + 300 s | 0 / 0 | — | 1 / 1 | — |
| silent + 480 s | 0 / 0 | — | 0 / 0 | — |
On main the target never leaves zero and every claim takes the clone tier. On the PR the pool grows within seconds, serves 72 % of the burst warm, follows the target down, and is empty again eight minutes after the last claim.
Not exercised on hardware
KillMode=process was not tested under a real systemctl stop; the evidence for it is a read-only survey of a long-running host that found one VMM inside the sandboxd service cgroup. The kernel config assertion for UNIX98_PTYS was checked by building only the config stage for linux/arm64 and linux/amd64 (both report CONFIG_UNIX98_PTYS=y); the boot image itself was not rebuilt.
log.go has imported it directly since the journald priority prefix landed; go mod tidy moves it out of the indirect block. The other four modules are already tidy.
recommit started a detached retry per failed persist. On a data_dir that stays unwritable the reap tick alone rolls back and calls it every 5 seconds, so the daemon grew one goroutine and one error line per tick with no client activity. commit always writes the current full state, so a single retrier converges every outstanding change.
ttl_seconds and the buffered exec's timeout_seconds were multiplied by time.Second unbounded. A large value wraps negative: a ttl_seconds in [9223372037, 2^63) read as unset and got the 5 minute default instead of the documented 24h cap, and a timeout_seconds such as 10000000000 woke a hibernated sandbox and then answered 504 without running the command.
portReadBuf was 64 KiB under a comment saying it fits one silkd data frame, but a port_forward frame carries BULK_CHUNK = 256 KiB and is 349,554 bytes on the wire, so every frame took bufio's fragment path. The constant now derives from wire.BulkChunk. Interleaved A/B (ABBA x3, 2000 frames per run): 9 allocations per frame to 1, median 75 ms to 47 ms. Preview path only; nothing on the claim or SDK relay path reads here.
Conn.recv wraps a socket failure in ProtocolError; Conn.send let a bare BrokenPipeError or ConnectionResetError out, the one SDK failure outside the SandboxError hierarchy. A write into a relay the node had just closed therefore escaped every caller that catches SandboxError, including the LangChain toolkit's tool-error mapping. The two places that tolerated a failed send now name the typed error.
shell.yml's only step is `make sh-lint`; a761567 added the Makefile to the Go workflow's paths and missed this one.
Corrects two sentences earlier commits on this branch added: a dropped connection kills a foreground command only once it next writes output (a silent one runs on; silkd notices a hang-up through a failed write), and on a claim with no lease ttl_seconds 0 mints a one hour preview URL. deploy.md: the fuller config's idle_hibernate_seconds 300 equalled the 5 minute lease a woken archive gets, so that sandbox could never hibernate again before the reap destroyed it (now 120, with the constraint stated); the minimal config's policy-less egress pool has no network. sandboxd-api.md, egress.md: PUT /v1/pools decides which policy layers a key has, and live claims follow at their next wake or restart; require_promoted belongs to volume claims only. mcp.md: ending a session releases its sandboxes. silkd.md: fs_push creates dest before the stream. langchain.md: from_checkpoint pins template and net. sdk.md: Close and Stop answer not_found for a session or server already gone.
a33b56a made recommit single-flight, which dropped a recommit that found the retrier busy. If the retrier had already cloned the state for its write — commitWake's temporary record, say — and the rollback's recommit arrived before it finished, claims.json kept the stale record with no one left to replace it; an unclean restart then lost the archived claim. The retrier now clears its flag, re-reads the sequence and carries on while a newer one is unwritten. TestRecommitDrainsAChangeItWasTooBusyToSee holds the first write on a FIFO to make the interleaving deterministic.
The wake stamped a fresh server-default 5 minute lease whatever TTL the claim was made with, and nothing renews a lease, so a sandbox archived after an hour of idling and woken by its next call was destroyed five minutes later — mid-use if still busy — with its archive checkpoint already consumed. The claim record now keeps the lease it asked for and a wake grants that again; records written before the field still get the default.
effectivePolicy asked whether the key was pooled at every arm, so a PUT /v1/pools that added a policy-less pool for a key tenants already held claims on took their egress away at the next wake or restart, and one that dropped such a pool widened the same claims to the tenant's whole allow-list. The claim record now carries the layer decided at claim time; a record written before the field keeps resolving against the live pool set, so an upgrade neither widens nor narrows a claim.
The guest kills a foreground exec whose connection dropped only when a write to the client fails, so a silent command (sleep, a quiet build) outlived every canceled ctx and every run(timeout=) cut. Run and run now keep the pid the started frame carried and, when the ctx or the timeout ends the stream, send it a kill over a connection of their own, bounded to 5 s and best effort, before returning the cancel — the shape of sandboxd's killExec for buffered exec. A session command reports no pid and is untouched. Hot path: one ctx.Err check on the success path, no extra RPC.
A wake grants the lease the claim was made with, a claim's policy layer is settled when it is made, a session command is the session's and Session.Close ends it, and a canceled Run or a run timeout kills the command it cuts.
A socket timeout bounds each read, not the reply: a guest trickling the kill's done frame a byte at a time kept run() past its timeout for as long as it liked. The cleanup now arms the same watchdog run() uses, which aborts the connection at deadline whatever arrives, and keeps only the last started pid instead of every started frame a guest cares to send.
Contributor
Author
|
Hardware rerun on the second half of the round (commits after 904d046: recommit single-flight + drain, wire-seconds saturation, preview port reader sizing, persisted lease, pinned policy layer, SDK kill-on-cancel), same bare-metal testbed, same isolated cocoon root and Seven-leg E2E,
Warm claims (claim/exec/release ×3 in the sandboxd-e2e leg): |
Seconds saturated only the positive half: min(n, MaxInt64/1e9) returns a negative n unchanged, so ttl_seconds -10000000000 still wrapped into a positive duration and clampTTL granted the 24 h cap where 0, the server default, was meant. A small negative already meant "default" through clampTTL's <= 0 branch; the low clamp keeps that and closes the wrap.
…parked The watchdog was cancelled outside the lease, so a timer firing between the park and the cancel shut down a connection already in the pool. The lease now wraps the whole cleanup: the cancel runs before the park, and a watchdog that fails to start leaves the connection to the lease's close.
recommit is one retrier per manager that writes the current state until nothing newer is pending, not a per-call background persist; LeaseSeconds is the lease the claim was granted, the clamped value, not the one it asked for. The test name follows the field.
Nothing has written the flat <id>/export layout or a .old sibling since generations landed: every publish renames <staging>/export to its generation dir. The dir Fetch, sweep and touch fallbacks, the s3 prefix fallback, the .old removals and the four tests that planted the old layout go with it. Records published before generations are no longer readable; there is no compatibility window to keep.
The connect and TLS phases already turned an OSError into ProtocolError, but the upgrade exchange (send, status line, headers) let a socket timeout or a connection reset out raw, so a node that accepts TCP and stalls the 101 escaped the adapters' SandboxError handling and broke the agent run. Every handshake failure now maps through one helper: a timeout is SandboxTimeout, anything else ProtocolError; a SandboxTimeout raised by remaining_timeout inside the exchange passes through unwrapped. docs/deploy.md: the egress pool example said the pool "or its tenants" needs an egress block; a tenant's claim of a pooled key needs both blocks and gets their intersection, so the sentence now says so.
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
1 participant
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.
Whole-repo round on main
893bf20: every tracked file read in full (388 files across Go, Rust, Python, guest images, scripts, CI and docs), then a second pass that followed five end-to-end flows on the fixed branch without seeing the first pass's findings. Each finding was checked against the source before anything changed; each bug fix carries a regression test that fails with the fix reverted.Two changes to look at first
packaging/sandboxd.servicegainsKillMode=process.docs/deploy.mdpromises that stopping sandboxd leaves VMs alive, but with the defaultcontrol-groupthat held only for VMMs the engine had moved into a scope of their own. A survey of a long-running host found one VMM, up for days, still inside the sandboxd service cgroup. Deployed nodes need the unit line to get the guarantee.ttl_secondsto 3600 (was unset → the node's 5 minutes). Nothing renews a lease, so a run that paused five minutes lost its sandbox for good; the MCP server already defaults to an hour for the same reason. An explicit value is unchanged.Bugs fixed
sandboxd
pool: a pool withwarm: 0underwarm_maxnever grew — the target needs a measured lead, the lead is measured only by a refill, a refill needs a target above zero. It also could not return to empty: the ceiling of any positive rate is 1. Both branches are the identity for a floor of 1 or more.pool: a retention purge of an archived claim journaledarchive_deleteand no terminal event, so its usage interval never closed andreaps_totalmissed it.pool: a hibernate adopted by restart reconcile was never journaled, so the laterwakehad nohibernateand the span billed as compute.pool:sweepStoreGenerationsran inline inRun's select; on a shared checkpoint root the hourly walk held refill, reap and the claim path's refill kick.pool:NewManagerdropped a failed template listing without a log line; the node then advertised no promoted template for the life of the process.store/s3:Fetchran the shared download under the first caller's request context — a client that hung up failed every concurrent brancher of that checkpoint, and a download slower than the client timeout never completed. Now detached under its own budget, the shapehealCheckpointalready uses.store/s3: a record whose meta survived a failed export cleanup answered a plain error where the dir backend answersErrNotFound, so every claim of that template key failed instead of cold-booting.server: the buffered-exec audit saw the frame with its newline, so a frame of exactly the cap was journaled asoversized.SDKs, adapters, MCP
sdk/go,sdk/python: a refusedDialPort/Watch/OpenPtythrew away the keep-alive connection, so polling a port that is not listening yet paid a dial, an upgrade and a TLS handshake per attempt.sdk/python:proxy_port's accept thread never retired on Linux (closing a socket from another thread does not wakeaccept); macOS does wake it, which kept the suite green. Measured in Linux containers on 3.9 and 3.11: 5 leaked threads before, 0 after.sdk/python: a guest frame missingdataorcoderaised a bareKeyErroroutside theSandboxErrorhierarchy.sdk/openai: a fresh client and handle per operation defeated keep-alive — every call paid an upgrade and aninfoprobe, then parked its connection for 30 s under its own timer.sdk/langchain: the claim deadline from sdk/python: bound a claim by the caller's deadline #212 reachedsandbox_execonly; and any SDK failure other than a missingread_filepath escapedtool.invoke()and ended the agent run instead of reaching the model as a tool error.mcp: thelogstool had no output cap whereexecandread_filedo.CI, images, e2e
--locked; the Go workflow did not trigger onMakefileor.golangci.yml; a freshmake golangci-lintbuilt the linter with the host's Go rather than the module's and then refused the config.SILKD_IMAGEdefault was still0.1.0; the kernel config assertion did not coverUNIX98_PTYS.meshsmokeprinted PASS with both owner assertions skipped when-peerwas empty.Docs
About twenty statements now say what the code does: a dropped connection kills a foreground exec;
execwith a session usesargvalone and merges stderr; a command's environment is cleared; an exited detached process is forgotten after 5 minutes; the 2-second post-exit drain;replaceis atomic per file, not per list; Python data-plane calls other thanrun/exectake no timeout; preview has no operator path and an archived claim kept forever has no lease to clamp to; a volume claim refused atmax_claimsdoes not redirect; an absolute-formhttps://forward request injects withoutintercept; an egress pool's warm VMs sit on the bridge unlocked until claim.Deliberately not changed
meta.jsoncommit marker is written without an fsync. The engine already fsyncs every exported file, so the marker is the one unsynced step; using the existingWriteFileSyncbreaks a test's fault-injection seam, and keeping the seam costs about 15 lines for a host-power-loss trigger.run/exechave no deadline (new API surface).Evidence
make go-lint: 10 ×0 issues.;go test -race -count=1in all five modules: 18 ok;asl ./...on both GOOS: clean apart from four advisory forwarder rows;cargo fmt/clippy -D warnings/teston macOS and in arust:1linux/arm64 container: boot/init 17, silkd 124 passed;ruff,pytest(209 + 10 + 13) and strictmypy: clean;shellcheck: clean. Production lines: net positive only inside the bug-fix commits; the layout and docs commits add none. Hardware results follow as a comment.