fix: close device, stream, and preview lifecycle gaps by CMGS · Pull Request #81 · cocoonstack/sandbox

CMGS · 2026-08-13T16:13:34Z

Summary

Started as three lifecycle gaps found by a close-repo review; a whole-design
audit of all 392 tracked files then turned up several more, including two
tenant-isolation holes that were runtime-proven, not inferred. Everything below
is on this branch.

Isolation and cluster correctness

A tenant could claim any other tenant's promoted template by name.
PoolKey.Hash() has no tenant axis and resolveGolden decoded the template
record only to discard rec.Tenant, so promote and delete were tenant-scoped
while the read path was not. Proven: acme promoted acme-private, beta
claimed the same key and got back acme's content digest — beta booted acme's
snapshot. A foreign template now resolves as absent, byte-identical to a name
that was never promoted, so existence stays unconfirmed. Root-promoted
templates stay shared, as pools are.
A tenant with no egress block inherited its pool's whole allow-list,
secret injections included, so adding a tenant token handed out the
operator's credentials. Both single-sided branches now deny: pool ∩ tenant,
with a missing policy on either side denying all; root has no tenant layer and
takes the pool policy whole. Verified first that ok=false really denies —
no listener means the egress-lane NIC stays nft-locked and the none-lane dial
is refused. Upgrading: a tenant that relied on inheriting must now declare
its own block.
A dead peer came back and stayed forever. forget dropped it, but merge
re-inserted any node it did not already know, so one push/pull from a peer
that had not yet seen the death resurrected it — permanently, since SWIM never
re-fires NotifyLeave — and re-gossiped it cluster-wide. SWIM membership is
now the authority: merge accepts gossip only about nodes it currently
reports. (An earlier epoch-tombstone commit had two residuals of its own —
memory-only, and entries that never freed — which this replaces.)

Lifecycle

Boot waits for the /dev node, not just the sysfs serial: the kernel
publishes sysfs first, and resolve_disks mounts with no retry. Same class as
the settle race measured at 3.5% of four-volume claims on bare metal — this
was its third instance, alive in boot/init for six weeks.
fs_watch overflow signals by dropping the sender rather than setting a flag
a parked loop could miss; the buffered ordered prefix is delivered before the
terminal error.
Preview: the pooled transport let a reused request skip PreviewDial, and
with it wakeResolved and the Transition lock that serializes a claim
against its own hibernation. Every request dials again.
Egress pools reject idle_hibernate_seconds (the lane refuses to hibernate,
so the setting could only ever fail), and the sweep skips egress claims of
unpooled keys, which the config gate cannot reach.
Go SDK Pty.Close is idempotent and unblocks an unread reader.

Supply chain and gates

Every base image was a floating tag and rust-toolchain said stable, so the
kernel build, sandbox-init and silkd were reproducible only by luck. Bases
are pinned by digest and both toolchains to the version the Linux gate runs;
dependabot owns refreshing them, because a pinned digest goes stale
silently.
shellcheck had no gate at all (all seven scripts already passed) — make sh-lint plus a workflow.

SDK surface

APIError is exported with its status: the SDK's own redirect walk branches
on it, so callers need it too. WithHTTPClient for a caller's own transport
or deadline; no blanket client timeout, because checkpoint and promote block
for as long as the snapshot takes.
engine is a pool-key axis the SDK never carried, so an Info → SetPools
round trip — a declarative full replace — silently drained every fc pool.
The two pool gauges omitted it too, so two pools differing only by engine
emitted one label set and the scrape failed.
claim_ref and the listing that reads it back now exist on both SDKs; Python
gains attach, drain, uncordon (the OpenAI adapter was hand-building a
Sandbox around the missing attach), ExitError.stdout, Pty.exit_code,
and Watcher.error. Go gains Checkpoint(id) — MCP was listing every
checkpoint on the node to resolve one id.
MCP claimed with the node's 5-minute default and renewed nothing, so an agent
session outliving it lost every sandbox mid-conversation: it now claims for an
hour, accepts net/size, and releases what it claimed when the session ends.

Docs

Eleven factual corrections against the code (the engine axis, the /metrics
inventory, secrets and egress_internal_allow, the keyed HEAD probe's 401,
the audit op name, which verbs are connection-bound, volume discovery scope),
the node-operation verbs cluster.md told operators to use but sdk.md never
documented, and the checkpoint-delete lifecycle reduced from three copies to one.

Validation

make go-lint (golangci run + fmt, dual GOOS, 5 modules) — 0 issues
asl ./... dual GOOS across the Go modules — 0 findings
go test -race -count=1 ./... — green in every module
cargo fmt --check, clippy --all-targets -D warnings, cargo test for
both Rust crates, in a Linux container on the pinned 1.97.1 toolchain
(silkd 13 suites, boot/init 17 tests)
ruff check + pytest (153) for the three Python packages
make sh-lint — 0 findings
Runtime proofs for the two isolation holes and the resurrection bug, each
written as a throwaway test, run against this branch, and deleted
The memberlist ordering the mesh change depends on is verified in upstream
source (v0.5.4 net.go:1302-1307): membership merges, firing NotifyJoin
synchronously, before the user-state delegate

Hardware round on the final branch (.79 bare metal, main@8f615d5 vs
branch@64c1aff, logs in ~/pr81-ab/logs/):

Claim hot path unchanged: settled warm volume-less claims, ABBA over four
daemon lives, 16 samples each, every sample warm-tier-proven, 0 flakes — main
p50 0.489/0.488 ms vs branch 0.482/0.475 ms; |A−B| is inside the A/A drift.
The tenant-gated template resolution and the egress fail-closed change cost
nothing measurable.
Boot unchanged: 25 interleaved traced boots per arm — the device-node
gate costs +4 µs in the resolve phase (p50 83→87 µs); total init→handoff
p50 1387→1352 µs, branch faster within noise.
Preview: main's pooled transport 0.216/0.227 ms vs the branch's
per-request re-dial 0.427/0.447 ms = +0.21 ms/request — the deliberate,
documented cost of per-request revocation/wake serialization (see the
preview commit).

One transient e2e suite failure was observed once during development and
analyzed: fail-fast shape (suite finished 1 s faster than a pass), occurred
while a second go test -race of the same package ran concurrently in the same
checkout, and did not reproduce in 87 subsequent runs across idle, CPU-loaded,
and faithfully-reproduced concurrent conditions, with CI green on the same
commit. Treated as environmental; the two tests that own the suite's wall time
(both waiting on the refill ticker) are where to look if it ever recurs.

Test-hygiene findings from the audit (13 of 79 Go test files, silkd's untested
git_push/git_pull, the Rust response-encode fixtures) are deliberately
deferred to a follow-up PR.

The overflow flag was only checked at the loop top, so a callback preempted between a full try_send and the store could leave the loop parked on an empty channel forever - a silent quiet watch, the exact loss the terminal error exists to prevent. Dropping the sender makes the closed channel the wakeup itself: the buffered ordered prefix drains first, then recv() returns None and the terminal overflow error goes out. Deletes the Arc/atomic machinery outright. The pty close error never reached readers (pipe readers always see ErrClosedPipe), so plain Close replaces CloseWithError.

DisableKeepAlives paid a measured +0.23ms relay dial per request to get per-request revocation, activity, and audit. PreviewTouch does the same three against the claim map for microseconds, so the kept-alive pool returns for sub-resource fan-out; the idle pool stays keyed per claim. Measured on .79: preview seq p50 back to the pre-change band (0.17-0.21ms vs 0.44ms), warm claims unchanged. deploy.md documents the bearer-token payload and the shared browser origin under one preview_advertise.

Reattach the preview_advertise doc to its field at one line, drop two edit-narration clauses and a restating test comment, compress the watch overflow WHYs, and make PortConn.Close match Pty.Close (the writer-side error value never reaches readers).

resolveGolden decoded the template record and dropped it, so a tenant token could claim any other tenant's promoted template by name and boot its snapshot - proven with a test that saw the owner's content digest come back under a foreign tenant. Promote and delete were already tenant-scoped; only the read path was not. A foreign template now resolves as absent, which is byte-identical to a name that was never promoted, so existence stays unconfirmed. Root-promoted templates remain shared, as pools are.

forget dropped a dead peer from the view, but merge re-inserted any node it did not already know, so one push/pull from a peer that had not yet seen the death resurrected it - permanently, since SWIM never fires NotifyLeave twice, and the node was then re-gossiped cluster-wide. Tombstone the epoch it left at: a lagging peer replaying that epoch is ignored, while the node's own restart always seeds a higher epoch and clears the tombstone.

The audit op is preview, once per request, not preview_dial. pty_open, lsp_request and port_forward are connection-bound alongside fs_watch. Volume discovery returns the gossiped union only to root; a tenant sees this node's local entries its ACL permits.

The Forget-mid-flight test spun on an atomic with no deadline, so a probe that never arrived hung until the binary timeout instead of failing; wait on a channel the handler closes. The swallows-failures test never checked the healthy peer was reached, so it passed even if the fan-out stopped at the first error - count that peer's hits.

A tenant configured without an egress block inherited the pool's entire allow-list including its secret injections, so adding a tenant token handed that tenant the operator's credentials - the opposite of the documented intersection. Both single-sided branches now deny: a tenant reaches only what both layers allow, and a pool without a policy grants nothing to anyone. Root keeps the pool policy whole, having no tenant layer to intersect.

Document engine as the fourth pool-key axis (claim body, pools table, and the three key examples that omitted it), list the archived/draining/digest-mismatch gauges and the archive counters /metrics actually emits, add the secrets and egress_internal_allow rows, name the 401 the keyed HEAD probe answers, and say the usage journal's key is the pool key's hash. Also: the audit record's egress decision/secret fields, sandboxd.dbg in the release assets, browser is amd64-only while base is multi-arch, browser in the os-image list, bench.sh and release.yml in the repo map, and the nft lock drops guest-initiated packets. Drops a third-party registry namespace from the android README.

Guest connection pooling let a reused preview request skip PreviewDial, and with it wakeResolved and the Transition lock that serializes a claim against its own hibernation: once the idle sweep passed its re-check and entered the seconds-long snapshot, a pooled request wrote into a freezing VM and never triggered the wake. Every request dials again, so authorization, the activity stamp, the audit record and the wake all ride the one path. Egress pools accepted idle_hibernate_seconds even though the lane refuses to hibernate at all, so the setting could only ever produce a failure. Config now rejects it, and the sweep skips egress claims of unpooled keys, which the config gate cannot reach - futile work whose only symptom was a logged error every tick, so nothing observable is left to assert.

The epoch tombstone the previous commit added carried two residuals of its own: it lived only in memory, so a restart forgot every death and a lagging peer could teach them back, and its entries were dropped only when the same node returned with a higher epoch, so a permanently retired node_id stayed forever. Membership already answers the question - merge now accepts gossip only about nodes SWIM currently reports, and NotifyJoin/NotifyLeave maintain that set. Both maps are bounded by live membership and rebuilt from SWIM on restart, and a node returns to the view by rejoining, not by out-numbering a tombstone.

Every base image was a floating tag, so the kernel build, sandbox-init and silkd were reproducible only by luck, and rust-toolchain named 'stable' — a new release could change codegen or fail -D warnings with nothing changed here. Bases are pinned by digest and both toolchains to the version the Linux gate actually runs. A pinned digest goes stale silently, so dependabot owns refreshing them (plus cargo, gomod and actions) rather than a human remembering. shellcheck had no gate at all: all seven scripts already pass, so this locks in what is already true.

httpError becomes APIError: the SDK's own redirect walk branches on the status, so a caller distinguishing gone from full from mid-heal needed the same field Python already exposes. WithHTTPClient lets a caller supply its own transport or deadline; the client sets no blanket one, because checkpoint and promote block for as long as the snapshot takes and ctx is where a Go caller says otherwise. claim_ref had no SDK at all despite being the aggregated apiserver's hook, and neither did the listing that reads it back, so both sides gain WithClaimRef / claim_ref and Sandboxes() / sandboxes(). Python gains attach, drain and uncordon: the OpenAI adapter was reaching around the missing attach by constructing a Sandbox by hand. MCP claimed with the node's 5-minute default and renewed nothing, so an agent session outliving it lost every sandbox mid-conversation; it now claims for an hour, says so in the tool description, accepts net and size, and releases what it claimed when the stdio session ends.

…mantics cluster.md told operators to call SetPools and Drain, which docs/sdk.md never mentioned despite claiming a per-method reference; both SDK pages now carry them alongside the new claim_ref and sandboxes surfaces. The checkpoint-delete lifecycle was written out in full in three places, so the API page keeps what a caller of that endpoint needs and links cluster.md for the rest.

exec() threw stdout away on a non-zero exit, so a failing build's log was lost where Go returns it alongside ExitError. Pty dropped the shell's exit code the exit frame carries, and Watcher ended iteration identically for a clean close and a dropped relay, so a caller could not tell them apart. Go gains Checkpoint(id), which is what Python already had and what MCP needed — it was listing every checkpoint on the node to resolve one id, and only ever saw the connected node's listing. The two adapters floored the base SDK at 0.1 while calling APIs that landed in 0.1.5.

ExitError now carries the stdout produced before the failure, Pty exposes the shell's exit code, and a Watcher tells a dropped relay from a clean close.

…he docs This PR made engine a documented pool-key axis, which exposed two places that never had it. The SDK's PoolKey and PoolSpec omitted it, so an Info-then- SetPools round trip — a declarative full replace — silently drained every fc pool. The two pool gauges omitted it too, so two pools differing only by engine emitted the same label set and the scrape failed. The rest is truthfulness: resolveGolden's godoc still claimed only a true absence cold-boots, which the tenant gate changed; watch.rs still called itself the one connection-bound verb the docs page just stopped saying; the SetPools cluster example did not compile; ClaimRef claimed to be empty on warm-pool claims that in fact carry it; and the egress intersection change needed to say out loud that a tenant inheriting its pool's policy now reaches nothing. PortConn keeps CloseWithError(net.ErrClosed): it is a net.Conn, and callers test for that sentinel — only Pty, which is not, takes the plain Close.

The new shellcheck gate failed on its first CI run: SC2015 flags the cleanup idiom because C runs whether or not B did, which here is the intent but reads as if-then-else. Older shellcheck releases report it and the version this laptop has does not, so the code says what it means instead.

The drift guard had no assertion for either, and engine is exactly the axis whose absence from the SDK key silently drained fc pools on an Info-SetPools round trip. Mutating the tag to json:"-" fails this test.

HasPromotedTemplate was tenant-blind while resolveGolden is not, so the two disagreed for a foreign tenant: routing saw a local golden, skipped the peer hop, and the claim cold-booted with routing believing a template had served it. Both now apply the same tenant test. The contract, stated precisely: a plain claim of a foreign name behaves exactly like a claim of a never-promoted name - the template axis is an image ref, so it cold-boots the public image or fails provisioning, and the owner's content is never touched. Answering 404 instead would be worse on two counts: it hands every tenant an existence oracle over other tenants' template names, and it lets one tenant deny everyone else a public image name by promoting a private template under it. Only require_promoted (the volume path, redirect retries) answers ErrUnknownTemplate. tplSet caches the owning tenant next to each id, so the ownership test stays in memory: the claim path must not grow an s3 read per warm miss.

create_sandbox claims for an hour, but fork passed a zero TTL and a branch claim set no timeout, so both fell back to the node's 5-minute default while being session-tracked with nothing to renew them - the exact mid-conversation expiry the create fix closed. Both now use the same lease and their tool descriptions say so. Also drops deploy.md's stale CH-only tag: Firecracker is a live opt-in engine axis, and the sentence's real content is that old state is not converted.

The fc opt-in never carried its weight: it offered nothing CH does not (volumes, checkpoint, fork, hibernate and the restore modes are all CH; clone performance measured equal), it had zero e2e or hardware coverage, and the axis never made it through the template lifecycle - DELETE built its key without engine, so an fc-promoted template could never be deleted by name, and no SDK could send or read the axis at all. Rather than thread a one-value-in-practice axis through claim, promote, handles and delete, remove it: PoolKey is (template, net, size) again, the hash drops the engine component, the volumes-require-ch guard and the fc test rows go with it, and the pool gauges return to three labels. cocoon keeps --fc for anyone driving it directly. Old pool-key hashes change; upgrades already never convert state.

TemplateOwners answered from raw key hashes, so a foreign tenant's promoted template escalated another tenant's claim: a volume claim that would have cold-booted locally was forced onto the promoted path and refused, and a plain claim earned a useless redirect that also signalled the name exists. The local check is tenant-aware; the remote one had nothing to be aware with — gossip deliberately carries no tenant. Scope the hash instead: gossip advertises hash(keyHash|owner), and an owner query probes the requester's own scope plus the operator's (root probes every configured tenant). A foreign template simply never matches, which is exactly how the claim path treats it, and the wire still carries nothing but opaque hashes. Salting costs two sha256 on the warm-miss redirect path only; mixed-version meshes disagree on hashes, and upgrades are lockstep.

CMGS marked this pull request as ready for review August 13, 2026 18:16

This was referenced Aug 13, 2026

fix: close preview and idle hibernate gaps #83

Closed

fix: stop shipping a stale silkd, and unbreak the Python stdio paths #84

Closed

test(python): pin all four enum sets, and the git_branch actions in use #85

Closed

CMGS added 11 commits August 14, 2026 03:27

fix: close device, stream, and preview lifecycle gaps

a7393e6

CMGS force-pushed the codex/close-repo-review branch from 4183bb3 to a951bf8 Compare August 13, 2026 19:29

CMGS added 10 commits August 14, 2026 03:38

docs: record the Python exit, pty and watch contracts

84cc8e7

ExitError now carries the stdout produced before the failure, Pty exposes the shell's exit code, and a Watcher tells a dropped relay from a clean close.

review: cut the restating half of the admit godoc

bc7e7b3

test(e2e): pin claim_ref and the engine axis on the wire

1198869

The drift guard had no assertion for either, and engine is exactly the axis whose absence from the SDK key silently drained fc pools on an Info-SetPools round trip. Mutating the tag to json:"-" fails this test.

CMGS force-pushed the codex/close-repo-review branch from 977a3bf to 64c1aff Compare August 13, 2026 20:25

CMGS added 2 commits August 14, 2026 10:54

CMGS force-pushed the codex/close-repo-review branch from 64c1aff to 5d8b083 Compare August 14, 2026 02:55

review: compress the tenant-cache comments to their WHY

4931efc

CMGS mentioned this pull request Aug 14, 2026

sandboxd: drop the Firecracker engine axis #86

Merged

CMGS added 2 commits August 14, 2026 11:27

CMGS force-pushed the codex/close-repo-review branch from 9c8b31d to 89304e5 Compare August 14, 2026 04:10

CMGS merged commit cb37b5c into main Aug 14, 2026
5 checks passed

CMGS deleted the codex/close-repo-review branch August 14, 2026 04:19

Navigation Menu

Uh oh!

fix: close device, stream, and preview lifecycle gaps - #81

fix: close device, stream, and preview lifecycle gaps#81
CMGS merged 26 commits into
mainfrom
codex/close-repo-review

CMGS commented Aug 13, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Conversation

CMGS commented Aug 13, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Isolation and cluster correctness

Lifecycle

Supply chain and gates

SDK surface

Docs

Validation

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

CMGS commented Aug 13, 2026 •

edited

Loading