review: stop during a capture, reader leak, tenant cap, egress audit, silkd hang-ups, tests that fail

.github/workflows/python.yml

@@ -24,6 +24,9 @@ on:
       - ".github/workflows/python.yml"
   workflow_dispatch:
 
+permissions:
+  contents: read
+
 jobs:
   test:
     runs-on: ubuntu-latest

Makefile

@@ -43,12 +43,12 @@ help: ## show this list
 	@grep -E '^[a-z-]+:.*##' $(MAKEFILE_LIST) | awk -F ':.*## ' '{printf "%-14s %s\n", $$1, $$2}'
 
 test: ## Rust tests: boot/init + silkd
-	cd boot/init && cargo test
-	cd silkd && cargo test
+	cd boot/init && cargo test --locked
+	cd silkd && cargo test --locked
 
 lint: ## Rust fmt --check + clippy -D warnings: boot/init + silkd
-	cd boot/init && cargo fmt --check && cargo clippy --all-targets -- -D warnings
-	cd silkd && cargo fmt --check && cargo clippy --all-targets -- -D warnings
+	cd boot/init && cargo fmt --check && cargo clippy --locked --all-targets -- -D warnings
+	cd silkd && cargo fmt --check && cargo clippy --locked --all-targets -- -D warnings
 
 sh-lint: ## shellcheck every tracked shell script
 	git ls-files '*.sh' | xargs shellcheck
@@ -78,7 +78,7 @@ boot-debug: ## boot image with busybox + /bin/sh on fatal errors
 	docker build -t $(BOOT_IMAGE)-debug --build-arg KERNEL_VERSION=$(KERNEL_VERSION) --build-arg KERNEL_MIRROR=$(KERNEL_MIRROR) --build-arg INITRD_DEBUG=1 boot
 
 extract: ## dump /boot artifacts into dist/ for boot-bench.sh
-	rm -rf dist && mkdir -p dist
+	rm -rf dist/boot && mkdir -p dist
 	cid=$$(docker create $(EXTRACT_IMAGE)) && docker cp $$cid:/boot dist/boot && docker rm $$cid
 
 extract-debug: ## extract from the boot-debug image

docs/cluster.md

@@ -341,11 +341,12 @@ checkpoint store and keeps each claim's registry row on the latest snapshot:
   the next round, a release, an archive, a wake, the hourly sweep and a restart
   settle it by reading the row: a row on that checkpoint promotes it, another
   owner's row drops the claim, and an absent row means the adopter finished
-  when the checkpoint is gone and a write that never landed when it is still
-  there. A refused write reads the row: one this node still owns only had a
+  when the checkpoint is gone or this node confirmed an earlier row, and a
+  write that never landed otherwise. A refused write reads the row: one this node still owns only had a
   stale epoch and is refreshed, and an absent or foreign row drops the claim and
-  its VM. A claim gets its first replica one interval after it is claimed (or
-  after a restart). A hibernated, archived or failed claim,
+  its VM. A claim gets its first replica one interval after it is claimed, after a
+  restart, or after it wakes from an archive or an adoption; until a claim woken
+  that way commits its first replica it has no row, and it is lost with its node. A hibernated, archived or failed claim,
   one whose tenant was removed, and one with volumes (capture refuses them) is
   skipped; a replicated claim that gains volumes
   drops its row and replica. The claim's policy pool, not its own template key,

docs/deploy.md

@@ -147,7 +147,7 @@ the intersection of the two.
 | `preview_secret` | — | cluster-shared HMAC secret signing preview tokens (all nodes share one) |
 | `preview_advertise` | = `preview_listen` | the browser-facing preview base URL, minted into every preview URL, so it must name a routable host (a wildcard `preview_listen` needs it set explicitly); nodes behind one TLS proxy may share it, while signed tokens route internally through each owner's `advertise_addr` |
 | `checkpoint_dir` | `<data_dir>/checkpoints` | where checkpoints and promoted templates live. Point it at a shared FUSE mount (JuiceFS over object storage, NFS) and every node sharing the mount can branch every checkpoint — records are generation-addressed with `meta.json` as the atomic commit pointer, so no cross-node locking is required of the filesystem. A re-publish retains its superseded export generation for at least ~1h and until a following hourly sweep so an in-flight clone that resolved the old metadata can finish; budget the current generation plus every generation retained across that grace-and-sweep window. An explicit delete can make a concurrent clone fail visibly. One contract on any shared root (mount or bucket): a template key has a single writer — promotes go to the sandbox's owner node, and operators must not race promotes of one name from different nodes (checkpoint ids are node-generated and never collide). Abandoned staging (`*.tmp`) is removed only once it is older than an hour, at startup and on the hourly generation sweep, so a publish in flight on another node of a shared root survives; the `meta.json` commit pointer is fsynced with its directory |
-| `checkpoint_store` | dir | checkpoint AND promoted-template backend (both live in one store root, id-namespaced ck_/tp_): `{"kind": "s3", "s3": {"bucket": "…", "prefix": "ck/", "endpoint": "…", "region": "…", "force_path_style": true}}` stores checkpoints in object storage (any node claims any checkpoint, no shared mount needed). Credentials come from the standard AWS chain (env/IAM role), never this file. Holes and all-zero chunks are never uploaded, and a fetch recreates the file at its full size and writes only the present chunks, so a mostly-empty disk costs its data, not its size, on the wire and on the fetching node. Consecutive captures of one claim upload only the chunks that changed (the first capture after a wake uploads everything again when no other live record of that claim remains, because the wake deletes the record it restored from). sandboxd never deletes a chunk or a manifest from the bucket; [Reclaiming S3 storage](#reclaiming-s3-storage) has the key layout and the operator's job. A publish or fetch is aborted after 30 minutes. Do not put an expiry lifecycle rule on the prefix: it would delete chunks a live record still names. Budget storage for every generation and chunk until your reclaim runs. When caching a new generation, each node prunes other cached generations of that record unused for at least an hour. The periodic generation sweep drops a node's cache of a record once the record is gone from the bucket and the cache is unused for at least an hour. Absent = the dir backend at `checkpoint_dir` |
+| `checkpoint_store` | dir | checkpoint AND promoted-template backend (both live in one store root, id-namespaced ck_/tp_): `{"kind": "s3", "s3": {"bucket": "…", "prefix": "ck/", "endpoint": "…", "region": "…", "force_path_style": true}}` stores checkpoints in object storage (any node claims any checkpoint, no shared mount needed). Credentials come from the standard AWS chain (env/IAM role), never this file. Holes and all-zero chunks are never uploaded, and a fetch recreates the file at its full size and writes only the present chunks, so a mostly-empty disk costs its data, not its size, on the wire and on the fetching node. Consecutive captures of one claim upload only the chunks that changed (the first capture after a wake uploads everything again when no other live record of that claim remains, because the wake deletes the record it restored from). sandboxd never deletes a chunk or a manifest from the bucket; [Reclaiming S3 storage](#reclaiming-s3-storage) has the key layout and the operator's job. A publish or fetch is aborted after 30 minutes. Each publish in flight holds up to 192 MiB of chunk buffers and sandboxd does not cap concurrent checkpoints or promotes, so the layer above it limits how many a tenant starts. Do not put an expiry lifecycle rule on the prefix: it would delete chunks a live record still names. Budget storage for every generation and chunk until your reclaim runs. When caching a new generation, each node prunes other cached generations of that record unused for at least an hour. The periodic generation sweep drops a node's cache of a record once the record is gone from the bucket and the cache is unused for at least an hour. Absent = the dir backend at `checkpoint_dir` |
 | `checkpoint_ttl_hours` | 0 (keep forever) | ages out checkpoints older than this; the sweep runs hourly and at startup. Explicit deletes never wait for it. Must be nonzero and match fleet-wide when `checkpoint_peer_heal` is on ([why](cluster.md#delete-is-eventual-not-a-fleet-wide-revocation)) |
 | `checkpoint_peer_heal` | false | on a cluster, lets a node pull a checkpoint it lacks from a peer — found via a live probe, not gossip — rather than failing the branch; see [placement lifecycle](cluster.md#checkpoints-on-a-cluster). Three requirements, all enforced at config load: a nonempty `api_token` (the blob transfer between peers authenticates with it; without one the raw record stream would be open), `mesh.cluster_key` set (the pull presents the fleet `api_token` to an address learned from the peer probe, so the gossip layer carrying that address must itself be authenticated), and `checkpoint_ttl_hours` nonzero. A shared checkpoint store (`checkpoint_store` kind `s3`) ignores this setting — every node already resolves every checkpoint directly, so there is nothing to heal |
 | `warm_max` (pool entry) | 0 (static) | turns on the demand-adaptive watermark for that pool: the warm target rises from `warm` toward `warm_max` while claims arrive faster than the measured provision lead covers, and decays back over ~a minute of silence. The pool follows the target down: warm VMs above it are destroyed once the count has stayed above it for a full decay period, so a burst does not leave the pool parked at its high-water mark |
@@ -655,6 +655,7 @@ ExecStart=/usr/local/bin/sandboxd -config /etc/sandboxd/config.json
 Restart=on-failure
 RestartSec=5s
 KillMode=process
+TimeoutStopSec=27min
 Environment=SANDBOXD_LOG_LEVEL=info
 
 [Install]
@@ -673,8 +674,12 @@ test the output for emptiness.
 
 Stopping sandboxd leaves VMs alive; the next start reconciles them. The unit's
 `KillMode=process` is what guarantees it: systemd signals sandboxd alone, so a
-VMM that cocoon could not move into its own scope, and any `cocoon` call still
-in flight, outlive the stop instead of dying with the service's cgroup. Claimed
+VMM that cocoon could not move into its own scope outlives the stop instead of
+dying with the service's cgroup. sandboxd starts no new `cocoon` command once
+stopped and waits for the running ones, since cocoon dies on its next log line
+once sandboxd's end of its stderr closes, and a hibernate killed that way leaves
+the guest paused. A save or hibernate can take 26 minutes, hence
+`TimeoutStopSec=27min`; stops normally take seconds. Claimed
 sandboxes are reaped when their TTL expires (default 5m, capped at 24h).
 
 A snapshot save, hibernate, export, restore or clone has 15 minutes. Once canceled, a

docs/egress.md

@@ -159,7 +159,8 @@ domain policy first; the allow-list widens the IP gate only.
 
 A [config reload](deploy.md#reloading-the-config) applies policy, internal
 allow, secret and upstream changes without a restart: a live claim's next
-request or connection is evaluated against the new policy.
+request or connection is evaluated against the new policy. Whether a claim has
+the SOCKS5 door is settled when it is claimed.
 
 Policy is per pool and per egress class; a tenant claim's effective policy is
 the intersection of its pool's and its tenant's class (a request must pass

docs/live-volumes.md

@@ -62,7 +62,7 @@ with client.new("rt:24.04", ttl_seconds=900) as sb:
 ```
 
 A successful run reads `v1`, then `v2`, and preserves every identity assertion.
-The repeated full target verifies that a settled mount can be retained.
+The repeated full target is a no-op on a settled mount.
 This example starts without other volumes. Retain any volumes you intend to
 keep in a full PUT, or use PATCH to replace only the dataset names. All mounted
 and attach-only entries support live removal in either mode.
@@ -72,16 +72,14 @@ mean an empty target.
 Go uses the same owning handle and client API token.
 
 ```go
-view, err := sb.PatchVolumes(ctx, sandbox.VolumePatch{
+v2 := sandbox.Volume{Name: "dataset-v2", Mount: "/datasets/current"}
+_, err := sb.PatchVolumes(ctx, sandbox.VolumePatch{
 	Remove: []string{"dataset-v1"},
-	Add: []sandbox.Volume{{Name: "dataset-v2", Mount: "/datasets/current"}},
+	Add:    []sandbox.Volume{v2},
 })
 if err != nil {
 	return err
 }
-if view.Converging {
-	return errors.New("volume request still running")
-}
 ```
 
 For a 409 mount conflict, use the guest path in `states[].error` to close

docs/sandboxd-api.md

@@ -477,7 +477,8 @@ for a blocked device call. Capture operations (hibernate, fork, checkpoint, and
 promote) are refused while either set is nonempty or a convergence request
 still owns admitted volume holds.
 
-Device calls honor cancellation and have a 30-second ceiling per volume; a
+Guest device calls honor cancellation and have a 30-second ceiling per volume, and a
+`cocoon` attach or detach can take 30 more to exit; a
 caller that disconnects cancels the run, leaving interrupted states to retry. Journal writes
 are atomic. Host-crash durability follows `sync_claims`. An older daemon
 ignores `VolumesWant`; finish pending volume changes before downgrading.
@@ -651,8 +652,8 @@ the export they were set on, which the new one replaces.
 
 400 invalid name, 401 bad api token, 409 when the name collides with a
 configured pool, the template is owned by another tenant, or the sandbox is
-on the egress lane, has volumes attached (see [egress](egress.md)), or is
-archived (`POST /wake` or an exec wakes it first), 404 unknown id or wrong
+on the egress lane, has volumes attached (see [egress](egress.md)), has failed,
+or is archived (`POST /wake` or an exec wakes it first), 404 unknown id or wrong
 sandbox token, 429 while the disk is under [`disk_reserve`](deploy.md), 503
 `lease lost; retry` for a replicated sandbox while its node's lease is overdue.
 
@@ -665,7 +666,8 @@ success, 404 unknown template, 409 when the key belongs to a configured pool
 (those goldens are owned by the node config). `digest=<content digest>` deletes
 only while the record still has that digest and answers 412 otherwise, so a
 caller that observed an older build never removes the newer one that replaced
-it. On a cluster, a node that does not
+it on this node; on a shared store a promote committing on another node at the
+same moment is not covered. On a cluster, a node that does not
 hold the template but sees an owner in gossip answers `200
 {"redirect": [addrs]}` — the claim redirect shape — and the SDK retries the
 delete at the owner. The retry carries `no_redirect=1`, mirroring the claim
@@ -684,7 +686,7 @@ Labels follow the [claim `metadata`](#post-v1claim) rules — at most 16 pairs,
 the same key and value grammar, at most 4 KiB as JSON — and replace the whole
 map; `{}` clears it. `digest=<content digest>` writes only while the record
 still has that digest and answers 412 otherwise, so a writer that observed an
-older build never relabels the newer one. They are stored beside the template's record, so they
+older build never relabels the newer one on this node. They are stored beside the template's record, so they
 survive a restart, are listed by the node that promoted the template or loaded
 it at startup, and go with the template when it is deleted; a re-promote starts with none. A tenant may label
 only templates it promoted — anything else is 404, root labels anything. 204 on
@@ -872,8 +874,8 @@ answers `200 {"checkpoint": {id, name?, sandbox_id, key, tenant?, created_at,
 policy_source?, no_egress?, guest_env?}}` — `tenant` records the calling tenant, absent for root.
 400 bad body or name, 401 bad api token, 404 unknown id or wrong sandbox
 token, 409 egress-lane sandbox, one with volumes attached (see
-[egress](egress.md)), or an archived one (`POST /wake` or an exec wakes it
-first), 429 while the disk is under [`disk_reserve`](deploy.md), 503 `lease lost; retry`
+[egress](egress.md)), a failed one, or an archived one (`POST /wake` or an exec
+wakes it first), 429 while the disk is under [`disk_reserve`](deploy.md), 503 `lease lost; retry`
 for a replicated sandbox while its node's lease is overdue.
 
 ## POST /v1/checkpoints/{id}/claim
@@ -1062,7 +1064,7 @@ template clone starts with the document its source held. The node writes it to
 is a no-op. silkd serves the address only in images that alias it on `lo`.
 
 204 on success; 400 when the body is not one JSON object within the bound; 404
-unknown id; 409 when the sandbox is hibernated, mid-transition or archived (this
+unknown id; 409 when the sandbox is failed, hibernated, mid-transition or archived (this
 verb never wakes one) or its image serves no instance metadata.
 
 ## GET /metrics

e2e/e2e_test.go

@@ -37,11 +37,11 @@ const nodeToken = "node-token"
 
 var testKey = types.PoolKey{Template: "rt:24.04", Net: types.NetNone, Size: types.SizeSmall}
 
-func TestClaimColdThenWarmThenCloseThroughTheWholeStack(t *testing.T) {
+func TestClaimThenWarmClaimThenCloseThroughTheWholeStack(t *testing.T) {
 	st := startStack(t, config.PoolSpec{PoolKey: testKey, Warm: 1})
 
 	var sb *sandbox.Sandbox
-	t.Run("cold claim before any refill", func(t *testing.T) {
+	t.Run("first claim", func(t *testing.T) {
 		var err error
 		sb, err = st.client.New(t.Context(), "rt:24.04")
 		if err != nil {
@@ -58,19 +58,23 @@ func TestClaimColdThenWarmThenCloseThroughTheWholeStack(t *testing.T) {
 
 	t.Run("refill then warm claim", func(t *testing.T) {
 		waitFor(t, st.oneWarm)
+		warmBefore := st.mgr.Counters().ClaimsWarm
 		warm, err := st.client.New(t.Context(), "rt:24.04")
 		if err != nil {
 			t.Fatalf("New: %v", err)
 		}
 		defer warm.Close()
+		if got := st.mgr.Counters().ClaimsWarm; got != warmBefore+1 {
+			t.Errorf("warm claims %d, want %d", got, warmBefore+1)
+		}
 		if out, err := warm.Exec(t.Context(), "echo", "warm"); err != nil || out != "warm\n" {
 			t.Errorf("exec on warm claim: %q, %v", out, err)
 		}
 	})
 
 	t.Run("close releases and revokes access", func(t *testing.T) {
 		if sb == nil {
-			t.Skip("cold claim subtest failed")
+			t.Skip("first claim subtest failed")
 		}
 		if err := sb.Close(); err != nil {
 			t.Fatalf("Close: %v", err)

mcp/server_test.go

@@ -197,12 +197,14 @@ func TestCloseBoxesWaitsForEveryReleaseOnce(t *testing.T) {
 	}
 	var wg sync.WaitGroup
 	for range 2 {
-		wg.Go(srv.closeBoxes)
+		wg.Go(func() {
+			srv.closeBoxes()
+			if got := releases.Load(); got != 3 {
+				t.Errorf("releases = %d after closeBoxes returned, want 3", got)
+			}
+		})
 	}
 	wg.Wait()
-	if got := releases.Load(); got != 3 {
-		t.Errorf("releases = %d after closeBoxes returned, want 3", got)
-	}
 }
 
 func TestExecRejectsAnEmptyCommand(t *testing.T) {

packaging/sandboxd.service

@@ -8,6 +8,7 @@ ExecStart=/usr/local/bin/sandboxd -config /etc/sandboxd/config.json
 Restart=on-failure
 RestartSec=5s
 KillMode=process
+TimeoutStopSec=27min
 Environment=SANDBOXD_LOG_LEVEL=info
 
 [Install]

sandboxd/egress/proxy.go

@@ -44,7 +44,8 @@ const (
 
 var (
 	// ErrRateLimited refuses a new upstream connection over the claim's connection rate.
-	ErrRateLimited = errors.New("egress: connection rate limit")
+	ErrRateLimited  = errors.New("egress: connection rate limit")
+	ErrInternalAddr = errors.New("egress: blocked internal address")
 
 	// hopHeaders are hop-by-hop and proxy-scoped headers this hop owns, never passed on.
 	hopHeaders = []string{
@@ -96,8 +97,11 @@ type Event struct {
 }
 
 func (ev *Event) attach(conn net.Conn, err error) {
-	if errors.Is(err, ErrRateLimited) {
+	switch {
+	case errors.Is(err, ErrRateLimited):
 		ev.Decision = DecisionRateLimited
+	case errors.Is(err, ErrInternalAddr):
+		ev.Decision = DecisionDeny
 	}
 	if ue, ok := errors.AsType[*upstreamError](err); ok {
 		ev.Upstream = ue.host
@@ -387,12 +391,15 @@ func (p *Proxy) relay(w http.ResponseWriter, r *http.Request, ev Event, rule Rul
 		out.Body = body
 	}
 	var received int64
+	var failed bool
 	defer func() {
 		var sent int64
 		if body != nil {
 			sent = body.n.Load()
 		}
-		p.reportTransfer(ev, sent, received)
+		if !failed || sent > 0 {
+			p.reportTransfer(ev, sent, received)
+		}
 	}()
 
 	resp, err := p.transport(route, mitm).RoundTrip(out)
@@ -403,6 +410,7 @@ func (p *Proxy) relay(w http.ResponseWriter, r *http.Request, ev Event, rule Rul
 	ev.attach(conn, err)
 	p.record(ev)
 	if err != nil {
+		failed = true
 		upstreamFailed(w, err)
 		return
 	}