review: guest-boundary fixes, SDK fixes, cuts, control-plane keep-alive

docs/deploy.md

@@ -125,7 +125,7 @@ gets 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) |
-| `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. Re-publish retains prior export generations until Delete so an in-flight fetch that selected old metadata can finish; budget storage for those generations. An explicit S3 Delete can still make a concurrent fetch that has not finished materializing fail visibly. A crash between upload and the meta.json commit marker leaves orphan objects invisible to listings — add an S3 lifecycle rule to reclaim them. 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. Re-publish retains prior export generations until Delete so an in-flight fetch that selected old metadata can finish; budget storage for those generations. When caching a new generation, each node prunes other cached generations of that record unused for at least an hour. An explicit S3 Delete can still make a concurrent fetch that has not finished materializing fail visibly. A crash between upload and the meta.json commit marker leaves orphan objects invisible to listings — add an S3 lifecycle rule to reclaim them. 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 — it is the expiry eligibility point for a healed replica a delete broadcast missed, after which its next successful hourly sweep removes it; persistent sweep failure extends retention until one succeeds, so it is not a hard ceiling |
 | `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 replica a delete broadcast missed becomes eligible for expiry after it, and its next successful hourly sweep removes it — so it is the finite eligibility point, not an exact ceiling). 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 |

docs/egress.md

@@ -167,13 +167,17 @@ as if it were that pool's claim: the template records its source pool, a
 re-promote records the new one, and a fork, checkpoint or branch of such a
 clone keeps it. So a clone of a template promoted from a pool with no policy
 has no egress, as that pool's claims have none, even for a tenant with its own
-policy; a template promoted from a sandbox of no pool records no source. Any other key with no configured pool — an image cold-booted on
-demand — has no pool layer, so a tenant's claim of one takes the tenant's
-policy alone (a root claim of one stays denied). A claim with `"egress": false`
+policy; a template promoted from a sandbox of no pool records no source. A pool's
+`egress` block belongs to the config file and stays bound to its key: when a
+`PUT /v1/pools` drops a config pool that has one, the claims made after it,
+now cold-booted on demand, still take that policy. Any other key that is in
+no pool at claim time — an image cold-booted on demand — has no pool layer, so
+a tenant's claim of one takes the tenant's policy alone (a root claim of one
+stays denied). A claim with `"egress": false`
 takes no policy at all, whatever its pool's: no door is bound, so the guest's
 dial is refused. Which layers a claim has is settled when it
 is made and kept for its life: a `PUT /v1/pools` that adds or drops a pool
-changes the claims made after it, never a live one (a record from before this
+without a config-file policy changes the claims made after it, never a live one (a record from before this
 rule resolves against the live pool set). Secrets are registered separately and
 referenced by name — the value comes from the environment, never the config
 file: a claim's own `guest: false` env first, the node's second

docs/mcp.md

@@ -42,12 +42,14 @@ in the guest, which can hold its reply up to 5 s past the cap.
 | `hibernate` | snapshot + stop, freeing memory while keeping id, files and processes; the next call that reaches the guest wakes it |
 | `promote` | publish the sandbox as a named template on its node; re-promoting replaces it |
 | `release` | destroy the sandbox and its files; the session forgets the id, so a second release is rejected as unknown |
-| `node_info` | warm pools, promoted templates, live claims, drain state, capacity and mesh peers |
+| `node_info` | warm pools, promoted templates, live claims, drain state, capacity and mesh peers; needs the operator token |
 
 Sandbox handles (and their tokens) are held by the server process for the
 session, and released with it: when the MCP client disconnects or the server
 is signalled, it destroys every sandbox that session claimed, whatever
-`ttl_seconds` asked for. `checkpoint` first if the state must survive.
+`ttl_seconds` asked for; a claim still in flight at the signal is left to its
+lease (one hour unless `ttl_seconds` says otherwise). `checkpoint` first if the
+state must survive.
 Checkpoints outlive sessions: `branch_checkpoint` accepts any known
 id without a listing round-trip. If the connected node does not hold it, the
 claim follows a live owner probe and redirect, or heals the checkpoint locally

docs/sandboxd-api.md

@@ -466,7 +466,7 @@ The URL's life is clamped to the claim's remaining lease; an archived claim
 kept forever (`archive_delete_after_seconds: 0`) has no lease, so there the
 requested `ttl_seconds` stands unclamped and `ttl_seconds: 0` mints a one-hour
 URL; releasing the sandbox ends it early either way.
-501 when the node
+400 a port of 0, 501 when the node
 has no `preview_listen`. The signed token embeds the sandbox id, port, and
 owner `advertise_addr`, so any node's preview listener can serve it (forwarding
 to the owner's main listener) and a released sandbox's URL simply stops
@@ -642,7 +642,7 @@ The env is persisted with the claim. A `guest: false` change takes effect on
 the egress proxy's next request at any time, a hibernated or archived claim
 included, and never touches the guest. A change to the guest entries is
 written to the guest at once, so it needs a running guest: on a hibernated,
-archived or transitioning sandbox it answers 409 (this verb never wakes one).
+archived, failed or transitioning sandbox it answers 409 (this verb never wakes one).
 A `PUT` identical to the stored env is written to a running guest again,
 so resending it repairs a guest file that was lost or not updated. A `PATCH`
 writes the guest file only when it changes a guest entry or sets one, so
@@ -653,7 +653,7 @@ are already live, but the guest write failed; resend the same request to
 deliver it. The guest side is described in [silkd](silkd.md#claim-env).
 `PUT`/`PATCH`/`DELETE` answer 204, `GET` 200; 400 a bad entry or an unknown body
 field; 404 unknown id or another tenant's claim; 409 a guest change on a
-paused sandbox.
+paused or failed sandbox.
 
 ## PUT /v1/sandboxes/{id}/instance-metadata
 

e2e/cmd/androidsmoke/main.go

@@ -152,8 +152,5 @@ func dialAdb(ctx context.Context, sb *sandbox.Sandbox) (string, error) {
 
 func head(s string) string {
 	lines := strings.Split(strings.TrimSpace(s), "\n")
-	if len(lines) > 12 {
-		lines = lines[:12]
-	}
-	return strings.Join(lines, "\n")
+	return strings.Join(lines[:min(len(lines), 12)], "\n")
 }

e2e/cmd/interceptsmoke/main.go

@@ -93,8 +93,5 @@ func grepLine(out, needle string) string {
 
 func tail(s string) string {
 	lines := strings.Split(strings.TrimRight(s, "\n"), "\n")
-	if len(lines) > 20 {
-		lines = lines[len(lines)-20:]
-	}
-	return strings.Join(lines, "\n")
+	return strings.Join(lines[max(0, len(lines)-20):], "\n")
 }

e2e/cmd/lifecycle/main.go

@@ -53,8 +53,7 @@ func run(addr, token, template, netShape string, wait time.Duration) error {
 	}
 	fmt.Printf("idle-hibernated then archived in %.1fs (local VM dropped)\n", elapsed.Seconds())
 
-	// the first access on an archived id is the cold wake: fetch the
-	// checkpoint, provision a fresh local VM, keep the id/token.
+	// The first access on an archived id is the cold wake: fetch the checkpoint into a fresh VM under the same id/token.
 	wakeStart := time.Now()
 	got, err := sb.ReadFile(ctx, markerPath)
 	if err != nil {
@@ -82,8 +81,7 @@ func run(addr, token, template, netShape string, wait time.Duration) error {
 	return nil
 }
 
-// waitArchived polls the node until archived reaches n, returning how long the
-// reaper's idle→hibernate→archive ladder took.
+// waitArchived returns how long the reaper's idle→hibernate→archive ladder took to archive n claims.
 func waitArchived(ctx context.Context, client *sandbox.Client, n int, timeout time.Duration) (time.Duration, error) {
 	start := time.Now()
 	deadline := start.Add(timeout)

mcp/e2e.py

@@ -94,7 +94,7 @@ def main() -> int:
 
         mcp.tool("hibernate", sandbox_id=sandbox_id)
         out = mcp.tool("exec", sandbox_id=sandbox_id, command="cat /root/m.txt")
-        assert out["stdout"] == "v2", out  # transparent wake
+        assert out["stdout"] == "v2", out
         print("  hibernate + transparent wake ok")
 
         forks = mcp.tool("fork", sandbox_id=sandbox_id, count=2)["sandbox_ids"]

mcp/server.go

@@ -47,7 +47,6 @@ type server struct {
 	template string
 
 	mu       sync.Mutex
-	closed   bool
 	boxes    map[string]*sandbox.Sandbox
 	ckpts    map[string]*sandbox.Checkpoint
 	release  sync.Once
@@ -72,9 +71,7 @@ func newServer(addr, token, template string) (*server, error) {
 	}, nil
 }
 
-// serve answers newline-delimited JSON-RPC 2.0 over r/w until EOF — the MCP
-// stdio transport. Requests are handled strictly in order: sandbox tools are
-// stateful, and an agent's tool calls arrive sequentially anyway.
+// serve answers the MCP stdio transport's JSON-RPC 2.0 lines until EOF, in order, since sandbox tools are stateful.
 func (s *server) serve(ctx context.Context, r *bufio.Reader, w io.Writer) error {
 	defer s.closeBoxes()
 	for {
@@ -165,7 +162,6 @@ func (s *server) box(id string) (*sandbox.Sandbox, error) {
 func (s *server) closeBoxes() {
 	s.release.Do(func() {
 		s.mu.Lock()
-		s.closed = true
 		boxes := slices.Collect(maps.Values(s.boxes))
 		clear(s.boxes)
 		s.mu.Unlock()
@@ -181,14 +177,8 @@ func (s *server) closeBoxes() {
 
 func (s *server) trackBox(sb *sandbox.Sandbox) {
 	s.mu.Lock()
-	closed := s.closed
-	if !closed {
-		s.boxes[sb.ID] = sb
-	}
-	s.mu.Unlock()
-	if closed {
-		_ = sb.Close() // claimed after the session's release ran; nothing else would ever release it
-	}
+	defer s.mu.Unlock()
+	s.boxes[sb.ID] = sb
 }
 
 func (s *server) dropBox(id string) {

mcp/server_test.go

@@ -174,35 +174,6 @@ func TestToolRepliesReplaceInvalidUTF8(t *testing.T) {
 	}
 }
 
-func TestTrackBoxAfterCloseReleasesTheClaim(t *testing.T) {
-	released := make(chan string, 1)
-	node := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		if strings.HasSuffix(r.URL.Path, "/release") {
-			released <- r.URL.Path
-		}
-		w.WriteHeader(http.StatusNoContent)
-	}))
-	t.Cleanup(node.Close)
-	addr := strings.TrimPrefix(node.URL, "http://")
-	srv, err := newServer(addr, "", "rt:24.04")
-	if err != nil {
-		t.Fatalf("server: %v", err)
-	}
-	srv.closeBoxes()
-	srv.trackBox(srv.client.Attach(addr, "sb_late", "tok"))
-	select {
-	case path := <-released:
-		if !strings.Contains(path, "sb_late") {
-			t.Errorf("released %s, want sb_late", path)
-		}
-	case <-time.After(5 * time.Second):
-		t.Fatal("a claim tracked after closeBoxes was never released")
-	}
-	if len(srv.boxes) != 0 {
-		t.Errorf("boxes = %d after a late track, want 0", len(srv.boxes))
-	}
-}
-
 func TestCloseBoxesWaitsForEveryReleaseOnce(t *testing.T) {
 	var releases atomic.Int32
 	node := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {

mcp/tools.go

@@ -10,7 +10,6 @@ import (
 	"strings"
 	"time"
 
-	"github.com/cocoonstack/sandbox/protocol/wire"
 	sandbox "github.com/cocoonstack/sandbox/sdk/go"
 )
 
@@ -77,7 +76,7 @@ var (
 			schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint"), "template_name": str("template name to publish as")}, "sandbox_id", "template_name"), toolPromote,
 		},
 		{"release", "Destroy a sandbox and free its resources; files and processes inside it are lost. This session forgets the id, so a second release of it is rejected as unknown.", schema(props{"sandbox_id": str("id returned by create_sandbox, fork, or branch_checkpoint")}, "sandbox_id"), toolRelease},
-		{"node_info", "Report the connected node's warm pools, promoted templates, live claims, drain state, capacity, and mesh peers as JSON.", schema(props{}), toolNodeInfo},
+		{"node_info", "Report the connected node's warm pools, promoted templates, live claims, drain state, capacity, and mesh peers as JSON. Needs the operator token; a tenant token is refused.", schema(props{}), toolNodeInfo},
 	}
 )
 
@@ -290,14 +289,6 @@ func toolReadFile(ctx context.Context, s *server, raw jsontext.Value) (string, e
 	if err != nil {
 		return "", err
 	}
-	// a device or a FIFO is not a file: /dev/null reads as empty and a writerless FIFO blocks in open
-	info, err := sb.Stat(ctx, args.Path)
-	if err != nil {
-		return "", err
-	}
-	if info.Kind != wire.FileKindFile {
-		return "", fmt.Errorf("%s is a %s; read_file reads a regular file", args.Path, info.Kind)
-	}
 	out := cappedOutput{stopAtCap: true}
 	if err := sb.ReadFileTo(ctx, args.Path, &out); errors.Is(err, errOutputCap) {
 		return "", fmt.Errorf("%s exceeds the %d-byte read_file cap; use exec with head, tail, or a checksum", args.Path, execOutputCap)

protocol/wire/frame.go

@@ -30,9 +30,6 @@ const (
 	// PortWriteChunk keeps a port data frame (payload x4/3 base64 plus envelope) well under MaxFrame.
 	PortWriteChunk = 1 << 20
 
-	// maxSkipDepth bounds a tag scan past nested siblings; wire values nest two deep.
-	maxSkipDepth = 16
-
 	// GitBranch.Action values (silkd's GitBranchOp).
 	BranchList     = "list"
 	BranchCreate   = "create"
@@ -682,16 +679,13 @@ func DecodeResponse(line []byte) (Response, error) {
 }
 
 // NewFrameScanner wraps r for newline-delimited frames capped at MaxFrame.
-// No pre-sized buffer: bulk frames outgrow any fixed start anyway.
 func NewFrameScanner(r io.Reader) *bufio.Scanner {
 	sc := bufio.NewScanner(r)
 	sc.Buffer(nil, MaxFrame)
 	return sc
 }
 
-// AppendBulkRequest renders a data-carrying request frame —
-// {"v":1,"op":<op>,"data":"<base64>"} plus newline — into buf, reused across
-// calls on the bulk send paths (base64's alphabet needs no JSON escaping).
+// AppendBulkRequest renders {"v":1,"op":<op>,"data":"<base64>"} and a newline into buf's storage; base64 needs no JSON escaping.
 func AppendBulkRequest(buf []byte, op string, data []byte) []byte {
 	buf = append(buf[:0], requestHead...)
 	buf = append(buf, op...)
@@ -713,8 +707,7 @@ func IsContinuation(line []byte) bool {
 	return false
 }
 
-// fastBulk slices the base64 data out of a canonical bulk frame, skipping the
-// json.Unmarshal that dominates downloads; any other shape falls back to slow.
+// fastBulk decodes a canonical bulk frame without json.Unmarshal; any other shape falls back to slow.
 func fastBulk(tag string, slow respDecoder, mk func([]byte) Response) respDecoder {
 	head := []byte(`{"type":"` + tag + `","data":"`)
 	return func(line []byte) (Response, error) {
@@ -785,35 +778,13 @@ func scanTag(line []byte, key string) (string, error) {
 			}
 			return tok.String(), nil
 		}
-		if err = skipValue(dec); err != nil {
+		if err = dec.SkipValue(); err != nil {
 			return "", err
 		}
 	}
 	return "", nil
 }
 
-// skipValue consumes one JSON value; a guest cannot grow the host stack with nesting.
-func skipValue(dec *jsontext.Decoder) error {
-	depth := 0
-	for {
-		tok, err := dec.ReadToken()
-		if err != nil {
-			return err
-		}
-		switch tok.Kind() {
-		case '{', '[':
-			if depth++; depth > maxSkipDepth {
-				return fmt.Errorf("value nested deeper than %d", maxSkipDepth)
-			}
-		case '}', ']':
-			depth--
-		}
-		if depth == 0 {
-			return nil
-		}
-	}
-}
-
 func decodeAs[T any](line []byte) (*T, error) {
 	v := new(T)
 	if err := json.Unmarshal(line, v); err != nil {

protocol/wire/frame_test.go

@@ -265,12 +265,12 @@ func TestTagAfterOtherKeys(t *testing.T) {
 }
 
 func TestTagScanBoundsNesting(t *testing.T) {
-	shallow := `{"x":` + strings.Repeat("[", maxSkipDepth) + strings.Repeat("]", maxSkipDepth) + `,"type":"done"}`
+	shallow := `{"x":` + strings.Repeat("[", 16) + strings.Repeat("]", 16) + `,"type":"done"}`
 	if _, err := DecodeResponse([]byte(shallow)); err != nil {
 		t.Fatalf("nesting at the cap rejected: %v", err)
 	}
 	deep := `{"x":` + strings.Repeat("[", 1<<20) + strings.Repeat("]", 1<<20) + `,"type":"done"}`
-	if _, err := DecodeResponse([]byte(deep)); err == nil || !strings.Contains(err.Error(), "nested deeper") {
+	if _, err := DecodeResponse([]byte(deep)); err == nil || !strings.Contains(err.Error(), "max depth") {
 		t.Fatalf("deep nesting: err %v, want the depth cap", err)
 	}
 }