amadan

carlos / platform Public

fix(console): the trash window has exactly one clock — the marker's

TrashApp is create-only, so a re-trash keeps the original marker. When the
follow-up read of that marker failed, the handler used to fall back to this
request's own clock: it answered a restorable_until 30 days out while
auto-purge measures from the marker's deleted_at, and it stamped the row
with a time that could never be corrected — materialize's trash case
short-circuits on an already-trashed row and TrashAppRow is guarded by
deleted_at = 0.

A window this request cannot confirm is now a 502 with nothing written to
the row; the retry takes the same idempotent path and reads the true
window. The row is stamped from the marker on every path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Paul Campbell 06fc6ab71b978c67ece3f79889e7cb4a8ab856e2 parent 7ec5daf
2 files changed, +114 −12
  • internal/console/apicli.go +25 −12
  • internal/console/apicli_test.go +89 −0
diff --git a/internal/console/apicli.go b/internal/console/apicli.go
index d20a3fd5e..4e81ff794 100644
--- a/internal/console/apicli.go
+++ b/internal/console/apicli.go
@@ -445,20 +445,33 @@ func (s *Server) handleAPITrashApp(w http.ResponseWriter, r *http.Request) {
jsonError(w, http.StatusBadGateway, "could not write the trash marker — nothing was deleted; try again")
return
}
- // Re-read, because TrashApp is create-only: an app already in the trash
- // keeps its ORIGINAL clock, and the caller must be told the deadline that
- // actually applies rather than the one this request would have set. A
- // marker that cannot be re-read is not worth failing over — it is
- // WRITTEN, the app is trashed — so the answer falls back to this
- // request's own clock, which is right in every case but the idempotent
- // re-trash.
- deletedAt := now
+ // Re-read, because TrashApp is create-only: an app already in the trash —
+ // trashed headless, or by a racing request — keeps its ORIGINAL clock,
+ // and that marker is the ONLY clock in this feature. The auto-purge pass
+ // destroys at the marker's DeletedAt + TrashCooloff, so a window invented
+ // here would over-promise the deadline, and a row stamped with this
+ // request's time would never self-heal: materialize's trash case
+ // short-circuits on an already-trashed row, and TrashAppRow is guarded by
+ // deleted_at = 0.
+ //
+ // So a marker that cannot be re-read is a 502 with NOTHING written to the
+ // row. The marker is on disk either way — the app IS trashed — and the
+ // retry takes the idempotent path and reads the true window out of it.
+ // Answering "trashed, until <a date nobody will honour>" is the one
+ // outcome a deletion feature must not produce.
tomb, err := release.ReadAppTombstone(ctx, s.cfg.Releases, acct.Sqid, app)
- if err != nil {
- log.Printf("console: api trash %s/%s: re-read marker: %v", acct.Sqid, app, err)
- } else if tomb != nil {
- deletedAt = tomb.DeletedAt
+ if err != nil || tomb == nil {
+ if err != nil {
+ log.Printf("console: api trash %s/%s: re-read marker: %v", acct.Sqid, app, err)
+ } else {
+ // Written and already gone: a restore raced this trash. Same
+ // answer — this request cannot say what the window is.
+ log.Printf("console: api trash %s/%s: marker vanished between write and read", acct.Sqid, app)
+ }
+ jsonError(w, http.StatusBadGateway, "could not confirm the trash window — try again")
+ return
}
+ deletedAt := tomb.DeletedAt
if err := s.store.TrashAppRow(acct.ID, app, deletedAt); err != nil {
log.Printf("console: api trash row %s/%s: %v", acct.Sqid, app, err)
jsonError(w, http.StatusInternalServerError,
diff --git a/internal/console/apicli_test.go b/internal/console/apicli_test.go
index cda333a2c..0534b16a5 100644
--- a/internal/console/apicli_test.go
+++ b/internal/console/apicli_test.go
@@ -1277,6 +1277,17 @@ func TestAPITrashAndRestoreApp(t *testing.T) {
if has, err := fx.st.HasTrashedAppIn(OpsAccountID, "titogo"); err != nil || !has {
t.Fatalf("HasTrashedAppIn after trash = %v %v, want true", has, err)
}
+ // One clock, and it is the MARKER's: the auto-purge pass measures the 30
+ // days from there, so a row stamped with anything else would give the
+ // Trash page a "days remaining" nothing honours.
+ trashed, err := fx.st.TrashedAppsIn(OpsAccountID)
+ if err != nil || len(trashed) != 1 || trashed[0].App != "titogo" {
+ t.Fatalf("TrashedAppsIn = %+v %v, want exactly titogo", trashed, err)
+ }
+ if trashed[0].DeletedAt.Unix() != tomb.DeletedAt.Unix() {
+ t.Fatalf("row deleted_at = %s, marker deleted_at = %s — they must be the same clock",
+ trashed[0].DeletedAt, tomb.DeletedAt)
+ }
// Every other door resolves through the same live-apps-only lookup, so a
// trashed app is simply not there any more.
@@ -1367,6 +1378,84 @@ func TestAPITrashTwiceKeepsTheClock(t *testing.T) {
}
}
+// blindMarkerStore writes the trash marker happily and then refuses to read
+// it back — the shape a half-broken bucket (or a permissions split) takes,
+// and the one case where this handler has written something it cannot
+// describe.
+type blindMarkerStore struct{ release.Store }
+
+func (b blindMarkerStore) Get(ctx context.Context, key string) ([]byte, string, error) {
+ if strings.HasSuffix(key, "/deleted.json") {
+ return nil, "", errors.New("AccessDenied: s3://carlos-prod-secret/deleted.json request id ABC123")
+ }
+ return b.Store.Get(ctx, key)
+}
+
+// TestAPITrashUnconfirmedWindowIsRefused: when the marker cannot be re-read,
+// the handler must not invent a deadline. Auto-purge destroys at the
+// MARKER's clock, and the row's own clock never self-heals (materialize
+// short-circuits on an already-trashed row, TrashAppRow is guarded by
+// deleted_at = 0) — so a guess here is a wrong "days remaining" forever.
+// 502, nothing written, and the retry produces the truth.
+func TestAPITrashUnconfirmedWindowIsRefused(t *testing.T) {
+ fx, token := trashFixture(t, "code-trash-unconfirmed")
+ // The app was already trashed headless — the marker exists with its own
+ // (old) clock while this console's row is still live, so the API call
+ // below takes TrashApp's idempotent path.
+ old := time.Now().Add(-9 * 24 * time.Hour)
+ if err := release.TrashApp(context.Background(), fx.rel, release.OpsAccount, "titogo", "paul@keymail.dev", old); err != nil {
+ t.Fatal(err)
+ }
+ fx.srv.cfg.Releases = blindMarkerStore{fx.rel}
+
+ status, body := postBody(t, fx.ts.URL+"/api/cli/apps/bab/titogo/delete", token, "")
+ if status != http.StatusBadGateway {
+ t.Fatalf("delete with an unreadable marker = %d %s, want 502", status, body)
+ }
+ if !strings.Contains(string(body), "try again") {
+ t.Fatalf("refusal = %s, want it to invite the retry that produces the true window", body)
+ }
+ if strings.Contains(string(body), "AccessDenied") || strings.Contains(string(body), "s3://") {
+ t.Fatalf("refusal leaks the store's own error text: %s", body)
+ }
+ // The row must be untouched: a row stamped now would outlive the marker's
+ // real deadline and could never be corrected.
+ if has, err := fx.st.HasTrashedAppIn(OpsAccountID, "titogo"); err != nil || has {
+ t.Fatalf("HasTrashedAppIn after an unconfirmed trash = %v %v, want false", has, err)
+ }
+ if has, err := fx.st.HasAppIn(OpsAccountID, "titogo"); err != nil || !has {
+ t.Fatalf("HasAppIn after an unconfirmed trash = %v %v, want the row still live", has, err)
+ }
+
+ // The retry, against a bucket that answers: the SAME idempotent path,
+ // now reporting the marker's original window and stamping the row with it.
+ fx.srv.cfg.Releases = fx.rel
+ status, out := postBody(t, fx.ts.URL+"/api/cli/apps/bab/titogo/delete", token, "")
+ if status != http.StatusOK {
+ t.Fatalf("retry delete = %d %s, want 200", status, out)
+ }
+ var res struct {
+ RestorableUntil string `json:"restorable_until"`
+ }
+ if err := json.Unmarshal(out, &res); err != nil {
+ t.Fatal(err)
+ }
+ until, err := time.Parse(time.RFC3339, res.RestorableUntil)
+ if err != nil {
+ t.Fatalf("restorable_until %q: %v", res.RestorableUntil, err)
+ }
+ if want := old.Add(release.TrashCooloff); until.Sub(want) > time.Minute || want.Sub(until) > time.Minute {
+ t.Fatalf("restorable_until = %s, want the ORIGINAL marker's window ~%s", until, want)
+ }
+ trashed, err := fx.st.TrashedAppsIn(OpsAccountID)
+ if err != nil || len(trashed) != 1 {
+ t.Fatalf("TrashedAppsIn = %+v %v, want exactly titogo", trashed, err)
+ }
+ if trashed[0].DeletedAt.Unix() != old.UTC().Unix() {
+ t.Fatalf("row deleted_at = %s, want the marker's own clock %s", trashed[0].DeletedAt, old.UTC())
+ }
+}
+
// TestAPITrashNonMemberIs404: a token holder who is not a member of the
// account learns nothing at all — not that the account exists, not that the
// app does, and not that either has a trash. Byte-identical to the 404 a