amadan

carlos / platform Public

console: the delete door's import check is bound to the delete (final fix F7)

refuseImportDelete let the ordinary delete run after it saw no record,
or a record that was not an uncommitted import, but the delete itself
was unconditional. An import begun in between (its record reserved, its
store provisioned) was deleted the ordinary way; the sweep could remove
its snapshot but never finish its store, and the host could not be
imported again.

A host the check finds absent is now the delete's idempotent answer
with nothing deleted, including when the rollback CAS finds it gone.
Otherwise the ordinary delete is a conditional delete on the etag the
check read (release.DeleteIfMatch); on a conflict the host is read and
decided once more, and a second conflict answers main's retry sentence.
A claim console keeps its claim retirement. TestAPIInstanceStoreFailure
now creates the record its delete fails on, since an absent host no
longer reaches the store.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
claude af78194c1804bf9756e57deaa84ff5830c9519bc parent e89c897
3 files changed, +213 −18
  • internal/console/apiinstanceimport.go +62 −18
  • internal/console/apiinstances_test.go +13 −0
  • internal/console/importdelete_test.go +138 −0
diff --git a/internal/console/apiinstanceimport.go b/internal/console/apiinstanceimport.go
index 00fd90bc5..3d5f72a3a 100644
--- a/internal/console/apiinstanceimport.go
+++ b/internal/console/apiinstanceimport.go
@@ -788,16 +788,17 @@ func (s *Server) finishImport(r *http.Request, sqid, app, host, id string) error
}
// 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) {
+// import rule, with the etag the ordinary delete is then bound to. 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, string, error) {
ctx, cancel := context.WithTimeout(ctx, instanceStoreTimeout)
defer cancel()
- raw, _, err := s.cfg.Releases.Get(ctx, provision.RecordKey(sqid, app, host))
+ raw, etag, err := s.cfg.Releases.Get(ctx, provision.RecordKey(sqid, app, host))
if err != nil {
- return nil, err
+ return nil, "", err
}
rec, err := provision.DecodeRecord(raw)
if err == nil && (rec.Account != sqid || rec.App != app || rec.Host != host) {
@@ -805,9 +806,9 @@ func (s *Server) readDeletedRecord(ctx context.Context, sqid, app, host string)
}
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 nil, etag, nil
}
- return &rec, nil
+ return &rec, etag, nil
}
// handleAPIImportAbort abandons an import before it is committed (spec
@@ -881,16 +882,20 @@ func (s *Server) abortAnswered(w http.ResponseWriter, r *http.Request, sqid, app
// refuseImportDelete is the delete door's import rule (spec §5.6, codex
// R2-7), run before the ordinary delete. answered: the delete must not go
// the ordinary way, and either this function has answered it (a refusal or
-// a failure), or, with finished, the import was finished here and the
-// caller writes the ordinary delete's audit row and answer.
+// a failure), or, with finished, the delete was done here (the import
+// finished, or the ordinary delete bound to this read) and the caller
+// writes the ordinary delete's audit row and answer. (false, false) is
+// left only to a claim console, whose ordinary delete is the claim's.
//
// - awaiting-upload, uploaded, refused: no box has swapped, so the record
// moves straight to rolled-back and is finished here (its store, its
// snapshot under seeds/<host>/<id>/, then the record).
// - rolled-back: finished here.
// - any other uncommitted state: 409, abort the import instead (SD-N57).
-// - committed, a --from seed, an unseeded record, or no record at all:
-// (false, false), the ordinary delete.
+// - committed, a --from seed, an unseeded record, or a record that will
+// not decode: the ordinary delete, conditional on the etag read here
+// (deleteRead), answered as finished.
+// - no record at all: finished, with nothing deleted (final review F7).
//
// 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
@@ -902,10 +907,21 @@ func (s *Server) abortAnswered(w http.ResponseWriter, r *http.Request, sqid, app
// 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) {
+ return s.importDeleteRule(w, r, acct, app, host, true)
+}
+
+// importDeleteRule is refuseImportDelete; again is whether a record that
+// changed under the ordinary delete may be read and decided once more.
+//
+// The ordinary delete is bound to what the rule read (final review F7):
+// absence is the delete's own idempotent answer, so no delete runs that
+// could remove an import begun since; and otherwise the record is deleted
+// only while it is the one the rule read (deleteRead).
+func (s *Server) importDeleteRule(w http.ResponseWriter, r *http.Request, acct *Account, app, host string, again bool) (answered, finished bool) {
sqid := acct.Sqid
- rec, err := s.readDeletedRecord(r.Context(), sqid, app, host)
+ rec, etag, err := s.readDeletedRecord(r.Context(), sqid, app, host)
if errors.Is(err, os.ErrNotExist) {
- return false, false
+ return true, true
}
if err != nil {
log.Printf("console: api instances delete %s/%s/%s: read the record: %v", sqid, app, host, err)
@@ -913,7 +929,7 @@ func (s *Server) refuseImportDelete(w http.ResponseWriter, r *http.Request, acct
return true, false
}
if rec == nil || !rec.Seed.Import() || rec.Seed.State == seeds.StateCommitted {
- return false, false
+ return s.deleteRead(w, r, acct, app, host, etag, again)
}
if s.cfg.HostClaim {
ref := seedClaimConsoleRefusal()
@@ -940,8 +956,9 @@ func (s *Server) refuseImportDelete(w http.ResponseWriter, r *http.Request, acct
case err == nil:
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
+ // finished first: the delete's idempotent answer, with no
+ // ordinary delete run after it (final review F7).
+ return true, true
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):
@@ -964,3 +981,30 @@ func (s *Server) refuseImportDelete(w http.ResponseWriter, r *http.Request, acct
}
return true, true
}
+
+// deleteRead is the ordinary delete of host's record, conditional on the
+// etag the import rule read it under (final review F7). A record gone
+// since is deleted already. A record that changed since may now be an
+// import begun after the rule's read: it is read and decided again, once,
+// and a second change answers main's retry sentence.
+//
+// A claim console retires records through the claim
+// (retireClaimHostRecord) and begins no import, so there the ordinary
+// delete runs as before: (false, false).
+func (s *Server) deleteRead(w http.ResponseWriter, r *http.Request, acct *Account, app, host, etag string, again bool) (answered, finished bool) {
+ if s.cfg.HostClaim {
+ return false, false
+ }
+ ctx, cancel := context.WithTimeout(r.Context(), instanceStoreTimeout)
+ defer cancel()
+ err := release.DeleteIfMatch(ctx, s.cfg.Releases, provision.RecordKey(acct.Sqid, app, host), etag)
+ switch {
+ case err == nil:
+ return true, true
+ case errors.Is(err, release.ErrCASConflict) && again:
+ return s.importDeleteRule(w, r, acct, app, host, false)
+ }
+ log.Printf("console: api instances delete %s/%s/%s: %v", acct.Sqid, app, host, err)
+ jsonError(w, http.StatusBadGateway, importDeleteFailed)
+ return true, false
+}
diff --git a/internal/console/apiinstances_test.go b/internal/console/apiinstances_test.go
index c90473faa..308156d84 100644
--- a/internal/console/apiinstances_test.go
+++ b/internal/console/apiinstances_test.go
@@ -2005,6 +2005,12 @@ func (u undeletableStore) Delete(context.Context, string) error {
return errors.New(brokenStoreErr)
}
+// DeleteIfMatch fails the same way: the delete door deletes a record
+// conditionally on the etag its import check read (final review F7).
+func (u undeletableStore) DeleteIfMatch(context.Context, string, string) error {
+ return errors.New(brokenStoreErr)
+}
+
// TestAPIInstanceStoreFailure: a bucket that cannot be read or written is a
// 502, and its error text — bucket names, request ids — never reaches the
// caller. A read failure is NOT reported as "instances are not enabled".
@@ -2030,6 +2036,13 @@ func TestAPIInstanceStoreFailure(t *testing.T) {
if strings.Contains(out, "instances enable") {
t.Fatalf("a read failure was reported as the feature being disabled: %s", out)
}
+ // The record must exist for a delete to be attempted: a host with no
+ // record is the delete's idempotent 200 with nothing deleted (final
+ // review F7).
+ fx.srv.cfg.Releases = fx.rel
+ if status, _, out := rawReq(t, http.MethodPost, u, token, `{"host":"acme.go.tito.io","region":"ie"}`); status != http.StatusCreated {
+ t.Fatalf("create = %d %s", status, out)
+ }
fx.srv.cfg.Releases = undeletableStore{fx.rel}
status, _, out = rawReq(t, http.MethodDelete, u+"/acme.go.tito.io", token, "")
if status != http.StatusBadGateway {
diff --git a/internal/console/importdelete_test.go b/internal/console/importdelete_test.go
new file mode 100644
index 000000000..69e431b98
--- /dev/null
+++ b/internal/console/importdelete_test.go
@@ -0,0 +1,138 @@
+package console
+
+import (
+ "context"
+ "errors"
+ "net/http"
+ "os"
+ "sync/atomic"
+ "testing"
+ "time"
+
+ "github.com/carlosframework/platform/internal/provision"
+ "github.com/carlosframework/platform/internal/release"
+ "github.com/carlosframework/platform/internal/seeds"
+)
+
+// afterRecordRead runs then after each of the first n reads of key: what
+// another request does between the delete door's import check and its
+// delete.
+type afterRecordRead struct {
+ release.Store
+ key string
+ n int32
+ reads atomic.Int32
+ then func()
+}
+
+func (s *afterRecordRead) Get(ctx context.Context, key string) ([]byte, string, error) {
+ raw, tag, err := s.Store.Get(ctx, key)
+ if key == s.key && s.reads.Add(1) <= s.n {
+ s.then()
+ }
+ return raw, tag, err
+}
+
+func (s *afterRecordRead) DeleteIfMatch(ctx context.Context, key, etag string) error {
+ return release.DeleteIfMatch(ctx, s.Store, key, etag)
+}
+
+// plainRecord writes a plain record of shop on host.
+func (f fromFixture) plainRecord(t *testing.T, host string) {
+ t.Helper()
+ if err := provision.CreateRecord(context.Background(), f.fx.rel, provision.Record{Schema: 1, Host: host, Slug: "old",
+ App: fromApp, Account: "bab", Channel: "stable", CreatedAt: time.Now().UTC()}); err != nil {
+ t.Fatal(err)
+ }
+}
+
+// Final review F7: the delete door's check finds no record, and an import
+// is begun before the delete would have run. Absence is the delete's own
+// idempotent answer: the ordinary delete is not run, and the import's
+// record stands.
+func TestImportDeleteOfAnAbsentHostLeavesAnImportBegunSince(t *testing.T) {
+ f := newImportFixture(t)
+ id := newSeedID(t)
+ key := provision.RecordKey("bab", fromApp, importHost)
+ f.fx.srv.cfg.Releases = &afterRecordRead{Store: f.fx.rel, key: key, n: 1, then: func() {
+ f.seedRecord(t, importHost, "", id, seeds.KindImport, seeds.StateAwaitingUpload)
+ }}
+ if code, out := deleteInstance(t, f, importHost); code != http.StatusOK || out["result"] != "deleted" {
+ t.Fatalf("delete of an absent host = %d %v, want the idempotent 200", code, out)
+ }
+ rec, err := f.record(t, importHost)
+ if err != nil {
+ t.Fatalf("the import begun since was deleted: %v", err)
+ }
+ requireImportState(t, rec, id, seeds.StateAwaitingUpload, "")
+}
+
+// The check reads a plain record; before the delete, that instance is
+// deleted and an import of the host begins and swaps. The ordinary delete
+// is bound to the plain record's etag, so it does not land: the door reads
+// the host again and answers as it answers for the import, SD-N57, with
+// the import's record standing.
+func TestImportDeleteReclassifiesARecordThatBecameAnImport(t *testing.T) {
+ f := newImportFixture(t)
+ f.plainRecord(t, importHost)
+ id := newSeedID(t)
+ key := provision.RecordKey("bab", fromApp, importHost)
+ f.fx.srv.cfg.Releases = &afterRecordRead{Store: f.fx.rel, key: key, n: 1, then: func() {
+ if err := f.fx.rel.Delete(context.Background(), key); err != nil {
+ t.Error(err)
+ }
+ f.seedRecord(t, importHost, "", id, seeds.KindImport, seeds.StateSwapped)
+ }}
+ if code, out := deleteInstance(t, f, importHost); code != http.StatusConflict || str(t, out, "error") != importAbortFirst(importHost) {
+ t.Fatalf("delete = %d %v, want 409 SD-N57", code, out)
+ }
+ rec, err := f.record(t, importHost)
+ if err != nil {
+ t.Fatalf("the import was deleted the ordinary way: %v", err)
+ }
+ requireImportState(t, rec, id, seeds.StateSwapped, "")
+ if rows := importAudit(t, f, "instance-delete"); len(rows) != 0 {
+ t.Fatalf("a refused delete audited: %q", rows)
+ }
+}
+
+// A record that changes under the delete twice is not chased: the second
+// conflict answers main's retry sentence, and the record stands.
+func TestImportDeleteGivesUpAfterASecondChange(t *testing.T) {
+ f := newImportFixture(t)
+ f.plainRecord(t, importHost)
+ key := provision.RecordKey("bab", fromApp, importHost)
+ f.fx.srv.cfg.Releases = &afterRecordRead{Store: f.fx.rel, key: key, n: 2, then: func() {
+ rec, etag, err := provision.GetRecordETag(context.Background(), f.fx.rel, "bab", fromApp, importHost)
+ if err != nil {
+ t.Error(err)
+ return
+ }
+ rec.Steering = map[string]string{"": "latency", "latency": ""}[rec.Steering]
+ if err := provision.UpdateRecord(context.Background(), f.fx.rel, *rec, etag); err != nil {
+ t.Error(err)
+ }
+ }}
+ if code, out := deleteInstance(t, f, importHost); code != http.StatusBadGateway || str(t, out, "error") != mainDeleteFailed {
+ t.Fatalf("delete = %d %v, want 502 %q", code, out, mainDeleteFailed)
+ }
+ if _, err := f.record(t, importHost); errors.Is(err, os.ErrNotExist) {
+ t.Fatal("a record that kept changing was deleted")
+ }
+}
+
+// The ordinary case is unchanged: a plain record the check read is
+// deleted, and a second delete of the gone host is the idempotent 200.
+func TestImportDeleteOfAPlainRecord(t *testing.T) {
+ f := newImportFixture(t)
+ f.plainRecord(t, importHost)
+ for range 2 {
+ if code, out := deleteInstance(t, f, importHost); code != http.StatusOK || out["result"] != "deleted" {
+ t.Fatalf("delete = %d %v", code, out)
+ }
+ f.requireRecordGone(t, importHost)
+ }
+ if rows := importAudit(t, f, "instance-delete"); len(rows) != 2 {
+ t.Fatalf("instance-delete rows = %q, want one per delete", rows)
+ }
+}