From 9ba767110b4b4ae890df238e252c7127379079d1 Mon Sep 17 00:00:00 2001 From: CMGS Date: Fri, 10 Jul 2026 01:22:18 +0800 Subject: [PATCH] review: tighten comments, fix stale claims-journal docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docs/performance.md's "Claims-journal write path" stated a decision (keep the in-mutex write) that F1/F1-part-2 reversed; rewrite it for the snapshot()/commit() split. claimStore's doc likewise still said the marshal runs under the manager mutex — F1-part-2 moved it off. Drop restate-of-code and scenario narrative from a few WHY comments; no behavior change. --- boot/init/src/cfg.rs | 3 --- docs/performance.md | 29 ++++++++++------------ sandboxd/engine/portconn.go | 9 +++---- sandboxd/pool/claims.go | 11 ++++---- sandboxd/server/server.go | 11 +++----- sdk/openai/cocoonsandbox_openai/adapter.py | 2 +- 6 files changed, 26 insertions(+), 39 deletions(-) diff --git a/boot/init/src/cfg.rs b/boot/init/src/cfg.rs index 337764f..679b2b0 100644 --- a/boot/init/src/cfg.rs +++ b/boot/init/src/cfg.rs @@ -327,16 +327,13 @@ mod tests { #[test] fn parse_ip_param_variants() { - // No DNS, zero gateway. let p = parse_ip_param("10.0.0.2::0.0.0.0:255.255.254.0:vm:eth1:off").unwrap(); assert_eq!(p.prefix, 23); assert_eq!(p.gateway, None); assert!(p.dns.is_empty()); assert_eq!(p.device, "eth1"); - // Zero DNS entries are filtered. let p = parse_ip_param("10.0.0.2::10.0.0.1:255.255.255.0:vm:eth0:off:0.0.0.0").unwrap(); assert!(p.dns.is_empty()); - // Shorthand and malformed forms are ignored. assert_eq!(parse_ip_param("dhcp"), None); assert_eq!(parse_ip_param("off"), None); assert_eq!(parse_ip_param("10.0.0.2::gw:not-a-mask:vm:eth0:off"), None); diff --git a/docs/performance.md b/docs/performance.md index 6c6809e..8789608 100644 --- a/docs/performance.md +++ b/docs/performance.md @@ -101,22 +101,19 @@ snapshot paths. ## Claims-journal write path -Every claim/release/hibernate persists `claims.json` inside the pool mutex -(marshal + write + rename). Benchmark (`BenchmarkStoreSaveScaling`, APFS -SSD), per save: - -| live claims | ns/op | B/op | -|---|---|---| -| 10 | ~181k (0.18ms) | 4.5k | -| 100 | ~240k (0.24ms) | 36k | -| 1000 | ~829k (0.83ms) | 385k | - -At realistic scale the in-mutex write is sub-millisecond and dwarfed by the -provision path (~40ms clone, ~230ms cold); the mutex-held serialization -does not approach a 2× claim-latency regression. **Decision: keep the -simple in-mutex write; do not move to a store-owned sequential writer.** -The benchmark stays as a regression sentinel — revisit only if it crosses -into the milliseconds at the deployment's real claim count. +`claim`/`release`/`hibernate`/`wake` persist `claims.json`, but the write is +kept off the manager mutex — the lock every data-plane op contends. `snapshot()` +takes a cheap by-value copy of the claim map under the mutex; `commit()` marshals +it, writes, and renames off the mutex, serialized and coalescing by sequence so +an older snapshot never overwrites a newer one. Only the startup `Reconcile` +pass (pre-contention) still marshals and writes in one call under the lock. + +`BenchmarkStorePersistContention` measures the ns a concurrent manager-mutex +acquire waits during a persist. Moving first the write (F1), then the marshal +(F1 part 2), off the lock cut that wait from tens of µs to tens of ns — the +marshal-off-lock split alone is a ~6× drop at 1000 live claims, a margin that +grows with claim count. `BenchmarkStoreSaveScaling` (the combined `save()` +Reconcile uses) stays as a regression sentinel. ## Measured and declined (decision data) diff --git a/sandboxd/engine/portconn.go b/sandboxd/engine/portconn.go index 1c52f8f..56dfbfb 100644 --- a/sandboxd/engine/portconn.go +++ b/sandboxd/engine/portconn.go @@ -20,12 +20,9 @@ const ( portReadBuf = 64 << 10 ) -// guestPortConn adapts silkd's framed port_forward channel to a plain -// net.Conn: the guest's TCP bytes ride inside newline-JSON `data` frames -// (base64 payloads), so writes wrap into `{"op":"data",...}` frames and -// reads unwrap `{"type":"data",...}` frames back to raw bytes. This is the -// server-side twin of the SDK's PortConn, so a reverse proxy can splice HTTP -// straight through it. +// guestPortConn adapts silkd's newline-JSON data-frame port_forward channel to +// a plain net.Conn (base64 payloads), the server-side twin of the SDK's +// PortConn, so a reverse proxy can splice HTTP straight through it. type guestPortConn struct { net.Conn r *bufio.Reader diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index ef5e567..9d2939e 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -41,12 +41,11 @@ type claimDTO struct { // VMs are deliberately not persisted: they are cheap to rebuild and unsafe // to trust after an unsupervised gap. // -// A write is split so the manager mutex covers only the marshal (a consistent -// snapshot of the claim map), not the file syscalls: snapshot() runs under the -// manager mutex, commit() writes off it. commit is serialized and coalescing — -// a snapshot no newer than one already on disk is dropped, because sequence -// order matches the manager-mutex mutation order, so the newest write wins and -// an older one only ever describes a superseded state. +// The write is split to keep the manager mutex off both the marshal and the +// file syscalls: snapshot() takes a cheap field-copy under the mutex, commit() +// marshals and writes off it — serialized and coalescing so a snapshot no newer +// than one already on disk is dropped (sequence order matches mutation order, +// so an older write only ever describes a superseded state). type claimStore struct { path string diff --git a/sandboxd/server/server.go b/sandboxd/server/server.go index cf20b3e..590afbd 100644 --- a/sandboxd/server/server.go +++ b/sandboxd/server/server.go @@ -210,13 +210,10 @@ func (s *Server) handleClaim(w http.ResponseWriter, r *http.Request) { } } -// redirectClaim answers a warm-miss claim with a redirect when a peer is the -// better node: one that reports warm sandboxes, or — when this node has no -// golden for the key — the owner of a promoted template gossip names, which -// provisions from it instead of us cold-booting a nonexistent image ref. A -// claim already redirected here carries no_redirect and must warm-or-provision -// locally, never bounce again — that avoids a two-node stale-view ping-pong -// when both just emptied their pools. +// redirectClaim redirects a warm-miss to a better peer — a warm holder, or the +// template owner when we lack a golden (so we don't cold-boot a nonexistent +// image ref). A no_redirect request must resolve locally, never bounce again, +// to avoid a two-node ping-pong. func (s *Server) redirectClaim(ctx context.Context, w http.ResponseWriter, req types.ClaimRequest, key types.PoolKey, hash string) bool { if s.placer == nil || req.NoRedirect { return false diff --git a/sdk/openai/cocoonsandbox_openai/adapter.py b/sdk/openai/cocoonsandbox_openai/adapter.py index ab6b493..b3ede58 100644 --- a/sdk/openai/cocoonsandbox_openai/adapter.py +++ b/sdk/openai/cocoonsandbox_openai/adapter.py @@ -165,7 +165,7 @@ async def delete(self, session: SandboxSession) -> SandboxSession: inner = session._inner if not isinstance(inner, CocoonSandboxSession): raise TypeError("CocoonSandboxClient.delete expects a CocoonSandboxSession") - await inner._shutdown_backend() # close any local port proxies + await inner._shutdown_backend() await asyncio.to_thread(inner._sandbox().close) # 404-safe in the SDK return session