amadan

carlos / platform Public

mail: pin Teardown's legacy delete and the Record round trip, fix em dashes

Review of the senders/app.json migration (task 3) found the LegacyKey
delete in Teardown had no test that could fail without it: every existing
fixture writes only the new key. Added a test that seeds email.json
directly and checks the store keys, not Get, since Get's own fallback
would mask a left-behind legacy object.

Also added a test for the Record<->Sender round trip's claim, made in its
own doc comments, that a Get-mutate-Put cycle preserves PerInstance,
Identity, Address, TenantARN and Salt: fields Record has no name for.
Confirmed both new tests fail without the code they pin.

Reworded three added comments that used an em dash, against this repo's
copy rule (full stop, new sentence, lower case).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Paul Campbell pushed by claude cea8229503821f32fe7e2ed09d9d682b2dcad130 parent 9a950b3
3 files changed, +93 −8
  • internal/mail/mail.go +5 −4
  • internal/mail/mail_test.go +46 −4
  • internal/mail/walk_lifecycle_test.go +42 −0
diff --git a/internal/mail/mail.go b/internal/mail/mail.go
index 7a8ba18f1..52c5453b5 100644
--- a/internal/mail/mail.go
+++ b/internal/mail/mail.go
@@ -102,9 +102,9 @@ type Standalone struct {
By string `json:"by"`
}
-// Record is the view every walk in walk.go works with — three blocks for
+// Record is the view every walk in walk.go works with. Three blocks for
// three actors, plus the standalone credentials, which have their own
-// lifetime. It used to be the object at Key; now it is the four blocks of
+// lifetime. It used to be the object at Key. Now it is the four blocks of
// the app sender, read and written through GetSender/PutSender.
type Record struct {
Schema int `json:"schema"`
@@ -175,8 +175,9 @@ func EnvNames(prefix string) (host, port, user, pass, from string) {
prefix + "_PASS", prefix + "_FROM"
}
-// Get reads the app sender as a Record — the view every walk in walk.go
-// works with. Absent is (nil, "", nil): "no email" is a state, not an error.
+// Get reads the app sender as a Record. It is the view every walk in
+// walk.go works with. Absent is (nil, "", nil): "no email" is a state, not
+// an error.
//
// The returned Record keeps a private handle on the Sender it was decoded
// from, so a read-modify-write through Put preserves the fields Record has
diff --git a/internal/mail/mail_test.go b/internal/mail/mail_test.go
index 80acdcdec..e80ab134b 100644
--- a/internal/mail/mail_test.go
+++ b/internal/mail/mail_test.go
@@ -112,10 +112,9 @@ func containsFold(haystack, needle string) bool {
}
// The migration (design §5). Get prefers senders/app.json and falls back to
-// email.json; when it served the fallback it returns an EMPTY etag, so every
-// writer's Put — and they all pass back the etag they were given — is a
-// create by construction, and a lost race is ErrCASConflict exactly as it is
-// today.
+// email.json; when it served the fallback it returns an EMPTY etag. Every
+// writer passes back the etag it was given, so the next Put is a create by
+// construction, and a lost race is ErrCASConflict exactly as it is today.
func TestGetFallsBackToEmailJSONWithAnEmptyETag(t *testing.T) {
ctx := context.Background()
s := release.NewDirStore(t.TempDir())
@@ -200,3 +199,46 @@ func TestListSendersAnswersFromTheLegacyRecord(t *testing.T) {
t.Fatalf("the legacy record did not read back as the app sender: %+v", got[0])
}
}
+
+// 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
+// rec.sender() forgot one, an ordinary pause would silently clear it.
+// SetPaused is the realistic shape: it reads through Record, changes only
+// the Provisioned block, and writes back through Put.
+func TestGetPutPreservesFieldsRecordHasNoNameFor(t *testing.T) {
+ ctx := context.Background()
+ s, p := release.NewDirStore(t.TempDir()), NewMemProvisioner("1")
+ now := time.Date(2026, 9, 15, 12, 0, 0, 0, time.UTC)
+
+ sn := &Sender{
+ ID: SenderAppID, Scope: ScopeApp, Source: SourceDomain,
+ Provisioned: &Provisioned{IAMUser: "carlos-mail-bag-harbourmail", KeyID: "AKIA1", Active: true, At: now},
+ PerInstance: true,
+ Identity: "arn:aws:ses:eu-west-1:1:identity/harbourmail.bag.oncarlos.com",
+ Address: "noreply@harbourmail.bag.oncarlos.com",
+ TenantARN: "arn:aws:ses:eu-west-1:1:tenant/carlos-bag",
+ Salt: "s3cr3tsalt",
+ }
+ if _, err := PutSender(ctx, s, "bag", "harbourmail", sn, ""); err != nil {
+ t.Fatalf("PutSender: %v", err)
+ }
+
+ if _, err := SetPaused(ctx, s, p, "bag", "harbourmail", true, PauseKindCap, now); err != nil {
+ t.Fatalf("SetPaused: %v", err)
+ }
+
+ got, _, err := GetSender(ctx, s, "bag", "harbourmail", SenderAppID)
+ if err != nil || got == nil {
+ t.Fatalf("GetSender = %+v %v", got, err)
+ }
+ if got.Provisioned == nil || got.Provisioned.Active || got.Provisioned.PauseKind != PauseKindCap {
+ t.Fatalf("SetPaused's own change did not take: %+v", got.Provisioned)
+ }
+ if !got.PerInstance {
+ t.Fatalf("the pause's read-modify-write cleared PerInstance: %+v", got)
+ }
+ if got.Identity != sn.Identity || got.Address != sn.Address || got.TenantARN != sn.TenantARN || got.Salt != sn.Salt {
+ t.Fatalf("the pause's read-modify-write lost a field: %+v", got)
+ }
+}
diff --git a/internal/mail/walk_lifecycle_test.go b/internal/mail/walk_lifecycle_test.go
index 574d76c12..2b48de825 100644
--- a/internal/mail/walk_lifecycle_test.go
+++ b/internal/mail/walk_lifecycle_test.go
@@ -2,7 +2,11 @@ package mail
import (
"context"
+ "encoding/json"
+ "errors"
+ "os"
"testing"
+ "time"
"github.com/carlosframework/platform/internal/appconfig"
"github.com/carlosframework/platform/internal/dnsrec"
@@ -536,4 +540,42 @@ func TestTeardownRemovesTheIdentityWhenTheLastAppGoes(t *testing.T) {
}
}
+// Teardown must delete BOTH objects, not just the current one. Every other
+// Teardown fixture builds its record through Enable/Declare, which write
+// only senders/app.json, so a fixture like those would pass whether or not
+// the legacy delete at walk.go's end is even there. This one seeds
+// email.json directly, the way an app that has never been migrated actually
+// has it, and checks the legacy key by reading the store directly rather
+// than through Get: Get falls back to email.json, so it would read back
+// nil-ish success even if the legacy object were left behind.
+func TestTeardownDeletesTheLegacyRecordToo(t *testing.T) {
+ ctx := context.Background()
+ s, p := release.NewDirStore(t.TempDir()), NewMemProvisioner("1")
+ now := time.Date(2026, 9, 15, 12, 0, 0, 0, time.UTC)
+
+ legacy := &Record{Schema: Schema, App: "harbourmail",
+ Declared: &Declared{EnvPrefix: "HARBOURMAIL_SMTP", From: "noreply@x.com", At: now, By: "owner@example.test"},
+ Provisioned: &Provisioned{IAMUser: "carlos-mail-bag-harbourmail", KeyID: "AKIA1", Active: true, At: now},
+ }
+ body, err := json.Marshal(legacy)
+ if err != nil {
+ t.Fatal(err)
+ }
+ if _, err := s.Put(ctx, LegacyKey("bag", "harbourmail"), body, ""); err != nil {
+ t.Fatal(err)
+ }
+
+ w := dnsrec.NewMemWriter()
+ if err := Teardown(ctx, s, p, w, testDeployment(), "bag", "harbourmail"); err != nil {
+ t.Fatalf("Teardown: %v", err)
+ }
+
+ if _, _, err := s.Get(ctx, Key("bag", "harbourmail")); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("senders/app.json survived teardown: %v", err)
+ }
+ if _, _, err := s.Get(ctx, LegacyKey("bag", "harbourmail")); !errors.Is(err, os.ErrNotExist) {
+ t.Fatalf("email.json survived teardown — a ghost record that would resurrect this app's email on the next Get: %v", err)
+ }
+}
+
func memWriterForTest() dnsrec.Writer { return dnsrec.NewMemWriter() }