#19: fix F1-F3 defects found auditing the phases against PLAN.md
Every fix here is a case where the code and the plan disagreed, and the code was wrong. Certificates (section 8.2, 8.4). The parser documented that it skipped expired entries but never checked, so an expired certificate would be served until every handshake failed. LastReload() only advanced when acme.json changed - and it does not change for sixty days - which made the timestamp risk R2 monitors indistinguishable from a watcher gone blind. Expiry is now checked on every pass, LastReload records the check and LastChange the swap, and the <7d/<24h alerts of section 8.4 exist. ARC (section 5). buildArcSealer resolved the DKIM key at startup, before runDnsReconciler had provisioned one. On a first boot ARC was disabled until someone restarted the process, which the log even admitted while offering no way for it to start. A LazySealer binds on first use instead. Health (section 8.4). There was no degraded state at all. /health now reports one - still 200, because restarting cannot fix a certificate Traefik never issued. Bounces. DSNs were sent From postmaster@lambdamail.local, a domain that does not resolve, so the notification was itself undeliverable. They now carry the configured mail host, a Message-ID, and Auto-Submitted so a vacation responder cannot loop against them. Fan-out atomicity (section 6.1). Persist ran one transaction per recipient, so a failure halfway through an alias fan-out left earlier recipients with a copy while the sender was told to retry - and the retry delivered to them twice. PersistAll commits the whole delivery or none of it. Verified against Postgres by reverting the repository to the old behaviour and confirming the new test fails on it. SMTP replies. The use case encoded replies in error text ("550 5.7.1 ...") which the adapter re-parsed by prefix, so rewording a message could turn a permanent refusal into a retry. SmtpRejection carries the decision and the adapter renders it. Repository tests. These only run with TEST_DATABASE_URL, which CI does set - so the job was red: copy and expunge seeded a 68-character digest into a CHAR(64) column, and three others used fixed digests that collide with the UNIQUE index on any re-run. A shared testBlobDigest() helper fixes both; the suite now passes twice in a row against one database. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
1 parent
2e0e65b167
commit
33d91fa247
23 files changed
+906
-139
No files matched your search
@@ -42,7 +42,7 @@ func TestCopyRepository_CopyMessages_AllocatesNewUIDsAndPreservesFlags(t *testin
|
||||
}
|
||||
blobID := uuid.New()
|
||||
_, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes, ref_count) VALUES ($1, $2, 'local', '/tmp/x', 10, 1)`,
|
||||
blobID, uuid.New().String()+uuid.New().String()[:32])
|
||||
blobID, testBlobDigest())
|
||||
if err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
|
||||
@@ -35,7 +35,7 @@ func TestExpungeRepository_Expunge_SetsExpungedAtAndDecrementsCounters(t *testin
|
||||
}
|
||||
blobID := uuid.New()
|
||||
_, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`,
|
||||
blobID, uuid.New().String()+uuid.New().String()[:32])
|
||||
blobID, testBlobDigest())
|
||||
if err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
|
||||
@@ -37,7 +37,7 @@ func TestFlagRepository_SetFlags_AddThenDelThenSet(t *testing.T) {
|
||||
}
|
||||
blobID := uuid.New()
|
||||
_, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`,
|
||||
blobID, "1111110000000000000000000000000000000000000000000000000000000000"[:64])
|
||||
blobID, testBlobDigest())
|
||||
if err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
|
||||
@@ -4,6 +4,7 @@ import (
|
||||
"context"
|
||||
"fmt"
|
||||
|
||||
"github.com/jackc/pgx/v5"
|
||||
"github.com/jackc/pgx/v5/pgxpool"
|
||||
|
||||
"lambdamail/protocols/internal/application/port"
|
||||
@@ -21,13 +22,37 @@ func NewInboundMessageRepository(pool *pgxpool.Pool) *InboundMessageRepository {
|
||||
return &InboundMessageRepository{pool: pool}
|
||||
}
|
||||
|
||||
func (r *InboundMessageRepository) Persist(ctx context.Context, input port.PersistInboundMessageInput) (int64, error) {
|
||||
// PersistAll records every recipient's copy in one transaction, so a delivery
|
||||
// that fans out is all-or-nothing and a retried SMTP transaction cannot leave
|
||||
// an earlier recipient with a duplicate.
|
||||
func (r *InboundMessageRepository) PersistAll(ctx context.Context, inputs []port.PersistInboundMessageInput) ([]int64, error) {
|
||||
if len(inputs) == 0 {
|
||||
return nil, nil
|
||||
}
|
||||
|
||||
tx, err := r.pool.Begin(ctx)
|
||||
if err != nil {
|
||||
return 0, err
|
||||
return nil, err
|
||||
}
|
||||
defer tx.Rollback(ctx) // no-op if Commit succeeds
|
||||
|
||||
uids := make([]int64, 0, len(inputs))
|
||||
for _, input := range inputs {
|
||||
uid, err := persistOne(ctx, tx, input)
|
||||
if err != nil {
|
||||
return nil, err
|
||||
}
|
||||
uids = append(uids, uid)
|
||||
}
|
||||
|
||||
if err := tx.Commit(ctx); err != nil {
|
||||
return nil, fmt.Errorf("commit: %w", err)
|
||||
}
|
||||
return uids, nil
|
||||
}
|
||||
|
||||
// persistOne writes one recipient's copy inside the caller's transaction.
|
||||
func persistOne(ctx context.Context, tx pgx.Tx, input port.PersistInboundMessageInput) (int64, error) {
|
||||
targetFolder := input.TargetFolderName
|
||||
if targetFolder == "" {
|
||||
targetFolder = "INBOX"
|
||||
@@ -35,7 +60,7 @@ func (r *InboundMessageRepository) Persist(ctx context.Context, input port.Persi
|
||||
|
||||
var folderID string
|
||||
var uid int64
|
||||
err = tx.QueryRow(ctx, `
|
||||
err := tx.QueryRow(ctx, `
|
||||
SELECT id, uid_next FROM folders
|
||||
WHERE mailbox_id = $1 AND (special_use = LOWER($2) OR LOWER(name) = LOWER($2))
|
||||
ORDER BY special_use IS NOT NULL DESC
|
||||
@@ -84,8 +109,5 @@ func (r *InboundMessageRepository) Persist(ctx context.Context, input port.Persi
|
||||
return 0, fmt.Errorf("insert outbox event: %w", err)
|
||||
}
|
||||
|
||||
if err := tx.Commit(ctx); err != nil {
|
||||
return 0, fmt.Errorf("commit: %w", err)
|
||||
}
|
||||
return uid, nil
|
||||
}
|
||||
+85
-14
@@ -38,7 +38,7 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T)
|
||||
|
||||
blobID := uuid.New()
|
||||
_, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`,
|
||||
blobID, "deadbeef00000000000000000000000000000000000000000000000000000000"[:64])
|
||||
blobID, testBlobDigest())
|
||||
if err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
@@ -46,22 +46,23 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T)
|
||||
repo := NewInboundMessageRepository(pool)
|
||||
blob := port.BlobRef{ID: blobID, SHA256: "deadbeef", SizeBytes: 10}
|
||||
|
||||
firstUID, err := repo.Persist(ctx, port.PersistInboundMessageInput{
|
||||
firstUIDs, err := repo.PersistAll(ctx, []port.PersistInboundMessageInput{{
|
||||
MailboxID: mailboxID, Blob: blob,
|
||||
SenderAddress: "sender@example.test", RecipientAddress: "user@msgrepo-test-" + domainID.String() + ".invalid",
|
||||
SPFResult: "none", DKIMResult: "none", DMARCResult: "none",
|
||||
})
|
||||
}})
|
||||
if err != nil {
|
||||
t.Fatalf("first Persist: %v", err)
|
||||
t.Fatalf("first PersistAll: %v", err)
|
||||
}
|
||||
secondUID, err := repo.Persist(ctx, port.PersistInboundMessageInput{
|
||||
secondUIDs, err := repo.PersistAll(ctx, []port.PersistInboundMessageInput{{
|
||||
MailboxID: mailboxID, Blob: blob,
|
||||
SenderAddress: "sender2@example.test", RecipientAddress: "user@msgrepo-test-" + domainID.String() + ".invalid",
|
||||
SPFResult: "none", DKIMResult: "none", DMARCResult: "none",
|
||||
})
|
||||
}})
|
||||
if err != nil {
|
||||
t.Fatalf("second Persist: %v", err)
|
||||
t.Fatalf("second PersistAll: %v", err)
|
||||
}
|
||||
firstUID, secondUID := firstUIDs[0], secondUIDs[0]
|
||||
if secondUID <= firstUID {
|
||||
t.Errorf("secondUID (%d) must be strictly greater than firstUID (%d) - RFC 3501 section 2.3.1.1", secondUID, firstUID)
|
||||
}
|
||||
@@ -71,7 +72,7 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T)
|
||||
t.Fatalf("query ref_count: %v", err)
|
||||
}
|
||||
if refCount != 2 {
|
||||
t.Errorf("ref_count = %d, want 2 after two Persist calls referencing the same blob", refCount)
|
||||
t.Errorf("ref_count = %d, want 2 after two PersistAll calls referencing the same blob", refCount)
|
||||
}
|
||||
|
||||
var unreadCount, totalCount int
|
||||
@@ -79,10 +80,10 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T)
|
||||
t.Fatalf("query folder counters: %v", err)
|
||||
}
|
||||
if unreadCount != 2 {
|
||||
t.Errorf("folder unread_count = %d, want 2 after two Persist calls", unreadCount)
|
||||
t.Errorf("folder unread_count = %d, want 2 after two PersistAll calls", unreadCount)
|
||||
}
|
||||
if totalCount != 2 {
|
||||
t.Errorf("folder total_count = %d, want 2 after two Persist calls", totalCount)
|
||||
t.Errorf("folder total_count = %d, want 2 after two PersistAll calls", totalCount)
|
||||
}
|
||||
|
||||
var usedBytes int64
|
||||
@@ -90,7 +91,7 @@ func TestInboundMessageRepository_Persist_AllocatesSequentialUIDs(t *testing.T)
|
||||
t.Fatalf("query mailbox used_bytes: %v", err)
|
||||
}
|
||||
if usedBytes != blob.SizeBytes*2 {
|
||||
t.Errorf("mailbox used_bytes = %d, want %d (two Persist calls of %d bytes each)", usedBytes, blob.SizeBytes*2, blob.SizeBytes)
|
||||
t.Errorf("mailbox used_bytes = %d, want %d (two PersistAll calls of %d bytes each)", usedBytes, blob.SizeBytes*2, blob.SizeBytes)
|
||||
}
|
||||
}
|
||||
|
||||
@@ -118,18 +119,88 @@ func TestInboundMessageRepository_Persist_ReturnsErrorWhenNoInboxFolderExists(t
|
||||
|
||||
blobID := uuid.New()
|
||||
_, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`,
|
||||
blobID, "cafebabe00000000000000000000000000000000000000000000000000000000"[:64])
|
||||
blobID, testBlobDigest())
|
||||
if err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
|
||||
repo := NewInboundMessageRepository(pool)
|
||||
_, err = repo.Persist(ctx, port.PersistInboundMessageInput{
|
||||
_, err = repo.PersistAll(ctx, []port.PersistInboundMessageInput{{
|
||||
MailboxID: mailboxID, Blob: port.BlobRef{ID: blobID, SizeBytes: 10},
|
||||
SenderAddress: "sender@example.test", RecipientAddress: "nofolder@example.test",
|
||||
SPFResult: "none", DKIMResult: "none", DMARCResult: "none",
|
||||
})
|
||||
}})
|
||||
if err == nil {
|
||||
t.Fatal("expected an error when the mailbox has no INBOX folder, got nil")
|
||||
}
|
||||
}
|
||||
|
||||
// An alias fanning out is one SMTP transaction with one reply. If a later
|
||||
// recipient's insert fails, the earlier ones must not remain: the sender is
|
||||
// told to retry, and the retry would deliver to them a second time.
|
||||
func TestInboundMessageRepository_PersistAll_RollsBackEveryRecipientOnFailure(t *testing.T) {
|
||||
pool := testPool(t)
|
||||
defer pool.Close()
|
||||
ctx := context.Background()
|
||||
|
||||
domainID := uuid.New()
|
||||
domainName := "fanout-test-" + domainID.String() + ".invalid"
|
||||
if _, err := pool.Exec(ctx, `INSERT INTO domains (id, name, punycode_name) VALUES ($1, $2, $3)`,
|
||||
domainID, domainName, domainName); err != nil {
|
||||
t.Fatalf("seed domain: %v", err)
|
||||
}
|
||||
defer pool.Exec(ctx, `DELETE FROM domains WHERE id = $1`, domainID)
|
||||
|
||||
// The first mailbox has an INBOX, the second deliberately does not, so
|
||||
// persisting the pair fails on the second entry.
|
||||
goodMailboxID, badMailboxID := uuid.New(), uuid.New()
|
||||
for i, id := range []uuid.UUID{goodMailboxID, badMailboxID} {
|
||||
local := []string{"good", "bad"}[i]
|
||||
if _, err := pool.Exec(ctx, `INSERT INTO mailboxes (id, domain_id, local_part, email_address, password_hash) VALUES ($1, $2, $3, $4, '$argon2id$v=19$m=65536,t=3,p=4$c29tZXNhbHQ$RdescudvJCsgt3ub+b+dWRWJTmaaJObG')`,
|
||||
id, domainID, local, local+"@"+domainName); err != nil {
|
||||
t.Fatalf("seed mailbox %s: %v", local, err)
|
||||
}
|
||||
}
|
||||
|
||||
folderID := uuid.New()
|
||||
if _, err := pool.Exec(ctx, `INSERT INTO folders (id, mailbox_id, name, special_use) VALUES ($1, $2, 'INBOX', 'inbox')`,
|
||||
folderID, goodMailboxID); err != nil {
|
||||
t.Fatalf("seed folder: %v", err)
|
||||
}
|
||||
|
||||
blobID := uuid.New()
|
||||
blobDigest := testBlobDigest()
|
||||
if _, err := pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 10)`,
|
||||
blobID, blobDigest); err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
defer pool.Exec(ctx, `DELETE FROM message_blobs WHERE id = $1`, blobID)
|
||||
blob := port.BlobRef{ID: blobID, SHA256: blobDigest, SizeBytes: 10}
|
||||
|
||||
repo := NewInboundMessageRepository(pool)
|
||||
_, err := repo.PersistAll(ctx, []port.PersistInboundMessageInput{
|
||||
{MailboxID: goodMailboxID, Blob: blob, SenderAddress: "sender@remote.test", RecipientAddress: "alias@" + domainName,
|
||||
SPFResult: "none", DKIMResult: "none", DMARCResult: "none"},
|
||||
{MailboxID: badMailboxID, Blob: blob, SenderAddress: "sender@remote.test", RecipientAddress: "alias@" + domainName,
|
||||
SPFResult: "none", DKIMResult: "none", DMARCResult: "none"},
|
||||
})
|
||||
if err == nil {
|
||||
t.Fatal("PersistAll succeeded even though the second recipient has no INBOX")
|
||||
}
|
||||
|
||||
var messageCount int
|
||||
if err := pool.QueryRow(ctx, `SELECT count(*) FROM email_messages WHERE mailbox_id = $1`, goodMailboxID).Scan(&messageCount); err != nil {
|
||||
t.Fatalf("count messages: %v", err)
|
||||
}
|
||||
if messageCount != 0 {
|
||||
t.Errorf("the first recipient kept %d message(s) after a failed fan-out; the sender's retry would duplicate them", messageCount)
|
||||
}
|
||||
|
||||
var refCount int
|
||||
if err := pool.QueryRow(ctx, `SELECT ref_count FROM message_blobs WHERE id = $1`, blobID).Scan(&refCount); err != nil {
|
||||
t.Fatalf("query ref_count: %v", err)
|
||||
}
|
||||
if refCount != 0 {
|
||||
t.Errorf("ref_count = %d after a rolled-back delivery, want 0", refCount)
|
||||
}
|
||||
}
|
||||
@@ -3,6 +3,7 @@ package postgres
|
||||
import (
|
||||
"context"
|
||||
"os"
|
||||
"strings"
|
||||
"testing"
|
||||
|
||||
"github.com/google/uuid"
|
||||
@@ -309,3 +310,10 @@ func TestMailboxRepository_ResolveDeliveryTargets_SkipsAliasDestinationThatIsNot
|
||||
t.Errorf("targets = %+v, want empty slice - external forwarding destinations are not internal mailboxes and outbound delivery does not exist yet", targets)
|
||||
}
|
||||
}
|
||||
|
||||
// testBlobDigest produces a unique 64-character value for message_blobs'
|
||||
// content_sha256, which is UNIQUE in the schema. A hardcoded digest makes the
|
||||
// test pass exactly once per database and fail on every re-run.
|
||||
func testBlobDigest() string {
|
||||
return strings.ReplaceAll(uuid.New().String()+uuid.New().String(), "-", "")[:64]
|
||||
}
|
||||
+1
-1
@@ -35,7 +35,7 @@ func TestMessageQueryRepository_ListMessages_ReturnsRowsOrderedByUID(t *testing.
|
||||
}
|
||||
blobID := uuid.New()
|
||||
_, err = pool.Exec(ctx, `INSERT INTO message_blobs (id, content_sha256, storage_driver, storage_path, size_bytes) VALUES ($1, $2, 'local', '/tmp/x', 42)`,
|
||||
blobID, "abcdef0000000000000000000000000000000000000000000000000000000000"[:64])
|
||||
blobID, testBlobDigest())
|
||||
if err != nil {
|
||||
t.Fatalf("seed blob: %v", err)
|
||||
}
|
||||
|
||||
Reference in new issue
Block a user