pool: register runtime volume catalog entries by minatoaquaMK2 · Pull Request #382 · cocoonstack/sandbox

minatoaquaMK2 · 2026-10-26T11:06:53Z

Part 2 of the #300 series, after #380 and #381, following the split agreed in #379.

Controllers can register existing volume paths without restarting sandboxd. Root-authenticated PUT and DELETE /v1/volumes/{name} require a configured API token. An identical registration succeeds. A changed definition, a static-name collision or unregistering a referenced name returns 409. Empty tenant lists allow every tenant, as with static config.

Registrations use a plain JSON map in runtime-volumes.json, loaded before claim recovery. PUT writes before publishing the entry. DELETE removes an unused entry from memory before writing, including when the name is already absent. A failed write returns 503 for the caller to retry. Successful writes use the existing audit log. An unchanged PUT skips both the write and the audit event.

Optional volume_roots limits cleaned runtime paths to configured roots. The check is lexical and does not resolve symlinks. Static entries are exempt. A saved registration outside the roots prevents startup. Unregister it before narrowing the roots.

Sandboxd does not create, inspect or delete the backing file. Live writable and per-volume attach-only support with SDKs remains in #383. The controller flow follows separately. Stale VM inventory handling is unchanged.

The two-restart failed-detach-write regression remains. Adopted archived claims now regain their volume holds after restart and release them when ownership is lost while no local VM exists. Older replica code can produce these claims when live RO attachment races export. #383 prevents new replicas from being published in that race.

A separate existing path can retain a volume hold after removing a running VM whose ownership moved to another node. This PR fixes the archived case only.

Verification

At 5cd5c4c, the full sandboxd race suite, vet, formatting and Linux and Darwin lint pass. Permission tests ran with DAC bypass disabled. Fault tests cover failed catalog writes, the two-restart sequence and archived reference recovery. Separate Manager tests reproduce the replica race and ownership transfer during wake.

A fresh Linux/KVM run on 5cd5c4c passed all 13 catalog scenarios with Cocoon v0.7.3. It covered authentication, ACLs, guest reads and writes, writer exclusion, daemon restart, unregister and startup refusal after narrowing roots. Offline checks after cleanup verified all four backing images. This run does not cover the later controller flow or power-loss recovery.

Performance

Removing the teardown handoff restores the empty-volume fast path. The helper regression is gone. Complete Release shows no measurable improvement.

These measurements compare main cf9280e, reviewed head 571f96f and fix 1e522a3. The measured release code is unchanged at 5cd5c4c. Each case has five interleaved samples of at least two seconds, on twelve pinned physical cores of a Xeon Gold 6548Y+, Go 1.27.1, GOMAXPROCS=12.

Values are median [min, max]. Parallel values measure aggregate throughput, not request latency.

Case	main	Before	After
Empty-volume quiesce + finish, serial, ns/op	35.95 [30.76, 36.19]	72.27 [71.91, 72.34]	36.51 [36.44, 36.60]
Same pair, 12 workers, ns/op	3.010 [2.993, 3.020]	1,143 [1,132, 1,212]	3.047 [3.043, 3.052]
Complete Release, serial, µs/op	178.2 [83.9, 200.0]	186.8 [168.5, 192.9]	185.7 [165.9, 192.4]
Complete Release, 12 workers, µs/op	96.53 [89.87, 100.53]	95.72 [94.69, 99.24]	95.23 [93.10, 97.86]

The helper allocates nothing. Separate CPU and mutex profiles locate its removed cost in two manager-lock acquisitions. Complete Release uses batches of 120 persisted claims, the real usage journal and a fake VM remover, with sync_claims=false. Claim-file replacement and journal work dominate it. All samples passed their work and error checks. The overlapping Release ranges do not establish a speedup, and these numbers do not measure real VM removal latency.

Load root-token registrations before claim recovery and refuse unregister while claims, operations or VM cleanup retain a reference. Keep detached references protected until their removal is committed.

A teardown that takes over an in-flight volume run's holds now parks them on the run, and finishVolumeRun releases them once. This drops the done poll, the requeues that re-ran vm rm every reap tick, and the ignored bool result of finishVolumeTeardown.

A stopped VM holds no volumes whether or not cocoon marks it stale, and a busy stale create now gets the same classification before it is queued and again when its retry inspects: running with a disk list takes per-volume holds, stopped or absent takes none. Only an Inspect error, a running VM without a disk list (cocoon v0.7.3 omits empty lists and swallows DiskList errors) or any other state keeps the node-wide fence. The table test covers each case by which claims and registrations are refused, with the disk list appearing only after the sweep so both retry paths classify.

The only owner caller checked its run under the same lock, so the check takes the owner ID instead of re-deriving it. It no longer adopts holds: each caller adopts the applied list it already holds and will store, under the same m.mu, so admission neither allocates nor takes a second list that must mirror the resolved one.

Every claimed volume is already counted in the admission holders, and the claim store keeps a per-name count for unacknowledged references, so the walk over every claim under m.mu adds nothing.

This reverts the Transition release around confirmVolumesClean that "pool: persist runtime volume registrations" introduced earlier in this PR. The clean check is one marker stat; dropping Transition around it let a lifecycle change interleave between reservation and staging for no gain.

The pool already rejects an invalid name with the same 400, so the handler check goes; the pool now validates before touching the pending catalog. The handler maps pool errors before context errors, so an uncertain catalog write still answers 503 without its own case. config.Load defaults static directio, and normalization builds the catalog entry directly instead of round-tripping through one-caller registration helpers. Marshaling the catalog record cannot fail: every string in it came through a UTF-8 validating JSON decode or a name regex.

Merge overlapping catalog, ACL, retirement and server tests into tables without per-case branching, share one failed-detach helper, and restore the base claim save helper and capture-test fixtures instead of rewriting their call sites. Drop cases for record file guards that no longer exist, rows that re-test the shared strict body decoder or static path validation, and a config test the pool ACL test covers. Every refusal and recovery path keeps a behavioural assertion.

The catalog map is replaced on every write, never edited, and a name a claim holds cannot be unregistered or rebound while held, so the snapshot taken with the hold state serves the whole pass instead of taking m.mu again for every mount.

Runtime registration rewrote resolveVolumes and the entry resolver but not the attach-only handling, so the two comments explaining why the flag is passed to validation and then cleared still apply.

The checkpoint count covers current and retained references, while the volume count covers retained references only, so the shared "volumes" names hid that difference. Name the count, its helper and the fast-path flag after the retained set they summarize, and seed a loaded claim's references with the same union commit uses instead of the change-aware helper with no previous list.

Registration and unregistration validate the name before they retry a pending catalog write, so an invalid name answers ErrBadVolume (400) instead of the pending write's 503. Nothing pinned the unregister refusal or that ordering. The uncertain-introduction test refused capture but never showed it working again once a snapshot acknowledges the recovery reference.

Keep the observable 503 instead of fence internals, plus the one assumption a reader needs: a stopped VM holds no volumes only because sandboxd controls VM starts and disk attachments in its namespace.

The tenants rule and the operator's backing duties were restated with small differences in the API reference, deploy guide, security model and live-volume example. Keep each in the API reference and link to it from the others. Drop the claim that temporary replacement files are not loaded: the daemon reads only the one record path, so the sentence described no behaviour of its own. Rewrap the overlong lines.

writeCatalog ignores the json.Marshal error, so a Go caller that bypasses the HTTP JSON decode could register a path that is not valid UTF-8 and persist a runtime volume catalog the daemon cannot load on next start. Validate the path in normalizeRegistration and reject it as ErrBadVolume (400 at the HTTP layer) so the catalog file always stays loadable. The name, directio, and tenant fields are already restricted to ASCII.

minatoaquaMK2 · 2026-10-27T00:11:29Z

One thing I'd like your call on before this lands.

With runtime registration, the root API token can register any absolute host path as a volume, writable and public if the request asks for it. That includes paths like data_dir/claims.json or a block device. So the token now gives host file read and write through any guest that claims the volume, not only control over sandboxes. Static config has the same reach, but changing it needs access to the host. The token works over the network.

My suggestion is an optional volume_roots list in the config:

When it is set, runtime registration only accepts a path that, after filepath.Clean, sits under one of those prefixes. Anything else gets a 400.
Static entries are not checked against it, so existing configs keep working.
When it is empty, behaviour stays what this PR does today.

That is a config field, a prefix check in normalizeRegistration, and one table test. I'm happy to add it here or in a follow-up, whichever you prefer. If you'd rather keep the current rule, I'll add a sentence to security.md saying the root token must be protected like host root.

Cocoon reports state=stopped, stale=true, pid=0 when it has lost a VM's PID file. That stop is inferred, not observed: the VMM can still be running and writing its disks. Since e942d31 every stopped VM took no holds, so a failed removal of such a VM lifted the node-wide fence and let a second writer attach, or the name be unregistered, while the old VMM might still hold it. Restore VMRecord.Stale and keep the fence for a stale stop on both the sweep and the busy stale-create retry, which share the classification. An ordinary stopped VM still takes no holds. The fence table gains a stale row for each path, and the inspect test again checks that the flag parses.

969b7ab dropped the claim walk from volumeReferenced on the premise that every claimed volume is counted in the admission holders or in the claim store's retained counts. AdoptArchived breaks that premise: it publishes the adopted claim in m.claimed without taking holders, and the store counts its volumes only once the claims.json commit starts. In that window unregister, or a re-registration under the same name, succeeded while a published claim still named the volume. Walk m.claimed under m.mu again, matching applied and wanted volumes by name without copying, ahead of the store check. The test adopts a peer's row that names a volume while an earlier claims.json write holds the adoption's own write back, and requires unregister to refuse.

The root API token can register any absolute host path as a volume, writable and open to every scope if the request asks for it, so the token reaches host files such as data_dir/claims.json over the network. Static config has the same reach but needs host access to change. An optional volume_roots list limits runtime registration to paths that, once cleaned, equal a root or sit under one on a separator boundary, so /srv/data2 does not match /srv/data and "/" admits everything. Anything else is the existing bad-volume error, a 400. The check is lexical, like static path validation; symlinks are not resolved. Config load rejects a relative or empty root and stores cleaned roots. Persisted registrations go through the same check at startup, so a narrowed list cannot leave an out-of-root registration claimable; like other invalid catalog metadata it prevents startup until the operator unregisters it or widens the roots. Static volumes are not checked, and an empty list keeps today's behaviour.

Operators need to know that the root token can register any absolute host path, and that volume_roots is the way to bound it. The config reference states the lexical rule, its symlink limit, the startup refusal and that static entries are exempt; security.md says to protect the token like host root or set the list, and links to the access-list rule instead of restating it.

Narrowing volume_roots left the daemon unable to start while any saved registration sat outside the new roots, so tightening the bound took the node down until the operator widened it again or edited runtime-volumes.json by hand. Startup now logs a warning naming the volume and leaves it out of the loaded catalog, so it cannot be claimed and the daemon serves the rest. The skip runs after the whole file validates, so malformed or conflicting metadata still refuses startup as before. A recovered claim that names the skipped volume keeps its holders and gets the same refusal as any name missing from the catalog. The entry stays on disk until the next registration change rewrites the file from the loaded catalog, which drops it.

minatoaquaMK2 · 2026-10-27T01:05:36Z

I went ahead and added the volume_roots allowlist in this PR, plus two fixes from another review pass.

volume_roots is an optional list of absolute paths. When it is set, runtime registration only accepts a path equal to a root or under one, checked lexically on separator boundaries after filepath.Clean, so /srv/data2 does not match /srv/data and .. cannot escape. Static volumes are not checked. An empty list keeps today's behaviour. After narrowing the list, saved registrations outside it are skipped with a warning at startup instead of stopping the daemon, the same way bad static tenant names are handled. They are dropped at the next catalog write.
A VM that cocoon reports as stopped with stale: true keeps the node-wide fence. That stop is inferred, for example when the PID file is gone, and the VMM may still be writing.
Unregister scans published claims again, without allocating, so a claim adopted from the registry is protected before its claim write lands.

vet, the full race suite, and Linux and Darwin lint pass. asl shows the same five advisories as main.

minatoaquaMK2 · 2026-10-27T01:49:57Z

I ran this PR at de8300b on a real KVM host. The host runs Ubuntu 26.04 with kernel 7.0.0-38, cocoon v0.7.3 and Cloud Hypervisor v54.0.0. I built sandboxd from a clean checkout with Go 1.27.1, and each run used its own cocoond and sandboxd.

Root PUT and DELETE work. A tenant token gets 403 and a missing token gets 401. Bad names, paths and fields get 400. Repeating the same PUT changes nothing. Changing a binding, reusing a path under another name, or taking a static volume's name gets 409.
With volume_roots set, a path outside the roots gets 400. So do a sibling prefix, a .. escape and data_dir/claims.json. A static volume outside the roots still mounts.
A tenant claim with a runtime volume boots, and the guest reads the expected file hashes on a read-only mount. A tenant outside the access list gets the same 400 as an unknown name.
DELETE gets 409 while the volume is claimed, read-only or writable. A second writable claim also gets 409. After release, DELETE succeeds and the guest's fsynced write is on the host image.
A sandboxd restart keeps the catalog file, the claim, its volume and the same VMM process. After I narrowed volume_roots and restarted, the daemon logged the warning and kept serving. It refused that entry for claims and re-registration, and the next catalog write dropped it from the file.
A plain claim, exec, hibernate, wake and release also work.

I ran it twice. Each run took about 24 seconds and left nothing behind. I did not drive live attach or the stale stopped VM fence on the VM. Unit tests cover the fence.

An explicit empty list behaves like unset, and DELETE of a registration skipped at startup is a no-op until the next catalog write. Say both so operators are not surprised.

CMGS · 2026-10-28T09:41:21Z

Thanks for splitting this out. The API surface matches what we want: root-only PUT/DELETE, the token requirement, idempotent PUT, 409 on a changed definition or a referenced DELETE, loading before claim recovery, and volume_roots as the bound on what the root token can reach. Three parts of the mechanism should go before this merges, though.

1. Drop the volume tables from `claimStore`

currentVolumes, retainedVolumes, retainedVolumeCounts and their helpers (volumeConflicts, detachingOwners, recoveryVolumesFor, hasRecoveryVolumes, referencesVolume, volumeRefs, appendRecoveryRefs) guard against a shared writer after a failed claims.json write. Main's ordering already prevents that:

The intent is committed before any attach. stageVolumeTarget takes the hold, and the attach runs only after the commit.
The device is detached before its hold is dropped.
Every commit rewrites the whole file, so the next successful write of any claim also corrects a stale row.

A stale journal can therefore only over-hold. It never under-holds. I checked this with a test on main cf9280e and on this branch, and it passes on both. The test runs these steps:

Live-attach data to claim A.
Break the store on detach, so the detach succeeds but its journal write fails. The journal still names data and memory holds nothing.
Restart from that journal. A reader hold comes back and a writer claim gets ErrVolumeBusy.
SetVolumes(nil) on A converges.
Restore the store. Claim B takes data rw.
Restart again. The result is exactly one writer, zero readers and a single attach.

The power-loss variant is not reachable either. Losing an acknowledged claims.json write takes a kernel or power loss, which also kills the VMM. Cocoon does not record hot-attached disks: CH DiskAttach only calls vm.add-disk, and Start rebuilds the config from the VM record. A cold-booted VM comes back without the hot disk. So a lost attach intent cannot leave a device attached, and the intent commit needs no forced fsync.

The tables also leak into the admission path, where checkResolved takes store.mu under m.mu, and into volumeView, changeVolumes and the replica scan. Every put also pays for volumeRefs. For unregister, catalog.holders plus the m.claimed walk you already added in 0df2bc5 cover every reference.

2. Replace the pending catalog proposal with write ordering

The pending proposal, the per-name 503 fence, the reap-loop retry and the commit-the-previous-proposal-first step can all go. Two orderings are enough:

Register: under writeMu, write the file, then publish the entry to memory.
Unregister: under writeMu, check for references and remove the entry from memory under m.mu in one step, then write the file. DELETE of an absent name still rewrites the file, so a retry after a failed write converges.

This keeps referenced names ⊆ the memory catalog ⊆ the file at all times. A failed write returns 503, and the caller's retry converges. The worst case is a restart before that retry, which brings back an unreferenced registration. This is the same idea as the volume intent ordering on main, and it needs no extra state.

3. Move the stale-VM fence to its own PR

The catalog.unknown counter, VMRecord.Stale and the resolveStaleVolumes classification are not part of the catalog, and the fence is node-wide. As you note in the description, Cocoon v0.7.3 omits disks both when the list is empty and when the query fails. A running stale VM with no disks therefore stays "unknown". Until its removal succeeds, every volume claim, live attach and registration on the node gets 503. That is the common case for a stale VM. The fix belongs in cocoon: emit disks: [] for an empty list and an error for a failed query. Main's holdStaleVolumes can then stay as it is. A stale stop with a live VMM, meaning a lost PID file without a host reboot, does not justify a node-wide fence on its own.

Smaller items

validateVolumes now warns about invalid static tenants but changes nothing. Either reject them or leave them alone.
The retryRemovals WithoutCancel change and moving the Reconcile commit out of m.mu are unrelated. Please put them in separate commits with their own rationale, or drop them.

Once these are applied, I expect the store diff to be gone and volume_catalog.go to shrink to the two orderings above.

Persist registration before publication and retire unreferenced names before deletion writes. Caller retries converge failures without pending proposals or a second volume-reference history. Preserve checkpoint references, entry identity checks and active cleanup holds.

Keep the stale applied reader recoverable after a successful detach cannot persist its result. Verify that retrying the empty target admits one writer, and a second restart preserves that writer without a stale reader or duplicate attach.

minatoaquaMK2 · 2026-10-28T12:21:05Z

Thanks for the detailed review. I've pushed 571f96f with the simplification you suggested.

Removed the volume tables and helpers from claimStore. Unregister now checks catalog.holders and published claims. claims.go has no diff against the base.
Replaced the pending proposal and background retry with the two write orderings. Register writes before publishing. Unregister checks references and removes the entry under m.mu, then writes. DELETE of an absent name also rewrites the file so caller retries converge.
Removed the stale-VM fence from this PR and restored main's inventory handling. The Cocoon inventory fix remains separate work.

I also dropped the static-tenant warnings and the unrelated WithoutCancel and Reconcile changes.

Added TestLiveDetachResultWriteFailureSurvivesTwoRestarts using the existing fake engine and real claims.json. It covers the failed detach-result write, conservative reader recovery, a successful retry, and exactly one writer after the second restart. It passed 30 race runs. Removing recovery adoption makes it fail by admitting the writer too early.

CMGS

thanks for the quick turn. the store tables and the pending proposal are gone, and the shape is right now. i ran /code, /simplify, use-modern-go and a LOC pass on 571f96f, plus an A/B bench. gates are green on my side: race tests for pool, server, config, types and e2e; golangci-lint 0 issues on linux and darwin; fmt clean; asl only shows the five forwarders that main already has.

inline notes, in order of weight:

must fix: drop the teardown handoff. it adds two m.mu locks to every release, with or without volumes (numbers on quiesceVolumes). removing it brings the path back to main.
must fix: the volume_roots row in deploy.md says DELETE does not rewrite the file. the code always rewrites.
cuts, about -27 prod lines: one loop in resolveVolumes; refuse startup instead of skipping entries outside volume_roots; replace the m.claimed walk with holders on AdoptArchived; drop the ctx.Err checks after writeMu; drop the UTF-8 check; plain JSON file shape, and return the marshal error.
add: recordAudit on register and unregister, like tenants and config reload.
docs: trim the PUT failure internals, the controller duties, and the recovery paragraph. sandboxd treats a disk as an attach interface. its docs should not prescribe the controller's job.
nits and tests: error placement, the allowedFor rename, a no-op write-back, a stale test name, the old bench axis, and two duplicate tests.

keep as they are: the resolvedVolume.entry pointer check, the reference check on register, withinRoots, the path conflict check, runtime, writeMu, the 403 on a node without a token, the runtime tenant-name check, and TestLiveDetachResultWriteFailureSurvivesTwoRestarts.

please also refresh the perf table in the description. it measures the replica scan from the old store-table design. the release-path A/B above is the number that matters now.

all of it nets about -39 prod lines, about -190 test lines and about -20 doc lines.

CMGS · 2026-10-28T14:57:37Z

 // quiesceVolumes unmounts a claim's writable mounts in reverse; a failed unmount keeps the marker.
 func (m *Manager) quiesceVolumes(ctx context.Context, sb *types.Sandbox) volumeTeardown {
-	td := volumeTeardown{holds: heldVolumes(sb)}
+	m.mu.Lock()


this lock and the run field only exist for the teardown handoff from df7c308. i think we can drop the handoff.

one test needs it: TestRemovedVMKeepsVolumeUntilCanceledWorkerReturns. i removed the handoff and ran the pool suite. only that test fails.

that test pauses DiskAttach after the VM is gone. once vm rm returns, the VMM is dead, and VM names are never reused. so no disk can attach. the paused worker only gets an error, and the test checks that too.

main releases the holds when the VM is gone. that is already safe. the handoff protects a count, not a device.

it also costs every release, with or without volumes. quiesceVolumes now takes m.mu every time. finishVolumeTeardown lost its early return, so it takes m.mu every time too. A/B on the quiesce + finish pair for a claim with no volumes (8 interleaved rounds, order rotated, benchstat, p=0.000):

main this PR PR without the handoff

serial 15.1 ns 37.1 ns 14.4 ns

12-way parallel 1.7 ns 245 ns 1.7 ns

suggested change:

drop volumeTeardown.run, volumeRun.teardown, and the release of run.teardown in finishVolumeRun.

build td from heldVolumes(sb) without the lock, like main. call catalogSnapshot() only when mounts > 0.

in finishVolumeTeardown, restore the len(td.holds) == 0 early return and call m.unreserveVolumes(td.holds). its doc line goes back to main's.

drop TestRemovedVMKeepsVolumeUntilCanceledWorkerReturns.

net -12 prod lines and about -54 test lines.

CMGS · 2026-10-28T14:57:37Z


-// finishVolumeTeardown runs only once the VM is gone, when its image lock is finally released.
+// finishVolumeTeardown runs once the VM is gone; an in-flight volume run releases the holds instead.
 func (m *Manager) finishVolumeTeardown(ctx context.Context, td volumeTeardown) {


see the note on quiesceVolumes. please bring back the early return here. a release with no volumes then never touches m.mu in this path.

CMGS · 2026-10-28T14:57:37Z

 | `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: empty means every authenticated scope, and 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); root always has access. `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 |
+| `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. After narrowing, a registration already in `runtime-volumes.json` outside the roots is skipped with a warning at startup, so it cannot be claimed, and the next registration change drops it from the file. A DELETE of a skipped name that nothing references returns 204 without rewriting the file. Keep `data_dir` outside every root. Unset or an empty list accepts any absolute path |


this row does not match the code. UnregisterVolume always rewrites the file, also for an absent name. the API doc says the same: "DELETE always writes the absence". i think 6f45658 changed the behavior and this row did not follow. if you take the startup change on loadRuntimeVolumes, drop the skip part of this row too.

CMGS · 2026-10-28T14:57:37Z

 		return nil, fmt.Errorf("%w: %w", ErrBadVolume, err)
 	}
-	resolved := make([]resolvedVolume, 0, len(applied))
+	m.mu.Lock()


this lock and the first loop look like a leftover from the pending design. one loop is enough: take catalogSnapshot() once, then do the nil check, allowedFor and resolveCatalogEntry in the same loop. checkResolvedLocked checks the entry pointer again under m.mu, so nothing gets weaker. about -5 lines.

CMGS · 2026-10-28T14:57:37Z

+		}
+		entries[name] = volume
+	}
+	// skip only once every entry validated, so other bad metadata still refuses startup


let's refuse startup here instead of skipping. the skip makes a hidden state: the entry is in the file but not in memory, and the next write drops it without a word. a bad entry already refuses startup, so this keeps one rule. the operator can unregister first or widen the roots. both converge. the second loop and this comment go away, about -7 lines.

CMGS · 2026-10-28T14:57:37Z

+	entry := m.catalog.entries["data"]
 	entry.tenants = []string{"other-tenant"}
-	m.volumes["data"] = entry
+	m.catalog.entries["data"] = entry


nit: entries are pointers now, so this write-back does nothing. drop the line.

CMGS · 2026-10-28T14:57:37Z

@@ -222,14 +219,18 @@ func TestVolumeAdmissionMatrix(t *testing.T) {
 func TestReserveVolumesExcludesReaderUnderWriter(t *testing.T) {


nit: reserveVolumes is gone, and this drives admitClaim now. maybe TestAdmitClaimExcludesReaderUnderWriter?

CMGS · 2026-10-28T14:57:37Z

+	"github.com/cocoonstack/sandbox/sandboxd/types"
+)
+
+func BenchmarkVolumeAdmissionWithCheckpoints(b *testing.B) {


claims.go has no diff now, so checkpoints=true and false run the same code. BenchmarkVolumeHoldsWithCheckpoints measures hasVolumeHolds, which this PR does not touch. let's keep one admission bench without the checkpoints axis and drop the rest, about -40 lines.

CMGS · 2026-10-28T14:57:37Z

+	}
+}
+
+func TestRuntimeCatalogValidatesTenantNames(t *testing.T) {


this overlaps the tenant table in volume_catalog_test.go. let's move the extra rows there and drop this test.

CMGS · 2026-10-28T14:57:38Z

 	}
 }

+func TestLoadPreservesInvalidStaticVolumeTenants(t *testing.T) {


TestInvalidStaticTenantListNeverBecomesPublic already loads this config and checks the effect. this one only repeats the load. let's drop it.

Use a plain registration map, reject saved paths outside volume_roots, and audit successful writes. Count adopted holds so reference checks use the existing holder map. Keep documentation and behavior in the same commit.

Remove the worker handoff after confirmed VM removal. Preserve failed-removal and rollback ownership while restoring the empty-volume path without manager locks.

Catalog writes no longer return context cancellation errors. Use the shared result mapping and remove fake cases for the retired pending-write protocol. Put the security API link on the operation names.

A live attach can race an older replica export and become part of the adopted claim. Restore its holds during recovery, as for running claims, so restart does not allow catalog removal. Cover both recovery and release of that persisted legacy state.

An older replica can carry volume references into an archived claim. Drop those holds with the owned claim when it has no local VM; a late wake cannot publish after that check. Use the same path for failed adoption to avoid a second release.

minatoaquaMK2 · 2026-10-29T00:41:40Z

Thanks for the careful review and the A/B measurement. Pushed 5cd5c4c.

I removed the teardown handoff, restored the empty-volume fast path and folded resolution into one loop. The catalog now uses plain JSON, rejects saved paths outside the roots at startup and records successful writes in the audit log. The redundant checks, claim walk and benchmark axis are gone. The two-restart regression stays.

An unchanged PUT skips the write and audit event. DELETE still writes when the name is absent, so its successful retry also records the removal.

I found a real producer for a volume-bearing peer row. Live RO attachment can finish while an older replica export is in progress. The later publication then includes that volume. I reproduced this through export and adoption without editing the row. Archived recovery now restores the hold, and ownership transfer releases it when there is no local VM. #383 prevents new publication in that race. A separate existing running-VM transfer path can still leave a stale hold after VM removal.

The release comparison used five interleaved samples per case on twelve pinned physical cores, Go 1.27.1 and GOMAXPROCS=12. These remain measurements of 1e522a3. The measured release code is unchanged at 5cd5c4c.

Case	main `cf9280e`	Before `571f96f`	After `1e522a3`
Empty-volume quiesce + finish, serial	35.95 ns/op	72.27 ns/op	36.51 ns/op
Same pair, 12 workers	3.010 ns/op	1,143 ns/op	3.047 ns/op
Complete Release, serial	178.2 µs/op	186.8 µs/op	185.7 µs/op
Complete Release, 12 workers	96.53 µs/op	95.72 µs/op	95.23 µs/op

These are medians. Parallel values measure aggregate throughput. Profiles point to the two manager locks in the helper. Complete Release uses real claims and usage journals, fsync off and a fake VM remover. Its serial ranges overlap, 168.5 to 192.9 µs before and 165.9 to 192.4 µs after. There is no measurable whole-release speedup. The description includes all ranges.

The final head passes the sandboxd race suite, vet, formatting and Linux and Darwin lint. A fresh 13-step Linux/KVM run passed catalog registration, guest I/O, writer exclusion, restart and unregister. Offline checks after cleanup preserved the data. I also rebased #383 and the controller branch onto this head. #383 remains draft.

A recovered archived claim must release its reservation after durable expiry, retain it when deletion fails, and leave another reader protected. Exercise cancellation and repeated retirement through writer admission and catalog removal.

Transfer the archived claim reservations only after validating the reap victim. Release them once claim deletion commits so canceled checkpoint cleanup cannot strand admission holds. Preserve the existing rollback ownership and use the archived state alone when forgetting a claim.

Use a record captured from the replica publication race at 5cd5c4c. Writer admission detects missing adoption holds and verifies that repeated ownership transfer cleanup preserves an independent reader.

Release archived reservations together after the claims write succeeds. Keep cleanup independent of canceled workers and avoid a second conditional loop in reapOnce. Use distinct error names in the regression tests.

Release a transferred running claim's volume holds after VM removal, report catalog marshal failures as permanent, check volume_roots inside catalog normalization, and align the runtime volume docs with the code.

WithVolumesAttachOnly and mount=False no longer validate locally (sandboxd answers 400), Volume carries attach_only, a mount reporting "not a valid block device" is retried within the setup budget (#404), and the README names runtime volume registration (#382).

minatoaquaMK2 added 3 commits October 26, 2026 10:38

pool: persist runtime volume registrations

2c2e02b

Load root-token registrations before claim recovery and refuse unregister while claims, operations or VM cleanup retain a reference. Keep detached references protected until their removal is committed.

test(pool): reproduce unrelated admission during catalog failure

247227e

fix(pool): scope catalog uncertainty to affected volumes

debf729

minatoaquaMK2 marked this pull request as draft October 26, 2026 13:05

minatoaquaMK2 added 19 commits October 26, 2026 13:31

pool: reproduce private name disclosure during catalog writes

af6ee9a

pool: check volume authorization before pending admission

8303c4b

pool: reproduce inventory changes during failed removal

e51545d

pool: preserve stale inventory observation after failed removal

aff9797

pool: align cleanup tests with post-removal inspection

f49765a

pool: count catalog references from holders and the journal

969b7ab

Every claimed volume is already counted in the admission holders, and the claim store keeps a per-name count for unacknowledged references, so the walk over every claim under m.mu adds nothing.

pool: restore the attach-only resolution comments

49b468a

Runtime registration rewrote resolveVolumes and the entry resolver but not the attach-only handling, so the two comments explaining why the flag is passed to validation and then cleared still apply.

docs: trim volume fence prose

70f98a9

Keep the observable 503 instead of fence internals, plus the one assumption a reader needs: a stopped VM holds no volumes only because sandboxd controls VM starts and disk attachments in its namespace.

minatoaquaMK2 added 5 commits October 27, 2026 00:38

minatoaquaMK2 marked this pull request as ready for review October 27, 2026 01:05

minatoaquaMK2 added 2 commits October 27, 2026 02:12

pool: group volume catalog methods

c5fa28f

docs: note empty volume_roots and DELETE of a skipped registration

09bb47b

An explicit empty list behaves like unset, and DELETE of a registration skipped at startup is a no-op until the next catalog write. Say both so operators are not surprised.

This was referenced Oct 27, 2026

pool: allow live writable and attach-only volumes minatoaquaMK2/sandbox#5

Closed

pool: allow live writable and attach-only volumes #383

Merged

minatoaquaMK2 added 2 commits October 28, 2026 10:14

CMGS reviewed Oct 28, 2026

View reviewed changes

minatoaquaMK2 added 5 commits October 29, 2026 00:01

pool: simplify runtime volume catalog persistence

3655d0e

Use a plain registration map, reject saved paths outside volume_roots, and audit successful writes. Count adopted holds so reference checks use the existing holder map. Keep documentation and behavior in the same commit.

pool: release volume holds when VM removal completes

0423215

Remove the worker handoff after confirmed VM removal. Preserve failed-removal and rollback ownership while restoring the empty-volume path without manager locks.

server: remove obsolete catalog cancellation responses

6da1ab7

Catalog writes no longer return context cancellation errors. Use the shared result mapping and remove fake cases for the retired pending-write protocol. Put the security API link on the operation names.

minatoaquaMK2 added 4 commits October 29, 2026 00:54

pool: cover archived volume holds through expiry

63a90c5

A recovered archived claim must release its reservation after durable expiry, retain it when deletion fails, and leave another reader protected. Exercise cancellation and repeated retirement through writer admission and catalog removal.

pool: test legacy replica volume adoption and retirement

f38a0ee

Use a record captured from the replica publication race at 5cd5c4c. Writer admission detects missing adoption holds and verifies that repeated ownership transfer cleanup preserves an independent reader.

pool: batch archived hold retirement after reap commits

d55e43a

Release archived reservations together after the claims write succeeds. Keep cleanup independent of canceled workers and avoid a second conditional loop in reapOnce. Use distinct error names in the regression tests.

CMGS merged commit f7b19b4 into cocoonstack:main Oct 29, 2026
1 check passed

CMGS mentioned this pull request Oct 29, 2026

pool: runtime volume catalog follow-ups to #382 #395

Merged

This was referenced Oct 29, 2026

pool: one rule per live volume invariant, whole-repo audit #398

Merged

pool: drop volume holds for archived claims; plan compat removals #399

Merged

minatoaquaMK2 mentioned this pull request Oct 29, 2026

docs: document controller-managed volume hibernation #403

Closed

CMGS mentioned this pull request Oct 29, 2026

review: whole-repo audit after the volume merges, with the #403 controller docs #405

Merged

	main	this PR	PR without the handoff
serial	15.1 ns	37.1 ns	14.4 ns
12-way parallel	1.7 ns	245 ns	1.7 ns

		@@ -222,14 +219,18 @@ func TestVolumeAdmissionMatrix(t *testing.T) {
		func TestReserveVolumesExcludesReaderUnderWriter(t *testing.T) {

Conversation

minatoaquaMK2 commented Oct 26, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Verification

Performance

Uh oh!

minatoaquaMK2 commented Oct 27, 2026

Uh oh!

minatoaquaMK2 commented Oct 27, 2026

Uh oh!

minatoaquaMK2 commented Oct 27, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

CMGS commented Oct 28, 2026

1. Drop the volume tables from claimStore

2. Replace the pending catalog proposal with write ordering

3. Move the stale-VM fence to its own PR

Smaller items

Uh oh!

minatoaquaMK2 commented Oct 28, 2026

Uh oh!

CMGS left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

minatoaquaMK2 commented Oct 29, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

minatoaquaMK2 commented Oct 26, 2026 •

edited

Loading

minatoaquaMK2 commented Oct 27, 2026 •

edited

Loading

1. Drop the volume tables from `claimStore`