amadan
moves: a box with no hibernate region still records its cleanup obligation, and one with no versioned store never holds gone back
Task 1 re-review, Minor A and B. ValidateRetired no longer requires BucketRegion: CARLOS_HIBERNATE_REGION is optional (the AWS default chain resolves ""), so such a box would otherwise skip every cleanup and page invalid evidence. Bucket and TombstoneOwner stay required; the invalid-evidence witness is now a listing whose object version has no ETag. A nil cfg.Versioned is a standing reason (logged, nothing written, gone proceeds); the doc comment names the two I/O corners left as errors (the op directory's MkdirAll, 10,000 corrupt names taken).
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4 files changed,
+56
−13
internal/host/movescleanup_retired.go+11 −7internal/host/movescleanup_retired_test.go+43 −3internal/offlinecleanup/wire.go+0 −2internal/offlinecleanup/wire_test.go+2 −1
CI failed — run details
CI log
=== gate === sigwho: pid 2 ppid 1 pgid 0 sid 0; child pid 3 .amadan/test-gate.sh gate: 2026-10-24T15:17:16Z on aarch64, 32 cpus, 61151 MB available, go1.26.8 linux/arm64 gate: +0s start launchctx gate: +0s start vet gate: +0s start gofmt gate: +1s start rest gate: +22s start internal_console@0 gate: +22s start internal_console@1 gate: +22s start internal_console@2 gate: +22s start internal_console@3 gate: +22s start internal_console@4 gate: +22s start internal_console@5 gate: +22s start internal_console@6 gate: +22s start internal_console@7 gate: +22s waiting for internal_console's shards before the next split package gate: +190s start root@0 gate: +190s start root@1 gate: +190s start root@2 gate: +190s waiting for root's shards before the next split package amadan: step gate failed: signal: terminated amadan: job failed at step gate — its output is above, not at the tail of this log
diff --git a/internal/host/movescleanup_retired.go b/internal/host/movescleanup_retired.go| index 7ea79ff70..2581ac8d8 100644 |
| --- a/internal/host/movescleanup_retired.go |
| +++ b/internal/host/movescleanup_retired.go |
| @@ -50,12 +50,16 @@ const retiredNoticeEvery = 24 * time.Hour |
| // type") in the obligation, which is still written. |
| // - Anything else writes nothing, logs once a day and answers nil, so |
| // the gone report is written as before this plan: no judge, another |
| -// region, no bucket or directories, an untracked lifetime, a version |
| -// id that names no one version; and, paged once per host, move and |
| -// rule as well, a seal that is absent, does not parse or fails |
| -// validation, evidence gone itself built that does not validate (a |
| -// bug or a misconfigured box), and a damaged retired.json that cannot |
| -// be moved aside (a directory there). |
| +// region, no bucket or directories, no versioned store, an untracked |
| +// lifetime, a version id that names no one version; and, paged once |
| +// per host, move and rule as well, a seal that is absent, does not |
| +// parse or fails validation, evidence gone itself built that does not |
| +// validate (a bug or a misconfigured box), and a damaged retired.json |
| +// that cannot be moved aside (a directory there). |
| +// |
| +// Two corners still return an error, as I/O: a MkdirAll of the op |
| +// directory that fails (ENOTDIR from a file in its place, say), and a |
| +// damaged file with 10,000 corrupt-<n> names already taken. |
| func (w *movesWorker) recordRetired(ctx context.Context, s release.Store, rec provision.Record) error { |
| cfg := w.cfg |
| m, host := rec.Move, rec.Host |
| @@ -112,7 +116,7 @@ func (w *movesWorker) recordRetired(ctx context.Context, s release.Store, rec pr |
| prefix := activator.HibernatePrefix(cfg.OwnPrefix, host) + "/" |
| if cfg.Versioned == nil { |
| - return errors.New("offline move: no versioned store on this box") |
| + return none("this box has no versioned store") |
| } |
| vs, err := cfg.Versioned(ctx, cfg.OwnBucket, cfg.OwnBucketRegion) |
| if err != nil { |
diff --git a/internal/host/movescleanup_retired_test.go b/internal/host/movescleanup_retired_test.go| index 91bc47e89..122c9f768 100644 |
| --- a/internal/host/movescleanup_retired_test.go |
| +++ b/internal/host/movescleanup_retired_test.go |
| @@ -876,15 +876,19 @@ func TestNoUsableSealSkipsAndPages(t *testing.T) { |
| } |
| // A standing condition never holds gone back: evidence that does not |
| -// validate (here a box with no bucket region) and a damaged record that |
| +// validate (here a listing whose object version has no ETag) and a damaged record that |
| // cannot be moved aside (a directory) each write nothing, page once, and |
| // let gone through. |
| func TestStandingConditionsPageAndNeverHoldGone(t *testing.T) { |
| t.Run("evidence that does not validate", func(t *testing.T) { |
| rw := newRetiredWorld(t, true) |
| - rw.ie.w.cfg.OwnBucketRegion = "" |
| - rw.ie.w.cfg.Versioned = func(context.Context, string, string) (lease.VersionedStore, error) { return rw.fake, nil } |
| rw.fake.Put(retBucket, retPrefix+"db", []byte("x")) |
| + rw.fake.ListHook = func(_ string, vs []lease.ObjectVersion) []lease.ObjectVersion { |
| + for i := range vs { |
| + vs[i].ETag = "" |
| + } |
| + return vs |
| + } |
| pages := rw.pages(rw.ie) |
| for range 2 { |
| if err := rw.gone(rw.ie, nil); err != nil { |
| @@ -934,3 +938,39 @@ func TestCleanupSkippedPageNamesTheMoveAndTheBox(t *testing.T) { |
| } |
| } |
| } |
| + |
| +// A box with no hibernate region still records its obligation: the region |
| +// is optional (the AWS default chain resolves it), and the store is asked |
| +// for it as configured. |
| +func TestNoBucketRegionStillRecordsRetired(t *testing.T) { |
| + rw := newRetiredWorld(t, true) |
| + rw.ie.w.cfg.OwnBucketRegion = "" |
| + rw.ie.w.cfg.Versioned = func(_ context.Context, bucket, region string) (lease.VersionedStore, error) { |
| + if bucket != retBucket || region != "" { |
| + return nil, fmt.Errorf("versioned store for %s in %q", bucket, region) |
| + } |
| + return rw.fake, nil |
| + } |
| + rw.fake.Put(retBucket, retPrefix+"db", []byte("x")) |
| + pages := rw.pages(rw.ie) |
| + if err := rw.gone(rw.ie, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + r, ok := rw.retired(rw.ie) |
| + if !ok || r.BucketRegion != "" || len(*pages) != 0 { |
| + t.Fatalf("written %v, region %q, pages %q", ok, r.BucketRegion, *pages) |
| + } |
| +} |
| + |
| +// A box with no versioned store writes nothing and lets gone through. |
| +func TestNoVersionedStoreNeverHoldsGone(t *testing.T) { |
| + rw := newRetiredWorld(t, true) |
| + rw.ie.w.cfg.Versioned = nil |
| + if err := rw.gone(rw.ie, nil); err != nil { |
| + t.Fatal(err) |
| + } |
| + rw.noRetired(rw.ie) |
| + if !rw.goneReported("ie-a") { |
| + t.Fatal("no gone report") |
| + } |
| +} |
diff --git a/internal/offlinecleanup/wire.go b/internal/offlinecleanup/wire.go| index f03638c7d..4d3564362 100644 |
| --- a/internal/offlinecleanup/wire.go |
| +++ b/internal/offlinecleanup/wire.go |
| @@ -317,8 +317,6 @@ func ValidateRetired(r Retired) error { |
| return fmt.Errorf("retired: from and to are both %q", r.From) |
| case r.Bucket == "": |
| return errors.New("retired: no bucket") |
| - case r.BucketRegion == "": |
| - return errors.New("retired: no bucket region") |
| case r.TombstoneOwner == "": |
| return errors.New("retired: no tombstone owner") |
| } |
diff --git a/internal/offlinecleanup/wire_test.go b/internal/offlinecleanup/wire_test.go| index 6cc5bbba9..5dfaaac06 100644 |
| --- a/internal/offlinecleanup/wire_test.go |
| +++ b/internal/offlinecleanup/wire_test.go |
| @@ -100,7 +100,6 @@ func TestValidateRetired(t *testing.T) { |
| {"superseding with nothing in flight", func(r *Retired) { r.State = StateCleaning; r.Superseding = SupersededRow }, "superseding"}, |
| {"written at", func(r *Retired) { r.WrittenAt = time.Time{} }, "written"}, |
| {"no bucket", func(r *Retired) { r.Bucket = "" }, "bucket"}, |
| - {"no bucket region", func(r *Retired) { r.BucketRegion = "" }, "region"}, |
| {"no tombstone owner", func(r *Retired) { r.TombstoneOwner = "" }, "tombstone"}, |
| {"a live link that is a regular file", func(r *Retired) { |
| r.LocalInventory[0].Identity = FileIdentity{Dev: 1, Inode: 2, Mode: ModeRegular} |
| @@ -196,6 +195,8 @@ func TestValidateRetired(t *testing.T) { |
| func(r *Retired) { r.Keys = map[string]KeyOutcome{k0(r).Key: {State: KeyUnsettled, Done: 2}} }, |
| func(r *Retired) { r.LocalInventory, r.LocalLeft = nil, nil; r.BucketInventory = nil }, |
| func(r *Retired) { r.Rev, r.PublishedRev = 4, 4 }, |
| + // CARLOS_HIBERNATE_REGION is optional: the AWS default chain resolves "". |
| + func(r *Retired) { r.BucketRegion = "" }, |
| func(r *Retired) { |
| r.LocalLeft = []KeptPath{{Which: WhichProgramsFile, Reason: ReasonUnexpectedType}} |
| }, |