amadan

carlos / platform Public

zdd-5 task 4: failure book hands out copies and retries failed saves

Lookup, ForApp and List return clones and the mutators are copy-on-write, so a
status read cannot race carlos instances retry. A failed save marks the book
dirty and the next mutating call or Prune retries it, so the record survives an
agent restart (spec §8.4). Prune documents that its callback runs under the lock.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude 7094629f7338e2963dcdedec45473033013268dc parent 505fd5c
2 files changed, +221 −17
  • internal/rollout/failures.go +51 −17
  • internal/rollout/failures_test.go +170 −0
diff --git a/internal/rollout/failures.go b/internal/rollout/failures.go
index bb119f247..b1caaea41 100644
--- a/internal/rollout/failures.go
+++ b/internal/rollout/failures.go
@@ -51,10 +51,25 @@ type failureFile struct {
// FailureBook is the box's failure records. An empty path keeps them in
// memory only, which is what tests and the simulation use.
+//
+// Records handed out (Lookup, ForApp, List) are copies, inner slices
+// included, because a status reader holds them outside the lock while
+// RetryHost and Record edit the book.
type FailureBook struct {
path string
mu sync.Mutex
recs []FailureRecord
+ // dirty is set when a save failed. Memory is the truth meanwhile, and
+ // every later mutating call and Prune retries the save until it lands,
+ // so the record still survives an agent restart (spec §8.4).
+ dirty bool
+}
+
+// cloneRec copies r's host lists so the copy shares no array with r.
+func cloneRec(r FailureRecord) FailureRecord {
+ r.HeldDown = slices.Clone(r.HeldDown)
+ r.Retried = slices.Clone(r.Retried)
+ return r
}
// OpenFailureBook loads the book from dataDir. A file that exists but cannot
@@ -80,15 +95,27 @@ func OpenFailureBook(dataDir string) (*FailureBook, error) {
return b, nil
}
+// saveLocked writes the book and records whether the file is now behind
+// memory, so a failure is retried by retryLocked rather than forgotten.
func (b *FailureBook) saveLocked() error {
if b.path == "" {
return nil
}
raw, err := json.MarshalIndent(failureFile{Schema: 1, Records: b.recs}, "", " ")
- if err != nil {
- return err
+ if err == nil {
+ err = release.WriteFileAtomic(b.path, raw, 0o644)
}
- return release.WriteFileAtomic(b.path, raw, 0o644)
+ b.dirty = err != nil
+ return err
+}
+
+// retryLocked is the save a call that changed nothing still owes when an
+// earlier save failed.
+func (b *FailureBook) retryLocked() error {
+ if !b.dirty {
+ return nil
+ }
+ return b.saveLocked()
}
func (b *FailureBook) find(acct, app, build string) int {
@@ -103,20 +130,21 @@ func (b *FailureBook) find(acct, app, build string) int {
// keeps the newer of the two PromotedAt stamps, so only a pointer signed
// after the latest failure clears it. The in-memory record is kept even when
// the file write fails, so the caller's transition still blocks the build;
-// the error is for the journal.
+// the error is for the journal, and the next call on the book (this one
+// again, if nothing new, or Prune) retries the save.
func (b *FailureBook) Record(r FailureRecord) (bool, error) {
b.mu.Lock()
defer b.mu.Unlock()
i := b.find(r.Account, r.App, r.Build)
if i < 0 {
r.Retried = nil
- b.recs = append(b.recs, r)
+ b.recs = append(b.recs, cloneRec(r))
return true, b.saveLocked()
}
cur := &b.recs[i]
changed := false
if j := slices.Index(cur.Retried, r.Host); j >= 0 {
- cur.Retried = slices.Delete(cur.Retried, j, j+1)
+ cur.Retried = slices.Delete(slices.Clone(cur.Retried), j, j+1)
changed = true
}
if r.PromotedAt.After(cur.PromotedAt) {
@@ -125,12 +153,12 @@ func (b *FailureBook) Record(r FailureRecord) (bool, error) {
}
for _, h := range r.HeldDown {
if !slices.Contains(cur.HeldDown, h) {
- cur.HeldDown = append(cur.HeldDown, h)
+ cur.HeldDown = append(slices.Clone(cur.HeldDown), h)
changed = true
}
}
if !changed {
- return false, nil
+ return false, b.retryLocked()
}
return false, b.saveLocked()
}
@@ -140,7 +168,7 @@ func (b *FailureBook) Lookup(acct, app, build string) (FailureRecord, bool) {
b.mu.Lock()
defer b.mu.Unlock()
if i := b.find(acct, app, build); i >= 0 {
- return b.recs[i], true
+ return cloneRec(b.recs[i]), true
}
return FailureRecord{}, false
}
@@ -174,12 +202,12 @@ func (b *FailureBook) RetryHost(acct, app, build, host string) (bool, error) {
defer b.mu.Unlock()
i := b.find(acct, app, build)
if i < 0 || slices.Contains(b.recs[i].Retried, host) {
- return false, nil
+ return false, b.retryLocked()
}
cur := &b.recs[i]
- cur.Retried = append(cur.Retried, host)
+ cur.Retried = append(slices.Clone(cur.Retried), host)
if j := slices.Index(cur.HeldDown, host); j >= 0 {
- cur.HeldDown = slices.Delete(cur.HeldDown, j, j+1)
+ cur.HeldDown = slices.Delete(slices.Clone(cur.HeldDown), j, j+1)
}
return true, b.saveLocked()
}
@@ -193,7 +221,7 @@ func (b *FailureBook) ClearOnPointer(acct, app, build string, promotedAt time.Ti
defer b.mu.Unlock()
i := b.find(acct, app, build)
if i < 0 || !promotedAt.After(b.recs[i].PromotedAt) {
- return false, nil
+ return false, b.retryLocked()
}
b.recs = slices.Delete(b.recs, i, i+1)
return true, b.saveLocked()
@@ -206,7 +234,7 @@ func (b *FailureBook) ForApp(acct, app string) []FailureRecord {
var out []FailureRecord
for _, r := range b.recs {
if r.Account == acct && r.App == app {
- out = append(out, r)
+ out = append(out, cloneRec(r))
}
}
return out
@@ -214,7 +242,9 @@ func (b *FailureBook) ForApp(acct, app string) []FailureRecord {
// Prune drops the records for builds no route on this box still has
// installed. "The record stays for any instance still on the failed build"
-// (spec §8.4), and only for those.
+// (spec §8.4), and only for those. installed is called under the book's lock
+// and must take no lock of its own (nor call back into the book). The
+// scheduler runs Prune every sweep, so it also retries a save that failed.
func (b *FailureBook) Prune(installed func(acct, app, build string) bool) error {
b.mu.Lock()
defer b.mu.Unlock()
@@ -225,7 +255,7 @@ func (b *FailureBook) Prune(installed func(acct, app, build string) bool) error
}
}
if len(kept) == len(b.recs) {
- return nil
+ return b.retryLocked()
}
b.recs = kept
return b.saveLocked()
@@ -235,5 +265,9 @@ func (b *FailureBook) Prune(installed func(acct, app, build string) bool) error
func (b *FailureBook) List() []FailureRecord {
b.mu.Lock()
defer b.mu.Unlock()
- return slices.Clone(b.recs)
+ out := make([]FailureRecord, len(b.recs))
+ for i, r := range b.recs {
+ out[i] = cloneRec(r)
+ }
+ return out
}
diff --git a/internal/rollout/failures_test.go b/internal/rollout/failures_test.go
index d70ade866..77c84ce95 100644
--- a/internal/rollout/failures_test.go
+++ b/internal/rollout/failures_test.go
@@ -3,6 +3,7 @@ package rollout
import (
"os"
"path/filepath"
+ "slices"
"testing"
"time"
)
@@ -127,3 +128,172 @@ func TestPruneKeepsRecordsSomeRouteStillInstalls(t *testing.T) {
t.Fatal("v3 is still installed and must stay held (spec §8.4)")
}
}
+
+// Review fix 1: a record handed out must not share its host lists with the
+// book, or a status read racing `carlos instances retry` sees a half-edited
+// list.
+func TestRecordsHandedOutAreCopies(t *testing.T) {
+ b := &FailureBook{}
+ in := rec("v2", "a.example.com")
+ in.HeldDown = []string{"a.example.com", "b.example.com", "c.example.com"}
+ if _, err := b.Record(in); err != nil {
+ t.Fatal(err)
+ }
+ in.HeldDown[0] = "scribbled-by-the-caller"
+ if got, _ := b.Lookup("bab", "blog", "v2"); got.HeldDown[0] != "a.example.com" {
+ t.Fatalf("Record kept the caller's HeldDown array: %v", got.HeldDown)
+ }
+
+ look, _ := b.Lookup("bab", "blog", "v2")
+ look.HeldDown[1] = "scribbled-on-lookup"
+ list := b.List()
+ list[0].HeldDown[2] = "scribbled-on-list"
+ forApp := b.ForApp("bab", "blog")
+ forApp[0].HeldDown[0] = "scribbled-on-forapp"
+
+ want := []string{"a.example.com", "b.example.com", "c.example.com"}
+ got, _ := b.Lookup("bab", "blog", "v2")
+ if !slices.Equal(got.HeldDown, want) {
+ t.Fatalf("book's HeldDown = %v, want %v: a handed-out record shares its array", got.HeldDown, want)
+ }
+
+ // Retried is shared the same way.
+ b.RetryHost("bab", "blog", "v2", "b.example.com")
+ look, _ = b.Lookup("bab", "blog", "v2")
+ look.Retried[0] = "scribbled-retried"
+ list = b.List()
+ list[0].Retried[0] = "scribbled-retried"
+ got, _ = b.Lookup("bab", "blog", "v2")
+ if !slices.Equal(got.Retried, []string{"b.example.com"}) {
+ t.Fatalf("book's Retried = %v", got.Retried)
+ }
+}
+
+// The mutators edit copy-on-write, so a slice taken from the book before an
+// edit (as a reader holding the old record would) never changes under it.
+func TestMutatorsDoNotEditSlicesInPlace(t *testing.T) {
+ b := &FailureBook{}
+ in := rec("v2", "a.example.com")
+ in.HeldDown = []string{"a.example.com", "b.example.com"}
+ b.Record(in)
+
+ b.mu.Lock()
+ oldHeld := b.recs[0].HeldDown
+ b.mu.Unlock()
+ b.RetryHost("bab", "blog", "v2", "a.example.com")
+ if !slices.Equal(oldHeld, []string{"a.example.com", "b.example.com"}) {
+ t.Fatalf("RetryHost edited the HeldDown array in place: %v", oldHeld)
+ }
+
+ b.mu.Lock()
+ oldRetried := b.recs[0].Retried
+ b.mu.Unlock()
+ b.Record(rec("v2", "a.example.com")) // spends a.example.com's retry
+ if !slices.Equal(oldRetried, []string{"a.example.com"}) {
+ t.Fatalf("Record edited the Retried array in place: %v", oldRetried)
+ }
+}
+
+// failBook returns a book whose file cannot be written (the path is a
+// directory), plus a func that makes it writable again.
+func failBook(t *testing.T) (*FailureBook, string, func()) {
+ t.Helper()
+ dir := t.TempDir()
+ b, err := OpenFailureBook(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ p := filepath.Join(dir, FailureFile)
+ if err := os.Mkdir(p, 0o755); err != nil {
+ t.Fatal(err)
+ }
+ return b, dir, func() {
+ if err := os.Remove(p); err != nil {
+ t.Fatal(err)
+ }
+ }
+}
+
+// Review fix 2 (spec §8.4: the record survives agent restarts): a failed save
+// is retried by the next call, even one that adds nothing new.
+func TestFailedSaveIsRetriedByTheNextRecord(t *testing.T) {
+ b, dir, heal := failBook(t)
+ if _, err := b.Record(rec("v2", "a.example.com")); err == nil {
+ t.Fatal("the first Record must report the failed save")
+ }
+ heal()
+ if added, err := b.Record(rec("v2", "a.example.com")); err != nil || added {
+ t.Fatalf("the repeat Record = %v, %v; want false, nil", added, err)
+ }
+ again, err := OpenFailureBook(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !again.Blocks("bab", "blog", "v2", "b.example.com", false) {
+ t.Fatal("the no-op Record did not retry the save: a restart would lose the record")
+ }
+}
+
+func TestFailedSaveIsRetriedByPrune(t *testing.T) {
+ b, dir, heal := failBook(t)
+ if _, err := b.Record(rec("v2", "a.example.com")); err == nil {
+ t.Fatal("the first Record must report the failed save")
+ }
+ if err := b.Prune(func(_, _, _ string) bool { return true }); err == nil {
+ t.Fatal("Prune must report that the save still fails")
+ }
+ heal()
+ if err := b.Prune(func(_, _, _ string) bool { return true }); err != nil {
+ t.Fatal(err)
+ }
+ again, err := OpenFailureBook(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, ok := again.Lookup("bab", "blog", "v2"); !ok {
+ t.Fatal("a Prune that drops nothing did not retry the save")
+ }
+}
+
+func TestFailedSaveIsRetriedAfterRetryHostAndClearOnPointer(t *testing.T) {
+ b, dir, heal := failBook(t)
+ b.Record(rec("v2", "a.example.com"))
+ b.Record(rec("v3", "a.example.com"))
+ if _, err := b.RetryHost("bab", "blog", "v2", "x.example.com"); err == nil {
+ t.Fatal("RetryHost must report the failed save")
+ }
+ heal()
+ // Nothing to clear here, but the pending save is still owed.
+ if ok, err := b.ClearOnPointer("bab", "blog", "v3", promotedAt); ok || err != nil {
+ t.Fatalf("ClearOnPointer = %v, %v", ok, err)
+ }
+ again, err := OpenFailureBook(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if got := again.List(); len(got) != 2 || !slices.Equal(got[0].Retried, []string{"x.example.com"}) {
+ t.Fatalf("reopened book = %+v", got)
+ }
+
+ // And a failing ClearOnPointer is itself retried.
+ if err := os.Remove(filepath.Join(dir, FailureFile)); err != nil {
+ t.Fatal(err)
+ }
+ if err := os.Mkdir(filepath.Join(dir, FailureFile), 0o755); err != nil {
+ t.Fatal(err)
+ }
+ if ok, err := b.ClearOnPointer("bab", "blog", "v3", promotedAt.Add(time.Second)); !ok || err == nil {
+ t.Fatalf("ClearOnPointer = %v, %v; want true and the save error", ok, err)
+ }
+ heal()
+ if _, err := b.RetryHost("bab", "blog", "v2", "x.example.com"); err != nil { // no-op
+ t.Fatal(err)
+ }
+ again, err = OpenFailureBook(dir)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, ok := again.Lookup("bab", "blog", "v3"); ok {
+ t.Fatal("the cleared record came back after a restart")
+ }
+}