amadan
console: fall through to the domain loop on a store error, not just a live-check (final review, item 2)
mailAppLive collapsed a store ERROR into the same false as a confirmed
absent/trashed app. recordAndEnforceMail's caller-named branch treated
that false as "drop the event": no count, no cap check, no domain
fallback, and SNS still saw a 200 so nothing ever retried it. The
domain-wide fallback loop forty-odd lines below states the opposite
policy for its own read errors ("fall through and charge").
Split mailAppLive into mailAppLiveErr (bool, error) for callers that
must tell a read error apart from a confirmed absence, and mailAppLive
(unchanged signature) for its one remaining caller where collapsing is
fine and now documented as such. The caller-named branch now falls
through to the domain loop on a store error instead of returning.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 file changed,
+49
−21
internal/console/sesevents.go+49 −21
diff --git a/internal/console/sesevents.go b/internal/console/sesevents.go| index a4c3f5c4a..25d197b67 100644 |
| --- a/internal/console/sesevents.go |
| +++ b/internal/console/sesevents.go |
| @@ -508,25 +508,39 @@ func (s *Server) recordAndEnforceMail(ctx context.Context, ev mail.Event, notice |
| // to, which nothing would ever read or reap. Returning rather than |
| // falling through is deliberate: SES named a sender, so the |
| // domain-wide loop is not the right answer for it either. |
| - if !s.mailAppLive(callerAcct, callerApp) { |
| + // |
| + // But that only holds for a CONFIRMED absence. A store ERROR here |
| + // is not proof the app is gone, and treating it as one would drop |
| + // the event outright: no count, no cap check, and no domain |
| + // fallback either, with SNS still seeing a 200 so nothing ever |
| + // retries it. The fallback loop below applies exactly the opposite |
| + // rule to its own read errors ("A read failure is not a reason to |
| + // drop the event: fall through and charge") for the same reason, so |
| + // a transient error here falls through to it too rather than being |
| + // swallowed. |
| + live, err := s.mailAppLiveErr(callerAcct, callerApp) |
| + if err != nil { |
| + log.Printf("console: ses events: checking %s/%s live: %v; falling back to domain attribution", callerAcct, callerApp, err) |
| + } else if !live { |
| return |
| - } |
| - s.recordMailMessage(ctx, callerAcct, callerApp, key, ev, true, now) |
| - // Counted ONCE, against the sender SES named, and the domain loop |
| - // below is skipped entirely. This is the visible change in the roll |
| - // note: a sibling app on the same domain is no longer charged for |
| - // this send, which is the intent, but a tenant watching its own |
| - // numbers will see them move. |
| - if err := s.store.RecordMailEvent(ctx, callerAcct, callerApp, callerSender, day, ev.Type); err != nil { |
| - log.Printf("console: ses events: recording %s for %s/%s/%s -> %v", ev.Type, callerAcct, callerApp, callerSender, err) |
| + } else { |
| + s.recordMailMessage(ctx, callerAcct, callerApp, key, ev, true, now) |
| + // Counted ONCE, against the sender SES named, and the domain loop |
| + // below is skipped entirely. This is the visible change in the roll |
| + // note: a sibling app on the same domain is no longer charged for |
| + // this send, which is the intent, but a tenant watching its own |
| + // numbers will see them move. |
| + if err := s.store.RecordMailEvent(ctx, callerAcct, callerApp, callerSender, day, ev.Type); err != nil { |
| + log.Printf("console: ses events: recording %s for %s/%s/%s -> %v", ev.Type, callerAcct, callerApp, callerSender, err) |
| + return |
| + } |
| + if _, reason, err := s.enforceMailLimits(ctx, callerAcct, callerApp, callerSender, at); err != nil { |
| + log.Printf("console: ses events: enforcing limits for %s/%s/%s -> %v", callerAcct, callerApp, callerSender, err) |
| + } else if reason != "" { |
| + log.Printf("console: ses events: paused %s/%s/%s -> %s", callerAcct, callerApp, callerSender, reason) |
| + } |
| return |
| } |
| - if _, reason, err := s.enforceMailLimits(ctx, callerAcct, callerApp, callerSender, at); err != nil { |
| - log.Printf("console: ses events: enforcing limits for %s/%s/%s -> %v", callerAcct, callerApp, callerSender, err) |
| - } else if reason != "" { |
| - log.Printf("console: ses events: paused %s/%s/%s -> %s", callerAcct, callerApp, callerSender, reason) |
| - } |
| - return |
| } |
| // The fallback, unchanged: SES named nobody CARLOS minted, so the |
| @@ -614,14 +628,28 @@ func (s *Server) recordMailMessage(ctx context.Context, acct, app, key string, e |
| } |
| // mailAppLive reports whether acct (a sqid) holds app as a live, untrashed |
| -// app right now. |
| +// app right now. It collapses a store error into false, like an absent app, |
| +// which is fine for its one caller (recordMailMessage's best-effort log |
| +// guard) but wrong for anything that treats "not live" as a reason to stop |
| +// processing an event entirely — use mailAppLiveErr there instead. |
| func (s *Server) mailAppLive(acct, app string) bool { |
| + live, err := s.mailAppLiveErr(acct, app) |
| + return err == nil && live |
| +} |
| + |
| +// mailAppLiveErr is mailAppLive with the error kept apart from a confirmed |
| +// absence: err != nil means the store read itself failed, not that the app |
| +// is gone. A caller deciding whether to drop an event needs that |
| +// distinction — an errored read is not evidence of anything. |
| +func (s *Server) mailAppLiveErr(acct, app string) (bool, error) { |
| a, err := s.store.AccountBySqid(acct) |
| - if err != nil || a == nil { |
| - return false |
| + if err != nil { |
| + return false, err |
| + } |
| + if a == nil { |
| + return false, nil |
| } |
| - ok, err := s.store.HasAppIn(a.ID, app) |
| - return err == nil && ok |
| + return s.store.HasAppIn(a.ID, app) |
| } |
| // confirmSNSSubscription GETs the verified, host-checked SubscribeURL. It is |