Repository navigation
review: guest-boundary fixes, SDK fixes, cuts, control-plane keep-alive - #278
Merged
Merged
Conversation
…te methods follow the public ones newJournalWriter joins journalWriter, Client.dialAgent trails upgradedConn (/code producer rule), and silkd's Table/session pub(crate) helpers move below the pub methods (/code-rs public-above-private).
…elds, min for the audit line clamp
…rop comments that restate the code
…strings to one line; __init__ returns annotated
guestPortConn.Read used bufio.ReadBytes, which buffers an unterminated line without bound. A guest root that replaces silkd on the agent port can stream one never-ending port_forward data frame and grow sandboxd until the host OOM-kills it, taking every tenant's control plane with it. Every other guest-frame reader is capped at wire.MaxFrame. readFrame takes the line with ReadSlice: a canonical data frame fits portReadBuf and is decoded straight out of the reader's buffer, so the per-frame allocation and copy ReadBytes made is gone too. Only a frame larger than the buffer is assembled, and that stops at MaxFrame.
silkdList and silkdReadFile appended every frame the guest agent sent. A tenant can promote a template whose agent is hostile; a claim of it with a volume then makes MountVolume's 2 s device probe collect as much as the guest streams. Both helpers only read /sys/block and a sysfs serial file, so cap them at 4096 entries and 64 KiB (the largest sysfs page).
HTTPPuller drained the response body on every return. After Untar failed partway (ENOSPC, a corrupt or truncated tar) the drain kept downloading the rest of a multi-GB record into io.Discard until the per-owner budget ran out, and only then did the healer try the next owner. One pull per heal gains nothing from connection reuse, so close the body without draining.
…rame A silkd before proto 2 answers a failing fs_write or fs_push with an error frame and closes. uploadRPC kept sending Data frames until one hit EPIPE and returned that, dropping the typed reason already read into terminal. A failed send now waits for the terminal read, which the closed connection ends at once, and prefers its error. The lease is not marked reusable on that path.
…r frame Same defect as the Go SDK: a silkd before proto 2 answers a failing fs_write or fs_push with an error frame and closes, the relay closes the client once its guest write fails, and the next chunk raised ProtocolError(broken pipe) with the typed reason unread. write_file and push now share _upload, which reads the terminal frame after a failed send and raises it, else the send error. FakeAgent gains slam, the relay's close on a failed guest write.
…y config-owned field GET /v1/info is root-only, so an MCP server started with a tenant token was refused on every node_info call with nothing in the tool description or docs saying why. PoolSpec's godoc named egress only, while PUT /v1/pools also rejects warmup, capture_trim and storage.
writeEnv refused a guest change on a paused claim but not a failed one, so after the VMM exited a PUT or PATCH stored the env, wrote to a dead vsock and answered 500; the docs tell a caller to resend after a 500, and every resend failed the same way. Every sibling verb already refuses a failed claim. Host-only changes stay accepted.
silkd runs as root, and git 2.35.2+ refuses a repository whose owner is someone else ("detected dubious ownership"), so every git verb but clone failed on a repo a workload created as an exec user or that fs_push extracted with the archive's uids. git_cmd now drops to the owner of the directory when silkd is root and root does not own it, with that user's HOME and USER when the uid has a passwd entry. This keeps git's guard against repo-controlled config running as root, which safe.directory=* would have dropped, and objects git writes stay owned by the repo's owner.
Verified as root on linux/arm64 (rust:1.98.1, git 2.47.3): the new test fails on main with the dubious-ownership error and passes with the fix, for an owner with a passwd entry (65534) and without one (501).
…nal mode fs_write and write_atomic (fs_replace) opened a predictable <path>.silkd-<pid>-<seq>.tmp with O_CREAT|O_TRUNC and chmod'ed it by path at commit. A non-root guest process that can write the destination directory could plant symlinks at the next names, and root silkd then wrote the upload through the link and chmod'ed its target. The temp file also sat at 0644 for the whole upload when 0600 was asked. create_tmp opens with create_new (O_EXCL, never follows a link) under the resolved mode and fixes the exact bits through the handle (fchmod), so a planted name fails the write with EEXIST and the file never exists wider than asked. Syscall count is unchanged on the upload path. Verified as root on linux/arm64 and on macOS.
…e broken pipe When tar refused an archive and exited before the stream ended (GNU tar on a compressed stream without -z), the next write to its stdin failed with EPIPE and push_staged answered "write: Broken pipe". A failed stdin write with a failed tar now reports tar's stderr; a bad or truncated client stream still wins. The test fails on linux/arm64 GNU tar 1.35 before the fix; macOS bsdtar reads the whole stream, so it passes there either way.
…ero-copy path feed_client parsed every client frame through serde, while fs_write and fs_push already slice a canonical data frame's base64 straight out of the line. It now tries decode_data_frame first and moves the decoded Vec into the handler's channel, falling back to serde for any other shape. Interleaved A/B in one release run, order swapped each round (macOS arm64): 256 KiB frame 138.7-141.5 us -> 96.6-97.4 us (-31%), 32 KiB 18.2-18.6 us -> 12.3-12.5 us (-33%).
…kspace root silkd's fs_pull carries a tree under its own basename, so persist_workspace returned workspace/... members while the Agents SDK's snapshots are relative to the root; hydrating one pushed the files to /workspace/workspace/... and the agent saw an empty workspace. Strip the root's name with the SDK's own strip_tar_member_prefix (present since openai-agents 0.17.0, the declared floor), as its docker backend does. The test's pull now returns a silkd-shaped tar instead of opaque bytes, which had hidden the defect.
…ck's error CocoonToolkit.__exit__ let a release APIError replace the exception the block raised, so a failing agent run surfaced as "release: ... (HTTP 500)". It now mirrors Sandbox.__exit__: a release failure raises only after a clean block.
proxy_port split local_addr at the last colon and handed "[::1]" to create_server, which cannot resolve it, so an IPv6 listen address always failed while the Go SDK's ProxyPort takes it. Strip the brackets and bind AF_INET6 for an IPv6 host.
The ports relay answers 400 for port 0, but POST /preview minted a URL for it whose every request then failed 502. Refuse it at mint time with the relay's message.
…publishing Promote held the sandbox's transition lock through PublishDigested: a full-content sha256 of the export on the dir store, the upload on s3. Every exec, relay, port or wake on the source sandbox waited the whole time. The lock only pins the source snapshot during the export, so Promote now exports through exportSource, as Checkpoint does, and publishes after the lock is released; publishTemplate folds away (prod -13).
recDone kept a record's lock entry at zero references unless a delete had asked for eviction, so every checkpoint or template id this node claimed or fetched but never deleted itself left a mutex in recLocks for the life of the process; on s3 or a cluster, deletes happen on the owner, so that is every claimed checkpoint. A reference is taken before the lock is waited on, so at zero there is no holder and no waiter and a fresh mutex is equivalent: drop the entry there. recDoneEvict, evictIfPending and recEvict fold away (prod -26); the eviction-ordering tests hold as written.
…walk, noContent Five behavior-preserving folds from the 2026-10-01 loc-justify round (CMGS approved the cut-list): - wire scanTag skips siblings with jsontext's iterative SkipValue (depth cap 10000) instead of a hand-rolled walker; the depth error text changes (-23) - failLocked, restartLocked's tail and commitTransition share commitIfLive; resolvePendingSnap keeps its own lock-held read of the pending name (-19) - LoadCA and IssueIntermediate parse root and intermediate with one bundle parser, so a trailing non-CA block is rejected in every file (-15) - s3 publishFiles walks export/ once for both publish modes; object keys are unchanged and checkpoint publishes now reject a non-regular file like digested ones (-13) - seven 204 closures become noContent(w) (-10)
…past the grace A template id is a hash of its key, so every re-promote of the same name is a new generation of one record, and each node that fetched one kept every generation's full export under staging/cache/<id>/ until restart or Delete. A cache hit now refreshes the generation's mtime, and installing a new generation removes siblings no fetch used within store.GenerationGrace, the bound the dir backend already gives a clone of a superseded generation (moved from dir to store so both share it). Hot-path cost: one utimes per cache hit.
Every claim, release, renew, owner or peers call opened a new connection through urllib, and a TLS handshake over https. Client now parks up to 8 idle http.client connections per origin and reuses one when it is younger than keep_alive and a zero-timeout select shows the node has not closed it; keep_alive=0 keeps dialing per call. A failed request is never retried, so a claim cannot be sent twice; a 4xx/5xx whose body breaks off keeps its status, as before. Interleaved A/B against a Go HTTP server on loopback, 2000 claims per arm, order swapped: p50 403.2/352.6 us (old) -> 81.0/73.1 us (new).
A guest user that can write a push destination could swap silkd's root-owned staging directory for a symlink between its creation and tar's chdir, so root tar extracted wherever the link pointed; the merge then walked staging and destination by path, so a swapped path made root rename the link target's files into the destination, and a destination subdirectory swapped between the check and the recursion redirected the merge. stage_dir now creates staging with mkdirat (0700), reopens it O_NOFOLLOW and requires silkd's uid, which no guest user can forge. tar starts in that fd through fchdir in pre_exec, and merge_tree walks openat/renameat/unlinkat with O_NOFOLLOW at every level. The destination path itself is still taken as the client gave it. The new test swaps the staging path for a symlink to a victim directory between stage_dir and push_staged; the same sequence on main moves the victim's file into the destination. Running it as root on linux/arm64 with /tmp on overlayfs caught a stale listing through the pre-extract fd, so merge_tree lists through a fresh openat of ".". nix gains the dir and fs features (no new crates).
read_file stat'ed the path before reading it, one extra relay RPC per call: silkd's fs_read has checked the kind on the opened fd since the same release, so the precheck only caught what silkd refuses; guests before v0.1.12 lose it. trackBox released a claim that landed after closeBoxes, but main returns once closeBoxes does, so that release raced process exit; such a claim is left to its tool lease, which docs/mcp.md now says (prod -17).
… /v1/pools drops the pool effectivePolicy reads the pool layer from the config-owned policy map by key, so a config pool with an egress block that PUT /v1/pools drops keeps applying its policy to later claims of that key; only a key without a config-file policy takes its layer from live pool membership at claim time. The page implied the dropped pool's key had no pool layer. Never more permissive than the config.
CMGS
changed the title
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 pass over every Go, Rust and Python file (296 files), plus the follow-ups from it. Every fix has a test that fails before it; performance changes carry interleaved A/B numbers.
Guest-boundary fixes (sandboxd, silkd)
guestPortConnread withReadBytes, so a guest root that replaces silkd could stream one never-ending frame and grow sandboxd until the host OOM-kills it. The newReadSlicepath also drops a ~350 KB allocation and copy per relayed frame.safe.directory=*, so repo config never runs as root.openat(".").Other correctness fixes
broken pipe.persist_workspacestrips the root basename, so a restored workspace is no longer empty.withblock.proxy_portbinds a bracketed IPv6 address.Performance
keep_aliveand only after a liveness probe, never retried. Claim p50 against a Go server on loopback: 403/353 → 81/73 µs.store.GenerationGrace. A cache hit refreshes the generation's use time.Cuts (behavior-preserving)
SkipValuereplaces the hand-rolled tag-scan skipper.commitIfLivefolds three persist-if-live tails.export/.noContentreplaces seven identical 204 closures.Review commits
Layout (type blocks own their constructors and producers), Go 1.27 idioms (
strings.CutLast,new(expr),min/max), and a comment-budget pass. Plus docs: Info/node_info need the operator token,PoolSpeclists every config-owned field, and a config pool's egress policy stays bound to its key afterPUT /v1/poolsdrops the pool.Numbers
Production code: Go −119, Go scaffold −8, Python +46, Rust +80 (net −1). Tests +379. Comment lines in Go/Rust: +26/−60.
Verification
make go-lint: 10 ×0 issues.(5 modules × linux/darwin).asl -forwarder=false: 0 findings on both GOOS.make go-test: 19 ok.GOOS=windows go vet: clean.make lintandmake teston macOS (159 passed); on linux/arm64 as root, boot/init 17 and silkd 144 passed, fmt/clippy clean.Not changed here: 209 in-body Go comments that start lowercase (the casing rule is deferred).