amadan
console: the delete door fails closed only on a store read; an undecodable record is deleted the ordinary way (Plan 6 Task 5 fix round 1)
The import rule now reads the record's bytes itself. A failed store read still answers main's delete sentence (M15); a body that will not decode, or that disagrees with its path, is logged and removed by the ordinary delete as before (review I1). An overlapping delete that finds the record already gone answers the ordinary idempotent 200 (M3). Tests cover both bad records, a --from seed and an unseeded instance on the ordinary path (M2), and the two overlapping-delete orders.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
2 files changed,
+134
−13
internal/console/apiinstanceimport.go+38 −9internal/console/apiinstanceimport_test.go+96 −4
CI passed — run details
diff --git a/internal/console/apiinstanceimport.go b/internal/console/apiinstanceimport.go| index f43e0f0b5..b7d9a3b1c 100644 |
| --- a/internal/console/apiinstanceimport.go |
| +++ b/internal/console/apiinstanceimport.go |
| @@ -768,6 +768,29 @@ func (s *Server) finishImport(r *http.Request, sqid, app, host, id string) error |
| return s.finishSeed(ctx, sqid, app, host, id) |
| } |
| +// readDeletedRecord is the delete door's read of host's record for its |
| +// import rule. Only the store read can fail it (preflight M15, review |
| +// I1): a body that will not decode, or that disagrees with its own path, |
| +// is no import this console can finish, so it answers nil and the |
| +// ordinary delete removes it as it always has. |
| +func (s *Server) readDeletedRecord(ctx context.Context, sqid, app, host string) (*provision.Record, error) { |
| + ctx, cancel := context.WithTimeout(ctx, instanceStoreTimeout) |
| + defer cancel() |
| + raw, _, err := s.cfg.Releases.Get(ctx, provision.RecordKey(sqid, app, host)) |
| + if err != nil { |
| + return nil, err |
| + } |
| + rec, err := provision.DecodeRecord(raw) |
| + if err == nil && (rec.Account != sqid || rec.App != app || rec.Host != host) { |
| + err = errors.New("the record disagrees with its own path") |
| + } |
| + if err != nil { |
| + log.Printf("console: api instances delete %s/%s/%s: the record could not be read as an import (%v), so it is deleted the ordinary way", sqid, app, host, err) |
| + return nil, nil |
| + } |
| + return &rec, nil |
| +} |
| + |
| // handleAPIImportAbort abandons an import before it is committed (spec |
| // §5.6): a record CAS to aborted, then the box rolls it back. The |
| // transition is provision.SeedAbort, which allows every state before |
| @@ -850,15 +873,18 @@ func (s *Server) abortAnswered(w http.ResponseWriter, r *http.Request, sqid, app |
| // - committed, a --from seed, an unseeded record, or no record at all: |
| // (false, false), the ordinary delete. |
| // |
| -// It fails closed (preflight M15): a record it cannot read, a seed id that |
| +// It fails closed (preflight M15): a store read that fails, a seed id that |
| // cannot name a key (M8) and a finish that fails answer main's delete |
| // sentence, so a read blip never lets the ordinary delete drop an |
| -// uncommitted import's record and leak what its seed made. On a claim |
| +// uncommitted import's record and leak what its seed made. A body that |
| +// was read but will not decode, or disagrees with its path, is no import |
| +// anything can finish, so the ordinary delete removes it as before |
| +// (review I1). On a claim |
| // console, where the finish runs nowhere, an uncommitted import is refused |
| // as every import door refuses there. |
| func (s *Server) refuseImportDelete(w http.ResponseWriter, r *http.Request, acct *Account, app, host string) (answered, finished bool) { |
| sqid := acct.Sqid |
| - rec, err := s.readImportRecord(r.Context(), sqid, app, host) |
| + rec, err := s.readDeletedRecord(r.Context(), sqid, app, host) |
| if errors.Is(err, os.ErrNotExist) { |
| return false, false |
| } |
| @@ -867,7 +893,7 @@ func (s *Server) refuseImportDelete(w http.ResponseWriter, r *http.Request, acct |
| jsonError(w, http.StatusBadGateway, importDeleteFailed) |
| return true, false |
| } |
| - if !rec.Seed.Import() || rec.Seed.State == seeds.StateCommitted { |
| + if rec == nil || !rec.Seed.Import() || rec.Seed.State == seeds.StateCommitted { |
| return false, false |
| } |
| if s.cfg.HostClaim { |
| @@ -885,16 +911,19 @@ func (s *Server) refuseImportDelete(w http.ResponseWriter, r *http.Request, acct |
| case seeds.StateAwaitingUpload, seeds.StateUploaded, seeds.StateRefused: |
| wctx, wcancel := context.WithTimeout(context.WithoutCancel(r.Context()), instanceStoreTimeout) |
| _, err := provision.SetRecordSeed(wctx, s.cfg.Releases, sqid, app, host, provision.SeedRolledBackBeforeSwap(id)) |
| - var now *provision.SeedSource |
| + var now *provision.Record |
| + var nowErr error |
| if lostTransition(err) { |
| - now = s.importNow(wctx, sqid, app, host, id) |
| + now, nowErr = s.readImportRecord(wctx, sqid, app, host) |
| } |
| wcancel() |
| switch { |
| case err == nil: |
| - case errors.Is(err, os.ErrNotExist): |
| - return false, false // gone already: the ordinary delete's idempotent answer |
| - case now != nil && now.State == seeds.StateRolledBack: |
| + case errors.Is(err, os.ErrNotExist), errors.Is(nowErr, os.ErrNotExist): |
| + // Gone already, by this CAS's read or by another delete that |
| + // finished first: the ordinary delete's idempotent answer. |
| + return false, false |
| + case now != nil && now.Seed != nil && now.Seed.ID == id && now.Seed.State == seeds.StateRolledBack: |
| // Another delete rolled it back first: finish it all the same. |
| case lostTransition(err): |
| s.importTransitionFailure(w, app, host, err) // SD-N60 |
diff --git a/internal/console/apiinstanceimport_test.go b/internal/console/apiinstanceimport_test.go| index 62d9c4427..bef67faef 100644 |
| --- a/internal/console/apiinstanceimport_test.go |
| +++ b/internal/console/apiinstanceimport_test.go |
| @@ -1830,12 +1830,104 @@ func TestImportDeleteRefusedUntilCommittedOrUnswapped(t *testing.T) { |
| } |
| } |
| - // Committed: the ordinary delete. |
| + // Committed, a --from seed in any state, and an unseeded instance: the |
| + // ordinary delete. |
| f.seedRecord(t, "done.shop.example.com", "", newSeedID(t), seeds.KindImport, seeds.StateCommitted) |
| - if code, out := deleteInstance(t, f, "done.shop.example.com"); code != http.StatusOK || out["result"] != "deleted" { |
| - t.Fatalf("delete of a committed import = %d %v", code, out) |
| + f.seedRecord(t, fromHost, "", newSeedID(t), seeds.KindReplica, seeds.StateReady) |
| + for _, host := range []string{"done.shop.example.com", fromHost, fromSource} { |
| + if code, out := deleteInstance(t, f, host); code != http.StatusOK || out["result"] != "deleted" { |
| + t.Fatalf("delete of %s = %d %v", host, code, out) |
| + } |
| + f.requireRecordGone(t, host) |
| + } |
| +} |
| + |
| +// A record the delete door can read but not decode, or one that disagrees |
| +// with its own path, is no import: the ordinary delete removes it, as it |
| +// did before the import rule (review I1). |
| +func TestImportDeleteRemovesARecordItCannotDecode(t *testing.T) { |
| + f := newImportFixture(t) |
| + var logs bytes.Buffer |
| + log.SetOutput(&logs) |
| + defer log.SetOutput(os.Stderr) |
| + other, _ := json.Marshal(provision.Record{Schema: 1, Host: "other.shop.example.com", Slug: "other", App: fromApp, Account: "bab", |
| + Channel: "stable", CreatedAt: time.Now().UTC(), |
| + Seed: &provision.SeedSource{ID: newSeedID(t), Kind: seeds.KindImport, State: seeds.StateAwaitingUpload, At: time.Now().UTC()}}) |
| + for host, body := range map[string][]byte{ |
| + "junk.shop.example.com": []byte("not json"), |
| + "mismatch.shop.example.com": other, |
| + } { |
| + key := provision.RecordKey("bab", fromApp, host) |
| + if _, err := f.fx.rel.Put(context.Background(), key, body, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + if code, out := deleteInstance(t, f, host); code != http.StatusOK || out["result"] != "deleted" { |
| + t.Fatalf("delete of %s = %d %v, want the ordinary 200", host, code, out) |
| + } |
| + if _, _, err := f.fx.rel.Get(context.Background(), key); !errors.Is(err, os.ErrNotExist) { |
| + t.Fatalf("%s survived its delete: %v", host, err) |
| + } |
| + if !strings.Contains(logs.String(), host+": the record could not be read as an import") { |
| + t.Fatalf("no log line names %s: %s", host, logs.String()) |
| + } |
| + } |
| +} |
| + |
| +// deleteRace plays a second delete of the same import against this one: |
| +// hook runs just before this delete's rollback CAS write (so it loses), |
| +// and gone, when set, deletes the record just before the re-read that |
| +// follows the loss, as the other delete's finish would. |
| +type deleteRace struct { |
| + release.Store |
| + key string |
| + hook func() |
| + gone bool |
| + puts atomic.Int32 |
| + reads atomic.Int32 |
| +} |
| + |
| +func (s *deleteRace) Put(ctx context.Context, key string, body []byte, ifMatch string) (string, error) { |
| + if key == s.key && ifMatch != "" && s.puts.Add(1) == 1 { |
| + s.hook() |
| + } |
| + return s.Store.Put(ctx, key, body, ifMatch) |
| +} |
| + |
| +func (s *deleteRace) Get(ctx context.Context, key string) ([]byte, string, error) { |
| + // Reads after the hook: the CAS's retry, then the delete door's re-read. |
| + if key == s.key && s.gone && s.puts.Load() > 0 && s.reads.Add(1) == 2 { |
| + if err := s.Store.Delete(ctx, key); err != nil { |
| + return nil, "", err |
| + } |
| + } |
| + return s.Store.Get(ctx, key) |
| +} |
| + |
| +func (s *deleteRace) DeleteIfMatch(ctx context.Context, key, etag string) error { |
| + return release.DeleteIfMatch(ctx, s.Store, key, etag) |
| +} |
| + |
| +// Two deletes of one pre-swap import overlap. The one whose rollback CAS |
| +// loses finishes the import when it finds it rolled back, and answers the |
| +// ordinary idempotent 200 when the other has already removed the record |
| +// (review M3). |
| +func TestImportDeletesOverlap(t *testing.T) { |
| + for _, gone := range []bool{false, true} { |
| + t.Run(map[bool]string{false: "rolled back", true: "finished"}[gone], func(t *testing.T) { |
| + f := newImportFixture(t) |
| + id := newSeedID(t) |
| + f.seedRecord(t, importHost, "", id, seeds.KindImport, seeds.StateUploaded) |
| + f.fx.srv.cfg.Releases = &deleteRace{Store: f.fx.rel, key: provision.RecordKey("bab", fromApp, importHost), gone: gone, hook: func() { |
| + if _, err := provision.SetRecordSeed(context.Background(), f.fx.rel, "bab", fromApp, importHost, provision.SeedRolledBackBeforeSwap(id)); err != nil { |
| + t.Error(err) |
| + } |
| + }} |
| + if code, out := deleteInstance(t, f, importHost); code != http.StatusOK || out["result"] != "deleted" { |
| + t.Fatalf("overlapping delete = %d %v, want 200", code, out) |
| + } |
| + f.requireRecordGone(t, importHost) |
| + }) |
| } |
| - f.requireRecordGone(t, "done.shop.example.com") |
| } |
| // The pre-swap delete of a private-storage import tears down the empty |