pool: runtime volume catalog follow-ups to #382

docs/deploy.md

@@ -120,7 +120,7 @@ gets the intersection of the two.
 | `client_advertise` | unset | client-facing HTTP(S) origin for this node, e.g. `https://node-a.sandbox.example.com`; published in owner, redirect, and peer responses. No path, query, fragment, or userinfo |
 | `bridges` / `networks` | unset | egress-lane attachment: a list of host bridge devices, or a list of CNI conflist names. Mutually exclusive; with neither set the node serves only the no-network lane. A Linux bridge holds at most 1024 ports (kernel `BR_MAX_PORTS`), so an N-entry list raises the node's egress ceiling to N×1024 — VMs spread over the list by a stable hash of the VM name, so size it with headroom (the spread is statistical, not exact). `bridges` keeps the raw TAP-on-bridge attachment (taps in the root netns, no per-VM network namespace or CNI plugin execution); `networks` runs the CNI chain per VM. [Guarded egress](egress.md) on the egress lane (an egress-lane pool policy or any tenant policy) needs `bridges` and rejects a CNI network at load; none-lane pool policies ride the proxy on either |
 | `volumes` | unset | node-local catalog of operator-managed dataset images: `[ {"name":"imagenet","path":"/srv/datasets/imagenet.img","directio":"off","tenants":["acme"]}, {"name":"scratch-db","path":"/srv/datasets/scratch.img","writable":true} ]`. Names match `^[a-z][a-z0-9_-]{0,19}$` and cannot start with `cocoon-`; paths are absolute; `directio` is `on`, `off`, or `auto` and defaults to `off` for both read-only and writable entries. `tenants` is an optional [access list](sandboxd-api.md#volume-access-lists); a listed name the tenant set does not hold grants nothing and logs a warning at boot (the [tenant API](sandboxd-api.md#tenants-v1tenants) may add it later). `writable` (default `false`) lets a claim request `mode: "rw"` on that entry — see [Dataset volumes](#dataset-volumes). The catalog is intentionally not part of the cluster digest |
-| `volume_roots` | unset | absolute directories that bound [runtime registration](sandboxd-api.md#put-v1volumesname): the registered path, once cleaned, must equal a root or sit under one, or the PUT returns 400. The check is lexical: `..` is resolved but symlinks are not followed, so a symlink under a root can point outside it. Static `volumes` entries are not checked. A saved registration outside the roots prevents startup. Unregister it before narrowing the roots, or widen the roots to include it. DELETE always rewrites the catalog, including for an absent name. Keep `data_dir` outside every root. Unset or an empty list accepts any absolute path |
+| `volume_roots` | unset | absolute directories that bound [runtime registration](sandboxd-api.md#put-v1volumesname): the registered path, once cleaned, must equal a root or sit under one, or the PUT returns 400. The check is lexical: `..` is resolved but symlinks are not followed, so a symlink under a root can point outside it. Static `volumes` entries are not checked. A saved registration outside the roots prevents startup. Unregister it before narrowing the roots, or widen the roots to include it. Keep `data_dir` outside every root. Unset or an empty list accepts any absolute path |
 | `secrets` | unset | credentials the egress proxy injects by name: `[{"name": "gh", "header": "Authorization", "value_env": "GH_TOKEN"}]`. A pool or egress-class rule references the name (on a claim where both apply, only the pool rule's secret is injected); the value comes from the claim's `guest: false` env, then the node's environment, never this file. Without `value_env` the secret's name is the env to read, and only claims supply it. See [egress](egress.md#claim-env-and-secrets) |
 | `egress_internal_allow` | unset | CIDR prefixes re-admitted through the egress proxy's SSRF guard, node-wide (every pool and tenant); `prefix:port,port` scopes an entry to those ports, a bare prefix admits every port. Prefer the port form. Prefixes, not a permit-private switch — the guest bridges are themselves ULA/RFC1918. See [egress](egress.md) |
 | `egress_upstream` | unset | per-claim upstream proxies: `{"claim_env": "EGRESS_UPSTREAM", "allow": ["res.example.com", "10.1.0.0/16"]}`. A claim's `guest: false` entry under `claim_env` (`http://` or `socks5://`, optional `user:pass@`, or `direct`) routes its egress through that upstream, which must be on `allow` (host names, IPs, CIDR prefixes). See [upstream proxies](egress.md#upstream-proxies) |
@@ -325,8 +325,7 @@ set, promoted-template intersection, and redirect follow the fleet-wide rule
 in [cluster](cluster.md#volumes-and-placement).
 
 Catalog `tenants` lists follow the
-[access-list rule](sandboxd-api.md#volume-access-lists). A listed tenant the
-set no longer holds grants nothing and logs a warning at boot.
+[access-list rule](sandboxd-api.md#volume-access-lists).
 `GET /v1/volumes` reports the caller-visible fleet union and holder count, plus
 the answering node's current local availability and whether the entry is
 `writable`, without exposing host paths or node addresses. Applied names,

docs/live-volumes.md

@@ -9,12 +9,8 @@ file. Keep each name bound to the same immutable image while held. Use Cocoon
 v0.7.2 or newer and set `SANDBOX_ADDR` and `SANDBOX_API_TOKEN` for the client.
 A sandbox token alone cannot manage volumes.
 
-A configured node root token can
-[register backing paths](sandboxd-api.md#put-v1volumesname) without editing
-static config. Registration persists only node-local catalog metadata and does
-not manage disk files. Runtime `tenants` lists follow the
-[access-list rule](sandboxd-api.md#volume-access-lists). Live changes remain
-limited to mounted read-only entries.
+Entries [registered at runtime](sandboxd-api.md#put-v1volumesname) work the
+same way.
 
 Pause application reads before replacement. Close files, mmap mappings, cwd,
 and bind mount references on the old disk. Replacement unmounts the old disk

docs/sandboxd-api.md

@@ -278,7 +278,9 @@ duplicate fields, and trailing JSON return 400.
 - `tenants` is the entry's [access list](#volume-access-lists).
 
 Success is 204. An identical normalized registration is idempotent. A different
-binding, changed fields, or a static name or cleaned path collision returns 409.
+binding, changed fields, a static name or cleaned path collision, or a name a
+claim still holds returns 409. Each change is an `op:"volumes"` audit record
+naming the volume added or removed.
 Invalid fields return 400. GET keeps its existing projection without host paths
 or tenant lists. Runtime entries support the existing live volume API.
 
@@ -297,27 +299,25 @@ cleanup directories: `<data_dir>/readers`, `<data_dir>/goldens`, and
 ### Volume access lists
 
 Static and runtime entries share one `tenants` rule. An omitted or empty list
-permits every authenticated scope. A nonempty list permits only claims owned
-by the listed tenants, and root-owned claims can use any entry. Live additions
-check the claim owner's list even when root sends the request; held names
-finish repair or removal without a new grant. Runtime registration rejects
-tenant names that do not match the tenant API syntax. Static config preserves
-its supplied tenant lists. An invalid static name matches no tenant, so a
-nonempty list containing only invalid names permits no tenant. A valid name
-for a tenant that does not exist yet grants nothing until the tenant is created.
+permits every authenticated scope. A nonempty list permits only claims owned by
+the listed tenants, and root-owned claims can use any entry. Runtime
+registration rejects tenant names that do not match the tenant API syntax.
+Static config preserves its supplied tenant lists. An invalid static name
+matches no tenant, so a nonempty list containing only invalid names permits no
+tenant. A valid name for a tenant that does not exist yet grants nothing until
+the tenant is created.
 
 ## DELETE /v1/volumes/{name}
 
 Auth and name validation match PUT. Deletes only runtime registration metadata.
-Success, including an absent unreferenced name, is 204. Static names and names
-referenced by a claim, an in-flight operation, or cleanup return 409.
-DELETE always writes the absence, including when the name is already absent.
+Static names and names referenced by a claim, an in-flight operation, or
+cleanup return 409. Otherwise DELETE writes the absence and returns 204, also
+for an absent name.
 Persistence failure returns 503 with `Retry-After`. Retry the same DELETE,
 even if GET no longer lists the name; a restart can restore the old registration.
 
-Before downgrading to a daemon without runtime registration, drain runtime
-references or supply equivalent static volume config and finish pending live
-volume changes.
+Before downgrading to a daemon without runtime registration, release claims
+that use runtime names or declare those names in static `volumes`.
 
 ## GET /v1/sandboxes/{id}/volumes
 
@@ -341,8 +341,7 @@ when the volume is in both sets, `pending` when desired but not yet confirmed,
 or `detaching` when applied but no longer desired. An interrupted device call
 can leave the actual guest state different from the confirmed set. A failed
 PUT or PATCH adds a top-level `error` naming the refusal, and the affected
-state may carry its own `error`. `converging` is true while a request worker
-is running.
+state may carry its own `error`. `converging` is true only while a request worker is running.
 `pending` with `converging: false` requires an explicit retry; there is no
 background convergence. GET retains the last safe per-volume error for the
 current daemon lifetime. After daemon restart, unfinished states report an

docs/security.md

@@ -105,7 +105,7 @@ exposes host image paths or access lists.
 Runtime [registration and unregistration](sandboxd-api.md#put-v1volumesname)
 require a configured root `api_token`;
 these endpoints return 403 when the token is unset. New catalog records use
-mode 0600. Reads and replacements use the daemon's filesystem permissions.
+mode 0600.
 Registration uses static configuration's path validation plus, when set, the
 [`volume_roots`](deploy.md#configuration) bound. It does not verify backing type
 or pin backing identity. Without

sandboxd/pool/claim.go

@@ -381,7 +381,7 @@ func (m *Manager) admitVolumes(tenant string, volumes []resolvedVolume) ([]types
 	return applied, nil
 }
 
-// admitClaim refuses a busy volume or an over-quota tenant before any VM is built.
+// admitClaim refuses a busy or retired volume or an over-quota tenant before any VM is built.
 func (m *Manager) admitClaim(tenant string, volumes []resolvedVolume, applied []types.Volume) error {
 	m.mu.Lock()
 	defer m.mu.Unlock()

sandboxd/pool/failover.go

@@ -394,34 +394,37 @@ func (m *Manager) forgetAdopted(ctx context.Context, sb *types.Sandbox) func() {
 			logger.Warnf(ctx, "clear archive ck %s: %v", ck, err)
 		}
 	}
-	if !m.forgetClaim(ctx, sb) {
+	holds, ok := m.forgetClaim(ctx, sb)
+	if !ok {
 		return nil
 	}
 	return func() {
 		if vm != "" {
-			m.destroy(ctx, vm)
+			m.removeOrRetry(ctx, vm, "", volumeTeardown{holds: holds})
 		}
 		m.dropSnap(ctx, snap)
 		logger.Infof(ctx, "sandbox %s moved to another node", sb.ID)
 	}
 }
 
-func (m *Manager) forgetClaim(ctx context.Context, sb *types.Sandbox) bool {
+func (m *Manager) forgetClaim(ctx context.Context, sb *types.Sandbox) ([]types.Volume, bool) {
 	m.mu.Lock()
 	if m.claimed[sb.ID] != sb {
 		m.mu.Unlock()
-		return false
+		return nil, false
 	}
+	holds := m.cancelVolumeRun(sb)
 	if sb.ArchiveCk != "" {
-		m.releaseVolumes(heldVolumes(sb))
+		m.releaseVolumes(holds)
+		holds = nil
 	}
 	m.dropClaimLocked(sb)
 	js := m.store.del(sb.ID)
 	m.mu.Unlock()
 	if err := m.store.commit(js); err != nil {
 		m.store.recommit(ctx, js)
 	}
-	return true
+	return holds, true
 }
 
 func (m *Manager) sweepOrphanArchives(ctx context.Context) {

sandboxd/pool/volume_bench_test.go

@@ -1,8 +1,6 @@
 package pool
 
 import (
-	"os"
-	"path/filepath"
 	"testing"
 
 	"github.com/cocoonstack/sandbox/sandboxd/config"
@@ -45,11 +43,7 @@ func volumeBenchManager(b *testing.B) *Manager {
 	if err := m.store.save(m.claimed); err != nil {
 		b.Fatal(err)
 	}
-	path := filepath.Join(b.TempDir(), "data.raw")
-	if err := os.WriteFile(path, []byte("benchmark backing"), 0o600); err != nil {
-		b.Fatal(err)
-	}
-	if err := m.RegisterVolume(b.Context(), "bench-data", types.VolumeRegistration{Path: path}); err != nil {
+	if err := m.RegisterVolume(b.Context(), "bench-data", types.VolumeRegistration{Path: writeVolumeImage(b, "data.raw", "benchmark backing")}); err != nil {
 		b.Fatal(err)
 	}
 	return m

sandboxd/pool/volume_catalog.go

@@ -34,15 +34,36 @@ func (c *volumeCatalog) withinRoots(path string) bool {
 	})
 }
 
+func (c *volumeCatalog) normalize(name string, spec types.VolumeRegistration) (*catalogVolume, error) {
+	spec.DirectIO = cmp.Or(spec.DirectIO, types.DirectIOOff)
+	for _, tenant := range spec.Tenants {
+		if !types.NameRe.MatchString(tenant) {
+			return nil, fmt.Errorf("%w: volume %q tenant %q must match %s", ErrBadVolume, name, tenant, types.NameRe)
+		}
+	}
+	if err := config.ValidateVolumeCatalog([]config.VolumeSpec{{
+		Name: name, Path: spec.Path, DirectIO: spec.DirectIO, Writable: spec.Writable, Tenants: spec.Tenants,
+	}}); err != nil {
+		return nil, fmt.Errorf("%w: %w", ErrBadVolume, err)
+	}
+	volume := &catalogVolume{
+		runtime:  true,
+		disk:     engine.VolumeSpec{Name: name, Path: filepath.Clean(spec.Path), DirectIO: spec.DirectIO},
+		tenants:  slices.Compact(slices.Sorted(slices.Values(spec.Tenants))),
+		writable: spec.Writable,
+	}
+	if !c.withinRoots(volume.disk.Path) {
+		return nil, fmt.Errorf("%w: volume %q path is outside volume_roots", ErrBadVolume, name)
+	}
+	return volume, nil
+}
+
 // RegisterVolume durably binds a root-managed name to a backing path.
 func (m *Manager) RegisterVolume(ctx context.Context, name string, spec types.VolumeRegistration) error {
-	volume, err := normalizeRegistration(name, spec)
+	volume, err := m.catalog.normalize(name, spec)
 	if err != nil {
 		return err
 	}
-	if !m.catalog.withinRoots(volume.disk.Path) {
-		return fmt.Errorf("%w: volume %q path is outside volume_roots", ErrBadVolume, name)
-	}
 	m.catalog.writeMu.Lock()
 	defer m.catalog.writeMu.Unlock()
 	m.mu.Lock()
@@ -110,24 +131,19 @@ func (m *Manager) loadRuntimeVolumes() error {
 	if err = json.Unmarshal(raw, &record); err != nil {
 		return fmt.Errorf("parse runtime volume catalog: %w", err)
 	}
-	entries := maps.Clone(m.catalog.entries)
 	for name, spec := range record {
-		volume, normalizeErr := normalizeRegistration(name, spec)
+		volume, normalizeErr := m.catalog.normalize(name, spec)
 		if normalizeErr != nil {
 			return fmt.Errorf("runtime volume catalog: %w", normalizeErr)
 		}
-		if !m.catalog.withinRoots(volume.disk.Path) {
-			return fmt.Errorf("runtime volume catalog: volume %q path is outside volume_roots", name)
-		}
 		if m.catalog.entries[name] != nil {
 			return fmt.Errorf("runtime volume catalog duplicates static name %q", name)
 		}
-		if err := catalogPathConflict(entries, volume.disk.Path); err != nil {
+		if err := catalogPathConflict(m.catalog.entries, volume.disk.Path); err != nil {
 			return fmt.Errorf("runtime volume catalog: %w", err)
 		}
-		entries[name] = volume
+		m.catalog.entries[name] = volume
 	}
-	m.catalog.entries = entries
 	return nil
 }
 
@@ -144,7 +160,7 @@ func (m *Manager) persistRuntimeCatalog(entries map[string]*catalogVolume) error
 	}
 	raw, err := json.Marshal(record)
 	if err != nil {
-		return fmt.Errorf("%w: marshal runtime volume catalog: %w", ErrVolumeCatalogUnavailable, err)
+		return fmt.Errorf("marshal runtime volume catalog: %w", err)
 	}
 	if err := utils.WriteFileSync(filepath.Join(m.dataDir, runtimeVolumesFile), raw, 0o600); err != nil {
 		return fmt.Errorf("%w: %w", ErrVolumeCatalogUnavailable, err)
@@ -160,23 +176,3 @@ func catalogPathConflict(entries map[string]*catalogVolume, path string) error {
 	}
 	return nil
 }
-
-func normalizeRegistration(name string, spec types.VolumeRegistration) (*catalogVolume, error) {
-	spec.DirectIO = cmp.Or(spec.DirectIO, types.DirectIOOff)
-	for _, tenant := range spec.Tenants {
-		if !types.NameRe.MatchString(tenant) {
-			return nil, fmt.Errorf("%w: volume %q tenant %q must match %s", ErrBadVolume, name, tenant, types.NameRe)
-		}
-	}
-	if err := config.ValidateVolumeCatalog([]config.VolumeSpec{{
-		Name: name, Path: spec.Path, DirectIO: spec.DirectIO, Writable: spec.Writable, Tenants: spec.Tenants,
-	}}); err != nil {
-		return nil, fmt.Errorf("%w: %w", ErrBadVolume, err)
-	}
-	return &catalogVolume{
-		runtime:  true,
-		disk:     engine.VolumeSpec{Name: name, Path: filepath.Clean(spec.Path), DirectIO: spec.DirectIO},
-		tenants:  slices.Compact(slices.Sorted(slices.Values(spec.Tenants))),
-		writable: spec.Writable,
-	}, nil
-}