From 06cb400cd58f8d48aa0ebbb79a62d0670ae31a29 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Sun, 25 Oct 2026 15:38:55 +0000 Subject: [PATCH 01/15] pool: retain checkpoints through failed claim transitions Retain current, acknowledged and attempted checkpoint references until claim writes settle. Queue cleanup after partial failures and keep expiry progressing without an archive registry. --- sandboxd/pool/archive.go | 21 +- sandboxd/pool/archive_test.go | 13 +- sandboxd/pool/checkpoint.go | 18 +- sandboxd/pool/checkpoint_bench_test.go | 66 ++++ sandboxd/pool/checkpoint_cleanup_test.go | 359 ++++++++++++++++++++ sandboxd/pool/checkpoint_references_test.go | 287 ++++++++++++++++ sandboxd/pool/claim.go | 4 +- sandboxd/pool/claims.go | 188 +++++++++- sandboxd/pool/claims_test.go | 27 +- sandboxd/pool/failover.go | 8 +- sandboxd/pool/filesystem_fault_test.go | 40 +++ sandboxd/pool/hibernate_test.go | 8 +- sandboxd/pool/reconcile.go | 4 +- sandboxd/pool/replica.go | 38 ++- 14 files changed, 1018 insertions(+), 63 deletions(-) create mode 100644 sandboxd/pool/checkpoint_bench_test.go create mode 100644 sandboxd/pool/checkpoint_cleanup_test.go create mode 100644 sandboxd/pool/checkpoint_references_test.go create mode 100644 sandboxd/pool/filesystem_fault_test.go diff --git a/sandboxd/pool/archive.go b/sandboxd/pool/archive.go index af26aa17..529f677c 100644 --- a/sandboxd/pool/archive.go +++ b/sandboxd/pool/archive.go @@ -280,7 +280,7 @@ func (m *Manager) commitWake(ctx context.Context, sb *types.Sandbox, vmName, soc } func (m *Manager) deleteOrphanArchiveCk(ctx context.Context, ckID string) { - if err := m.deleteArchiveCk(ctx, ckID); err != nil { + if _, err := m.deleteArchiveCk(ctx, ckID); err != nil { log.WithFunc("pool.deleteOrphanArchiveCk").Warnf(ctx, "delete orphaned archive ck %s: %v", ckID, err) } } @@ -351,17 +351,24 @@ func (m *Manager) clearArchiveCk(ckID string) error { return err } -func (m *Manager) deleteArchiveCk(ctx context.Context, ckID string) error { +func (m *Manager) deleteArchiveCk(ctx context.Context, ckID string) (bool, error) { if err := m.markArchiveCk(ckID); err != nil { - return fmt.Errorf("track: %w", err) + return false, fmt.Errorf("track: %w", err) } - if err := m.deleteCkLocked(ctx, ckID); err != nil { - return err + unlock := m.lockRec(ckID) + if m.store.referencesCheckpoint(ckID) { + unlock() + return false, nil + } + err := m.ckpts.Delete(ctx, ckID) + unlock() + if err != nil { + return false, err } if err := m.clearArchiveCk(ckID); err != nil { - return fmt.Errorf("clear: %w", err) + return false, fmt.Errorf("clear: %w", err) } - return nil + return true, nil } func (m *Manager) retryArchiveDeletes(ctx context.Context) { diff --git a/sandboxd/pool/archive_test.go b/sandboxd/pool/archive_test.go index c9ffeee9..468724f9 100644 --- a/sandboxd/pool/archive_test.go +++ b/sandboxd/pool/archive_test.go @@ -680,11 +680,14 @@ func TestArchiveDeleteRetryRechecksWakeRollback(t *testing.T) { m.retryArchiveDeletes(t.Context()) close(retried) }() - waitFor(t, func() bool { - m.recLocksMu.Lock() - defer m.recLocksMu.Unlock() - return m.recRefs[ck] >= 2 - }) + select { + case <-retried: + case <-time.After(3 * time.Second): + t.Fatal("retry did not defer the checkpoint still referenced by the journal") + } + if !ckExists(t, m, ck) { + t.Fatal("retry deleted the checkpoint before wake persistence completed") + } breakStore(t, m) m.store.writeMu.Unlock() locked = false diff --git a/sandboxd/pool/checkpoint.go b/sandboxd/pool/checkpoint.go index e02b70b2..5bc0ce39 100644 --- a/sandboxd/pool/checkpoint.go +++ b/sandboxd/pool/checkpoint.go @@ -5,7 +5,6 @@ import ( "encoding/json/v2" "errors" "fmt" - "maps" "os" "path/filepath" "slices" @@ -318,16 +317,15 @@ func (m *Manager) vetoIfHealPending(ckptID string) { func (m *Manager) pinnedArchiveCks() map[string]struct{} { m.mu.Lock() defer m.mu.Unlock() - pinned := maps.Clone(m.pendingCks) + pinned := m.store.checkpointReferences() + for id := range m.pendingCks { + pinned[id] = struct{}{} + } for _, sb := range m.claimed { - if sb.ArchiveCk != "" { - pinned[sb.ArchiveCk] = struct{}{} - } - if sb.ReplicaCk != "" { - pinned[sb.ReplicaCk] = struct{}{} - } - if sb.ReplicaPending != "" { - pinned[sb.ReplicaPending] = struct{}{} + for _, id := range [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} { + if id != "" { + pinned[id] = struct{}{} + } } } return pinned diff --git a/sandboxd/pool/checkpoint_bench_test.go b/sandboxd/pool/checkpoint_bench_test.go new file mode 100644 index 00000000..5bc2c26e --- /dev/null +++ b/sandboxd/pool/checkpoint_bench_test.go @@ -0,0 +1,66 @@ +package pool + +import ( + "fmt" + "testing" + "time" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func BenchmarkClaimCommitWithCheckpoints(b *testing.B) { + for _, checkpoints := range []bool{false, true} { + b.Run(fmt.Sprintf("claims=5000/checkpoints=%t", checkpoints), func(b *testing.B) { + claims := checkpointBenchClaims(checkpoints) + s := newClaimStore(b.TempDir(), false) + if err := s.save(claims); err != nil { + b.Fatal(err) + } + one := claims["sb_0000000000000000"] + b.ReportAllocs() + for b.Loop() { + one.Deadline = one.Deadline.Add(time.Second) + if err := s.commit(s.set(one)); err != nil { + b.Fatal(err) + } + } + loaded, err := s.load() + if err != nil || len(loaded) != 5000 || !loaded[one.ID].Deadline.Equal(one.Deadline) || loaded[one.ID].ReplicaCk != one.ReplicaCk { + b.Fatalf("commit output: rows=%d err=%v", len(loaded), err) + } + }) + } +} + +func BenchmarkPinnedArchiveCheckpoints(b *testing.B) { + for _, checkpoints := range []bool{false, true} { + b.Run(fmt.Sprintf("claims=5000/checkpoints=%t", checkpoints), func(b *testing.B) { + m := newTestManager(b, newFakeEngine(), config.PoolSpec{PoolKey: testKey}) + m.claimed = checkpointBenchClaims(checkpoints) + if err := m.store.save(m.claimed); err != nil { + b.Fatal(err) + } + want := 0 + if checkpoints { + want = 5000 + } + b.ReportAllocs() + for b.Loop() { + if got := len(m.pinnedArchiveCks()); got != want { + b.Fatalf("pinned=%d want=%d", got, want) + } + } + }) + } +} + +func checkpointBenchClaims(checkpoints bool) map[string]*types.Sandbox { + claims := benchClaims(5000) + if checkpoints { + for id, sb := range claims { + sb.ReplicaCk = "ck_" + id[3:] + } + } + return claims +} diff --git a/sandboxd/pool/checkpoint_cleanup_test.go b/sandboxd/pool/checkpoint_cleanup_test.go new file mode 100644 index 00000000..cf5b5c8c --- /dev/null +++ b/sandboxd/pool/checkpoint_cleanup_test.go @@ -0,0 +1,359 @@ +package pool + +import ( + "errors" + "fmt" + "testing" + "time" + + "github.com/cocoonstack/sandbox/sandboxd/config" + "github.com/cocoonstack/sandbox/sandboxd/store" + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +var strandedStates = []strandedState{ + {name: "published replica", setup: func(t *testing.T, m *Manager, _ *failoverCluster, sb *types.Sandbox) string { + replicateNow(t, m, sb) + return sb.ReplicaCk + }}, + {name: "pending replica", setup: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) + } + return sb.ReplicaPending + }}, + {name: "archive intent", setup: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { + if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.archive(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) + } + return sb.ArchivePending + }}, +} + +func TestReplicaRetirementKeepsTombstoneUntilClaimCommit(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + replicateNow(t, m, sb) + ck := sb.ReplicaCk + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + sb.Transition.Lock() + err := m.dropReplica(t.Context(), sb) + sb.Transition.Unlock() + if err == nil { + t.Error("retirement acknowledged a failed claim commit") + } + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil || !ok || !rec.Consumed || !ckExists(t, m, ck) { + t.Fatalf("failed commit lost recovery tombstone/checkpoint: %+v, exists=%t, err=%v", rec, ok, err) + } + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if rec, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok || !rec.Consumed { + t.Fatalf("sweep erased unacknowledged retirement: %+v, exists=%t, err=%v", rec, ok, err) + } + restoreWrites() + restarted := c.restart(t, m) + if err = restarted.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + restored, ok := restarted.byID(sb.ID) + if !ok { + t.Fatal("restart dropped a claim after failed retirement commit") + } + restarted.retireReplica(t.Context(), restored) + if restored.HasReplica() { + t.Fatal("restart did not finish retirement") + } +} + +func TestReplicaUnpublishedIntentSurvivesFailedClearAndRestart(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) + } + pending := sb.ReplicaPending + if pending == "" || !ckExists(t, m, pending) { + t.Fatal("missing unpublished checkpoint") + } + m.archives = c.reg + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + m.retireReplica(t.Context(), sb) + persisted, err := newClaimStore(m.dataDir, false).load() + if err != nil || persisted[sb.ID].ReplicaPending != pending { + t.Fatalf("failure did not retain old disk state: %+v, %v", persisted, err) + } + if !ckExists(t, m, pending) { + t.Error("failed pending clear deleted the checkpoint used to distinguish an unpublished intent from adoption") + } + t.Logf("failed unpublished clear: memory_pending=%q durable_pending=%q checkpoint_exists=%t synced=%t", sb.ReplicaPending, persisted[sb.ID].ReplicaPending, ckExists(t, m, pending), m.store.synced()) + restoreWrites() + restarted := c.restart(t, m) + if err := restarted.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + restored, ok := restarted.byID(sb.ID) + if !ok { + t.Fatal("restart mistook a failed pending clear for adoption and discarded the claim") + } + restarted.retireReplica(t.Context(), restored) + if restored.HasReplica() { + t.Fatal("restart failed to retire unpublished replica") + } +} + +func TestReplicaPendingCheckpointCleanupWaitsForAcknowledgment(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) + } + pending := sb.ReplicaPending + m.archives = c.reg + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + m.retireReplica(t.Context(), sb) + if sb.ReplicaPending != "" || m.store.synced() { + t.Fatal("expected memory clear before snapshot acknowledgment") + } + if deleted, err := m.deleteArchiveCk(t.Context(), pending); err != nil || deleted { + t.Fatalf("orphan delete ignored unacknowledged claim: deleted=%t, %v", deleted, err) + } + m.retryArchiveDeletes(t.Context()) + if !ckExists(t, m, pending) || !m.deleteMarked(pending) { + t.Fatal("cleanup discarded pending checkpoint or retry marker") + } + t.Logf("cleanup deferred: checkpoint=%s exists=%t marker=%t synced=%t", pending, ckExists(t, m, pending), m.deleteMarked(pending), m.store.synced()) + restoreWrites() + if err := m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + other := mustClaim(t, m, testKey) + m.mu.Lock() + other.LeaseSeconds++ + m.store.set(other) + m.mu.Unlock() + m.retryArchiveDeletes(t.Context()) + if ckExists(t, m, pending) || m.deleteMarked(pending) { + t.Fatal("acknowledged cleanup leaked checkpoint or retry marker") + } + t.Logf("cleanup acknowledged: exists=%t marker=%t synced=%t", ckExists(t, m, pending), m.deleteMarked(pending), m.store.synced()) +} + +func TestReplicaSettledCheckpointCleanupRetriesAfterFailedCommit(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + replicateNow(t, m, sb) + previous := sb.ReplicaCk + m.archives = &hookRegistry{Registry: c.reg, losePutReply: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) + } + pending := sb.ReplicaPending + m.archives = c.reg + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + sb.Transition.Lock() + err := m.settleReplicaIntent(t.Context(), sb) + sb.Transition.Unlock() + if err == nil || sb.ReplicaPending != "" || sb.ReplicaCk != pending || m.store.synced() { + t.Fatalf("settle state: %+v, %v", sb, err) + } + if !ckExists(t, m, previous) || !m.deleteMarked(previous) { + t.Fatal("failed settle lost old checkpoint or its queued cleanup") + } + m.retryArchiveDeletes(t.Context()) + if !ckExists(t, m, previous) { + t.Fatal("old checkpoint deleted before clear acknowledgment") + } + restoreWrites() + if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + m.retryArchiveDeletes(t.Context()) + if ckExists(t, m, previous) || m.deleteMarked(previous) || !ckExists(t, m, pending) { + t.Fatal("acknowledged settle failed to clean old checkpoint or removed current checkpoint") + } +} + +func TestReplicaConsumedRowPurgeWaitsForSnapshot(t *testing.T) { + for _, claimed := range []bool{false, true} { + t.Run(fmt.Sprintf("claimed=%t", claimed), func(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + replicateNow(t, m, sb) + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + m.retireReplica(t.Context(), sb) + if !claimed { + m.mu.Lock() + delete(m.claimed, sb.ID) + m.store.del(sb.ID) + m.mu.Unlock() + } + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil || !ok || !rec.Consumed { + t.Fatalf("setup: %+v, %t, %v", rec, ok, err) + } + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok { + t.Fatalf("sweep dropped unacknowledged row: %t, %v", ok, err) + } + restoreWrites() + if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || ok { + t.Fatalf("sweep retained acknowledged row: %t, %v", ok, err) + } + }) + } +} + +func TestArchiveCheckpointCleanupIgnoresUnrelatedClaimWrites(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + checkpoint, _, err := m.publishCheckpoint(t.Context(), sb, store.CheckpointID(randHex(8)), "orphan", "", true) + if err != nil { + t.Fatal(err) + } + ck := checkpoint.ID + other := mustClaim(t, m, testKey) + m.mu.Lock() + other.LeaseSeconds++ + m.store.set(other) + m.mu.Unlock() + if m.store.synced() { + t.Fatal("expected an unrelated unacknowledged claim write") + } + if deleted, err := m.deleteArchiveCk(t.Context(), ck); err != nil || !deleted { + t.Fatalf("unrelated claim delayed checkpoint cleanup: deleted=%t, %v", deleted, err) + } + if ckExists(t, m, ck) || m.deleteMarked(ck) { + t.Fatal("unreferenced checkpoint or delete marker remained") + } +} + +func TestFailoverDisabledExitKeepsReplicaCheckpoint(t *testing.T) { + exits := []struct { + name string + run func(t *testing.T, m *Manager, sb *types.Sandbox) + }{ + {"release", func(t *testing.T, m *Manager, sb *types.Sandbox) { + if err := m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + }}, + {"expire", func(t *testing.T, m *Manager, sb *types.Sandbox) { + m.mu.Lock() + sb.Deadline = time.Now().Add(-time.Minute) + m.mu.Unlock() + m.reapOnce(t.Context()) + waitFor(t, func() bool { return m.Counters().Reaps == 1 }) + }}, + } + for _, state := range strandedStates { + for _, exit := range exits { + t.Run(state.name+"/"+exit.name, func(t *testing.T) { + m, sb, ck := restartStranded(t, state) + exit.run(t, m, sb) + if _, held := m.byID(sb.ID); held || !ckExists(t, m, ck) { + t.Fatalf("%s without a registry: held=%t checkpoint kept=%t", exit.name, held, ckExists(t, m, ck)) + } + }) + } + } +} + +func TestReplicaFailedClearQueuesCheckpointCleanup(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + replicateNow(t, m, sb) + ck := sb.ReplicaCk + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + sb.Transition.Lock() + err := m.dropReplica(t.Context(), sb) + sb.Transition.Unlock() + if err == nil || sb.HasReplica() { + t.Fatalf("expected an unacknowledged memory clear: replica=%t, %v", sb.HasReplica(), err) + } + if !m.deleteMarked(ck) { + t.Error("failed replica clear did not queue its checkpoint for cleanup") + } + m.retryArchiveDeletes(t.Context()) + if !ckExists(t, m, ck) { + t.Fatal("queued cleanup deleted a checkpoint still referenced by claims.json") + } + restoreWrites() + if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + m.retryArchiveDeletes(t.Context()) + if ckExists(t, m, ck) || m.deleteMarked(ck) { + t.Fatal("checkpoint cleanup did not finish after its reference retired") + } +} + +type strandedState struct { + name string + setup func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string +} + +func restartStranded(t *testing.T, state strandedState) (*Manager, *types.Sandbox, string) { + t.Helper() + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + ck := state.setup(t, m, c, sb) + if ck == "" { + t.Fatal("fixture left no failover checkpoint") + } + restarted := newManagerWithoutRegistry(t, m, c) + if err := restarted.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + restored, ok := restarted.byID(sb.ID) + if !ok { + t.Fatal("startup without a registry lost the claim") + } + if restored.ReplicaCk != sb.ReplicaCk || restored.ReplicaPending != sb.ReplicaPending || restored.ArchivePending != sb.ArchivePending { + t.Fatalf("startup without a registry changed failover fields: replica=%q pending=%q archive=%q", restored.ReplicaCk, restored.ReplicaPending, restored.ArchivePending) + } + if !ckExists(t, restarted, ck) || restarted.deleteMarked(ck) { + t.Fatalf("startup without a registry queued or deleted checkpoint %s", ck) + } + return restarted, restored, ck +} + +func newManagerWithoutRegistry(t *testing.T, old *Manager, c *failoverCluster) *Manager { + t.Helper() + cfg := &config.Config{ + APIToken: "root", DataDir: old.dataDir, CheckpointDir: c.ckDir, + Pools: []config.PoolSpec{{PoolKey: testKey}}, + } + m, err := NewManager(t.Context(), cfg, old.eng, testSecrets(t), testBackends(t, cfg)) + if err != nil { + t.Fatal(err) + } + if m.archives != nil { + t.Fatal("failover-disabled fixture unexpectedly has a registry") + } + return m +} diff --git a/sandboxd/pool/checkpoint_references_test.go b/sandboxd/pool/checkpoint_references_test.go new file mode 100644 index 00000000..edcd5152 --- /dev/null +++ b/sandboxd/pool/checkpoint_references_test.go @@ -0,0 +1,287 @@ +package pool + +import ( + "encoding/json/v2" + "errors" + "fmt" + "io" + "io/fs" + "os" + "path/filepath" + "sync" + "syscall" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestClaimCheckpointReferencesTrackPossibleSnapshots(t *testing.T) { + if withoutDACOverride(t) { + return + } + dir := t.TempDir() + s := newClaimStore(dir, true) + sb := &types.Sandbox{ID: "owner", ArchiveCk: "archive", ArchivePending: "archive-pending", ReplicaCk: "replica", ReplicaPending: "replica-pending", FromCheckpoint: "branch-origin"} + snap := s.set(sb) + if refs := s.checkpointReferences(); len(refs) != 4 { + t.Fatalf("current references: %+v", refs) + } + if err := s.commit(snap); err != nil { + t.Fatal(err) + } + sb.ReplicaPending = "uncertain-replica" + if err := os.Chmod(dir, 0o300); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) + err := s.commit(s.set(sb)) + if !errors.Is(err, fs.ErrPermission) { + t.Fatalf("expected post-rename permission failure: %v", err) + } + if err = os.Chmod(dir, 0o700); err != nil { + t.Fatal(err) + } + loaded, err := newClaimStore(dir, false).load() + if err != nil || loaded[sb.ID].ReplicaPending != "uncertain-replica" { + t.Fatalf("post-rename file: %+v, %v", loaded, err) + } + sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending = "", "", "", "" + s.set(sb) + refs := s.checkpointReferences() + if len(refs) != 5 { + t.Fatalf("lost snapshot references: %+v", refs) + } + delete(refs, "uncertain-replica") + if !s.referencesCheckpoint("uncertain-replica") { + t.Fatal("caller mutated retained reference set") + } + s.set(&types.Sandbox{ID: "other", ReplicaPending: "other-replica"}) + if s.hasCheckpointReferences("unknown") || !s.hasCheckpointReferences("other") { + t.Fatal("checkpoint references crossed ownership") + } + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if s.hasCheckpointReferences(sb.ID) || len(s.checkpointReferences()) != 1 { + t.Fatal("acknowledgment did not retire only removed references") + } +} + +func TestClaimCapturedCheckpointReferenceSurvivesConcurrentClear(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", ReplicaPending: "captured-replica"} + release := captureClaimWrite(t, s, s.set(sb), sb.ID) + sb.ReplicaPending = "" + s.set(sb) + if !s.referencesCheckpoint("captured-replica") { + t.Fatal("captured reference disappeared before acknowledgment") + } + var rows map[string]*types.Sandbox + if err := json.Unmarshal(release(), &rows); err != nil { + t.Fatal(err) + } + if rows[sb.ID].ReplicaPending != "captured-replica" { + t.Fatal("captured write lost its original checkpoint") + } + if !s.referencesCheckpoint("captured-replica") { + t.Fatal("acknowledgment lost captured checkpoint") + } + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if s.hasCheckpointReferences(sb.ID) { + t.Fatal("later acknowledgment retained obsolete checkpoint") + } +} + +func TestClaimCheckpointReferencesSurviveFailedStartupRewrite(t *testing.T) { + for _, omit := range []bool{false, true} { + t.Run(fmt.Sprint(omit), func(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", ReplicaPending: "before-recovery"} + if err := s.commit(s.set(sb)); err != nil { + t.Fatal(err) + } + s = newClaimStore(filepath.Dir(s.path), false) + claims, err := s.load() + if err != nil { + t.Fatal(err) + } + if omit { + delete(claims, sb.ID) + } else { + claims[sb.ID].ReplicaPending = "" + } + restore := failClaimWrites(t, s) + if err = s.save(claims); err == nil { + t.Fatal("startup rewrite unexpectedly acknowledged") + } + if !s.referencesCheckpoint("before-recovery") { + t.Fatal("failed startup rewrite retired loaded checkpoint") + } + restore() + if err = s.save(claims); err != nil { + t.Fatal(err) + } + if s.hasCheckpointReferences(sb.ID) || s.referencesCheckpoint("before-recovery") { + t.Fatal("successful startup rewrite retained old checkpoint") + } + }) + } +} + +func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + first := &types.Sandbox{ID: "first", ReplicaCk: "shared", ArchivePending: "shared"} + second := &types.Sandbox{ID: "second", ArchiveCk: "shared"} + if err := s.commit(s.set(first, second)); err != nil { + t.Fatal(err) + } + s.del(first.ID) + if !s.referencesCheckpoint("shared") { + t.Fatal("memory deletion lost shared checkpoint") + } + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if !s.referencesCheckpoint("shared") || s.hasCheckpointReferences(first.ID) || !s.hasCheckpointReferences(second.ID) { + t.Fatal("one owner's retirement changed another owner's reference") + } + if err := s.commit(s.del(second.ID)); err != nil { + t.Fatal(err) + } + if s.referencesCheckpoint("shared") || len(s.checkpointReferences()) != 0 { + t.Fatal("last owner left shared checkpoint pinned") + } +} + +func TestClaimReferenceChangesStayDirtyAfterFailedReturnToOriginal(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", ReplicaCk: "original"} + if err := s.commit(s.set(sb)); err != nil { + t.Fatal(err) + } + restore := failClaimWrites(t, s) + sb.ReplicaCk = "attempted" + if err := s.commit(s.set(sb)); err == nil { + t.Fatal("expected write failure") + } + sb.ReplicaCk = "original" + s.set(sb) + if !s.referencesCheckpoint("original") || !s.referencesCheckpoint("attempted") { + t.Fatal("failed return lost a possible snapshot") + } + restore() + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if !s.referencesCheckpoint("original") || s.referencesCheckpoint("attempted") { + t.Fatal("successful return did not retire failed attempt") + } +} + +func TestClaimCapturedResetAndRecreatedOwner(t *testing.T) { + for _, reset := range []bool{false, true} { + t.Run(fmt.Sprint(reset), func(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", ReplicaCk: "captured"} + release := captureClaimWrite(t, s, s.set(sb), sb.ID) + if reset { + s.reset(map[string]*types.Sandbox{}) + } else { + s.del(sb.ID) + } + s.set(&types.Sandbox{ID: sb.ID, ReplicaCk: "recreated"}) + var captured map[string]*types.Sandbox + if err := json.Unmarshal(release(), &captured); err != nil { + t.Fatal(err) + } + if captured[sb.ID].ReplicaCk != "captured" { + t.Fatal("in-flight snapshot changed") + } + if !s.referencesCheckpoint("captured") || !s.referencesCheckpoint("recreated") { + t.Fatal("acknowledgment lost captured or newer reference") + } + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if s.referencesCheckpoint("captured") || !s.referencesCheckpoint("recreated") { + t.Fatal("new snapshot did not retire old owner references") + } + }) + } +} + +func TestClaimReferencesSurviveRenameFailure(t *testing.T) { + s := newClaimStore(t.TempDir(), false) + sb := &types.Sandbox{ID: "owner", ReplicaCk: "captured"} + if err := os.Mkdir(s.path, 0o700); err != nil { + t.Fatal(err) + } + if err := s.commit(s.set(sb)); err == nil { + t.Fatal("expected rename failure") + } + s.del(sb.ID) + if !s.referencesCheckpoint("captured") { + t.Fatal("failed rename released attempted checkpoint") + } + if err := os.Remove(s.path); err != nil { + t.Fatal(err) + } + if err := s.commit(s.mark()); err != nil { + t.Fatal(err) + } + if s.referencesCheckpoint("captured") { + t.Fatal("successful rewrite did not retire failed snapshot") + } +} + +func failClaimWrites(t *testing.T, s *claimStore) func() { + t.Helper() + tmp := s.path + ".tmp" + if err := os.Mkdir(tmp, 0o700); err != nil { + t.Fatal(err) + } + restore := sync.OnceFunc(func() { + if err := os.Remove(tmp); err != nil { + t.Error(err) + } + }) + t.Cleanup(restore) + return restore +} + +func captureClaimWrite(t *testing.T, s *claimStore, snap claimSnapshot, owner string) func() []byte { + t.Helper() + fifo := s.path + ".tmp" + if err := syscall.Mkfifo(fifo, 0o600); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- s.commit(snap) }() + waitFor(t, func() bool { + s.mu.Lock() + defer s.mu.Unlock() + return !s.retained[owner].attempted.empty() + }) + var raw []byte + release := sync.OnceFunc(func() { + r, err := os.Open(fifo) + if err != nil { + t.Fatal(err) + } + raw, err = io.ReadAll(r) + _ = r.Close() + if err != nil { + t.Fatal(err) + } + if err := <-done; err != nil { + t.Fatal(err) + } + if err := os.Remove(s.path); err != nil { + t.Fatal(err) + } + }) + t.Cleanup(release) + return func() []byte { release(); return raw } +} diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index 683074ee..adc786ea 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -696,10 +696,8 @@ func (m *Manager) pauseExpired(ctx context.Context, sb *types.Sandbox) error { func (m *Manager) purgeArchiveCk(ctx context.Context, id, ck, tenant string) { logger := log.WithFunc("pool.purgeArchiveCk") - if err := m.deleteCkLocked(ctx, ck); err != nil { + if _, err := m.deleteArchiveCk(ctx, ck); err != nil { logger.Warnf(ctx, "delete archive ck %s: %v", ck, err) - } else if err := m.clearArchiveCk(ck); err != nil { - logger.Warnf(ctx, "clear archive ck %s: %v", ck, err) } m.counters.archiveDeletes.Add(1) m.recordUsage(ctx, usageEvent{Event: "archive_delete", ID: id, Reference: ck, Tenant: tenant}) diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index 5d7111df..2043cb05 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -68,8 +68,41 @@ type claimDTO struct { // claimRow keeps its encoded member until set replaces the row; set never edits one. type claimRow struct { - dto claimDTO - raw []byte + id string + refs *claimRefs + dto claimDTO + raw []byte +} + +type claimRefs struct { + checkpoints []string +} + +func (r claimRefs) empty() bool { return len(r.checkpoints) == 0 } + +func (r claimRefs) matches(sb *types.Sandbox) bool { + checkpoints := [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} + for _, ck := range checkpoints { + if ck != "" && !slices.Contains(r.checkpoints, ck) { + return false + } + } + for _, ck := range r.checkpoints { + if !slices.Contains(checkpoints[:], ck) { + return false + } + } + return true +} + +type retainedClaimRefs struct { + acknowledged claimRefs + attempted claimRefs +} + +type referenceSnapshot struct { + id string + refs claimRefs } // claimStore persists claimed sandboxes across daemon restarts; warm VMs are not persisted. @@ -81,19 +114,28 @@ type claimStore struct { writeMu sync.Mutex buf []byte - mu sync.Mutex - rows map[string]*claimRow - seq uint64 - written uint64 + mu sync.Mutex + rows map[string]*claimRow + seq uint64 + written uint64 + retained map[string]retainedClaimRefs + changed map[string]struct{} + checkpoints map[string]int recommitting atomic.Bool } func newClaimStore(dataDir string, sync bool) *claimStore { - return &claimStore{path: filepath.Join(dataDir, "claims.json"), sync: sync, rows: map[string]*claimRow{}} + return &claimStore{ + path: filepath.Join(dataDir, "claims.json"), sync: sync, + rows: map[string]*claimRow{}, retained: map[string]retainedClaimRefs{}, + changed: map[string]struct{}{}, checkpoints: map[string]int{}, + } } func (s *claimStore) load() (map[string]*types.Sandbox, error) { + s.writeMu.Lock() + defer s.writeMu.Unlock() raw, err := os.ReadFile(s.path) if errors.Is(err, fs.ErrNotExist) { return map[string]*types.Sandbox{}, nil @@ -110,6 +152,20 @@ func (s *claimStore) load() (map[string]*types.Sandbox, error) { sb.VolumesWant = sb.Volumes } } + s.mu.Lock() + for id, retained := range s.retained { + retained.acknowledged = claimRefs{} + s.changeRetainedRefs(id, retained) + s.changed[id] = struct{}{} + } + for _, sb := range claims { + row := newClaimRow(sb, nil) + retained := s.retained[row.id] + retained.acknowledged = *row.refs + s.changeRetainedRefs(row.id, retained) + s.changed[row.id] = struct{}{} + } + s.mu.Unlock() return claims, nil } @@ -118,7 +174,9 @@ func (s *claimStore) set(sbs ...*types.Sandbox) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, sb := range sbs { - s.rows[sb.ID] = &claimRow{dto: dtoOf(sb)} + row := newClaimRow(sb, s.rows[sb.ID]) + s.changeCurrentRefs(sb.ID, *row.refs) + s.rows[sb.ID] = row } s.seq++ return claimSnapshot{seq: s.seq} @@ -129,6 +187,7 @@ func (s *claimStore) del(ids ...string) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, id := range ids { + s.changeCurrentRefs(id, claimRefs{}) delete(s.rows, id) } s.seq++ @@ -152,9 +211,19 @@ func (s *claimStore) pending(snap claimSnapshot) bool { func (s *claimStore) reset(claims map[string]*types.Sandbox) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() - s.rows = make(map[string]*claimRow, len(claims)) + for id := range s.retained { + s.changed[id] = struct{}{} + } + for id := range s.rows { + if _, ok := claims[id]; !ok { + s.changeCurrentRefs(id, claimRefs{}) + delete(s.rows, id) + } + } for id, sb := range claims { - s.rows[id] = &claimRow{dto: dtoOf(sb)} + row := newClaimRow(sb, s.rows[id]) + s.changeCurrentRefs(id, *row.refs) + s.rows[id] = row } s.seq++ return claimSnapshot{seq: s.seq} @@ -170,6 +239,18 @@ func (s *claimStore) commit(snap claimSnapshot) error { return nil } rows, seq := slices.AppendSeq(make([]*claimRow, 0, len(s.rows)), maps.Values(s.rows)), s.seq + captured := make([]referenceSnapshot, 0, len(s.changed)) + for id := range s.changed { + refs := s.currentRefs(id) + captured = append(captured, referenceSnapshot{id: id, refs: refs}) + retained := s.retained[id] + for _, ck := range refs.checkpoints { + if !slices.Contains(retained.attempted.checkpoints, ck) { + retained.attempted.checkpoints = append(retained.attempted.checkpoints, ck) + } + } + s.changeRetainedRefs(id, retained) + } s.mu.Unlock() s.buf = append(s.buf[:0], '{') for i, row := range rows { @@ -190,6 +271,12 @@ func (s *claimStore) commit(snap claimSnapshot) error { return err } s.mu.Lock() + for _, snapshot := range captured { + s.changeRetainedRefs(snapshot.id, retainedClaimRefs{acknowledged: snapshot.refs}) + if sameClaimRefs(s.currentRefs(snapshot.id), snapshot.refs) { + delete(s.changed, snapshot.id) + } + } s.written = seq s.mu.Unlock() return nil @@ -251,6 +338,70 @@ func (s *claimStore) synced() bool { return s.written == s.seq } +func (s *claimStore) currentRefs(id string) claimRefs { + if row := s.rows[id]; row != nil { + return *row.refs + } + return claimRefs{} +} + +func (s *claimStore) changeCurrentRefs(id string, after claimRefs) { + before := s.currentRefs(id) + if sameClaimRefs(before, after) { + return + } + s.changeCheckpointCounts(before, -1) + s.changeCheckpointCounts(after, 1) + s.changed[id] = struct{}{} +} + +func (s *claimStore) changeCheckpointCounts(refs claimRefs, delta int) { + for _, ck := range refs.checkpoints { + s.checkpoints[ck] += delta + if s.checkpoints[ck] == 0 { + delete(s.checkpoints, ck) + } + } +} + +func (s *claimStore) changeRetainedRefs(id string, after retainedClaimRefs) { + before := s.retained[id] + for i, retained := range [2]retainedClaimRefs{before, after} { + delta := 2*i - 1 + for _, refs := range [2]claimRefs{retained.acknowledged, retained.attempted} { + s.changeCheckpointCounts(refs, delta) + } + } + if after.acknowledged.empty() && after.attempted.empty() { + delete(s.retained, id) + } else { + s.retained[id] = after + } +} + +func (s *claimStore) referencesCheckpoint(id string) bool { + s.mu.Lock() + defer s.mu.Unlock() + return s.checkpoints[id] > 0 +} + +func (s *claimStore) checkpointReferences() map[string]struct{} { + s.mu.Lock() + defer s.mu.Unlock() + refs := make(map[string]struct{}, len(s.checkpoints)) + for ck := range s.checkpoints { + refs[ck] = struct{}{} + } + return refs +} + +func (s *claimStore) hasCheckpointReferences(owner string) bool { + s.mu.Lock() + defer s.mu.Unlock() + retained := s.retained[owner] + return len(s.currentRefs(owner).checkpoints) != 0 || len(retained.acknowledged.checkpoints) != 0 || len(retained.attempted.checkpoints) != 0 +} + func encodeRow(dto *claimDTO) ([]byte, error) { raw, err := json.Marshal(map[string]*claimDTO{dto.ID: dto}) if err != nil { @@ -279,3 +430,20 @@ func volumesWantDTO(sb *types.Sandbox) []types.Volume { } return append([]types.Volume{}, sb.VolumesWant...) } + +func newClaimRow(sb *types.Sandbox, previous *claimRow) *claimRow { + if previous != nil && previous.refs.matches(sb) { + return &claimRow{id: sb.ID, refs: previous.refs, dto: dtoOf(sb)} + } + refs := &claimRefs{} + for _, id := range [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} { + if id != "" && !slices.Contains(refs.checkpoints, id) { + refs.checkpoints = append(refs.checkpoints, id) + } + } + return &claimRow{id: sb.ID, refs: refs, dto: dtoOf(sb)} +} + +func sameClaimRefs(a, b claimRefs) bool { + return slices.Equal(a.checkpoints, b.checkpoints) +} diff --git a/sandboxd/pool/claims_test.go b/sandboxd/pool/claims_test.go index 0539a537..518ee3e9 100644 --- a/sandboxd/pool/claims_test.go +++ b/sandboxd/pool/claims_test.go @@ -1,6 +1,7 @@ package pool import ( + "encoding/json/v2" "errors" "io" "io/fs" @@ -58,23 +59,37 @@ func TestRecommitDrainsAChangeItWasTooBusyToSee(t *testing.T) { return m.store.set(sb) } + sb.ReplicaPending = "first" m.store.recommit(t.Context(), change()) waitFor(t, func() bool { - if m.store.writeMu.TryLock() { - m.store.writeMu.Unlock() - return false - } - return true + m.store.mu.Lock() + defer m.store.mu.Unlock() + return slices.Contains(m.store.retained[sb.ID].attempted.checkpoints, "first") }) + sb.ReplicaPending = "second" m.store.recommit(t.Context(), change()) r, err := os.Open(fifo) if err != nil { t.Fatalf("open fifo: %v", err) } - _, _ = io.Copy(io.Discard, r) + raw, err := io.ReadAll(r) + if err != nil { + t.Fatal(err) + } + var captured map[string]*types.Sandbox + if err = json.Unmarshal(raw, &captured); err != nil { + t.Fatal(err) + } + if captured[sb.ID].ReplicaPending != "first" { + t.Fatal("FIFO did not capture original snapshot") + } _ = r.Close() waitFor(t, func() bool { return !m.store.recommitting.Load() && !m.store.pending(m.store.mark()) }) + loaded, err := newClaimStore(filepath.Dir(m.store.path), false).load() + if err != nil || loaded[sb.ID].ReplicaPending != "second" { + t.Fatalf("recommit missed newer projection: %+v, %v", loaded, err) + } } func TestStoreRoundTrip(t *testing.T) { diff --git a/sandboxd/pool/failover.go b/sandboxd/pool/failover.go index dd0896f4..b02b20dd 100644 --- a/sandboxd/pool/failover.go +++ b/sandboxd/pool/failover.go @@ -477,10 +477,14 @@ func (m *Manager) dropStaleRow(ctx context.Context, sb *types.Sandbox, rec archi func (m *Manager) purgeArchiveRow(ctx context.Context, rec archives.Record) { logger := log.WithFunc("pool.purgeArchiveRow") - if err := m.deleteArchiveCk(ctx, rec.Checkpoint); err != nil { + deleted, err := m.deleteArchiveCk(ctx, rec.Checkpoint) + if err != nil { logger.Warnf(ctx, "purge archive %s: delete ck %s: %v", rec.ID, rec.Checkpoint, err) return } + if !deleted { + return + } switch err := m.archives.Delete(ctx, rec.ID, rec.Epoch); { case errors.Is(err, archives.ErrLost): return @@ -647,5 +651,5 @@ func (m *Manager) archiveRowed(sb *types.Sandbox) bool { } func (m *Manager) expiryDropsRow(sb *types.Sandbox) bool { - return m.archiveRowed(sb) || sb.HasReplica() && !m.archivesAtDeadline(sb) + return m.archiveRowed(sb) || m.archives != nil && sb.HasReplica() && !m.archivesAtDeadline(sb) } diff --git a/sandboxd/pool/filesystem_fault_test.go b/sandboxd/pool/filesystem_fault_test.go new file mode 100644 index 00000000..7ddaa831 --- /dev/null +++ b/sandboxd/pool/filesystem_fault_test.go @@ -0,0 +1,40 @@ +package pool + +import ( + "os" + "os/exec" + "regexp" + "runtime" + "strings" + "testing" +) + +func withoutDACOverride(t *testing.T) bool { + t.Helper() + if os.Geteuid() != 0 || os.Getenv("SANDBOX_TEST_WITHOUT_DAC") == t.Name() { + return false + } + if runtime.GOOS != "linux" { + t.Skip("post-rename permission fixture requires non-root or Linux setpriv") + } + setpriv, err := exec.LookPath("setpriv") + if err != nil { + t.Skip("post-rename permission fixture requires setpriv to drop root DAC capabilities") + } + binary, err := os.Executable() + if err != nil { + t.Fatal(err) + } + parts := strings.Split(t.Name(), "/") + for i, part := range parts { + parts[i] = "^" + regexp.QuoteMeta(part) + "$" + } + cmd := exec.Command(setpriv, "--bounding-set=-dac_override,-dac_read_search", binary, "-test.run="+strings.Join(parts, "/"), "-test.v") + cmd.Env = append(os.Environ(), "SANDBOX_TEST_WITHOUT_DAC="+t.Name()) + output, err := cmd.CombinedOutput() + if err != nil { + t.Fatalf("test without DAC override: %v\n%s", err, output) + } + t.Logf("test without DAC override:\n%s", output) + return true +} diff --git a/sandboxd/pool/hibernate_test.go b/sandboxd/pool/hibernate_test.go index 1d355272..ffc4dd9e 100644 --- a/sandboxd/pool/hibernate_test.go +++ b/sandboxd/pool/hibernate_test.go @@ -35,7 +35,7 @@ func TestHibernatePersistFailureSurfacesAndConverges(t *testing.T) { if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { t.Fatalf("Hibernate retry after heal: %v", err) } - got, err := (&claimStore{path: broken}).load() + got, err := newClaimStore(filepath.Dir(broken), false).load() if err != nil || got[sb.ID] == nil || got[sb.ID].HibernateSnap == "" || got[sb.ID].PendingSnap != "" { t.Fatalf("journal after healed retry: %+v, %v", got[sb.ID], err) } @@ -75,7 +75,7 @@ func TestWakePersistFailureSurfaces(t *testing.T) { } return eng.snapRemoved(snap) }) - got, loadErr := (&claimStore{path: broken}).load() + got, loadErr := newClaimStore(filepath.Dir(broken), false).load() if loadErr != nil || got[sb.ID] == nil || got[sb.ID].HibernateSnap != "" { t.Fatalf("journal after converge: %+v, %v", got[sb.ID], loadErr) } @@ -305,7 +305,7 @@ func TestResolveAdoptBillsWhenPersistFails(t *testing.T) { t.Fatalf("heal store dir: %v", err) } waitFor(t, func() bool { - got, err := (&claimStore{path: broken}).load() + got, err := newClaimStore(filepath.Dir(broken), false).load() return err == nil && got[sb.ID] != nil && got[sb.ID].HibernateSnap != "" }) }) @@ -365,7 +365,7 @@ func TestFlushClaimsClosesShutdownWindow(t *testing.T) { if err := m.FlushClaims(); err != nil { t.Fatalf("FlushClaims: %v", err) } - got, err := (&claimStore{path: broken}).load() + got, err := newClaimStore(filepath.Dir(broken), false).load() if err != nil || got[sb.ID] == nil || got[sb.ID].HibernateSnap != "" { t.Fatalf("journal after flush: %+v, %v", got[sb.ID], err) } diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index 3bfa3cf1..b8f836ec 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -313,9 +313,9 @@ func (m *Manager) reclaimOrphanArchiveCks(ctx context.Context, claims map[string if row, ok := rows[ckpt.SandboxID]; ok && (row.Owner != m.nodeID || row.Checkpoint == ckpt.ID) { continue } - if err := m.deleteArchiveCk(ctx, ckpt.ID); err != nil { + if deleted, err := m.deleteArchiveCk(ctx, ckpt.ID); err != nil { logger.Warnf(ctx, "reclaim %s: %v", ckpt.ID, err) - } else { + } else if deleted { logger.Infof(ctx, "reclaimed orphan archive ck %s", ckpt.ID) } } diff --git a/sandboxd/pool/replica.go b/sandboxd/pool/replica.go index 4706ddd4..911451b6 100644 --- a/sandboxd/pool/replica.go +++ b/sandboxd/pool/replica.go @@ -124,19 +124,17 @@ func (m *Manager) replicate(ctx context.Context, sb *types.Sandbox) error { return errWokeMeanwhile } if err := m.store.commit(intent); err != nil { - m.clearReplicaIntent(ctx, sb, ckID) - m.deleteOrphanArchiveCk(ctx, ckID) - return fmt.Errorf("replicate %s: persist intent: %w", sb.ID, err) + clearErr := m.clearReplicaIntent(ctx, sb, ckID) + return fmt.Errorf("replicate %s: persist intent: %w", sb.ID, errors.Join(err, clearErr)) } epoch, err := m.replaceReplica(ctx, sb, ckID) switch { case errors.Is(err, archives.ErrLost): - m.clearReplicaIntent(ctx, sb, ckID) - m.deleteOrphanArchiveCk(ctx, ckID) + clearErr := m.clearReplicaIntent(ctx, sb, ckID) if settleErr := m.refusedReplace(ctx, sb); settleErr != nil && !errors.Is(settleErr, ErrUnknownSandbox) { return fmt.Errorf("replicate %s: settle refused write: %w", sb.ID, settleErr) } - return fmt.Errorf("replicate %s: registry: %w", sb.ID, err) + return fmt.Errorf("replicate %s: registry: %w", sb.ID, errors.Join(err, clearErr)) case err != nil: return fmt.Errorf("replicate %s: registry: %w", sb.ID, err) } @@ -184,7 +182,7 @@ func (m *Manager) settleReplicaIntent(ctx context.Context, sb *types.Sandbox) er return ErrUnknownSandbox case ok && !rec.Consumed && rec.Checkpoint == pending: var prev string - live, _ := m.commitIfLive(ctx, sb, func() { + live, commitErr := m.commitIfLive(ctx, sb, func() { prev, sb.ReplicaCk, sb.ReplicaPending, sb.ArchiveEpoch = sb.ReplicaCk, pending, "", rec.Epoch }) if !live { @@ -193,11 +191,12 @@ func (m *Manager) settleReplicaIntent(ctx context.Context, sb *types.Sandbox) er if prev != "" { m.deleteOrphanArchiveCk(ctx, prev) } + if commitErr != nil { + return fmt.Errorf("persist settled replica: %w", commitErr) + } return nil } - m.clearReplicaIntent(ctx, sb, pending) - m.deleteOrphanArchiveCk(ctx, pending) - return nil + return m.clearReplicaIntent(ctx, sb, pending) } // replicaGone reports whether the absence of sb's row means another node adopted the claim and finished with it. @@ -218,12 +217,20 @@ func (m *Manager) replicaGone(ctx context.Context, sb *types.Sandbox) (bool, err return false, err } -func (m *Manager) clearReplicaIntent(ctx context.Context, sb *types.Sandbox, ck string) { - _, _ = m.commitIfLive(ctx, sb, func() { +func (m *Manager) clearReplicaIntent(ctx context.Context, sb *types.Sandbox, ck string) error { + live, err := m.commitIfLive(ctx, sb, func() { if sb.ReplicaPending == ck { sb.ReplicaPending = "" } }) + m.deleteOrphanArchiveCk(ctx, ck) + if !live { + return ErrUnknownSandbox + } + if err != nil { + return fmt.Errorf("persist replica intent clear: %w", err) + } + return nil } func (m *Manager) retireReplica(ctx context.Context, sb *types.Sandbox) { @@ -247,14 +254,17 @@ func (m *Manager) dropReplica(ctx context.Context, sb *types.Sandbox) error { return err } var ck string - live, _ := m.commitIfLive(ctx, sb, func() { + live, err := m.commitIfLive(ctx, sb, func() { ck, sb.ReplicaCk = sb.ReplicaCk, "" }) if !live { return ErrUnknownSandbox } - m.dropTombstone(ctx, sb.ID, consumed) m.deleteOrphanArchiveCk(ctx, ck) + if err != nil { + return fmt.Errorf("persist replica retirement: %w", err) + } + m.dropTombstone(ctx, sb.ID, consumed) return nil } From 6638904a93a54c25465e7599268edb49fce16065 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Sun, 25 Oct 2026 15:39:12 +0000 Subject: [PATCH 02/15] pool: derive checkpoint pins from recorded claims Use the existing store reference set and pending checkpoints instead of rescanning live claims. Guard stale hibernate and archive writers before they can overwrite a released or replaced claim row. --- sandboxd/pool/archive.go | 5 +- sandboxd/pool/checkpoint.go | 12 +-- sandboxd/pool/claim.go | 17 ++-- sandboxd/pool/hibernate.go | 18 ++-- sandboxd/pool/imds_test.go | 3 + sandboxd/pool/pool_test.go | 16 ++++ sandboxd/pool/reconcile_test.go | 6 +- sandboxd/pool/stale_claim_test.go | 141 ++++++++++++++++++++++++++++++ 8 files changed, 184 insertions(+), 34 deletions(-) create mode 100644 sandboxd/pool/stale_claim_test.go diff --git a/sandboxd/pool/archive.go b/sandboxd/pool/archive.go index 529f677c..e16a4be8 100644 --- a/sandboxd/pool/archive.go +++ b/sandboxd/pool/archive.go @@ -122,10 +122,7 @@ func (m *Manager) archive(ctx context.Context, sb *types.Sandbox) error { intent := m.store.set(sb) m.mu.Unlock() if err = m.store.commit(intent); err != nil { - m.mu.Lock() - sb.ArchivePending = "" - rb := m.store.set(sb) - m.mu.Unlock() + rb, _ := m.setIfLive(sb, func() { sb.ArchivePending = "" }) sb.Transition.Unlock() m.store.recommit(ctx, rb) m.deleteOrphanArchiveCk(ctx, ck.ID) diff --git a/sandboxd/pool/checkpoint.go b/sandboxd/pool/checkpoint.go index 5bc0ce39..e6a32474 100644 --- a/sandboxd/pool/checkpoint.go +++ b/sandboxd/pool/checkpoint.go @@ -5,6 +5,7 @@ import ( "encoding/json/v2" "errors" "fmt" + "maps" "os" "path/filepath" "slices" @@ -318,16 +319,7 @@ func (m *Manager) pinnedArchiveCks() map[string]struct{} { m.mu.Lock() defer m.mu.Unlock() pinned := m.store.checkpointReferences() - for id := range m.pendingCks { - pinned[id] = struct{}{} - } - for _, sb := range m.claimed { - for _, id := range [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} { - if id != "" { - pinned[id] = struct{}{} - } - } - } + maps.Copy(pinned, m.pendingCks) return pinned } diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index adc786ea..d2ca8507 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -703,15 +703,18 @@ func (m *Manager) purgeArchiveCk(ctx context.Context, id, ck, tenant string) { m.recordUsage(ctx, usageEvent{Event: "archive_delete", ID: id, Reference: ck, Tenant: tenant}) } -func (m *Manager) commitIfLive(ctx context.Context, sb *types.Sandbox, mutate func()) (bool, error) { +func (m *Manager) setIfLive(sb *types.Sandbox, mutate func()) (claimSnapshot, bool) { m.mu.Lock() - live := m.claimed[sb.ID] == sb - var js claimSnapshot - if live { - mutate() - js = m.store.set(sb) + defer m.mu.Unlock() + if m.claimed[sb.ID] != sb { + return claimSnapshot{}, false } - m.mu.Unlock() + mutate() + return m.store.set(sb), true +} + +func (m *Manager) commitIfLive(ctx context.Context, sb *types.Sandbox, mutate func()) (bool, error) { + js, live := m.setIfLive(sb, mutate) if !live { return false, nil } diff --git a/sandboxd/pool/hibernate.go b/sandboxd/pool/hibernate.go index cac827ed..4e464fbf 100644 --- a/sandboxd/pool/hibernate.go +++ b/sandboxd/pool/hibernate.go @@ -93,11 +93,12 @@ func (m *Manager) hibernateLocked(ctx context.Context, sb *types.Sandbox) error // cocoon rejects sb.ID's underscore, so the snapshot name is VMName-based snap := hibernatePrefix + strings.TrimPrefix(sb.VMName, vmPrefix) + "-" + randHex(3) // journal the intent first so Reconcile can adopt a hibernate whose commit never landed - if err := m.store.commit(m.setPendingSnap(sb, snap)); err != nil { - m.mu.Lock() - sb.PendingSnap = "" - m.store.set(sb) - m.mu.Unlock() + intent, live := m.setIfLive(sb, func() { sb.PendingSnap = snap }) + if !live { + return ErrUnknownSandbox + } + if err := m.store.commit(intent); err != nil { + m.setIfLive(sb, func() { sb.PendingSnap = "" }) return fmt.Errorf("hibernate %s: persist intent: %w", sb.ID, err) } readerSafe := false @@ -323,13 +324,6 @@ func (m *Manager) dropStale(ctx context.Context, sb *types.Sandbox) { } } -func (m *Manager) setPendingSnap(sb *types.Sandbox, snap string) claimSnapshot { - m.mu.Lock() - defer m.mu.Unlock() - sb.PendingSnap = snap - return m.store.set(sb) -} - // settlePendingSnap settles a dangling hibernate intent and bills an adopted one, which never was. func (m *Manager) settlePendingSnap(ctx context.Context, sb *types.Sandbox) (adopted bool, err error) { adopted, err = m.resolvePendingSnap(ctx, sb) diff --git a/sandboxd/pool/imds_test.go b/sandboxd/pool/imds_test.go index 011d5693..5cca29d2 100644 --- a/sandboxd/pool/imds_test.go +++ b/sandboxd/pool/imds_test.go @@ -54,7 +54,10 @@ func TestSetInstanceMetadataRefusesAPausedSandboxWithoutWaking(t *testing.T) { if err := m.SetInstanceMetadata(t.Context(), running.ID, doc); !errors.Is(err, ErrPaused) { t.Errorf("pending hibernate: %v, want ErrPaused", err) } + m.mu.Lock() running.PendingSnap, running.ArchiveCk = "", "ck" + m.store.set(running) + m.mu.Unlock() if err := m.SetInstanceMetadata(t.Context(), running.ID, doc); !errors.Is(err, ErrArchived) { t.Errorf("archived: %v, want ErrArchived", err) } diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index 136cd118..bce9f8e5 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -970,6 +970,7 @@ func TestRenewRefusesAnArchivedClaim(t *testing.T) { sb := mustClaim(t, m, testKey) m.mu.Lock() sb.ArchiveCk, sb.Deadline = "ck_1", time.Time{} + m.store.set(sb) m.mu.Unlock() if _, err := m.Renew(t.Context(), sb.ID, Cred{Token: sb.Token}, 0, ""); !errors.Is(err, ErrArchived) { @@ -1087,7 +1088,10 @@ func TestOperatorPortDialNeverWakesAPausedSandbox(t *testing.T) { "pending hibernate": func() { running.PendingSnap = "snap" }, "archived": func() { running.PendingSnap, running.ArchiveCk = "", "ck" }, } { + m.mu.Lock() set() + m.store.set(running) + m.mu.Unlock() if _, err := m.DialPort(t.Context(), running.ID, operator, 49983); !errors.Is(err, ErrPaused) { t.Errorf("%s: %v, want ErrPaused", name, err) } @@ -1939,6 +1943,18 @@ func newTestManagerAt(t testing.TB, eng *fakeEngine, dataDir string, pools ...co if err != nil { t.Fatalf("setup manager: %v", err) } + t.Cleanup(func() { + m.mu.Lock() + defer m.mu.Unlock() + m.store.mu.Lock() + defer m.store.mu.Unlock() + for id, sb := range m.claimed { + row := m.store.rows[id] + if row == nil || !row.refs.matches(sb) { + t.Errorf("claim %s references differ from its store row", id) + } + } + }) return m } diff --git a/sandboxd/pool/reconcile_test.go b/sandboxd/pool/reconcile_test.go index 907c2ba4..ec820677 100644 --- a/sandboxd/pool/reconcile_test.go +++ b/sandboxd/pool/reconcile_test.go @@ -24,7 +24,11 @@ func TestReconcileJournalsAnAdoptedHibernate(t *testing.T) { if err := eng.Hibernate(t.Context(), sb.VMName, snap); err != nil { t.Fatalf("engine hibernate: %v", err) } - if err := m.store.commit(m.setPendingSnap(sb, snap)); err != nil { + intent, live := m.setIfLive(sb, func() { sb.PendingSnap = snap }) + if !live { + t.Fatal("hibernate claim disappeared before intent") + } + if err := m.store.commit(intent); err != nil { t.Fatalf("journal intent: %v", err) } diff --git a/sandboxd/pool/stale_claim_test.go b/sandboxd/pool/stale_claim_test.go new file mode 100644 index 00000000..5b2dfd7f --- /dev/null +++ b/sandboxd/pool/stale_claim_test.go @@ -0,0 +1,141 @@ +package pool + +import ( + "errors" + "testing" + + "github.com/cocoonstack/sandbox/sandboxd/types" +) + +func TestHibernateRejectsStaleClaim(t *testing.T) { + for _, replace := range []bool{false, true} { + name := "released" + if replace { + name = "replaced" + } + t.Run(name, func(t *testing.T) { + eng := newFakeEngine() + m := newTestManager(t, eng) + sb := mustClaim(t, m, testKey) + ck, err := m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "", "") + if err != nil { + t.Fatal(err) + } + if err = m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + if replace { + m.mu.Lock() + replacement := &types.Sandbox{ID: sb.ID, ReplicaCk: ck.ID} + m.addClaimLocked(replacement) + js := m.store.set(replacement) + m.mu.Unlock() + if err = m.store.commit(js); err != nil { + t.Fatal(err) + } + } + sb.Transition.Lock() + err = m.hibernateLocked(t.Context(), sb) + sb.Transition.Unlock() + if !errors.Is(err, ErrUnknownSandbox) { + t.Errorf("stale Hibernate: %v, want ErrUnknownSandbox", err) + } + if eng.hibernateCount() != 0 { + t.Error("stale Hibernate reached the engine") + } + assertStaleClaimPreserved(t, m, sb.ID, ck.ID, replace) + }) + } +} + +func TestIntentRollbackPreservesCurrentClaim(t *testing.T) { + for _, operation := range []string{"hibernate", "archive"} { + for _, replace := range []bool{false, true} { + name := operation + "/released" + if replace { + name = operation + "/replaced" + } + t.Run(name, func(t *testing.T) { + var m *Manager + if operation == "archive" { + m = newFailoverCluster(t).node(t, "a", t.TempDir(), keepAnHour) + } else { + m = newTestManager(t, newFakeEngine()) + } + sb := mustClaim(t, m, testKey) + ck, err := m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "", "") + if err != nil { + t.Fatal(err) + } + if operation == "archive" { + if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { + t.Fatal(err) + } + } + m.store.writeMu.Lock() + locked := true + t.Cleanup(func() { + if locked { + m.store.writeMu.Unlock() + } + }) + done := make(chan error, 1) + go func() { + if operation == "archive" { + done <- m.archive(t.Context(), sb) + } else { + done <- m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}) + } + }() + waitFor(t, func() bool { + m.mu.Lock() + defer m.mu.Unlock() + return sb.PendingSnap != "" || sb.ArchivePending != "" + }) + m.mu.Lock() + m.dropClaimLocked(sb) + m.store.del(sb.ID) + if replace { + replacement := &types.Sandbox{ID: sb.ID, ReplicaCk: ck.ID} + m.addClaimLocked(replacement) + m.store.set(replacement) + } + m.mu.Unlock() + restore := failClaimWrites(t, m.store) + m.store.writeMu.Unlock() + locked = false + if err := <-done; err == nil { + t.Fatal("intent write succeeded despite filesystem failure") + } + restore() + if err := m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + waitFor(t, func() bool { return !m.store.recommitting.Load() }) + assertStaleClaimPreserved(t, m, sb.ID, ck.ID, replace) + }) + } + } +} + +func assertStaleClaimPreserved(t *testing.T, m *Manager, id, ck string, replaced bool) { + t.Helper() + loaded, err := newClaimStore(m.dataDir, false).load() + if err != nil { + t.Fatal(err) + } + if replaced { + if loaded[id] == nil || loaded[id].ReplicaCk != ck || loaded[id].PendingSnap != "" { + t.Errorf("stale operation overwrote replacement on disk: %+v", loaded[id]) + } + } else if loaded[id] != nil { + t.Errorf("stale operation resurrected released claim on disk: %+v", loaded[id]) + } + if err := m.markArchiveCk(ck); err != nil { + t.Fatal(err) + } + m.retryArchiveDeletes(t.Context()) + if exists := ckExists(t, m, ck); exists != replaced { + t.Errorf("checkpoint survived cleanup = %v, want %v", exists, replaced) + } +} From 886fbca1d3f7feff26e0af7a05ca2df466803da7 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 02:46:13 +0000 Subject: [PATCH 03/15] pool: assert replica cleanup keeps its checkpoint through tombstone deletion --- sandboxd/pool/replica_test.go | 7 +++++++ 1 file changed, 7 insertions(+) diff --git a/sandboxd/pool/replica_test.go b/sandboxd/pool/replica_test.go index e36dee45..bdef433b 100644 --- a/sandboxd/pool/replica_test.go +++ b/sandboxd/pool/replica_test.go @@ -694,11 +694,18 @@ func TestReplicaExpiryDropsTheRowThenTheClaim(t *testing.T) { vm := lockedVMName(sb) replicateNow(t, a, sb) ck := a.replicaOf(sb) + checkpointBeforeDelete := make(chan bool, 1) + a.archives = &hookRegistry{Registry: c.reg, beforeDelete: func() { + checkpointBeforeDelete <- ckExists(t, a, ck) + }} a.mu.Lock() sb.Deadline = time.Now().Add(-time.Second) a.mu.Unlock() a.reapOnce(t.Context()) + if !<-checkpointBeforeDelete { + t.Fatal("checkpoint disappeared before its registry tombstone was deleted") + } waitFor(t, func() bool { return a.replicaOf(sb) == "" && !ckExists(t, a, ck) }) if _, ok, _ := c.reg.Get(t.Context(), sb.ID); ok { t.Error("the row outlived the first reap") From 02b96ab74cbe82b18447cccb9e2f9de022e8cc4c Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 02:57:25 +0000 Subject: [PATCH 04/15] pool: preserve replica cleanup order and avoid duplicate delete markers --- sandboxd/pool/archive.go | 4 ++++ sandboxd/pool/claim.go | 2 +- sandboxd/pool/replica.go | 3 ++- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/sandboxd/pool/archive.go b/sandboxd/pool/archive.go index e16a4be8..9624529c 100644 --- a/sandboxd/pool/archive.go +++ b/sandboxd/pool/archive.go @@ -352,6 +352,10 @@ func (m *Manager) deleteArchiveCk(ctx context.Context, ckID string) (bool, error if err := m.markArchiveCk(ckID); err != nil { return false, fmt.Errorf("track: %w", err) } + return m.finishArchiveCkDelete(ctx, ckID) +} + +func (m *Manager) finishArchiveCkDelete(ctx context.Context, ckID string) (bool, error) { unlock := m.lockRec(ckID) if m.store.referencesCheckpoint(ckID) { unlock() diff --git a/sandboxd/pool/claim.go b/sandboxd/pool/claim.go index d2ca8507..f9427792 100644 --- a/sandboxd/pool/claim.go +++ b/sandboxd/pool/claim.go @@ -696,7 +696,7 @@ func (m *Manager) pauseExpired(ctx context.Context, sb *types.Sandbox) error { func (m *Manager) purgeArchiveCk(ctx context.Context, id, ck, tenant string) { logger := log.WithFunc("pool.purgeArchiveCk") - if _, err := m.deleteArchiveCk(ctx, ck); err != nil { + if _, err := m.finishArchiveCkDelete(ctx, ck); err != nil { logger.Warnf(ctx, "delete archive ck %s: %v", ck, err) } m.counters.archiveDeletes.Add(1) diff --git a/sandboxd/pool/replica.go b/sandboxd/pool/replica.go index 911451b6..8cad931e 100644 --- a/sandboxd/pool/replica.go +++ b/sandboxd/pool/replica.go @@ -260,11 +260,12 @@ func (m *Manager) dropReplica(ctx context.Context, sb *types.Sandbox) error { if !live { return ErrUnknownSandbox } - m.deleteOrphanArchiveCk(ctx, ck) if err != nil { + m.deleteOrphanArchiveCk(ctx, ck) return fmt.Errorf("persist replica retirement: %w", err) } m.dropTombstone(ctx, sb.ID, consumed) + m.deleteOrphanArchiveCk(ctx, ck) return nil } From 06ef5c9d07ff48524a2f0d2e526321dd021123ba Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 03:03:02 +0000 Subject: [PATCH 05/15] pool: simplify retained checkpoint references and fault tests --- sandboxd/pool/archive_test.go | 2 +- sandboxd/pool/checkpoint_bench_test.go | 3 +- sandboxd/pool/checkpoint_cleanup_test.go | 253 +++++++++----------- sandboxd/pool/checkpoint_references_test.go | 41 +++- sandboxd/pool/claims.go | 153 ++++-------- sandboxd/pool/claims_test.go | 18 +- sandboxd/pool/failover_test.go | 6 +- sandboxd/pool/filesystem_fault_test.go | 40 ---- sandboxd/pool/pool_test.go | 2 +- sandboxd/pool/replica_test.go | 2 +- sandboxd/pool/volume_mutation_test.go | 4 +- 11 files changed, 208 insertions(+), 316 deletions(-) delete mode 100644 sandboxd/pool/filesystem_fault_test.go diff --git a/sandboxd/pool/archive_test.go b/sandboxd/pool/archive_test.go index 468724f9..f3012e57 100644 --- a/sandboxd/pool/archive_test.go +++ b/sandboxd/pool/archive_test.go @@ -522,7 +522,7 @@ func TestArchiveWakeRequiresDeleteMarker(t *testing.T) { if !archived || !ckExists(t, m, ck) { t.Fatal("failed wake did not retain the archived claim and checkpoint") } - claims, err := m.store.load() + claims, err := newClaimStore(m.dataDir, false).load() if err != nil || claims[sb.ID] == nil || claims[sb.ID].ArchiveCk != ck { t.Fatalf("failed wake changed the durable archive claim: %+v, %v", claims[sb.ID], err) } diff --git a/sandboxd/pool/checkpoint_bench_test.go b/sandboxd/pool/checkpoint_bench_test.go index 5bc2c26e..e3ba1b6c 100644 --- a/sandboxd/pool/checkpoint_bench_test.go +++ b/sandboxd/pool/checkpoint_bench_test.go @@ -2,6 +2,7 @@ package pool import ( "fmt" + "path/filepath" "testing" "time" @@ -25,7 +26,7 @@ func BenchmarkClaimCommitWithCheckpoints(b *testing.B) { b.Fatal(err) } } - loaded, err := s.load() + loaded, err := newClaimStore(filepath.Dir(s.path), false).load() if err != nil || len(loaded) != 5000 || !loaded[one.ID].Deadline.Equal(one.Deadline) || loaded[one.ID].ReplicaCk != one.ReplicaCk { b.Fatalf("commit output: rows=%d err=%v", len(loaded), err) } diff --git a/sandboxd/pool/checkpoint_cleanup_test.go b/sandboxd/pool/checkpoint_cleanup_test.go index cf5b5c8c..d543dd37 100644 --- a/sandboxd/pool/checkpoint_cleanup_test.go +++ b/sandboxd/pool/checkpoint_cleanup_test.go @@ -36,119 +36,125 @@ var strandedStates = []strandedState{ } func TestReplicaRetirementKeepsTombstoneUntilClaimCommit(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - replicateNow(t, m, sb) - ck := sb.ReplicaCk - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - sb.Transition.Lock() - err := m.dropReplica(t.Context(), sb) - sb.Transition.Unlock() - if err == nil { - t.Error("retirement acknowledged a failed claim commit") - } - rec, ok, err := c.reg.Get(t.Context(), sb.ID) - if err != nil || !ok || !rec.Consumed || !ckExists(t, m, ck) { - t.Fatalf("failed commit lost recovery tombstone/checkpoint: %+v, exists=%t, err=%v", rec, ok, err) - } - m.reconcileOwnRow(t.Context(), rec, time.Now()) - if rec, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok || !rec.Consumed { - t.Fatalf("sweep erased unacknowledged retirement: %+v, exists=%t, err=%v", rec, ok, err) - } - restoreWrites() - restarted := c.restart(t, m) - if err = restarted.Reconcile(t.Context()); err != nil { - t.Fatal(err) - } - restored, ok := restarted.byID(sb.ID) - if !ok { - t.Fatal("restart dropped a claim after failed retirement commit") - } - restarted.retireReplica(t.Context(), restored) - if restored.HasReplica() { - t.Fatal("restart did not finish retirement") + for _, restart := range []bool{false, true} { + t.Run(fmt.Sprintf("restart=%t", restart), func(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + replicateNow(t, m, sb) + ck := sb.ReplicaCk + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + sb.Transition.Lock() + err := m.dropReplica(t.Context(), sb) + sb.Transition.Unlock() + if err == nil || sb.HasReplica() { + t.Fatalf("expected an unacknowledged memory clear: replica=%t, %v", sb.HasReplica(), err) + } + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil || !ok || !rec.Consumed || !ckExists(t, m, ck) { + t.Fatalf("failed commit lost recovery tombstone/checkpoint: %+v, exists=%t, err=%v", rec, ok, err) + } + if !m.deleteMarked(ck) { + t.Error("failed replica clear did not queue its checkpoint for cleanup") + } + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if rec, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok || !rec.Consumed { + t.Fatalf("sweep erased unacknowledged retirement: %+v, exists=%t, err=%v", rec, ok, err) + } + m.retryArchiveDeletes(t.Context()) + if !ckExists(t, m, ck) || !m.deleteMarked(ck) { + t.Fatal("cleanup discarded unacknowledged checkpoint or retry marker") + } + restoreWrites() + if restart { + m = c.restart(t, m) + if err = m.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + restored, ok := m.byID(sb.ID) + if !ok { + t.Fatal("restart dropped a claim after failed retirement commit") + } + m.retireReplica(t.Context(), restored) + if restored.HasReplica() { + t.Fatal("restart did not finish retirement") + } + } else if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + m.retryArchiveDeletes(t.Context()) + if ckExists(t, m, ck) || m.deleteMarked(ck) { + t.Fatal("checkpoint cleanup did not finish after its reference retired") + } + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || ok { + t.Fatalf("acknowledged retirement retained tombstone: %t, %v", ok, err) + } + }) } } func TestReplicaUnpublishedIntentSurvivesFailedClearAndRestart(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - m.archives = &hookRegistry{Registry: c.reg, failPut: true} - if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { - t.Fatal(err) - } - pending := sb.ReplicaPending - if pending == "" || !ckExists(t, m, pending) { - t.Fatal("missing unpublished checkpoint") - } - m.archives = c.reg - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - m.retireReplica(t.Context(), sb) - persisted, err := newClaimStore(m.dataDir, false).load() - if err != nil || persisted[sb.ID].ReplicaPending != pending { - t.Fatalf("failure did not retain old disk state: %+v, %v", persisted, err) - } - if !ckExists(t, m, pending) { - t.Error("failed pending clear deleted the checkpoint used to distinguish an unpublished intent from adoption") - } - t.Logf("failed unpublished clear: memory_pending=%q durable_pending=%q checkpoint_exists=%t synced=%t", sb.ReplicaPending, persisted[sb.ID].ReplicaPending, ckExists(t, m, pending), m.store.synced()) - restoreWrites() - restarted := c.restart(t, m) - if err := restarted.Reconcile(t.Context()); err != nil { - t.Fatal(err) - } - restored, ok := restarted.byID(sb.ID) - if !ok { - t.Fatal("restart mistook a failed pending clear for adoption and discarded the claim") - } - restarted.retireReplica(t.Context(), restored) - if restored.HasReplica() { - t.Fatal("restart failed to retire unpublished replica") - } -} - -func TestReplicaPendingCheckpointCleanupWaitsForAcknowledgment(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - m.archives = &hookRegistry{Registry: c.reg, failPut: true} - if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { - t.Fatal(err) - } - pending := sb.ReplicaPending - m.archives = c.reg - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - m.retireReplica(t.Context(), sb) - if sb.ReplicaPending != "" || m.store.synced() { - t.Fatal("expected memory clear before snapshot acknowledgment") - } - if deleted, err := m.deleteArchiveCk(t.Context(), pending); err != nil || deleted { - t.Fatalf("orphan delete ignored unacknowledged claim: deleted=%t, %v", deleted, err) - } - m.retryArchiveDeletes(t.Context()) - if !ckExists(t, m, pending) || !m.deleteMarked(pending) { - t.Fatal("cleanup discarded pending checkpoint or retry marker") - } - t.Logf("cleanup deferred: checkpoint=%s exists=%t marker=%t synced=%t", pending, ckExists(t, m, pending), m.deleteMarked(pending), m.store.synced()) - restoreWrites() - if err := m.store.commit(m.store.mark()); err != nil { - t.Fatal(err) - } - other := mustClaim(t, m, testKey) - m.mu.Lock() - other.LeaseSeconds++ - m.store.set(other) - m.mu.Unlock() - m.retryArchiveDeletes(t.Context()) - if ckExists(t, m, pending) || m.deleteMarked(pending) { - t.Fatal("acknowledged cleanup leaked checkpoint or retry marker") + for _, restart := range []bool{false, true} { + t.Run(fmt.Sprintf("restart=%t", restart), func(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) + } + pending := sb.ReplicaPending + if pending == "" || !ckExists(t, m, pending) { + t.Fatal("missing unpublished checkpoint") + } + m.archives = c.reg + m.store.recommitting.Store(true) + restoreWrites := failClaimWrites(t, m.store) + m.retireReplica(t.Context(), sb) + persisted, err := newClaimStore(m.dataDir, false).load() + if err != nil || persisted[sb.ID].ReplicaPending != pending { + t.Fatalf("failure did not retain old disk state: %+v, %v", persisted, err) + } + if sb.ReplicaPending != "" || m.store.synced() { + t.Fatal("expected memory clear before snapshot acknowledgment") + } + if deleted, err := m.deleteArchiveCk(t.Context(), pending); err != nil || deleted { + t.Fatalf("orphan delete ignored unacknowledged claim: deleted=%t, %v", deleted, err) + } + m.retryArchiveDeletes(t.Context()) + if !ckExists(t, m, pending) || !m.deleteMarked(pending) { + t.Fatal("cleanup discarded pending checkpoint or retry marker") + } + restoreWrites() + if restart { + m = c.restart(t, m) + if err := m.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + restored, ok := m.byID(sb.ID) + if !ok { + t.Fatal("restart mistook a failed pending clear for adoption and discarded the claim") + } + m.retireReplica(t.Context(), restored) + if restored.HasReplica() { + t.Fatal("restart failed to retire unpublished replica") + } + } else if err := m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) + } + other := mustClaim(t, m, testKey) + m.mu.Lock() + other.LeaseSeconds++ + m.store.set(other) + m.mu.Unlock() + m.retryArchiveDeletes(t.Context()) + if ckExists(t, m, pending) || m.deleteMarked(pending) { + t.Fatal("acknowledged cleanup leaked checkpoint or retry marker") + } + }) } - t.Logf("cleanup acknowledged: exists=%t marker=%t synced=%t", ckExists(t, m, pending), m.deleteMarked(pending), m.store.synced()) } func TestReplicaSettledCheckpointCleanupRetriesAfterFailedCommit(t *testing.T) { @@ -280,37 +286,6 @@ func TestFailoverDisabledExitKeepsReplicaCheckpoint(t *testing.T) { } } -func TestReplicaFailedClearQueuesCheckpointCleanup(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - replicateNow(t, m, sb) - ck := sb.ReplicaCk - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - sb.Transition.Lock() - err := m.dropReplica(t.Context(), sb) - sb.Transition.Unlock() - if err == nil || sb.HasReplica() { - t.Fatalf("expected an unacknowledged memory clear: replica=%t, %v", sb.HasReplica(), err) - } - if !m.deleteMarked(ck) { - t.Error("failed replica clear did not queue its checkpoint for cleanup") - } - m.retryArchiveDeletes(t.Context()) - if !ckExists(t, m, ck) { - t.Fatal("queued cleanup deleted a checkpoint still referenced by claims.json") - } - restoreWrites() - if err = m.store.commit(m.store.mark()); err != nil { - t.Fatal(err) - } - m.retryArchiveDeletes(t.Context()) - if ckExists(t, m, ck) || m.deleteMarked(ck) { - t.Fatal("checkpoint cleanup did not finish after its reference retired") - } -} - type strandedState struct { name string setup func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string diff --git a/sandboxd/pool/checkpoint_references_test.go b/sandboxd/pool/checkpoint_references_test.go index edcd5152..9f388288 100644 --- a/sandboxd/pool/checkpoint_references_test.go +++ b/sandboxd/pool/checkpoint_references_test.go @@ -16,8 +16,8 @@ import ( ) func TestClaimCheckpointReferencesTrackPossibleSnapshots(t *testing.T) { - if withoutDACOverride(t) { - return + if os.Geteuid() == 0 { + t.Skip("requires non-root: root can read the write-only directory") } dir := t.TempDir() s := newClaimStore(dir, true) @@ -56,13 +56,13 @@ func TestClaimCheckpointReferencesTrackPossibleSnapshots(t *testing.T) { t.Fatal("caller mutated retained reference set") } s.set(&types.Sandbox{ID: "other", ReplicaPending: "other-replica"}) - if s.hasCheckpointReferences("unknown") || !s.hasCheckpointReferences("other") { + if !s.referencesCheckpoint("other-replica") { t.Fatal("checkpoint references crossed ownership") } if err := s.commit(s.mark()); err != nil { t.Fatal(err) } - if s.hasCheckpointReferences(sb.ID) || len(s.checkpointReferences()) != 1 { + if s.referencesCheckpoint("uncertain-replica") || len(s.checkpointReferences()) != 1 { t.Fatal("acknowledgment did not retire only removed references") } } @@ -89,7 +89,7 @@ func TestClaimCapturedCheckpointReferenceSurvivesConcurrentClear(t *testing.T) { if err := s.commit(s.mark()); err != nil { t.Fatal(err) } - if s.hasCheckpointReferences(sb.ID) { + if s.referencesCheckpoint("captured-replica") { t.Fatal("later acknowledgment retained obsolete checkpoint") } } @@ -123,7 +123,7 @@ func TestClaimCheckpointReferencesSurviveFailedStartupRewrite(t *testing.T) { if err = s.save(claims); err != nil { t.Fatal(err) } - if s.hasCheckpointReferences(sb.ID) || s.referencesCheckpoint("before-recovery") { + if s.referencesCheckpoint("before-recovery") { t.Fatal("successful startup rewrite retained old checkpoint") } }) @@ -137,6 +137,13 @@ func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { if err := s.commit(s.set(first, second)); err != nil { t.Fatal(err) } + first.ReplicaCk = "" + if err := s.commit(s.set(first)); err != nil { + t.Fatal(err) + } + if !s.referencesCheckpoint("shared") { + t.Fatal("clearing one duplicate slot lost shared checkpoint") + } s.del(first.ID) if !s.referencesCheckpoint("shared") { t.Fatal("memory deletion lost shared checkpoint") @@ -144,7 +151,7 @@ func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { if err := s.commit(s.mark()); err != nil { t.Fatal(err) } - if !s.referencesCheckpoint("shared") || s.hasCheckpointReferences(first.ID) || !s.hasCheckpointReferences(second.ID) { + if !s.referencesCheckpoint("shared") { t.Fatal("one owner's retirement changed another owner's reference") } if err := s.commit(s.del(second.ID)); err != nil { @@ -162,20 +169,28 @@ func TestClaimReferenceChangesStayDirtyAfterFailedReturnToOriginal(t *testing.T) t.Fatal(err) } restore := failClaimWrites(t, s) - sb.ReplicaCk = "attempted" - if err := s.commit(s.set(sb)); err == nil { - t.Fatal("expected write failure") + attempted := []string{"attempt-1", "attempt-2", "attempt-3", "attempt-4", "attempt-5"} + for _, ck := range attempted { + sb.ReplicaCk = ck + if err := s.commit(s.set(sb)); err == nil { + t.Fatal("expected write failure") + } } sb.ReplicaCk = "original" s.set(sb) - if !s.referencesCheckpoint("original") || !s.referencesCheckpoint("attempted") { + if !s.referencesCheckpoint("original") || len(s.checkpointReferences()) != len(attempted)+1 { t.Fatal("failed return lost a possible snapshot") } + for _, ck := range attempted { + if !s.referencesCheckpoint(ck) { + t.Fatalf("failed return lost attempted checkpoint %s", ck) + } + } restore() if err := s.commit(s.mark()); err != nil { t.Fatal(err) } - if !s.referencesCheckpoint("original") || s.referencesCheckpoint("attempted") { + if !s.referencesCheckpoint("original") || len(s.checkpointReferences()) != 1 { t.Fatal("successful return did not retire failed attempt") } } @@ -262,7 +277,7 @@ func captureClaimWrite(t *testing.T, s *claimStore, snap claimSnapshot, owner st waitFor(t, func() bool { s.mu.Lock() defer s.mu.Unlock() - return !s.retained[owner].attempted.empty() + return len(s.retained[owner]) != 0 }) var raw []byte release := sync.OnceFunc(func() { diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index 2043cb05..cec1b869 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -68,43 +68,11 @@ type claimDTO struct { // claimRow keeps its encoded member until set replaces the row; set never edits one. type claimRow struct { - id string - refs *claimRefs + refs [4]string dto claimDTO raw []byte } -type claimRefs struct { - checkpoints []string -} - -func (r claimRefs) empty() bool { return len(r.checkpoints) == 0 } - -func (r claimRefs) matches(sb *types.Sandbox) bool { - checkpoints := [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} - for _, ck := range checkpoints { - if ck != "" && !slices.Contains(r.checkpoints, ck) { - return false - } - } - for _, ck := range r.checkpoints { - if !slices.Contains(checkpoints[:], ck) { - return false - } - } - return true -} - -type retainedClaimRefs struct { - acknowledged claimRefs - attempted claimRefs -} - -type referenceSnapshot struct { - id string - refs claimRefs -} - // claimStore persists claimed sandboxes across daemon restarts; warm VMs are not persisted. type claimStore struct { path string @@ -118,7 +86,7 @@ type claimStore struct { rows map[string]*claimRow seq uint64 written uint64 - retained map[string]retainedClaimRefs + retained map[string][]string changed map[string]struct{} checkpoints map[string]int @@ -128,14 +96,12 @@ type claimStore struct { func newClaimStore(dataDir string, sync bool) *claimStore { return &claimStore{ path: filepath.Join(dataDir, "claims.json"), sync: sync, - rows: map[string]*claimRow{}, retained: map[string]retainedClaimRefs{}, + rows: map[string]*claimRow{}, retained: map[string][]string{}, changed: map[string]struct{}{}, checkpoints: map[string]int{}, } } func (s *claimStore) load() (map[string]*types.Sandbox, error) { - s.writeMu.Lock() - defer s.writeMu.Unlock() raw, err := os.ReadFile(s.path) if errors.Is(err, fs.ErrNotExist) { return map[string]*types.Sandbox{}, nil @@ -153,17 +119,12 @@ func (s *claimStore) load() (map[string]*types.Sandbox, error) { } } s.mu.Lock() - for id, retained := range s.retained { - retained.acknowledged = claimRefs{} - s.changeRetainedRefs(id, retained) - s.changed[id] = struct{}{} - } for _, sb := range claims { - row := newClaimRow(sb, nil) - retained := s.retained[row.id] - retained.acknowledged = *row.refs - s.changeRetainedRefs(row.id, retained) - s.changed[row.id] = struct{}{} + refs := newClaimRow(sb).refs + if refs != [4]string{} { + s.changeRetainedRefs(sb.ID, refs[:]) + } + s.changed[sb.ID] = struct{}{} } s.mu.Unlock() return claims, nil @@ -174,8 +135,8 @@ func (s *claimStore) set(sbs ...*types.Sandbox) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, sb := range sbs { - row := newClaimRow(sb, s.rows[sb.ID]) - s.changeCurrentRefs(sb.ID, *row.refs) + row := newClaimRow(sb) + s.changeCurrentRefs(sb.ID, row.refs) s.rows[sb.ID] = row } s.seq++ @@ -187,7 +148,7 @@ func (s *claimStore) del(ids ...string) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, id := range ids { - s.changeCurrentRefs(id, claimRefs{}) + s.changeCurrentRefs(id, [4]string{}) delete(s.rows, id) } s.seq++ @@ -211,18 +172,15 @@ func (s *claimStore) pending(snap claimSnapshot) bool { func (s *claimStore) reset(claims map[string]*types.Sandbox) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() - for id := range s.retained { - s.changed[id] = struct{}{} - } for id := range s.rows { if _, ok := claims[id]; !ok { - s.changeCurrentRefs(id, claimRefs{}) + s.changeCurrentRefs(id, [4]string{}) delete(s.rows, id) } } for id, sb := range claims { - row := newClaimRow(sb, s.rows[id]) - s.changeCurrentRefs(id, *row.refs) + row := newClaimRow(sb) + s.changeCurrentRefs(id, row.refs) s.rows[id] = row } s.seq++ @@ -239,14 +197,17 @@ func (s *claimStore) commit(snap claimSnapshot) error { return nil } rows, seq := slices.AppendSeq(make([]*claimRow, 0, len(s.rows)), maps.Values(s.rows)), s.seq - captured := make([]referenceSnapshot, 0, len(s.changed)) + captured := make(map[string][]string, len(s.changed)) for id := range s.changed { refs := s.currentRefs(id) - captured = append(captured, referenceSnapshot{id: id, refs: refs}) - retained := s.retained[id] - for _, ck := range refs.checkpoints { - if !slices.Contains(retained.attempted.checkpoints, ck) { - retained.attempted.checkpoints = append(retained.attempted.checkpoints, ck) + captured[id] = nil + if refs != [4]string{} { + captured[id] = refs[:] + } + retained := slices.Clone(s.retained[id]) + for _, ck := range refs { + if ck != "" && !slices.Contains(retained, ck) { + retained = append(retained, ck) } } s.changeRetainedRefs(id, retained) @@ -271,10 +232,12 @@ func (s *claimStore) commit(snap claimSnapshot) error { return err } s.mu.Lock() - for _, snapshot := range captured { - s.changeRetainedRefs(snapshot.id, retainedClaimRefs{acknowledged: snapshot.refs}) - if sameClaimRefs(s.currentRefs(snapshot.id), snapshot.refs) { - delete(s.changed, snapshot.id) + for id, refs := range captured { + s.changeRetainedRefs(id, refs) + var slots [4]string + copy(slots[:], refs) + if s.currentRefs(id) == slots { + delete(s.changed, id) } } s.written = seq @@ -338,25 +301,28 @@ func (s *claimStore) synced() bool { return s.written == s.seq } -func (s *claimStore) currentRefs(id string) claimRefs { +func (s *claimStore) currentRefs(id string) [4]string { if row := s.rows[id]; row != nil { - return *row.refs + return row.refs } - return claimRefs{} + return [4]string{} } -func (s *claimStore) changeCurrentRefs(id string, after claimRefs) { +func (s *claimStore) changeCurrentRefs(id string, after [4]string) { before := s.currentRefs(id) - if sameClaimRefs(before, after) { + if before == after { return } - s.changeCheckpointCounts(before, -1) - s.changeCheckpointCounts(after, 1) + s.changeCheckpointCounts(before[:], -1) + s.changeCheckpointCounts(after[:], 1) s.changed[id] = struct{}{} } -func (s *claimStore) changeCheckpointCounts(refs claimRefs, delta int) { - for _, ck := range refs.checkpoints { +func (s *claimStore) changeCheckpointCounts(refs []string, delta int) { + for _, ck := range refs { + if ck == "" { + continue + } s.checkpoints[ck] += delta if s.checkpoints[ck] == 0 { delete(s.checkpoints, ck) @@ -364,15 +330,10 @@ func (s *claimStore) changeCheckpointCounts(refs claimRefs, delta int) { } } -func (s *claimStore) changeRetainedRefs(id string, after retainedClaimRefs) { - before := s.retained[id] - for i, retained := range [2]retainedClaimRefs{before, after} { - delta := 2*i - 1 - for _, refs := range [2]claimRefs{retained.acknowledged, retained.attempted} { - s.changeCheckpointCounts(refs, delta) - } - } - if after.acknowledged.empty() && after.attempted.empty() { +func (s *claimStore) changeRetainedRefs(id string, after []string) { + s.changeCheckpointCounts(s.retained[id], -1) + s.changeCheckpointCounts(after, 1) + if len(after) == 0 { delete(s.retained, id) } else { s.retained[id] = after @@ -395,13 +356,6 @@ func (s *claimStore) checkpointReferences() map[string]struct{} { return refs } -func (s *claimStore) hasCheckpointReferences(owner string) bool { - s.mu.Lock() - defer s.mu.Unlock() - retained := s.retained[owner] - return len(s.currentRefs(owner).checkpoints) != 0 || len(retained.acknowledged.checkpoints) != 0 || len(retained.attempted.checkpoints) != 0 -} - func encodeRow(dto *claimDTO) ([]byte, error) { raw, err := json.Marshal(map[string]*claimDTO{dto.ID: dto}) if err != nil { @@ -431,19 +385,6 @@ func volumesWantDTO(sb *types.Sandbox) []types.Volume { return append([]types.Volume{}, sb.VolumesWant...) } -func newClaimRow(sb *types.Sandbox, previous *claimRow) *claimRow { - if previous != nil && previous.refs.matches(sb) { - return &claimRow{id: sb.ID, refs: previous.refs, dto: dtoOf(sb)} - } - refs := &claimRefs{} - for _, id := range [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} { - if id != "" && !slices.Contains(refs.checkpoints, id) { - refs.checkpoints = append(refs.checkpoints, id) - } - } - return &claimRow{id: sb.ID, refs: refs, dto: dtoOf(sb)} -} - -func sameClaimRefs(a, b claimRefs) bool { - return slices.Equal(a.checkpoints, b.checkpoints) +func newClaimRow(sb *types.Sandbox) *claimRow { + return &claimRow{refs: [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending}, dto: dtoOf(sb)} } diff --git a/sandboxd/pool/claims_test.go b/sandboxd/pool/claims_test.go index 518ee3e9..020a9ecc 100644 --- a/sandboxd/pool/claims_test.go +++ b/sandboxd/pool/claims_test.go @@ -64,7 +64,7 @@ func TestRecommitDrainsAChangeItWasTooBusyToSee(t *testing.T) { waitFor(t, func() bool { m.store.mu.Lock() defer m.store.mu.Unlock() - return slices.Contains(m.store.retained[sb.ID].attempted.checkpoints, "first") + return slices.Contains(m.store.retained[sb.ID], "first") }) sb.ReplicaPending = "second" m.store.recommit(t.Context(), change()) @@ -103,7 +103,7 @@ func TestStoreRoundTrip(t *testing.T) { if err := s.save(claims); err != nil { t.Fatalf("save (sync=%v): %v", fsync, err) } - got, err := s.load() + got, err := newClaimStore(filepath.Dir(s.path), false).load() if err != nil { t.Fatalf("load (sync=%v): %v", fsync, err) } @@ -124,7 +124,7 @@ func TestClaimStoreSnapshotDetachesVolumes(t *testing.T) { if err := s.commit(snap); err != nil { t.Fatalf("commit: %v", err) } - got, err := s.load() + got, err := newClaimStore(filepath.Dir(s.path), false).load() if err != nil { t.Fatalf("load: %v", err) } @@ -180,7 +180,7 @@ func TestClaimStoreCommitCoalesces(t *testing.T) { if err := s.commit(older); err != nil { t.Fatalf("commit older: %v", err) } - if got, err := s.load(); err != nil || len(got) != 2 { + if got, err := newClaimStore(filepath.Dir(s.path), false).load(); err != nil || len(got) != 2 { t.Errorf("stale snapshot overwrote the newer state: %d claims (%v), want 2", len(got), err) } } @@ -196,7 +196,7 @@ func TestClaimStoreIncrementalProjection(t *testing.T) { if err := s.commit(s.set(a)); err != nil { t.Fatalf("commit update: %v", err) } - got, err := s.load() + got, err := newClaimStore(filepath.Dir(s.path), false).load() if err != nil { t.Fatalf("load: %v", err) } @@ -206,13 +206,13 @@ func TestClaimStoreIncrementalProjection(t *testing.T) { if delErr := s.commit(s.del("sb_a")); delErr != nil { t.Fatalf("commit del: %v", delErr) } - if got, err = s.load(); err != nil || len(got) != 1 || got["sb_b"] == nil { + if got, err = newClaimStore(filepath.Dir(s.path), false).load(); err != nil || len(got) != 1 || got["sb_b"] == nil { t.Fatalf("after del: %+v, %v", got, err) } if resetErr := s.commit(s.reset(map[string]*types.Sandbox{"sb_c": {ID: "sb_c", Key: testKey}})); resetErr != nil { t.Fatalf("commit reset: %v", resetErr) } - if got, err = s.load(); err != nil || len(got) != 1 || got["sb_c"] == nil { + if got, err = newClaimStore(filepath.Dir(s.path), false).load(); err != nil || len(got) != 1 || got["sb_c"] == nil { t.Fatalf("after reset: %+v, %v", got, err) } } @@ -229,7 +229,7 @@ func TestClaimStoreMarkRepersists(t *testing.T) { if !s.synced() { t.Error("not synced after flushing the mark") } - if got, err := s.load(); err != nil || len(got) != 1 { + if got, err := newClaimStore(filepath.Dir(s.path), false).load(); err != nil || len(got) != 1 { t.Errorf("after mark flush: %+v, %v", got, err) } } @@ -242,7 +242,7 @@ func TestClaimStoreConcurrentCommits(t *testing.T) { wg.Go(func() { _ = s.commit(s.set(sb)) }) } wg.Wait() - if got, err := s.load(); err != nil || len(got) != 1 { + if got, err := newClaimStore(filepath.Dir(s.path), false).load(); err != nil || len(got) != 1 { t.Fatalf("after concurrent commits: %+v, %v", got, err) } } diff --git a/sandboxd/pool/failover_test.go b/sandboxd/pool/failover_test.go index 0adb7413..505dc55c 100644 --- a/sandboxd/pool/failover_test.go +++ b/sandboxd/pool/failover_test.go @@ -40,7 +40,7 @@ func TestFailoverArchiveWritesTheRow(t *testing.T) { if err = json.Unmarshal(rec.Claim, claim); err != nil || claim.ID != sb.ID || claim.Token != "" || claim.VMName != "" || claim.ArchiveCk != sb.ArchiveCk { t.Errorf("row claim %+v %v, want the archived record without token or VM", claim, err) } - stored, err := a.store.load() + stored, err := newClaimStore(a.dataDir, false).load() if err != nil || stored[sb.ID].ArchiveEpoch != 1 { t.Errorf("claims.json epoch %v %v, want 1", stored[sb.ID], err) } @@ -1492,7 +1492,7 @@ func TestFailoverArchivePersistsItsIntentBeforeThePut(t *testing.T) { } var intent string a.archives = &hookRegistry{Registry: c.reg, beforePut: func() { - stored, err := a.store.load() + stored, err := newClaimStore(a.dataDir, false).load() if err != nil { t.Errorf("load: %v", err) return @@ -1502,7 +1502,7 @@ func TestFailoverArchivePersistsItsIntentBeforeThePut(t *testing.T) { if err := a.archive(t.Context(), sb); err != nil { t.Fatalf("archive: %v", err) } - stored, err := a.store.load() + stored, err := newClaimStore(a.dataDir, false).load() if err != nil { t.Fatalf("load: %v", err) } diff --git a/sandboxd/pool/filesystem_fault_test.go b/sandboxd/pool/filesystem_fault_test.go deleted file mode 100644 index 7ddaa831..00000000 --- a/sandboxd/pool/filesystem_fault_test.go +++ /dev/null @@ -1,40 +0,0 @@ -package pool - -import ( - "os" - "os/exec" - "regexp" - "runtime" - "strings" - "testing" -) - -func withoutDACOverride(t *testing.T) bool { - t.Helper() - if os.Geteuid() != 0 || os.Getenv("SANDBOX_TEST_WITHOUT_DAC") == t.Name() { - return false - } - if runtime.GOOS != "linux" { - t.Skip("post-rename permission fixture requires non-root or Linux setpriv") - } - setpriv, err := exec.LookPath("setpriv") - if err != nil { - t.Skip("post-rename permission fixture requires setpriv to drop root DAC capabilities") - } - binary, err := os.Executable() - if err != nil { - t.Fatal(err) - } - parts := strings.Split(t.Name(), "/") - for i, part := range parts { - parts[i] = "^" + regexp.QuoteMeta(part) + "$" - } - cmd := exec.Command(setpriv, "--bounding-set=-dac_override,-dac_read_search", binary, "-test.run="+strings.Join(parts, "/"), "-test.v") - cmd.Env = append(os.Environ(), "SANDBOX_TEST_WITHOUT_DAC="+t.Name()) - output, err := cmd.CombinedOutput() - if err != nil { - t.Fatalf("test without DAC override: %v\n%s", err, output) - } - t.Logf("test without DAC override:\n%s", output) - return true -} diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index bce9f8e5..a21503f8 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1950,7 +1950,7 @@ func newTestManagerAt(t testing.TB, eng *fakeEngine, dataDir string, pools ...co defer m.store.mu.Unlock() for id, sb := range m.claimed { row := m.store.rows[id] - if row == nil || !row.refs.matches(sb) { + if row == nil || row.refs != [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} { t.Errorf("claim %s references differ from its store row", id) } } diff --git a/sandboxd/pool/replica_test.go b/sandboxd/pool/replica_test.go index bdef433b..0a157b61 100644 --- a/sandboxd/pool/replica_test.go +++ b/sandboxd/pool/replica_test.go @@ -32,7 +32,7 @@ func TestReplicaRoundPublishesMovesTheRowAndDropsThePrevious(t *testing.T) { if first == "" || rec.Owner != "a" || rec.Checkpoint != first || rec.Consumed || rec.TokenSHA256 != tenants.TokenSHA256(sb.Token) || !rec.Deadline.IsZero() { t.Fatalf("row %+v with replica %q, want a live row of a on the replica with the token hash and no deadline", rec, first) } - if stored, loadErr := a.store.load(); loadErr != nil || stored[sb.ID].ReplicaCk != first || stored[sb.ID].ArchiveEpoch != rec.Epoch { + if stored, loadErr := newClaimStore(a.dataDir, false).load(); loadErr != nil || stored[sb.ID].ReplicaCk != first || stored[sb.ID].ArchiveEpoch != rec.Epoch { t.Errorf("claims.json replica %q epoch %d (%v), want %q at %d", stored[sb.ID].ReplicaCk, stored[sb.ID].ArchiveEpoch, loadErr, first, rec.Epoch) } meta, err := a.loadCheckpoint(t.Context(), first) diff --git a/sandboxd/pool/volume_mutation_test.go b/sandboxd/pool/volume_mutation_test.go index ce30fd1a..4f94b749 100644 --- a/sandboxd/pool/volume_mutation_test.go +++ b/sandboxd/pool/volume_mutation_test.go @@ -287,7 +287,7 @@ func TestLiveVolumeDoesNotBlockLifecycle(t *testing.T) { if attachErr == nil { t.Fatal("released worker reported success") } - claims, err := m.store.load() + claims, err := newClaimStore(m.dataDir, false).load() if err != nil || claims[sb.ID] != nil { t.Fatalf("late worker resurrected claim: %v %v", claims, err) } @@ -547,7 +547,7 @@ func TestReleaseOwnsUncertainHoldsUntilVMRemoval(t *testing.T) { if volumeHoldersOf(m, "data").readers != 1 { t.Fatal("late writer freed teardown-owned hold") } - claims, err := m.store.load() + claims, err := newClaimStore(m.dataDir, false).load() if err != nil || claims[sb.ID] != nil { t.Fatalf("late writer resurrected claim: %v %v", claims, err) } From 5315c8b42c1c2bc6206f9f39697dc0d59ebc8d94 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 03:06:00 +0000 Subject: [PATCH 06/15] pool: avoid shadowing the retirement lookup result --- sandboxd/pool/checkpoint_cleanup_test.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/sandboxd/pool/checkpoint_cleanup_test.go b/sandboxd/pool/checkpoint_cleanup_test.go index d543dd37..c0566c13 100644 --- a/sandboxd/pool/checkpoint_cleanup_test.go +++ b/sandboxd/pool/checkpoint_cleanup_test.go @@ -72,8 +72,8 @@ func TestReplicaRetirementKeepsTombstoneUntilClaimCommit(t *testing.T) { if err = m.Reconcile(t.Context()); err != nil { t.Fatal(err) } - restored, ok := m.byID(sb.ID) - if !ok { + restored, present := m.byID(sb.ID) + if !present { t.Fatal("restart dropped a claim after failed retirement commit") } m.retireReplica(t.Context(), restored) From a0ca583e5f5813608f43dad212254d3b15ef1b4b Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:03:04 +0000 Subject: [PATCH 07/15] pool: keep checkpoint references out of claim rows A [4]string on every claimRow made each row 64 bytes larger, although most claims hold no checkpoint. The store now keeps current references in its own map, holding only claims that have one, so rows stay at main's size. checkpointRefs replaces newClaimRow and is the one place that lists the four checkpoint fields; load and the test manager's consistency check use it too. --- sandboxd/pool/claims.go | 34 +++++++++++++++++----------------- sandboxd/pool/pool_test.go | 3 +-- 2 files changed, 18 insertions(+), 19 deletions(-) diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index cec1b869..7595f7d9 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -68,9 +68,8 @@ type claimDTO struct { // claimRow keeps its encoded member until set replaces the row; set never edits one. type claimRow struct { - refs [4]string - dto claimDTO - raw []byte + dto claimDTO + raw []byte } // claimStore persists claimed sandboxes across daemon restarts; warm VMs are not persisted. @@ -86,6 +85,7 @@ type claimStore struct { rows map[string]*claimRow seq uint64 written uint64 + refs map[string][4]string retained map[string][]string changed map[string]struct{} checkpoints map[string]int @@ -96,7 +96,7 @@ type claimStore struct { func newClaimStore(dataDir string, sync bool) *claimStore { return &claimStore{ path: filepath.Join(dataDir, "claims.json"), sync: sync, - rows: map[string]*claimRow{}, retained: map[string][]string{}, + rows: map[string]*claimRow{}, refs: map[string][4]string{}, retained: map[string][]string{}, changed: map[string]struct{}{}, checkpoints: map[string]int{}, } } @@ -120,7 +120,7 @@ func (s *claimStore) load() (map[string]*types.Sandbox, error) { } s.mu.Lock() for _, sb := range claims { - refs := newClaimRow(sb).refs + refs := checkpointRefs(sb) if refs != [4]string{} { s.changeRetainedRefs(sb.ID, refs[:]) } @@ -135,9 +135,8 @@ func (s *claimStore) set(sbs ...*types.Sandbox) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, sb := range sbs { - row := newClaimRow(sb) - s.changeCurrentRefs(sb.ID, row.refs) - s.rows[sb.ID] = row + s.changeCurrentRefs(sb.ID, checkpointRefs(sb)) + s.rows[sb.ID] = &claimRow{dto: dtoOf(sb)} } s.seq++ return claimSnapshot{seq: s.seq} @@ -179,9 +178,8 @@ func (s *claimStore) reset(claims map[string]*types.Sandbox) claimSnapshot { } } for id, sb := range claims { - row := newClaimRow(sb) - s.changeCurrentRefs(id, row.refs) - s.rows[id] = row + s.changeCurrentRefs(id, checkpointRefs(sb)) + s.rows[id] = &claimRow{dto: dtoOf(sb)} } s.seq++ return claimSnapshot{seq: s.seq} @@ -302,10 +300,7 @@ func (s *claimStore) synced() bool { } func (s *claimStore) currentRefs(id string) [4]string { - if row := s.rows[id]; row != nil { - return row.refs - } - return [4]string{} + return s.refs[id] } func (s *claimStore) changeCurrentRefs(id string, after [4]string) { @@ -315,6 +310,11 @@ func (s *claimStore) changeCurrentRefs(id string, after [4]string) { } s.changeCheckpointCounts(before[:], -1) s.changeCheckpointCounts(after[:], 1) + if after == [4]string{} { + delete(s.refs, id) + } else { + s.refs[id] = after + } s.changed[id] = struct{}{} } @@ -385,6 +385,6 @@ func volumesWantDTO(sb *types.Sandbox) []types.Volume { return append([]types.Volume{}, sb.VolumesWant...) } -func newClaimRow(sb *types.Sandbox) *claimRow { - return &claimRow{refs: [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending}, dto: dtoOf(sb)} +func checkpointRefs(sb *types.Sandbox) [4]string { + return [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} } diff --git a/sandboxd/pool/pool_test.go b/sandboxd/pool/pool_test.go index a21503f8..56e4148f 100644 --- a/sandboxd/pool/pool_test.go +++ b/sandboxd/pool/pool_test.go @@ -1949,8 +1949,7 @@ func newTestManagerAt(t testing.TB, eng *fakeEngine, dataDir string, pools ...co m.store.mu.Lock() defer m.store.mu.Unlock() for id, sb := range m.claimed { - row := m.store.rows[id] - if row == nil || row.refs != [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} { + if m.store.rows[id] == nil || m.store.refs[id] != checkpointRefs(sb) { t.Errorf("claim %s references differ from its store row", id) } } From 2a9c5a3b8668f899e0c965da827c9d935491b401 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:04:44 +0000 Subject: [PATCH 08/15] pool: give the claim store one write path set, del and reset each moved the reference counts and then replaced or dropped the row, and the two steps had to stay in that order. put does both, with a nil sandbox meaning delete, so a caller cannot get the order wrong or update one without the other. load now reads references in the same loop that fills VolumesWant, and records and marks only the claims that have some. A claim without references had nothing to retain, so marking it changed was a no-op. commit captures each changed claim's references as a [4]string and compares them with ==, so the copy back into fixed slots goes. appendRefs builds both the attempted and the written retained lists. --- sandboxd/pool/claims.go | 98 +++++++++++++++++++---------------------- 1 file changed, 46 insertions(+), 52 deletions(-) diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index 7595f7d9..061e1c50 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -113,20 +113,17 @@ func (s *claimStore) load() (map[string]*types.Sandbox, error) { if err := json.Unmarshal(raw, &claims); err != nil { return nil, fmt.Errorf("parse claims: %w", err) } + s.mu.Lock() + defer s.mu.Unlock() for _, sb := range claims { if sb.VolumesWant == nil { sb.VolumesWant = sb.Volumes } - } - s.mu.Lock() - for _, sb := range claims { - refs := checkpointRefs(sb) - if refs != [4]string{} { - s.changeRetainedRefs(sb.ID, refs[:]) + if refs := checkpointRefs(sb); refs != [4]string{} { + s.changeRetainedRefs(sb.ID, appendRefs(nil, refs)) + s.changed[sb.ID] = struct{}{} } - s.changed[sb.ID] = struct{}{} } - s.mu.Unlock() return claims, nil } @@ -135,8 +132,7 @@ func (s *claimStore) set(sbs ...*types.Sandbox) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, sb := range sbs { - s.changeCurrentRefs(sb.ID, checkpointRefs(sb)) - s.rows[sb.ID] = &claimRow{dto: dtoOf(sb)} + s.put(sb.ID, sb) } s.seq++ return claimSnapshot{seq: s.seq} @@ -147,13 +143,35 @@ func (s *claimStore) del(ids ...string) claimSnapshot { s.mu.Lock() defer s.mu.Unlock() for _, id := range ids { - s.changeCurrentRefs(id, [4]string{}) - delete(s.rows, id) + s.put(id, nil) } s.seq++ return claimSnapshot{seq: s.seq} } +// put replaces id's row with sb's projection, or drops it when sb is nil, and moves the checkpoint counts with it. +func (s *claimStore) put(id string, sb *types.Sandbox) { + var refs [4]string + if sb == nil { + delete(s.rows, id) + } else { + refs = checkpointRefs(sb) + s.rows[id] = &claimRow{dto: dtoOf(sb)} + } + before := s.refs[id] + if before == refs { + return + } + s.changeCheckpointCounts(before[:], -1) + s.changeCheckpointCounts(refs[:], 1) + if refs == [4]string{} { + delete(s.refs, id) + } else { + s.refs[id] = refs + } + s.changed[id] = struct{}{} +} + // mark sequences a persist of the projection as it already stands. func (s *claimStore) mark() claimSnapshot { s.mu.Lock() @@ -173,13 +191,11 @@ func (s *claimStore) reset(claims map[string]*types.Sandbox) claimSnapshot { defer s.mu.Unlock() for id := range s.rows { if _, ok := claims[id]; !ok { - s.changeCurrentRefs(id, [4]string{}) - delete(s.rows, id) + s.put(id, nil) } } for id, sb := range claims { - s.changeCurrentRefs(id, checkpointRefs(sb)) - s.rows[id] = &claimRow{dto: dtoOf(sb)} + s.put(id, sb) } s.seq++ return claimSnapshot{seq: s.seq} @@ -195,20 +211,10 @@ func (s *claimStore) commit(snap claimSnapshot) error { return nil } rows, seq := slices.AppendSeq(make([]*claimRow, 0, len(s.rows)), maps.Values(s.rows)), s.seq - captured := make(map[string][]string, len(s.changed)) + captured := make(map[string][4]string, len(s.changed)) for id := range s.changed { - refs := s.currentRefs(id) - captured[id] = nil - if refs != [4]string{} { - captured[id] = refs[:] - } - retained := slices.Clone(s.retained[id]) - for _, ck := range refs { - if ck != "" && !slices.Contains(retained, ck) { - retained = append(retained, ck) - } - } - s.changeRetainedRefs(id, retained) + captured[id] = s.refs[id] + s.changeRetainedRefs(id, appendRefs(s.retained[id], s.refs[id])) } s.mu.Unlock() s.buf = append(s.buf[:0], '{') @@ -231,10 +237,8 @@ func (s *claimStore) commit(snap claimSnapshot) error { } s.mu.Lock() for id, refs := range captured { - s.changeRetainedRefs(id, refs) - var slots [4]string - copy(slots[:], refs) - if s.currentRefs(id) == slots { + s.changeRetainedRefs(id, appendRefs(nil, refs)) + if s.refs[id] == refs { delete(s.changed, id) } } @@ -299,25 +303,6 @@ func (s *claimStore) synced() bool { return s.written == s.seq } -func (s *claimStore) currentRefs(id string) [4]string { - return s.refs[id] -} - -func (s *claimStore) changeCurrentRefs(id string, after [4]string) { - before := s.currentRefs(id) - if before == after { - return - } - s.changeCheckpointCounts(before[:], -1) - s.changeCheckpointCounts(after[:], 1) - if after == [4]string{} { - delete(s.refs, id) - } else { - s.refs[id] = after - } - s.changed[id] = struct{}{} -} - func (s *claimStore) changeCheckpointCounts(refs []string, delta int) { for _, ck := range refs { if ck == "" { @@ -388,3 +373,12 @@ func volumesWantDTO(sb *types.Sandbox) []types.Volume { func checkpointRefs(sb *types.Sandbox) [4]string { return [4]string{sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending} } + +func appendRefs(list []string, refs [4]string) []string { + for _, ck := range refs { + if ck != "" && !slices.Contains(list, ck) { + list = append(list, ck) + } + } + return list +} From ac9c2506e87068bc82b455e321f7a33fc550063d Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:05:38 +0000 Subject: [PATCH 09/15] pool: return the persist error when a replica intent clear fails replicate joined the clear error into the error it returned. When the claim had gone, the clear returned ErrUnknownSandbox, and benignSweepErr then treated the joined error as benign, so the sweep hid the persist or registry failure. replicate returns main's error again and ignores the clear result; settleReplicaIntent still returns it. --- sandboxd/pool/replica.go | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/sandboxd/pool/replica.go b/sandboxd/pool/replica.go index 8cad931e..6524154d 100644 --- a/sandboxd/pool/replica.go +++ b/sandboxd/pool/replica.go @@ -124,17 +124,17 @@ func (m *Manager) replicate(ctx context.Context, sb *types.Sandbox) error { return errWokeMeanwhile } if err := m.store.commit(intent); err != nil { - clearErr := m.clearReplicaIntent(ctx, sb, ckID) - return fmt.Errorf("replicate %s: persist intent: %w", sb.ID, errors.Join(err, clearErr)) + _ = m.clearReplicaIntent(ctx, sb, ckID) + return fmt.Errorf("replicate %s: persist intent: %w", sb.ID, err) } epoch, err := m.replaceReplica(ctx, sb, ckID) switch { case errors.Is(err, archives.ErrLost): - clearErr := m.clearReplicaIntent(ctx, sb, ckID) + _ = m.clearReplicaIntent(ctx, sb, ckID) if settleErr := m.refusedReplace(ctx, sb); settleErr != nil && !errors.Is(settleErr, ErrUnknownSandbox) { return fmt.Errorf("replicate %s: settle refused write: %w", sb.ID, settleErr) } - return fmt.Errorf("replicate %s: registry: %w", sb.ID, errors.Join(err, clearErr)) + return fmt.Errorf("replicate %s: registry: %w", sb.ID, err) case err != nil: return fmt.Errorf("replicate %s: registry: %w", sb.ID, err) } From 975dd62ca4ff35dbd0d0c71fe413e838c1f31e02 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:08:49 +0000 Subject: [PATCH 10/15] pool: table the checkpoint reference fault tests The capture, failed-write and post-rename tests ran one sequence: write under a fault, change the claim, check what stays pinned, commit, check again. They are now one table of {acked, next or startup rewrite, fault, mutation, on disk, pinned after the fault, pinned after the commit}. The on-disk column shows that every checkpoint claims.json names stays pinned. TestClaimReferencesSurviveRenameFailure goes: it reached the same failure branch as failClaimWrites. So does the assertion that mutated the checkpointReferences result to prove it is a copy. The shared reference test keeps its two checks that can fail; the others ran while the second owner still held the checkpoint. TestRecommitDrainsAChangeItWasTooBusyToSee is back to main's body; the table covers the snapshot and reference checks it had gained. --- sandboxd/pool/checkpoint_references_test.go | 405 ++++++++------------ sandboxd/pool/claims_test.go | 27 +- 2 files changed, 162 insertions(+), 270 deletions(-) diff --git a/sandboxd/pool/checkpoint_references_test.go b/sandboxd/pool/checkpoint_references_test.go index 9f388288..27801188 100644 --- a/sandboxd/pool/checkpoint_references_test.go +++ b/sandboxd/pool/checkpoint_references_test.go @@ -1,13 +1,12 @@ package pool import ( - "encoding/json/v2" "errors" - "fmt" "io" "io/fs" + "maps" "os" - "path/filepath" + "slices" "sync" "syscall" "testing" @@ -15,239 +14,182 @@ import ( "github.com/cocoonstack/sandbox/sandboxd/types" ) -func TestClaimCheckpointReferencesTrackPossibleSnapshots(t *testing.T) { - if os.Geteuid() == 0 { - t.Skip("requires non-root: root can read the write-only directory") - } - dir := t.TempDir() - s := newClaimStore(dir, true) - sb := &types.Sandbox{ID: "owner", ArchiveCk: "archive", ArchivePending: "archive-pending", ReplicaCk: "replica", ReplicaPending: "replica-pending", FromCheckpoint: "branch-origin"} - snap := s.set(sb) - if refs := s.checkpointReferences(); len(refs) != 4 { - t.Fatalf("current references: %+v", refs) - } - if err := s.commit(snap); err != nil { - t.Fatal(err) - } - sb.ReplicaPending = "uncertain-replica" - if err := os.Chmod(dir, 0o300); err != nil { - t.Fatal(err) - } - t.Cleanup(func() { _ = os.Chmod(dir, 0o700) }) - err := s.commit(s.set(sb)) - if !errors.Is(err, fs.ErrPermission) { - t.Fatalf("expected post-rename permission failure: %v", err) - } - if err = os.Chmod(dir, 0o700); err != nil { - t.Fatal(err) - } - loaded, err := newClaimStore(dir, false).load() - if err != nil || loaded[sb.ID].ReplicaPending != "uncertain-replica" { - t.Fatalf("post-rename file: %+v, %v", loaded, err) - } - sb.ArchiveCk, sb.ArchivePending, sb.ReplicaCk, sb.ReplicaPending = "", "", "", "" - s.set(sb) - refs := s.checkpointReferences() - if len(refs) != 5 { - t.Fatalf("lost snapshot references: %+v", refs) - } - delete(refs, "uncertain-replica") - if !s.referencesCheckpoint("uncertain-replica") { - t.Fatal("caller mutated retained reference set") - } - s.set(&types.Sandbox{ID: "other", ReplicaPending: "other-replica"}) - if !s.referencesCheckpoint("other-replica") { - t.Fatal("checkpoint references crossed ownership") - } - if err := s.commit(s.mark()); err != nil { - t.Fatal(err) - } - if s.referencesCheckpoint("uncertain-replica") || len(s.checkpointReferences()) != 1 { - t.Fatal("acknowledgment did not retire only removed references") - } -} - -func TestClaimCapturedCheckpointReferenceSurvivesConcurrentClear(t *testing.T) { - s := newClaimStore(t.TempDir(), false) - sb := &types.Sandbox{ID: "owner", ReplicaPending: "captured-replica"} - release := captureClaimWrite(t, s, s.set(sb), sb.ID) - sb.ReplicaPending = "" - s.set(sb) - if !s.referencesCheckpoint("captured-replica") { - t.Fatal("captured reference disappeared before acknowledgment") - } - var rows map[string]*types.Sandbox - if err := json.Unmarshal(release(), &rows); err != nil { - t.Fatal(err) - } - if rows[sb.ID].ReplicaPending != "captured-replica" { - t.Fatal("captured write lost its original checkpoint") - } - if !s.referencesCheckpoint("captured-replica") { - t.Fatal("acknowledgment lost captured checkpoint") - } - if err := s.commit(s.mark()); err != nil { - t.Fatal(err) - } - if s.referencesCheckpoint("captured-replica") { - t.Fatal("later acknowledgment retained obsolete checkpoint") - } -} +const ( + failWrite = iota + captureWrite + failAfterRename +) -func TestClaimCheckpointReferencesSurviveFailedStartupRewrite(t *testing.T) { - for _, omit := range []bool{false, true} { - t.Run(fmt.Sprint(omit), func(t *testing.T) { - s := newClaimStore(t.TempDir(), false) - sb := &types.Sandbox{ID: "owner", ReplicaPending: "before-recovery"} - if err := s.commit(s.set(sb)); err != nil { - t.Fatal(err) +func TestClaimCheckpointReferencesOutliveUnacknowledgedWrites(t *testing.T) { + recreate := func(s *claimStore) { s.set(&types.Sandbox{ID: "owner", ReplicaCk: "recreated"}) } + for _, tc := range []struct { + name string + fault int + acked, next *types.Sandbox + startup func(claims map[string]*types.Sandbox) + mutate func(s *claimStore) + onDisk, afterFault, afterCommit []string + }{ + { + name: "captured write then clear", fault: captureWrite, + next: &types.Sandbox{ID: "owner", ReplicaPending: "captured"}, + mutate: func(s *claimStore) { s.set(&types.Sandbox{ID: "owner"}) }, + onDisk: []string{"captured"}, afterFault: []string{"captured"}, + }, + { + name: "captured write then delete and recreate", fault: captureWrite, + next: &types.Sandbox{ID: "owner", ReplicaCk: "captured"}, + mutate: func(s *claimStore) { s.del("owner"); recreate(s) }, + onDisk: []string{"captured"}, afterFault: []string{"captured", "recreated"}, afterCommit: []string{"recreated"}, + }, + { + name: "captured write then reset and recreate", fault: captureWrite, + next: &types.Sandbox{ID: "owner", ReplicaCk: "captured"}, + mutate: func(s *claimStore) { s.reset(map[string]*types.Sandbox{}); recreate(s) }, + onDisk: []string{"captured"}, afterFault: []string{"captured", "recreated"}, afterCommit: []string{"recreated"}, + }, + { + name: "failed startup rewrite clears the loaded reference", fault: failWrite, + acked: &types.Sandbox{ID: "owner", ReplicaPending: "loaded"}, + startup: func(claims map[string]*types.Sandbox) { claims["owner"].ReplicaPending = "" }, + onDisk: []string{"loaded"}, afterFault: []string{"loaded"}, + }, + { + name: "failed startup rewrite drops the loaded claim", fault: failWrite, + acked: &types.Sandbox{ID: "owner", ReplicaPending: "loaded"}, + startup: func(claims map[string]*types.Sandbox) { delete(claims, "owner") }, + onDisk: []string{"loaded"}, afterFault: []string{"loaded"}, + }, + { + name: "failed writes then a return to the original", fault: failWrite, + acked: &types.Sandbox{ID: "owner", ReplicaCk: "original"}, + next: &types.Sandbox{ID: "owner", ReplicaCk: "attempt-1"}, + mutate: func(s *claimStore) { + for _, ck := range []string{"attempt-2", "attempt-3", "attempt-4"} { + _ = s.commit(s.set(&types.Sandbox{ID: "owner", ReplicaCk: ck})) + } + s.set(&types.Sandbox{ID: "owner", ReplicaCk: "original"}) + }, + onDisk: []string{"original"}, + afterFault: []string{"attempt-1", "attempt-2", "attempt-3", "attempt-4", "original"}, + afterCommit: []string{"original"}, + }, + { + name: "write fails after the rename", fault: failAfterRename, + acked: &types.Sandbox{ID: "owner", ArchiveCk: "archive", ArchivePending: "archive-pending", ReplicaCk: "replica", ReplicaPending: "replica-pending", FromCheckpoint: "branch-origin"}, + next: &types.Sandbox{ID: "owner", ReplicaPending: "uncertain"}, + mutate: func(s *claimStore) { + s.set(&types.Sandbox{ID: "owner"}, &types.Sandbox{ID: "other", ReplicaPending: "other-replica"}) + }, + onDisk: []string{"uncertain"}, + afterFault: []string{"archive", "archive-pending", "other-replica", "replica", "replica-pending", "uncertain"}, + afterCommit: []string{"other-replica"}, + }, + } { + t.Run(tc.name, func(t *testing.T) { + if tc.fault == failAfterRename && os.Geteuid() == 0 { + t.Skip("requires non-root: root can read the write-only directory") } - s = newClaimStore(filepath.Dir(s.path), false) - claims, err := s.load() - if err != nil { - t.Fatal(err) + dir := t.TempDir() + s := newClaimStore(dir, tc.fault == failAfterRename) + if tc.acked != nil { + if err := s.commit(s.set(tc.acked)); err != nil { + t.Fatal(err) + } } - if omit { - delete(claims, sb.ID) + var snap claimSnapshot + if tc.startup != nil { + s = newClaimStore(dir, false) + claims, err := s.load() + if err != nil { + t.Fatal(err) + } + tc.startup(claims) + snap = s.reset(claims) } else { - claims[sb.ID].ReplicaPending = "" - } - restore := failClaimWrites(t, s) - if err = s.save(claims); err == nil { - t.Fatal("startup rewrite unexpectedly acknowledged") + snap = s.set(tc.next) } - if !s.referencesCheckpoint("before-recovery") { - t.Fatal("failed startup rewrite retired loaded checkpoint") + var finish func() + switch tc.fault { + case failWrite: + finish = failClaimWrites(t, s) + if err := s.commit(snap); err == nil { + t.Fatal("write succeeded under the fault") + } + case failAfterRename: + if err := os.Chmod(dir, 0o300); err != nil { + t.Fatal(err) + } + finish = func() { _ = os.Chmod(dir, 0o700) } + t.Cleanup(finish) + if err := s.commit(snap); !errors.Is(err, fs.ErrPermission) { + t.Fatalf("want a failed directory sync after the rename: %v", err) + } + case captureWrite: + if err := syscall.Mkfifo(s.path+".tmp", 0o600); err != nil { + t.Fatal(err) + } + done := make(chan error, 1) + go func() { done <- s.commit(snap) }() + waitFor(t, func() bool { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.retained["owner"]) != 0 + }) + finish = func() { + r, err := os.Open(s.path + ".tmp") + if err != nil { + t.Fatal(err) + } + raw, err := io.ReadAll(r) + _ = r.Close() + if err = errors.Join(err, <-done); err != nil { + t.Fatal(err) + } + if err = os.Remove(s.path); err != nil { + t.Fatal(err) + } + if err = os.WriteFile(s.path, raw, 0o600); err != nil { + t.Fatal(err) + } + } } - restore() - if err = s.save(claims); err != nil { - t.Fatal(err) - } - if s.referencesCheckpoint("before-recovery") { - t.Fatal("successful startup rewrite retained old checkpoint") - } - }) - } -} - -func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { - s := newClaimStore(t.TempDir(), false) - first := &types.Sandbox{ID: "first", ReplicaCk: "shared", ArchivePending: "shared"} - second := &types.Sandbox{ID: "second", ArchiveCk: "shared"} - if err := s.commit(s.set(first, second)); err != nil { - t.Fatal(err) - } - first.ReplicaCk = "" - if err := s.commit(s.set(first)); err != nil { - t.Fatal(err) - } - if !s.referencesCheckpoint("shared") { - t.Fatal("clearing one duplicate slot lost shared checkpoint") - } - s.del(first.ID) - if !s.referencesCheckpoint("shared") { - t.Fatal("memory deletion lost shared checkpoint") - } - if err := s.commit(s.mark()); err != nil { - t.Fatal(err) - } - if !s.referencesCheckpoint("shared") { - t.Fatal("one owner's retirement changed another owner's reference") - } - if err := s.commit(s.del(second.ID)); err != nil { - t.Fatal(err) - } - if s.referencesCheckpoint("shared") || len(s.checkpointReferences()) != 0 { - t.Fatal("last owner left shared checkpoint pinned") - } -} - -func TestClaimReferenceChangesStayDirtyAfterFailedReturnToOriginal(t *testing.T) { - s := newClaimStore(t.TempDir(), false) - sb := &types.Sandbox{ID: "owner", ReplicaCk: "original"} - if err := s.commit(s.set(sb)); err != nil { - t.Fatal(err) - } - restore := failClaimWrites(t, s) - attempted := []string{"attempt-1", "attempt-2", "attempt-3", "attempt-4", "attempt-5"} - for _, ck := range attempted { - sb.ReplicaCk = ck - if err := s.commit(s.set(sb)); err == nil { - t.Fatal("expected write failure") - } - } - sb.ReplicaCk = "original" - s.set(sb) - if !s.referencesCheckpoint("original") || len(s.checkpointReferences()) != len(attempted)+1 { - t.Fatal("failed return lost a possible snapshot") - } - for _, ck := range attempted { - if !s.referencesCheckpoint(ck) { - t.Fatalf("failed return lost attempted checkpoint %s", ck) - } - } - restore() - if err := s.commit(s.mark()); err != nil { - t.Fatal(err) - } - if !s.referencesCheckpoint("original") || len(s.checkpointReferences()) != 1 { - t.Fatal("successful return did not retire failed attempt") - } -} - -func TestClaimCapturedResetAndRecreatedOwner(t *testing.T) { - for _, reset := range []bool{false, true} { - t.Run(fmt.Sprint(reset), func(t *testing.T) { - s := newClaimStore(t.TempDir(), false) - sb := &types.Sandbox{ID: "owner", ReplicaCk: "captured"} - release := captureClaimWrite(t, s, s.set(sb), sb.ID) - if reset { - s.reset(map[string]*types.Sandbox{}) - } else { - s.del(sb.ID) + if tc.mutate != nil { + tc.mutate(s) } - s.set(&types.Sandbox{ID: sb.ID, ReplicaCk: "recreated"}) - var captured map[string]*types.Sandbox - if err := json.Unmarshal(release(), &captured); err != nil { + finish() + loaded, err := newClaimStore(dir, false).load() + if err != nil { t.Fatal(err) } - if captured[sb.ID].ReplicaCk != "captured" { - t.Fatal("in-flight snapshot changed") + var onDisk []string + for _, sb := range loaded { + onDisk = appendRefs(onDisk, checkpointRefs(sb)) } - if !s.referencesCheckpoint("captured") || !s.referencesCheckpoint("recreated") { - t.Fatal("acknowledgment lost captured or newer reference") + slices.Sort(onDisk) + if pinned := slices.Sorted(maps.Keys(s.checkpointReferences())); !slices.Equal(onDisk, tc.onDisk) || !slices.Equal(pinned, tc.afterFault) { + t.Fatalf("after the fault: on disk %v, pinned %v; want %v, %v", onDisk, pinned, tc.onDisk, tc.afterFault) } - if err := s.commit(s.mark()); err != nil { + if err = s.commit(s.mark()); err != nil { t.Fatal(err) } - if s.referencesCheckpoint("captured") || !s.referencesCheckpoint("recreated") { - t.Fatal("new snapshot did not retire old owner references") + if pinned := slices.Sorted(maps.Keys(s.checkpointReferences())); !slices.Equal(pinned, tc.afterCommit) { + t.Fatalf("after the next commit: pinned %v, want %v", pinned, tc.afterCommit) } }) } } -func TestClaimReferencesSurviveRenameFailure(t *testing.T) { +func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { s := newClaimStore(t.TempDir(), false) - sb := &types.Sandbox{ID: "owner", ReplicaCk: "captured"} - if err := os.Mkdir(s.path, 0o700); err != nil { + if err := s.commit(s.set(&types.Sandbox{ID: "first", ReplicaCk: "shared", ArchivePending: "shared"}, &types.Sandbox{ID: "second", ArchiveCk: "shared"})); err != nil { t.Fatal(err) } - if err := s.commit(s.set(sb)); err == nil { - t.Fatal("expected rename failure") - } - s.del(sb.ID) - if !s.referencesCheckpoint("captured") { - t.Fatal("failed rename released attempted checkpoint") - } - if err := os.Remove(s.path); err != nil { - t.Fatal(err) - } - if err := s.commit(s.mark()); err != nil { - t.Fatal(err) + if err := s.commit(s.del("first")); err != nil || !s.referencesCheckpoint("shared") { + t.Fatalf("one owner's retirement released another owner's checkpoint: %v", err) } - if s.referencesCheckpoint("captured") { - t.Fatal("successful rewrite did not retire failed snapshot") + if err := s.commit(s.del("second")); err != nil || len(s.checkpointReferences()) != 0 { + t.Fatalf("last owner left the shared checkpoint pinned: %v", err) } } @@ -265,38 +207,3 @@ func failClaimWrites(t *testing.T, s *claimStore) func() { t.Cleanup(restore) return restore } - -func captureClaimWrite(t *testing.T, s *claimStore, snap claimSnapshot, owner string) func() []byte { - t.Helper() - fifo := s.path + ".tmp" - if err := syscall.Mkfifo(fifo, 0o600); err != nil { - t.Fatal(err) - } - done := make(chan error, 1) - go func() { done <- s.commit(snap) }() - waitFor(t, func() bool { - s.mu.Lock() - defer s.mu.Unlock() - return len(s.retained[owner]) != 0 - }) - var raw []byte - release := sync.OnceFunc(func() { - r, err := os.Open(fifo) - if err != nil { - t.Fatal(err) - } - raw, err = io.ReadAll(r) - _ = r.Close() - if err != nil { - t.Fatal(err) - } - if err := <-done; err != nil { - t.Fatal(err) - } - if err := os.Remove(s.path); err != nil { - t.Fatal(err) - } - }) - t.Cleanup(release) - return func() []byte { release(); return raw } -} diff --git a/sandboxd/pool/claims_test.go b/sandboxd/pool/claims_test.go index 020a9ecc..f7e19311 100644 --- a/sandboxd/pool/claims_test.go +++ b/sandboxd/pool/claims_test.go @@ -1,7 +1,6 @@ package pool import ( - "encoding/json/v2" "errors" "io" "io/fs" @@ -59,37 +58,23 @@ func TestRecommitDrainsAChangeItWasTooBusyToSee(t *testing.T) { return m.store.set(sb) } - sb.ReplicaPending = "first" m.store.recommit(t.Context(), change()) waitFor(t, func() bool { - m.store.mu.Lock() - defer m.store.mu.Unlock() - return slices.Contains(m.store.retained[sb.ID], "first") + if m.store.writeMu.TryLock() { + m.store.writeMu.Unlock() + return false + } + return true }) - sb.ReplicaPending = "second" m.store.recommit(t.Context(), change()) r, err := os.Open(fifo) if err != nil { t.Fatalf("open fifo: %v", err) } - raw, err := io.ReadAll(r) - if err != nil { - t.Fatal(err) - } - var captured map[string]*types.Sandbox - if err = json.Unmarshal(raw, &captured); err != nil { - t.Fatal(err) - } - if captured[sb.ID].ReplicaPending != "first" { - t.Fatal("FIFO did not capture original snapshot") - } + _, _ = io.Copy(io.Discard, r) _ = r.Close() waitFor(t, func() bool { return !m.store.recommitting.Load() && !m.store.pending(m.store.mark()) }) - loaded, err := newClaimStore(filepath.Dir(m.store.path), false).load() - if err != nil || loaded[sb.ID].ReplicaPending != "second" { - t.Fatalf("recommit missed newer projection: %+v, %v", loaded, err) - } } func TestStoreRoundTrip(t *testing.T) { From 7c7cc3b2a7d5e46bfe561ac0ce7dba3b92656328 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:11:36 +0000 Subject: [PATCH 11/15] pool: table the checkpoint cleanup tests The replica retirement, unpublished intent and settled intent tests ran one sequence: break claim writes, act, check the checkpoint and its delete marker survive a retry, restore, restart or commit, check the cleanup finished. They are now one table of {setup, act}, each run live and across a restart; the settled intent gains the restart case. The consumed-row purge keeps only its released-claim case, as a table row; its claimed case repeated the retirement checks. The table's tail holds the one unrelated-write check and runs the orphan sweep to show no tombstone outlives the landed write. failClaimWrites now parks the background retrier itself, so callers no longer set recommitting by hand. The no-registry fixtures move into their one test, the checks that could not fire go, and the restored claim is compared through checkpointRefs. --- sandboxd/pool/checkpoint_cleanup_test.go | 439 ++++++++------------ sandboxd/pool/checkpoint_references_test.go | 2 + sandboxd/pool/stale_claim_test.go | 1 - 3 files changed, 166 insertions(+), 276 deletions(-) diff --git a/sandboxd/pool/checkpoint_cleanup_test.go b/sandboxd/pool/checkpoint_cleanup_test.go index c0566c13..907ef02b 100644 --- a/sandboxd/pool/checkpoint_cleanup_test.go +++ b/sandboxd/pool/checkpoint_cleanup_test.go @@ -7,255 +7,170 @@ import ( "time" "github.com/cocoonstack/sandbox/sandboxd/config" - "github.com/cocoonstack/sandbox/sandboxd/store" "github.com/cocoonstack/sandbox/sandboxd/types" ) -var strandedStates = []strandedState{ - {name: "published replica", setup: func(t *testing.T, m *Manager, _ *failoverCluster, sb *types.Sandbox) string { +func TestCheckpointCleanupWaitsForTheClaimWrite(t *testing.T) { + replicated := func(t *testing.T, m *Manager, _ *failoverCluster, sb *types.Sandbox) string { replicateNow(t, m, sb) return sb.ReplicaCk - }}, - {name: "pending replica", setup: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { - m.archives = &hookRegistry{Registry: c.reg, failPut: true} - if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { - t.Fatal(err) - } - return sb.ReplicaPending - }}, - {name: "archive intent", setup: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { - if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { - t.Fatal(err) + } + keepsTombstone := func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) { + rec, ok, err := c.reg.Get(t.Context(), sb.ID) + if err != nil || !ok || !rec.Consumed { + t.Fatalf("retirement lost its tombstone: %+v, %t, %v", rec, ok, err) } - m.archives = &hookRegistry{Registry: c.reg, failPut: true} - if err := m.archive(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { - t.Fatal(err) + m.reconcileOwnRow(t.Context(), rec, time.Now()) + if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok { + t.Fatalf("sweep erased an unacknowledged retirement: %t, %v", ok, err) } - return sb.ArchivePending - }}, -} - -func TestReplicaRetirementKeepsTombstoneUntilClaimCommit(t *testing.T) { - for _, restart := range []bool{false, true} { - t.Run(fmt.Sprintf("restart=%t", restart), func(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - replicateNow(t, m, sb) - ck := sb.ReplicaCk - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - sb.Transition.Lock() - err := m.dropReplica(t.Context(), sb) - sb.Transition.Unlock() - if err == nil || sb.HasReplica() { - t.Fatalf("expected an unacknowledged memory clear: replica=%t, %v", sb.HasReplica(), err) - } - rec, ok, err := c.reg.Get(t.Context(), sb.ID) - if err != nil || !ok || !rec.Consumed || !ckExists(t, m, ck) { - t.Fatalf("failed commit lost recovery tombstone/checkpoint: %+v, exists=%t, err=%v", rec, ok, err) - } - if !m.deleteMarked(ck) { - t.Error("failed replica clear did not queue its checkpoint for cleanup") - } - m.reconcileOwnRow(t.Context(), rec, time.Now()) - if rec, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok || !rec.Consumed { - t.Fatalf("sweep erased unacknowledged retirement: %+v, exists=%t, err=%v", rec, ok, err) - } - m.retryArchiveDeletes(t.Context()) - if !ckExists(t, m, ck) || !m.deleteMarked(ck) { - t.Fatal("cleanup discarded unacknowledged checkpoint or retry marker") - } - restoreWrites() - if restart { - m = c.restart(t, m) - if err = m.Reconcile(t.Context()); err != nil { + } + for _, tc := range []struct { + name string + setup func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string + act func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) + }{ + { + name: "replica retirement", + setup: replicated, + act: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) { + sb.Transition.Lock() + err := m.dropReplica(t.Context(), sb) + sb.Transition.Unlock() + if err == nil || sb.HasReplica() { + t.Fatalf("dropReplica: replica=%t, %v; want a memory clear and the write error", sb.HasReplica(), err) + } + keepsTombstone(t, m, c, sb) + }, + }, + { + name: "retirement of a released claim", + setup: replicated, + act: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) { + m.retireReplica(t.Context(), sb) + m.mu.Lock() + delete(m.claimed, sb.ID) + m.store.del(sb.ID) + m.mu.Unlock() + keepsTombstone(t, m, c, sb) + }, + }, + { + name: "unpublished replica intent", + setup: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { t.Fatal(err) } - restored, present := m.byID(sb.ID) - if !present { - t.Fatal("restart dropped a claim after failed retirement commit") + m.archives = c.reg + return sb.ReplicaPending + }, + act: func(t *testing.T, m *Manager, _ *failoverCluster, sb *types.Sandbox) { + m.retireReplica(t.Context(), sb) + if sb.ReplicaPending != "" { + t.Fatal("retirement kept the intent in memory") } - m.retireReplica(t.Context(), restored) - if restored.HasReplica() { - t.Fatal("restart did not finish retirement") + }, + }, + { + name: "settled replica intent", + setup: func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { + replicateNow(t, m, sb) + previous := sb.ReplicaCk + m.archives = &hookRegistry{Registry: c.reg, losePutReply: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) } - } else if err = m.store.commit(m.store.mark()); err != nil { - t.Fatal(err) - } - m.retryArchiveDeletes(t.Context()) - if ckExists(t, m, ck) || m.deleteMarked(ck) { - t.Fatal("checkpoint cleanup did not finish after its reference retired") - } - m.reconcileOwnRow(t.Context(), rec, time.Now()) - if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || ok { - t.Fatalf("acknowledged retirement retained tombstone: %t, %v", ok, err) - } - }) - } -} - -func TestReplicaUnpublishedIntentSurvivesFailedClearAndRestart(t *testing.T) { - for _, restart := range []bool{false, true} { - t.Run(fmt.Sprintf("restart=%t", restart), func(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - m.archives = &hookRegistry{Registry: c.reg, failPut: true} - if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { - t.Fatal(err) - } - pending := sb.ReplicaPending - if pending == "" || !ckExists(t, m, pending) { - t.Fatal("missing unpublished checkpoint") - } - m.archives = c.reg - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - m.retireReplica(t.Context(), sb) - persisted, err := newClaimStore(m.dataDir, false).load() - if err != nil || persisted[sb.ID].ReplicaPending != pending { - t.Fatalf("failure did not retain old disk state: %+v, %v", persisted, err) - } - if sb.ReplicaPending != "" || m.store.synced() { - t.Fatal("expected memory clear before snapshot acknowledgment") - } - if deleted, err := m.deleteArchiveCk(t.Context(), pending); err != nil || deleted { - t.Fatalf("orphan delete ignored unacknowledged claim: deleted=%t, %v", deleted, err) - } - m.retryArchiveDeletes(t.Context()) - if !ckExists(t, m, pending) || !m.deleteMarked(pending) { - t.Fatal("cleanup discarded pending checkpoint or retry marker") - } - restoreWrites() - if restart { - m = c.restart(t, m) - if err := m.Reconcile(t.Context()); err != nil { + m.archives = c.reg + return previous + }, + act: func(t *testing.T, m *Manager, _ *failoverCluster, sb *types.Sandbox) { + pending := sb.ReplicaPending + sb.Transition.Lock() + err := m.settleReplicaIntent(t.Context(), sb) + sb.Transition.Unlock() + if err == nil || sb.ReplicaPending != "" || sb.ReplicaCk != pending { + t.Fatalf("settle: %+v, %v; want the intent settled in memory and the write error", sb, err) + } + }, + }, + } { + for _, restart := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/restart=%t", tc.name, restart), func(t *testing.T) { + c := newReplicaCluster(t) + m := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, m, testKey) + ck := tc.setup(t, m, c, sb) + restoreWrites := failClaimWrites(t, m.store) + tc.act(t, m, c, sb) + m.retryArchiveDeletes(t.Context()) + if !ckExists(t, m, ck) || !m.deleteMarked(ck) { + t.Fatal("cleanup dropped the checkpoint or its delete marker before the claim write landed") + } + restoreWrites() + if restart { + m = c.restart(t, m) + if err := m.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + var ok bool + if sb, ok = m.byID(sb.ID); !ok { + t.Fatal("restart dropped the claim") + } + m.retireReplica(t.Context(), sb) + } else if err := m.store.commit(m.store.mark()); err != nil { t.Fatal(err) } - restored, ok := m.byID(sb.ID) - if !ok { - t.Fatal("restart mistook a failed pending clear for adoption and discarded the claim") + other := mustClaim(t, m, testKey) + m.mu.Lock() + other.LeaseSeconds++ + m.store.set(other) + m.mu.Unlock() + m.retryArchiveDeletes(t.Context()) + if ckExists(t, m, ck) || m.deleteMarked(ck) { + t.Fatal("cleanup did not finish once the claim write landed") } - m.retireReplica(t.Context(), restored) - if restored.HasReplica() { - t.Fatal("restart failed to retire unpublished replica") + for _, live := range checkpointRefs(sb) { + if live != "" && !ckExists(t, m, live) { + t.Errorf("cleanup deleted live checkpoint %s", live) + } } - } else if err := m.store.commit(m.store.mark()); err != nil { - t.Fatal(err) - } - other := mustClaim(t, m, testKey) - m.mu.Lock() - other.LeaseSeconds++ - m.store.set(other) - m.mu.Unlock() - m.retryArchiveDeletes(t.Context()) - if ckExists(t, m, pending) || m.deleteMarked(pending) { - t.Fatal("acknowledged cleanup leaked checkpoint or retry marker") - } - }) - } -} - -func TestReplicaSettledCheckpointCleanupRetriesAfterFailedCommit(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - replicateNow(t, m, sb) - previous := sb.ReplicaCk - m.archives = &hookRegistry{Registry: c.reg, losePutReply: true} - if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { - t.Fatal(err) - } - pending := sb.ReplicaPending - m.archives = c.reg - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - sb.Transition.Lock() - err := m.settleReplicaIntent(t.Context(), sb) - sb.Transition.Unlock() - if err == nil || sb.ReplicaPending != "" || sb.ReplicaCk != pending || m.store.synced() { - t.Fatalf("settle state: %+v, %v", sb, err) - } - if !ckExists(t, m, previous) || !m.deleteMarked(previous) { - t.Fatal("failed settle lost old checkpoint or its queued cleanup") - } - m.retryArchiveDeletes(t.Context()) - if !ckExists(t, m, previous) { - t.Fatal("old checkpoint deleted before clear acknowledgment") - } - restoreWrites() - if err = m.store.commit(m.store.mark()); err != nil { - t.Fatal(err) - } - m.retryArchiveDeletes(t.Context()) - if ckExists(t, m, previous) || m.deleteMarked(previous) || !ckExists(t, m, pending) { - t.Fatal("acknowledged settle failed to clean old checkpoint or removed current checkpoint") + m.sweepOrphanArchives(t.Context()) + if rec, ok, _ := c.reg.Get(t.Context(), sb.ID); ok && rec.Consumed { + t.Error("sweep kept the tombstone after the claim write landed") + } + }) + } } } -func TestReplicaConsumedRowPurgeWaitsForSnapshot(t *testing.T) { - for _, claimed := range []bool{false, true} { - t.Run(fmt.Sprintf("claimed=%t", claimed), func(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) +func TestFailoverDisabledExitKeepsReplicaCheckpoint(t *testing.T) { + states := []struct { + name string + setup func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string + }{ + {"published replica", func(t *testing.T, m *Manager, _ *failoverCluster, sb *types.Sandbox) string { replicateNow(t, m, sb) - m.store.recommitting.Store(true) - restoreWrites := failClaimWrites(t, m.store) - m.retireReplica(t.Context(), sb) - if !claimed { - m.mu.Lock() - delete(m.claimed, sb.ID) - m.store.del(sb.ID) - m.mu.Unlock() - } - rec, ok, err := c.reg.Get(t.Context(), sb.ID) - if err != nil || !ok || !rec.Consumed { - t.Fatalf("setup: %+v, %t, %v", rec, ok, err) - } - m.reconcileOwnRow(t.Context(), rec, time.Now()) - if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || !ok { - t.Fatalf("sweep dropped unacknowledged row: %t, %v", ok, err) + return sb.ReplicaCk + }}, + {"pending replica", func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.replicate(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) } - restoreWrites() - if err = m.store.commit(m.store.mark()); err != nil { + return sb.ReplicaPending + }}, + {"archive intent", func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string { + if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { t.Fatal(err) } - m.reconcileOwnRow(t.Context(), rec, time.Now()) - if _, ok, err = c.reg.Get(t.Context(), sb.ID); err != nil || ok { - t.Fatalf("sweep retained acknowledged row: %t, %v", ok, err) + m.archives = &hookRegistry{Registry: c.reg, failPut: true} + if err := m.archive(t.Context(), sb); !errors.Is(err, ErrArchiveStoreDown) { + t.Fatal(err) } - }) - } -} - -func TestArchiveCheckpointCleanupIgnoresUnrelatedClaimWrites(t *testing.T) { - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - checkpoint, _, err := m.publishCheckpoint(t.Context(), sb, store.CheckpointID(randHex(8)), "orphan", "", true) - if err != nil { - t.Fatal(err) - } - ck := checkpoint.ID - other := mustClaim(t, m, testKey) - m.mu.Lock() - other.LeaseSeconds++ - m.store.set(other) - m.mu.Unlock() - if m.store.synced() { - t.Fatal("expected an unrelated unacknowledged claim write") - } - if deleted, err := m.deleteArchiveCk(t.Context(), ck); err != nil || !deleted { - t.Fatalf("unrelated claim delayed checkpoint cleanup: deleted=%t, %v", deleted, err) - } - if ckExists(t, m, ck) || m.deleteMarked(ck) { - t.Fatal("unreferenced checkpoint or delete marker remained") + return sb.ArchivePending + }}, } -} - -func TestFailoverDisabledExitKeepsReplicaCheckpoint(t *testing.T) { exits := []struct { name string run func(t *testing.T, m *Manager, sb *types.Sandbox) @@ -273,11 +188,32 @@ func TestFailoverDisabledExitKeepsReplicaCheckpoint(t *testing.T) { waitFor(t, func() bool { return m.Counters().Reaps == 1 }) }}, } - for _, state := range strandedStates { + for _, state := range states { for _, exit := range exits { t.Run(state.name+"/"+exit.name, func(t *testing.T) { - m, sb, ck := restartStranded(t, state) - exit.run(t, m, sb) + c := newReplicaCluster(t) + old := c.node(t, "a", t.TempDir(), keepAnHour) + sb := mustClaim(t, old, testKey) + ck := state.setup(t, old, c, sb) + cfg := &config.Config{ + APIToken: "root", DataDir: old.dataDir, CheckpointDir: c.ckDir, + Pools: []config.PoolSpec{{PoolKey: testKey}}, + } + m, err := NewManager(t.Context(), cfg, old.eng, testSecrets(t), testBackends(t, cfg)) + if err != nil { + t.Fatal(err) + } + if err = m.Reconcile(t.Context()); err != nil { + t.Fatal(err) + } + restored, ok := m.byID(sb.ID) + if !ok || checkpointRefs(restored) != checkpointRefs(sb) { + t.Fatalf("startup without a registry changed the claim: %+v", restored) + } + if !ckExists(t, m, ck) || m.deleteMarked(ck) { + t.Fatalf("startup without a registry queued or deleted checkpoint %s", ck) + } + exit.run(t, m, restored) if _, held := m.byID(sb.ID); held || !ckExists(t, m, ck) { t.Fatalf("%s without a registry: held=%t checkpoint kept=%t", exit.name, held, ckExists(t, m, ck)) } @@ -285,50 +221,3 @@ func TestFailoverDisabledExitKeepsReplicaCheckpoint(t *testing.T) { } } } - -type strandedState struct { - name string - setup func(t *testing.T, m *Manager, c *failoverCluster, sb *types.Sandbox) string -} - -func restartStranded(t *testing.T, state strandedState) (*Manager, *types.Sandbox, string) { - t.Helper() - c := newReplicaCluster(t) - m := c.node(t, "a", t.TempDir(), keepAnHour) - sb := mustClaim(t, m, testKey) - ck := state.setup(t, m, c, sb) - if ck == "" { - t.Fatal("fixture left no failover checkpoint") - } - restarted := newManagerWithoutRegistry(t, m, c) - if err := restarted.Reconcile(t.Context()); err != nil { - t.Fatal(err) - } - restored, ok := restarted.byID(sb.ID) - if !ok { - t.Fatal("startup without a registry lost the claim") - } - if restored.ReplicaCk != sb.ReplicaCk || restored.ReplicaPending != sb.ReplicaPending || restored.ArchivePending != sb.ArchivePending { - t.Fatalf("startup without a registry changed failover fields: replica=%q pending=%q archive=%q", restored.ReplicaCk, restored.ReplicaPending, restored.ArchivePending) - } - if !ckExists(t, restarted, ck) || restarted.deleteMarked(ck) { - t.Fatalf("startup without a registry queued or deleted checkpoint %s", ck) - } - return restarted, restored, ck -} - -func newManagerWithoutRegistry(t *testing.T, old *Manager, c *failoverCluster) *Manager { - t.Helper() - cfg := &config.Config{ - APIToken: "root", DataDir: old.dataDir, CheckpointDir: c.ckDir, - Pools: []config.PoolSpec{{PoolKey: testKey}}, - } - m, err := NewManager(t.Context(), cfg, old.eng, testSecrets(t), testBackends(t, cfg)) - if err != nil { - t.Fatal(err) - } - if m.archives != nil { - t.Fatal("failover-disabled fixture unexpectedly has a registry") - } - return m -} diff --git a/sandboxd/pool/checkpoint_references_test.go b/sandboxd/pool/checkpoint_references_test.go index 27801188..f3e0d8c3 100644 --- a/sandboxd/pool/checkpoint_references_test.go +++ b/sandboxd/pool/checkpoint_references_test.go @@ -193,12 +193,14 @@ func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { } } +// failClaimWrites makes claims.json writes fail until restore and keeps the background retrier from starting. func failClaimWrites(t *testing.T, s *claimStore) func() { t.Helper() tmp := s.path + ".tmp" if err := os.Mkdir(tmp, 0o700); err != nil { t.Fatal(err) } + s.recommitting.Store(true) restore := sync.OnceFunc(func() { if err := os.Remove(tmp); err != nil { t.Error(err) diff --git a/sandboxd/pool/stale_claim_test.go b/sandboxd/pool/stale_claim_test.go index 5b2dfd7f..703d487a 100644 --- a/sandboxd/pool/stale_claim_test.go +++ b/sandboxd/pool/stale_claim_test.go @@ -111,7 +111,6 @@ func TestIntentRollbackPreservesCurrentClaim(t *testing.T) { if err := m.store.commit(m.store.mark()); err != nil { t.Fatal(err) } - waitFor(t, func() bool { return !m.store.recommitting.Load() }) assertStaleClaimPreserved(t, m, sb.ID, ck.ID, replace) }) } From a35223b83798c364685098fc6e00a2e33ee626c8 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:13:18 +0000 Subject: [PATCH 12/15] pool: table the stale claim tests The stale hibernate test and the intent rollback test both claim, checkpoint, make the claim stale, released or replaced, and then check claims.json and the checkpoint cleanup. They are now one table of {newManager, prepare, run}, each run with and without a replacement, and the one-caller assertion helper is inlined. The stale hibernate now drops the claim the same way the rollback cases do instead of going through Release; either way the sandbox is no longer the claimed one, which is what hibernateLocked checks. The writeMu cleanup guard goes: no cleanup takes writeMu, so a failure there only leaks the blocked goroutine. --- sandboxd/pool/stale_claim_test.go | 198 +++++++++++++----------------- 1 file changed, 88 insertions(+), 110 deletions(-) diff --git a/sandboxd/pool/stale_claim_test.go b/sandboxd/pool/stale_claim_test.go index 703d487a..885069aa 100644 --- a/sandboxd/pool/stale_claim_test.go +++ b/sandboxd/pool/stale_claim_test.go @@ -2,139 +2,117 @@ package pool import ( "errors" + "fmt" "testing" "github.com/cocoonstack/sandbox/sandboxd/types" ) -func TestHibernateRejectsStaleClaim(t *testing.T) { - for _, replace := range []bool{false, true} { - name := "released" - if replace { - name = "replaced" - } - t.Run(name, func(t *testing.T) { - eng := newFakeEngine() - m := newTestManager(t, eng) - sb := mustClaim(t, m, testKey) - ck, err := m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "", "") - if err != nil { - t.Fatal(err) - } - if err = m.Release(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { - t.Fatal(err) - } - if replace { +func TestStaleClaimOperationsPreserveTheCurrentClaim(t *testing.T) { + hibernate := func(t *testing.T, m *Manager, sb *types.Sandbox) error { + return m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}) + } + rollback := func(op func(*testing.T, *Manager, *types.Sandbox) error) func(*testing.T, *Manager, *types.Sandbox, func()) { + return func(t *testing.T, m *Manager, sb *types.Sandbox, stale func()) { + m.store.writeMu.Lock() + done := make(chan error, 1) + go func() { done <- op(t, m, sb) }() + waitFor(t, func() bool { m.mu.Lock() - replacement := &types.Sandbox{ID: sb.ID, ReplicaCk: ck.ID} - m.addClaimLocked(replacement) - js := m.store.set(replacement) - m.mu.Unlock() - if err = m.store.commit(js); err != nil { - t.Fatal(err) - } - } - sb.Transition.Lock() - err = m.hibernateLocked(t.Context(), sb) - sb.Transition.Unlock() - if !errors.Is(err, ErrUnknownSandbox) { - t.Errorf("stale Hibernate: %v, want ErrUnknownSandbox", err) - } - if eng.hibernateCount() != 0 { - t.Error("stale Hibernate reached the engine") + defer m.mu.Unlock() + return sb.PendingSnap != "" || sb.ArchivePending != "" + }) + stale() + restore := failClaimWrites(t, m.store) + m.store.writeMu.Unlock() + if err := <-done; err == nil { + t.Fatal("intent write succeeded despite filesystem failure") } - assertStaleClaimPreserved(t, m, sb.ID, ck.ID, replace) - }) + restore() + } } -} - -func TestIntentRollbackPreservesCurrentClaim(t *testing.T) { - for _, operation := range []string{"hibernate", "archive"} { - for _, replace := range []bool{false, true} { - name := operation + "/released" - if replace { - name = operation + "/replaced" - } - t.Run(name, func(t *testing.T) { - var m *Manager - if operation == "archive" { - m = newFailoverCluster(t).node(t, "a", t.TempDir(), keepAnHour) - } else { - m = newTestManager(t, newFakeEngine()) + for _, tc := range []struct { + name string + newManager func(t *testing.T) *Manager + prepare func(t *testing.T, m *Manager, sb *types.Sandbox) + run func(t *testing.T, m *Manager, sb *types.Sandbox, stale func()) + }{ + { + name: "hibernate", + newManager: func(t *testing.T) *Manager { return newTestManager(t, newFakeEngine()) }, + run: func(t *testing.T, m *Manager, sb *types.Sandbox, stale func()) { + stale() + sb.Transition.Lock() + err := m.hibernateLocked(t.Context(), sb) + sb.Transition.Unlock() + if !errors.Is(err, ErrUnknownSandbox) { + t.Errorf("stale Hibernate: %v, want ErrUnknownSandbox", err) + } + if m.eng.(*fakeEngine).hibernateCount() != 0 { + t.Error("stale Hibernate reached the engine") } + }, + }, + { + name: "hibernate intent rollback", + newManager: func(t *testing.T) *Manager { return newTestManager(t, newFakeEngine()) }, + run: rollback(hibernate), + }, + { + name: "archive intent rollback", + newManager: func(t *testing.T) *Manager { return newFailoverCluster(t).node(t, "a", t.TempDir(), keepAnHour) }, + prepare: func(t *testing.T, m *Manager, sb *types.Sandbox) { + if err := hibernate(t, m, sb); err != nil { + t.Fatal(err) + } + }, + run: rollback(func(t *testing.T, m *Manager, sb *types.Sandbox) error { return m.archive(t.Context(), sb) }), + }, + } { + for _, replace := range []bool{false, true} { + t.Run(fmt.Sprintf("%s/replace=%t", tc.name, replace), func(t *testing.T) { + m := tc.newManager(t) sb := mustClaim(t, m, testKey) ck, err := m.Checkpoint(t.Context(), sb.ID, Cred{Token: sb.Token}, "", "") if err != nil { t.Fatal(err) } - if operation == "archive" { - if err := m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}); err != nil { - t.Fatal(err) - } + if tc.prepare != nil { + tc.prepare(t, m, sb) } - m.store.writeMu.Lock() - locked := true - t.Cleanup(func() { - if locked { - m.store.writeMu.Unlock() - } - }) - done := make(chan error, 1) - go func() { - if operation == "archive" { - done <- m.archive(t.Context(), sb) - } else { - done <- m.Hibernate(t.Context(), sb.ID, Cred{Token: sb.Token}) - } - }() - waitFor(t, func() bool { + tc.run(t, m, sb, func() { m.mu.Lock() defer m.mu.Unlock() - return sb.PendingSnap != "" || sb.ArchivePending != "" + m.dropClaimLocked(sb) + m.store.del(sb.ID) + if replace { + replacement := &types.Sandbox{ID: sb.ID, ReplicaCk: ck.ID} + m.addClaimLocked(replacement) + m.store.set(replacement) + } }) - m.mu.Lock() - m.dropClaimLocked(sb) - m.store.del(sb.ID) - if replace { - replacement := &types.Sandbox{ID: sb.ID, ReplicaCk: ck.ID} - m.addClaimLocked(replacement) - m.store.set(replacement) + if err = m.store.commit(m.store.mark()); err != nil { + t.Fatal(err) } - m.mu.Unlock() - restore := failClaimWrites(t, m.store) - m.store.writeMu.Unlock() - locked = false - if err := <-done; err == nil { - t.Fatal("intent write succeeded despite filesystem failure") + loaded, err := newClaimStore(m.dataDir, false).load() + if err != nil { + t.Fatal(err) } - restore() - if err := m.store.commit(m.store.mark()); err != nil { + if replace { + if loaded[sb.ID] == nil || loaded[sb.ID].ReplicaCk != ck.ID || loaded[sb.ID].PendingSnap != "" { + t.Errorf("stale operation overwrote replacement on disk: %+v", loaded[sb.ID]) + } + } else if loaded[sb.ID] != nil { + t.Errorf("stale operation resurrected released claim on disk: %+v", loaded[sb.ID]) + } + if err = m.markArchiveCk(ck.ID); err != nil { t.Fatal(err) } - assertStaleClaimPreserved(t, m, sb.ID, ck.ID, replace) + m.retryArchiveDeletes(t.Context()) + if exists := ckExists(t, m, ck.ID); exists != replace { + t.Errorf("checkpoint survived cleanup = %v, want %v", exists, replace) + } }) } } } - -func assertStaleClaimPreserved(t *testing.T, m *Manager, id, ck string, replaced bool) { - t.Helper() - loaded, err := newClaimStore(m.dataDir, false).load() - if err != nil { - t.Fatal(err) - } - if replaced { - if loaded[id] == nil || loaded[id].ReplicaCk != ck || loaded[id].PendingSnap != "" { - t.Errorf("stale operation overwrote replacement on disk: %+v", loaded[id]) - } - } else if loaded[id] != nil { - t.Errorf("stale operation resurrected released claim on disk: %+v", loaded[id]) - } - if err := m.markArchiveCk(ck); err != nil { - t.Fatal(err) - } - m.retryArchiveDeletes(t.Context()) - if exists := ckExists(t, m, ck); exists != replaced { - t.Errorf("checkpoint survived cleanup = %v, want %v", exists, replaced) - } -} From 4587788a5f5cc7805d25ea3442796eb59e8dfd46 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:18:04 +0000 Subject: [PATCH 13/15] pool: drop a liveness check the hibernate adoption test cannot fail The sandbox was claimed one line earlier, so setIfLive always reports it live there. Were it not, the uncommitted intent would fail the adoption checks that follow anyway. --- sandboxd/pool/reconcile_test.go | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/sandboxd/pool/reconcile_test.go b/sandboxd/pool/reconcile_test.go index ec820677..b50d78dc 100644 --- a/sandboxd/pool/reconcile_test.go +++ b/sandboxd/pool/reconcile_test.go @@ -24,10 +24,7 @@ func TestReconcileJournalsAnAdoptedHibernate(t *testing.T) { if err := eng.Hibernate(t.Context(), sb.VMName, snap); err != nil { t.Fatalf("engine hibernate: %v", err) } - intent, live := m.setIfLive(sb, func() { sb.PendingSnap = snap }) - if !live { - t.Fatal("hibernate claim disappeared before intent") - } + intent, _ := m.setIfLive(sb, func() { sb.PendingSnap = snap }) if err := m.store.commit(intent); err != nil { t.Fatalf("journal intent: %v", err) } From 41e6f2b06fac598139304a0b7415419f82cb58e8 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 08:03:44 +0000 Subject: [PATCH 14/15] pool: drop redundant claim store helper comments --- sandboxd/pool/checkpoint_references_test.go | 1 - sandboxd/pool/claims.go | 1 - 2 files changed, 2 deletions(-) diff --git a/sandboxd/pool/checkpoint_references_test.go b/sandboxd/pool/checkpoint_references_test.go index f3e0d8c3..3125c68a 100644 --- a/sandboxd/pool/checkpoint_references_test.go +++ b/sandboxd/pool/checkpoint_references_test.go @@ -193,7 +193,6 @@ func TestClaimSharedCheckpointReferencesRetirePerOwner(t *testing.T) { } } -// failClaimWrites makes claims.json writes fail until restore and keeps the background retrier from starting. func failClaimWrites(t *testing.T, s *claimStore) func() { t.Helper() tmp := s.path + ".tmp" diff --git a/sandboxd/pool/claims.go b/sandboxd/pool/claims.go index 061e1c50..2f07ecaf 100644 --- a/sandboxd/pool/claims.go +++ b/sandboxd/pool/claims.go @@ -149,7 +149,6 @@ func (s *claimStore) del(ids ...string) claimSnapshot { return claimSnapshot{seq: s.seq} } -// put replaces id's row with sb's projection, or drops it when sb is nil, and moves the checkpoint counts with it. func (s *claimStore) put(id string, sb *types.Sandbox) { var refs [4]string if sb == nil { From 7cb73793d97d7ff95c3a789171b4d799343cac08 Mon Sep 17 00:00:00 2001 From: minatoaquaMK2 Date: Mon, 26 Oct 2026 07:58:26 +0000 Subject: [PATCH 15/15] pool: read orphan archive references through checkpointRefs The orphan sweep listed the four checkpoint fields by hand, a second copy that would drift if a field were added. --- sandboxd/pool/reconcile.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/sandboxd/pool/reconcile.go b/sandboxd/pool/reconcile.go index b8f836ec..a116b580 100644 --- a/sandboxd/pool/reconcile.go +++ b/sandboxd/pool/reconcile.go @@ -296,7 +296,10 @@ func (m *Manager) reclaimOrphanArchiveCks(ctx context.Context, claims map[string continue } orig, mine := claims[ckpt.SandboxID] - if !mine || ckpt.Node != m.nodeID || orig.ArchiveCk == ckpt.ID || orig.ReplicaCk == ckpt.ID || orig.ReplicaPending == ckpt.ID || orig.ArchivePending == ckpt.ID { + if !mine || ckpt.Node != m.nodeID { + continue + } + if refs := checkpointRefs(orig); slices.Contains(refs[:], ckpt.ID) { continue } orphans = append(orphans, ckpt)