amadan
Plan 2: fixes from the executability review
The tenant association moves after PutDomain. An association is the one thing that stops an identity being deleted, and Teardown's orphan-claim sweep deletes the identity behind a claim that has no record, so associating any earlier makes that sweep unreachable. Also: the fifteen test call sites two signature changes touch, the fourth MailCounts caller, a real store hoist in place of a variable that does not exist, and ListDirs instead of listing every release artifact in the bucket at console start.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 file changed,
+190
−42
docs/superpowers/plans/2026-09-15-tenant-email-2-senders.md+190 −42
diff --git a/docs/superpowers/plans/2026-09-15-tenant-email-2-senders.md b/docs/superpowers/plans/2026-09-15-tenant-email-2-senders.md| index 7b835339d..2144fd6be 100644 |
| --- a/docs/superpowers/plans/2026-09-15-tenant-email-2-senders.md |
| +++ b/docs/superpowers/plans/2026-09-15-tenant-email-2-senders.md |
| @@ -10,8 +10,11 @@ |
| **Spec:** `docs/superpowers/specs/2026-09-15-tenant-email-senders-design.md` — §11 step 2 is the whole of this plan's mandate. §5 (the record, the names, the caller index, the migration), §8 (attribution by caller, `mail_sender_counts`, caps, pauses, status) and §7's one sentence "Associations are wired in step 2, not here" are in scope. §6 and the rest of §7 are **not**: no instance senders, no external identities, no address claims, no per-instance config layer, no `svcrole.Mail`, no new doors. |
| +**Two parts of §8 are deferred to step 3, and that is a ruling rather than an oversight.** §8's "SES's own pauses" paragraph asks for three things this plan does not build: `EnsureTenant` storing `TenantArn` on the record, `SetTenantSending`/`TenantStatus` on the provisioner with `UpdateReputationEntityCustomerManagedStatus` becoming the pause verb for a sender that has a tenant, and status reading `GetReputationEntity` so that an SES-imposed pause is reported apart from a CARLOS one (`SendingStatusAggregate`, `CustomerManagedStatus` and `AwsSesManagedStatus` kept distinct, per the spike at §4.1 item 4). They are deferred because all three are only reachable once a send is attributed to a tenant, which no send is until step 3 emits `X-SES-TENANT`: adopting the tenant pause verb now would move every existing app's pause onto a control SES does not yet apply to them, and reporting an SES pause distinctly would report a field that is `null` for every sender in the fleet. `SetKeyActive` stays the pause verb for this release, which is what it already is. The `TenantARN` field lands on the record in task 1 with nothing setting it, so step 3 has somewhere to write. |
| + |
| ## Global Constraints |
| +- **Run `gofmt -w` on every file you touched before running the gate.** Several code blocks in this plan are pasted with trailing comments that gofmt will realign (task 1's `Sender` struct, task 2's `longAcct`/`longApp` pair, task 5's `CallerIndex`). `gofmt -l .` only NAMES an unformatted file, so a plan followed literally goes red on the gate's last step rather than being fixed by it. |
| - **The gate before every commit:** `go build ./... && go vet ./... && go test ./... -count=1` all clean, and `gofmt -l .` empty over the whole tree. Env: `GOCACHE=/tmp/claude-1001/gocache GOFLAGS=-mod=mod GOPROXY=off`. Socket-binding tests need `TMPDIR=/tmp/g`. Go commands may need the sandbox disabled; "read-only file system" from the toolchain is the tell. |
| - **This repo lands through `amadan branch merge`, never a GitHub PR.** Branch `worktree-tenant-email-senders`. CI is `.amadan/ci.d/`, triggered by a push to the `amadan` remote; a branch pushed only to `origin` has been gated by nothing. |
| - **No capability change.** Every task must leave the tree shippable on its own: an app that can send today sends after each commit, with the same credential, the same cap and the same delivered variables. The one deliberate behaviour change is task 11's attribution narrowing, which is a roll note, not a capability. |
| @@ -551,7 +554,7 @@ git commit -m "mail: SenderNames owns every AWS name and its bound (plan 2, task |
| ### Task 3: The migration to `senders/app.json` |
| **Files:** |
| -- Modify: `internal/mail/mail.go` (`Key` at `:114`, `Get` at `:130`, `Put` at `:154`), `internal/mail/mail_test.go`, `internal/mail/walk.go` (`Teardown`'s delete at `:1297`) |
| +- Modify: `internal/mail/mail.go` (`Key` at `:114`, `Get` at `:130`, `Put` at `:154`), `internal/mail/mail_test.go` (`TestKeyIsUnderTheAppPrefix` at `:13-17`, rewritten), `internal/mail/walk.go` (`Teardown`'s delete at `:1297`) |
| **Interfaces:** |
| - Produces: |
| @@ -567,6 +570,21 @@ git commit -m "mail: SenderNames owns every AWS name and its bound (plan 2, task |
| - [ ] **Step 1: Write the failing test** |
| +`internal/mail/mail_test.go`, REPLACING `TestKeyIsUnderTheAppPrefix` (`:13-17`), which pins `Key` at the old object and goes red the moment `Key` moves. It is the reason this task is not additive: |
| + |
| +```go |
| +func TestKeyIsUnderTheAppPrefix(t *testing.T) { |
| + if got, want := Key("bag", "harbourmail"), "apps/bag/harbourmail/email/senders/app.json"; got != want { |
| + t.Fatalf("Key = %q, want %q", got, want) |
| + } |
| + // The pre-senders object keeps its name, because Get still reads it and |
| + // the sweep still has to find it. |
| + if got, want := LegacyKey("bag", "harbourmail"), "apps/bag/harbourmail/email.json"; got != want { |
| + t.Fatalf("LegacyKey = %q, want %q", got, want) |
| + } |
| +} |
| +``` |
| + |
| `internal/mail/mail_test.go` (append): |
| ```go |
| @@ -1008,6 +1026,7 @@ import ( |
| "fmt" |
| "log" |
| "os" |
| + "path" |
| "strings" |
| "github.com/carlosframework/platform/internal/release" |
| @@ -1027,41 +1046,81 @@ import ( |
| // A failure on one app is logged and the sweep continues: one unreadable |
| // object must not stop the other five hundred, and every deletion is |
| // independent. |
| +// |
| +// It walks with ListDirs, twice, and never with List. List(ctx, "apps/") on |
| +// a production bucket returns EVERY object under every app — every release |
| +// artifact of every version of every app — to find the handful of keys |
| +// ending in /email.json, and it would do it at console start. ListDirs is |
| +// the delimiter-aware listing (release/store.go:50-58): one page of |
| +// accounts, one page of apps per account, then two Gets per app. |
| func SweepLegacyRecords(ctx context.Context, s release.Store, enabled bool) (int, error) { |
| if !enabled { |
| return 0, nil |
| } |
| - keys, err := s.List(ctx, "apps/") |
| + accts, err := s.ListDirs(ctx, "apps/") |
| if err != nil { |
| return 0, fmt.Errorf("mail: sweep legacy records: %w", err) |
| } |
| deleted := 0 |
| - for _, key := range keys { |
| - if !strings.HasSuffix(key, "/email.json") { |
| + for _, acctPrefix := range accts { |
| + // ListDirs hands back a FULL prefix ending in "/", never a bare |
| + // name, so the account is the last segment of it. |
| + acct := path.Base(strings.TrimSuffix(acctPrefix, "/")) |
| + if !release.ValidAccount(acct) { |
| continue |
| } |
| - rest := strings.TrimSuffix(strings.TrimPrefix(key, "apps/"), "/email.json") |
| - acct, app, found := strings.Cut(rest, "/") |
| - if !found || !release.ValidAccount(acct) || !release.ValidApp(app) { |
| + apps, err := s.ListDirs(ctx, acctPrefix) |
| + if err != nil { |
| + log.Printf("mail: sweep legacy records: listing %s -> %v", acctPrefix, err) |
| continue |
| } |
| - if _, _, err := s.Get(ctx, SenderKey(acct, app, SenderAppID)); err != nil { |
| - if !errors.Is(err, os.ErrNotExist) { |
| - log.Printf("mail: sweep legacy records: reading %s -> %v", SenderKey(acct, app, SenderAppID), err) |
| + for _, appPrefix := range apps { |
| + app := path.Base(strings.TrimSuffix(appPrefix, "/")) |
| + if !release.ValidApp(app) { |
| + continue |
| } |
| - continue // not migrated: this object is still the only copy |
| - } |
| - if err := s.Delete(ctx, key); err != nil { |
| - log.Printf("mail: sweep legacy records: deleting %s -> %v", key, err) |
| - continue |
| + if _, _, err := s.Get(ctx, SenderKey(acct, app, SenderAppID)); err != nil { |
| + if !errors.Is(err, os.ErrNotExist) { |
| + log.Printf("mail: sweep legacy records: reading %s -> %v", SenderKey(acct, app, SenderAppID), err) |
| + } |
| + continue // not migrated: this object is still the only copy |
| + } |
| + legacy := LegacyKey(acct, app) |
| + if _, _, err := s.Get(ctx, legacy); err != nil { |
| + if !errors.Is(err, os.ErrNotExist) { |
| + log.Printf("mail: sweep legacy records: reading %s -> %v", legacy, err) |
| + } |
| + continue // nothing to remove, which is the steady state |
| + } |
| + if err := s.Delete(ctx, legacy); err != nil { |
| + log.Printf("mail: sweep legacy records: deleting %s -> %v", legacy, err) |
| + continue |
| + } |
| + deleted++ |
| } |
| - deleted++ |
| } |
| return deleted, nil |
| } |
| ``` |
| -`cmd/console/main.go`, immediately after `go srv.StartAlertWatcher(context.Background())` (`:334`): |
| +Imports: `path` rather than `path/filepath`, since these are bucket keys and not OS paths. `fmt` is still used by the first wrap. |
| + |
| +`cmd/console/main.go`: there is no variable holding the release store to reuse. The `Config` literal starts at `:265` and passes `Releases: openStore()` inline (`:269`), and `srv.cfg` is unexported (`internal/console/server.go:680`), so the sweep cannot read it back off the server. Hoist it: |
| + |
| +```go |
| + // One release.Store, named, because the senders sweep below needs the |
| + // same one the server got. |
| + rel := openStore() |
| + srv, err := console.NewServer(console.Config{ |
| + Origin: origin, |
| + Keymail: envOr("CONSOLE_KEYMAIL", "https://keymail.dev"), |
| + Registry: envOr("CARLOS_REGISTRY", "/data/registry.db"), |
| + Releases: rel, |
| +``` |
| + |
| +Leave the rest of the literal untouched, including the `Bucket` comment below `Releases` that mentions `openStore()` by name. |
| + |
| +Then, immediately after `go srv.StartAlertWatcher(context.Background())` (`:334`): |
| ```go |
| // The senders migration's tail (tenant-email senders design §5): once a |
| @@ -1071,7 +1130,7 @@ func SweepLegacyRecords(ctx context.Context, s release.Store, enabled bool) (int |
| // it never touches an app that has not been migrated. |
| if os.Getenv("CARLOS_MAIL_LEGACY_SWEEP") == "1" { |
| go func() { |
| - n, err := mail.SweepLegacyRecords(context.Background(), relStore, true) |
| + n, err := mail.SweepLegacyRecords(context.Background(), rel, true) |
| if err != nil { |
| log.Printf("console: mail legacy sweep: %v", err) |
| return |
| @@ -1081,7 +1140,7 @@ func SweepLegacyRecords(ctx context.Context, s release.Store, enabled bool) (int |
| } |
| ``` |
| -Use whatever this function already calls the `release.Store` it passed as `Releases` in the `Config` literal (`:274` onwards); the name above is a placeholder for that variable and must be read from the file, not assumed. Add the `internal/mail` import if `cmd/console/main.go` does not already have it. |
| +Add the `internal/mail` import if `cmd/console/main.go` does not already have it. |
| - [ ] **Step 4: Run it to verify it passes** |
| @@ -1604,7 +1663,9 @@ git commit -m "mail: SenderOfCaller parses first and reads the index only for a |
| ### Task 7: Tenant resource associations |
| **Files:** |
| -- Modify: `internal/mail/provisioner.go` (`:49`), `internal/mail/memprovisioner.go` (`:40`, `:86`), `internal/mail/aws.go` (`sesAPI` at `:54`, after `EnsureTenant` at `:190`), `internal/mail/domain.go` (`VerifiedConfigSetARNs` at `:164`), `internal/mail/walk.go` (`EnsureDomain` at `:200-305`, `Teardown` at `:1190-1208`), `internal/mail/memprovisioner_test.go`, `internal/mail/walk_domain_test.go` |
| +- Modify: `internal/mail/provisioner.go` (`:49`), `internal/mail/memprovisioner.go` (`:40`, `:86`), `internal/mail/aws.go` (`sesAPI` at `:54`, after `EnsureTenant` at `:182`), `internal/mail/domain.go` (`VerifiedConfigSetARNs` at `:164`), `internal/mail/walk.go` (`EnsureDomain`'s association block goes in AFTER `PutDomain` at `:280`, beside `RefreshPolicies` at `:297`; `Teardown` at `:1190-1208`), `internal/mail/memprovisioner_test.go`, `internal/mail/walk_domain_test.go` |
| + |
| +**THE ASSOCIATION GOES AFTER `PutDomain`, AND THAT PLACEMENT IS LOAD-BEARING.** Do not associate beside `EnsureIdentity` (`:202-205`) or beside `EnsureConfigSet` (`:213-216`). `Teardown`'s orphan-claim sweep calls `p.DeleteIdentity` (`walk.go:1288`) on a claim that has NO domain record, so it never reaches the disassociate this task adds to the region loop — and once `MemProvisioner.DeleteIdentity` refuses an associated identity, an association written before the record exists makes that sweep unreachable. Associating only after the record is durable means an orphan claim never holds an associated identity, so the sweep still deletes it. |
| **Interfaces:** |
| - Produces: |
| @@ -1792,7 +1853,7 @@ func (p *MemProvisioner) DeleteIdentity(_ context.Context, region, domain string |
| } |
| ``` |
| -`internal/mail/aws.go`: `sesAPI` (`:54`) gains the two operations, and after `EnsureTenant` (`:190`): |
| +`internal/mail/aws.go`: `sesAPI` (`:54`) gains the two operations, and after `EnsureTenant` (`:182`): |
| ```go |
| // AssociateTenantResource associates an identity or a configuration set with |
| @@ -1828,7 +1889,7 @@ func (p *awsProvisioner) DisassociateTenantResource(ctx context.Context, tenant, |
| } |
| ``` |
| -Update the file's `TENANT ATTRIBUTION` header (`:19-25`) and `EnsureTenant`'s `PRECONDITION FOR SENDING, NOT YET MET HERE` paragraph (`:171-181`) to say that `EnsureDomain` now wires the associations, and record the spike's answer (design §4.1): the association is inert for the identity's owner in every respect except deletion, which is confirmed against the live API, and `ses:TenantName` IS enforced over SMTP. No send carries `X-SES-TENANT` in this release all the same, because nothing in step 2 composes an outgoing message or writes a tenant-conditioned policy. Step 3 is where both land. |
| +Update the file's `TENANT ATTRIBUTION` header (`:19-25`) and `EnsureTenant`'s `PRECONDITION FOR SENDING, NOT YET MET HERE` paragraph (above `:182`) to say that `EnsureDomain` now wires the associations, and record the spike's answer (design §4.1): the association is inert for the identity's owner in every respect except deletion, which is confirmed against the live API, and `ses:TenantName` IS enforced over SMTP. No send carries `X-SES-TENANT` in this release all the same, because nothing in step 2 composes an outgoing message or writes a tenant-conditioned policy. Step 3 is where both land. |
| `internal/mail/domain.go`, extract the ARN composition `VerifiedConfigSetARNs` (`:170-174`) does inline and call it from there: |
| @@ -1849,25 +1910,32 @@ func ConfigSetARN(identityARN, configSet string) (string, bool) { |
| } |
| ``` |
| -`internal/mail/walk.go`, `EnsureDomain`: after `p.EnsureIdentity` succeeds (`:205`) associate the identity, and inside the `d.SNSTopicARN != ""` branch after `EnsureConfigSet` (`:216`) associate the set. |
| +`internal/mail/walk.go`, `EnsureDomain`: BOTH associations go in one block immediately after `PutDomain` succeeds (`:280-282`) and immediately before the `GRANT BEFORE BIND` comment and `RefreshPolicies` (`:297`). `tenant` (`:181`), `ident` (`:202`) and `configSet` (`:213`) are all still in scope there. |
| ```go |
| - // The account's tenant gets this identity (design §7). Association does |
| - // not modify the identity and one identity may belong to many tenants, |
| - // so this changes nothing anyone can observe today. It is wired now |
| - // because SES validates the association on every tenant-attributed send |
| - // and step 3 sends that way. |
| + // The account's tenant gets this identity, and its configuration set |
| + // (design §7). Association does not modify the resource and one resource |
| + // may belong to many tenants, so this changes nothing anyone can observe |
| + // today. It is wired now because SES validates the association on every |
| + // tenant-attributed send and step 3 sends that way. |
| + // |
| + // AFTER PutDomain, not beside EnsureIdentity, and that ordering is the |
| + // whole of what keeps Teardown's orphan-claim sweep working. An |
| + // association is the one thing that stops an identity being deleted, and |
| + // the sweep (walk.go:1288) deletes the identity behind a claim that has |
| + // NO record — so it never runs the disassociate below. Associate before |
| + // the record is durable and a crash between the two leaves an identity |
| + // nothing can ever reap. |
| if err := p.AssociateTenantResource(ctx, tenant, ident.ARN); err != nil { |
| return nil, nil, fmt.Errorf("mail: associate identity %s with tenant %s: %w", domain, tenant, err) |
| } |
| -``` |
| - |
| -```go |
| + if configSet != "" { |
| if setARN, ok := ConfigSetARN(ident.ARN, configSet); ok { |
| if err := p.AssociateTenantResource(ctx, tenant, setARN); err != nil { |
| return nil, nil, fmt.Errorf("mail: associate configuration set %s with tenant %s: %w", configSet, tenant, err) |
| } |
| } |
| + } |
| ``` |
| `internal/mail/walk.go`, `Teardown`'s region loop (`:1190-1208`), before `DeleteIdentity`: |
| @@ -1893,7 +1961,14 @@ func ConfigSetARN(identityARN, configSet string) (string, bool) { |
| go test ./internal/mail/ -count=1 |
| ``` |
| -Expected: PASS. |
| +Expected: PASS, and in particular these two stay green, because they are the ones the new refusal can break: |
| + |
| +``` |
| +--- PASS: TestTeardownOrphanSweepDestroysTheIdentityBeforeReleasing |
| +--- PASS: TestTeardownOrphanSweepDoesNotCrossAccountsOnAMatchingAppName |
| +``` |
| + |
| +**If either of those goes red, the fix is the ORDERING, never the fake.** Both live in `internal/mail/walk_domainclaim_test.go` (`:493` and `:589`). Each injects `p.FailNext["EnsureConfigSet"]` (`:503`, `:601`) so `EnsureDomain` dies after `EnsureIdentity` and before any record is written, then expects `Teardown` to succeed (`:516`, `:615`) by destroying the orphaned identity. If they fail with "identity is still associated with tenant", the association was written too early: move it back to after `PutDomain`. Do NOT weaken `MemProvisioner.DeleteIdentity`'s refusal, and do not make the orphan sweep disassociate first — the refusal is what SES actually does (design §4.1, confirmed against the live API), and a fake that permits the delete would pass here and fail on the first real teardown. |
| - [ ] **Step 5: Gate and commit** |
| @@ -1929,7 +2004,7 @@ git commit -m "mail: associate each account's tenant with its identities and its |
| func TestMailSenderCountsAreBackfilledExactlyOnce(t *testing.T) { |
| dir := t.TempDir() |
| path := filepath.Join(dir, "console.db") |
| - st, err := OpenStore(path) |
| + st, err := Open(path) |
| if err != nil { |
| t.Fatal(err) |
| } |
| @@ -1988,7 +2063,7 @@ WHERE account = 'bag' AND app = 'harbourmail' AND sender = 'app' AND day = '2026 |
| } |
| ``` |
| -Add `"path/filepath"` to the test file's imports. Use whatever this package's store constructor is actually called; read it from `internal/console/store.go:40-83` rather than assuming `OpenStore`. |
| +Add `"path/filepath"` to the test file's imports. The constructor is `Open`, not `OpenStore` (`internal/console/store.go:53`). |
| - [ ] **Step 2: Run it to verify it fails** |
| @@ -2109,7 +2184,9 @@ git commit -m "console: mail_sender_counts, its one-shot backfill and the legacy |
| ### Task 9: `RecordMailEvent` and `MailCounts` take a sender |
| **Files:** |
| -- Modify: `internal/console/mailcounts.go` (`RecordMailEvent` at `:106`, `MailCounts` at `:120`), `internal/console/sesevents.go` (`:523`), `internal/console/apimail.go` (`:403`), `internal/console/mailcounts_test.go` (`:83`, `:88`, `:91`), `internal/console/mailevents_test.go` (`:165`, `:306`, `:418`), `internal/console/sesevents_test.go` (`:295`, `:778`) |
| +- Modify: `internal/console/mailcounts.go` (`RecordMailEvent` at `:106`, `MailCounts` at `:120`, and its OWN caller inside `enforceMailLimits` at `:230`), `internal/console/sesevents.go` (`:523`), `internal/console/apimail.go` (`:403`), `internal/console/mailcounts_test.go` (`:83`, `:88`, `:91`), `internal/console/mailevents_test.go` (`:165`, `:306`, `:418`), `internal/console/sesevents_test.go` (`:295`, `:778`) |
| + |
| +There are FOUR production call sites, not three. `enforceMailLimits` reads the counts itself at `mailcounts.go:230` — `c, err := s.store.MailCounts(ctx, acct, app, day)` — in the same file the signature changes in, and the package does not compile without it. |
| **Interfaces:** |
| - Produces: |
| @@ -2237,6 +2314,7 @@ Call sites, each taking the app sender because that is the only sender any of th |
| - `internal/console/sesevents.go:523` -> `s.store.RecordMailEvent(ctx, a.Sqid, app, mail.SenderAppID, day, ev.Type)` (task 11 replaces this line properly). |
| - `internal/console/apimail.go:403` -> `s.store.MailCounts(ctx, acct.Sqid, app, "", day)`: the top-level reply fields describe the app as a whole and keep doing so. |
| +- `internal/console/mailcounts.go:230`, inside `enforceMailLimits` -> `s.store.MailCounts(ctx, acct, app, "", day)`. `""` and not `mail.SenderAppID`, deliberately: `enforceMailLimits` still takes no sender in this task, so the only cap it can enforce is the app's whole day, which is exactly what it enforces today. Task 12 gives the function a sender and this line becomes `s.store.MailCounts(ctx, acct, app, sender, day)`. |
| - `internal/console/mailcounts_test.go:83`, `:88`, `:91` -> add `mail.SenderAppID` after the app. |
| - `internal/console/mailevents_test.go:165`, `:306`, `:418` and `internal/console/sesevents_test.go:295`, `:778` -> add `""` after the app, since each asserts the app's whole day. |
| @@ -2446,8 +2524,8 @@ git commit -m "console: the rename, purge and reap sweeps take mail_sender_count |
| - Modify: `internal/console/sesevents.go` (`recordAndEnforceMail` at `:466-535`), `internal/console/mailevents_test.go` (`:163-168`), `internal/console/sesevents_test.go` |
| **Interfaces:** |
| -- Consumes: `mail.SenderOfCaller`. |
| -- Produces: no signature change. `recordAndEnforceMail` counts once against the resolved sender and falls back to the domain loop only when the caller is unknown. |
| +- Consumes: `mail.SenderOfCaller`, `mail.GetSender`, `s.mailAppLive` (`sesevents.go:558` uses it for the log row). |
| +- Produces: no signature change. `recordAndEnforceMail` counts once against the resolved sender, only for a live app, and falls back to the domain loop only when the caller is unknown, skipping any app in per-instance mode. |
| - [ ] **Step 1: Write the failing test** |
| @@ -2479,6 +2557,17 @@ and, further down in the same test, after the two unnamed-caller events are reco |
| } |
| ``` |
| +and, at the very end of the same test (before its closing brace at `:206`), pin the gate the stray case now needs. The stray event is already there at `:172-174`: its caller is `mail.UserName(release.OpsAccount, "not-an-app")`, which parses cleanly and so takes the named branch. |
| + |
| +```go |
| + // The stray caller parses into a well-formed account and app, so it is |
| + // "named" — but "not-an-app" is not a live app, and a count row keyed on |
| + // a name no app answers to is a row nothing reads and nothing reaps. |
| + if c, _ := fx.st.MailCounts(ctx, release.OpsAccount, "not-an-app", "", day); c.Bounces != 0 { |
| + t.Errorf("a name that is not an app was given %d bounces", c.Bounces) |
| + } |
| +``` |
| + |
| `internal/console/mailevents_test.go` (append a new test): |
| ```go |
| @@ -2535,6 +2624,18 @@ Expected: `jb-site bounces = 1, want 0`. |
| // existed still resolves with no index entry of its own. |
| callerAcct, callerApp, callerSender, named := mail.SenderOfCaller(ctx, s.cfg.Releases, ev.Caller) |
| if named { |
| + // The COUNT is gated on the app being live, exactly as the log row |
| + // already is (recordMailMessage, sesevents.go:558). A caller name |
| + // can parse into an account and an app that is not a live app — |
| + // mail.UserName(acct, "not-an-app") is a well-formed name for an |
| + // app that was trashed, or never existed — and counting it would |
| + // insert a mail_sender_counts row keyed on a name no app answers |
| + // 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) { |
| + 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 |
| @@ -2568,6 +2669,18 @@ Expected: `jb-site bounces = 1, want 0`. |
| } |
| logHere := domainVerifiedForEvent(d, ev.Domain) |
| for _, app := range d.Apps { |
| + // An app in per-instance mode is skipped (design §8). Its |
| + // app sender is deliberately dead — no instance sends |
| + // through it — so charging it for a domain-wide event it |
| + // cannot have caused would pause a credential nobody holds |
| + // and hide the instance that actually sent. A read failure |
| + // is not a reason to drop the event: fall through and |
| + // charge, which is today's behaviour. |
| + if sn, _, err := mail.GetSender(ctx, s.cfg.Releases, a.Sqid, app, mail.SenderAppID); err != nil { |
| + log.Printf("console: ses events: reading the app sender for %s/%s -> %v", a.Sqid, app, err) |
| + } else if sn != nil && sn.PerInstance { |
| + continue |
| + } |
| if logHere { |
| s.recordMailMessage(ctx, a.Sqid, app, key, ev, false, now) |
| } |
| @@ -2610,7 +2723,38 @@ git commit -m "console: SES events are counted against the sender SES named, not |
| ### Task 12: Caps and pauses per sender |
| **Files:** |
| -- Modify: `internal/mail/walk.go` (`SetPaused` at `:962`), `internal/console/mailcounts.go` (`resumeStaleCapPause` at `:160`, `enforceMailLimits` at `:204`), `internal/console/apimail.go` (`handleAPIEmailStatus` at `:652`), `internal/console/sesevents.go` (the two calls from task 11), `internal/mail/walk_lifecycle_test.go`, `internal/console/mailcounts_test.go` |
| +- Modify: `internal/mail/walk.go` (`SetPaused` at `:962`), `internal/console/mailcounts.go` (`resumeStaleCapPause` at `:160`, `enforceMailLimits` at `:204`, and the `MailCounts` read at `:230`), `internal/console/apimail.go` (`handleAPIEmailStatus` at `:652`), `internal/console/sesevents.go` (the two calls from task 11), `internal/mail/walk_lifecycle_test.go`, `internal/console/mailcounts_test.go` |
| + |
| +**Both signatures have existing test callers, and there are fifteen of them.** They are not listed as an afterthought: `resumeStaleCapPause`'s four do not just need an extra argument, they need rewriting, because each one currently fetches a `*mail.Record` and asserts on it. |
| + |
| +- `enforceMailLimits`, eleven callers in `internal/console/mailcounts_test.go` at `:98`, `:119`, `:139`, `:169`, `:209`, `:221`, `:264`, `:276`, `:304`, `:311`, `:341`. Each is mechanical: insert `mail.SenderAppID` after `"harbourmail"`, so `fx.srv.enforceMailLimits(ctx, release.OpsAccount, "harbourmail", now)` becomes `fx.srv.enforceMailLimits(ctx, release.OpsAccount, "harbourmail", mail.SenderAppID, now)`. |
| +- `resumeStaleCapPause`, four callers in the same file at `:385`, `:407`, `:426`, `:448`, one per test: `TestResumeStaleCapPauseClearsACapPauseFromYesterday`, `...LeavesATodayCapPauseAlone`, `...NeverTouchesARatePause`, `...NeverTouchesAKindlessPause`. Each is preceded by a `mail.Get` and each asserts on `got.Provisioned`, so the `mail.Get` becomes a `mail.GetSender` and the sender argument goes in beside it. The whole of the first one, rewritten, is the shape for all four: |
| + |
| +```go |
| +func TestResumeStaleCapPauseClearsACapPauseFromYesterday(t *testing.T) { |
| + fx, now := mailFixture(t) |
| + pauseDirectly(t, fx, "harbourmail", mail.PauseKindCap, now.AddDate(0, 0, -1)) |
| + sn, _, err := mail.GetSender(context.Background(), fx.rel, release.OpsAccount, "harbourmail", mail.SenderAppID) |
| + if 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("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) |
| + } |
| +} |
| +``` |
| + |
| + The three assertion bodies are unchanged, because `*mail.Sender` and `*mail.Record` expose `Provisioned` identically. `pauseDirectly` (`:360`) keeps using `mail.Get`/`mail.Put` and needs no change: task 3 made those the app sender's accessors, so it is already writing `senders/app.json`. |
| **Interfaces:** |
| - Produces: |
| @@ -2662,6 +2806,8 @@ func TestPerInstanceModeRefusesAResume(t *testing.T) { |
| } |
| ``` |
| +Add `"errors"` to `walk_lifecycle_test.go`'s imports. Its block (`:1-10`) is `context`, `testing`, then `appconfig`, `dnsrec`, `release` — there is no `errors` in it today, so the `errors.Is` above does not compile as written. |
| + |
| `internal/console/mailcounts_test.go` (append): |
| ```go |
| @@ -2885,7 +3031,9 @@ func (s *Server) enforceMailLimits(ctx context.Context, acct, app, sender string |
| } |
| ``` |
| -`internal/console/sesevents.go`: restore the sender argument on the two `enforceMailLimits` calls from task 11. |
| +`internal/console/sesevents.go`: restore the sender argument on the two `enforceMailLimits` calls from task 11 — `s.enforceMailLimits(ctx, callerAcct, callerApp, callerSender, at)` in the named branch and `s.enforceMailLimits(ctx, a.Sqid, app, mail.SenderAppID, at)` in the fallback loop. |
| + |
| +`internal/console/mailcounts_test.go`: update the fifteen existing callers enumerated under **Files** above — the eleven `enforceMailLimits` calls take `mail.SenderAppID` after the app, and the four `resumeStaleCapPause` tests are rewritten to the shape shown there. |
| - [ ] **Step 4: Run it to verify it passes** |
| @@ -3145,7 +3293,7 @@ Expected: build failure, `unknown field Senders`. |
| - [ ] **Step 3: Write the code** |
| -`mail_cli.go`, beside `apiEmailCounts` (`:245`): |
| +`mail_cli.go`, beside `apiEmailCounts` (`:239`): |
| ```go |
| // apiEmailSender is one sender in the status reply (tenant-email senders |
| @@ -3222,9 +3370,9 @@ git commit -m "carlos email status prints a line per sender (plan 2, task 14)" |
| **Out of scope and deliberately absent.** No instance senders, no per-instance config layer, no `ConfigAck` change, no `svcrole.Mail`, no new doors, no external identities, no address claims, no provisioner refusal wrapper, no `X-SES-TENANT` anywhere, no `<P>_CONFIG_SET`. `PerInstance`, `PauseKindPerInstance`, `Host`, `Address`, `Identity` and `TenantARN` land on the record and nothing sets them. |
| -**Two rulings this plan takes.** (1) `DisassociateTenantResource` is built in task 7 even though §7 lists it among step 3's verbs: SES refuses to delete an identity while an association exists, so associating in step 2 without disassociating would break `Teardown` on the first real run, which is a capability regression and not a deferral. (2) The caller index's "account reap" deletion is `Teardown`'s, because the console refuses to delete an account that still holds apps (`handleDeleteAccount`, `internal/console/ownership.go:82`; `DeleteAccount` re-checks inside its own transaction), so every index entry an account holds goes through a per-app teardown. No separate account sweep is built. |
| +**Three rulings this plan takes.** (0) Task 7's associations go in after `PutDomain`, not beside `EnsureIdentity`. An association is the one thing that stops an identity being deleted, and `Teardown`'s orphan-claim sweep deletes the identity behind a claim that has no record, so it never reaches the disassociate. Associating only once the record is durable is what keeps that sweep able to do its job. (1) `DisassociateTenantResource` is built in task 7 even though §7 lists it among step 3's verbs: SES refuses to delete an identity while an association exists, so associating in step 2 without disassociating would break `Teardown` on the first real run, which is a capability regression and not a deferral. (2) The caller index's "account reap" deletion is `Teardown`'s, because the console refuses to delete an account that still holds apps (`handleDeleteAccount`, `internal/console/ownership.go:82`; `DeleteAccount` re-checks inside its own transaction), so every index entry an account holds goes through a per-app teardown. No separate account sweep is built. |
| -**Placeholder scan.** Task 4 names the `release.Store` variable in `cmd/console/main.go` as something to read rather than assume, and task 8 says the same about the store constructor's name; both are one-line reads in files this plan cites by line. Task 13's optional bare-app helper is marked droppable. Everything else carries code. |
| +**Placeholder scan.** None left. Task 4 hoists `rel := openStore()` itself, with the real lines shown, because `cmd/console/main.go:269` passes `Releases: openStore()` inline and `srv.cfg` is unexported; task 8 names the constructor as `Open` (`internal/console/store.go:53`). Task 13's optional bare-app helper is marked droppable. Everything else carries code. |
| **Type consistency.** `Sender`, `SenderAppID`, `ScopeApp`, `ScopeInstance`, `SourceDomain`, `SourceAddress`, `PauseKindPerInstance`, `SenderKey`, `SendersPrefix`, `ValidSenderID`, `SenderNameSet`, `SenderNames`, `StandaloneUser`, `ErrNameTooLong`, `CallerIndex`, `CallerKey`, `GetCallerIndex`, `PutCallerIndex`, `DeleteCallerIndex`, `ErrCallerClaimed`, `ResolveSenderNames`, `SenderOfCaller`, `LegacyKey`, `GetSender`, `PutSender`, `ListSenders`, `SweepLegacyRecords`, `ConfigSetARN`, `AssociateTenantResource`, `DisassociateTenantResource`, `SetSenderPaused`, `ErrPerInstance`, `mailCapFor`, `mailAppTimeSince`, `DropLegacyMailCounts`, `apiEmailSender` are spelled identically across every task that produces or consumes them. |