From 92505c785f66a9e96e2a1592efd9584ef804cef8 Mon Sep 17 00:00:00 2001 From: CMGS Date: Fri, 11 Sep 2026 11:13:12 +0900 Subject: [PATCH 1/4] sandboxd: run the pool warmup in every clone at refill A restore maps the golden memory lazily, so the pages the golden warmup made resident still fault into each clone on first touch. Under nested virtualization that first touch is the whole time-to-interactive: at 100 concurrent claims the first `node -v` in a fresh clone measured p50 824 ms against 92 ms once the pages were in, while the host sat half idle. Re-running the warmup in the clone before it joins the warm pool moves that cost to refill; the claim path is unchanged. --- docs/deploy.md | 2 +- sandboxd/config/config.go | 2 +- sandboxd/pool/pool_test.go | 12 +++++---- sandboxd/pool/refill.go | 17 ++++++++++++ sandboxd/pool/warmup_test.go | 50 ++++++++++++++++++++++++++++++++++++ 5 files changed, 76 insertions(+), 7 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index d9795d9a..f1388ce3 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -105,7 +105,7 @@ sandboxd reads one JSON file (`-config`, default | `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 | -| `warmup` (pool entry) | unset | argv run in the golden VM after readiness and before its snapshot, so the files it touches are page-cache-resident in every clone — e.g. `["node", "-e", "0"]` on a Node flavor. It runs under the engine's 2-minute command timeout in silkd's base environment (`PATH`, `TERM`, and the node's proxy variables on the none lane); a non-zero exit or a timeout fails the golden build, so the pool stays unfilled until the config is fixed. Config-owned like `egress`: `PUT /v1/pools` rejects it, and a golden built with a different warmup is rebuilt | +| `warmup` (pool entry) | unset | argv run in the golden VM after readiness and before its snapshot, so the files it touches are page-cache-resident in every clone, and again in every clone before it joins the warm pool, so those pages are already faulted into the restored VM when the first command runs — e.g. `["node", "-e", "0"]` on a Node flavor. It runs under the engine's 2-minute command timeout in silkd's base environment (`PATH`, `TERM`, and the node's proxy variables on the none lane); a non-zero exit or a timeout fails the golden build, so the pool stays unfilled until the config is fixed. Config-owned like `egress`: `PUT /v1/pools` rejects it, and a golden built with a different warmup is rebuilt | | `max_claims` | 0 (unlimited) | node-wide cap on live claims; claim/fork/branch requests beyond it answer 429 with the pool state unharmed (on a cluster, normal warm-candidate placement applies, with volume claims limited to candidates holding every requested volume) | | `audit_log` | false | append every relayed request frame's op + addressing fields (never payloads) to `/audit.jsonl`, size-rotated with one `.1` backup. Records are `{t, id, op}` plus whichever addressing fields the op carries (`argv`, `path`, `dest`, `from`, `to`, `url`, `session`, `port`), plus `decision` and `secret` (the ref name, never its value) on `egress` records; preview accesses record as op `preview`, one per request. A request frame whose first line exceeds 4 KiB is skipped, never truncated | | `idle_hibernate_seconds` | 0 (off) | node-wide idle policy for unpooled claims (template/checkpoint claims): a none-lane claim with no data-plane connection for this long is hibernated; the next call that reaches the guest wakes it transparently. Per-pool `idle_hibernate_seconds` does the same for that pool's claims; pooled keys ignore the node-wide value, and egress pools reject it because they cannot resume safely. Opt in deliberately: a wake costs latency and the snapshot, so callers with their own idle logic must not pay twice | diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index ed11592f..d7b15e12 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -45,7 +45,7 @@ type PoolSpec struct { // Egress is this pool's allow-list, intersected with the tenant's; nil denies all egress. Egress *egress.Policy `json:"egress,omitempty"` - // Warmup runs in the golden VM before its snapshot, so every clone starts with its page cache. + // Warmup runs in the golden VM before its snapshot and again in each clone after restore. Warmup []string `json:"warmup,omitempty"` // IdleHibernateSeconds, when >0, hibernates idle claims after that many seconds. diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 6a45df17..19c17fa8 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -795,6 +795,9 @@ type fakeEngine struct { exportContent []byte caInstalls []string warmups [][]string + warmupSocks []string + warmupSnaps []int + warmupErr error staleReconciles []string installCAErr error diskAttachErr error @@ -1031,14 +1034,13 @@ func (f *fakeEngine) InstallCACert(_ context.Context, vsockSocket string, _ []by return f.installCAErr } -func (f *fakeEngine) Warmup(_ context.Context, _ string, argv []string) error { +func (f *fakeEngine) Warmup(_ context.Context, sock string, argv []string) error { f.mu.Lock() defer f.mu.Unlock() - if len(f.snapSaves) > 0 { - return fmt.Errorf("warmup after snapshot") - } f.warmups = append(f.warmups, argv) - return nil + f.warmupSocks = append(f.warmupSocks, sock) + f.warmupSnaps = append(f.warmupSnaps, len(f.snapSaves)) + return f.warmupErr } func (f *fakeEngine) DiskAttach(_ context.Context, _ string, spec engine.VolumeSpec) error { diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index a3d712b6..a0908359 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -90,6 +90,9 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { } sb, err = m.readyBounded(ctx, sb, time.Now().Add(warmProbeTimeout)) } + if err == nil { + err = m.warmClone(ctx, p.key, sb) + } keep := false var fails int var wait time.Duration @@ -218,6 +221,20 @@ func (m *Manager) poolWarmup(key types.PoolKey) []string { return m.poolWarmups[key] } +// warmClone re-runs the pool warmup in a restored clone: a restore maps the golden memory lazily, +// so the pages the warmup touched fault in here, at refill, instead of under the first claim. +func (m *Manager) warmClone(ctx context.Context, key types.PoolKey, sb *types.Sandbox) error { + warmup := m.poolWarmup(key) + if len(warmup) == 0 { + return nil + } + if err := m.eng.Warmup(ctx, sb.VsockSocket, warmup); err != nil { + m.destroy(ctx, sb.VMName) + return fmt.Errorf("clone warmup: %w", err) + } + return nil +} + // writeGoldenCASidecar records or clears the baked-CA fingerprint; a rotated CA forces a rebuild. func (m *Manager) writeGoldenCASidecar(final string, caBaked bool) error { var stamp string diff --git a/sandboxd/pool/warmup_test.go b/sandboxd/pool/warmup_test.go index 63620e77..68db93ed 100644 --- a/sandboxd/pool/warmup_test.go +++ b/sandboxd/pool/warmup_test.go @@ -7,6 +7,7 @@ import ( "slices" "strings" "testing" + "testing/synctest" "github.com/cocoonstack/sandbox/sandboxd/config" ) @@ -22,6 +23,9 @@ func TestGoldenBuildRunsWarmupBeforeSnapshot(t *testing.T) { if len(eng.warmups) != 1 || !slices.Equal(eng.warmups[0], argv) { t.Fatalf("warmups = %v, want [%v]", eng.warmups, argv) } + if eng.warmupSnaps[0] != 0 { + t.Fatalf("golden warmup ran after %d snapshot saves, want 0", eng.warmupSnaps[0]) + } stamp, err := os.ReadFile(final + warmupSidecarSuffix) if err != nil { t.Fatalf("read warmup sidecar: %v", err) @@ -87,3 +91,49 @@ func TestPoolSpecRejectsEmptyWarmupArgument(t *testing.T) { t.Errorf("ValidateLimits error = %v, want an empty-argument rejection", err) } } + +func TestRefillWarmsEveryClone(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + eng := newFakeEngine() + argv := []string{"node", "-e", "0"} + m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 2, Warmup: argv}) + m.pools[testKey].goldenDir = "/goldens/x" + + m.refillOnce(t.Context()) + waitFor(t, func() bool { + infos, _ := m.Info() + return infos[0].Warm == 2 && infos[0].Refilling == 0 + }) + eng.mu.Lock() + defer eng.mu.Unlock() + if len(eng.warmups) != 2 || !slices.Equal(eng.warmups[0], argv) || !slices.Equal(eng.warmups[1], argv) { + t.Fatalf("warmups = %v, want %v once per clone", eng.warmups, argv) + } + if eng.warmupSocks[0] == "" || eng.warmupSocks[0] == eng.warmupSocks[1] { + t.Errorf("warmup sockets = %v, want one per clone", eng.warmupSocks) + } + }) +} + +func TestRefillCloneWarmupFailureCleansUp(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + eng := newFakeEngine() + eng.warmupErr = errors.New("node: not found") + m := newTestManager(t, eng, config.PoolSpec{PoolKey: testKey, Warm: 1, Warmup: []string{"node", "-e", "0"}}) + m.pools[testKey].goldenDir = "/goldens/x" + + m.refillOnce(t.Context()) + waitFor(t, func() bool { + infos, _ := m.Info() + return infos[0].Warm == 0 && infos[0].Refilling == 0 && len(eng.removedNames()) == 1 + }) + eng.mu.Lock() + eng.warmupErr = nil + eng.mu.Unlock() + m.refillOnce(t.Context()) + waitFor(t, func() bool { + infos, _ := m.Info() + return infos[0].Warm == 1 && infos[0].Refilling == 0 + }) + }) +} From 5fc208ad113daecaca33fd4587d01747eb429e0b Mon Sep 17 00:00:00 2001 From: CMGS Date: Fri, 11 Sep 2026 11:32:47 +0900 Subject: [PATCH 2/4] sandboxd: add release_delay_seconds to keep teardown out of a claim burst A release drops and journals the claim, then removes the VM inline. In a 100-way burst the early destroys land while other claims still run their first command: with destroys held until the burst ended the same command measured p95 254-293 ms, inline 620-646 ms. With release_delay_seconds set the release returns once the claim is gone and the VM, volume, egress and snapshot cleanup run that many seconds later; teardown failures are logged by the removal queue as before. --- docs/deploy.md | 1 + sandboxd/config/config.go | 29 +++++++++++++++++------------ sandboxd/pool/claim.go | 17 +++++++++++++++-- sandboxd/pool/pool.go | 8 +++++--- sandboxd/pool/pool_test.go | 24 ++++++++++++++++++++++++ 5 files changed, 62 insertions(+), 17 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index f1388ce3..3ec258f8 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -97,6 +97,7 @@ sandboxd reads one JSON file (`-config`, default | `tenants` | unset | multi-tenant tokens next to `api_token`: `[{"name": "acme", "token": "…", "max_claims": 50}]`. A tenant token reaches the resource-creating verbs (claim, fork, promote, checkpoint, preview), catalog discovery, and its own sandbox/checkpoint listings; everything it creates is stamped with the tenant name. Root-only surfaces (per-id sandbox reads, `GET /v1/info`, `PUT /v1/pools`, `POST/DELETE /v1/drain`, `/metrics`) answer it 403. `max_claims` (0 = unlimited) caps that tenant's live claims next to the node-wide cap. Requires `api_token` set. Names and tokens must be unique, tokens distinct from `api_token`. On a cluster all nodes must carry the same tenants set (the SDK replays whichever token authorized a redirect), and per-node caps mean a tenant's effective cluster limit is `max_claims` × nodes. Empty = exactly the single-token behavior | | `max_fork_count` | 16 | children a single `fork` may create; each is a full-RAM VM, so this bounds one request's memory blast radius to the node's capacity | | `refill_concurrency` | 0 (auto) | concurrent VM provisioning budget, shared by warm-pool refills, fork clones, and the reap/hibernate/reconcile engine batches. 0 sizes it from the node: `NumCPU*2/3` clamped to [4, 256] — a 384-core node gets 256; small nodes keep a floor of 4 | +| `release_delay_seconds` | 0 | seconds between a release returning and its VM teardown. The claim is dropped and journaled at release; only the `cocoon vm rm`, volume and snapshot cleanup wait, so a burst of releases does not compete with the claims still running. 0 tears down inline | | `preview_listen` | (off) | address for a preview HTTP server that serves guest ports under signed URLs; needs `preview_secret` | | `preview_secret` | — | cluster-shared HMAC secret signing preview tokens (all nodes share one) | | `preview_advertise` | = `preview_listen` | the browser-facing preview base URL; nodes behind one TLS proxy may share it, while signed tokens route internally through each owner's `advertise_addr` | diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index d7b15e12..0bc4726b 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -231,6 +231,9 @@ type Config struct { // RefillConcurrency caps concurrent VM provisioning node-wide; 0 auto-scales with CPUs. RefillConcurrency int `json:"refill_concurrency,omitzero"` + // ReleaseDelaySeconds, when >0, delays a released VM's teardown so it stays out of a claim burst. + ReleaseDelaySeconds int `json:"release_delay_seconds,omitempty"` + // Mesh, when set, joins this node to a memberlist cluster; nil is a mesh of one. Mesh *MeshConfig `json:"mesh,omitempty"` @@ -297,6 +300,20 @@ func (c *Config) applyDefaults() { } func (c *Config) validate() error { + for _, n := range []struct { + name string + value int + }{ + {"release_delay_seconds", c.ReleaseDelaySeconds}, + {"refill_concurrency", c.RefillConcurrency}, + {"max_claims", c.MaxClaims}, + {"idle_hibernate_seconds", c.IdleHibernateSeconds}, + {"checkpoint_ttl_hours", c.CheckpointTTLHours}, + } { + if n.value < 0 { + return fmt.Errorf("%s must not be negative, got %d", n.name, n.value) + } + } if err := c.validateAttachment(); err != nil { return err } @@ -307,21 +324,12 @@ func (c *Config) validate() error { if c.MaxForkCount < 1 { return fmt.Errorf("max_fork_count must be at least 1, got %d", c.MaxForkCount) } - if c.RefillConcurrency < 0 { - return fmt.Errorf("refill_concurrency must not be negative, got %d", c.RefillConcurrency) - } if err := c.RestoreMode.Validate(); err != nil { return fmt.Errorf("restore_mode: %w", err) } - if c.MaxClaims < 0 { - return fmt.Errorf("max_claims must not be negative, got %d", c.MaxClaims) - } if c.PreviewListen != "" && c.PreviewSecret == "" { return fmt.Errorf("preview_listen needs preview_secret") } - if c.IdleHibernateSeconds < 0 { - return fmt.Errorf("idle_hibernate_seconds must not be negative, got %d", c.IdleHibernateSeconds) - } if err := validateArchiveWindow(c.IdleHibernateSeconds, c.ArchiveAfterSeconds, c.ArchiveDeleteAfterSeconds); err != nil { return err } @@ -336,9 +344,6 @@ func (c *Config) validate() error { return fmt.Errorf("checkpoint_store kind %q: want dir or s3", cs.Kind) } } - if c.CheckpointTTLHours < 0 { - return fmt.Errorf("checkpoint_ttl_hours must not be negative") - } if c.CheckpointPeerHeal && (c.Mesh == nil || c.Mesh.ClusterKey == "") { return fmt.Errorf("checkpoint_peer_heal requires an encrypted mesh (set mesh.cluster_key)") } diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index f7d248a6..da2582df 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -167,6 +167,21 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM m.untrack(m.pendingCks, ck) } + m.counters.releases.Add(1) + m.recordUsage(ctx, usageEvent{Event: "release", ID: id, VMName: vmName}) + if m.releaseDelay > 0 && vmName != "" { + time.AfterFunc(m.releaseDelay, func() { + if err := m.teardown(ctx, id, sb, vmName, snap); err != nil { + log.WithFunc("pool.releaseResolved").Errorf(ctx, err, "delayed teardown of %s", id) + } + }) + return nil + } + return m.teardown(ctx, id, sb, vmName, snap) +} + +// teardown frees what a released claim still holds: volumes, the VM, its egress state and snapshot. +func (m *Manager) teardown(ctx context.Context, id string, sb *types.Sandbox, vmName, snap string) error { td := m.quiesceVolumes(ctx, sb) var err error if vmName == "" { @@ -176,8 +191,6 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand } m.disarmEgress(id, err == nil) m.dropSnap(ctx, snap) - m.counters.releases.Add(1) - m.recordUsage(ctx, usageEvent{Event: "release", ID: id, VMName: vmName}) return err } diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index a7db4648..a1ca46e6 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -325,9 +325,10 @@ type Manager struct { atCapacityUntil time.Time atCapacityReason string - refillSem chan struct{} - probeSem chan struct{} - refillKick chan struct{} + refillSem chan struct{} + releaseDelay time.Duration + probeSem chan struct{} + refillKick chan struct{} } // NewManager builds a manager from the node config; ctx bounds backend construction. @@ -367,6 +368,7 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg dial: newEgressDialer(parsePrefixes(cfg.EgressInternalAllow)).DialContext, sweep: netfilter.SweepExcept, refillSem: make(chan struct{}, refill), + releaseDelay: time.Duration(cfg.ReleaseDelaySeconds) * time.Second, probeSem: make(chan struct{}, refill), refillKick: make(chan struct{}, 1), healSem: make(chan struct{}, maxConcurrentHeals), diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 19c17fa8..1ccd5f9b 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -214,6 +214,30 @@ func TestReleaseAfterResolveTearsDownOnce(t *testing.T) { } } +func TestReleaseDelayDefersTeardown(t *testing.T) { + synctest.Test(t, func(t *testing.T) { + eng := newFakeEngine() + m := newTestManager(t, eng) + m.releaseDelay = 2 * time.Second + sb := mustClaim(t, m, testKey) + + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatalf("release: %v", err) + } + if _, g := m.Info(); g.Claimed != 0 { + t.Fatalf("claimed=%d after release, want 0", g.Claimed) + } + if removed := eng.removedNames(); len(removed) != 0 { + t.Fatalf("removes=%v before the delay, want none", removed) + } + time.Sleep(2 * time.Second) + synctest.Wait() + if removed := eng.removedNames(); len(removed) != 1 || removed[0] != sb.VMName { + t.Fatalf("removes=%v after the delay, want %s", removed, sb.VMName) + } + }) +} + func TestReleaseByOperatorCred(t *testing.T) { eng := newFakeEngine() m := newTestManager(t, eng) From 760a1123c2e3ac24571e9136c8c11511f9aa7e58 Mon Sep 17 00:00:00 2001 From: CMGS Date: Fri, 11 Sep 2026 13:20:27 +0900 Subject: [PATCH 3/4] sandboxd: run a delayed release through the removal queue release_delay_seconds armed one time.AfterFunc per release, so a burst of releases became the same burst of `cocoon vm rm` a delay later, outside the provisioning budget every other batch teardown runs on. The release now parks the VM in pendingRemovals with a not-before stamp; retryRemovals dispatches only the due entries on the reap tick through runBounded, so the teardown is rate-limited by refill_concurrency and a survivor keeps the existing retry path. The egress listener still closes at release; the tap unlock and the volume hold release follow the removal, as on a retried survivor. --- docs/deploy.md | 2 +- sandboxd/config/config.go | 4 ++-- sandboxd/pool/claim.go | 31 ++++++++++++------------------- sandboxd/pool/pool.go | 1 + sandboxd/pool/pool_test.go | 5 +++-- sandboxd/pool/remove.go | 26 ++++++++++++++++---------- 6 files changed, 35 insertions(+), 34 deletions(-) diff --git a/docs/deploy.md b/docs/deploy.md index 3ec258f8..6b528245 100644 --- a/docs/deploy.md +++ b/docs/deploy.md @@ -97,7 +97,7 @@ sandboxd reads one JSON file (`-config`, default | `tenants` | unset | multi-tenant tokens next to `api_token`: `[{"name": "acme", "token": "…", "max_claims": 50}]`. A tenant token reaches the resource-creating verbs (claim, fork, promote, checkpoint, preview), catalog discovery, and its own sandbox/checkpoint listings; everything it creates is stamped with the tenant name. Root-only surfaces (per-id sandbox reads, `GET /v1/info`, `PUT /v1/pools`, `POST/DELETE /v1/drain`, `/metrics`) answer it 403. `max_claims` (0 = unlimited) caps that tenant's live claims next to the node-wide cap. Requires `api_token` set. Names and tokens must be unique, tokens distinct from `api_token`. On a cluster all nodes must carry the same tenants set (the SDK replays whichever token authorized a redirect), and per-node caps mean a tenant's effective cluster limit is `max_claims` × nodes. Empty = exactly the single-token behavior | | `max_fork_count` | 16 | children a single `fork` may create; each is a full-RAM VM, so this bounds one request's memory blast radius to the node's capacity | | `refill_concurrency` | 0 (auto) | concurrent VM provisioning budget, shared by warm-pool refills, fork clones, and the reap/hibernate/reconcile engine batches. 0 sizes it from the node: `NumCPU*2/3` clamped to [4, 256] — a 384-core node gets 256; small nodes keep a floor of 4 | -| `release_delay_seconds` | 0 | seconds between a release returning and its VM teardown. The claim is dropped and journaled at release; only the `cocoon vm rm`, volume and snapshot cleanup wait, so a burst of releases does not compete with the claims still running. 0 tears down inline | +| `release_delay_seconds` | 0 | seconds a released VM waits in the removal queue before `cocoon vm rm`. The claim is dropped and journaled and its egress listener closed at release; the VM removal, the volume hold release and the egress tap unlock run on the first reap tick (5 s) after the delay, on the `refill_concurrency` budget like every other batch teardown, so a burst of releases does not compete with the claims still running. Until then the VM holds its memory and its volume reservations without counting as a claim. 0 removes inline | | `preview_listen` | (off) | address for a preview HTTP server that serves guest ports under signed URLs; needs `preview_secret` | | `preview_secret` | — | cluster-shared HMAC secret signing preview tokens (all nodes share one) | | `preview_advertise` | = `preview_listen` | the browser-facing preview base URL; nodes behind one TLS proxy may share it, while signed tokens route internally through each owner's `advertise_addr` | diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index 0bc4726b..8ee0d716 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -231,8 +231,8 @@ type Config struct { // RefillConcurrency caps concurrent VM provisioning node-wide; 0 auto-scales with CPUs. RefillConcurrency int `json:"refill_concurrency,omitzero"` - // ReleaseDelaySeconds, when >0, delays a released VM's teardown so it stays out of a claim burst. - ReleaseDelaySeconds int `json:"release_delay_seconds,omitempty"` + // ReleaseDelaySeconds, when >0, parks a released VM in the removal queue for that long instead of removing it inline. + ReleaseDelaySeconds int `json:"release_delay_seconds,omitzero"` // Mesh, when set, joins this node to a memberlist cluster; nil is a mesh of one. Mesh *MeshConfig `json:"mesh,omitempty"` diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index da2582df..efad5129 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -167,30 +167,23 @@ func (m *Manager) releaseResolved(ctx context.Context, id string, sb *types.Sand m.purgeArchiveCk(ctx, id, ck, sb.Tenant) // archived: no local VM m.untrack(m.pendingCks, ck) } - m.counters.releases.Add(1) - m.recordUsage(ctx, usageEvent{Event: "release", ID: id, VMName: vmName}) - if m.releaseDelay > 0 && vmName != "" { - time.AfterFunc(m.releaseDelay, func() { - if err := m.teardown(ctx, id, sb, vmName, snap); err != nil { - log.WithFunc("pool.releaseResolved").Errorf(ctx, err, "delayed teardown of %s", id) - } - }) - return nil - } - return m.teardown(ctx, id, sb, vmName, snap) -} - -// teardown frees what a released claim still holds: volumes, the VM, its egress state and snapshot. -func (m *Manager) teardown(ctx context.Context, id string, sb *types.Sandbox, vmName, snap string) error { td := m.quiesceVolumes(ctx, sb) var err error - if vmName == "" { + removed := vmName == "" + switch { + case removed: m.finishVolumeTeardown(ctx, td) // archived: no VM to confirm gone - } else if !m.removeOrRetry(ctx, vmName, id, "", td) { - err = fmt.Errorf("vm %s survived removal", vmName) + case m.releaseDelay > 0: + m.queueRemoval(vmName, id, "", td, time.Now().Add(m.releaseDelay)) + default: + if removed = m.removeOrRetry(ctx, vmName, id, "", td); !removed { + err = fmt.Errorf("vm %s survived removal", vmName) + } } - m.disarmEgress(id, err == nil) + m.disarmEgress(id, removed) m.dropSnap(ctx, snap) + m.counters.releases.Add(1) + m.recordUsage(ctx, usageEvent{Event: "release", ID: id, VMName: vmName}) return err } diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index a1ca46e6..dd2cb7fa 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -160,6 +160,7 @@ type pendingRemoval struct { tap string staleCreate bool volumes volumeTeardown + notBefore time.Time } type pool struct { diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 1ccd5f9b..f466598d 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -214,7 +214,7 @@ func TestReleaseAfterResolveTearsDownOnce(t *testing.T) { } } -func TestReleaseDelayDefersTeardown(t *testing.T) { +func TestReleaseDelayQueuesTeardown(t *testing.T) { synctest.Test(t, func(t *testing.T) { eng := newFakeEngine() m := newTestManager(t, eng) @@ -227,11 +227,12 @@ func TestReleaseDelayDefersTeardown(t *testing.T) { if _, g := m.Info(); g.Claimed != 0 { t.Fatalf("claimed=%d after release, want 0", g.Claimed) } + m.retryRemovals(t.Context()).Wait() if removed := eng.removedNames(); len(removed) != 0 { t.Fatalf("removes=%v before the delay, want none", removed) } time.Sleep(2 * time.Second) - synctest.Wait() + m.retryRemovals(t.Context()).Wait() if removed := eng.removedNames(); len(removed) != 1 || removed[0] != sb.VMName { t.Fatalf("removes=%v after the delay, want %s", removed, sb.VMName) } diff --git a/sandboxd/pool/remove.go b/sandboxd/pool/remove.go index 117d3402..8404ea56 100644 --- a/sandboxd/pool/remove.go +++ b/sandboxd/pool/remove.go @@ -6,6 +6,7 @@ import ( "maps" "slices" "sync" + "time" "github.com/projecteru2/core/log" @@ -48,13 +49,13 @@ func (m *Manager) removeOrRetry(ctx context.Context, name, sandboxID, tap string m.finishVolumeTeardown(ctx, td) return true } - m.queueRemoval(name, sandboxID, tap, td) + m.queueRemoval(name, sandboxID, tap, td, time.Time{}) return false } -func (m *Manager) queueRemoval(name, sandboxID, tap string, td volumeTeardown) { +func (m *Manager) queueRemoval(name, sandboxID, tap string, td volumeTeardown, notBefore time.Time) { m.mu.Lock() - m.pendingRemovals[name] = pendingRemoval{sandboxID: sandboxID, tap: tap, volumes: td} + m.pendingRemovals[name] = pendingRemoval{sandboxID: sandboxID, tap: tap, volumes: td, notBefore: notBefore} m.mu.Unlock() } @@ -64,16 +65,21 @@ func (m *Manager) queueStaleCreate(name, tap string) { m.mu.Unlock() } -// retryRemovals empties the queue to dispatch, so the next tick cannot double-dispatch. +// retryRemovals moves the due entries out of the queue to dispatch, so the next tick cannot double-dispatch. func (m *Manager) retryRemovals(ctx context.Context) *sync.WaitGroup { + now := time.Now() + batch := map[string]pendingRemoval{} m.mu.Lock() - if len(m.pendingRemovals) == 0 { - m.mu.Unlock() - return new(sync.WaitGroup) + for name, pending := range m.pendingRemovals { + if !now.Before(pending.notBefore) { + batch[name] = pending + delete(m.pendingRemovals, name) + } } - batch := m.pendingRemovals - m.pendingRemovals = map[string]pendingRemoval{} m.mu.Unlock() + if len(batch) == 0 { + return new(sync.WaitGroup) + } names := slices.Collect(maps.Keys(batch)) return m.runBounded(ctx, len(names), func(ctx context.Context, i int) { m.retryRemoval(ctx, names[i], batch[names[i]]) @@ -96,7 +102,7 @@ func (m *Manager) retryRemoval(ctx context.Context, name string, pending pending } } if !m.removeVM(ctx, name) { - m.queueRemoval(name, pending.sandboxID, pending.tap, pending.volumes) + m.queueRemoval(name, pending.sandboxID, pending.tap, pending.volumes, time.Time{}) return } m.finishRemoval(ctx, pending) From 75651d7042a74de6de78f7b4b671c6b5f36afc21 Mon Sep 17 00:00:00 2001 From: CMGS Date: Fri, 11 Sep 2026 13:20:27 +0900 Subject: [PATCH 4/4] review: PR #149 round One warmup runner for the golden build and the clone (the golden keeps the argv for its sidecar stamp), warmClone reads the pool key from the sandbox, the two-line rationale becomes one, and the CA-install error assigns the outer variable instead of shadowing it. releaseDelay moves next to the other node-wide lifecycle scalars, release_delay_seconds takes omitzero like every sibling int, and the non-negative table follows the struct's field order. The fake engine latches warmup-after-snapshot as one flag and the clone test copies the fake's slices before asserting, as the file's other accessors do. --- sandboxd/config/config.go | 12 ++++++------ sandboxd/pool/pool.go | 10 +++++----- sandboxd/pool/pool_test.go | 4 ++-- sandboxd/pool/refill.go | 25 +++++++++++++------------ sandboxd/pool/warmup_test.go | 15 ++++++++------- 5 files changed, 34 insertions(+), 32 deletions(-) diff --git a/sandboxd/config/config.go b/sandboxd/config/config.go index 8ee0d716..7f6f861b 100644 --- a/sandboxd/config/config.go +++ b/sandboxd/config/config.go @@ -300,18 +300,18 @@ func (c *Config) applyDefaults() { } func (c *Config) validate() error { - for _, n := range []struct { + for _, field := range []struct { name string value int }{ - {"release_delay_seconds", c.ReleaseDelaySeconds}, - {"refill_concurrency", c.RefillConcurrency}, - {"max_claims", c.MaxClaims}, {"idle_hibernate_seconds", c.IdleHibernateSeconds}, {"checkpoint_ttl_hours", c.CheckpointTTLHours}, + {"max_claims", c.MaxClaims}, + {"refill_concurrency", c.RefillConcurrency}, + {"release_delay_seconds", c.ReleaseDelaySeconds}, } { - if n.value < 0 { - return fmt.Errorf("%s must not be negative, got %d", n.name, n.value) + if field.value < 0 { + return fmt.Errorf("%s must not be negative, got %d", field.name, field.value) } } if err := c.validateAttachment(); err != nil { diff --git a/sandboxd/pool/pool.go b/sandboxd/pool/pool.go index dd2cb7fa..5d45793b 100644 --- a/sandboxd/pool/pool.go +++ b/sandboxd/pool/pool.go @@ -271,6 +271,7 @@ type Manager struct { // tenantMax doubles as the set of known tenants; a 0 cap means unlimited. maxClaims int + releaseDelay time.Duration draining bool // guarded by m.mu; deliberately not persisted tenantMax map[string]int tenantLive map[string]int @@ -326,10 +327,9 @@ type Manager struct { atCapacityUntil time.Time atCapacityReason string - refillSem chan struct{} - releaseDelay time.Duration - probeSem chan struct{} - refillKick chan struct{} + refillSem chan struct{} + probeSem chan struct{} + refillKick chan struct{} } // NewManager builds a manager from the node config; ctx bounds backend construction. @@ -347,6 +347,7 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg dataDir: cfg.DataDir, egress: cfg.HasEgress(), lockEgress: len(cfg.Bridges) > 0, + releaseDelay: time.Duration(cfg.ReleaseDelaySeconds) * time.Second, maxFork: maxFork, store: newClaimStore(cfg.DataDir), volumes: make(map[string]catalogVolume, len(cfg.Volumes)), @@ -369,7 +370,6 @@ func NewManager(ctx context.Context, cfg *config.Config, eng Engine, secrets *eg dial: newEgressDialer(parsePrefixes(cfg.EgressInternalAllow)).DialContext, sweep: netfilter.SweepExcept, refillSem: make(chan struct{}, refill), - releaseDelay: time.Duration(cfg.ReleaseDelaySeconds) * time.Second, probeSem: make(chan struct{}, refill), refillKick: make(chan struct{}, 1), healSem: make(chan struct{}, maxConcurrentHeals), diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index f466598d..84f01259 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -821,7 +821,7 @@ type fakeEngine struct { caInstalls []string warmups [][]string warmupSocks []string - warmupSnaps []int + warmupAfterSnap bool warmupErr error staleReconciles []string installCAErr error @@ -1064,7 +1064,7 @@ func (f *fakeEngine) Warmup(_ context.Context, sock string, argv []string) error defer f.mu.Unlock() f.warmups = append(f.warmups, argv) f.warmupSocks = append(f.warmupSocks, sock) - f.warmupSnaps = append(f.warmupSnaps, len(f.snapSaves)) + f.warmupAfterSnap = f.warmupAfterSnap || len(f.snapSaves) > 0 return f.warmupErr } diff --git a/sandboxd/pool/refill.go b/sandboxd/pool/refill.go index a0908359..e84b0a03 100644 --- a/sandboxd/pool/refill.go +++ b/sandboxd/pool/refill.go @@ -91,7 +91,7 @@ func (m *Manager) refillOne(ctx context.Context, p *pool, golden string) { sb, err = m.readyBounded(ctx, sb, time.Now().Add(warmProbeTimeout)) } if err == nil { - err = m.warmClone(ctx, p.key, sb) + err = m.warmClone(ctx, sb) } keep := false var fails int @@ -193,15 +193,13 @@ func (m *Manager) buildGoldenSteps(ctx context.Context, key types.PoolKey, name, } caBaked := m.poolIntercepts(key) if caBaked { - if err := m.eng.InstallCACert(ctx, sock, m.egressCA.CertPEM()); err != nil { + if err = m.eng.InstallCACert(ctx, sock, m.egressCA.CertPEM()); err != nil { return fmt.Errorf("install egress ca: %w", err) } } - warmup := m.poolWarmup(key) - if len(warmup) > 0 { - if err := m.eng.Warmup(ctx, sock, warmup); err != nil { - return fmt.Errorf("warmup: %w", err) - } + warmup, err := m.runWarmup(ctx, key, sock) + if err != nil { + return fmt.Errorf("warmup: %w", err) } if err := m.eng.SnapshotSave(ctx, name, snap); err != nil { return err @@ -221,14 +219,17 @@ func (m *Manager) poolWarmup(key types.PoolKey) []string { return m.poolWarmups[key] } -// warmClone re-runs the pool warmup in a restored clone: a restore maps the golden memory lazily, -// so the pages the warmup touched fault in here, at refill, instead of under the first claim. -func (m *Manager) warmClone(ctx context.Context, key types.PoolKey, sb *types.Sandbox) error { +func (m *Manager) runWarmup(ctx context.Context, key types.PoolKey, sock string) ([]string, error) { warmup := m.poolWarmup(key) if len(warmup) == 0 { - return nil + return nil, nil } - if err := m.eng.Warmup(ctx, sb.VsockSocket, warmup); err != nil { + return warmup, m.eng.Warmup(ctx, sock, warmup) +} + +// a restore maps the golden memory lazily, so the warmup's pages fault in here instead of under the first claim +func (m *Manager) warmClone(ctx context.Context, sb *types.Sandbox) error { + if _, err := m.runWarmup(ctx, sb.Key, sb.VsockSocket); err != nil { m.destroy(ctx, sb.VMName) return fmt.Errorf("clone warmup: %w", err) } diff --git a/sandboxd/pool/warmup_test.go b/sandboxd/pool/warmup_test.go index 68db93ed..0054ec39 100644 --- a/sandboxd/pool/warmup_test.go +++ b/sandboxd/pool/warmup_test.go @@ -23,8 +23,8 @@ func TestGoldenBuildRunsWarmupBeforeSnapshot(t *testing.T) { if len(eng.warmups) != 1 || !slices.Equal(eng.warmups[0], argv) { t.Fatalf("warmups = %v, want [%v]", eng.warmups, argv) } - if eng.warmupSnaps[0] != 0 { - t.Fatalf("golden warmup ran after %d snapshot saves, want 0", eng.warmupSnaps[0]) + if eng.warmupAfterSnap { + t.Fatal("golden warmup ran after the snapshot save") } stamp, err := os.ReadFile(final + warmupSidecarSuffix) if err != nil { @@ -105,12 +105,13 @@ func TestRefillWarmsEveryClone(t *testing.T) { return infos[0].Warm == 2 && infos[0].Refilling == 0 }) eng.mu.Lock() - defer eng.mu.Unlock() - if len(eng.warmups) != 2 || !slices.Equal(eng.warmups[0], argv) || !slices.Equal(eng.warmups[1], argv) { - t.Fatalf("warmups = %v, want %v once per clone", eng.warmups, argv) + warmups, socks := slices.Clone(eng.warmups), slices.Clone(eng.warmupSocks) + eng.mu.Unlock() + if len(warmups) != 2 || !slices.Equal(warmups[0], argv) || !slices.Equal(warmups[1], argv) { + t.Fatalf("warmups = %v, want %v once per clone", warmups, argv) } - if eng.warmupSocks[0] == "" || eng.warmupSocks[0] == eng.warmupSocks[1] { - t.Errorf("warmup sockets = %v, want one per clone", eng.warmupSocks) + if socks[0] == "" || socks[0] == socks[1] { + t.Errorf("warmup sockets = %v, want one per clone", socks) } }) }