review: solo whole-repo round on d5d7887 (config seed fix, cuts, owner calls)

docs/sandboxd-api.md

@@ -444,7 +444,7 @@ destroyed and the pool entry retires. `net`/`size` default like a claim's.
 Answers the fresh `GET /v1/info` payload. 400 bad key, negative warm/idle,
 `warm_max` below `warm`, `idle_hibernate_seconds` on an egress pool, a
 negative archive duration or an `archive_after_seconds` not above the pool's
-`idle_hibernate_seconds`, duplicate pool, or a config-owned `egress`/`warmup`/`capture_trim`/`storage`
+`idle_hibernate_seconds`, duplicate pool, or a config-owned `egress`/`warmup`/`capture_trim`/`storage`/`egress_upstream_env`
 field; 401 bad api
 token; 409 egress pool on a node without an egress attachment.
 

mcp/server.go

@@ -2,6 +2,7 @@ package main
 
 import (
 	"bufio"
+	"cmp"
 	"context"
 	"encoding/json/jsontext"
 	"encoding/json/v2"
@@ -120,11 +121,8 @@ func (s *server) dispatch(ctx context.Context, req *rpcRequest) rpcResponse {
 		}
 		text, err := s.callTool(ctx, call.Name, call.Arguments)
 		if err != nil {
-			if text == "" {
-				text = err.Error()
-			}
 			return result(req.ID, map[string]any{
-				"content": []map[string]any{{"type": "text", "text": text}},
+				"content": []map[string]any{{"type": "text", "text": cmp.Or(text, err.Error())}},
 				"isError": true,
 			})
 		}

sandboxd/egress/policy.go

@@ -80,15 +80,13 @@ func (p Policy) Validate() error {
 		case "*.":
 			return fmt.Errorf("allow[%d]: host %q needs a domain after the wildcard", i, r.Host)
 		}
-		seen := make(map[uint16]struct{}, len(r.Ports))
-		for _, port := range r.Ports {
+		for j, port := range r.Ports {
 			if port == 0 {
 				return fmt.Errorf("allow[%d]: port 0 is not a destination", i)
 			}
-			if _, dup := seen[port]; dup {
+			if slices.Contains(r.Ports[:j], port) {
 				return fmt.Errorf("allow[%d]: port %d repeated", i, port)
 			}
-			seen[port] = struct{}{}
 		}
 	}
 	if p.Socks5 && !p.admitsTunnel() {

sandboxd/pool/archive.go

@@ -315,12 +315,11 @@ func (m *Manager) retryArchiveDelete(ctx context.Context, ckID string) {
 	}
 	err := m.ckpts.Delete(ctx, ckID)
 	l.Unlock()
+	m.recDone(ckID)
 	if err != nil {
-		m.recDone(ckID)
 		log.WithFunc("pool.retryArchiveDelete").Warnf(ctx, "delete %s: %v", ckID, err)
 		return
 	}
-	m.recDone(ckID)
 	if err := m.clearArchiveCk(ckID); err != nil {
 		log.WithFunc("pool.retryArchiveDelete").Warnf(ctx, "clear %s: %v", ckID, err)
 	}

sandboxd/pool/checkpoint.go

@@ -361,12 +361,8 @@ func (m *Manager) deleteCkLocked(ctx context.Context, ckID string) error {
 	l.Lock()
 	err := m.ckpts.Delete(ctx, ckID)
 	l.Unlock()
-	if err != nil {
-		m.recDone(ckID)
-		return err
-	}
 	m.recDone(ckID)
-	return nil
+	return err
 }
 
 func (m *Manager) loadCheckpoint(ctx context.Context, ckptID string) (types.Checkpoint, error) {

sandboxd/pool/pool.go

@@ -384,14 +384,7 @@ type Manager struct {
 
 // NewManager builds a manager from the node config; ctx bounds backend construction.
 func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *egress.SecretStore) (*Manager, error) {
-	maxFork := cfg.MaxForkCount
-	if maxFork < 1 {
-		maxFork = defaultMaxFork
-	}
-	refill := cfg.RefillConcurrency
-	if refill < 1 {
-		refill = defaultRefill
-	}
+	refill := cmp.Or(cfg.RefillConcurrency, defaultRefill)
 	m := &Manager{
 		eng:             eng,
 		dataDir:         cfg.DataDir,
@@ -400,7 +393,7 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg
 		releaseDelay:    time.Duration(cfg.ReleaseDelaySeconds) * time.Second,
 		vmmRestart:      cfg.VMMRestart,
 		cocoondSocket:   cfg.CocoondSocket,
-		maxFork:         maxFork,
+		maxFork:         cmp.Or(cfg.MaxForkCount, defaultMaxFork),
 		store:           newClaimStore(cfg.DataDir, cfg.SyncClaims),
 		volumes:         make(map[string]catalogVolume, len(cfg.Volumes)),
 		volumeAdmission: map[string]volumeHolders{},

sandboxd/pool/poolstore.go

@@ -101,14 +101,15 @@ func (m *Manager) adoptPersistedPools(ctx context.Context) error {
 	return nil
 }
 
-// poolSeedHash digests a pool set's warm-target shape, order-independent, without the config-owned egress, warmup, capture_trim and storage.
+// poolSeedHash digests a pool set's warm-target shape, order-independent, without the config-owned egress, warmup, capture_trim, storage and egress_upstream_env.
 func poolSeedHash(specs []config.PoolSpec) string {
 	shaped := slices.Clone(specs)
 	for i := range shaped {
 		shaped[i].Egress = nil
 		shaped[i].Warmup = nil
 		shaped[i].CaptureTrim = false
 		shaped[i].Storage = ""
+		shaped[i].EgressUpstreamEnv = ""
 	}
 	slices.SortFunc(shaped, func(a, b config.PoolSpec) int { return strings.Compare(a.Hash(), b.Hash()) })
 	return utils.DigestHex(shaped)

sandboxd/pool/poolstore_test.go

@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/cocoonstack/sandbox/sandboxd/config"
+	"github.com/cocoonstack/sandbox/sandboxd/egress"
 	"github.com/cocoonstack/sandbox/sandboxd/types"
 )
 
@@ -68,6 +69,24 @@ func TestPoolSeedHashBytesArePinned(t *testing.T) {
 	}
 }
 
+func TestPoolSeedHashIgnoresConfigOwnedFields(t *testing.T) {
+	base := config.PoolSpec{PoolKey: seedKey, Warm: 2}
+	want := poolSeedHash([]config.PoolSpec{base})
+	for name, edit := range map[string]func(*config.PoolSpec){
+		"egress":              func(s *config.PoolSpec) { s.Egress = &egress.Policy{Allow: []egress.Rule{{Host: "api.example.com"}}} },
+		"warmup":              func(s *config.PoolSpec) { s.Warmup = []string{"true"} },
+		"capture_trim":        func(s *config.PoolSpec) { s.CaptureTrim = true },
+		"storage":             func(s *config.PoolSpec) { s.Storage = "20G" },
+		"egress_upstream_env": func(s *config.PoolSpec) { s.EgressUpstreamEnv = "POOL_UPSTREAM" },
+	} {
+		spec := base
+		edit(&spec)
+		if got := poolSeedHash([]config.PoolSpec{spec}); got != want {
+			t.Errorf("a %s edit changed the seed: the API cannot carry it, so a restart would warn of an override", name)
+		}
+	}
+}
+
 func TestConcurrentSetPoolsPersistLatest(t *testing.T) {
 	dir := t.TempDir()
 	m := newTestManagerAt(t, newFakeEngine(), dir, config.PoolSpec{PoolKey: seedKey, Warm: 1})

sandboxd/pool/template.go

@@ -354,18 +354,15 @@ func (m *Manager) resolveGolden(ctx context.Context, key types.PoolKey, tenant s
 	id := store.TemplateID(key.Hash())
 	l := m.recLock(id)
 	l.RLock()
+	cleanup := func() { l.RUnlock(); m.recDone(id) }
 	dir, meta, digest, err := m.tpls.Fetch(ctx, id)
-	if errors.Is(err, store.ErrNotFound) {
-		l.RUnlock()
-		m.recDone(id)
-		return goldenResolution{}, nil
-	}
 	if err != nil {
-		l.RUnlock()
-		m.recDone(id)
+		cleanup()
+		if errors.Is(err, store.ErrNotFound) {
+			return goldenResolution{}, nil
+		}
 		return goldenResolution{}, err
 	}
-	cleanup := func() { l.RUnlock(); m.recDone(id) }
 	var rec templateRecord
 	if err := json.Unmarshal(meta, &rec); err != nil {
 		cleanup()

sandboxd/server/server.go

@@ -2,6 +2,7 @@
 package server
 
 import (
+	"cmp"
 	"context"
 	"crypto/subtle"
 	"encoding/json/jsontext"
@@ -468,10 +469,7 @@ func (s *Server) handleSandboxes(w http.ResponseWriter, r *http.Request) {
 		writeErr(w, http.StatusBadRequest, err.Error())
 		return
 	}
-	scope := tenantFrom(r.Context())
-	if scope == "" {
-		scope = query.Get("tenant")
-	}
+	scope := cmp.Or(tenantFrom(r.Context()), query.Get("tenant"))
 	writeJSON(w, http.StatusOK, SandboxListResponse{Sandboxes: s.mgr.Sandboxes(scope, query.Get("claim_ref"), filter)})
 }
 

sandboxd/store/peer/transport.go

@@ -145,8 +145,6 @@ func Untar(r io.Reader, dst string) error {
 			if err := writeFile(target, tr, os.FileMode(hdr.Mode).Perm()); err != nil { //nolint:gosec // Perm masks to 0777
 				return err
 			}
-		default:
-			continue
 		}
 	}
 }

sandboxd/types/types.go

@@ -779,10 +779,7 @@ func ValidateVolumes(volumes []Volume, attachOnly bool) ([]Volume, error) {
 			applied[i] = Volume{Name: volume.Name, Mode: mode, AttachOnly: true}
 			continue
 		}
-		mount := volume.Mount
-		if mount == "" {
-			mount = DefaultVolumeMount(volume.Name)
-		}
+		mount := cmp.Or(volume.Mount, DefaultVolumeMount(volume.Name))
 		if err := validateVolumeMount(mount); err != nil {
 			return nil, fmt.Errorf("volumes[%d] mount %q: %w", i, mount, err)
 		}

sdk/go/pools.go

@@ -6,7 +6,7 @@ import (
 	"net/http"
 )
 
-// PoolSpec is a desired warm pool for SetPools; egress, warmup, capture_trim and storage are config-owned, so it has no field for them.
+// PoolSpec is a desired warm pool for SetPools; egress, warmup, capture_trim, storage and egress_upstream_env are config-owned, so it has no field for them.
 type PoolSpec struct {
 	Template                  string   `json:"template"`
 	Net                       NetShape `json:"net,omitempty"`

sdk/python/cocoonsandbox/frames.py

@@ -4,6 +4,7 @@
 
 import base64
 import binascii
+import contextlib
 import json
 import sys
 from typing import Any
@@ -41,10 +42,8 @@ def decode_response(line: bytes) -> dict[str, Any]:
         if te > 0 and line[9:te] in (b"stdout", b"stderr", b"data") and line.startswith(b'","data":"', te):
             de = line.find(b'"', te + 10)
             if de > 0 and line[de:] in (b'"}', b'"}\n'):
-                try:
+                with contextlib.suppress(binascii.Error):
                     return {"type": line[9:te].decode(), "data": base64.b64decode(line[te + 10 : de], validate=True)}
-                except binascii.Error:
-                    pass
     frame = json.loads(line)
     if not isinstance(frame, dict) or "type" not in frame:
         raise ValueError(f"frame without a type tag: {line[:80]!r}")

sdk/python/cocoonsandbox/sandbox.py

@@ -668,8 +668,7 @@ def cut() -> None:
 
 def _feed_stdin(conn: Conn, stdin: bytes) -> None:
     with contextlib.suppress(SandboxError, OSError):  # the reader reports the real failure
-        if stdin:
-            _send_chunks(conn, stdin, op="stdin")
+        _send_chunks(conn, stdin, op="stdin")
         conn.send("stdin_close")