amadan

carlos / platform Public

console: make the per-sender cap test discriminating; per-instance resume conflict

TestTheCapIsEnforcedPerSender paused on 50 sends regardless of whether a
sibling's traffic was wrongly summed in, and never checked the sibling's
own record, so it did not detect a regression to the old summed read or a
pause landing on the wrong sender. Rewritten to a 25/40 split that trips
only under a summed read, plus two new tests that pin the pause target and
the PerInstance resume guard directly; verified against both reported
mutations with go test -overlay.

Also gives ErrPerInstance its own branch in the operator pause door
(apimail.go) instead of falling into the generic transient-failure 502, so
a resume refused for per-instance mode is not misreported as something a
retry could fix.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Paul Campbell pushed by claude 84e4072f0cb842cb12e0ebb1dac5de784a1cf65b parent eb0bb9d
3 files changed, +166 −4
  • internal/console/apimail.go +8 −0
  • internal/console/apimail_test.go +41 −0
  • internal/console/mailcounts_test.go +117 −4
diff --git a/internal/console/apimail.go b/internal/console/apimail.go
index 9ebbbb5a0..6c895e1a0 100644
--- a/internal/console/apimail.go
+++ b/internal/console/apimail.go
@@ -1369,6 +1369,14 @@ func (s *Server) handleAPIEmailPause(w http.ResponseWriter, r *http.Request) {
jsonError(w, http.StatusConflict, "no provisioned email credential to pause — enable email first")
return
}
+ // A permanent refusal, not a transient one. A retry never succeeds
+ // on its own: the app has moved to per-instance senders, and the
+ // pause an operator wants belongs to an instance sender, not the
+ // app sender this door still resumes.
+ if errors.Is(err, mail.ErrPerInstance) {
+ jsonError(w, http.StatusConflict, "the app is in per-instance mode. resume its instance senders instead of the app sender.")
+ return
+ }
if err != nil {
log.Printf("console: api email pause %s/%s: %v", acct.Sqid, app, err)
jsonError(w, http.StatusBadGateway, "could not change the pause state — try again")
diff --git a/internal/console/apimail_test.go b/internal/console/apimail_test.go
index 82f57ed3d..cbe1b9997 100644
--- a/internal/console/apimail_test.go
+++ b/internal/console/apimail_test.go
@@ -998,6 +998,47 @@ func TestAPIEmailPauseCapRaiseDoesNotResume(t *testing.T) {
}
}
+// TestAPIEmailPauseRefusesAResumeInPerInstanceMode: the operator door
+// answers a permanent conflict, never the generic "try again" 502, when
+// asked to resume an app sender that has moved to per-instance senders.
+// Nothing in this release sets PerInstance itself, so this hand-sets it —
+// the point is that a retry can never succeed here, and the operator needs
+// to be told that plainly rather than sent to retry forever.
+func TestAPIEmailPauseRefusesAResumeInPerInstanceMode(t *testing.T) {
+ fx, fake, _, token := emailArmed(t, "code-email-pause-perinstance")
+ app, domain := "titogo", "titogo.bab.oncarlos.com"
+ provisionEmail(t, fx, fake, token, app, domain, "hello@"+domain)
+ ops := emailOperatorToken(t, fx, "code-email-pause-perinstance-ops")
+
+ if status, out := postJSON(t, emailURL(fx, app)+"/pause", map[string]any{"paused": true}, ops); status != http.StatusOK {
+ t.Fatalf("pause = %d %v", status, out)
+ }
+
+ sn, etag, err := mail.GetSender(context.Background(), fx.rel, release.OpsAccount, app, mail.SenderAppID)
+ if err != nil {
+ t.Fatal(err)
+ }
+ sn.PerInstance = true
+ if _, err := mail.PutSender(context.Background(), fx.rel, release.OpsAccount, app, sn, etag); err != nil {
+ t.Fatal(err)
+ }
+
+ status, out := postJSON(t, emailURL(fx, app)+"/pause", map[string]any{"paused": false}, ops)
+ if status != http.StatusConflict {
+ t.Fatalf("resume in per-instance mode = %d %v, want 409", status, out)
+ }
+ msg, _ := out["error"].(string)
+ if !strings.Contains(msg, "per-instance") {
+ t.Fatalf("error = %q, want it to name per-instance mode", msg)
+ }
+ if strings.Contains(strings.ToLower(msg), "try again") {
+ t.Fatalf("refusal promised a retry would help, but this refusal is permanent: %q", msg)
+ }
+ if emailRecord(t, fx, app).Provisioned.Active {
+ t.Fatal("the refused resume left the record active")
+ }
+}
+
// TestAPIEmailPauseRefusesAnEmptyBody: neither paused nor a positive
// daily_cap is a request that would change nothing — refused rather than
// silently answering 200 having done nothing.
diff --git a/internal/console/mailcounts_test.go b/internal/console/mailcounts_test.go
index 1c0ea4663..61a2a0ab0 100644
--- a/internal/console/mailcounts_test.go
+++ b/internal/console/mailcounts_test.go
@@ -573,22 +573,50 @@ func TestMailCountsAreKeptAndReadPerSender(t *testing.T) {
}
// The cap applies per sender, so one sender's breach pauses that sender and
-// leaves its siblings sending.
+// leaves its siblings sending. The counts are chosen to discriminate a
+// summed read from a per-sender one: 25 (app) + 40 (sibling) = 65, over the
+// cap of 50, so a summed MailCounts read would trip here even though
+// neither sender has yet reached 50 on its own.
func TestTheCapIsEnforcedPerSender(t *testing.T) {
fx, now := mailFixture(t)
ctx := context.Background()
day := now.UTC().Format("2006-01-02")
- for i := 0; i < 50; i++ {
+
+ // A provisioned, active sibling sender — checked below so a pause is
+ // proven to leave it alone, not just left unread.
+ sibling := &mail.Sender{
+ ID: "book.example.com", Scope: mail.ScopeInstance, Source: mail.SourceDomain,
+ Provisioned: &mail.Provisioned{IAMUser: "book-u", KeyID: "AKIA2", Active: true, At: now},
+ }
+ if _, err := mail.PutSender(ctx, fx.rel, release.OpsAccount, "harbourmail", sibling, ""); err != nil {
+ t.Fatal(err)
+ }
+
+ for i := 0; i < 25; i++ {
if err := fx.st.RecordMailEvent(ctx, release.OpsAccount, "harbourmail", mail.SenderAppID, day, mail.EventSend); err != nil {
t.Fatal(err)
}
}
- // A second sender's traffic does not count towards the first one's cap.
- for i := 0; i < 50; i++ {
+ for i := 0; i < 40; i++ {
if err := fx.st.RecordMailEvent(ctx, release.OpsAccount, "harbourmail", "book.example.com", day, mail.EventSend); err != nil {
t.Fatal(err)
}
}
+ paused, _, err := fx.srv.enforceMailLimits(ctx, release.OpsAccount, "harbourmail", mail.SenderAppID, now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if paused {
+ t.Fatal("the app sender paused on 25 of its own sends plus a sibling's 40 — the cap is being read summed across senders")
+ }
+
+ // Push the app sender's OWN count past its own cap of 50. The sibling's
+ // count is untouched at 40, still under its own cap.
+ for i := 0; i < 26; i++ {
+ if err := fx.st.RecordMailEvent(ctx, release.OpsAccount, "harbourmail", mail.SenderAppID, day, mail.EventSend); err != nil {
+ t.Fatal(err)
+ }
+ }
paused, reason, err := fx.srv.enforceMailLimits(ctx, release.OpsAccount, "harbourmail", mail.SenderAppID, now)
if err != nil {
t.Fatal(err)
@@ -600,9 +628,94 @@ func TestTheCapIsEnforcedPerSender(t *testing.T) {
if err != nil || sn.Provisioned.Active || sn.Provisioned.PauseKind != mail.PauseKindCap {
t.Fatalf("the app sender's record = %+v %v", sn.Provisioned, err)
}
+ // The pause must land on the sender that breached, never a sibling that
+ // did not: the sibling is still under its own cap and must still be
+ // Active.
+ sib, _, err := mail.GetSender(ctx, fx.rel, release.OpsAccount, "harbourmail", "book.example.com")
+ if err != nil || sib == nil || !sib.Provisioned.Active {
+ t.Fatalf("the sibling sender's record = %+v %v, want it left Active", sib, err)
+ }
+
// The day roll clears a cap pause, still, and only a cap pause.
fresh, resumed, err := fx.srv.resumeStaleCapPause(ctx, release.OpsAccount, "harbourmail", mail.SenderAppID, sn, now.AddDate(0, 0, 1))
if err != nil || !resumed || fresh == nil || !fresh.Provisioned.Active {
t.Fatalf("the day roll did not clear the cap pause: %+v %v %v", fresh, resumed, err)
}
+ // The sibling is still untouched by the day roll on the app sender's
+ // pause — resumeStaleCapPause was given the app sender, not the
+ // sibling, so the sibling's own state must not have moved.
+ sib, _, err = mail.GetSender(ctx, fx.rel, release.OpsAccount, "harbourmail", "book.example.com")
+ if err != nil || sib == nil || !sib.Provisioned.Active {
+ t.Fatalf("the sibling sender's record = %+v %v after the day roll, want it left Active", sib, err)
+ }
+}
+
+// The pause targets the sender that actually breached its cap, never a
+// hard-coded stand-in for it: if enforceMailLimits paused the app sender
+// regardless of which sender was asked for, a sibling's own breach would
+// leave the sibling sending and silently pause the app sender instead.
+func TestTheCapPauseTargetsTheBreachingSenderNotTheAppSender(t *testing.T) {
+ fx, now := mailFixture(t)
+ ctx := context.Background()
+ day := now.UTC().Format("2006-01-02")
+
+ sibling := &mail.Sender{
+ ID: "book.example.com", Scope: mail.ScopeInstance, Source: mail.SourceDomain,
+ Provisioned: &mail.Provisioned{IAMUser: "book-u", KeyID: "AKIA2", Active: true, At: now},
+ }
+ if _, err := mail.PutSender(ctx, fx.rel, release.OpsAccount, "harbourmail", sibling, ""); err != nil {
+ t.Fatal(err)
+ }
+
+ // Only the SIBLING breaches its cap; the app sender sends nothing.
+ for i := 0; i < 50; i++ {
+ if err := fx.st.RecordMailEvent(ctx, release.OpsAccount, "harbourmail", "book.example.com", day, mail.EventSend); err != nil {
+ t.Fatal(err)
+ }
+ }
+ paused, reason, err := fx.srv.enforceMailLimits(ctx, release.OpsAccount, "harbourmail", "book.example.com", now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if !paused || !strings.Contains(reason, "daily cap 50") {
+ t.Fatalf("sibling sender = %v %q, want a cap pause", paused, reason)
+ }
+ sib, _, err := mail.GetSender(ctx, fx.rel, release.OpsAccount, "harbourmail", "book.example.com")
+ if err != nil || sib.Provisioned.Active || sib.Provisioned.PauseKind != mail.PauseKindCap {
+ t.Fatalf("the sibling sender's record = %+v %v, want it paused", sib.Provisioned, err)
+ }
+ // The app sender never breached anything and must still be Active — a
+ // pause hard-coded to the app sender would show up here instead.
+ app, _, err := mail.GetSender(ctx, fx.rel, release.OpsAccount, "harbourmail", mail.SenderAppID)
+ if err != nil || app == nil || !app.Provisioned.Active {
+ t.Fatalf("the app sender's record = %+v %v, want it left Active", app, err)
+ }
+}
+
+// resumeStaleCapPause must never resume a PerInstance sender's stale cap
+// pause: resuming reactivates every key it touches, including the app
+// credential every instance still holds, which is exactly what per-instance
+// mode exists to prevent (design section 6.3). Nothing sets PerInstance in
+// this release; this pins the guard directly so it cannot regress silently.
+func TestResumeStaleCapPauseNeverTouchesAPerInstanceSender(t *testing.T) {
+ fx, now := mailFixture(t)
+ pauseDirectly(t, fx, "harbourmail", mail.PauseKindCap, now.AddDate(0, 0, -1))
+ sn, etag, err := mail.GetSender(context.Background(), fx.rel, release.OpsAccount, "harbourmail", mail.SenderAppID)
+ if err != nil {
+ t.Fatal(err)
+ }
+ sn.PerInstance = true
+ if _, err := mail.PutSender(context.Background(), fx.rel, release.OpsAccount, "harbourmail", sn, etag); err != nil {
+ t.Fatal(err)
+ }
+ got, resumed, err := fx.srv.resumeStaleCapPause(context.Background(), release.OpsAccount, "harbourmail", mail.SenderAppID, sn, now)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if resumed {
+ t.Fatal("resumed a PerInstance sender's stale cap pause — re-arms a credential every instance still holds")
+ }
+ if got.Provisioned.Active {
+ t.Fatal("a PerInstance sender's cap pause from yesterday was lifted")
+ }
}