review: whole-repo round on main 21dc4e7 - #214
Merged
Merged
Conversation
netutil.LimitListener wraps every accepted conn in a type that embeds the net.Conn interface, so the *net.UnixConn the doors hand out lost CloseWrite and the splice fast path. A guest tunnel whose upstream closed first never saw EOF, and each such tunnel held two descriptors and one door slot until the claim ended. The cap now lives in a listener whose conns embed the *net.UnixConn itself, so half-close and splice(2) work again.
… exports The delete-intent marker was written without a directory sync, so a crash between the marker and the checkpoint delete could lose the marker while the journal (which is synced) already dropped the claim: the wake image stayed in the store with nothing left to reclaim it. The marker now rides the same syncDir the volume dirty marker uses. A fork export interrupted by a restart left <data_dir>/fork-* at full image size; Reconcile now sweeps it beside the golden *.tmp staging dirs.
…outable preview_advertise Two pool entries resolving to one key were accepted at load and seeded last-wins for the pool but first-wins for its egress and warmup maps, so the node ran the second entry's warm target under the first entry's policy; PUT /v1/pools already rejected the same input. Two volumes sharing a path under different names defeated writer exclusion, which is keyed by name. preview_advertise defaulted to a wildcard preview_listen and minted http://:8443/p/... URLs no client can dial; it now needs a host like advertise_addr does. The preview handler routes an unmapped claim error through writeResult instead of dropping writePoolErr's verdict.
zerolog's ConsoleWriter colourises unless NoColor is set, so every record reaching journald carried escape sequences around the timestamp and level. One constructor now serves the daemon and the tests.
…ONNECTs A zero-length domain name passed the handshake, matched a wildcard rule and was recorded as an allow before the dialer refused it; RFC 1928 calls it malformed, so the handshake now answers a general failure. An intercepted CONNECT was never recorded as a decision: only the inner requests were, so a guest that reached an intercepted host and aborted the TLS handshake left no trace.
A pool leaving the desired set stays in the map while a refill is in flight; a tenant claim of that key in the window was treated as pooled and served with no egress door, then got the tenant policy two seconds later.
The sweep lists every store record and deletes expired ones serially; on the s3 backend that held the first refill, so warm pools stayed empty and every claim in the window cold-booted. The function already guards itself with a CAS, so it detaches like retryArchiveDeletes.
…h-only checkpoint claims connect probed the daemon's proto on every handle's first call, but the lease only parks a connection when the platform can probe it and the keep-alive window is open, so WithKeepAlive(0) against a pre-proto-2 daemon paid a second dial per handle for a reuse that never happened. The probe now runs only when its answer can matter. Checkpoint.New rejected WithVolumes and WithClaimRef but let WithVolumesAttachOnly through, where the wire request silently dropped it. Lookup keeps the peer-discovery error instead of reporting a missing sandbox when discovery never ran, Connect documents that a seed list keeps its first address, and the EOF checks use errors.Is.
A host that terminated the stdio server with a signal skipped the deferred release, so every sandbox the session claimed stayed held for its one-hour tool TTL. main now runs serve beside a signal context and releases on either exit. exec and spawn ran sh -c "" and reported exit 0 when the required command argument was missing; both refuse it now.
…a failed pty start; flush frames before an oversized one A find ended silently on any readable byte, so a request pipelined behind it (legal since proto 2) truncated the walk with no done or error frame and left the client waiting; the walk now treats buffered bytes as a queued request and only a hang-up as the end. write_frames rendered a batch into one buffer and returned on the first frame over the cap, discarding every frame rendered before it, so a find that hit an oversized match dropped the matches ahead of it; the prefix is written before the error. pty::open swallowed a failed started write and returned Ok, so the server awaited a feeder on a connection whose writer was gone; it returns the error like exec does. Chunk::into_response lost its unreachable stdout and stderr arms, and the exec foreground channel is built once instead of cloned twice.
…s disk ids cocoon's Firecracker backend names layer and COW disks by device path; resolve_disks matched ids against virtio serials only, so such a boot polled sysfs for the whole budget and powered off. A /dev/... id now resolves as soon as its node exists, mirroring the shell hook this initramfs replaced.
TestEveryVerbHasAFixture listed 35 of 39 requests and 20 of 21 responses; the four LSP types were outside the guard. resp_info.json pinned proto 1 against a daemon that serves proto 2, and req_fs_find.json carried a glob that matches nothing (.rs anchors to a file literally named .rs).
…setup messages watch_e2e reimplemented send and next_frame inline; pty_e2e accepted any exit code for exit; two error tests checked the type without the kind; the harness panicked without the offending line or git's stderr; request_on was pub with no external caller.
exec.rs and pty.rs import proto with self like the other ten modules; proc.rs calls sysutil through its import and names the ring snapshot for what it does; find.rs spells the Arc clone explicitly. Comments that restated the code or misnamed their subject go (debug_token, MNT_SECURE, the console note, the duplicated ip= layout); two inline comments move to the lowercase register; FIND_MAX_FILE's doc names both verbs it bounds and BootCfg.timeout its real scope. lsp.rs drops a discarded bool binding and cfg.rs indexes a slice its guard already proved long enough.
…eld, make close idempotent, chunk pty writes run(timeout) left the handle's first RPC unbounded: the proto probe ran before the watchdog on a socket with no timeout, so a wedged guest hung the call forever. The probe now runs under the remaining budget. A 200 reply or a silkd frame missing a required member raised KeyError past the SDK hierarchy; require_field and _need turn those into APIError and ProtocolError. Session.close and Lsp.stop raised not_found on a second call (and Lsp's with-exit after a request stream, which reaps the server). Pty.read now latches EOF like PortConn and Pty.write chunks like every other bulk path, so a large paste no longer emits a frame over the cap that killed the guest shell without a word. Watcher.error stays None after a close from another thread, as documented. SandboxTimeout joins the SandboxError hierarchy while still being a TimeoutError. The bare data frame renders without a str round trip (3x on 1 MiB chunks). Tests: the four raw-reply cases share one fixture, FakeNode lives in conftest, and test_proxy reads the monotonic clock.
…port a missing file as a tool error The OpenAI provider claimed a sandbox and then built the session state; a bad snapshot spec raised past the claim and leaked it for its TTL. The LangChain read tool promised a tool error for a missing path and raised a Python exception into the agent loop instead; it now raises ToolException under handle_tool_error.
Pool: one owner for settle-and-bill (settlePendingSnap), for the destroy fan-out (destroyAll) and for the needs-unmount predicate; reapOnce compacts with slices.DeleteFunc; Audit drops a guard its only caller already makes; applyVolumes loses a single-volume arm the errgroup path already covers; resolveGolden returns a zero value with a nil-safe release instead of six no-op closures; archiveCkPinned reads the pinned set; claimLoaded rejects before it materializes an export; the reconcile lock step tests locked[tap] once. Server: closeOnCancel owns the cancel-then-close pair, the unknown sandbox reply goes through writePoolErr, handleDeleteTemplate builds a PoolKey instead of a request body. Egress: nonInjectable is a slice, marshalCA takes the concrete key types, Event loses two fields nothing read and Proxy the identity that fed them. Store: RequireRegular owns the regular-file contract for both backends, s3 names its concurrency and batch constants and joins keys explicitly. Mesh inlines persistEpoch. mcp looks a tool up with slices.IndexFunc. sdk/go: sendChunks owns the chunked write loop. e2e: meshsmoke and crossnode use harness.Claim, lifecycle bounds its run, egresssmoke drops a condition its guard made constant, interceptsmoke folds case on both sides of grepLine. journal.close had test callers only. Tests: claimTenant, a table for validateHealedCheckpoint, testCA over testing.TB, one table for Client.Volumes, postExec over postJSON, new(*expr) in the s3 fake.
Wire pairs and vocabulary types cluster ahead of the type they serve (claimResponse after claimRequest's methods, checkpoint and preview wire types above their handles, volume admission types above catalogVolume); utilities sit below every type block (mesh.go short/containsAll, frame.go NewFrameScanner in the decode group); producers trail the type they produce (Compose below composite, resolveGolden below goldenResolution); the record-lock layer moves out of template.go into reclock.go, lockedVMName joins the pool.go leaf helpers, the sandbox list reply and handler leave metrics.go for server.go, and Config groups its fields by concern instead of one blank line per field.
Godocs on unexported funcs that restated their bodies go (archiveOnce, archive, publishCheckpoint, sweepExpiredCheckpoints, idleOnce, skipIdle, wake, respFail, parseRecord, loadEpoch, the five sdk/go RPC helpers, the decodeReq/decodeResp adapter note, two rpcbench samplers); multi-line narrative on unexported items compresses to one STE 101 line (frame.go's encodeTagged/frameTag/scanTag, the silkdtest fakes, mcp's server and cappedOutput, the mcp package doc drops product names); inline comments that repeated a sibling or misnamed their subject are fixed (Create -> O_CREATE, the archive rollback and commit notes, the reapAction consts, journal.VolumesRW, the storetest phase labels); godocs that stated a contract the code does not have are corrected (store.Fetch's release, PreviewDial's authorization, clearHealPending's lock, the two-peer cap on VolumeOwners and TemplateVolumeOwners). 26 comment lines added, 76 removed.
egress: audit lines exist only under audit_log and carry no tenant; the intercepted CONNECT is now a recorded decision. API: exec answers 504 for timeout_seconds only, writable is never emitted on peer-only volume rows. SDKs: the keep-alive window is Unix-only and parks at most 8 connections, PreviewURL/preview_url take the api token, ReadFileTo joins the Go files reference, the Python guide states its bytes-not-streams exception and the type key every frame-derived dict carries, the adapter table names the per-call user refusal, the LangChain README imports langgraph. README and index quote the measured cold-boot range, list packaging/, describe make help honestly and add make bench; the boot table admits /dev paths and the shared wait budget. benchmarks and performance stamp the rows that lacked a host or commit. deploy documents the tenant egress field, the root-only stats route and the preview_advertise host requirement. desktop's warmup example is bounded by wall time under the 2-minute engine cap. Three image headers match what their RUN steps install.
…binary; python.yml lints e2e The environment stamp bench.sh emits had no sandboxd row, so every pasted results block needed its commit added by hand and three of them never got one. The prebuilt guard admitted a run with only SANDBOXD_BIN and DEMO_BIN set and then died on an unbound RPCBENCH_BIN under set -u. python.yml triggers on e2e/**/*.py, so e2e/tls_client.py is linted on its own changes.
The half-close test closes the CONNECT reply body and stops shadowing err; validate delegates the preview and pool-key checks so gocyclo stays under 30; the reconcile lock step keeps its two flat ifs (nestif); a test message spells color the way misspell wants.
…'s original error, size lifecycle's deadline from -wait, correct the boot timeout doc A claim finishing between closeBoxes and process exit was recorded too late and held for its TTL: trackBox now releases a claim tracked after the session closed, and main closes stdin on a signal so the read loop's own release runs, falling back to closeBoxes after a grace. The OpenAI provider's cleanup release no longer replaces the construction error it was cleaning up after. lifecycle's overall deadline follows -wait instead of a fixed 20 minutes. cocoon.timeout covers the disk set only; NIC MACs have a fixed 200 ms budget.
Contributor
Author
Hardware: seven E2E legs on this branch's binariescocoon-test1 (384c, Debian 13, isolated cocoon root
Warm claims on the PR run: 0.4–1.2 ms ( |
… adapter's dual-failure case is pinned closeBoxes is single-flight: it releases every tracked claim concurrently and returns only once they are done, and a second caller (main's grace fallback racing the read loop's defer) waits for the first instead of finding an empty map and letting the process exit mid-release. Test: TestCloseBoxesWaitsForEveryReleaseOnce. The adapter test now also raises from Sandbox.close and asserts the snapshot error still propagates.
…ite, lookup discovery error) Run starts the retention sweep asynchronously so a slow store cannot delay the first refill; the test blocks Metas and asserts a warm VM still appears. The pty test injects a writer that fails on the started frame and asserts the error propagates, the table entry is gone, and the shell is dead. Lookup's test breaks peer discovery and asserts the peers error is joined alongside the no-owner verdict.
namesHost required a scheme or a port, so a bare hostname failed startup validation although Mint prepends http:// to exactly that shape. The schemeless branch now treats a string without a port as the host and still rejects an empty or unspecified one.
Mint prepends http:// to a value without a scheme, so validation now parses the same string. A bare "[::]" kept its brackets past SplitHostPort and slipped through as a routable host.
Contributor
Author
Hardware: E2E reruns on the review-round binariesSame kit and bare-metal host as the previous comment (isolated cocoon root, CH v54.0.0). sandboxd and the seven drivers rebuilt from each commit below; rt guests keep this branch's silkd overlay, desktop guest =
No |
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 audit on main 21dc4e7: every Go, Rust and Python file read in full against
/code,/code-rs,/code-py; the docs read against the code;/simplifyand/loc-justifylenses over every package. Fixes land first, each in its own commit with a regression test where one is cheap; the review commits are behaviour-preserving.Fixes
7dde24e):netutil.LimitListenerwrapped the accepted conn in a type embedding thenet.Conninterface, so the*net.UnixConnlostCloseWriteand the splice fast path. A tunnel whose upstream closed first never delivered EOF to the guest and held two descriptors and a door slot until the claim ended. The cap now lives in a listener whose conns embed the*net.UnixConn. Test:TestEgressDoorHalfCloseReachesTheGuestfails on main withi/o timeout, passes here.9af3df9).preview_advertisethat names no host (http://:8443/p/…) are rejected at load (6516196).349e6f1).1de3ca4).effectivePolicyresolves against the active pool set (2431316); the checkpoint TTL sweep runs off the Run loop so an s3 backend no longer holds the first refill (fe5ec58).WithKeepAlive(0)against a proto-1 daemon);Checkpoint.NewrefusesWithVolumesAttachOnlyinstead of dropping it;Lookupkeeps the peer-discovery error (5e97899).exec/spawnrefuse an empty command instead of runningsh -c ""(a78dd82).find_serves_a_request_pipelined_behind_it_after_donehangs on main);write_frameswrites the frames rendered before an oversized one; a failed ptystartedwrite returns the error like exec (9b7829b)./dev/vdXids, as cocoon's Firecracker lane passes them, resolve (c7d8bc6).run(timeout)bounds the proto probe (the handle's first RPC could hang forever); a truncated 200 or a frame without a required member raisesAPIError/ProtocolErrorinstead ofKeyError;Session.close/Lsp.stopare idempotent (awith lsp.request()exit used to raisenot_found);Pty.readlatches EOF andPty.writechunks like every other bulk path;Watcher.errorstaysNoneafter a close from another thread;SandboxTimeoutjoins the hierarchy while remaining aTimeoutError(2aeff90).ToolExceptionfor a missing path as its description promises (e4e0d5a).TestEveryVerbHasAFixturecovers the four LSP types;resp_info.jsonsays proto 2;req_fs_find.jsoncarries a glob that matches something; the Rust corpus test round-trips every response (f1bfc4e).Review commits
0ed354fsimplify/dedup (prod −135 lines net): one owner for settle-and-bill, the destroy fan-out, the cancel-then-close pair, the regular-file export contract, the chunked write loop;Event.Sandbox/Tenantandjournal.closehad no production reader/caller.8993167declaration layout per/codeFile Organization (pure moves;aslclean on both GOOS).a496feccomment budget: 26 comment lines added, 76 removed across Go; Rust and Python tightened in their own commits.d945161docs aligned with the code (audit_log-conditional audit lines, exec 504 semantics, keep-alive caveats and the 8-connection cap, PreviewURL's api token,ReadFileTo, the Python bytes-not-streams exception, the cold-boot range,packaging/, stamps on unstamped benchmark rows, the preview_advertise host requirement, a wall-clock-bounded desktop warmup).Gates
Size
Branch diff: prod code +799 −683 (net +116), test code +555 −409, docs +65 −47. The prod increase is the fixes (door limiter, config validation helpers,
closeOnCancel,RequireRegular,require_field/_need,SandboxTimeout, the MCP signal path); the review commits are net negative (−135 in the simplify commit, −50 comment lines in the budget commit).Hardware
Baseline on main 21dc4e7 (cocoon-test1, isolated cocoon root, ghcr 24.04-5158a14 guests): sandboxd-e2e 22.0 s, egress none 3.9 s / egress 7.2 s, socks 16.3 s, intercept 3.8 s, archive 41.4 s, desktop 51.4 s — 7/7 PASS, warm claims 0.4–1.2 ms. The same seven legs on this branch's binaries and a silkd overlay follow as a comment.