amadan
mail: ListSenders must not fall back to the legacy record on an errored read (final review, item 5)
The per-key loop swallowed any Get failure with `continue`, including the app sender's own key. When that left appSender nil, the code fell to GetSender's legacy-record fallback, which after migration serves the STALE pre-migration email.json state — the status reply and the resume loop would read a stale Active and PauseKind. Track whether the error came from the app sender's own key specifically (known from the key name alone, no unmarshal needed): if so, return that error rather than falling back. A sibling sender's Get error is untouched, since only the app sender feeds the legacy fallback. Pinned with a test using a small store wrapper that forces one key's Get to fail while the rest of the store still works.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 files changed,
+71
−0
internal/mail/mail.go+16 −0internal/mail/mail_test.go+55 −0
diff --git a/internal/mail/mail.go b/internal/mail/mail.go| index 52c5453b5..ef31cdf46 100644 |
| --- a/internal/mail/mail.go |
| +++ b/internal/mail/mail.go |
| @@ -269,14 +269,27 @@ func ListSenders(ctx context.Context, s release.Store, acct, app string) ([]*Sen |
| if err != nil { |
| return nil, err |
| } |
| + appKey := SenderKey(acct, app, SenderAppID) |
| var rest []*Sender |
| var appSender *Sender |
| + // appSenderErr is set only when the app sender's OWN key came back from |
| + // List (so the object exists) but its Get errored. That is not the same |
| + // as the key being absent, and must not be treated as one: falling |
| + // through to GetSender's legacy-record fallback below would read |
| + // email.json, which after migration is the STALE pre-migration state — |
| + // exactly what the status reply and the resume loop must not mistake |
| + // for the current Active/PauseKind. A sibling sender's Get error is not |
| + // tracked here; only the app sender feeds the legacy fallback. |
| + var appSenderErr error |
| for _, k := range keys { |
| if !strings.HasSuffix(k, ".json") { |
| continue |
| } |
| raw, _, err := s.Get(ctx, k) |
| if err != nil { |
| + if k == appKey { |
| + appSenderErr = err |
| + } |
| continue |
| } |
| var sn Sender |
| @@ -290,6 +303,9 @@ func ListSenders(ctx context.Context, s release.Store, acct, app string) ([]*Sen |
| rest = append(rest, &sn) |
| } |
| if appSender == nil { |
| + if appSenderErr != nil { |
| + return nil, fmt.Errorf("mail: %s: %w", appKey, appSenderErr) |
| + } |
| appSender, _, err = GetSender(ctx, s, acct, app, SenderAppID) |
| if err != nil { |
| return nil, err |
diff --git a/internal/mail/mail_test.go b/internal/mail/mail_test.go| index e80ab134b..805bd4f4e 100644 |
| --- a/internal/mail/mail_test.go |
| +++ b/internal/mail/mail_test.go |
| @@ -200,6 +200,61 @@ func TestListSendersAnswersFromTheLegacyRecord(t *testing.T) { |
| } |
| } |
| +// errOnGetStore wraps a release.Store and forces Get on one specific key to |
| +// fail with a non-NotExist error, standing in for a transient store error |
| +// as distinct from that key being genuinely absent. |
| +type errOnGetStore struct { |
| + release.Store |
| + failKey string |
| + err error |
| +} |
| + |
| +func (s *errOnGetStore) Get(ctx context.Context, key string) ([]byte, string, error) { |
| + if key == s.failKey { |
| + return nil, "", s.err |
| + } |
| + return s.Store.Get(ctx, key) |
| +} |
| + |
| +// ListSenders must not read the legacy record when the app sender's OWN key |
| +// errored rather than being absent: a store hiccup reading senders/app.json |
| +// is not proof the app was never migrated, and falling back to email.json |
| +// would hand the status reply and the resume loop the STALE pre-migration |
| +// Active/PauseKind instead of an error they can retry against. |
| +func TestListSendersDoesNotFallBackToLegacyOnAnErroredRead(t *testing.T) { |
| + ctx := context.Background() |
| + dir := release.NewDirStore(t.TempDir()) |
| + now := time.Date(2026, 9, 15, 12, 0, 0, 0, time.UTC) |
| + |
| + // The stale, pre-migration record: paused. |
| + legacy, err := json.Marshal(&Record{Schema: Schema, App: "harbourmail", |
| + Declared: &Declared{EnvPrefix: "HARBOURMAIL_SMTP", From: "noreply@x.com", At: now, By: "o@example.test"}, |
| + Provisioned: &Provisioned{Active: false, PauseKind: PauseKindCap, At: now}}) |
| + if err != nil { |
| + t.Fatal(err) |
| + } |
| + if _, err := dir.Put(ctx, LegacyKey("bag", "harbourmail"), legacy, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + // The migrated, current record: active. |
| + sn := &Sender{ID: SenderAppID, Scope: ScopeApp, Source: SourceDomain, |
| + Declared: &Declared{EnvPrefix: "HARBOURMAIL_SMTP", From: "noreply@x.com", At: now, By: "o@example.test"}, |
| + Provisioned: &Provisioned{Active: true, At: now}} |
| + if _, err := PutSender(ctx, dir, "bag", "harbourmail", sn, ""); err != nil { |
| + t.Fatal(err) |
| + } |
| + |
| + failing := &errOnGetStore{ |
| + Store: dir, |
| + failKey: SenderKey("bag", "harbourmail", SenderAppID), |
| + err: errors.New("mail_test: injected transient store error"), |
| + } |
| + got, err := ListSenders(ctx, failing, "bag", "harbourmail") |
| + if err == nil { |
| + t.Fatalf("ListSenders = %+v, want an error rather than the stale legacy record", got) |
| + } |
| +} |
| + |
| // The Record view's read-modify-write must not lose the fields Record has |
| // no name for. Tasks 5 and 7 start populating PerInstance, Identity, |
| // Address, TenantARN and Salt directly on the Sender; if sn.record() or |