amadan
tenant email: six residual fixes from the final whole-branch review
R1: Enable's repair path dropped PauseKind on rewrite, silently turning a CAP pause kind-less (nothing then auto-resumes it, and the CLI describes it as needing an operator) — now carried forward alongside Active. R2: enforceMailLimits' day-roll resume only ever ran on the SES event path, which a paused app (deactivated IAM key) never generates traffic for — an app alone on its own domain saw its cap pause as permanent. Extracted the check into resumeStaleCapPause, shared by enforceMailLimits and the new call in handleAPIEmailStatus, so a member reading `carlos email status` after the UTC day roll clears a stale cap pause itself. CLI copy updated to say so; a rate or kind-less pause is untouched either way. R3: appsettings.go's store-destroy door no longer runs mail.Teardown. Its confirm word is the bucket name and its flash names only the object store — consent for destroying the SES identity, IAM principal, delivered credentials, DNS and the global domain claim was never obtained there. ownership.go's handlePurgeApp (the whole-app delete) keeps the call. R4: moved domainMatchesEvent (with its own doc comment) back above recordAndEnforceMail's doc comment in sesevents.go, so the comment sits over the function it documents again. R5: hoisted the "email estate but no MailProvisioner" refusal in handlePurgeApp above objstore.Teardown, so a doomed purge refuses before destroying the store, matching appsettings.go's ordering. R6: spec §9 amended to match — Provisioned carries PauseKind now, and the cap-vs-rate resume rule (cap clears at the UTC day roll, checked on both the event and status-read paths; rate/kind-less never auto-resume) is recorded. Mutation-tested R1, R2, R3 (and R5, though not required): reverted each fix in turn and confirmed a test fails before restoring it. Full report in .superpowers/sdd/2026-08-19-tenant-email/residuals-report.md.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
13 files changed,
+486
−123
docs/superpowers/specs/2026-08-19-tenant-email-design.md+32 −5internal/console/apimail.go+14 −0internal/console/apimail_test.go+77 −0internal/console/appsettings.go+8 −41internal/console/appsettings_test.go+63 −0internal/console/deletion_test.go+47 −0internal/console/mailcounts.go+67 −32internal/console/mailcounts_test.go+105 −0internal/console/ownership.go+32 −22internal/console/sesevents.go+14 −14internal/mail/walk.go+14 −7internal/mail/walk_enable_test.go+6 −0mail_cli.go+7 −2
diff --git a/docs/superpowers/specs/2026-08-19-tenant-email-design.md b/docs/superpowers/specs/2026-08-19-tenant-email-design.md| index 96b8941c8..0b5668996 100644 |
| --- a/docs/superpowers/specs/2026-08-19-tenant-email-design.md |
| +++ b/docs/superpowers/specs/2026-08-19-tenant-email-design.md |
| @@ -555,11 +555,38 @@ complains. So a pause MUST surface at the moment it happens, with the |
| counts that caused it. A silent pause is a bug. |
| **Amended 2026-08-20 on where the reason lives.** The reason string reaches |
| -the **audit log** and nothing else: `Provisioned` carries only `Active`, so |
| -`carlos email status` shows the pause, the counts and the cap, and points at |
| -the audit log for the reason rather than printing it. Adding |
| -`Provisioned.Reason` is additive and small; until it exists this document |
| -must not claim the status surface carries it. |
| +the **audit log** and nothing else: `Provisioned` carries `Active` and |
| +`PauseKind`, not a free-text reason, so `carlos email status` shows the |
| +pause, the counts and the cap, and points at the audit log for the reason |
| +rather than printing it. Adding `Provisioned.Reason` is additive and small; |
| +until it exists this document must not claim the status surface carries it. |
| + |
| +**Amended 2026-08-20 on the two kinds of pause, and which one clears |
| +itself.** `Provisioned.PauseKind` distinguishes a **cap** pause (the daily |
| +budget spent) from a **rate** pause (a bounce/complaint threshold breached |
| +— abuse). The difference is load-bearing, because only one of the two is |
| +safe to lift on a timer: |
| + |
| +- A **cap** pause is a PER-DAY limit, computed from counts that reset every |
| + UTC day, so it clears itself once a day has rolled over since it was |
| + applied — same-day never clears, that would make the cap meaningless. |
| + The check runs in two places, sharing one function so they cannot drift: |
| + on the SES event path (`enforceMailLimits`, which is also where a cap |
| + pause is first applied), and on the `carlos email status` READ path. |
| + The second place exists because a paused app's IAM key is deactivated — |
| + it sends nothing, so no SES event ever arrives to run the event-path |
| + check — so an app alone on its own domain would otherwise see its cap |
| + pause as permanent; reading status is the moment that resume actually |
| + happens for it. |
| +- A **rate** pause never auto-resumes, day roll or not — SES's |
| + account-review thresholds are not a daily allowance, and lifting the |
| + brake on a schedule would let an abusive tenant breach it again every |
| + day forever. It needs a deployment operator, through `carlos email |
| + resume`. |
| +- A pause with no kind recorded (`PauseKind == ""` — every record written |
| + before this split, and every manual operator pause) is treated the same |
| + as a rate pause for this purpose: it never auto-resumes either. Fail |
| + safe: the default is "needs a human," not "clears itself." |
| **Suppression.** SES's account-level suppression list does the enforcing. |
| CARLOS ingests suppression events so a tenant can see and clear its own |
diff --git a/internal/console/apimail.go b/internal/console/apimail.go| index 948cc0093..6f52f4c21 100644 |
| --- a/internal/console/apimail.go |
| +++ b/internal/console/apimail.go |
| @@ -585,6 +585,20 @@ func (s *Server) handleAPIEmailStatus(w http.ResponseWriter, r *http.Request) { |
| }) |
| return |
| } |
| + // R2 (review follow-up): resume a stale CAP pause on the status READ |
| + // path, not only on the SES event path. A paused app's IAM key is |
| + // deactivated, so it sends nothing, so no events ever arrive to run |
| + // enforceMailLimits' day-roll check — a member running `carlos email |
| + // status` is exactly the moment a rolled-over daily budget should |
| + // clear. Shares resumeStaleCapPause with enforceMailLimits so the two |
| + // can never drift; a RATE pause or a kind-less pause is left untouched |
| + // by that function, fail-safe. |
| + rec, _, err = s.resumeStaleCapPause(ctx, acct.Sqid, app, rec, time.Now()) |
| + if err != nil { |
| + log.Printf("console: api email status resume %s/%s: %v", acct.Sqid, app, err) |
| + jsonError(w, http.StatusBadGateway, "could not read the email record — try again") |
| + return |
| + } |
| out, err := s.emailReply(ctx, acct, app, rec) |
| if err != nil { |
| log.Printf("console: api email delivery %s/%s: %v", acct.Sqid, app, err) |
diff --git a/internal/console/apimail_test.go b/internal/console/apimail_test.go| index bc5225c6a..2322f56d5 100644 |
| --- a/internal/console/apimail_test.go |
| +++ b/internal/console/apimail_test.go |
| @@ -379,6 +379,83 @@ func TestAPIEmailStatusDefaultHostEmptyWithNoPlatformSuffix(t *testing.T) { |
| } |
| } |
| +// TestAPIEmailStatusResumesAStaleCapPause: R2, review follow-up. The SES |
| +// event path (enforceMailLimits) is enforceMailLimits' only production |
| +// caller, and a paused app's IAM key is deactivated — it sends nothing, so |
| +// no SES event ever arrives to run that check. The status READ door must |
| +// run the same day-roll check itself, or an app alone on its own domain |
| +// would see its cap pause as permanent, contradicting what the CLI tells |
| +// the member. Proves the wiring end to end, over HTTP — not just the |
| +// shared function in isolation. |
| +func TestAPIEmailStatusResumesAStaleCapPause(t *testing.T) { |
| + fx, fake, _, token := emailArmed(t, "code-email-status-resume") |
| + app, domain := "titogo", "titogo.bab.oncarlos.com" |
| + provisionEmail(t, fx, fake, token, app, domain, "hello@"+domain) |
| + |
| + rec, etag, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, app) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + rec.Provisioned.Active = false |
| + rec.Provisioned.PauseKind = mail.PauseKindCap |
| + rec.Provisioned.At = time.Now().AddDate(0, 0, -1) // yesterday, UTC |
| + if _, err := mail.Put(context.Background(), fx.rel, release.OpsAccount, app, rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + status, out := getJSON(t, emailURL(fx, app), token) |
| + if status != http.StatusOK { |
| + t.Fatalf("status = %d %v", status, out) |
| + } |
| + if out["active"] != true { |
| + t.Fatalf("active = %v, want true — a cap pause from yesterday must clear on a status read", out["active"]) |
| + } |
| + |
| + got := emailRecord(t, fx, app) |
| + if !got.Provisioned.Active { |
| + t.Fatal("the record on disk is still paused after the status read resumed it") |
| + } |
| + if got.Provisioned.PauseKind != "" { |
| + t.Fatalf("PauseKind = %q on disk after resume, want cleared", got.Provisioned.PauseKind) |
| + } |
| +} |
| + |
| +// TestAPIEmailStatusLeavesARatePauseAlone: the same door must NEVER resume |
| +// a RATE pause — that pause exists because the tenant is abusive, not |
| +// because a budget rolled over, and it needs a human. |
| +func TestAPIEmailStatusLeavesARatePauseAlone(t *testing.T) { |
| + fx, fake, _, token := emailArmed(t, "code-email-status-no-resume-rate") |
| + app, domain := "titogo", "titogo.bab.oncarlos.com" |
| + provisionEmail(t, fx, fake, token, app, domain, "hello@"+domain) |
| + |
| + rec, etag, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, app) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + rec.Provisioned.Active = false |
| + rec.Provisioned.PauseKind = mail.PauseKindRate |
| + rec.Provisioned.At = time.Now().AddDate(0, 0, -1) // yesterday, UTC |
| + if _, err := mail.Put(context.Background(), fx.rel, release.OpsAccount, app, rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + status, out := getJSON(t, emailURL(fx, app), token) |
| + if status != http.StatusOK { |
| + t.Fatalf("status = %d %v", status, out) |
| + } |
| + if out["active"] != false { |
| + t.Fatalf("active = %v, want false — a rate pause must never auto-resume", out["active"]) |
| + } |
| + |
| + got := emailRecord(t, fx, app) |
| + if got.Provisioned.Active { |
| + t.Fatal("the record on disk was resumed — a rate pause must never auto-resume") |
| + } |
| + if got.Provisioned.PauseKind != mail.PauseKindRate { |
| + t.Fatalf("PauseKind = %q on disk, want it to survive as %q", got.Provisioned.PauseKind, mail.PauseKindRate) |
| + } |
| +} |
| + |
| // TestAPIEmailDeclareRefusesAnUnaddressedFrom: a From with no "@" is refused |
| // before anything is written. |
| func TestAPIEmailDeclareRefusesAnUnaddressedFrom(t *testing.T) { |
diff --git a/internal/console/appsettings.go b/internal/console/appsettings.go| index d5ada86bb..2d1755265 100644 |
| --- a/internal/console/appsettings.go |
| +++ b/internal/console/appsettings.go |
| @@ -38,7 +38,6 @@ import ( |
| "time" |
| "github.com/carlosframework/platform/internal/domains" |
| - "github.com/carlosframework/platform/internal/mail" |
| "github.com/carlosframework/platform/internal/objstore" |
| "github.com/carlosframework/platform/internal/release" |
| ) |
| @@ -600,37 +599,17 @@ func (s *Server) handleStoreDestroy(w http.ResponseWriter, r *http.Request) { |
| "this deployment has to tear it down.") |
| return |
| } |
| - // An email estate is torn down alongside the object store here too (I2, |
| - // review follow-up): mail.Teardown is the ONLY place that releases this |
| - // app's global domain claims (spec §4a), and this button — like |
| - // ownership.go's handlePurgeApp, which already does the same — is one |
| - // of the two backend-destroying surfaces objstore.Teardown was called |
| - // from with no email counterpart. Checked BEFORE requireFreshAuth, same |
| - // as the store's own backend check above: a doomed request must not |
| - // spend the passkey. |
| - mrec, _, mrerr := mail.Get(ctx, s.cfg.Releases, acct.Sqid, app) |
| - if mrerr != nil { |
| - log.Printf("console: store destroy mail read %s/%s: %v", acct.Sqid, app, mrerr) |
| - fail("Could not read “" + app + "”'s email record — nothing was destroyed. Try again.") |
| - return |
| - } |
| - mailEstate := mrec != nil && (mrec.Provisioned != nil || len(mrec.Standalone) > 0) |
| - if !mailEstate { |
| - // A domain claim can exist with no delivered credential at all — |
| - // `carlos email domains add` alone, never followed by `enable` — |
| - // and mail.Teardown releases that claim too. |
| - if doms, derr := s.mailAppDomains(ctx, acct, app); derr == nil && len(doms) > 0 { |
| - mailEstate = true |
| - } |
| - } |
| - if mailEstate && s.cfg.MailProvisioner == nil { |
| - fail("This console has no SES backend configured, so it cannot delete “" + app + "”'s email " + |
| - "credentials — nothing was destroyed. An operator of this deployment has to tear it down.") |
| - return |
| - } |
| if !s.requireFreshAuth(w, r, addr, raw) { |
| return |
| } |
| + // R3 (review follow-up): this door tears down ONLY the object store. |
| + // It used to also tear down the app's email estate — the SES identity, |
| + // the IAM principal, delivered credentials, published DNS and, |
| + // irreversibly and globally, the domain claim — but consent for THAT |
| + // was never obtained here: the confirm word is the bucket name, the |
| + // button says "Destroy store", and the success flash below names only |
| + // the object store. Email teardown belongs to ownership.go's |
| + // handlePurgeApp, which destroys the whole app and says so. |
| if err := objstore.Teardown(ctx, s.cfg.Releases, s.cfg.StoreProvisioner, acct.Sqid, app); err != nil { |
| log.Printf("console: store destroy %s/%s: %v", acct.Sqid, app, err) |
| if errors.Is(err, release.ErrCASConflict) { |
| @@ -641,18 +620,6 @@ func (s *Server) handleStoreDestroy(w http.ResponseWriter, r *http.Request) { |
| "backend is reachable.") |
| return |
| } |
| - if s.cfg.MailProvisioner != nil { |
| - mdep, mwriter := s.mailDeploymentFor() |
| - if terr := mail.Teardown(ctx, s.cfg.Releases, s.cfg.MailProvisioner, mwriter, mdep, acct.Sqid, app); terr != nil { |
| - log.Printf("console: store destroy mail teardown %s/%s: %v", acct.Sqid, app, terr) |
| - fail("Destroyed the object store, but could not tear down “" + app + "”'s email estate — " + |
| - "the store is already gone; try again to finish tearing down email.") |
| - return |
| - } |
| - if mailEstate { |
| - s.audit(acct, app, addr, "email-teardown", "tore down the email estate alongside the object store") |
| - } |
| - } |
| what, flash := "the store declaration", "Withdrew “"+app+"”'s store declaration." |
| if estate { |
| what = "bucket " + want |
diff --git a/internal/console/appsettings_test.go b/internal/console/appsettings_test.go| index 5440b1a24..9c1985f31 100644 |
| --- a/internal/console/appsettings_test.go |
| +++ b/internal/console/appsettings_test.go |
| @@ -18,6 +18,8 @@ import ( |
| "testing" |
| "time" |
| + "github.com/carlosframework/platform/internal/dnsrec" |
| + "github.com/carlosframework/platform/internal/mail" |
| "github.com/carlosframework/platform/internal/objstore" |
| "github.com/carlosframework/platform/internal/registry" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -818,6 +820,67 @@ func TestStoreDestroyDestroysTheEstateAndAnnounces(t *testing.T) { |
| } |
| } |
| +// TestStoreDestroyLeavesTheEmailEstateAlone: R3, review follow-up. A |
| +// previous fix wave wired mail.Teardown into THIS door — but this door's |
| +// confirm word is the bucket name, its button says "Destroy store", and its |
| +// success flash (asserted above) names only the object store. Nothing here |
| +// ever told the owner that confirming would also destroy the SES identity, |
| +// the IAM principal, delivered credentials, published DNS and — globally, |
| +// irreversibly — the domain claim. Consent for the store is not consent for |
| +// email; ownership.go's handlePurgeApp (the whole-app delete) is the door |
| +// that destroys both, and says so. |
| +// |
| +// The proof is the claim's own semantics (mirroring |
| +// TestPurgeReleasesTheDomainClaim in deletion_test.go), not an internal peek |
| +// at bucket keys: a SECOND, unrelated account trying to claim the SAME |
| +// domain is refused with ErrDomainClaimed both before and after the store |
| +// destroy — if this door released it, the second account's EnsureDomain |
| +// would start succeeding. |
| +func TestStoreDestroyLeavesTheEmailEstateAlone(t *testing.T) { |
| + fx := newConsoleFixture(t) |
| + fake := armStoreBackend(t, fx) |
| + bucket := seedStore(t, fx, fake, "kass", false) |
| + |
| + mfake := mail.NewMemProvisioner("1") |
| + writer := dnsrec.NewMemWriter() |
| + fx.srv.cfg.MailProvisioner = mfake |
| + fx.srv.cfg.DNSWriter = writer |
| + dep := mail.Deployment{Region: "eu-west-1"} // no PlatformSuffix, matching |
| + // TestPurgeReleasesTheDomainClaim: keeps this about the claim gate |
| + // alone, not the separate platform-name-ownership gate. |
| + ctx := context.Background() |
| + now := time.Now() |
| + domain := "store-destroy-claim.example.com" |
| + // Deliberately no mail.Declare/Enable — kass has a domain record (and |
| + // the claim it holds) but no Provisioned credential, exactly the shape |
| + // `carlos email domains add` alone leaves. If this door's old |
| + // mail.Teardown wiring were still here, it would find and release this |
| + // claim too — mail.Teardown traces the domain-referrer set, not just a |
| + // delivered credential. |
| + if _, _, err := mail.EnsureDomain(ctx, fx.rel, mfake, writer, dep, fx.sqid, "kass", domain, now); err != nil { |
| + t.Fatalf("EnsureDomain for kass: %v", err) |
| + } |
| + if _, _, err := mail.EnsureDomain(ctx, fx.rel, mfake, writer, dep, "zzzz", "other", domain, now); !errors.Is(err, mail.ErrDomainClaimed) { |
| + t.Fatalf("a second account's EnsureDomain before the destroy = %v, want ErrDomainClaimed", err) |
| + } |
| + |
| + c := signInPaul(t, fx) |
| + csrf := sessionToken(t, fx, c) |
| + status, loc, body := postGated(t, fx, c, fx.u("/apps/kass/store/destroy"), |
| + url.Values{"csrf": {csrf}, "confirm": {bucket}}) |
| + if status != http.StatusSeeOther || loc != fx.path("/apps/kass/settings") { |
| + t.Fatalf("store destroy = %d -> %q:\n%s", status, loc, body) |
| + } |
| + estateGone(t, fake, "a store destroy that must leave email alone") |
| + if rec := storedRecord(t, fx, "kass"); rec != nil { |
| + t.Fatalf("the store record survived its own destruction: %+v", rec) |
| + } |
| + |
| + if _, _, err := mail.EnsureDomain(ctx, fx.rel, mfake, writer, dep, "zzzz", "other", domain, now); !errors.Is(err, mail.ErrDomainClaimed) { |
| + t.Fatalf("the domain claim was released by a store destroy: err = %v, want it still ErrDomainClaimed", err) |
| + } |
| +} |
| + |
| // A declaration is a request nobody has acted on: no bucket, no key, nothing to |
| // spend backend authority on. So a console with no backend at all may still |
| // withdraw it — and the word to type is the APP's name, because that is the |
diff --git a/internal/console/deletion_test.go b/internal/console/deletion_test.go| index b7805cdad..824ff7294 100644 |
| --- a/internal/console/deletion_test.go |
| +++ b/internal/console/deletion_test.go |
| @@ -592,6 +592,53 @@ func TestPurgeRefusesAStoreThisConsoleCannotTearDown(t *testing.T) { |
| } |
| } |
| +// TestPurgeRefusesNoMailBackendBeforeTouchingTheStore: R5, review |
| +// follow-up. The "this app has an email estate but no MailProvisioner" |
| +// refusal used to sit AFTER objstore.Teardown had already destroyed the |
| +// object store, mirroring the very bug |
| +// TestPurgeRefusesAStoreThisConsoleCannotTearDown above already guards |
| +// against on the store side — a purge that was always going to be refused |
| +// destroyed the store first. Same shape, but for the email-side refusal: |
| +// the store must still be standing after it. |
| +func TestPurgeRefusesNoMailBackendBeforeTouchingTheStore(t *testing.T) { |
| + fx, c, csrf := trashPageFixture(t) |
| + fake := armStoreBackend(t, fx) |
| + bucket := seedStore(t, fx, fake, "kass", false) |
| + |
| + // An email estate with no MailProvisioner configured — this deployment |
| + // cannot tear it down, so the whole purge must refuse before it |
| + // touches anything. |
| + now := time.Now() |
| + if _, err := mail.Declare(context.Background(), fx.rel, release.OpsAccount, "kass", |
| + "KASS_SMTP", "noreply@kass.example.com", "paul@keymail.dev", now); err != nil { |
| + t.Fatal(err) |
| + } |
| + mrec, metag, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, "kass") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + mrec.Provisioned = &mail.Provisioned{IAMUser: "u", KeyID: "AKIA1", Active: true, At: now} |
| + if _, err := mail.Put(context.Background(), fx.rel, release.OpsAccount, "kass", mrec, metag); err != nil { |
| + t.Fatal(err) |
| + } |
| + fx.srv.cfg.MailProvisioner = nil |
| + |
| + status, loc, body := postGated(t, fx, c, fx.u("/trash/delete"), |
| + url.Values{"csrf": {csrf}, "app": {"kass"}, "confirm": {"kass"}}) |
| + if status != http.StatusSeeOther { |
| + t.Fatalf("purge with no mail backend = %d:\n%s", status, body) |
| + } |
| + // R5: the store must be untouched — the refusal must have fired BEFORE |
| + // objstore.Teardown, not after. |
| + estateStanding(t, fx, fake, "kass", bucket, "a purge refused for lacking a mail backend") |
| + if rec := storedRecord(t, fx, "kass"); rec == nil || rec.Provisioned == nil { |
| + t.Fatalf("the store record did not survive the refusal: %+v", rec) |
| + } |
| + if _, _, page := get(t, c, fx.ts.URL+loc); !strings.Contains(page, "no SES backend configured") { |
| + t.Fatalf("the refusal does not name the missing backend:\n%s", page) |
| + } |
| +} |
| + |
| // The whole point of the door: the backend estate comes down FIRST, and only |
| // then is the deployment prefix swept — store.json included, now that it names |
| // nothing. |
diff --git a/internal/console/mailcounts.go b/internal/console/mailcounts.go| index 2d3ceebbb..60e4bd265 100644 |
| --- a/internal/console/mailcounts.go |
| +++ b/internal/console/mailcounts.go |
| @@ -124,6 +124,64 @@ FROM mail_counts WHERE account = ? AND app = ? AND day = ?`, acct, app, day) |
| return c, nil |
| } |
| +// resumeStaleCapPause is the ONE place the day-roll resume check lives |
| +// (R2, review follow-up). enforceMailLimits' only production caller is the |
| +// SES event path (recordAndEnforceMail): a paused app's IAM key is |
| +// deactivated, so it sends nothing, so no events ever arrive to run that |
| +// check, so an app alone on its own domain saw its cap pause as permanent |
| +// as before the cap/rate split — while the CLI now promises it clears |
| +// automatically. The email-status READ path (handleAPIEmailStatus) is the |
| +// other caller: a member running `carlos email status` is exactly the |
| +// moment a rolled-over daily budget should clear, and it is a moment that |
| +// happens without SES traffic. |
| +// |
| +// Both callers share this one function so they can never drift. |
| +// |
| +// Fail-safe by construction, matching "RATE AND MANUAL PAUSES NEVER |
| +// AUTO-RESUME" below: resumes ONLY when rec is currently paused (Active == |
| +// false), the pause is a CAP pause (PauseKind == mail.PauseKindCap — a |
| +// RATE pause or a kind-less manual/pre-existing pause, PauseKind == "", |
| +// both fall through untouched), and the pause was applied on a UTC day |
| +// STRICTLY EARLIER than now — same-day resumes would make the cap |
| +// meaningless. |
| +// |
| +// Returns the freshly re-read record when it resumes (rec's SetPaused |
| +// write moves Active, PauseKind and At, so callers must not patch the |
| +// in-memory copy by hand) and whether a resume happened; rec is passed |
| +// straight back, and resumed is false, in every other case, including |
| +// rec == nil or rec.Provisioned == nil. |
| +func (s *Server) resumeStaleCapPause(ctx context.Context, acct, app string, rec *mail.Record, now time.Time) (*mail.Record, bool, error) { |
| + if rec == nil || rec.Provisioned == nil || rec.Provisioned.Active { |
| + return rec, false, nil |
| + } |
| + // A RATE pause or a MANUAL (operator) pause — PauseKind is anything |
| + // other than mail.PauseKindCap, including "" — never auto-resumes: see |
| + // "RATE AND MANUAL PAUSES NEVER AUTO-RESUME" below. |
| + if rec.Provisioned.PauseKind != mail.PauseKindCap { |
| + return rec, false, nil |
| + } |
| + pausedDay := rec.Provisioned.At.UTC().Format("2006-01-02") |
| + today := now.UTC().Format("2006-01-02") |
| + if pausedDay >= today { |
| + // Paused earlier TODAY (or a clock oddity puts the pause in the |
| + // future) — not a day roll yet. |
| + return rec, false, nil |
| + } |
| + if _, err := mail.SetPaused(ctx, s.cfg.Releases, s.cfg.MailProvisioner, acct, app, false, "", now); err != nil { |
| + return nil, false, err |
| + } |
| + if a, err := s.store.AccountBySqid(acct); err != nil { |
| + log.Printf("console: mail resume audit: resolving account %s: %v", acct, err) |
| + } else if a != nil { |
| + s.audit(a, app, "ses", "mail.resumed", fmt.Sprintf("%s: daily cap pause cleared on the UTC day roll", app)) |
| + } |
| + fresh, _, err := mail.Get(ctx, s.cfg.Releases, acct, app) |
| + if err != nil { |
| + return nil, false, err |
| + } |
| + return fresh, true, nil |
| +} |
| + |
| // enforceMailLimits pauses an app that has exceeded its daily cap or is past |
| // either SES threshold, and reports why. |
| // |
| @@ -145,41 +203,18 @@ func (s *Server) enforceMailLimits(ctx context.Context, acct, app string, now ti |
| return false, "", nil |
| } |
| if !rec.Provisioned.Active { |
| - // I3 (review follow-up): a CAP pause is a PER-DAY limit, because |
| - // the counts it was computed from are per-day — it MUST clear on |
| - // the UTC day roll, or a free-tier member's ordinary first day |
| - // over 50 messages stops their email forever, with resume gated |
| - // behind an operator allowlist that ships empty |
| - // (CARLOS_MAIL_OPERATORS, fail-closed). A RATE pause or a MANUAL |
| - // (operator) pause — PauseKind is anything other than |
| - // mail.PauseKindCap, including "" — still never auto-resumes: see |
| - // "RATE AND MANUAL PAUSES NEVER AUTO-RESUME" below, which still |
| - // holds for those two. |
| - if rec.Provisioned.PauseKind != mail.PauseKindCap { |
| - return false, "", nil |
| - } |
| - pausedDay := rec.Provisioned.At.UTC().Format("2006-01-02") |
| - today := now.UTC().Format("2006-01-02") |
| - if pausedDay >= today { |
| - // Paused earlier TODAY (or a clock oddity puts the pause in |
| - // the future) — not a day roll yet. |
| - return false, "", nil |
| - } |
| - if _, err := mail.SetPaused(ctx, s.cfg.Releases, s.cfg.MailProvisioner, acct, app, false, "", now); err != nil { |
| - return false, "", err |
| - } |
| - if a, err := s.store.AccountBySqid(acct); err != nil { |
| - log.Printf("console: mail resume audit: resolving account %s: %v", acct, err) |
| - } else if a != nil { |
| - s.audit(a, app, "ses", "mail.resumed", fmt.Sprintf("%s: daily cap pause cleared on the UTC day roll", app)) |
| - } |
| - // Re-read: SetPaused just wrote a fresh record (Active, PauseKind |
| - // and At all moved), and everything below reads rec fresh rather |
| - // than patching the in-memory copy by hand. |
| - rec, _, err = mail.Get(ctx, s.cfg.Releases, acct, app) |
| + fresh, resumed, err := s.resumeStaleCapPause(ctx, acct, app, rec, now) |
| if err != nil { |
| return false, "", err |
| } |
| + if !resumed { |
| + return false, "", nil |
| + } |
| + // Re-read via resumeStaleCapPause: SetPaused just wrote a fresh |
| + // record (Active, PauseKind and At all moved), and everything |
| + // below reads rec fresh rather than patching the in-memory copy by |
| + // hand. |
| + rec = fresh |
| if rec == nil || rec.Provisioned == nil || !rec.Provisioned.Active { |
| return false, "", nil |
| } |
diff --git a/internal/console/mailcounts_test.go b/internal/console/mailcounts_test.go| index ac27f01fe..ed54dc1dc 100644 |
| --- a/internal/console/mailcounts_test.go |
| +++ b/internal/console/mailcounts_test.go |
| @@ -351,3 +351,108 @@ func TestEnforceRecordsTheReasonInTheAuditLog(t *testing.T) { |
| } |
| t.Fatalf("no audit entry naming the pause reason: %+v", entries) |
| } |
| + |
| +// pauseDirectly writes rec.Provisioned's Active/PauseKind/At by hand — the |
| +// way SetPaused already would, without needing to breach a threshold |
| +// first. Used by the resumeStaleCapPause tests below, which pin the shared |
| +// day-roll function's contract directly (R2, review follow-up) rather than |
| +// only through enforceMailLimits' caller. |
| +func pauseDirectly(t *testing.T, fx *consoleFixture, app, kind string, at time.Time) { |
| + t.Helper() |
| + rec, etag, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, app) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + rec.Provisioned.Active = false |
| + rec.Provisioned.PauseKind = kind |
| + rec.Provisioned.At = at |
| + if _, err := mail.Put(context.Background(), fx.rel, release.OpsAccount, app, rec, etag); err != nil { |
| + t.Fatal(err) |
| + } |
| +} |
| + |
| +// R2: resumeStaleCapPause is the ONE function both enforceMailLimits (the |
| +// SES event path) and handleAPIEmailStatus (the status READ path) share, |
| +// so these four cases pin its contract directly. |
| + |
| +func TestResumeStaleCapPauseClearsACapPauseFromYesterday(t *testing.T) { |
| + fx, now := mailFixture(t) |
| + pauseDirectly(t, fx, "seapointish", mail.PauseKindCap, now.AddDate(0, 0, -1)) |
| + rec, _, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, "seapointish") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + got, resumed, err := fx.srv.resumeStaleCapPause(context.Background(), release.OpsAccount, "seapointish", rec, now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if !resumed { |
| + t.Fatal("did not resume a cap pause applied on an earlier UTC day") |
| + } |
| + if !got.Provisioned.Active { |
| + t.Fatal("resumed record still reports Active == false") |
| + } |
| + if got.Provisioned.PauseKind != "" { |
| + t.Fatalf("PauseKind = %q after resume, want cleared", got.Provisioned.PauseKind) |
| + } |
| +} |
| + |
| +func TestResumeStaleCapPauseLeavesATodayCapPauseAlone(t *testing.T) { |
| + fx, now := mailFixture(t) |
| + pauseDirectly(t, fx, "seapointish", mail.PauseKindCap, now) // paused earlier the SAME UTC day |
| + rec, _, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, "seapointish") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + got, resumed, err := fx.srv.resumeStaleCapPause(context.Background(), release.OpsAccount, "seapointish", rec, now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if resumed { |
| + t.Fatal("resumed a cap pause applied the SAME UTC day — that makes the cap meaningless") |
| + } |
| + if got.Provisioned.Active { |
| + t.Fatal("same-day cap pause was lifted") |
| + } |
| +} |
| + |
| +func TestResumeStaleCapPauseNeverTouchesARatePause(t *testing.T) { |
| + fx, now := mailFixture(t) |
| + pauseDirectly(t, fx, "seapointish", mail.PauseKindRate, now.AddDate(0, 0, -1)) |
| + rec, _, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, "seapointish") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + got, resumed, err := fx.srv.resumeStaleCapPause(context.Background(), release.OpsAccount, "seapointish", rec, now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if resumed { |
| + t.Fatal("resumed a RATE pause — a rate pause must never auto-resume, day roll or not") |
| + } |
| + if got.Provisioned.Active { |
| + t.Fatal("a rate pause from yesterday was lifted") |
| + } |
| + if got.Provisioned.PauseKind != mail.PauseKindRate { |
| + t.Fatalf("PauseKind = %q, want it to survive as %q", got.Provisioned.PauseKind, mail.PauseKindRate) |
| + } |
| +} |
| + |
| +func TestResumeStaleCapPauseNeverTouchesAKindlessPause(t *testing.T) { |
| + fx, now := mailFixture(t) |
| + pauseDirectly(t, fx, "seapointish", "", now.AddDate(0, 0, -1)) |
| + rec, _, err := mail.Get(context.Background(), fx.rel, release.OpsAccount, "seapointish") |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + got, resumed, err := fx.srv.resumeStaleCapPause(context.Background(), release.OpsAccount, "seapointish", rec, now) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if resumed { |
| + t.Fatal("resumed a kind-less pause (pre-existing record or manual pause) — fail safe means these never auto-resume") |
| + } |
| + if got.Provisioned.Active { |
| + t.Fatal("a kind-less pause from yesterday was lifted") |
| + } |
| +} |
diff --git a/internal/console/ownership.go b/internal/console/ownership.go| index 4d2a05aec..9889757c9 100644 |
| --- a/internal/console/ownership.go |
| +++ b/internal/console/ownership.go |
| @@ -340,6 +340,36 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) { |
| fail(purgeAdvice(err, app)) |
| return |
| } |
| + // R5 (review follow-up): this refusal — "there is an email estate but |
| + // no MailProvisioner to tear it down with" — is read and checked BEFORE |
| + // the store teardown below, not after. It used to sit next to the |
| + // mail.Teardown call further down, which is AFTER objstore.Teardown |
| + // already destroys the object store — so a purge that was always going |
| + // to be refused destroyed the store first. Hoisted here, matching how |
| + // appsettings.go's store-destroy door already orders its own |
| + // backend-missing checks before either destructive call. mrec and |
| + // mailEstate are reused below, at the actual mail.Teardown call, so the |
| + // two can never disagree about what estate exists. |
| + mrec, _, mrerr := mail.Get(r.Context(), s.cfg.Releases, acct.Sqid, app) |
| + if mrerr != nil { |
| + log.Printf("console: purge mail read %s/%s: %v", acct.Sqid, app, mrerr) |
| + fail("Could not read “" + app + "”'s email record — nothing was deleted; try again.") |
| + return |
| + } |
| + mailEstate := mrec != nil && (mrec.Provisioned != nil || len(mrec.Standalone) > 0) |
| + if !mailEstate { |
| + // A domain claim can exist with no delivered credential at all — |
| + // `carlos email domains add` alone, never followed by `enable` — |
| + // and mail.Teardown releases that claim too. The audit trail |
| + // below should say so. |
| + if doms, derr := s.mailAppDomains(r.Context(), acct, app); derr == nil && len(doms) > 0 { |
| + mailEstate = true |
| + } |
| + } |
| + if mailEstate && s.cfg.MailProvisioner == nil { |
| + fail("“" + app + "” has a provisioned email credential, and this console has no SES backend configured — it cannot be deleted here until email is torn down.") |
| + return |
| + } |
| // A provisioned store is torn down BEFORE the prefix sweep — this door |
| // is the only purge path with backend authority (objstore spec §9; |
| // box-side auto-purge refuses via release.ErrStoreProvisioned). Failure |
| @@ -393,28 +423,8 @@ func (s *Server) handlePurgeApp(w http.ResponseWriter, r *http.Request) { |
| // records and this app's ORPHANED claims even when the app never |
| // declared email at all — its own doc comment), so it runs whenever a |
| // MailProvisioner is configured, not only when a Provisioned/Standalone |
| - // estate is already known to exist; the estate check below only decides |
| - // whether a MISSING provisioner has to refuse the whole delete. |
| - mrec, _, mrerr := mail.Get(r.Context(), s.cfg.Releases, acct.Sqid, app) |
| - if mrerr != nil { |
| - log.Printf("console: purge mail read %s/%s: %v", acct.Sqid, app, mrerr) |
| - fail("Could not read “" + app + "”'s email record — nothing was deleted; try again.") |
| - return |
| - } |
| - mailEstate := mrec != nil && (mrec.Provisioned != nil || len(mrec.Standalone) > 0) |
| - if !mailEstate { |
| - // A domain claim can exist with no delivered credential at all — |
| - // `carlos email domains add` alone, never followed by `enable` — |
| - // and mail.Teardown releases that claim too. The audit trail |
| - // below should say so. |
| - if doms, derr := s.mailAppDomains(r.Context(), acct, app); derr == nil && len(doms) > 0 { |
| - mailEstate = true |
| - } |
| - } |
| - if mailEstate && s.cfg.MailProvisioner == nil { |
| - fail("“" + app + "” has a provisioned email credential, and this console has no SES backend configured — it cannot be deleted here until email is torn down.") |
| - return |
| - } |
| + // estate is already known to exist. mrec and mailEstate were already |
| + // computed and refused on above (R5), before the store teardown ran. |
| if s.cfg.MailProvisioner != nil { |
| mctx, mcancel := context.WithTimeout(r.Context(), storeTeardownTimeout) |
| defer mcancel() |
diff --git a/internal/console/sesevents.go b/internal/console/sesevents.go| index c9618b0a8..0aaa0b2c7 100644 |
| --- a/internal/console/sesevents.go |
| +++ b/internal/console/sesevents.go |
| @@ -413,20 +413,6 @@ func (s *Server) handleSESEvents(w http.ResponseWriter, r *http.Request) { |
| } |
| } |
| -// recordAndEnforceMail resolves ev.Domain to the account(s)/app(s) that own |
| -// it and, for each, records the count and re-evaluates the cap and rate |
| -// thresholds (Task 16, mailcounts.go). Best-effort throughout: a lookup or |
| -// storage failure here is logged and swallowed, never turned into a non-2xx |
| -// — the caller has already committed to answering 200 for every recognised |
| -// notification, retried or not, and losing one count must not become an |
| -// unbounded SNS retry flood. |
| -// |
| -// Domain identity (mail.DomainRecord) is ACCOUNT-scoped, and there is no |
| -// reverse index from a bare domain to the account that owns it — so every |
| -// live console account is walked. Console deployments run a small, |
| -// bounded number of accounts; this is the same shape ListDomains' own |
| -// callers (mail.Teardown, mail.grantedDomains) already accept for a single |
| -// account, one level up. |
| // domainMatchesEvent reports whether ev's domain attributes to d — either |
| // the identity's own domain (the sender-supplied From SES usually reports |
| // as mail.source) or, for ANY region, d's recorded custom MAIL FROM |
| @@ -450,6 +436,20 @@ func domainMatchesEvent(d *mail.DomainRecord, domain string) bool { |
| return false |
| } |
| +// recordAndEnforceMail resolves ev.Domain to the account(s)/app(s) that own |
| +// it and, for each, records the count and re-evaluates the cap and rate |
| +// thresholds (Task 16, mailcounts.go). Best-effort throughout: a lookup or |
| +// storage failure here is logged and swallowed, never turned into a non-2xx |
| +// — the caller has already committed to answering 200 for every recognised |
| +// notification, retried or not, and losing one count must not become an |
| +// unbounded SNS retry flood. |
| +// |
| +// Domain identity (mail.DomainRecord) is ACCOUNT-scoped, and there is no |
| +// reverse index from a bare domain to the account that owns it — so every |
| +// live console account is walked. Console deployments run a small, |
| +// bounded number of accounts; this is the same shape ListDomains' own |
| +// callers (mail.Teardown, mail.grantedDomains) already accept for a single |
| +// account, one level up. |
| func (s *Server) recordAndEnforceMail(ctx context.Context, ev mail.Event) { |
| accts, err := s.store.Accounts() |
| if err != nil { |
diff --git a/internal/mail/walk.go b/internal/mail/walk.go| index 6ba3cf7c6..f7e7c5aca 100644 |
| --- a/internal/mail/walk.go |
| +++ b/internal/mail/walk.go |
| @@ -546,15 +546,22 @@ func Enable(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| } |
| } |
| - // Active is carried forward, not reset. A re-walk that repairs missing |
| - // delivery for a paused app is legitimate; silently un-pausing it is |
| - // not — Active only ever moves from false to true through the pause |
| - // verb itself (Task 13), never as a side effect of this walk running. |
| - // A first-time enable (no prior Provisioned block) starts active. |
| - active, prev := true, "" |
| + // Active and PauseKind are carried forward, not reset. A re-walk that |
| + // repairs missing delivery for a paused app is legitimate; silently |
| + // un-pausing it is not — Active only ever moves from false to true |
| + // through the pause verb itself (Task 13), never as a side effect of |
| + // this walk running. PauseKind must travel with Active: dropping it |
| + // here would turn a CAP pause into a kind-less one on the very next |
| + // repair walk, which enforceMailLimits and the status-read auto-resume |
| + // (R2) never resume, and which the CLI describes as needing an |
| + // operator — resurrecting the exact defect PauseKind was added to fix, |
| + // through this door instead of SetPaused. A first-time enable (no |
| + // prior Provisioned block) starts active with no pause kind. |
| + active, prev, pauseKind := true, "", "" |
| if rec.Provisioned != nil { |
| prev = rec.Provisioned.PrevKeyID |
| active = rec.Provisioned.Active |
| + pauseKind = rec.Provisioned.PauseKind |
| } |
| // Reap before minting: reaching here means delivery is missing, so any |
| @@ -586,7 +593,7 @@ func Enable(ctx context.Context, s release.Store, p Provisioner, sealPub *ecdh.P |
| } |
| rec.Provisioned = &Provisioned{ |
| - IAMUser: user, UserARN: arn, KeyID: keyID, PrevKeyID: prev, Active: active, At: now, |
| + IAMUser: user, UserARN: arn, KeyID: keyID, PrevKeyID: prev, Active: active, PauseKind: pauseKind, At: now, |
| } |
| if _, err := Put(ctx, s, acct, app, rec, etag); err != nil { |
| return nil, err |
diff --git a/internal/mail/walk_enable_test.go b/internal/mail/walk_enable_test.go| index 9e861d953..488d6083b 100644 |
| --- a/internal/mail/walk_enable_test.go |
| +++ b/internal/mail/walk_enable_test.go |
| @@ -223,6 +223,7 @@ func TestEnableCarriesAPausedActiveForwardThroughARewalk(t *testing.T) { |
| t.Fatalf("Get: %v", err) |
| } |
| got.Provisioned.Active = false |
| + got.Provisioned.PauseKind = PauseKindCap |
| if _, err := Put(ctx, s, "bag", "seapointish", got, etag); err != nil { |
| t.Fatalf("Put: %v", err) |
| } |
| @@ -240,6 +241,11 @@ func TestEnableCarriesAPausedActiveForwardThroughARewalk(t *testing.T) { |
| if second.Provisioned.Active { |
| t.Fatal("a re-walk silently resumed a paused credential") |
| } |
| + // R1: PauseKind must survive the rewrite alongside Active — dropping it |
| + // turns a CAP pause into a kind-less one, which nothing auto-resumes. |
| + if second.Provisioned.PauseKind != PauseKindCap { |
| + t.Fatalf("PauseKind = %q, want %q to survive the re-walk", second.Provisioned.PauseKind, PauseKindCap) |
| + } |
| } |
| // If the record ever names a key that is not what was actually delivered |
diff --git a/mail_cli.go b/mail_cli.go| index b4b123557..34707b2d4 100644 |
| --- a/mail_cli.go |
| +++ b/mail_cli.go |
| @@ -846,13 +846,18 @@ func printEmailStatus(w io.Writer, res apiEmailReply) { |
| // and 404s for an ordinary member — pointing every paused |
| // member at it, regardless of why, was actively wrong advice. |
| // A CAP pause (PauseKindCap) clears itself on the next UTC day |
| - // roll (mailcounts.go's enforceMailLimits); nothing to run. A |
| + // roll — evaluated both on the SES event path |
| + // (mailcounts.go's enforceMailLimits) and on THIS status read |
| + // itself (R2, review follow-up: a paused app sends nothing, so |
| + // no SES event ever arrives to trigger the event-path check — |
| + // running `carlos email status` after the roll is what |
| + // actually clears it for an app alone on its own domain). A |
| // RATE pause, or a manual pause this reply carries no kind |
| // for, does not clear on its own and genuinely needs an |
| // operator. |
| switch res.Provisioned.PauseKind { |
| case "cap": |
| - mutedf(w, " daily cap reached — this clears automatically at the next UTC day roll; no action needed") |
| + mutedf(w, " daily cap reached — this clears at the next UTC day roll; running `carlos email status` after that will clear it") |
| case "rate": |
| mutedf(w, " bounce/complaint rate over threshold — this does not clear on its own; ask a deployment operator to resume it") |
| default: |