diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index ae1f25599..8b60f17dc 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -47,6 +47,7 @@ jobs: with: filters: | go-services: + - '.github/workflows/ci.yml' - 'go.mod' - 'go.sum' - 'internal/**' @@ -220,6 +221,13 @@ jobs: go test -race ./internal/tasks -run '^Test(LiveWarmup(SafetyHoldReschedules|Recovery|TaskDoesNotApplyResults|DispatchDefersInvalidHours)|WarmupSenderDistinguishes)' -count=1 go test -race ./internal/app/consumer -run '^TestLiveStoredSendResult' -count=1 + - name: Warmup charge and authoritative result regressions (PostgreSQL) + env: + WARMBLY_TEST_DB: postgres://warmbly:warmbly@127.0.0.1:5432/warmbly_test?sslmode=disable + run: | + go test -race -v -p 1 ./internal/repository ./internal/app/consumer ./internal/tasks ./internal/app/orgtransfer -run '^TestLive(Warmup(ReservationPreservesOriginalChargeDay|RefundCASProtectsConcurrentAndRestampedTasks|RefundAndResultRollbackTogether|ReplyRefundUsesChargedShapeAfterParentDeletion|ChargeAndRefundUseUTCInNonUTCSession|ChargeProofRollsBackWhenCounterWriteFails|CancellationPathsRefundOnlyProvenCharge|GenericCompletionDoesNotProveChargedAccounting|SendFailureGivesTheDayBack|SendGiveBackIsIdempotent|SafetyHoldReschedulesInsteadOfBlockingTheDispatcher|Recovery(RetiresExistingUnstartedDispatchOnce|HandlesPartialReservations|PreservesExecutionAndReceiptEvidence|KeepsOtherUnknownAndRestrictiveHolds|AndBeginCannotBothWin))|AppliedWarmupFailureRepairsOnlyRefundedChargeDeadLetters|GiveBackDailySend(ReversesADispatchedWarmupSend|LeavesOtherDaysAlone)|AccountDailyUsage(CountsWarmupTasksNotTheStatisticsRow|ScopesWarmupToTypeAndDay)|ImportWarmupStatisticsCannotGrantChargeRefundAuthority|Legacy(FailedStatusCannotProveNonExecution|JSONAndAvroFailurePersistConservativeAdmission)|AuthoritativeDeadLetterResultsReconcileWithoutReplay|DeadLetterResultEffectsAndSettlementRollBackTogether|Campaign(DeadLetterWorkerResultReleasesReservationOnce|ForeignKeyCleanupDoesNotProveWakeupOnly)|UserEmailDeadLetterUsesUnderlyingSendTaskType|UnknownSendOperatorResolutionReconcilesAndAudits|RecordedWorkerOutcomeSurvivesLostEvent|UnreservedLegacyFailedTaskRetainsUnknownHold|UnknownResolutionRejectsMissingEvidenceAndLiveExecution|SendRecoveryResolutionRequiresEvidenceAndRetainsUnknown)$' -count=1 + go test -race -v -p 1 ./internal/infrastructure/db -run '^TestLiveWarmupChargeUpgradeKeepsLegacyUnverifiedAndPreservesEvidence$' -count=1 + - name: Mailbox sync admission, replay, truthful state and upgrade regressions (PostgreSQL) env: WARMBLY_TEST_DB: postgres://warmbly:warmbly@127.0.0.1:5432/warmbly_test?sslmode=disable diff --git a/cmd/warmblyctl/remote_output.go b/cmd/warmblyctl/remote_output.go index e7ad9e866..5c3c13484 100644 --- a/cmd/warmblyctl/remote_output.go +++ b/cmd/warmblyctl/remote_output.go @@ -102,7 +102,7 @@ arrival_aging arrival_due arrival_leased arrival_pending arrival_stage_0 arrival arrival_stage_2 arrival_stage_3 arrival_wait backfill_complete backfill_pending backfill_running backfill_stale_running campaign_reserved campaign_reserved_aging confirmed_other_1h confirmed_warmup_1h confirmed_warmup_proxy_1h credential_errors -dispatch_due dispatch_old_failure dispatch_persistent dlq_due dlq_exhausted dlq_replayed +dispatch_due dispatch_old_failure dispatch_persistent dlq_due dlq_exhausted dlq_replayed dlq_unverified dlq_unscheduled dlq_wait effects_leased effects_pending effects_repeated effects_unleased failed_results_1h failed_results_24h future_schedule holds_authentication holds_conflict warmup_invalid_window holds_permanent holds_unknown inactive_credential_errors job_error_old job_error_recent diff --git a/docs/content/docs/api/reference/deliverability-ops.mdx b/docs/content/docs/api/reference/deliverability-ops.mdx index 2ec34e0e7..bf239fb23 100644 --- a/docs/content/docs/api/reference/deliverability-ops.mdx +++ b/docs/content/docs/api/reference/deliverability-ops.mdx @@ -292,13 +292,15 @@ Auth: **Scope** `SEND_CAMPAIGNS` · **Org permission** `send_campaigns` | Parameter | In | Type | Description | |-----------|----|------|-------------| -| `status` | query | string | Optional status filter (e.g. `pending`, `replayed`). | +| `status` | query | string | Optional status filter (e.g. `pending`, `replayed`, `resolved`). | | `limit` | query | integer | Max rows to return, 1 to 200 (default 100). | ### Response Returns a `data` array of dead-letter records. This endpoint is not cursor-paginated; it returns up to `limit` rows in one response. +Send dead letters become `resolved` only after an authoritative worker or operator-confirmed send outcome is reconciled. Recovery does not resend mail: the original payload, error and attempts stay available for inspection. Unknown outcomes remain pending and held. A missing Sent-folder copy, reservation marker or executor marker is not proof that nothing was sent. + ```json { "data": [ @@ -324,7 +326,9 @@ Returns a `data` array of dead-letter records. This endpoint is not cursor-pagin `POST /tasks/dlq/:id/replay` -Re-dispatches a dead-lettered task. Because a replay can transmit real mail, this requires the send permission rather than plain write access. +Replays only a dead-lettered campaign wakeup with explicit durable wakeup intent, rechecked under its campaign lock. Once a task binds a contact or sequence for sending, its send intent stays restrictive even after either record is deleted. Legacy or imported tasks without verified wakeup intent and all send-capable tasks return `409 Conflict` and retain their dead-letter evidence. A retry date, JSON payload, null contact/sequence, or missing send marker cannot grant replay. This route requires the send permission rather than plain write access. + +Database downgrades refuse to discard this provenance while send-intent or unresolved unverified dead-letter rows exist. Keep the schema and roll forward instead. Auth: **Scope** `SEND_CAMPAIGNS` · **Org permission** `send_campaigns` diff --git a/docs/content/docs/development/admin-panel.mdx b/docs/content/docs/development/admin-panel.mdx index 794d109cd..80599c185 100644 --- a/docs/content/docs/development/admin-panel.mdx +++ b/docs/content/docs/development/admin-panel.mdx @@ -71,7 +71,7 @@ Relative ages use your browser's current clock when a snapshot arrives, with per Mailbox diagnostic participation, sender/recipient flags and shared/rolling limits are organization-scoped mailbox fields, not new admin-panel switches. Use the existing authorized mailbox PATCH; no new UI is implied. An operator-reviewed `send_recovery_resolution` binds the current held task/reason and definite repair/provider evidence, never an unknown send. See [send safety](/development/send-safety/). | **Seed panel** | The instance's placement seed inboxes by provider, adding and removing them, every workspace's recent [placement tests](/guides/placement-tests/) with their results, and an operator test that counts against no allowance | | **Campaigns** | Every campaign on the instance with its state and owner | -| **Sends** | In-flight reservations no worker has answered, and the reclaimer that resolves them after 30 minutes; dead letters with replay; task failures; customer webhook delivery health, with reclaim | +| **Sends** | In-flight reservations no worker has answered, and the reclaimer that resolves them after 30 minutes; dead letters with replay limited to non-sending campaign wakeups; task failures; customer webhook delivery health, with reclaim | ## Accounts diff --git a/docs/content/docs/development/data-control.mdx b/docs/content/docs/development/data-control.mdx index ae5762100..2d73a897a 100644 --- a/docs/content/docs/development/data-control.mdx +++ b/docs/content/docs/development/data-control.mdx @@ -285,6 +285,8 @@ The `rsync` alternative works too and is sometimes simpler, as long as the stack New durable admissions retain an organization-encrypted arrival payload in `sync_arrival_outbox` alongside the provider map. The queue tracks delivery stages and retries consumer failures under the original message identity. A delivered arrival is removed; an unresolved arrival is retained for retry until it completes or its owning mailbox or organization is deleted. Deleting the mailbox intentionally removes that mailbox's pending work. This queue does not schedule new sends or authorize repeating a send. +Provider reoffers check the canonical message map before admitting new mail. IMAP copies with the same RFC Message-ID in one fetched batch use that same known-message path after the first admission, preserving the stored body and identity without consuming the message budget twice. Live copies still reconcile their UID and flags. This worker fix needs no schema change or backfill, and does not infer historical delivery success from a legacy map without durable admission proof. + A full-instance backup must preserve the database, wrapped organization keys, the root key needed to unwrap them, and referenced message-body storage together. Restoring a queue without its keys or body objects cannot restore delivery. Use the normal consistent instance backup, not a separate copy of just the queue. A workspace archive excludes the queue and provider mappings whose arrivals are still pending. Drain pending work on the source before moving a workspace; importing an archive does not resume source-instance delivery. The consumer logs aggregate pending counts and oldest-arrival age. Arrivals older than five minutes, or delivery/inspection failures continuing for one minute, raise a deduplicated operator alert through the existing operator-notification integration when configured. Logs and notifications contain counts and age, not message bodies or addresses. Operator notifications repeat at most once per fifteen minutes; without an operator channel, monitor consumer logs for `sync arrival queue needs attention`. An empty queue is not proof of provider success or historical completeness. diff --git a/docs/content/docs/development/instance-health.mdx b/docs/content/docs/development/instance-health.mdx index a413b3ac3..832ba11ec 100644 --- a/docs/content/docs/development/instance-health.mdx +++ b/docs/content/docs/development/instance-health.mdx @@ -67,7 +67,7 @@ Two neighbouring endpoints round out the surface, both under **Instance** in the ## Operational monitoring -**Operations** renders the version `"1"` snapshot from `GET /admin/instance/monitoring`. It covers registered workers, worker sample receipt, confirmed sends, campaign reservations, dispatch, safety holds, sync state and fair-use waits, stored mailbox errors, jobs, dead letters, webhooks, customer notification rows, send-result effects, domain checks, warmup mailbox loading and broker queue observations. Unsupported sources remain visible as unavailable, including detailed send-wait attribution and external tracking-broker diagnostics. Optional arrival-outbox measurements are unavailable when their schema is absent; monitoring needs no arrival migration and does not require a new worker wire version. +**Operations** renders the version `"1"` snapshot from `GET /admin/instance/monitoring`. It covers registered workers, worker sample receipt, confirmed sends, campaign reservations, dispatch, safety holds, sync state and fair-use waits, stored mailbox errors, jobs, dead letters, webhooks, customer notification rows, send-result effects, domain checks, warmup mailbox loading and broker queue observations. Dead letters without safe replay proof remain visible as recovery-unverified even when a retry date has passed; the due, waiting and unscheduled counts only cover non-sending campaign wakeup passes. The consumer does not have a task scheduler for automatic replay. Unsupported sources remain visible as unavailable, including detailed send-wait attribution and external tracking-broker diagnostics. Optional arrival-outbox measurements are unavailable when their schema is absent; monitoring needs no arrival migration and does not require a new worker wire version. The backend collects with an eight-second overall timeout, three seconds per source and at most four concurrent collectors. It shares a one-minute snapshot across admins, then filters source permissions for each response. HTTP responses are not cacheable. On collection failure it can retain a source's last good measurement for at most five minutes, marked stale and with its original observation/evidence clocks. After that it is unavailable. Source samples are bounded; measured/expected scope counts and incomplete coverage expose truncation. Counts in partial samples may be lower bounds, never extrapolated fleet totals. diff --git a/docs/content/docs/development/send-safety.mdx b/docs/content/docs/development/send-safety.mdx index 4ffc43b3c..e048fdab7 100644 --- a/docs/content/docs/development/send-safety.mdx +++ b/docs/content/docs/development/send-safety.mdx @@ -39,6 +39,10 @@ The executor rechecks organization, mailbox, worker, provider, durable task/nonc Result reconciliation commits task, campaign progress, variant, suppression and warmup-health changes together. Bounce webhooks and notifications enter an idempotent durable outbox in that transaction, and result consumers drain it after commit. Failed delivery retains a leased retry row; a consumer restart does not lose the effect. Stable downstream webhook delivery and notification-feed IDs prevent duplicate stored rows. External integration, Slack/push and realtime delivery remain at least once/best effort, not exactly-once delivery. Core accounting and unknown holds remain durable. +Warmup authorization records the UTC charge day and whether a reply was counted in the same transaction as the statistics increment. Outbound reservation preserves the initial completion timestamp. A definitive failure or safe unstarted cancellation refunds only that recorded charge, at most once, in the same transaction as the task change. Completion timestamps and dispatch/executor nonces alone never authorize a refund. Pre-upgrade and imported tasks have no positive charge record: known outcomes can still settle, but historical statistics are not decremented by inference. Unknown outcomes remain held and unresolved. + +If a modern warmup task becomes dead-lettered after its definitive failure and refund were committed, the matching authoritative result repairs only task and dead-letter metadata. It does not repeat accounting, callbacks or mailbox hold changes. Already-applied failures without positive refunded-charge proof remain protected. + ## Evidence-based repair holds Authentication, permanent and conflicting-result holds do not clear merely because time passed, a reconnect occurred or an unrelated send succeeded. An organization-authorized operator can use `send_recovery_resolution` in the existing mailbox PATCH after investigating the current held task and reason: @@ -115,6 +119,8 @@ Hard bounce/complaint/suppression and authentication failures remain negative ev Organization export retains participation, ceilings, restrictive task/account evidence and repair history. Executor worker/nonce/start/result handles reset on import. Exact diagnostic authorization and nonces, including raw warmup receipt provenance, are instance-local and excluded; registered placement aggregates travel as history, never as execution grants. See [workspace moves](/guides/workspace-export-import/). +Per-task warmup charge and refund markers reset on organization import; imported statistics remain history, not destination-instance refund authority. Whole-instance database backups retain those markers. The additive charge-provenance migration leaves all historical markers NULL without backfill; apply it before starting the updated backend and result consumers. Rollback refuses to remove recorded charge or refund evidence; retain the additive schema and roll forward. + Immutable attempt timestamps and recipient counts travel with the sending group and retain restrictive rate evidence; their historical nonces do not authorize execution. Raw warmup receipt provenance remains source-instance-local and excluded, while registered placement aggregates travel. The source-instance reconciliation outbox is excluded from organization archives so an import cannot replay customer notifications. Drain pending effects before moving; whole-instance backups retain the queue. Delivered outbox rows expire after seven days; pending rows remain until delivery or organization/task deletion. They contain event/notification metadata, not raw MIME, provider responses or credentials. Repair history lasts with the organization and must not contain sensitive message content. diff --git a/docs/content/docs/development/warmblyctl.mdx b/docs/content/docs/development/warmblyctl.mdx index 083f681f3..c01f173a9 100644 --- a/docs/content/docs/development/warmblyctl.mdx +++ b/docs/content/docs/development/warmblyctl.mdx @@ -172,7 +172,7 @@ warmblyctl admin recovery restart-backfill --id --server laptop \ Substitute the same real canonical UUID in both places. Clearing a provider throttle can resume provider work; restarting backfill resets the existing sync backfill state. Neither action establishes sending authority. -Dead-letter replay is never part of diagnosis. Inspect the task, mailbox, provider receipts and uncertain-send protections first. Replay additionally requires a separate risk acknowledgement: +Dead-letter replay is never part of diagnosis. Only non-sending campaign wakeup passes can be replayed; send-capable dead letters remain pending until their outcome is independently reconciled through normal recovery. Inspect the task, mailbox, provider receipts and uncertain-send protections first. Replay additionally requires a separate risk acknowledgement: ```bash warmblyctl admin recovery replay --id --server laptop \ diff --git a/docs/content/docs/guides/mailboxes.mdx b/docs/content/docs/guides/mailboxes.mdx index 26545d32c..294bf51b3 100644 --- a/docs/content/docs/guides/mailboxes.mdx +++ b/docs/content/docs/guides/mailboxes.mdx @@ -280,6 +280,10 @@ Connecting a mailbox does two things: it imports the mailbox's recent history, a The Sync card distinguishes a completed history import from a recent successful live check. An unfinished pass, provider failure, recovery scan or budget hold does not refresh the last successful check or erase the last observed deferred count. That count is an observation from sync, not a measurement of all mail still on the provider. A worker heartbeat alone is not evidence that a mailbox is current. Repeated connection failures refresh the warning's observation time without generating a new notification each pass; no later successful check means recovery is unverified. +Sync respects an explicit provider retry window for temporary service failures as well as rate limits. On IMAP, an incomplete folder-status response or a temporary status failure keeps the existing folders and their sync positions for the next pass. Missing status metadata is requested separately, not treated as proof that a folder was deleted. + +The IMAP folder cap limits which folders are opened, not which folders exist. Folders outside the cap keep their saved sync positions. A capped pass cannot establish a rename or refresh the last successful live check. Folder retirement requires a complete name listing showing absence, an explicit provider not-found response, or your folder exclusion setting. On Outlook, an explicit retry window stops the remaining folder requests in that pass, even if an earlier folder failed without a retry window. + If Gmail rejects an ordinary import continuation token, the import restarts inside its original fixed date window. Already mapped mail is deduplicated, and the existing message cap and sync budgets still apply. Outlook recovers an expired delta checkpoint by replaying the configured recent-history window, reconciling known messages and following changes from a fresh checkpoint only after recovery completes. Older messages never imported are outside that recovery window. Authentication failures, throttles and transport errors are not treated as expired checkpoints. On IMAP, discovering a new folder or a changed UIDVALIDITY generation during or after the initial import schedules a bounded recent-history scan. Generation recovery reconciles known message handles and read state without restarting the global import count. In folders excluded from history imports, such as Junk and Trash, recovery updates only known messages and does not import unseen history. An unreadable or incomplete message body remains retryable rather than being stored as a successful empty message. A folder without usable modification sequences falls back to UIDNEXT checks and periodic flags even if the server advertises CONDSTORE. Changes are read with the standard CONDSTORE fetch. If a server answers that in a form Warmbly cannot read, the worker asks the other supported ways, and when none can be read it uses the same UIDNEXT checks. The mailbox keeps syncing either way. Failed flag publication keeps the previous baseline so it can retry. Background lookups do not mark unread messages as seen. @@ -454,6 +458,8 @@ The same tab decides where warmup mail lands **in your own mail client**: its ow Warmbly retries filing until the worker confirms success, including after a worker restart or reassignment. Reconnecting also files recognized warmup mail left behind while access was unavailable, using your current placement setting. Choosing **Inbox** deliberately leaves it in place. +On Outlook, a failed, ambiguous or incomplete lookup of a warmup message's stable identity stops deletion before removing the provider message or its stored body. Unavailable lookup is not proof that the message is missing. Each lookup uses one request. Missing or null message collections, invalid identities and responses containing a continuation return a temporary error without following the continuation. Only an explicit terminal empty collection proves absence. A completed empty lookup and a provider-confirmed missing item retain the existing idempotent cleanup behavior. This safeguard needs no schema change or backfill and does not change the existing command retry limit. If the lookup remains incomplete through that limit, the stored body remains in place rather than being deleted on uncertain evidence. + Keep warmup running after campaigns begin rather than switching it off once a mailbox looks ready. See [Warmup](/guides/warmup/). A mailbox marked as a [placement seed inbox](/guides/placement-tests/#your-own-seed-inboxes) is the exception: it never warms up (starting warmup on it is refused until it is unmarked) and never sends campaign mail, because a test inbox has to stay a stranger to your senders. diff --git a/docs/content/docs/guides/workspace-export-import.mdx b/docs/content/docs/guides/workspace-export-import.mdx index 316273817..0598d2cfc 100644 --- a/docs/content/docs/guides/workspace-export-import.mdx +++ b/docs/content/docs/guides/workspace-export-import.mdx @@ -57,6 +57,8 @@ A workspace archive is a single file holding everything one workspace owns. It e Diagnostic participation, shared/rolling limits, restrictive send evidence and audited repair-resolution history travel with their registered mailbox/task/organization data. Source-worker executor handles reset; exact provider-message DKIM authorization/nonces are excluded. Portable minimal receipt proof remains historical evidence, never an execution grant. The reconciliation webhook/notification outbox is source-instance-local and excluded to prevent replay on import; drain pending effects before moving. Whole-instance database backups retain it. See [send-safety retention and rollback](/development/send-safety/#upgrade-rollback-and-portability). +Campaign task replay intent resets to unverified on workspace import. The destination cannot replay an imported dead letter as a verified non-sending wakeup. A contact or sequence binding remains restrictive send intent even if that record is later removed. Whole-instance backups retain the durable intent; migration rollback refuses to discard retained send intent or unverified pending campaign dead letters. + This page moves **one workspace** between two running instances, re-sealing its secrets for the destination's keys. To move an entire self-hosted install (every workspace, its users, its platform admins) use `warmblyctl backup` and `warmblyctl restore`, which carry the database, the blob root and the encryption keys as one bundle. See [data control](/development/data-control/#backups). The two are not interchangeable: a bundle cannot be applied to a single workspace, and this archive cannot restore an instance. @@ -91,6 +93,8 @@ Inbox, send history, delivery events, and logs are the ones that grow without li The pending warmup-filing queue stays on its source instance. The destination resyncs mail and files recognized warmup messages using its own workers and the mailbox's placement setting. +Warmup statistics travel as history. Per-task charge and refund markers reset on import so settling an old task cannot refund another send's destination statistics. Whole-instance database backups retain these markers. + [Test workspace samples](/development/accounts-and-access/#adding-sample-data) move as ordinary contacts, campaigns and CRM records in their corresponding groups; reply templates are in **Assistant**. Their instance-wide admin audit marker does not travel, and import does not grant managed Test access. A restricted member stays restricted on the destination. Their campaign and folder grants travel with **Campaigns**, so an import without that group brings them in with no campaign access rather than more. diff --git a/internal/app/advanced/campaign_replay_test.go b/internal/app/advanced/campaign_replay_test.go index da8827668..f8525b321 100644 --- a/internal/app/advanced/campaign_replay_test.go +++ b/internal/app/advanced/campaign_replay_test.go @@ -19,6 +19,10 @@ type replayRepo struct { replayed int } +func (r *replayRepo) ListRetryableDeadLetters(context.Context, int) ([]models.TaskDeadLetter, error) { + return []models.TaskDeadLetter{*r.dlq}, nil +} + func (r *replayRepo) GetTaskDeadLetter(context.Context, uuid.UUID, uuid.UUID) (*models.TaskDeadLetter, error) { return r.dlq, nil } @@ -32,6 +36,10 @@ type replayTasks struct { repository.TaskRepository task *repository.Task campaignID *uuid.UUID + contactID *uuid.UUID + sequenceID *uuid.UUID + intent repository.CampaignDispatchIntent + replayErr error ctErr error created bool inserted int @@ -52,7 +60,46 @@ func (t *replayTasks) GetCampaignTask(context.Context, uuid.UUID) (*repository.C if t.ctErr != nil { return nil, t.ctErr } - return &repository.CampaignTask{TaskID: t.task.ID, CampaignID: t.campaignID}, nil + return &repository.CampaignTask{TaskID: t.task.ID, CampaignID: t.campaignID, ContactID: t.contactID, SequenceID: t.sequenceID, DispatchIntent: t.intent}, nil +} + +func TestDeadLetterNeverReplaysSendTasksWithoutAuthority(t *testing.T) { + for _, tc := range []struct { + name, dlqType, taskType string + contact, sequence bool + intent repository.CampaignDispatchIntent + }{ + {name: "legacy warmup send", dlqType: "warmup", taskType: "warmup"}, + {name: "user email send", dlqType: "user_email", taskType: "user_email"}, + {name: "campaign contact send", dlqType: "campaign", taskType: "campaign", contact: true}, + {name: "campaign sequence send", dlqType: "campaign", taskType: "campaign", sequence: true}, + {name: "mismatched dead letter", dlqType: "warmup", taskType: "campaign"}, + {name: "legacy null FKs", dlqType: "campaign", taskType: "campaign", intent: repository.CampaignDispatchUnverified}, + {name: "send after FK cleanup", dlqType: "campaign", taskType: "campaign", intent: repository.CampaignDispatchSend}, + } { + t.Run(tc.name, func(t *testing.T) { + id := uuid.New() + campaignID := uuid.New() + repo := &replayRepo{dlq: &models.TaskDeadLetter{ID: uuid.New(), TaskID: id, TaskType: tc.dlqType}} + tasks := &replayTasks{task: &repository.Task{ID: id, TaskType: tc.taskType, Status: "dead_lettered"}, campaignID: &campaignID, created: true, intent: tc.intent} + if tc.contact { + tasks.contactID = &id + } + if tc.sequence { + tasks.sequenceID = &id + } + svc := &service{repo: repo, taskRepo: tasks, tasksClient: replayClient{}} + if xerr := svc.ReplayDeadLetter(t.Context(), uuid.New(), repo.dlq.ID); xerr == nil { + t.Fatal("send dead letter was allowed through manual replay") + } + if n, xerr := svc.ProcessRetryableDeadLetters(t.Context()); xerr != nil || n != 0 { + t.Fatalf("auto replay result = %d, %v", n, xerr) + } + if tasks.inserted != 0 || tasks.repended != 0 || repo.replayed != 0 { + t.Fatalf("send mutated: inserted=%d pending=%d replayed=%d", tasks.inserted, tasks.repended, repo.replayed) + } + }) + } } func (t *replayTasks) CreateTaskWithLock(context.Context, *repository.Task, *repository.CampaignTask) (bool, error) { @@ -62,6 +109,16 @@ func (t *replayTasks) CreateTaskWithLock(context.Context, *repository.Task, *rep return t.created, nil } +func (t *replayTasks) CreateCampaignReplayTask(ctx context.Context, task *repository.Task, ct *repository.CampaignTask, source uuid.UUID) (bool, error) { + if source != t.task.ID { + return false, errors.New("wrong replay source") + } + if t.replayErr != nil { + return false, t.replayErr + } + return t.CreateTaskWithLock(ctx, task, ct) +} + func (t *replayTasks) UpdateTaskScheduledAt(context.Context, uuid.UUID, time.Time, string) error { return nil } @@ -89,6 +146,7 @@ func TestReplayDeadLetterForACampaignPass(t *testing.T) { for _, tc := range []struct { name string ctErr error + replayErr error created bool queueFails bool wantErr bool @@ -100,11 +158,12 @@ func TestReplayDeadLetterForACampaignPass(t *testing.T) { {name: "chain already has its next pass", created: false, wantReplayed: 1}, {name: "campaign task unreadable", ctErr: errors.New("connection reset"), wantErr: true}, {name: "queue refuses the new pass", created: true, queueFails: true, wantErr: true, wantInserted: 1, wantDeleted: 1}, + {name: "intent changed before lock", replayErr: repository.ErrCampaignReplayUnverified, wantErr: true}, } { t.Run(tc.name, func(t *testing.T) { - task := &repository.Task{ID: uuid.New(), TaskType: "campaign", EmailAccountID: uuid.New()} - repo := &replayRepo{dlq: &models.TaskDeadLetter{ID: uuid.New(), TaskID: task.ID}} - tasks := &replayTasks{task: task, campaignID: &campaignID, ctErr: tc.ctErr, created: tc.created} + task := &repository.Task{ID: uuid.New(), TaskType: "campaign", EmailAccountID: uuid.New(), Status: "dead_lettered"} + repo := &replayRepo{dlq: &models.TaskDeadLetter{ID: uuid.New(), TaskID: task.ID, TaskType: "campaign"}} + tasks := &replayTasks{task: task, campaignID: &campaignID, ctErr: tc.ctErr, replayErr: tc.replayErr, created: tc.created, intent: repository.CampaignDispatchWakeup} svc := &service{repo: repo, taskRepo: tasks, tasksClient: replayClient{fail: tc.queueFails}} xerr := svc.ReplayDeadLetter(context.Background(), uuid.New(), repo.dlq.ID) diff --git a/internal/app/advanced/service.go b/internal/app/advanced/service.go index fe830727b..fc3078151 100644 --- a/internal/app/advanced/service.go +++ b/internal/app/advanced/service.go @@ -2288,26 +2288,15 @@ func (s *service) ReplayDeadLetter(ctx context.Context, organizationID, deadLett if task == nil { return errx.ErrNotFound } - if _, handled, rerr := s.replayCampaignPass(ctx, task); handled { - if rerr != nil { - return toErrx(rerr) - } - if err := s.repo.MarkTaskDeadLetterReplayed(ctx, deadLetterID); err != nil { - return toErrx(err) - } - return nil + if dlq.TaskType != "campaign" || task.TaskType != "campaign" { + return errx.New(errx.Conflict, "send dead letters require authoritative outcome evidence and cannot be replayed") } - - scheduleAt := time.Now().UTC().Add(10 * time.Second) - cloudTaskName, err := s.tasksClient.CreateTask(ctx, &proto.ProcessTask{TaskId: task.ID.String()}, scheduleAt) - if err != nil { - return toErrx(err) + _, handled, rerr := s.replayCampaignPass(ctx, task) + if rerr != nil { + return toErrx(rerr) } - if err := s.taskRepo.UpdateTaskScheduledAt(ctx, task.ID, scheduleAt, cloudTaskName); err != nil { - return toErrx(err) - } - if err := s.taskRepo.UpdateTaskStatus(ctx, task.ID, "pending"); err != nil { - return toErrx(err) + if !handled { + return errx.New(errx.Conflict, "only non-sending campaign passes can be replayed") } if err := s.repo.MarkTaskDeadLetterReplayed(ctx, deadLetterID); err != nil { return toErrx(err) @@ -2322,22 +2311,27 @@ func (s *service) ReplayDeadLetter(ctx context.Context, organizationID, deadLett // campaign pass; replayed, that a new pass was queued. An error means nothing // was queued and the dead letter stays for another try. func (s *service) replayCampaignPass(ctx context.Context, task *repository.Task) (replayed, handled bool, err error) { - if task == nil || task.TaskType != "campaign" { + if task == nil || task.TaskType != "campaign" || task.Status != "dead_lettered" { return false, false, nil } ct, err := s.taskRepo.GetCampaignTask(ctx, task.ID) if err != nil { return false, true, err } - if ct == nil || ct.CampaignID == nil { - // The campaign is gone; there is nothing to replay into. - return false, true, nil + if ct == nil || ct.DispatchIntent != repository.CampaignDispatchWakeup || ct.ContactID != nil || ct.SequenceID != nil { + return false, false, nil + } + if ct.CampaignID == nil { + return false, false, nil } at := time.Now().UTC().Add(10 * time.Second) id := uuid.New() - created, err := s.taskRepo.CreateTaskWithLock(ctx, + created, err := s.taskRepo.CreateCampaignReplayTask(ctx, &repository.Task{ID: id, TaskType: "campaign", EmailAccountID: task.EmailAccountID, Status: "pending", ScheduledAt: &at}, - &repository.CampaignTask{TaskID: id, CampaignID: ct.CampaignID}) + &repository.CampaignTask{TaskID: id, CampaignID: ct.CampaignID}, task.ID) + if errors.Is(err, repository.ErrCampaignReplayUnverified) { + return false, false, nil + } if err != nil { return false, true, err } @@ -2680,10 +2674,14 @@ func (s *service) ProcessRetryableDeadLetters(ctx context.Context) (int, *errx.E retried := 0 for _, dlq := range items { + if dlq.TaskType != "campaign" { + continue + } task, err := s.taskRepo.GetTask(ctx, dlq.TaskID) - if err != nil || task == nil { - // Mark as exhausted if the task no longer exists - _ = s.repo.MarkTaskDeadLetterReplayed(ctx, dlq.ID) + if err != nil { + return retried, toErrx(err) + } + if task == nil || task.TaskType != "campaign" { continue } @@ -2700,30 +2698,6 @@ func (s *service) ProcessRetryableDeadLetters(ctx context.Context) (int, *errx.E _ = s.repo.MarkTaskDeadLetterReplayed(ctx, dlq.ID) continue } - - // Check if attempts exceeded - if dlq.Attempts >= dlq.MaxAttempts { - _ = s.repo.IncrementDeadLetterAttempt(ctx, dlq.ID, nil) - continue - } - - // Schedule retry via Cloud Tasks - scheduleAt := time.Now().UTC().Add(10 * time.Second) - cloudTaskName, err := s.tasksClient.CreateTask(ctx, &proto.ProcessTask{TaskId: task.ID.String()}, scheduleAt) - if err != nil { - // Compute next retry with exponential backoff - backoff := time.Duration(30*(1<'` + f.task.String() + `'::uuid OR status<>'resolved') NOT VALID`) + t.Cleanup(func() { + if _, err := h.Exec(context.Background(), `ALTER TABLE task_dead_letters DROP CONSTRAINT IF EXISTS `+constraint); err != nil { + t.Error(err) + } + }) + if err := s.TaskRepo.(repository.SendResultRecovery).ApplySendResult(t.Context(), result, func(context.Context) error { callbacks++; return nil }); err == nil { + t.Fatal("failed dead-letter write committed terminal repair") + } + var status, dlq string + if err := h.QueryRow(t.Context(), `SELECT t.status,d.status FROM tasks t JOIN task_dead_letters d ON d.task_id=t.id WHERE t.id=$1`, f.task).Scan(&status, &dlq); err != nil || status != "dead_lettered" || dlq != "pending" || callbacks != 0 { + t.Fatalf("partial terminal repair: status=%s dlq=%s callbacks=%d err=%v", status, dlq, callbacks, err) + } + exec(`ALTER TABLE task_dead_letters DROP CONSTRAINT ` + constraint) + } + for range 2 { + if err := s.TaskRepo.(repository.SendResultRecovery).ApplySendResult(t.Context(), result, func(context.Context) error { callbacks++; return nil }); err != nil { + t.Fatal(err) + } + } + var status, dlq string + var sent int + var unchanged, evidence bool + if err := h.QueryRow(t.Context(), `SELECT t.status,d.status,ws.emails_sent, + t.send_result_state='failed' AND t.send_result_applied_at=$2 AND t.send_released_at IS NOT DISTINCT FROM $3::timestamptz AND w.warmup_refunded_at IS NOT DISTINCT FROM $4::timestamptz, + d.payload='{"task_id":"preserved"}'::jsonb AND d.last_error='late dispatcher failure' AND d.attempts=2 AND d.replayed_at IS NULL AND e.send_recovery_hold AND e.send_recovery_reason='authentication' AND e.send_recovery_task_id=t.id + FROM tasks t LEFT JOIN warmup_tasks w ON w.task_id=t.id JOIN task_dead_letters d ON d.task_id=t.id JOIN email_accounts e ON e.id=t.email_account_id JOIN warmup_statistics ws ON ws.email_account_id=e.id AND ws.date=(NOW() AT TIME ZONE 'UTC')::date WHERE t.id=$1`, f.task, applied, released, refunded).Scan(&status, &dlq, &sent, &unchanged, &evidence); err != nil { + t.Fatal(err) + } + wantStatus, wantDLQ, wantSent := "dead_lettered", "pending", 3 + if proof == "refunded" { + wantStatus, wantDLQ = "failed", "resolved" + } + if proof == "refunded" || proof == "ambiguous" { + wantSent = 2 + } + if status != wantStatus || dlq != wantDLQ || sent != wantSent || callbacks != 0 || !unchanged || !evidence { + t.Fatalf("terminal repair status=%s dlq=%s sent=%d callbacks=%d unchanged=%v evidence=%v", status, dlq, sent, callbacks, unchanged, evidence) + } + }) + } + } +} diff --git a/internal/app/consumer/warmup_send_giveback_live_test.go b/internal/app/consumer/warmup_send_giveback_live_test.go index 1b300fb6f..2d271c72c 100644 --- a/internal/app/consumer/warmup_send_giveback_live_test.go +++ b/internal/app/consumer/warmup_send_giveback_live_test.go @@ -86,6 +86,7 @@ func TestLiveWarmupSendFailureGivesTheDayBack(t *testing.T) { taskID := uuid.New() exec(`INSERT INTO tasks (id, task_type, email_account_id, status, message_id, completed_at) VALUES ($1, 'warmup', $2, 'completed', '', $3)`, taskID, f.mailbox, day) + exec(`INSERT INTO warmup_tasks(task_id,warmup_charged_date,warmup_reply_charged) VALUES($1,($2::timestamptz AT TIME ZONE 'UTC')::date,true)`, taskID, day) exec(`INSERT INTO warmup_tokens (token, task_id, sender_account_id, recipient_account_id, conversation_turn) VALUES (gen_random_uuid(), $1, $2, $2, 1)`, taskID, f.mailbox) if err := s.WarmupRepo.IncrementDailyCount(ctx, f.mailbox, day); err != nil { @@ -98,7 +99,7 @@ func TestLiveWarmupSendFailureGivesTheDayBack(t *testing.T) { read := func() (sent, replied int) { t.Helper() if err := handle.Pool.QueryRow(ctx, `SELECT emails_sent, emails_replied FROM warmup_statistics - WHERE email_account_id = $1 AND date = DATE($2)`, f.mailbox, day).Scan(&sent, &replied); err != nil { + WHERE email_account_id = $1 AND date = ($2::timestamptz AT TIME ZONE 'UTC')::date`, f.mailbox, day).Scan(&sent, &replied); err != nil { t.Fatalf("read statistics: %v", err) } return sent, replied @@ -167,6 +168,9 @@ func TestLiveWarmupSendGiveBackIsIdempotent(t *testing.T) { VALUES ($1, 'warmup', $2, 'completed', '', $3)`, taskID, f.mailbox, day); err != nil { t.Fatalf("fixture: %v", err) } + if _, err := handle.Pool.Exec(ctx, `INSERT INTO warmup_tasks(task_id,warmup_charged_date,warmup_reply_charged) VALUES($1,($2::timestamptz AT TIME ZONE 'UTC')::date,false)`, taskID, day); err != nil { + t.Fatalf("fixture: %v", err) + } for i := 0; i < 3; i++ { if err := s.WarmupRepo.IncrementDailyCount(ctx, f.mailbox, day); err != nil { t.Fatalf("increment: %v", err) @@ -185,7 +189,7 @@ func TestLiveWarmupSendGiveBackIsIdempotent(t *testing.T) { var sent int if err := handle.Pool.QueryRow(ctx, `SELECT emails_sent FROM warmup_statistics - WHERE email_account_id = $1 AND date = DATE($2)`, f.mailbox, day).Scan(&sent); err != nil { + WHERE email_account_id = $1 AND date = ($2::timestamptz AT TIME ZONE 'UTC')::date`, f.mailbox, day).Scan(&sent); err != nil { t.Fatalf("read statistics: %v", err) } if sent != 2 { diff --git a/internal/app/orgtransfer/export_test.go b/internal/app/orgtransfer/export_test.go index 16dd91e41..7c5d636c8 100644 --- a/internal/app/orgtransfer/export_test.go +++ b/internal/app/orgtransfer/export_test.go @@ -2,9 +2,22 @@ package orgtransfer import ( "encoding/json" + "slices" "testing" ) +func TestCampaignReplayIntentResetsOnWorkspaceImport(t *testing.T) { + for _, tbl := range Tables { + if tbl.Name == "campaign_tasks" { + if !slices.Contains(tbl.ResetOnImport, "dispatch_intent") { + t.Fatal("workspace import grants source-instance replay intent") + } + return + } + } + t.Fatal("campaign tasks missing from portable data registry") +} + // An archive is a customer download. The risk verdict and the evidence behind // it are operator-facing, so they must not ride along in the manifest, and the // same list must refuse them on the way back in. diff --git a/internal/app/orgtransfer/spec.go b/internal/app/orgtransfer/spec.go index 424ab46d9..4c873c949 100644 --- a/internal/app/orgtransfer/spec.go +++ b/internal/app/orgtransfer/spec.go @@ -814,13 +814,14 @@ var Tables = []Table{ }, { Name: "campaign_tasks", Group: models.OrgDataGroupSending, - Scope: `task_id IN ` + orgTasks, + Scope: `task_id IN ` + orgTasks, + ResetOnImport: []string{"dispatch_intent"}, }, { Name: "warmup_tasks", Group: models.OrgDataGroupSending, PartnerRefs: []string{"target_account_id"}, Scope: `task_id IN ` + orgTasks, - ResetOnImport: []string{"parent_task_id", "parent_received_id", "parent_message_id", "lineage_version", "schedule_revision", "queued_revision", "dispatch_nonce", "dispatch_worker_id", "dispatch_started_at", "dispatch_result"}, + ResetOnImport: []string{"parent_task_id", "parent_received_id", "parent_message_id", "lineage_version", "schedule_revision", "queued_revision", "dispatch_nonce", "dispatch_worker_id", "dispatch_started_at", "dispatch_result", "warmup_charged_date", "warmup_reply_charged", "warmup_refunded_at"}, }, { Name: "warmup_tokens", Group: models.OrgDataGroupSending, diff --git a/internal/app/orgtransfer/warmup_charge_live_test.go b/internal/app/orgtransfer/warmup_charge_live_test.go new file mode 100644 index 000000000..5bd4a747a --- /dev/null +++ b/internal/app/orgtransfer/warmup_charge_live_test.go @@ -0,0 +1,42 @@ +package orgtransfer + +import ( + "context" + "testing" + "time" + + "github.com/google/uuid" + "github.com/warmbly/warmbly/internal/models" + "github.com/warmbly/warmbly/internal/repository" +) + +func TestLiveImportWarmupStatisticsCannotGrantChargeRefundAuthority(t *testing.T) { + svc, pool := liveImportService(t) + dest := newTenantFixture(t, pool) + ctx := t.Context() + mailbox, task := uuid.New(), uuid.New() + if _, err := pool.Exec(ctx, `INSERT INTO email_accounts(id,user_id,organization_id,email,name,signature_plain,signature_html,provider) VALUES($1,$2,$3,$4,'Imported','','','smtp_imap')`, mailbox, dest.owner, dest.org, "import-"+mailbox.String()[:8]+"@example.test"); err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _, _ = pool.Exec(context.Background(), `DELETE FROM email_accounts WHERE id=$1`, mailbox) }) + day := time.Now().UTC().Format("2006-01-02") + archive := tenantArchive(t, map[string][]map[string]any{ + "tasks": {{"id": task, "email_account_id": mailbox, "task_type": "warmup", "status": "completed", "message_id": "", "completed_at": time.Now().UTC()}}, + "warmup_tasks": {{"task_id": task, "warmup_charged_date": day, "warmup_reply_charged": true, "warmup_refunded_at": time.Now().UTC()}}, + "warmup_statistics": {{"email_account_id": mailbox, "date": day, "emails_sent": 3, "emails_replied": 2, "target_volume": 10}}, + }) + if _, err := svc.ImportFrom(ctx, dest.org, archive, ImportOptions{Conflict: models.OrgImportConflictOverwrite, ActorUserID: dest.owner}, nil); err != nil { + t.Fatal(err) + } + var cleared bool + if err := pool.QueryRow(ctx, `SELECT warmup_charged_date IS NULL AND warmup_reply_charged IS NULL AND warmup_refunded_at IS NULL FROM warmup_tasks WHERE task_id=$1`, task).Scan(&cleared); err != nil || !cleared { + t.Fatalf("import retained source charge authority: cleared=%v error=%v", cleared, err) + } + if err := repository.NewWarmupRepository(pool).FailWarmupSend(ctx, mailbox, task, time.Now(), "Send failed", "Imported historical result"); err != nil { + t.Fatal(err) + } + var sent, replies int + if err := pool.QueryRow(ctx, `SELECT emails_sent,emails_replied FROM warmup_statistics WHERE email_account_id=$1 AND date=$2::date`, mailbox, day).Scan(&sent, &replies); err != nil || sent != 3 || replies != 2 { + t.Fatalf("imported task refunded historical statistics: sent=%d replies=%d error=%v", sent, replies, err) + } +} diff --git a/internal/app/worker/event_warmup_action.go b/internal/app/worker/event_warmup_action.go index 60eda4ed2..129494518 100644 --- a/internal/app/worker/event_warmup_action.go +++ b/internal/app/worker/event_warmup_action.go @@ -207,7 +207,7 @@ func (w *WorkerService) runGraphWarmupActions(ctx context.Context, mail *wmail.W msgID := action.GmailID if action.RFCMessageID != "" { if resolved, err := client.ResolveMessageID(ctx, action.RFCMessageID); err != nil { - if hasWarmupAction(action.Actions, models.WarmupActionFile) { + if hasWarmupAction(action.Actions, models.WarmupActionFile) || hasWarmupAction(action.Actions, models.WarmupActionDelete) { return fmt.Errorf("locate warmup message (Graph): %w", err) } } else if resolved != "" { diff --git a/internal/app/worker/graph_lookup_completeness_test.go b/internal/app/worker/graph_lookup_completeness_test.go new file mode 100644 index 000000000..1e71a2046 --- /dev/null +++ b/internal/app/worker/graph_lookup_completeness_test.go @@ -0,0 +1,59 @@ +package worker + +import ( + "context" + "errors" + "io" + "net/http" + "strings" + "testing" + "time" + + "github.com/google/uuid" + "github.com/warmbly/warmbly/internal/app/worker/wmail" + "github.com/warmbly/warmbly/internal/client/msgraph" + "github.com/warmbly/warmbly/internal/errx" + "github.com/warmbly/warmbly/internal/models" + "golang.org/x/oauth2" +) + +func TestGraphWarmupDeleteIncompleteLookupRetainsBody(t *testing.T) { + for _, tc := range []struct{ name, body string }{ + {"missing collection", `{}`}, + {"terminal absence followed by identity", `{"value":[]}{"value":[{"id":"live-id"}]}`}, + {"null collection", `{"value":null}`}, + {"continuation not exhausted", `{"value":[],"@odata.nextLink":"https://graph.microsoft.com/v1.0/me/messages?$skiptoken=more"}`}, + {"foreign continuation", `{"value":[],"@odata.nextLink":"https://other.example.test/collect"}`}, + {"blank identity", `{"value":[{"id":" "}]}`}, + } { + t.Run(tc.name, func(t *testing.T) { + lookups, deletes := 0, 0 + transport := filingTransport(func(r *http.Request) (*http.Response, error) { + status, body := http.StatusOK, tc.body + if r.Method == http.MethodGet { + lookups++ + } else if r.Method == http.MethodDelete { + deletes++ + status, body = http.StatusNotFound, `{"error":{"code":"ErrorItemNotFound"}}` + } + return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: make(http.Header), Request: r}, nil + }) + ctx := context.WithValue(t.Context(), oauth2.HTTPClient, &http.Client{Transport: transport}) + client := &msgraph.Client{} + if err := client.Init(ctx, &oauth2.Token{AccessToken: "test", Expiry: time.Now().Add(time.Hour)}, oauth2.Config{}); err != nil { + t.Fatal(err) + } + account := uuid.New() + w, _ := filingWorker(account, &filingIMAP{}) + store := &warmupDeleteStore{} + w.mailManager.Emails[account] = &wmail.WMail{ID: account, Storage: store, GraphData: &wmail.GraphData{Client: client}} + action := models.WarmupEmailAction{EmailID: account, GmailID: "stale-id", InternalID: uuid.NewString(), RFCMessageID: "", Actions: []string{models.WarmupActionDelete}} + ctx = context.WithValue(ctx, deliveryKey{}, delivery{attempt: 1, redelivers: true}) + err := w.HandleWarmupAction(ctx, action) + var merr *errx.MailError + if !errors.As(err, &merr) || merr.Code != errx.MailErrorCodeServerUnreachable || merr.ResolveMethod != errx.MailErrorResolveMethodRetry || store.deletes != 0 || deletes != 0 || lookups != 1 { + t.Fatalf("incomplete lookup acknowledged or deleted: lookup=%d provider_delete=%d body_delete=%d err=%v", lookups, deletes, store.deletes, err) + } + }) + } +} diff --git a/internal/app/worker/warmup_filing_recovery_test.go b/internal/app/worker/warmup_filing_recovery_test.go index d07a4389a..c0eb64495 100644 --- a/internal/app/worker/warmup_filing_recovery_test.go +++ b/internal/app/worker/warmup_filing_recovery_test.go @@ -14,8 +14,10 @@ import ( "github.com/warmbly/warmbly/internal/app/worker/wmail" "github.com/warmbly/warmbly/internal/client/goog" "github.com/warmbly/warmbly/internal/client/msgraph" + "github.com/warmbly/warmbly/internal/errx" "github.com/warmbly/warmbly/internal/infrastructure/codec" "github.com/warmbly/warmbly/internal/infrastructure/eventbus" + "github.com/warmbly/warmbly/internal/infrastructure/storage" "github.com/warmbly/warmbly/internal/models" "golang.org/x/oauth2" ) @@ -135,6 +137,134 @@ type filingTransport func(*http.Request) (*http.Response, error) func (f filingTransport) RoundTrip(r *http.Request) (*http.Response, error) { return f(r) } +type warmupDeleteStore struct { + storage.Store + deletes int + err error +} + +func (s *warmupDeleteStore) Delete(context.Context, string) error { + s.deletes++ + return s.err +} + +func TestGraphWarmupDeleteHoldsOnUnconfirmedLookup(t *testing.T) { + for _, tc := range []struct { + name string + status int + body string + err error + }{ + {"503 retry window", 503, `{"error":{"code":"Unavailable"}}`, nil}, + {"429 retry window", 429, `{"error":{"code":"Throttled"}}`, nil}, + {"lookup resource unavailable", 404, `{"error":{"code":"ErrorItemNotFound"}}`, nil}, + {"deadline", 0, "", context.DeadlineExceeded}, + {"ambiguous identity", 200, `{"value":[{"id":""}]}`, nil}, + {"malformed response", 200, `{`, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + failed, deletes := true, 0 + transport := filingTransport(func(r *http.Request) (*http.Response, error) { + body, status := `{"value":[{"id":"resolved"}]}`, http.StatusOK + h := make(http.Header) + switch r.Method { + case http.MethodGet: + if !strings.HasSuffix(r.URL.Path, "/messages") { + t.Fatalf("unexpected lookup: %s", r.URL.Path) + } + if failed { + if tc.err != nil { + return nil, tc.err + } + body, status = tc.body, tc.status + h.Set("Retry-After", "600") + } + case http.MethodDelete: + deletes++ + if !failed && !strings.HasSuffix(r.URL.Path, "/messages/resolved") { + t.Fatalf("delete used stale identity: %s", r.URL.Path) + } + body, status = "", http.StatusNoContent + default: + t.Fatalf("unexpected request: %s", r.Method) + } + return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: h, Request: r}, nil + }) + ctx := context.WithValue(t.Context(), oauth2.HTTPClient, &http.Client{Transport: transport}) + client := &msgraph.Client{} + if err := client.Init(ctx, &oauth2.Token{AccessToken: "test", Expiry: time.Now().Add(time.Hour)}, oauth2.Config{}); err != nil { + t.Fatal(err) + } + account := uuid.New() + w, bus := filingWorker(account, &filingIMAP{}) + store := &warmupDeleteStore{} + w.mailManager.Emails[account] = &wmail.WMail{ID: account, Storage: store, GraphData: &wmail.GraphData{Client: client}} + action := models.WarmupEmailAction{EmailID: account, GmailID: "old-id", InternalID: uuid.NewString(), RFCMessageID: "", Actions: []string{models.WarmupActionDelete}} + ctx = context.WithValue(ctx, deliveryKey{}, delivery{attempt: 1, redelivers: true}) + err := w.HandleWarmupAction(ctx, action) + if err == nil || deletes != 0 || store.deletes != 0 || len(bus.events) != 0 { + t.Fatalf("unconfirmed lookup acknowledged or deleted: err=%v provider=%d bodies=%d events=%d", err, deletes, store.deletes, len(bus.events)) + } + if tc.status == 503 || tc.status == 429 { + var mailErr *errx.MailError + if !errors.As(err, &mailErr) || mailErr.RetryAfter != 10*time.Minute || mailErr.Failure == nil || mailErr.Failure.Status != tc.status { + t.Fatalf("provider evidence lost: %v", err) + } + } + failed = false + if err := w.HandleWarmupAction(ctx, action); err != nil || deletes != 1 || store.deletes != 1 { + t.Fatalf("confirmed retry did not clean up: err=%v provider=%d bodies=%d", err, deletes, store.deletes) + } + }) + } +} + +func TestGraphWarmupDeletePreservesAbsentAndLegacyCleanup(t *testing.T) { + for _, tc := range []struct { + name, stableID, providerID string + deletes int + }{ + {"confirmed absent with old provider key", "", "old-id", 1}, + {"confirmed absent without provider key", "", "", 0}, + {"legacy provider key", "", "old-id", 1}, + } { + t.Run(tc.name, func(t *testing.T) { + lookups, deletes := 0, 0 + transport := filingTransport(func(r *http.Request) (*http.Response, error) { + body, status := `{"value":[]}`, http.StatusOK + if r.Method == http.MethodGet { + lookups++ + } else if r.Method == http.MethodDelete { + deletes++ + body, status = `{"error":{"code":"ErrorItemNotFound"}}`, http.StatusNotFound + } + return &http.Response{StatusCode: status, Body: io.NopCloser(strings.NewReader(body)), Header: make(http.Header), Request: r}, nil + }) + ctx := context.WithValue(t.Context(), oauth2.HTTPClient, &http.Client{Transport: transport}) + client := &msgraph.Client{} + if err := client.Init(ctx, &oauth2.Token{AccessToken: "test", Expiry: time.Now().Add(time.Hour)}, oauth2.Config{}); err != nil { + t.Fatal(err) + } + account := uuid.New() + w, _ := filingWorker(account, &filingIMAP{}) + store := &warmupDeleteStore{err: errors.New("body store unavailable")} + w.mailManager.Emails[account] = &wmail.WMail{ID: account, Storage: store, GraphData: &wmail.GraphData{Client: client}} + action := models.WarmupEmailAction{EmailID: account, GmailID: tc.providerID, InternalID: uuid.NewString(), RFCMessageID: tc.stableID, Actions: []string{models.WarmupActionDelete}} + ctx = context.WithValue(ctx, deliveryKey{}, delivery{attempt: 1, redelivers: true}) + if err := w.HandleWarmupAction(ctx, action); !errors.Is(err, store.err) || store.deletes != 1 { + t.Fatalf("body-store error must remain retryable: err=%v bodies=%d", err, store.deletes) + } + store.err = nil + if err := w.HandleWarmupAction(ctx, action); err != nil || deletes != 2*tc.deletes || store.deletes != 2 { + t.Fatalf("absent/legacy cleanup changed: err=%v provider=%d bodies=%d", err, deletes, store.deletes) + } + if (lookups == 0) != (tc.stableID == "") { + t.Fatalf("legacy lookup behavior changed: %d", lookups) + } + }) + } +} + func TestDurableGmailFilingResolvesStableIDAndRetriesProviderFailure(t *testing.T) { failing, modifications := true, 0 transport := filingTransport(func(r *http.Request) (*http.Response, error) { diff --git a/internal/app/worker/wmail/arrival_durability_test.go b/internal/app/worker/wmail/arrival_durability_test.go index 764f55363..0f464b7ff 100644 --- a/internal/app/worker/wmail/arrival_durability_test.go +++ b/internal/app/worker/wmail/arrival_durability_test.go @@ -14,6 +14,7 @@ import ( goimap "github.com/emersion/go-imap/v2" "github.com/google/uuid" "github.com/warmbly/warmbly/internal/client/msgraph" + "github.com/warmbly/warmbly/internal/client/smtpimap/imap" "github.com/warmbly/warmbly/internal/models" "github.com/warmbly/warmbly/internal/repository" ) @@ -26,6 +27,141 @@ type durableTestMap struct { admitErr error } +type completedArrivalMap struct { + recoveryMessageMap + admissions int + lost bool + reads int + readErrAt int +} + +func (m *completedArrivalMap) Get(ctx context.Context, userID, emailID uuid.UUID, messageID string) (*repository.EmailMessageData, error) { + m.reads++ + if m.readErrAt == m.reads { + return nil, errors.New("canonical map lookup unavailable") + } + return m.recoveryMessageMap.Get(ctx, userID, emailID, messageID) +} + +func (m *completedArrivalMap) AdmitArrival(_ context.Context, data repository.EmailMessageData, _ *repository.PendingArrival) error { + m.admissions++ + if _, exists := m.data[data.MessageID]; exists { + return repository.ErrArrivalAdmissionUnconfirmed + } + m.data[data.MessageID] = data + if m.lost { + return errors.New("successful admission response lost after consumer drain") + } + return nil +} + +func TestCompletedArrivalLostResponseReofferUsesKnownMapAcrossProviders(t *testing.T) { + for _, provider := range []string{"google", "graph", "imap"} { + t.Run(provider, func(t *testing.T) { + var events []captured + var w *WMail + var offer func() (bool, error) + key := "provider-id" + switch provider { + case "google": + w = newGoogleTestMail(t, (&fakeGmail{}).serve(t), &events) + offer = func() (bool, error) { return w.onGoogleMessageAdded(t.Context(), key, "thread") } + case "graph": + var relayed []models.SyncState + w = newGraphTestMail(t, newFakeGraph().serve(t), &events, &relayed) + offer = func() (bool, error) { return w.onGraphMessageSeen(t.Context(), msgraph.FolderInbox, key, false) } + case "imap": + key = "" + var captured *[]captured + w, captured = newIMAPTestMail(&fakeImapConn{}, &fixedBudget{allow: 10}, &models.Mailbox{Name: "INBOX", UIDValidity: 1}) + offer = func() (bool, error) { + ok, err := w.imapApply(t.Context(), []*imap.Fetched{{Email: &models.EmailMessageData{MessageID: key, UID: 1}}}, false, &tickStats{}, nil) + events = *captured + if err != nil { + return ok, err + } + return ok, nil + } + } + m := &completedArrivalMap{recoveryMessageMap: recoveryMessageMap{data: map[string]repository.EmailMessageData{}}, lost: true} + w.EmailMessageMapRepository = m + storage := &retainedArrivalStore{body: map[string][]byte{}} + w.Storage = storage + if ok, err := offer(); ok || (provider != "imap" && err == nil) || m.admissions != 1 || len(m.data) != 1 { + t.Fatalf("lost response did not retain canonical admission: ok=%t err=%v calls=%d maps=%d", ok, err, m.admissions, len(m.data)) + } + canonical := m.data[key].ID + w.googleTick, w.graphTick = nil, nil + if ok, err := offer(); !ok || err != nil { + t.Fatalf("completed admission was retried instead of known-map path: ok=%t err=%v", ok, err) + } + if m.admissions != 1 || m.data[key].ID != canonical || len(storage.body) != 1 || storage.deletes != 0 || len(newEmails(events)) != 0 { + t.Fatal("known reoffer rewrote admission identity/body or published another arrival") + } + }) + } +} + +func TestIMAPBatchDuplicateCompletedArrivalUsesCanonicalMap(t *testing.T) { + for _, backfill := range []bool{false, true} { + t.Run(fmt.Sprintf("backfill=%t", backfill), func(t *testing.T) { + budget := &fixedBudget{allow: 10} + w, events := newIMAPTestMail(&fakeImapConn{}, budget, &models.Mailbox{Name: "INBOX", UIDValidity: 1}) + m := &completedArrivalMap{recoveryMessageMap: recoveryMessageMap{data: map[string]repository.EmailMessageData{}}} + w.EmailMessageMapRepository = m + storage := &retainedArrivalStore{body: map[string][]byte{}} + w.Storage = storage + key := "" + stats := &tickStats{} + ok, err := w.imapApply(t.Context(), []*imap.Fetched{ + {Email: &models.EmailMessageData{MessageID: key, UID: 1, ModSeq: 3, Flags: []string{"\\Seen"}}}, + {Email: &models.EmailMessageData{MessageID: key, UID: 2}}, + }, backfill, stats, nil) + if !ok || err != nil { + t.Fatalf("same-batch duplicate retried completed admission: ok=%t err=%v calls=%d", ok, err, m.admissions) + } + if m.admissions != 1 || len(m.data) != 1 || len(storage.body) != 1 || storage.deletes != 0 || budget.admitted != 1 { + t.Fatalf("duplicate re-admitted, re-budgeted or lost body: calls=%d maps=%d bodies=%d budget=%d", m.admissions, len(m.data), len(storage.body), budget.admitted) + } + updates := 0 + for _, e := range *events { + if e.eventType == models.JobEventTypeEmailUpdate { + updates++ + u := e.body.(*models.JobEventEmailUpdate) + if u.ID.String() != m.data[key].ID || u.UID != 1 || u.ModSeq != 3 || len(u.Flags) != 1 || u.Flags[0] != "\\Seen" { + t.Fatal("duplicate metadata lost canonical identity, UID or flags") + } + } + } + if len(newEmails(*events)) != 0 || (!backfill && (updates != 1 || stats.seen != 1)) || (backfill && (updates != 0 || stats.seen != 0)) { + t.Fatal("duplicate published another arrival, skipped reconciliation or inflated flood observation") + } + }) + } +} + +func TestIMAPBatchDuplicateCanonicalLookupFailureHoldsPass(t *testing.T) { + for _, backfill := range []bool{false, true} { + t.Run(fmt.Sprintf("backfill=%t", backfill), func(t *testing.T) { + budget := &fixedBudget{allow: 10} + w, events := newIMAPTestMail(&fakeImapConn{}, budget, &models.Mailbox{Name: "INBOX", UIDValidity: 1}) + m := &completedArrivalMap{recoveryMessageMap: recoveryMessageMap{data: map[string]repository.EmailMessageData{}}, readErrAt: 3} + w.EmailMessageMapRepository = m + storage := &retainedArrivalStore{body: map[string][]byte{}} + w.Storage = storage + stats := &tickStats{} + key := "" + ok, err := w.imapApply(t.Context(), []*imap.Fetched{ + {Email: &models.EmailMessageData{MessageID: key, UID: 1}}, + {Email: &models.EmailMessageData{MessageID: key, UID: 2}}, + }, backfill, stats, nil) + if ok || err != nil || !stats.aborted || m.admissions != 1 || len(storage.body) != 1 || storage.deletes != 0 || budget.admitted != 1 || len(newEmails(*events)) != 0 { + t.Fatal("failed canonical lookup re-admitted duplicate, lost body or completed the pass") + } + }) + } +} + func (m *durableTestMap) AdmitArrival(ctx context.Context, data repository.EmailMessageData, p *repository.PendingArrival) error { if m.admitErr != nil { return m.admitErr diff --git a/internal/app/worker/wmail/governor_test.go b/internal/app/worker/wmail/governor_test.go index 56cc4bcfc..623667ceb 100644 --- a/internal/app/worker/wmail/governor_test.go +++ b/internal/app/worker/wmail/governor_test.go @@ -188,14 +188,17 @@ func TestNextSyncDelay(t *testing.T) { } func TestNextSyncDelayDoesNotUndercutProviderRetryAfter(t *testing.T) { - w := &WMail{} - retryAfter := 12 * time.Minute - mailErr := errx.MError(errx.MailErrorWarning, errx.MailErrorCodeSendingTooFast, "throttled", errx.MailErrorResolveMethodRetry) - mailErr.RetryAfter = retryAfter - - for i := 0; i < 50; i++ { - if got := w.nextSyncDelay(time.Minute, mailErr); got < retryAfter { - t.Fatalf("delay %v retried before provider's %v window", got, retryAfter) - } + for _, code := range []errx.MailErrorCode{errx.MailErrorCodeSendingTooFast, errx.MailErrorCodeServerUnreachable, errx.MailErrorCodeConnectionLost} { + t.Run(string(code), func(t *testing.T) { + w := &WMail{transportFailures: 1} + retryAfter := 12 * time.Minute + mailErr := errx.MError(errx.MailErrorWarning, code, "retry later", errx.MailErrorResolveMethodRetry) + mailErr.RetryAfter = retryAfter + for i := 0; i < 50; i++ { + if got := w.nextSyncDelay(time.Minute, mailErr); got < retryAfter { + t.Fatalf("delay %v retried before provider's %v window", got, retryAfter) + } + } + }) } } diff --git a/internal/app/worker/wmail/imap_conn.go b/internal/app/worker/wmail/imap_conn.go index 3e36c4a24..32a4af02a 100644 --- a/internal/app/worker/wmail/imap_conn.go +++ b/internal/app/worker/wmail/imap_conn.go @@ -7,7 +7,6 @@ import ( goimap "github.com/emersion/go-imap/v2" "github.com/warmbly/warmbly/internal/client/smtpimap/imap" "github.com/warmbly/warmbly/internal/errx" - "github.com/warmbly/warmbly/internal/models" ) // ImapConn is everything the worker drives on one IMAP connection. @@ -16,7 +15,7 @@ import ( // holds an *imap.Client. type ImapConn interface { // Sync pass. - Folders() ([]models.Mailbox, *errx.MailError) + ListFolders() (imap.FolderListing, *errx.MailError) // FolderOverflow is how many folders the last listing left out for the // cap, FolderConflicts how many it left out for a duplicate UIDVALIDITY. FolderOverflow() int diff --git a/internal/app/worker/wmail/provider_presence_deadline_test.go b/internal/app/worker/wmail/provider_presence_deadline_test.go new file mode 100644 index 000000000..cd4846585 --- /dev/null +++ b/internal/app/worker/wmail/provider_presence_deadline_test.go @@ -0,0 +1,170 @@ +package wmail + +import ( + "net/http" + "net/http/httptest" + "sync/atomic" + "testing" + "time" + + "github.com/warmbly/warmbly/internal/errx" + "github.com/warmbly/warmbly/internal/models" +) + +func TestSyncFolderCapDoesNotAuthorizeRename(t *testing.T) { + inbox := models.Mailbox{Name: "INBOX", UIDValidity: 7, UIDNext: 101, HighestModSeq: 100} + conn := &fakeImapConn{folders: []models.Mailbox{inbox, {Name: "NewlyKeptFolder", UIDValidity: 42, UIDNext: 101, HighestModSeq: 100}}, overflow: 1, present: []string{"StillOnServerButOutsideCap"}} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, &inbox) + w.SmtpImapData.Mailboxes = append(w.SmtpImapData.Mailboxes, &models.Mailbox{Name: "StillOnServerButOutsideCap", UIDValidity: 42, UIDNext: 101, HighestModSeq: 100}) + if err := w.Sync(t.Context()); err != nil { + t.Fatal(err) + } + for _, event := range *events { + if event.eventType == models.JobEventTypeMailboxRename { + t.Fatalf("bounded listing inferred false rename: %+v", event.body) + } + } +} + +func TestSyncGraphRetryAfterStopsInflightRequests(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(out http.ResponseWriter, _ *http.Request) { + requests.Add(1) + out.Header().Set("Retry-After", "720") + out.Header().Set("Content-Type", "application/json") + out.WriteHeader(http.StatusServiceUnavailable) + _, _ = out.Write([]byte(`{"error":{"code":"ServiceUnavailable"}}`)) + })) + defer srv.Close() + var events []captured + var relayed []models.SyncState + w := newGraphTestMail(t, srv, &events, &relayed) + previous := seedSyncTruth(w, &relayed) + err := w.syncOnce(t.Context()) + if err == nil || err.RetryAfter != 12*time.Minute || requests.Load() != 1 || err.Failure == nil || err.Failure.RetryAt == nil { + t.Fatalf("early hidden retry or lost deadline: requests=%d err=%+v", requests.Load(), err) + } + if time.Now().Add(w.nextSyncDelay(time.Minute, err)).Before(*err.Failure.RetryAt) { + t.Fatal("next pass undercuts provider deadline") + } + assertNoSyncSuccess(t, w, previous) +} + +func TestSyncGraphLaterFolderRetryAfterOverridesEarlierError(t *testing.T) { + var requests atomic.Int32 + srv := httptest.NewServer(http.HandlerFunc(func(out http.ResponseWriter, _ *http.Request) { + if requests.Add(1) == 2 { + out.Header().Set("Retry-After", "720") + } + out.Header().Set("Content-Type", "application/json") + out.WriteHeader(http.StatusServiceUnavailable) + _, _ = out.Write([]byte(`{"error":{"code":"ServiceUnavailable"}}`)) + })) + defer srv.Close() + var events []captured + var relayed []models.SyncState + w := newGraphTestMail(t, srv, &events, &relayed) + previous := seedSyncTruth(w, &relayed) + err := w.syncOnce(t.Context()) + if err == nil || err.RetryAfter < 12*time.Minute || requests.Load() != 2 || err.Failure == nil || err.Failure.RetryAt == nil { + t.Fatalf("later provider deadline discarded: requests=%d err=%+v delay=%s", requests.Load(), err, w.nextSyncDelay(time.Minute, err)) + } + if time.Now().Add(w.nextSyncDelay(time.Minute, err)).Before(*err.Failure.RetryAt) { + t.Fatal("later deadline was undercut by backoff") + } + assertNoSyncSuccess(t, w, previous) +} + +func TestSyncFolderCapPreservesUnselectedStateWithoutClaimingRecovery(t *testing.T) { + inbox := models.Mailbox{Name: "INBOX", UIDValidity: 7, UIDNext: 101, HighestModSeq: 100} + conn := &fakeImapConn{folders: []models.Mailbox{inbox}, overflow: 1, present: []string{"StillOnServerButOutsideCap"}} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, &inbox) + w.SmtpImapData.Mailboxes = append(w.SmtpImapData.Mailboxes, &models.Mailbox{Name: "StillOnServerButOutsideCap", UIDValidity: 42, UIDNext: 101, HighestModSeq: 100}) + var relayed []models.SyncState + previous := seedSyncTruth(w, &relayed) + w.transportFailures = 2 + w.EmailType = models.InboxProviderSMTPIMAP + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + if gone := mailboxDeletes(*events); len(gone) != 0 { + t.Fatalf("bounded listing overflow=%d retired a folder without absence evidence: %v", conn.overflow, gone) + } + assertNoSyncSuccess(t, w, previous) + if w.transportFailures != 2 || len(w.SmtpImapData.Mailboxes) != 2 { + t.Fatal("capped pass erased saved folder or transport warning") + } + conn.overflow = 0 + conn.folders = append(conn.folders, *w.SmtpImapData.Mailboxes[1]) + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + assertSyncCatchUp(t, w, previous) + if w.transportFailures != 0 { + t.Fatal("complete uncapped pass did not confirm recovery") + } +} + +func TestSyncFolderCapRetiresOnlyCompleteAbsenceOrExplicitExclusion(t *testing.T) { + for _, excluded := range []bool{false, true} { + t.Run(map[bool]string{false: "absent", true: "excluded"}[excluded], func(t *testing.T) { + inbox := models.Mailbox{Name: "INBOX", UIDValidity: 7, UIDNext: 101, HighestModSeq: 100} + conn := &fakeImapConn{folders: []models.Mailbox{inbox}, overflow: 1, present: []string{"RetainedOutsideCap"}} + budget := &skipBudget{fixedBudget: &fixedBudget{allow: 10}} + if excluded { + budget.skip = []string{"Retired"} + conn.present = append(conn.present, "Retired") + } + w, events := newIMAPTestMail(conn, budget, &inbox) + w.SmtpImapData.Mailboxes = append(w.SmtpImapData.Mailboxes, + &models.Mailbox{Name: "Retired", UIDValidity: 42, UIDNext: 101, HighestModSeq: 100}, + &models.Mailbox{Name: "RetainedOutsideCap", UIDValidity: 43, UIDNext: 101, HighestModSeq: 100}) + if err := w.Sync(t.Context()); err != nil { + t.Fatal(err) + } + deletes := mailboxEvents(*events, models.JobEventTypeMailboxDelete) + if len(deletes) != 1 { + t.Fatalf("expected one positively authorized retirement: %+v", deletes) + } + retired := deletes[0].body.(*models.JobEventMailboxDelete) + if retired.Mailbox != "Retired" || retired.Skipped != excluded || len(w.SmtpImapData.Mailboxes) != 2 { + t.Fatalf("wrong cap retirement: %+v", retired) + } + }) + } +} + +func TestNextSyncDelayPreservesRetryAfterAcrossBackoff(t *testing.T) { + for _, code := range []errx.MailErrorCode{errx.MailErrorCodeSendingTooFast, errx.MailErrorCodeServerUnreachable, errx.MailErrorCodeConnectionLost} { + for _, deadline := range []time.Duration{30 * time.Second, 90 * time.Second, 12 * time.Minute, time.Hour} { + for _, failures := range []int{0, 1, 3, 9} { + w := &WMail{transportFailures: failures} + mailErr := errx.MError(errx.MailErrorWarning, code, "fixture", errx.MailErrorResolveMethodRetry) + mailErr.RetryAfter = deadline + for range 500 { + if got := w.nextSyncDelay(time.Minute, mailErr); got < deadline { + t.Fatalf("code=%s failures=%d deadline=%s delay=%s", code, failures, deadline, got) + } + } + } + } + } +} + +func TestSyncPresentFolderWithoutStatusRetainsRecoveryEvidence(t *testing.T) { + inbox := models.Mailbox{Name: "INBOX", UIDValidity: 7, UIDNext: 101, HighestModSeq: 100} + conn := &fakeImapConn{folders: []models.Mailbox{inbox}, present: []string{"Unselectable"}} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 10}, &inbox) + w.SmtpImapData.Mailboxes = append(w.SmtpImapData.Mailboxes, &models.Mailbox{Name: "Unselectable", UIDValidity: 42, UIDNext: 101, HighestModSeq: 100}) + var relayed []models.SyncState + previous := seedSyncTruth(w, &relayed) + w.EmailType = models.InboxProviderSMTPIMAP + w.transportFailures = 2 + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + assertNoSyncSuccess(t, w, previous) + if len(mailboxDeletes(*events)) != 0 || w.transportFailures != 2 { + t.Fatal("presence without usable status retired state or cleared warning") + } +} diff --git a/internal/app/worker/wmail/start_sync.go b/internal/app/worker/wmail/start_sync.go index 2db54e559..394aedc5a 100644 --- a/internal/app/worker/wmail/start_sync.go +++ b/internal/app/worker/wmail/start_sync.go @@ -46,13 +46,12 @@ func (w *WMail) nextSyncDelay(base time.Duration, last *errx.MailError) time.Dur d := base minimum := base / 2 switch { + case last != nil && last.RetryAfter > 0 && (isTransportError(last) || last.Code == errx.MailErrorCodeSendingTooFast): + // Service-unavailable responses can supply the same retry window as 429. + d = max(last.RetryAfter, base) + minimum = d case last != nil && last.Code == errx.MailErrorCodeSendingTooFast: - // The provider returned 429. Respect its window when supplied. d = syncBackoffMax - if last.RetryAfter > 0 { - d = max(last.RetryAfter, base) - minimum = d - } case last != nil && isTransportError(last): // The server is unreachable. Retry soon after the first failure (a // dropped session reconnects on the next pass and costs one dial), diff --git a/internal/app/worker/wmail/sync_imap.go b/internal/app/worker/wmail/sync_imap.go index 49c727a38..b0f611041 100644 --- a/internal/app/worker/wmail/sync_imap.go +++ b/internal/app/worker/wmail/sync_imap.go @@ -43,12 +43,16 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { // A mailbox left selected by the previous pass freezes LIST-STATUS on this // connection, so release it before asking what changed. client.ReleaseMailbox() - folders, err := client.Folders() + listing, err := client.ListFolders() if err != nil { return err } + if listing.Present == nil { + return errx.ErrMailServerUnreachable + } + folders := listing.Folders w.reportFolderOverflow() - // Folders() already drops Gmail's label views. Dropping them here too + // ListFolders already drops Gmail's label views. Dropping them here too // costs nothing and keeps the pass correct against any listing: a view // that reached it would re-file known mail as archive under a second UID. folders = slices.DeleteFunc(folders, func(b models.Mailbox) bool { return imapVirtualFolder(&b) }) @@ -59,7 +63,8 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { // baselined. They are kept aside so mail that moves into one of them can // be recognised as filed rather than lost. var skipped []models.Mailbox - if skip := w.skipFolders(); len(skip) > 0 { + skip := w.skipFolders() + if len(skip) > 0 { folders = slices.DeleteFunc(folders, func(b models.Mailbox) bool { if !imap.SkipsFolder(b, skip) { return false @@ -76,12 +81,19 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { // changed. A rename read as a delete plus a first sighting would orphan // every message filed under the old name and re-import the folder's // history under the new one. - if err := w.imapFollowRenames(folders); err != nil { - return nil + selectionComplete := client.FolderOverflow() == 0 && client.FolderConflicts() == 0 + if selectionComplete { + if err := w.imapFollowRenames(folders, listing.Present); err != nil { + return nil + } } // CONDSTORE is effective per folder; NOMODSEQ uses UIDNEXT and periodic flags. - caughtUp := true + caughtUp := selectionComplete + selectedNames := make(map[string]struct{}, len(folders)) + for _, box := range folders { + selectedNames[box.Name] = struct{}{} + } for i := range folders { box := &folders[i] @@ -212,17 +224,15 @@ func (w *WMail) Sync(ctx context.Context) *errx.MailError { } } - // Collect deletions first to avoid modifying the slice during iteration. - // Renames were already followed above, so a name missing from the listing - // at this point really is a folder that is gone. + // Only complete LIST presence or an explicit exclusion can retire state. var deleted []string var gone []*models.Mailbox -outer: for _, box := range w.SmtpImapData.Mailboxes { - for _, f := range folders { - if box.Name == f.Name { - continue outer + if _, present := listing.Present[box.Name]; present && !imapVirtualFolder(box) && !imap.SkipsFolder(*box, skip) { + if _, selected := selectedNames[box.Name]; !selected { + caughtUp = false } + continue } gone = append(gone, box) } @@ -234,7 +244,7 @@ outer: UIDValidity: box.UIDValidity, // A folder that is still on the server but now excluded takes // the mail already stored from it along. - Skipped: imapRetiredIntoSkipped(box, gone, skipped), + Skipped: imap.SkipsFolder(*box, skip) || imapRetiredIntoSkipped(box, gone, skipped), }); err != nil { return nil } @@ -683,17 +693,7 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill if perr != nil { continue } - if err := w.onEvent(models.JobEventTypeEmailUpdate, &models.JobEventEmailUpdate{ - UserID: w.UserID, - EmailID: w.ID, - ID: internalID, - UID: f.Email.UID, - ModSeq: f.Email.ModSeq, - Mailbox: w.SmtpImapData.mailbox, - FolderPath: w.SmtpImapData.folderPath, - Folder: w.SmtpImapData.folder, - Flags: f.Email.Flags, - }); err != nil { + if err := w.imapUpdateKnown(f, internalID); err != nil { return false, w.controlPlaneError(err, stats) } if recovery != nil { @@ -703,8 +703,12 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill if !backfill && len(fresh) > 0 { ids := make([]string, 0, len(fresh)) + seen := make(map[string]bool, len(fresh)) for _, f := range fresh { - ids = append(ids, f.Email.MessageID) + if !seen[f.Email.MessageID] { + ids = append(ids, f.Email.MessageID) + seen[f.Email.MessageID] = true + } } if w.observeLive(ctx, ids, stats) { return false, nil @@ -712,7 +716,25 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill } all := true + stored := make(map[string]bool, len(fresh)) for _, f := range fresh { + // Earlier copies in this batch can already be delivered before this offer. + if stored[f.Email.MessageID] { + internal, err := w.EmailMessageMapRepository.Get(ctx, w.UserID, w.ID, f.Email.MessageID) + if err != nil { + return false, w.controlPlaneError(err, stats) + } + if internal != nil { + if !backfill { + if id, err := uuid.Parse(internal.ID); err == nil { + if err := w.imapUpdateKnown(f, id); err != nil { + return false, w.controlPlaneError(err, stats) + } + } + } + continue + } + } done, err := w.imapStoreFetched(ctx, f, backfill, stats, recovery) if err != nil { return false, err @@ -724,6 +746,7 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill } continue } + stored[f.Email.MessageID] = true if backfill { w.imapImportCursor(w.SmtpImapData.folderPath, f.Email.UID, false, recovery) } @@ -731,6 +754,20 @@ func (w *WMail) imapApply(ctx context.Context, fetched []*imap.Fetched, backfill return all, nil } +func (w *WMail) imapUpdateKnown(f *imap.Fetched, id uuid.UUID) error { + return w.onEvent(models.JobEventTypeEmailUpdate, &models.JobEventEmailUpdate{ + UserID: w.UserID, + EmailID: w.ID, + ID: id, + UID: f.Email.UID, + ModSeq: f.Email.ModSeq, + Mailbox: w.SmtpImapData.mailbox, + FolderPath: w.SmtpImapData.folderPath, + Folder: w.SmtpImapData.folder, + Flags: f.Email.Flags, + }) +} + func (w *WMail) imapStoreFetched(ctx context.Context, f *imap.Fetched, backfill bool, stats *tickStats, recovery *imapFolderRecovery) (bool, *errx.MailError) { if stats.aborted || ctx.Err() != nil { return false, nil @@ -1043,17 +1080,12 @@ func (w *WMail) setWalking(box *models.Mailbox) { // go missing while one arrives, and picking either would move the wrong // folder's mail into it. Those fall through to the ordinary delete and // first-sight paths, which lose nothing that was not already gone. -func (w *WMail) imapFollowRenames(folders []models.Mailbox) error { - listed := make(map[string]struct{}, len(folders)) - for i := range folders { - listed[folders[i].Name] = struct{}{} - } - +func (w *WMail) imapFollowRenames(folders []models.Mailbox, present map[string]struct{}) error { // Group both sides by UIDVALIDITY: the stored folders that are no longer // listed, and the listed folders that are not stored. gone := map[uint32][]*models.Mailbox{} for _, before := range w.SmtpImapData.Mailboxes { - if _, still := listed[before.Name]; still || before.UIDValidity == 0 { + if _, still := present[before.Name]; still || before.UIDValidity == 0 { continue } gone[before.UIDValidity] = append(gone[before.UIDValidity], before) diff --git a/internal/app/worker/wmail/sync_imap_test.go b/internal/app/worker/wmail/sync_imap_test.go index 5cc493ee7..b31774e3a 100644 --- a/internal/app/worker/wmail/sync_imap_test.go +++ b/internal/app/worker/wmail/sync_imap_test.go @@ -21,6 +21,8 @@ import ( type fakeImapConn struct { ImapConn folders []models.Mailbox + present []string + folderErr *errx.MailError changed []goimap.UID fetches int released int @@ -54,9 +56,27 @@ type fakeImapConn struct { // in place, and a fake that shared its backing array would lose folders // between passes. func (c *fakeImapConn) Folders() ([]models.Mailbox, *errx.MailError) { + if c.folderErr != nil { + return nil, c.folderErr + } return append([]models.Mailbox(nil), c.folders...), nil } +func (c *fakeImapConn) ListFolders() (imap.FolderListing, *errx.MailError) { + folders, err := c.Folders() + if err != nil { + return imap.FolderListing{}, err + } + present := map[string]struct{}{} + for _, f := range folders { + present[f.Name] = struct{}{} + } + for _, name := range c.present { + present[name] = struct{}{} + } + return imap.FolderListing{Folders: folders, Present: present}, nil +} + func (c *fakeImapConn) FolderOverflow() int { return c.overflow } func (c *fakeImapConn) FolderConflicts() int { return c.conflicts } @@ -351,10 +371,17 @@ type backfillImapConn struct { } func (c *backfillImapConn) Folders() ([]models.Mailbox, *errx.MailError) { return c.folders, nil } -func (c *backfillImapConn) FolderOverflow() int { return 0 } -func (c *backfillImapConn) FolderConflicts() int { return 0 } -func (c *backfillImapConn) HasCondStore() bool { return true } -func (c *backfillImapConn) ReleaseMailbox() {} +func (c *backfillImapConn) ListFolders() (imap.FolderListing, *errx.MailError) { + present := map[string]struct{}{} + for _, f := range c.folders { + present[f.Name] = struct{}{} + } + return imap.FolderListing{Folders: c.folders, Present: present}, nil +} +func (c *backfillImapConn) FolderOverflow() int { return 0 } +func (c *backfillImapConn) FolderConflicts() int { return 0 } +func (c *backfillImapConn) HasCondStore() bool { return true } +func (c *backfillImapConn) ReleaseMailbox() {} func (c *backfillImapConn) SelectForSyncState(folder string) (imap.Selected, *errx.MailError) { count, err := c.SelectForSync(folder) diff --git a/internal/app/worker/wmail/sync_truth_test.go b/internal/app/worker/wmail/sync_truth_test.go index 306f2fe48..500b8751f 100644 --- a/internal/app/worker/wmail/sync_truth_test.go +++ b/internal/app/worker/wmail/sync_truth_test.go @@ -6,6 +6,7 @@ import ( "fmt" "net/http" "net/http/httptest" + "sync/atomic" "testing" "time" @@ -63,6 +64,81 @@ func assertSyncCatchUp(t *testing.T, w *WMail, previous time.Time) { } } +func TestSyncTruthGraphServiceUnavailableHonorsRetryWindowAndRecovers(t *testing.T) { + var recovered atomic.Bool + srv := httptest.NewServer(http.HandlerFunc(func(out http.ResponseWriter, req *http.Request) { + out.Header().Set("Content-Type", "application/json") + if !recovered.Load() { + out.Header().Set("Retry-After", "720") + out.WriteHeader(http.StatusServiceUnavailable) + _, _ = out.Write([]byte(`{"error":{"code":"ServiceUnavailable"}}`)) + return + } + _, _ = fmt.Fprintf(out, `{"value":[],"@odata.deltaLink":"https://graph.microsoft.com%s?$deltatoken=recovered"}`, req.URL.Path) + })) + defer srv.Close() + var events []captured + var relayed []models.SyncState + w := newGraphTestMail(t, srv, &events, &relayed) + previous := seedSyncTruth(w, &relayed) + before := w.GraphData.Client.DeltaLinks[msgraph.FolderInbox] + + for range 2 { + err := w.syncOnce(t.Context()) + if err == nil || err.Code != errx.MailErrorCodeServerUnreachable || err.RetryAfter != 12*time.Minute { + t.Fatalf("missing retryable provider evidence: %v", err) + } + if delay := w.nextSyncDelay(time.Minute, err); delay < err.RetryAfter { + t.Fatalf("service retry window undercut: %v < %v", delay, err.RetryAfter) + } + assertNoSyncSuccess(t, w, previous) + if w.GraphData.Client.DeltaLinks[msgraph.FolderInbox] != before { + t.Fatal("failed service request advanced delta cursor") + } + } + if w.transportFailures != 2 { + t.Fatalf("failed passes reset transport evidence: %d", w.transportFailures) + } + recovered.Store(true) + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + assertSyncCatchUp(t, w, previous) + if w.transportFailures != 0 || w.GraphData.Client.DeltaLinks[msgraph.FolderInbox] == before { + t.Fatal("provider-confirmed complete pass failed to update recovery state") + } +} + +func TestSyncTruthImapFolderFailurePreservesSavedStateUntilRecovery(t *testing.T) { + box := &models.Mailbox{Name: "Sent", UIDValidity: 7, UIDNext: 10, HighestModSeq: 30} + conn := &fakeImapConn{folders: []models.Mailbox{*box}, folderErr: errx.ErrMailServerUnreachable} + w, events := newIMAPTestMail(conn, &fixedBudget{allow: 100}, box) + w.EmailType = models.InboxProviderSMTPIMAP + var relayed []models.SyncState + previous := seedSyncTruth(w, &relayed) + w.tracker.state.BackfillCursor.Folders = map[string]models.SyncFolderCursor{"Sent": {Done: true, UID: 7}} + for range 2 { + if err := w.syncOnce(t.Context()); err == nil || err.Code != errx.MailErrorCodeServerUnreachable { + t.Fatalf("folder failure hidden: %v", err) + } + assertNoSyncSuccess(t, w, previous) + if hasEvent(*events, models.JobEventTypeMailboxDelete) || len(w.SmtpImapData.Mailboxes) != 1 || w.SmtpImapData.Mailboxes[0] != box { + t.Fatal("failed folder listing retired saved mailbox state") + } + if cursor, ok := w.tracker.state.BackfillCursor.Folders["Sent"]; !ok || cursor.UID != 7 || !cursor.Done { + t.Fatal("failed folder listing lost the durable recovery cursor") + } + } + conn.folderErr = nil + if err := w.syncOnce(t.Context()); err != nil { + t.Fatal(err) + } + assertSyncCatchUp(t, w, previous) + if hasEvent(*events, models.JobEventTypeMailboxDelete) || w.transportFailures != 0 { + t.Fatal("complete provider recovery did not preserve folder state") + } +} + func TestSyncTruthGoogleFailureHeartbeatAndRecovery(t *testing.T) { failing := true srv := httptest.NewServer(http.HandlerFunc(func(out http.ResponseWriter, _ *http.Request) { diff --git a/internal/client/msgraph/actions.go b/internal/client/msgraph/actions.go index 75ebc643e..d309442eb 100644 --- a/internal/client/msgraph/actions.go +++ b/internal/client/msgraph/actions.go @@ -2,11 +2,14 @@ package msgraph import ( "context" + "encoding/json" "errors" "io" "net/http" "net/url" "strings" + + "github.com/warmbly/warmbly/internal/errx" ) // Warmup mailbox actions. These mirror the goog/imap warmup actions so the @@ -153,26 +156,38 @@ func (c *Client) move(ctx context.Context, messageID, destinationID string) (str // ResolveMessageID returns the current Graph message id for the message with the // given immutable RFC 5322 internetMessageId, searching across folders. Graph // ids change on move, so warmup actions re-resolve against this stable key. -// Returns an empty string (no error) when the message can't be found. +// Only an explicit terminal empty collection establishes absence. func (c *Client) ResolveMessageID(ctx context.Context, internetMessageID string) (string, error) { + if strings.TrimSpace(internetMessageID) == "" { + return "", errMessageLookupIncomplete + } u := c.messagesByInternetID(internetMessageID, "id", 1) var resp struct { - Value []struct { - ID string `json:"id"` - } `json:"value"` + Value []GraphMessage `json:"value"` + NextLink json.RawMessage `json:"@odata.nextLink"` } if err := c.doJSON(ctx, "GET", u, nil, &resp); err != nil { + if errors.Is(err, errGraphTrailingJSON) { + return "", errMessageLookupIncomplete + } return "", err } + // Never follow a continuation or infer absence from an incomplete page. + if resp.Value == nil || resp.NextLink != nil || len(resp.Value) > 1 { + return "", errMessageLookupIncomplete + } if len(resp.Value) == 0 { return "", nil } - if resp.Value[0].ID == "" { - return "", errors.New("graph: message lookup returned an empty id") + id := resp.Value[0].ID + if id == "" || strings.TrimSpace(id) != id { + return "", errMessageLookupIncomplete } - return resp.Value[0].ID, nil + return id, nil } +var errMessageLookupIncomplete = errx.MError(errx.MailErrorWarning, errx.MailErrorCodeServerUnreachable, "The message lookup could not establish a complete identity or confirmed absence. Retry later.", errx.MailErrorResolveMethodRetry) + // LocateRFCMessageID reports whether any folder still holds the message, and // whether every copy is in Deleted Items. Delta reports a move as a removal. func (c *Client) LocateRFCMessageID(ctx context.Context, internetMessageID string) (found, trashed bool, err error) { diff --git a/internal/client/msgraph/delta.go b/internal/client/msgraph/delta.go index b1c4e7155..2655d0aaf 100644 --- a/internal/client/msgraph/delta.go +++ b/internal/client/msgraph/delta.go @@ -95,7 +95,7 @@ func (c *Client) SyncPass(ctx context.Context) (bool, error) { if folder == FolderArchive && errors.As(err, &mailErr) && mailErr.Code == errx.MailErrorCodeNotFound { continue } - if errors.As(err, &mailErr) && (mailErr.Type == errx.MailErrorCritical || mailErr.Code == errx.MailErrorCodeSendingTooFast) { + if errors.As(err, &mailErr) && (mailErr.Type == errx.MailErrorCritical || mailErr.Code == errx.MailErrorCodeSendingTooFast || mailErr.RetryAfter > 0) { return false, err } complete = false diff --git a/internal/client/msgraph/msgraph.go b/internal/client/msgraph/msgraph.go index 0a3876687..3dbe33579 100644 --- a/internal/client/msgraph/msgraph.go +++ b/internal/client/msgraph/msgraph.go @@ -161,12 +161,23 @@ func (c *Client) doJSON(ctx context.Context, method, url string, in, out any) er return HandleError(resp) } if out != nil { - err := json.NewDecoder(resp.Body).Decode(out) - var networkError net.Error - if errors.As(err, &networkError) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { - return transportError(err) + decoder := json.NewDecoder(resp.Body) + if err := decoder.Decode(out); err != nil { + var networkError net.Error + if errors.As(err, &networkError) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return transportError(err) + } + return err } - return err + var trailing json.RawMessage + if err := decoder.Decode(&trailing); err != io.EOF { + var networkError net.Error + if errors.As(err, &networkError) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, context.Canceled) { + return transportError(err) + } + return errGraphTrailingJSON + } + return nil } if _, err := io.Copy(io.Discard, resp.Body); err != nil { return transportError(err) @@ -174,6 +185,8 @@ func (c *Client) doJSON(ctx context.Context, method, url string, in, out any) er return nil } +var errGraphTrailingJSON = errors.New("graph: response contains extra JSON data") + // transportError classifies a failure of the HTTP call itself. The client's // token source runs inside it, so a grant the provider has revoked arrives // here rather than as a status code, and reporting that as "the mail server diff --git a/internal/client/msgraph/warmup_filing_test.go b/internal/client/msgraph/warmup_filing_test.go index 356ea5e34..eac3ebeb3 100644 --- a/internal/client/msgraph/warmup_filing_test.go +++ b/internal/client/msgraph/warmup_filing_test.go @@ -2,6 +2,7 @@ package msgraph import ( "context" + "errors" "fmt" "net/http" "net/http/httptest" @@ -9,9 +10,106 @@ import ( "testing" "time" + "github.com/warmbly/warmbly/internal/errx" "golang.org/x/oauth2" ) +func TestResolveMessageIDRequiresCompleteIdentityLookup(t *testing.T) { + for _, tc := range []struct { + name, body, want string + uncertain bool + }{ + {"terminal absence", `{"value":[]}`, "", false}, + {"terminal absence with whitespace", "{\"value\":[]} \n", "", false}, + {"terminal identity", `{"value":[{"id":"live-id"}]}`, "live-id", false}, + {"terminal absence followed by identity", `{"value":[]}{"value":[{"id":"live-id"}]}`, "", true}, + {"identity followed by another page", `{"value":[{"id":"live-id"}]}{"value":[]}`, "", true}, + {"terminal absence followed by junk", `{"value":[]}private-marker`, "", true}, + {"missing collection", `{}`, "", true}, + {"null collection", `{"value":null}`, "", true}, + {"null response", `null`, "", true}, + {"missing identity", `{"value":[{}]}`, "", true}, + {"null identity", `{"value":[{"id":null}]}`, "", true}, + {"empty identity", `{"value":[{"id":""}]}`, "", true}, + {"blank identity", `{"value":[{"id":" \t\n "}]}`, "", true}, + {"padded identity", `{"value":[{"id":" live-id "}]}`, "", true}, + {"ambiguous identities", `{"value":[{"id":"one"},{"id":"two"}]}`, "", true}, + {"empty page continues", `{"value":[],"@odata.nextLink":"https://graph.microsoft.com/v1.0/me/messages?$skiptoken=more"}`, "", true}, + {"identity page continues", `{"value":[{"id":"live-id"}],"@odata.nextLink":"https://graph.microsoft.com/v1.0/me/messages?$skiptoken=more"}`, "", true}, + {"foreign continuation", `{"value":[],"@odata.nextLink":"https://other.example.test/collect?token=private-marker"}`, "", true}, + {"relative continuation", `{"value":[],"@odata.nextLink":"/v1.0/me/messages?$skiptoken=more"}`, "", true}, + {"http continuation", `{"value":[],"@odata.nextLink":"http://graph.microsoft.com/v1.0/me/messages?$skiptoken=more"}`, "", true}, + {"empty continuation", `{"value":[],"@odata.nextLink":""}`, "", true}, + {"blank continuation", `{"value":[],"@odata.nextLink":" "}`, "", true}, + {"null continuation", `{"value":[],"@odata.nextLink":null}`, "", true}, + {"wrong continuation type", `{"value":[],"@odata.nextLink":{"token":"private-marker"}}`, "", true}, + } { + t.Run(tc.name, func(t *testing.T) { + requests := 0 + c := deltaTestClient(t, func(w http.ResponseWriter, r *http.Request) { + requests++ + if r.Method != http.MethodGet || r.URL.Path != "/v1.0/me/messages" || r.URL.Query().Get("$top") != "1" || r.URL.Query().Get("$select") != "id" || r.URL.Query().Get("$filter") != "internetMessageId eq ''" { + t.Errorf("lookup changed scope or followed continuation: %s %s", r.Method, r.URL.Path) + } + fmt.Fprint(w, tc.body) + }, nil) + id, err := c.ResolveMessageID(t.Context(), "") + if id != tc.want || (err != nil) != tc.uncertain || requests != 1 { + t.Fatalf("lookup: id=%q err=%v requests=%d", id, err, requests) + } + if tc.uncertain { + merr := mailErrorOf(t, err) + if !errors.Is(err, errMessageLookupIncomplete) || merr.Type != errx.MailErrorWarning || merr.Code != errx.MailErrorCodeServerUnreachable || merr.ResolveMethod != errx.MailErrorResolveMethodRetry { + t.Fatalf("incomplete lookup must be temporary: %v", err) + } + } + if err != nil && strings.Contains(err.Error(), "private-marker") { + t.Fatal("lookup error exposed continuation payload") + } + }) + } +} + +func TestResolveMessageIDRejectsBlankIdentityBeforeRequest(t *testing.T) { + requests := 0 + c := deltaTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + requests++ + fmt.Fprint(w, `{"value":[]}`) + }, nil) + for _, id := range []string{"", " \t\n "} { + if _, err := c.ResolveMessageID(t.Context(), id); !errors.Is(err, errMessageLookupIncomplete) { + t.Fatalf("blank identity must fail closed: %v", err) + } + } + if requests != 0 { + t.Fatal("blank identity queried provider") + } +} + +func TestResolveMessageIDMalformedResponseDoesNotEstablishAbsence(t *testing.T) { + for _, body := range []string{"{", "", `{"value":{}}`, `{"value":"private-marker"}`, `{"value":[null]}`, `{"value":[{"id":42}]}`} { + c := graphStatus(t, http.StatusOK, body) + if id, err := c.ResolveMessageID(t.Context(), ""); err == nil || id != "" || strings.Contains(err.Error(), "private-marker") { + t.Fatalf("malformed response resolved or exposed payload: id=%q err=%v", id, err) + } + } +} + +func TestResolveMessageIDPreservesProviderRetryEvidence(t *testing.T) { + requests := 0 + c := deltaTestClient(t, func(w http.ResponseWriter, _ *http.Request) { + requests++ + w.Header().Set("Retry-After", "90") + w.WriteHeader(http.StatusServiceUnavailable) + fmt.Fprint(w, `{"error":{"code":"ServiceUnavailable"}}`) + }, nil) + if id, err := c.ResolveMessageID(t.Context(), ""); id != "" || err == nil { + t.Fatalf("unavailable provider resolved: id=%q err=%v", id, err) + } else if merr := mailErrorOf(t, err); merr.Code != errx.MailErrorCodeServerUnreachable || merr.RetryAfter != 90*time.Second || requests != 1 { + t.Fatalf("lost provider retry evidence: code=%s retry=%v requests=%d", merr.Code, merr.RetryAfter, requests) + } +} + func TestWarmupFilingDoesNotMoveAlreadyFiledGraphMessage(t *testing.T) { for _, archive := range []bool{false, true} { t.Run(map[bool]string{false: "folder", true: "archive"}[archive], func(t *testing.T) { diff --git a/internal/client/smtpimap/imap/folders.go b/internal/client/smtpimap/imap/folders.go index 07d70e342..6c9f18f05 100644 --- a/internal/client/smtpimap/imap/folders.go +++ b/internal/client/smtpimap/imap/folders.go @@ -1,8 +1,8 @@ package imap import ( - "errors" "fmt" + "slices" "sort" "strings" "unicode/utf8" @@ -14,6 +14,16 @@ import ( "github.com/warmbly/warmbly/internal/models" ) +type FolderListing struct { + Folders []models.Mailbox + // Present includes names outside the sync cap, from the complete LIST. + Present map[string]struct{} +} + +func (c *Client) ListFolders() (FolderListing, *errx.MailError) { + return c.listFoldersCapped(config.MaxEmailFolders) +} + // Folders lists the selectable folders of the account with the cursors the // sync loop keys on (UIDVALIDITY, UIDNEXT and, on a CONDSTORE server, // HIGHESTMODSEQ). @@ -32,8 +42,13 @@ func (c *Client) Folders() ([]models.Mailbox, *errx.MailError) { // foldersCapped is Folders with the cap injectable, so a test can exercise // the overflow without standing up a hundred folders. func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { + listing, err := c.listFoldersCapped(limit) + return listing.Folders, err +} + +func (c *Client) listFoldersCapped(limit int) (FolderListing, *errx.MailError) { if err := c.ensureConnected(); err != nil { - return nil, err + return FolderListing{}, err } c.lifecycle.RLock() defer c.lifecycle.RUnlock() @@ -47,12 +62,16 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { // Asking a server without CONDSTORE for HIGHESTMODSEQ is a BAD. HighestModSeq: caps.Has(imap.CapCondStore), } - opts, listStatus := listOptionsFor(caps, status) + opts, _ := listOptionsFor(caps, status) var all []models.Mailbox + present := map[string]struct{}{} statuses := map[string]*imap.StatusData{} cmd := c.client.List("", "*", opts) for f := cmd.Next(); f != nil; f = cmd.Next() { + if !slices.Contains(f.Attrs, imap.MailboxAttrNonExistent) { + present[f.Mailbox] = struct{}{} + } attrs := make([]string, len(f.Attrs)) for i := range f.Attrs { attrs[i] = string(f.Attrs[i]) @@ -67,7 +86,7 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { } } if err := cmd.Close(); err != nil { - return nil, c.handleError(err) + return FolderListing{}, c.handleError(err) } // Before the cap, not after: a name the server listed twice would @@ -83,24 +102,21 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { for _, box := range kept { st := statuses[box.Name] if st == nil { - if listStatus { - // The server was asked and said nothing: the folder is not - // one it can open for us. - continue - } data, err := c.client.Status(box.Name, status).Wait() if err != nil { - var imapErr *imap.Error - if errors.As(err, &imapErr) { - // One folder the server will not report on must not - // take the rest of the account with it. - log.Warn().Err(err).Str("folder", box.Name).Msg("imap: STATUS refused; folder skipped") + merr := c.handleError(err) + if merr.Code == errx.ErrMailResourceNotFound.Code { + // Only explicit absence can retire a folder's stored state. + delete(present, box.Name) continue } - return nil, c.handleError(err) + return FolderListing{}, merr } st = data } + if st == nil || st.UIDValidity == 0 || st.UIDNext == 0 { + return FolderListing{}, errx.ErrMailServerUnreachable + } box.UIDValidity = st.UIDValidity box.UIDNext = uint32(st.UIDNext) box.HighestModSeq = st.HighestModSeq @@ -110,7 +126,7 @@ func (c *Client) foldersCapped(limit int) ([]models.Mailbox, *errx.MailError) { resp = append(resp, box) } - return resp, nil + return FolderListing{Folders: resp, Present: present}, nil } // listOptionsFor is the LIST options a server's capabilities allow, and diff --git a/internal/client/smtpimap/imap/server_test.go b/internal/client/smtpimap/imap/server_test.go index 07185a593..76f316cb8 100644 --- a/internal/client/smtpimap/imap/server_test.go +++ b/internal/client/smtpimap/imap/server_test.go @@ -3,6 +3,7 @@ package imap import ( "bufio" "context" + "fmt" "io" "net" "strings" @@ -13,6 +14,7 @@ import ( "github.com/emersion/go-imap/v2" "github.com/emersion/go-imap/v2/imapserver" "github.com/emersion/go-imap/v2/imapserver/imapmemserver" + "github.com/warmbly/warmbly/internal/errx" "github.com/warmbly/warmbly/internal/models" ) @@ -262,6 +264,87 @@ func TestFoldersWithoutListStatus(t *testing.T) { } } +func TestFoldersStatusFailureDoesNotBecomeFolderAbsence(t *testing.T) { + for _, tc := range []struct { + name, reply string + code errx.MailErrorCode + }{ + {"unavailable", "NO [UNAVAILABLE] Try later", errx.MailErrorCodeServerUnreachable}, + {"server bug", "NO [SERVERBUG] Try later", errx.MailErrorCodeServerUnreachable}, + {"connection limit", "NO [LIMIT] Try later", errx.MailErrorCodeSendingTooFast}, + {"authentication", "NO [AUTHENTICATIONFAILED] Sign in again", errx.MailErrorCodeInvalidCredentials}, + {"authorization", "NO [AUTHORIZATIONFAILED] Not permitted", errx.MailErrorCodeAuthorizationFailed}, + {"unknown refusal", "NO Could not obtain status", errx.MailErrorCodeImapUnknown}, + } { + t.Run(tc.name, func(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + return fields[0] + " " + tc.reply + } + return "" + }, "Sent") + if err := c.Connect(); err != nil { + t.Fatal(err) + } + boxes, err := c.Folders() + if err == nil || err.Code != tc.code || len(boxes) != 0 { + t.Fatalf("partial listing could retire Sent: boxes=%v err=%v, want %s", boxes, err, tc.code) + } + }) + } +} + +func TestFoldersExplicitlyMissingStatusFolderCanBeSkipped(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + return fields[0] + " NO [NONEXISTENT] Folder removed" + } + return "" + }, "Sent") + boxes, err := c.Folders() + if err != nil || len(boxes) != 1 || boxes[0].Name != "INBOX" { + t.Fatalf("authoritative missing folder did not preserve remaining listing: %v %v", boxes, err) + } +} + +func TestFoldersMissingListStatusFallsBackToStatus(t *testing.T) { + suppressed := 0 + c, wire := rewritingServer(t, imap.CapSet{imap.CapIMAP4rev1: {}, imap.CapListExtended: {}, imap.CapListStatus: {}}, nil, func(line string) string { + if strings.HasPrefix(strings.ToUpper(line), "* STATUS ") && suppressed < 2 { + suppressed++ + return "" + } + return line + }, "Sent") + boxes, err := c.Folders() + if err != nil || len(boxes) != 2 || len(wire.commands("STATUS")) != 2 { + t.Fatalf("missing LIST-STATUS metadata became folder absence: boxes=%v err=%v fallback=%v", boxes, err, wire.commands("STATUS")) + } +} + +func TestFoldersIncompleteStatusRemainsRetryable(t *testing.T) { + for _, reply := range []string{"OK Status unavailable", "OK Status completed"} { + t.Run(reply, func(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + if reply == "OK Status completed" { + return "* STATUS Sent (MESSAGES 0)\r\n" + fields[0] + " " + reply + } + return fields[0] + " " + reply + } + return "" + }, "Sent") + boxes, err := c.Folders() + if err == nil || err.Code != errx.MailErrorCodeServerUnreachable || len(boxes) != 0 { + t.Fatalf("incomplete status was treated as authoritative: boxes=%v err=%v", boxes, err) + } + }) + } +} + // A nested folder must be listed: "%" stopped at the top level, which is how // Gmail's [Gmail]/Sent Mail and Dovecot's INBOX.Sent went unsynced. func TestFoldersListsNested(t *testing.T) { @@ -317,6 +400,72 @@ func TestFoldersCapKeepsInboxAndSpecialFolders(t *testing.T) { } } +func TestFolderListingPreservesCompleteNamesWithBoundedStatus(t *testing.T) { + for _, listStatus := range []bool{false, true} { + t.Run(fmt.Sprintf("LIST-STATUS=%t", listStatus), func(t *testing.T) { + var caps imap.CapSet + if listStatus { + caps = imap.CapSet{imap.CapIMAP4rev1: {}, imap.CapListExtended: {}, imap.CapListStatus: {}} + } + suppressed := 0 + c, wire := rewritingServer(t, caps, nil, func(line string) string { + if listStatus && strings.HasPrefix(strings.ToUpper(line), "* STATUS ") && suppressed < 5 { + suppressed++ + return "" + } + return line + }, "Sent", "Archive", "Outside", "Other") + listing, err := c.listFoldersCapped(2) + if err != nil { + t.Fatal(err) + } + if len(listing.Folders) != 2 || c.FolderOverflow() != 3 || len(wire.commands("STATUS")) != 2 { + t.Fatalf("cap did not bound status work: listing=%+v commands=%v", listing, wire.commands("STATUS")) + } + for _, name := range []string{"INBOX", "Sent", "Archive", "Outside", "Other"} { + if _, present := listing.Present[name]; !present { + t.Fatalf("complete LIST presence lost %q outside cap", name) + } + } + for _, box := range listing.Folders { + if box.UIDValidity == 0 || box.UIDNext == 0 { + t.Fatalf("kept folder has incomplete status: %+v", box) + } + } + }) + } +} + +func TestFolderListingRequiresCompleteDiscoveryOrExplicitAbsence(t *testing.T) { + for _, reply := range []string{"NO [UNAVAILABLE] Try later", "NO [NONEXISTENT] Folder removed"} { + t.Run(reply, func(t *testing.T) { + c, _ := interceptingServer(t, nil, func(line string) string { + fields := strings.Fields(line) + if len(fields) >= 3 && strings.EqualFold(fields[1], "STATUS") && strings.Contains(line, "Sent") { + return fields[0] + " " + reply + } + return "" + }, "Sent") + listing, err := c.ListFolders() + if strings.Contains(reply, "UNAVAILABLE") { + if err == nil || listing.Present != nil || len(listing.Folders) != 0 { + t.Fatalf("failed pass leaked retirement authority: %+v err=%v", listing, err) + } + return + } + if err != nil { + t.Fatal(err) + } + if _, present := listing.Present["Sent"]; present { + t.Fatal("explicit resource absence was not retained") + } + if _, present := listing.Present["INBOX"]; !present { + t.Fatal("remaining positive presence was lost") + } + }) + } +} + // The UIDNEXT search is the incremental set without CONDSTORE: everything at // or above the cursor, and nothing below it. func TestSearchNewSince(t *testing.T) { diff --git a/internal/infrastructure/db/campaign_dispatch_intent_upgrade_test.go b/internal/infrastructure/db/campaign_dispatch_intent_upgrade_test.go new file mode 100644 index 000000000..998c81237 --- /dev/null +++ b/internal/infrastructure/db/campaign_dispatch_intent_upgrade_test.go @@ -0,0 +1,124 @@ +package db + +import ( + "context" + "net/url" + "os" + "strings" + "testing" + + "github.com/golang-migrate/migrate/v4/source/iofs" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" +) + +func TestLiveCampaignDispatchIntentUpgradePreservesUnverifiedHistoryAndRollback(t *testing.T) { + dsn := os.Getenv("WARMBLY_TEST_DB") + if dsn == "" { + t.Skip("WARMBLY_TEST_DB not set") + } + u, err := url.Parse(dsn) + if err != nil || (u.Hostname() != "localhost" && u.Hostname() != "127.0.0.1") { + t.Fatal("upgrade requires local PostgreSQL") + } + admin, err := pgx.Connect(t.Context(), dsn) + if err != nil { + t.Fatal(err) + } + defer admin.Close(context.Background()) + name := "campaign_intent_upgrade_" + uuid.New().String()[:8] + quoted := pgx.Identifier{name}.Sanitize() + if _, err := admin.Exec(t.Context(), "CREATE DATABASE "+quoted); err != nil { + t.Fatal(err) + } + defer func() { + if _, err := admin.Exec(context.Background(), "DROP DATABASE "+quoted); err != nil { + t.Error(err) + } + }() + u.Path = "/" + name + source, err := iofs.New(migrationsFS, "migrations") + if err != nil { + t.Fatal(err) + } + m, err := openMigrate(source, u.String()) + if err != nil { + t.Fatal(err) + } + defer m.Close() + if err := m.Migrate(281); err != nil { + t.Fatal(err) + } + conn, err := pgx.Connect(t.Context(), u.String()) + if err != nil { + t.Fatal(err) + } + defer conn.Close(context.Background()) + exec := func(sql string, args ...any) { + t.Helper() + if _, err := conn.Exec(t.Context(), sql, args...); err != nil { + t.Fatal(err) + } + } + user, org, mailbox, campaign, contact, step := uuid.New(), uuid.New(), uuid.New(), uuid.New(), uuid.New(), uuid.New() + legacy, bound, wakeup := uuid.New(), uuid.New(), uuid.New() + exec(`INSERT INTO users(id,email,first_name,last_name) VALUES($1,$2,'Upgrade','')`, user, user.String()+"@example.test") + exec(`INSERT INTO organizations(id,name,owner_user_id) VALUES($1,'Upgrade',$2)`, org, user) + exec(`INSERT INTO email_accounts(id,user_id,organization_id,email,name,signature_plain,signature_html,provider,send_recovery_hold,send_recovery_reason,send_recovery_task_id) VALUES($1,$2,$3,$4,'Upgrade','','','smtp_imap',true,'unknown',$5)`, mailbox, user, org, mailbox.String()+"@example.test", legacy) + exec(`INSERT INTO campaigns(id,user_id,organization_id,name,description,status,days,timezone,created_at,updated_at) VALUES($1,$2,$3,'Upgrade','','active',127,'UTC',NOW(),NOW())`, campaign, user, org) + exec(`INSERT INTO contacts(id,user_id,organization_id,email,first_name,last_name,company,phone,custom_fields) VALUES($1,$2,$3,'lead@example.test','','','','','{}')`, contact, user, org) + exec(`INSERT INTO sequences(id,campaign_id,organization_id,name,subject,body_plain,body_html,wait_after,position) VALUES($1,$2,$3,'Step','Subject','Text','',0,1)`, step, campaign, org) + for _, task := range []uuid.UUID{legacy, bound} { + exec(`INSERT INTO tasks(id,email_account_id,task_type,status,message_id) VALUES($1,$2,'campaign','dead_lettered','')`, task, mailbox) + exec(`INSERT INTO campaign_tasks(task_id,campaign_id) VALUES($1,$2)`, task, campaign) + exec(`INSERT INTO task_dead_letters(task_id,task_type,payload,last_error,attempts) VALUES($1,'campaign','{"dispatch_intent":"wakeup"}','original',1)`, task) + } + exec(`UPDATE campaign_tasks SET contact_id=$2,sequence_id=$3 WHERE task_id=$1`, bound, contact, step) + if err := m.Up(); err != nil { + t.Fatal(err) + } + var unchanged bool + if err := conn.QueryRow(t.Context(), `SELECT COUNT(*)=2 AND bool_and(ct.dispatch_intent='unverified' AND t.send_result_state IS NULL AND t.send_result_applied_at IS NULL AND d.status='pending' AND d.attempts=1 AND d.last_error='original' AND d.payload='{"dispatch_intent":"wakeup"}'::jsonb AND ea.send_recovery_hold AND ea.send_recovery_reason='unknown') FROM campaign_tasks ct JOIN tasks t ON t.id=ct.task_id JOIN task_dead_letters d ON d.task_id=t.id JOIN email_accounts ea ON ea.id=t.email_account_id`).Scan(&unchanged); err != nil || !unchanged { + t.Fatal("upgrade granted authority or changed unknown evidence", unchanged, err) + } + exec(`UPDATE campaign_tasks SET dispatch_intent='wakeup' WHERE task_id=$1`, legacy) + var intent string + if err := conn.QueryRow(t.Context(), `SELECT dispatch_intent::text FROM campaign_tasks WHERE task_id=$1`, legacy).Scan(&intent); err != nil || intent != "unverified" { + t.Fatal("legacy promoted to verified wakeup", intent, err) + } + exec(`DELETE FROM contacts WHERE id=$1`, contact) + exec(`DELETE FROM sequences WHERE id=$1`, step) + exec(`UPDATE campaign_tasks SET dispatch_intent='wakeup' WHERE task_id=$1`, bound) + if err := conn.QueryRow(t.Context(), `SELECT dispatch_intent FROM campaign_tasks WHERE task_id=$1`, bound).Scan(&intent); err != nil || intent != "send" { + t.Fatal("FK cleanup forgot prior bound send", intent, err) + } + exec(`INSERT INTO tasks(id,email_account_id,task_type,status,message_id) VALUES($1,$2,'campaign','dead_lettered','')`, wakeup, mailbox) + exec(`INSERT INTO campaign_tasks(task_id,campaign_id,dispatch_intent) VALUES($1,$2,'wakeup')`, wakeup, campaign) + if _, err := conn.Exec(t.Context(), `UPDATE campaign_tasks SET dispatch_intent='arbitrary' WHERE task_id=$1`, wakeup); err == nil { + t.Fatal("intent accepted arbitrary value") + } + down, err := migrationsFS.ReadFile("migrations/000282_campaign_dispatch_intent.down.sql") + if err != nil { + t.Fatal(err) + } + if _, err := conn.Exec(t.Context(), string(down)); err == nil || !strings.Contains(err.Error(), "restrictive recovery evidence") { + t.Fatal("rollback discarded retained send evidence", err) + } + exec(`DELETE FROM tasks WHERE id=$1`, bound) + if _, err := conn.Exec(t.Context(), string(down)); err == nil || !strings.Contains(err.Error(), "restrictive recovery evidence") { + t.Fatal("rollback discarded unverified pending dead letter", err) + } + // Only this isolated fixture is removed to exercise a clean down/up pair. + exec(`DELETE FROM tasks WHERE id IN($1,$2)`, legacy, wakeup) + exec(string(down)) + up, err := migrationsFS.ReadFile("migrations/000282_campaign_dispatch_intent.up.sql") + if err != nil { + t.Fatal(err) + } + exec(string(up)) + exec(`INSERT INTO tasks(id,email_account_id,task_type,status,message_id) VALUES($1,$2,'campaign','dead_lettered','')`, legacy, mailbox) + exec(`INSERT INTO campaign_tasks(task_id,campaign_id) VALUES($1,$2)`, legacy, campaign) + if err := conn.QueryRow(t.Context(), `SELECT dispatch_intent::text FROM campaign_tasks WHERE task_id=$1`, legacy).Scan(&intent); err != nil || intent != "unverified" { + t.Fatal("clean down/up lost restrictive default", intent, err) + } +} diff --git a/internal/infrastructure/db/migrations/000282_campaign_dispatch_intent.down.sql b/internal/infrastructure/db/migrations/000282_campaign_dispatch_intent.down.sql new file mode 100644 index 000000000..f09fad647 --- /dev/null +++ b/internal/infrastructure/db/migrations/000282_campaign_dispatch_intent.down.sql @@ -0,0 +1,17 @@ +DO $$ +BEGIN + IF EXISTS ( + SELECT 1 FROM campaign_tasks ct + LEFT JOIN task_dead_letters d ON d.task_id = ct.task_id AND d.status = 'pending' + WHERE ct.dispatch_intent = 'send' + OR (ct.dispatch_intent = 'unverified' AND d.id IS NOT NULL) + ) THEN + RAISE EXCEPTION 'campaign dispatch intent is restrictive recovery evidence; retain it and roll forward'; + END IF; +END; +$$; + +DROP TRIGGER campaign_dispatch_intent_monotonic ON campaign_tasks; +DROP FUNCTION retain_campaign_dispatch_intent(); +ALTER TABLE campaign_tasks DROP COLUMN dispatch_intent; +DROP TYPE campaign_dispatch_intent; diff --git a/internal/infrastructure/db/migrations/000282_campaign_dispatch_intent.up.sql b/internal/infrastructure/db/migrations/000282_campaign_dispatch_intent.up.sql new file mode 100644 index 000000000..dc63c36af --- /dev/null +++ b/internal/infrastructure/db/migrations/000282_campaign_dispatch_intent.up.sql @@ -0,0 +1,24 @@ +CREATE TYPE campaign_dispatch_intent AS ENUM ('unverified', 'wakeup', 'send'); + +ALTER TABLE campaign_tasks + ADD COLUMN dispatch_intent campaign_dispatch_intent NOT NULL DEFAULT 'unverified'; + +CREATE FUNCTION retain_campaign_dispatch_intent() RETURNS trigger LANGUAGE plpgsql AS $$ +BEGIN + IF TG_OP = 'UPDATE' THEN + IF OLD.dispatch_intent = 'send' OR OLD.contact_id IS NOT NULL OR OLD.sequence_id IS NOT NULL THEN + NEW.dispatch_intent := 'send'; + ELSIF OLD.dispatch_intent = 'unverified' AND NEW.dispatch_intent = 'wakeup' THEN + NEW.dispatch_intent := 'unverified'; + END IF; + END IF; + IF NEW.contact_id IS NOT NULL OR NEW.sequence_id IS NOT NULL THEN + NEW.dispatch_intent := 'send'; + END IF; + RETURN NEW; +END; +$$; + +CREATE TRIGGER campaign_dispatch_intent_monotonic + BEFORE INSERT OR UPDATE ON campaign_tasks + FOR EACH ROW EXECUTE FUNCTION retain_campaign_dispatch_intent(); diff --git a/internal/infrastructure/db/migrations/000283_warmup_charge_provenance.down.sql b/internal/infrastructure/db/migrations/000283_warmup_charge_provenance.down.sql new file mode 100644 index 000000000..c0c293372 --- /dev/null +++ b/internal/infrastructure/db/migrations/000283_warmup_charge_provenance.down.sql @@ -0,0 +1,12 @@ +DO $$ +BEGIN + IF EXISTS (SELECT 1 FROM warmup_tasks WHERE warmup_charged_date IS NOT NULL) THEN + RAISE EXCEPTION 'Cannot remove recorded warmup charge and refund evidence; retain the schema and roll forward'; + END IF; +END $$; + +ALTER TABLE warmup_tasks + DROP CONSTRAINT warmup_tasks_charge_provenance, + DROP COLUMN warmup_charged_date, + DROP COLUMN warmup_reply_charged, + DROP COLUMN warmup_refunded_at; diff --git a/internal/infrastructure/db/migrations/000283_warmup_charge_provenance.up.sql b/internal/infrastructure/db/migrations/000283_warmup_charge_provenance.up.sql new file mode 100644 index 000000000..8ec708012 --- /dev/null +++ b/internal/infrastructure/db/migrations/000283_warmup_charge_provenance.up.sql @@ -0,0 +1,8 @@ +ALTER TABLE warmup_tasks + ADD COLUMN warmup_charged_date DATE DEFAULT NULL, + ADD COLUMN warmup_reply_charged BOOLEAN DEFAULT NULL, + ADD COLUMN warmup_refunded_at TIMESTAMPTZ DEFAULT NULL, + ADD CONSTRAINT warmup_tasks_charge_provenance CHECK ( + (warmup_charged_date IS NULL AND warmup_reply_charged IS NULL AND warmup_refunded_at IS NULL) + OR (warmup_charged_date IS NOT NULL AND warmup_reply_charged IS NOT NULL) + ); diff --git a/internal/infrastructure/db/warmup_charge_upgrade_test.go b/internal/infrastructure/db/warmup_charge_upgrade_test.go new file mode 100644 index 000000000..1eb799f8e --- /dev/null +++ b/internal/infrastructure/db/warmup_charge_upgrade_test.go @@ -0,0 +1,110 @@ +package db + +import ( + "context" + "net/url" + "os" + "strings" + "testing" + + "github.com/golang-migrate/migrate/v4/source/iofs" + "github.com/google/uuid" + "github.com/jackc/pgx/v5" +) + +func TestLiveWarmupChargeUpgradeKeepsLegacyUnverifiedAndPreservesEvidence(t *testing.T) { + dsn := os.Getenv("WARMBLY_TEST_DB") + if dsn == "" { + t.Skip("WARMBLY_TEST_DB not set") + } + u, err := url.Parse(dsn) + if err != nil || (u.Hostname() != "localhost" && u.Hostname() != "127.0.0.1") { + t.Fatal("upgrade requires local PostgreSQL") + } + admin, err := pgx.Connect(t.Context(), dsn) + if err != nil { + t.Fatal(err) + } + defer admin.Close(context.Background()) + name := "warmup_charge_upgrade_" + uuid.New().String()[:8] + quoted := pgx.Identifier{name}.Sanitize() + if _, err = admin.Exec(t.Context(), "CREATE DATABASE "+quoted); err != nil { + t.Fatal(err) + } + defer func() { + if _, err := admin.Exec(context.Background(), "DROP DATABASE "+quoted); err != nil { + t.Error(err) + } + }() + u.Path = "/" + name + source, err := iofs.New(migrationsFS, "migrations") + if err != nil { + t.Fatal(err) + } + m, err := openMigrate(source, u.String()) + if err != nil { + t.Fatal(err) + } + defer m.Close() + if err = m.Migrate(282); err != nil { + t.Fatal(err) + } + conn, err := pgx.Connect(t.Context(), u.String()) + if err != nil { + t.Fatal(err) + } + defer conn.Close(context.Background()) + exec := func(sql string, args ...any) { + t.Helper() + if _, err := conn.Exec(t.Context(), sql, args...); err != nil { + t.Fatal(err) + } + } + user, org, mailbox, completed, deadletter := uuid.New(), uuid.New(), uuid.New(), uuid.New(), uuid.New() + exec(`INSERT INTO users(id,email,first_name,last_name) VALUES($1,$2,'Upgrade','')`, user, user.String()+"@example.test") + exec(`INSERT INTO organizations(id,name,owner_user_id) VALUES($1,'Upgrade',$2)`, org, user) + exec(`INSERT INTO email_accounts(id,user_id,organization_id,email,name,signature_plain,signature_html,provider,send_recovery_hold,send_recovery_reason,send_recovery_task_id) VALUES($1,$2,$3,$4,'Upgrade','','','smtp_imap',true,'unknown',$5)`, mailbox, user, org, mailbox.String()+"@example.test", deadletter) + for i, task := range []uuid.UUID{completed, deadletter} { + status := "completed" + if i == 1 { + status = "dead_lettered" + } + exec(`INSERT INTO tasks(id,email_account_id,task_type,status,message_id,completed_at,send_reserved_at,send_executor_nonce,send_result_state) VALUES($1,$2,'warmup',$3,'',NOW(),NOW(),$4,'unknown')`, task, mailbox, status, uuid.New()) + exec(`INSERT INTO warmup_tasks(task_id,lineage_version,dispatch_nonce) VALUES($1,1,$2)`, task, uuid.New()) + } + exec(`INSERT INTO warmup_statistics(email_account_id,date,emails_sent,emails_replied,target_volume) VALUES($1,(NOW() AT TIME ZONE 'UTC')::date,2,1,10)`, mailbox) + if err = m.Migrate(283); err != nil { + t.Fatal(err) + } + var untouched bool + if err = conn.QueryRow(t.Context(), `SELECT BOOL_AND(w.warmup_charged_date IS NULL AND w.warmup_reply_charged IS NULL AND w.warmup_refunded_at IS NULL AND t.completed_at IS NOT NULL AND t.send_reserved_at IS NOT NULL AND t.send_executor_nonce IS NOT NULL AND w.dispatch_nonce IS NOT NULL AND t.send_result_state='unknown' AND t.send_result_applied_at IS NULL AND ea.send_recovery_hold AND ea.send_recovery_task_id=$2 AND ws.emails_sent=2 AND ws.emails_replied=1) FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id JOIN email_accounts ea ON ea.id=t.email_account_id JOIN warmup_statistics ws ON ws.email_account_id=ea.id WHERE t.email_account_id=$1`, mailbox, deadletter).Scan(&untouched); err != nil || !untouched { + t.Fatalf("upgrade fabricated charge proof or altered unknown recovery: untouched=%v error=%v", untouched, err) + } + if _, err = conn.Exec(t.Context(), `UPDATE warmup_tasks SET warmup_charged_date=(NOW() AT TIME ZONE 'UTC')::date WHERE task_id=$1`, completed); err == nil { + t.Fatal("partial charge provenance accepted") + } + exec(`UPDATE warmup_tasks SET warmup_charged_date=(NOW() AT TIME ZONE 'UTC')::date,warmup_reply_charged=false WHERE task_id=$1`, completed) + down, err := migrationsFS.ReadFile("migrations/000283_warmup_charge_provenance.down.sql") + if err != nil { + t.Fatal(err) + } + for _, refunded := range []bool{false, true} { + if refunded { + exec(`UPDATE warmup_tasks SET warmup_refunded_at=NOW() WHERE task_id=$1`, completed) + } + if _, err = conn.Exec(t.Context(), string(down)); err == nil || !strings.Contains(err.Error(), "Cannot remove recorded warmup charge") { + t.Fatalf("rollback discarded charge/refund evidence (refunded=%v): %v", refunded, err) + } + } + exec(`UPDATE warmup_tasks SET warmup_charged_date=NULL,warmup_reply_charged=NULL,warmup_refunded_at=NULL WHERE task_id=$1`, completed) + if err = m.Steps(-1); err != nil { + t.Fatal(err) + } + if err = m.Steps(1); err != nil { + t.Fatal(err) + } + version, dirty, err := m.Version() + if err != nil || version != 283 || dirty { + t.Fatalf("final migrated schema version=%d dirty=%v error=%v", version, dirty, err) + } +} diff --git a/internal/repository/campaign_replay_intent_live_test.go b/internal/repository/campaign_replay_intent_live_test.go new file mode 100644 index 000000000..8a83e0bca --- /dev/null +++ b/internal/repository/campaign_replay_intent_live_test.go @@ -0,0 +1,186 @@ +package repository + +import ( + "context" + "errors" + "net/url" + "os" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/warmbly/warmbly/internal/infrastructure/db" +) + +func liveCampaignReplayDB(t *testing.T) *pgxpool.Pool { + t.Helper() + dsn := os.Getenv("WARMBLY_TEST_DB") + if dsn == "" { + t.Skip("WARMBLY_TEST_DB not set") + } + u, err := url.Parse(dsn) + if err != nil || (u.Hostname() != "localhost" && u.Hostname() != "127.0.0.1") { + t.Fatal("replay fixture requires local PostgreSQL") + } + admin, err := pgx.Connect(t.Context(), dsn) + if err != nil { + t.Fatal(err) + } + name := "campaign_replay_" + uuid.New().String()[:8] + quoted := pgx.Identifier{name}.Sanitize() + if _, err := admin.Exec(t.Context(), "CREATE DATABASE "+quoted); err != nil { + _ = admin.Close(context.Background()) + t.Fatal(err) + } + t.Cleanup(func() { + if _, err := admin.Exec(context.Background(), "DROP DATABASE "+quoted); err != nil { + t.Error(err) + } + _ = admin.Close(context.Background()) + }) + u.Path = "/" + name + if err := db.RunMigrations(u.String()); err != nil { + t.Fatal(err) + } + pool, err := pgxpool.New(t.Context(), u.String()) + if err != nil { + t.Fatal(err) + } + t.Cleanup(pool.Close) + return pool +} + +func TestLiveCampaignReplayIntentTransitionsFilterBeforeLimitAndLock(t *testing.T) { + pool := liveCampaignReplayDB(t) + f := newWarmupUsageFixture(t, pool) + r := NewTaskRepository(pool) + campaign, contact, sequence := uuid.New(), uuid.New(), uuid.New() + f.exec(`INSERT INTO campaigns(id,user_id,organization_id,name,description,status,days,timezone,created_at,updated_at) VALUES($1,$2,$3,'Replay','','active',127,'UTC',NOW(),NOW())`, campaign, f.user, f.org) + f.exec(`INSERT INTO contacts(id,user_id,organization_id,email,first_name,last_name,company,phone,custom_fields) VALUES($1,$2,$3,'lead@example.test','','','','','{}')`, contact, f.user, f.org) + f.exec(`INSERT INTO sequences(id,campaign_id,organization_id,name,subject,body_plain,body_html,wait_after,position) VALUES($1,$2,$3,'Step','Subject','Text','',0,1)`, sequence, campaign, f.org) + if err := r.UpdateCampaignTaskTracking(t.Context(), uuid.New(), contact, sequence); !errors.Is(err, pgx.ErrNoRows) { + t.Fatal("missing tracking row accepted as durable intent", err) + } + for range 60 { + id := uuid.New() + f.exec(`INSERT INTO tasks(id,email_account_id,task_type,status,message_id) VALUES($1,$2,'campaign','dead_lettered','')`, id, f.account) + f.exec(`INSERT INTO campaign_tasks(task_id,campaign_id) VALUES($1,$2)`, id, campaign) + f.exec(`INSERT INTO task_dead_letters(task_id,task_type,next_retry_at,payload) VALUES($1,'campaign',NOW()-INTERVAL '2 hours','{"dispatch_intent":"wakeup","safe":true}')`, id) + } + newWakeup := func() uuid.UUID { + t.Helper() + id := uuid.New() + created, err := r.CreateTaskWithLock(t.Context(), &Task{ID: id, TaskType: "campaign", Status: "dead_lettered", EmailAccountID: f.account}, &CampaignTask{TaskID: id, CampaignID: &campaign}) + if err != nil || !created { + t.Fatalf("create wakeup: %t %v", created, err) + } + f.exec(`INSERT INTO task_dead_letters(task_id,task_type,next_retry_at,payload,last_error,attempts) VALUES($1,'campaign',NOW()-INTERVAL '1 hour','{"original":true}','retained',1)`, id) + return id + } + send := newWakeup() + if err := r.UpdateCampaignTaskTracking(t.Context(), send, contact, sequence); err != nil { + t.Fatal(err) + } + f.exec(`DELETE FROM contacts WHERE id=$1`, contact) + f.exec(`DELETE FROM sequences WHERE id=$1`, sequence) + f.exec(`UPDATE campaign_tasks SET dispatch_intent='wakeup' WHERE task_id=$1`, send) + ct, err := r.GetCampaignTask(t.Context(), send) + if err != nil || ct.DispatchIntent != CampaignDispatchSend || ct.ContactID != nil || ct.SequenceID != nil { + t.Fatalf("send capability lost after FK cleanup: %+v %v", ct, err) + } + wakeup := newWakeup() + listed, err := NewAdvancedOutreachRepository(pool).ListRetryableDeadLetters(t.Context(), 50) + if err != nil || len(listed) != 1 || listed[0].TaskID != wakeup { + t.Fatalf("unsafe legacy rows filled LIMIT before wakeup: %+v %v", listed, err) + } + for _, row := range listed { + ct, err := r.GetCampaignTask(t.Context(), row.TaskID) + if err != nil || ct.DispatchIntent != CampaignDispatchWakeup || ct.ContactID != nil || ct.SequenceID != nil { + t.Fatalf("unverified replay candidate: %+v %v", ct, err) + } + } + for _, source := range []uuid.UUID{send, uuid.New()} { + id := uuid.New() + if _, err := r.CreateCampaignReplayTask(t.Context(), &Task{ID: id, TaskType: "campaign", Status: "pending", EmailAccountID: f.account}, &CampaignTask{TaskID: id, CampaignID: &campaign}, source); !errors.Is(err, ErrCampaignReplayUnverified) { + t.Fatalf("unverified source replay: %v", err) + } + } + var created atomic.Int32 + var wg sync.WaitGroup + errs := make(chan error, 8) + for range 8 { + wg.Add(1) + go func() { + defer wg.Done() + id := uuid.New() + ok, err := r.CreateCampaignReplayTask(t.Context(), &Task{ID: id, TaskType: "campaign", Status: "pending", EmailAccountID: f.account}, &CampaignTask{TaskID: id, CampaignID: &campaign}, wakeup) + if err != nil { + errs <- err + } + if ok { + created.Add(1) + } + }() + } + wg.Wait() + close(errs) + for err := range errs { + t.Fatal(err) + } + if created.Load() != 1 { + t.Fatal("per-campaign replay created duplicate chains", created.Load()) + } + var intact bool + if err := pool.QueryRow(t.Context(), `SELECT status='pending' AND attempts=1 AND last_error='retained' AND payload='{"original":true}'::jsonb FROM task_dead_letters WHERE task_id=$1`, wakeup).Scan(&intact); err != nil || !intact { + t.Fatal("constructor changed original evidence", intact, err) + } +} + +func TestLiveCampaignReplayRechecksIntentAfterRowLockWait(t *testing.T) { + pool := liveCampaignReplayDB(t) + f := newWarmupUsageFixture(t, pool) + r := NewTaskRepository(pool) + campaign, source, replay := uuid.New(), uuid.New(), uuid.New() + f.exec(`INSERT INTO campaigns(id,user_id,organization_id,name,description,status,days,timezone,created_at,updated_at) VALUES($1,$2,$3,'Replay','','active',127,'UTC',NOW(),NOW())`, campaign, f.user, f.org) + if ok, err := r.CreateTaskWithLock(t.Context(), &Task{ID: source, TaskType: "campaign", Status: "dead_lettered", EmailAccountID: f.account}, &CampaignTask{TaskID: source, CampaignID: &campaign}); err != nil || !ok { + t.Fatal(ok, err) + } + f.exec(`INSERT INTO task_dead_letters(task_id,task_type,next_retry_at) VALUES($1,'campaign',NOW())`, source) + tx, err := pool.Begin(t.Context()) + if err != nil { + t.Fatal(err) + } + defer tx.Rollback(context.Background()) + if _, err := tx.Exec(t.Context(), `UPDATE campaign_tasks SET dispatch_intent='send' WHERE task_id=$1`, source); err != nil { + t.Fatal(err) + } + result := make(chan error, 1) + go func() { + _, err := r.CreateCampaignReplayTask(t.Context(), &Task{ID: replay, TaskType: "campaign", Status: "pending", EmailAccountID: f.account}, &CampaignTask{TaskID: replay, CampaignID: &campaign}, source) + result <- err + }() + ctx, cancel := context.WithTimeout(t.Context(), 5*time.Second) + defer cancel() + for { + var waiting bool + if err := pool.QueryRow(ctx, `SELECT EXISTS(SELECT 1 FROM pg_stat_activity WHERE datname=current_database() AND wait_event_type='Lock' AND query LIKE 'SELECT true FROM task_dead_letters%')`).Scan(&waiting); err != nil { + t.Fatal(err) + } + if waiting { + break + } + if ctx.Err() != nil { + t.Fatal(ctx.Err()) + } + } + if err := tx.Commit(t.Context()); err != nil { + t.Fatal(err) + } + if err := <-result; !errors.Is(err, ErrCampaignReplayUnverified) { + t.Fatalf("stale wakeup snapshot allowed replay: %v", err) + } +} diff --git a/internal/repository/pg_advanced_outreach.go b/internal/repository/pg_advanced_outreach.go index 90375a124..d28520ed3 100644 --- a/internal/repository/pg_advanced_outreach.go +++ b/internal/repository/pg_advanced_outreach.go @@ -1314,13 +1314,15 @@ func (r *advancedOutreachRepository) ListRetryableDeadLetters(ctx context.Contex limit = 10 } query := ` - SELECT id, task_id, task_type, payload, last_error, attempts, max_attempts, status, next_retry_at, replayed_at, created_at, updated_at - FROM task_dead_letters - WHERE status = 'pending' - AND next_retry_at IS NOT NULL - AND next_retry_at <= NOW() - AND attempts < max_attempts - ORDER BY next_retry_at ASC + SELECT d.id, d.task_id, d.task_type, d.payload, d.last_error, d.attempts, d.max_attempts, d.status, d.next_retry_at, d.replayed_at, d.created_at, d.updated_at + FROM task_dead_letters d + JOIN tasks t ON t.id = d.task_id + WHERE d.status = 'pending' + AND d.next_retry_at IS NOT NULL + AND d.next_retry_at <= NOW() + AND d.attempts < d.max_attempts + AND ` + safeDLQCampaignPass + ` + ORDER BY d.next_retry_at ASC LIMIT $1 ` rows, err := resultDB(ctx, r.db).Query(ctx, query, limit) diff --git a/internal/repository/pg_monitoring.go b/internal/repository/pg_monitoring.go index 080e9e093..028f785d9 100644 --- a/internal/repository/pg_monitoring.go +++ b/internal/repository/pg_monitoring.go @@ -238,10 +238,11 @@ var monitoringSections = map[string]monitoringSection{ {id: "job_ok", title: "Jobs with latest successful wrapper completion", unit: "jobs", condition: "healthy", severity: "info", note: "Name-keyed instance rows, not replica heartbeats; wrapper return does not prove subsystem throughput.", window: 0 * time.Second, query: `SELECT COUNT(*), NULL::bigint, NULL::bigint, NULL::bigint, MIN(j.last_finished_at::timestamptz), MAX(j.last_finished_at::timestamptz), MIN(j.next_run_at::timestamptz) FROM scheduled_job_runs j WHERE j.last_status='ok'`}, }}, "dead_letters": {permission: models.AdminPermViewCampaigns, investigate: "/sends?tab=dead-letters", queries: []monitoringQuery{ - {id: "dlq_due", title: "Dead letters due for retry", unit: "records", condition: "backlog", severity: "warning", note: "Durable queue state; attempts are delivery/replay attempts, replay is not provider confirmation.", window: 0 * time.Second, query: `SELECT COUNT(*), COUNT(DISTINCT t.email_account_id), COUNT(DISTINCT e.organization_id), COUNT(*) FILTER(WHERE e.organization_id IS NULL), MIN(d.next_retry_at::timestamptz), MAX(d.updated_at::timestamptz), MIN(d.next_retry_at::timestamptz) FROM task_dead_letters d LEFT JOIN tasks t ON t.id=d.task_id LEFT JOIN email_accounts e ON e.id=t.email_account_id WHERE d.status='pending' AND d.next_retry_at<=$1 AND d.attempts$1 AND d.attempts$1 AND d.attempts=d.max_attempts`}, - {id: "dlq_unscheduled", title: "Pending dead letters without retry slot", unit: "records", condition: "unknown", severity: "warning", note: "Durable queue state; attempts are delivery/replay attempts, replay is not provider confirmation.", window: 0 * time.Second, query: `SELECT COUNT(*), COUNT(DISTINCT t.email_account_id), COUNT(DISTINCT e.organization_id), COUNT(*) FILTER(WHERE e.organization_id IS NULL), MIN(d.created_at::timestamptz), MAX(d.updated_at::timestamptz), MIN(d.next_retry_at::timestamptz) FROM task_dead_letters d LEFT JOIN tasks t ON t.id=d.task_id LEFT JOIN email_accounts e ON e.id=t.email_account_id WHERE d.status='pending' AND d.next_retry_at IS NULL AND d.attempts 0 - ) THEN GREATEST(ws.emails_replied - 1, 0) - ELSE ws.emails_replied - END - WHERE ws.email_account_id = $1 - AND ws.date = DATE($3) - `, accountID, taskID, date); err != nil { + if err = refundWarmupCharge(ctx, tx, taskID, accountID); err != nil { return err } if _, err = tx.Exec(ctx, `UPDATE tasks SET status = 'failed', updated_at = NOW() WHERE id = $1`, taskID); err != nil { @@ -1139,6 +1130,17 @@ func (r *warmupRepository) FailWarmupSend(ctx context.Context, accountID, taskID return tx.Commit(ctx) } +func refundWarmupCharge(ctx context.Context, tx pgx.Tx, taskID, accountID uuid.UUID) error { + _, err := tx.Exec(ctx, `WITH refunded AS (UPDATE warmup_tasks w SET warmup_refunded_at=NOW() + FROM tasks t WHERE w.task_id=t.id AND t.id=$1 AND t.email_account_id=$2 AND t.task_type='warmup' + AND w.warmup_charged_date IS NOT NULL AND w.warmup_reply_charged IS NOT NULL AND w.warmup_refunded_at IS NULL + RETURNING w.warmup_charged_date,w.warmup_reply_charged) + UPDATE warmup_statistics s SET emails_sent=GREATEST(s.emails_sent-1,0), + emails_replied=GREATEST(s.emails_replied-CASE WHEN r.warmup_reply_charged THEN 1 ELSE 0 END,0) + FROM refunded r WHERE s.email_account_id=$2 AND s.date=r.warmup_charged_date`, taskID, accountID) + return err +} + func (r *warmupRepository) LastWarmupSendFailure(ctx context.Context, accountID uuid.UUID, since time.Time) (*models.WarmupSendFailure, error) { f := &models.WarmupSendFailure{} var first time.Time diff --git a/internal/repository/send_admission.go b/internal/repository/send_admission.go index cf1ebfe3b..22af8fcb1 100644 --- a/internal/repository/send_admission.go +++ b/internal/repository/send_admission.go @@ -228,7 +228,7 @@ func (r *taskRepository) ReserveOutbound(ctx context.Context, in OutboundReserva } _, err = tx.Exec(ctx, `UPDATE tasks SET send_reserved_at=NOW(),send_business_day=(NOW() AT TIME ZONE $2)::date,send_recipients=$3, send_released_at=NULL,send_executor_nonce=$4,send_executor_worker=$5,send_executor_started_at=NULL,send_executor_result=NULL, - send_result_state='unknown',status='completed',completed_at=NOW() WHERE id=$1`, in.TaskID, timezone, recipients, nonce, in.WorkerID) + send_result_state='unknown',status='completed',completed_at=CASE WHEN $6 THEN COALESCE(completed_at,NOW()) ELSE NOW() END WHERE id=$1`, in.TaskID, timezone, recipients, nonce, in.WorkerID, lane == "warmup") if err != nil { return uuid.Nil, err } @@ -496,7 +496,7 @@ func (r *taskRepository) cancelOutbound(ctx context.Context, task, mailbox, work } } if lane == "warmup" { - if _, err = tx.Exec(ctx, `UPDATE warmup_statistics SET emails_sent=GREATEST(emails_sent-1,0),emails_replied=GREATEST(emails_replied-CASE WHEN w.parent_task_id IS NULL THEN 0 ELSE 1 END,0) FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id WHERE w.task_id=$2 AND warmup_statistics.email_account_id=$1 AND date=DATE(t.completed_at)`, mailbox, task); err != nil { + if err = refundWarmupCharge(ctx, tx, task, mailbox); err != nil { return err } if _, err = tx.Exec(ctx, `DELETE FROM warmup_tokens WHERE task_id=$1`, task); err != nil { diff --git a/internal/repository/unknown_send_resolution.go b/internal/repository/unknown_send_resolution.go index 8d6861acd..6a063f890 100644 --- a/internal/repository/unknown_send_resolution.go +++ b/internal/repository/unknown_send_resolution.go @@ -91,33 +91,31 @@ func queueUnknownSendResolution(ctx context.Context, tx pgx.Tx, org, mailbox str func (r *taskRepository) ListUnappliedSendResults(ctx context.Context, limit int) ([]models.SendEmailResult, error) { rows, err := r.db.Query(ctx, `WITH recorded AS ( SELECT t.id,t.created_at,t.send_result_state,t.send_result_applied_at,t.send_executor_result AS result,1 AS priority - FROM tasks t WHERE t.send_executor_result IS NOT NULL AND t.status<>'cancelled' AND t.send_result_applied_at IS NULL + FROM tasks t WHERE t.send_executor_result IS NOT NULL AND t.status<>'cancelled' + AND (t.send_result_applied_at IS NULL OR (`+repairableSendDeadLetterPredicate+`)) UNION ALL SELECT t.id,t.created_at,t.send_result_state,t.send_result_applied_at,t.send_executor_result,1 FROM send_recovery_resolutions r JOIN tasks t ON t.id=r.recovery_task_id WHERE r.previous_reason='unknown' AND r.conflict_detected_at IS NULL AND r.confirmed_result IS NOT NULL AND t.send_executor_result IS NOT NULL AND t.send_result_applied_at IS NOT NULL AND t.status<>'cancelled' UNION ALL SELECT t.id,t.created_at,t.send_result_state,t.send_result_applied_at,w.dispatch_result,1 FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id - WHERE w.dispatch_result IS NOT NULL AND t.status<>'cancelled' AND t.send_result_applied_at IS NULL + WHERE w.dispatch_result IS NOT NULL AND t.status<>'cancelled' + AND (t.send_result_applied_at IS NULL OR (`+repairableSendDeadLetterPredicate+`)) UNION ALL SELECT t.id,t.created_at,t.send_result_state,t.send_result_applied_at,w.dispatch_result,1 FROM send_recovery_resolutions r JOIN tasks t ON t.id=r.recovery_task_id JOIN warmup_tasks w ON w.task_id=t.id WHERE r.previous_reason='unknown' AND r.conflict_detected_at IS NULL AND r.confirmed_result IS NOT NULL AND w.dispatch_result IS NOT NULL AND t.send_result_applied_at IS NOT NULL AND t.status<>'cancelled' UNION ALL SELECT t.id,t.created_at,t.send_result_state,t.send_result_applied_at,r.confirmed_result,2 FROM send_recovery_resolutions r JOIN tasks t ON t.id=r.recovery_task_id - WHERE r.confirmed_result IS NOT NULL AND t.send_result_applied_at IS NULL AND t.status<>'cancelled' - UNION ALL - SELECT t.id,t.created_at,t.send_result_state,t.send_result_applied_at,jsonb_build_object('task_id',t.id,'success',false,'legacy_error','Send failed before reservation', - 'error',jsonb_build_object('failure',jsonb_build_object('protocol','internal','stage','prepare','scope','mailbox','disposition','retry'))),3 - FROM tasks t WHERE t.status='failed' AND t.send_result_state='unknown' AND t.send_result_applied_at IS NULL - AND t.send_reserved_at IS NULL AND t.send_executor_nonce IS NULL AND t.send_executor_started_at IS NULL - AND NOT EXISTS(SELECT 1 FROM warmup_tasks w WHERE w.task_id=t.id AND w.dispatch_nonce IS NOT NULL)), + WHERE r.confirmed_result IS NOT NULL AND (t.send_result_applied_at IS NULL OR (`+repairableSendDeadLetterPredicate+`)) AND t.status<>'cancelled' + ), definitive AS (SELECT *,CASE WHEN result->>'success'='true' THEN 'sent' ELSE 'failed' END AS outcome FROM recorded WHERE result->>'success'='true' OR result->'error'->'failure'->>'disposition' IN('retry','throttle','permanent','authentication') OR (result->'error'->>'failure' IS NULL AND result->'error'->>'code' IN ('RECIPIENT_REJECTED','SEND_REJECTED','DOMAIN_AUTH_REJECTED','AUTHENTICATION_FAILED','INVALID_CREDENTIALS','GOOGLE_AUTHENTICATION_FAILED','SENDING_TOO_FAST','QUOTA_EXCEEDED','UNSUPPORTED'))), pending AS (SELECT DISTINCT ON(id) id,created_at,result FROM definitive - WHERE send_result_applied_at IS NULL OR outcome IS DISTINCT FROM send_result_state ORDER BY id,priority) + WHERE send_result_applied_at IS NULL OR outcome IS DISTINCT FROM send_result_state + OR EXISTS(SELECT 1 FROM tasks t WHERE t.id=definitive.id AND `+repairableSendDeadLetterPredicate+`) ORDER BY id,priority) SELECT id,result FROM pending ORDER BY created_at LIMIT $1`, limit) if err != nil { return nil, err diff --git a/internal/repository/warmup_charge_live_test.go b/internal/repository/warmup_charge_live_test.go new file mode 100644 index 000000000..c75f72234 --- /dev/null +++ b/internal/repository/warmup_charge_live_test.go @@ -0,0 +1,315 @@ +package repository + +import ( + "context" + "errors" + "os" + "sync" + "testing" + "time" + + "github.com/google/uuid" + "github.com/jackc/pgx/v5/pgconn" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/warmbly/warmbly/internal/errx" + "github.com/warmbly/warmbly/internal/models" +) + +func newWarmupChargeFixture(t *testing.T, reply bool) (*poolLinkFixture, *taskRepository, uuid.UUID, uuid.UUID) { + t.Helper() + f, r := lineageFixture(t) + ctx := t.Context() + worker := uuid.New() + queries := []struct { + sql string + args []any + }{ + {`INSERT INTO fleet_nodes(id,role,active,last_seen_at,warmup_send_protocol) VALUES($1,'worker',true,NOW(),2)`, []any{worker}}, + {`INSERT INTO workers(id) VALUES($1)`, []any{worker}}, + {`UPDATE email_accounts SET worker_id=$1,shared_daily_limit=100,rolling_recipient_limit=100,warmup_max=50 WHERE id=$2`, []any{worker, f.sender}}, + {`INSERT INTO warmup_pool_participants(pool_id,email_account_id) SELECT wp.id,ea.id FROM warmup_pools wp CROSS JOIN email_accounts ea WHERE wp.pool_type='free' AND ea.id IN($1,$2) ON CONFLICT DO NOTHING`, []any{f.sender, f.recipient}}, + {`UPDATE tasks SET status='active' WHERE id=$1`, []any{f.task}}, + {`INSERT INTO warmup_tasks(task_id,target_account_id,lineage_version,subject,scenario_version,rendering_version,max_turns) VALUES($1,$2,1,'Pinned diagnostic','diagnostic-v1','canonical-v1',3)`, []any{f.task, f.recipient}}, + } + for _, q := range queries { + if _, err := f.pool.Exec(ctx, q.sql, q.args...); err != nil { + t.Fatal(err) + } + } + if reply { + parent := uuid.New() + if _, err := f.pool.Exec(ctx, `INSERT INTO tasks(id,email_account_id,task_type,status,message_id) VALUES($1,$2,'warmup','completed','')`, parent, f.recipient); err != nil { + t.Fatal(err) + } + if _, err := f.pool.Exec(ctx, `UPDATE warmup_tasks SET parent_task_id=$1 WHERE task_id=$2`, parent, f.task); err != nil { + t.Fatal(err) + } + } + t.Cleanup(func() { _, _ = f.pool.Exec(context.Background(), `DELETE FROM fleet_nodes WHERE id=$1`, worker) }) + f.token(t, "", "Pinned diagnostic", false) + nonce, err := r.AuthorizeWarmupDispatch(ctx, f.task, f.sender, worker) + if err != nil { + t.Fatal(err) + } + return f, r, worker, nonce +} + +func TestLiveWarmupReservationPreservesOriginalChargeDay(t *testing.T) { + f, r, worker, _ := newWarmupChargeFixture(t, false) + ctx := t.Context() + for _, q := range []string{ + `UPDATE tasks SET completed_at=completed_at-INTERVAL '1 day' WHERE id=$1`, + `UPDATE warmup_tasks SET warmup_charged_date=warmup_charged_date-1 WHERE task_id=$1`, + } { + if _, err := f.pool.Exec(ctx, q, f.task); err != nil { + t.Fatal(err) + } + } + if _, err := f.pool.Exec(ctx, `UPDATE warmup_statistics SET date=date-1 WHERE email_account_id=$1`, f.sender); err != nil { + t.Fatal(err) + } + if err := f.warmup.IncrementDailyCount(ctx, f.sender, time.Now()); err != nil { + t.Fatal(err) + } + original, err := r.GetTask(ctx, f.task) + if err != nil { + t.Fatal(err) + } + address := "pl-" + f.recipient.String()[:8] + "@test.local" + if _, err = r.ReserveOutbound(ctx, OutboundReservation{TaskID: f.task, MailboxID: f.sender, OrganizationID: f.org, WorkerID: worker, Provider: models.InboxProviderSMTPIMAP, Recipients: []string{address}}); err != nil { + t.Fatal(err) + } + reserved, err := r.GetTask(ctx, f.task) + if err != nil || reserved.CompletedAt == nil || original.CompletedAt == nil || !reserved.CompletedAt.Equal(*original.CompletedAt) { + t.Fatalf("reservation rewrote the charge timestamp: original=%+v reserved=%+v error=%v", original, reserved, err) + } + // Generic completion cannot change the independently recorded charge day. + if err = r.UpdateTaskStatus(ctx, f.task, "completed"); err != nil { + t.Fatal(err) + } + result := models.SendEmailResult{TaskID: f.task, Error: &models.EmailSendError{Failure: &errx.SendFailure{Disposition: errx.SendRetry}}} + for range 2 { + if err = r.ApplySendResult(ctx, result, func(ctx context.Context) error { + return f.warmup.FailWarmupSend(ctx, f.sender, f.task, time.Now(), "Send failed", "Provider refused") + }); err != nil { + t.Fatal(err) + } + } + var yesterday, today int + if err = f.pool.QueryRow(ctx, `SELECT SUM(emails_sent) FILTER(WHERE date=(NOW() AT TIME ZONE 'UTC')::date-1),SUM(emails_sent) FILTER(WHERE date=(NOW() AT TIME ZONE 'UTC')::date) FROM warmup_statistics WHERE email_account_id=$1`, f.sender).Scan(&yesterday, &today); err != nil { + t.Fatal(err) + } + if yesterday != 0 || today != 1 { + t.Fatalf("wrong-day refund: yesterday=%d today=%d; want 0/1", yesterday, today) + } +} + +func TestLiveWarmupRefundCASProtectsConcurrentAndRestampedTasks(t *testing.T) { + f, r, _, _ := newWarmupChargeFixture(t, false) + ctx := t.Context() + var wg sync.WaitGroup + for range 12 { + wg.Add(1) + go func() { + defer wg.Done() + if err := f.warmup.FailWarmupSend(ctx, f.sender, f.task, time.Now(), "Send failed", "Provider refused"); err != nil { + t.Error(err) + } + }() + } + wg.Wait() + var sent int + var refunded time.Time + if err := f.pool.QueryRow(ctx, `SELECT ws.emails_sent,w.warmup_refunded_at FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id JOIN warmup_statistics ws ON ws.email_account_id=t.email_account_id AND ws.date=w.warmup_charged_date WHERE w.task_id=$1`, f.task).Scan(&sent, &refunded); err != nil || sent != 0 { + t.Fatalf("concurrent refund sent=%d error=%v", sent, err) + } + if err := f.warmup.IncrementDailyCount(ctx, f.sender, time.Now()); err != nil { + t.Fatal(err) + } + if err := r.UpdateTaskStatus(ctx, f.task, "completed"); err != nil { + t.Fatal(err) + } + if err := f.warmup.FailWarmupSend(ctx, f.sender, f.task, time.Now(), "Send failed", "Duplicate"); err != nil { + t.Fatal(err) + } + var again time.Time + if err := f.pool.QueryRow(ctx, `SELECT ws.emails_sent,w.warmup_refunded_at FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id JOIN warmup_statistics ws ON ws.email_account_id=t.email_account_id AND ws.date=w.warmup_charged_date WHERE w.task_id=$1`, f.task).Scan(&sent, &again); err != nil || sent != 1 || !again.Equal(refunded) { + t.Fatalf("restamping refunded another task: sent=%d refunded=%v again=%v error=%v", sent, refunded, again, err) + } +} + +func TestLiveWarmupRefundAndResultRollbackTogether(t *testing.T) { + f, r, _, _ := newWarmupChargeFixture(t, false) + ctx := t.Context() + want := errors.New("later result write failed") + result := models.SendEmailResult{TaskID: f.task, Error: &models.EmailSendError{Failure: &errx.SendFailure{Disposition: errx.SendRetry}}} + err := r.ApplySendResult(ctx, result, func(ctx context.Context) error { + if err := f.warmup.FailWarmupSend(ctx, f.sender, f.task, time.Now(), "Send failed", "Provider refused"); err != nil { + return err + } + return want + }) + if !errors.Is(err, want) { + t.Fatal(err) + } + var status, state string + var refunded, applied bool + var sent int + if err = f.pool.QueryRow(ctx, `SELECT t.status,t.send_result_state,t.send_result_applied_at IS NOT NULL,w.warmup_refunded_at IS NOT NULL,ws.emails_sent FROM tasks t JOIN warmup_tasks w ON w.task_id=t.id JOIN warmup_statistics ws ON ws.email_account_id=t.email_account_id AND ws.date=w.warmup_charged_date WHERE t.id=$1`, f.task).Scan(&status, &state, &applied, &refunded, &sent); err != nil { + t.Fatal(err) + } + if status != "completed" || state != "unknown" || applied || refunded || sent != 1 { + t.Fatalf("partial refund committed: status=%s state=%s applied=%v refunded=%v sent=%d", status, state, applied, refunded, sent) + } +} + +func TestLiveWarmupReplyRefundUsesChargedShapeAfterParentDeletion(t *testing.T) { + f, _, _, _ := newWarmupChargeFixture(t, true) + ctx := t.Context() + if _, err := f.pool.Exec(ctx, `DELETE FROM tasks WHERE id=(SELECT parent_task_id FROM warmup_tasks WHERE task_id=$1)`, f.task); err != nil { + t.Fatal(err) + } + var detached, reply bool + if err := f.pool.QueryRow(ctx, `SELECT parent_task_id IS NULL,warmup_reply_charged FROM warmup_tasks WHERE task_id=$1`, f.task).Scan(&detached, &reply); err != nil || !detached || !reply { + t.Fatalf("reply proof lost with parent: detached=%v reply=%v error=%v", detached, reply, err) + } + if err := f.warmup.FailWarmupSend(ctx, f.sender, f.task, time.Now(), "Send failed", "Provider refused"); err != nil { + t.Fatal(err) + } + var sent, replies int + if err := f.pool.QueryRow(ctx, `SELECT emails_sent,emails_replied FROM warmup_statistics WHERE email_account_id=$1`, f.sender).Scan(&sent, &replies); err != nil || sent != 0 || replies != 0 { + t.Fatalf("incorrect reply refund: sent=%d replies=%d error=%v", sent, replies, err) + } +} + +func TestLiveWarmupChargeAndRefundUseUTCInNonUTCSession(t *testing.T) { + f, _, worker, nonce := newWarmupChargeFixture(t, false) + ctx := t.Context() + config, err := pgxpool.ParseConfig(os.Getenv("WARMBLY_TEST_DB")) + if err != nil { + t.Fatal(err) + } + zone := "Etc/GMT-14" + if time.Now().UTC().Hour() < 10 { + zone = "Etc/GMT+12" + } + config.ConnConfig.RuntimeParams["timezone"] = zone + pool, err := pgxpool.NewWithConfig(ctx, config) + if err != nil { + t.Fatal(err) + } + t.Cleanup(pool.Close) + r := &taskRepository{db: pool} + warmup := &warmupRepository{db: pool} + if err = r.DeferWarmupDispatch(ctx, f.task, f.sender, worker, nonce, time.Now().Add(-time.Second)); err != nil { + t.Fatal(err) + } + if claimed, err := r.ClaimWarmupTask(ctx, f.task, time.Now()); err != nil || !claimed { + t.Fatalf("reclaim: claimed=%v error=%v", claimed, err) + } + f.token(t, "", "Pinned diagnostic", false) + if _, err = r.AuthorizeWarmupDispatch(ctx, f.task, f.sender, worker); err != nil { + t.Fatal(err) + } + var differentDay, utcCharge bool + if err = pool.QueryRow(ctx, `SELECT CURRENT_DATE<>(NOW() AT TIME ZONE 'UTC')::date,warmup_charged_date=(NOW() AT TIME ZONE 'UTC')::date FROM warmup_tasks WHERE task_id=$1`, f.task).Scan(&differentDay, &utcCharge); err != nil || !differentDay || !utcCharge { + t.Fatalf("charge follows session rather than UTC: differentDay=%v utcCharge=%v error=%v", differentDay, utcCharge, err) + } + if err = warmup.IncrementDailyCount(ctx, f.sender, time.Now()); err != nil { + t.Fatal(err) + } + if err = warmup.IncrementReplyCount(ctx, f.sender, time.Now()); err != nil { + t.Fatal(err) + } + if err = warmup.FailWarmupSend(ctx, f.sender, f.task, time.Now(), "Send failed", "Provider refused"); err != nil { + t.Fatal(err) + } + var utcSent, utcReplies, sessionRows int + if err = pool.QueryRow(ctx, `SELECT SUM(emails_sent) FILTER(WHERE date=(NOW() AT TIME ZONE 'UTC')::date),SUM(emails_replied) FILTER(WHERE date=(NOW() AT TIME ZONE 'UTC')::date),COUNT(*) FILTER(WHERE date=CURRENT_DATE) FROM warmup_statistics WHERE email_account_id=$1`, f.sender).Scan(&utcSent, &utcReplies, &sessionRows); err != nil || utcSent != 1 || utcReplies != 1 || sessionRows != 0 { + t.Fatalf("non-UTC refund changed another day: utcSent=%d utcReplies=%d sessionRows=%d error=%v", utcSent, utcReplies, sessionRows, err) + } +} + +func TestLiveWarmupChargeProofRollsBackWhenCounterWriteFails(t *testing.T) { + f, r, worker, _ := newWarmupChargeFixture(t, false) + ctx := t.Context() + queries := []struct { + sql string + arg uuid.UUID + }{ + {`UPDATE warmup_statistics SET emails_sent=2147483647 WHERE email_account_id=$1`, f.sender}, + {`UPDATE warmup_tasks SET dispatch_nonce=NULL,dispatch_worker_id=NULL,warmup_charged_date=NULL,warmup_reply_charged=NULL,warmup_refunded_at=NULL WHERE task_id=$1`, f.task}, + {`UPDATE tasks SET status='active',completed_at=NULL,send_result_state=NULL WHERE id=$1`, f.task}, + {`UPDATE email_accounts SET send_recovery_hold=false,send_recovery_reason=NULL,send_recovery_task_id=NULL WHERE id=$1`, f.sender}, + } + for _, q := range queries { + if _, err := f.pool.Exec(ctx, q.sql, q.arg); err != nil { + t.Fatal(err) + } + } + _, err := r.AuthorizeWarmupDispatch(ctx, f.task, f.sender, worker) + var sqlError *pgconn.PgError + if !errors.As(err, &sqlError) || sqlError.Code != "22003" { + t.Fatalf("expected counter overflow, got %v", err) + } + var status string + var charged, completed, nonce, held bool + if err = f.pool.QueryRow(ctx, `SELECT t.status,t.completed_at IS NOT NULL,w.warmup_charged_date IS NOT NULL OR w.warmup_reply_charged IS NOT NULL,w.dispatch_nonce IS NOT NULL,ea.send_recovery_hold FROM tasks t JOIN warmup_tasks w ON w.task_id=t.id JOIN email_accounts ea ON ea.id=t.email_account_id WHERE t.id=$1`, f.task).Scan(&status, &completed, &charged, &nonce, &held); err != nil { + t.Fatal(err) + } + if status != "active" || completed || charged || nonce || held { + t.Fatalf("counter failure committed charge authority: status=%s completed=%v charged=%v nonce=%v held=%v", status, completed, charged, nonce, held) + } +} + +func TestLiveWarmupCancellationPathsRefundOnlyProvenCharge(t *testing.T) { + for _, path := range []string{"defer", "cancel", "recover"} { + for _, proven := range []bool{false, true} { + name := path + "/legacy" + if proven { + name = path + "/charged" + } + t.Run(name, func(t *testing.T) { + f, r, worker, nonce := newWarmupChargeFixture(t, false) + ctx := t.Context() + if !proven { + if _, err := f.pool.Exec(ctx, `UPDATE warmup_tasks SET warmup_charged_date=NULL,warmup_reply_charged=NULL WHERE task_id=$1`, f.task); err != nil { + t.Fatal(err) + } + } + var err error + switch path { + case "defer": + err = r.DeferWarmupDispatch(ctx, f.task, f.sender, worker, nonce, time.Now()) + case "cancel": + reservation := &OutboundReservation{TaskID: f.task, MailboxID: f.sender, OrganizationID: f.org, WorkerID: worker, Provider: models.InboxProviderSMTPIMAP, Recipients: []string{"pl-" + f.recipient.String()[:8] + "@test.local"}} + var reserved uuid.UUID + if reserved, err = r.ReserveOutbound(ctx, *reservation); err == nil { + err = r.CancelOutbound(ctx, f.task, f.sender, worker, reserved) + } + case "recover": + var retired bool + retired, err = r.retireUnstartedWarmupDispatch(ctx, f.task, f.sender, time.Now().Add(time.Second)) + if !retired { + t.Fatalf("unstarted task not retired: %v", err) + } + } + if err != nil { + t.Fatal(err) + } + var sent int + var refunded bool + if err = f.pool.QueryRow(ctx, `SELECT ws.emails_sent,w.warmup_refunded_at IS NOT NULL FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id JOIN warmup_statistics ws ON ws.email_account_id=t.email_account_id WHERE t.id=$1`, f.task).Scan(&sent, &refunded); err != nil { + t.Fatal(err) + } + want := 1 + if proven { + want = 0 + } + if sent != want || refunded != proven { + t.Fatalf("refund without proof or skipped valid charge: sent=%d refunded=%v proven=%v", sent, refunded, proven) + } + }) + } + } +} diff --git a/internal/repository/warmup_daily_usage_live_test.go b/internal/repository/warmup_daily_usage_live_test.go index 5c706580f..8df2836ae 100644 --- a/internal/repository/warmup_daily_usage_live_test.go +++ b/internal/repository/warmup_daily_usage_live_test.go @@ -118,6 +118,7 @@ func TestLiveGiveBackDailySendReversesADispatchedWarmupSend(t *testing.T) { f.exec(`INSERT INTO tasks (id, task_type, email_account_id, status, message_id, completed_at) VALUES ($1, 'warmup', $2, 'completed', '', $3)`, id, f.account, day) } + f.exec(`INSERT INTO warmup_tasks(task_id,warmup_charged_date,warmup_reply_charged) VALUES($1,($2::timestamptz AT TIME ZONE 'UTC')::date,false),($3,($2::timestamptz AT TIME ZONE 'UTC')::date,true)`, newThread, day, reply) // Only a nonzero conversation turn consumed the reply counter. f.exec(`INSERT INTO warmup_tokens (token, task_id, sender_account_id, recipient_account_id, conversation_turn) VALUES ($1, $2, $3, $3, 0)`, uuid.New(), newThread, f.account) @@ -172,6 +173,7 @@ func TestLiveGiveBackDailySendLeavesOtherDaysAlone(t *testing.T) { taskID := uuid.New() f.exec(`INSERT INTO tasks (id, task_type, email_account_id, status, message_id, completed_at) VALUES ($1, 'warmup', $2, 'completed', '', $3)`, taskID, f.account, counted) + f.exec(`INSERT INTO warmup_tasks(task_id,warmup_charged_date,warmup_reply_charged) VALUES($1,($2::timestamptz AT TIME ZONE 'UTC')::date,false)`, taskID, counted) f.exec(`INSERT INTO warmup_statistics (email_account_id, date, emails_sent, target_volume) VALUES ($1, DATE($2), 5, 10)`, f.account, counted) f.exec(`INSERT INTO warmup_statistics (email_account_id, date, emails_sent, target_volume) diff --git a/internal/repository/warmup_dispatch.go b/internal/repository/warmup_dispatch.go index a6b2be4dd..7032fd77d 100644 --- a/internal/repository/warmup_dispatch.go +++ b/internal/repository/warmup_dispatch.go @@ -47,10 +47,12 @@ func (r *taskRepository) AuthorizeWarmupDispatch(ctx context.Context, task, mail } nonce := uuid.New() var saved uuid.UUID - err = tx.QueryRow(ctx, `UPDATE warmup_tasks w SET dispatch_nonce=$4,dispatch_worker_id=$3 + err = tx.QueryRow(ctx, `UPDATE warmup_tasks w SET dispatch_nonce=$4,dispatch_worker_id=$3, + warmup_charged_date=(NOW() AT TIME ZONE 'UTC')::date,warmup_reply_charged=(w.parent_task_id IS NOT NULL),warmup_refunded_at=NULL FROM tasks t JOIN email_accounts ea ON ea.id=t.email_account_id JOIN fleet_nodes n ON n.id=ea.worker_id WHERE w.task_id=t.id AND t.id=$1 AND t.email_account_id=$2 AND ea.worker_id=$3 AND t.status='active' - AND w.lineage_version=1 AND w.dispatch_nonce IS NULL AND ea.status='active' + AND w.lineage_version=1 AND w.dispatch_nonce IS NULL + AND (w.warmup_charged_date IS NULL OR w.warmup_refunded_at IS NOT NULL) AND ea.status='active' AND NOT ea.send_recovery_hold AND (ea.send_cooldown_until IS NULL OR ea.send_cooldown_until<=NOW()) AND n.role='worker' AND n.active AND n.last_seen_at>NOW()-$5::interval AND n.warmup_send_protocol>=1 RETURNING w.dispatch_nonce`, task, mailbox, worker, nonce, WorkerLivenessWindow).Scan(&saved) @@ -64,7 +66,7 @@ func (r *taskRepository) AuthorizeWarmupDispatch(ctx context.Context, task, mail return uuid.Nil, err } if _, err = tx.Exec(ctx, `INSERT INTO warmup_statistics(email_account_id,date,emails_sent,emails_replied,target_volume) - SELECT $2,CURRENT_DATE,1,CASE WHEN parent_task_id IS NULL THEN 0 ELSE 1 END,0 FROM warmup_tasks WHERE task_id=$1 + SELECT $2,warmup_charged_date,1,CASE WHEN warmup_reply_charged THEN 1 ELSE 0 END,0 FROM warmup_tasks WHERE task_id=$1 ON CONFLICT(email_account_id,date) DO UPDATE SET emails_sent=warmup_statistics.emails_sent+1,emails_replied=warmup_statistics.emails_replied+EXCLUDED.emails_replied`, task, mailbox); err != nil { return uuid.Nil, err } @@ -173,16 +175,15 @@ func (r *taskRepository) DeferWarmupDispatch(ctx context.Context, task, mailbox, if _, err = tx.Exec(ctx, `SELECT pg_advisory_xact_lock(hashtextextended($1,269))`, mailbox.String()); err != nil { return err } - var reply bool - var completed time.Time + var deferred uuid.UUID err = tx.QueryRow(ctx, `UPDATE warmup_tasks w SET dispatch_nonce=NULL,dispatch_worker_id=NULL,schedule_revision=schedule_revision+1 FROM tasks t WHERE w.task_id=t.id AND t.id=$1 AND t.email_account_id=$2 AND w.dispatch_worker_id=$3 AND w.dispatch_nonce=$4 AND w.dispatch_started_at IS NULL AND w.dispatch_result IS NULL AND t.send_result_applied_at IS NULL - RETURNING w.parent_task_id IS NOT NULL,t.completed_at`, task, mailbox, worker, nonce).Scan(&reply, &completed) + RETURNING w.task_id`, task, mailbox, worker, nonce).Scan(&deferred) if err != nil { return err } - if _, err = tx.Exec(ctx, `UPDATE warmup_statistics SET emails_sent=GREATEST(emails_sent-1,0),emails_replied=GREATEST(emails_replied-CASE WHEN $3 THEN 1 ELSE 0 END,0) WHERE email_account_id=$1 AND date=DATE($2)`, mailbox, completed, reply); err != nil { + if err = refundWarmupCharge(ctx, tx, task, mailbox); err != nil { return err } if _, err = tx.Exec(ctx, `DELETE FROM warmup_tokens WHERE task_id=$1`, task); err != nil { diff --git a/internal/repository/warmup_dispatch_recovery.go b/internal/repository/warmup_dispatch_recovery.go index 7a634eae7..fe6a176e3 100644 --- a/internal/repository/warmup_dispatch_recovery.go +++ b/internal/repository/warmup_dispatch_recovery.go @@ -91,10 +91,7 @@ func (r *taskRepository) retireUnstartedWarmupDispatch(ctx context.Context, task if tag.RowsAffected() == 0 { return false, nil } - if _, err = tx.Exec(ctx, `UPDATE warmup_statistics SET emails_sent=GREATEST(emails_sent-1,0), - emails_replied=GREATEST(emails_replied-CASE WHEN w.parent_task_id IS NULL THEN 0 ELSE 1 END,0) - FROM warmup_tasks w JOIN tasks t ON t.id=w.task_id - WHERE t.id=$2 AND warmup_statistics.email_account_id=$1 AND date=DATE(t.completed_at)`, mailbox, task); err != nil { + if err = refundWarmupCharge(ctx, tx, task, mailbox); err != nil { return false, err } if _, err = tx.Exec(ctx, `DELETE FROM warmup_tokens WHERE task_id=$1`, task); err != nil { diff --git a/internal/tasks/campaign_send_live_test.go b/internal/tasks/campaign_send_live_test.go index ec5be6ae5..ebe6b1ac2 100644 --- a/internal/tasks/campaign_send_live_test.go +++ b/internal/tasks/campaign_send_live_test.go @@ -37,6 +37,47 @@ type breakingProgressRepo struct { breakStamp bool } +type rejectingCampaignTrackingRepo struct { + repository.TaskRepository +} + +func (r rejectingCampaignTrackingRepo) UpdateCampaignTaskTracking(context.Context, uuid.UUID, uuid.UUID, uuid.UUID) error { + return errors.New("simulated durable tracking write failure") +} + +func TestLiveCampaignSendRequiresDurableIntentBeforeReservation(t *testing.T) { + f := newSendFixture(t) + id := uuid.New() + at := time.Now().Add(-time.Minute) + if ok, err := f.svc.taskRepo.CreateTaskWithLock(t.Context(), &Task{ID: id, TaskType: "campaign", EmailAccountID: f.mailbox, Status: "pending", ScheduledAt: &at}, &CampaignTask{TaskID: id, CampaignID: &f.campaign}); err != nil || !ok { + t.Fatal(ok, err) + } + original := f.svc.taskRepo + f.svc.taskRepo = rejectingCampaignTrackingRepo{TaskRepository: original} + if xerr := f.svc.HandleCampaignTask(&proto.ProcessTask{TaskId: id.String()}); xerr == nil { + t.Fatal("failed durable intent write allowed execution") + } + if f.sender.count() != 0 { + t.Fatal("send dispatched without durable send intent") + } + var dispatched int + if err := f.pool.QueryRow(t.Context(), `SELECT COUNT(*) FROM campaign_contact_progress WHERE campaign_id=$1 AND dispatched_at IS NOT NULL`, f.campaign).Scan(&dispatched); err != nil || dispatched != 0 { + t.Fatal("send reserved without durable intent", dispatched, err) + } + ct, err := original.GetCampaignTask(t.Context(), id) + if err != nil || ct.DispatchIntent != repository.CampaignDispatchWakeup { + t.Fatal("unsent wakeup intent changed", ct, err) + } + f.svc.taskRepo = original + if xerr := f.svc.HandleCampaignTask(&proto.ProcessTask{TaskId: id.String()}); xerr != nil { + t.Fatal(xerr) + } + ct, err = original.GetCampaignTask(t.Context(), id) + if err != nil || ct.DispatchIntent != repository.CampaignDispatchSend || f.sender.count() != 1 { + t.Fatal("durable send intent not recorded before successful retry", ct, err) + } +} + func (b *breakingProgressRepo) RecordEmailSent(ctx context.Context, campaignID, contactID, sequenceID uuid.UUID) error { if b.breakStamp { return errors.New("simulated transient database error on the progress write") diff --git a/internal/tasks/campaign_task.go b/internal/tasks/campaign_task.go index 2d4cf2a5c..6ee4d36ed 100644 --- a/internal/tasks/campaign_task.go +++ b/internal/tasks/campaign_task.go @@ -504,8 +504,10 @@ func (s *tasksService) HandleCampaignTask(task *proto.ProcessTask) (result *errx // This allows the tracking consumer to find the correct contact/sequence when // processing open/click events from the tracking pixel service if err := s.taskRepo.UpdateCampaignTaskTracking(ctx, taskID, contact.ID, sequence.ID); err != nil { - // Log but don't fail - tracking can still work via fallback methods - log.Warn().Err(err).Str("campaign_id", campaign.ID.String()).Str("task_id", taskID.String()).Msg("Failed to update campaign task tracking") + errs.CaptureException(err) + s.retryCampaignTickLater(ctx, taskRecord) + executionStatus = "failed" + return errx.InternalError() } // stop_on_reply is enforced inside FindRoutedPairs (STEP 6), and it is now diff --git a/internal/tasks/warmup_dispatch_recovery_live_test.go b/internal/tasks/warmup_dispatch_recovery_live_test.go index 7da290d2c..666d6c8fc 100644 --- a/internal/tasks/warmup_dispatch_recovery_live_test.go +++ b/internal/tasks/warmup_dispatch_recovery_live_test.go @@ -70,9 +70,9 @@ func newWarmupRecoveryFixture(t *testing.T) *warmupRecoveryFixture { f.exec(t, `UPDATE email_accounts SET worker_id=$2,warmup=NOW()-INTERVAL '30 days',warmup_pool_type='free',send_recovery_hold=true,send_recovery_reason='unknown',send_recovery_task_id=$3 WHERE id=$1`, f.sender.ID, f.worker, f.task) f.exec(t, `INSERT INTO tasks(id,email_account_id,task_type,status,message_id,completed_at,send_reserved_at,send_result_state,send_executor_nonce,send_executor_worker,send_recipients) VALUES($1,$2,'warmup','completed','',NOW()-INTERVAL '1 hour',NOW()-INTERVAL '1 hour','unknown',$3,$4,ARRAY[$5])`, f.task, f.sender.ID, f.nonce, f.worker, "pick-"+f.atWorkspace.String()[:8]+"@acme.test") - f.exec(t, `INSERT INTO warmup_tasks(task_id,lineage_version,dispatch_nonce,dispatch_worker_id) VALUES($1,1,$2,$3)`, f.task, f.nonce, f.worker) + f.exec(t, `INSERT INTO warmup_tasks(task_id,lineage_version,dispatch_nonce,dispatch_worker_id,warmup_charged_date,warmup_reply_charged) VALUES($1,1,$2,$3,(NOW() AT TIME ZONE 'UTC')::date,false)`, f.task, f.nonce, f.worker) f.exec(t, `INSERT INTO warmup_tokens(token,task_id,sender_account_id,recipient_account_id) VALUES($1,$2,$3,$4)`, uuid.New(), f.task, f.sender.ID, f.atWorkspace) - f.exec(t, `INSERT INTO warmup_statistics(email_account_id,date,emails_sent,emails_replied,target_volume) VALUES($1,CURRENT_DATE,1,0,8)`, f.sender.ID) + f.exec(t, `INSERT INTO warmup_statistics(email_account_id,date,emails_sent,emails_replied,target_volume) VALUES($1,(NOW() AT TIME ZONE 'UTC')::date,1,0,8)`, f.sender.ID) t.Cleanup(func() { _, _ = f.pool.Exec(context.Background(), `DELETE FROM warmup_received WHERE sender_account_id=$1`, f.sender.ID) _, _ = f.pool.Exec(context.Background(), `DELETE FROM fleet_nodes WHERE id=$1`, f.worker) @@ -94,7 +94,7 @@ func (f *warmupRecoveryFixture) checkRetired(t *testing.T) { var released, applied, held bool var sent, tokens int if err := f.pool.QueryRow(t.Context(), `SELECT t.status,t.send_result_state,t.send_released_at IS NOT NULL,t.send_result_applied_at IS NOT NULL,ea.send_recovery_hold, - (SELECT emails_sent FROM warmup_statistics WHERE email_account_id=ea.id AND date=CURRENT_DATE),(SELECT count(*) FROM warmup_tokens WHERE task_id=t.id) + (SELECT emails_sent FROM warmup_statistics WHERE email_account_id=ea.id AND date=(NOW() AT TIME ZONE 'UTC')::date),(SELECT count(*) FROM warmup_tokens WHERE task_id=t.id) FROM tasks t JOIN email_accounts ea ON ea.id=t.email_account_id WHERE t.id=$1`, f.task).Scan(&status, &state, &released, &applied, &held, &sent, &tokens); err != nil { t.Fatal(err) } @@ -190,7 +190,7 @@ func TestLiveWarmupRecoveryPreservesExecutionAndReceiptEvidence(t *testing.T) { } var held bool var sent int - if err := f.pool.QueryRow(t.Context(), `SELECT send_recovery_hold,(SELECT emails_sent FROM warmup_statistics WHERE email_account_id=$1 AND date=CURRENT_DATE) FROM email_accounts WHERE id=$1`, f.sender.ID).Scan(&held, &sent); err != nil || !held || sent != 1 { + if err := f.pool.QueryRow(t.Context(), `SELECT send_recovery_hold,(SELECT emails_sent FROM warmup_statistics WHERE email_account_id=$1 AND date=(NOW() AT TIME ZONE 'UTC')::date) FROM email_accounts WHERE id=$1`, f.sender.ID).Scan(&held, &sent); err != nil || !held || sent != 1 { t.Fatalf("held=%t sent=%d err=%v", held, sent, err) } })