diff --git a/cmd/e2a/main.go b/cmd/e2a/main.go index 7e13c1726..09eefa404 100644 --- a/cmd/e2a/main.go +++ b/cmd/e2a/main.go @@ -121,6 +121,7 @@ func main() { flag.IntVar(&spFlags.activeBillingContract, "active-billing-contract", -1, "verified active billing contract level") flag.StringVar(&spFlags.rollbackBillingDigest, "rollback-billing-digest", "", "verified rollback billing image digest") flag.IntVar(&spFlags.rollbackBillingContract, "rollback-billing-contract", -1, "verified rollback billing contract level") + flag.BoolVar(&spFlags.reconcile, "reconcile-legacy-sending-jobs", false, "stamp a sending operation reference onto every pending provider-submitting job enqueued without one (cancelling orphans whose source row is gone), print counts, then exit; nonzero unless every job was decided") flag.BoolVar(&spFlags.capabilities, "print-capabilities", false, "print the machine-readable capability marker (contract level, policy source, operator commitments), then exit") flag.StringVar(&spFlags.reason, "reason", "", "nonblank reason recorded in the audit row of a sending-protection mutation") flag.Parse() @@ -365,6 +366,9 @@ func main() { }) outboundJobs := outboundSending.jobs registrars = append(registrars, outboundJobs) + // Platform mail the API sends itself (public feedback) crosses the same + // seam with tokens from the same gate. + sendingGate, providerSubmitter := outboundSending.gate, outboundSending.submitter registrars = append(registrars, sendramp.NewMaintenanceJobs(rampStore)) // Queue depth/age gauges: a 30s maintenance periodic sampling river_job // per queue+state (docs/observability.md). @@ -389,10 +393,17 @@ func main() { // later via SetDeliverer — mirrors inbound's late-bound Processor. Gated on the // same relay+public-URL config as the notifier itself; when unconfigured, no jobs // register and the hold takes the plain path (no notification). - var notifyJobs *hitlnotify.Jobs notifierEnabled := cfg.OutboundSMTP.FromDomain != "" && cfg.HTTP.PublicURL != "" - if notifierEnabled { - notifyJobs = hitlnotify.NewJobs(store) + notification := newNotificationJobs(notificationDeps{ + store: store, + pool: pool, + gate: sendingGate, + metrics: metrics, + hitlEnabled: notifierEnabled, + webhookEnabled: cfg.OutboundSMTP.FromDomain != "", + }) + notifyJobs := notification.hitl + if notifyJobs != nil { registrars = append(registrars, notifyJobs) } @@ -405,9 +416,8 @@ func main() { // (generic dashboard copy instead of a link). When unconfigured, no jobs // register and the sweep transitions state without notifications // (pre-feature behavior). - var webhookNotifyJobs *webhooknotify.Jobs - if cfg.OutboundSMTP.FromDomain != "" { - webhookNotifyJobs = webhooknotify.NewJobs(store).WithMetrics(metrics) + webhookNotifyJobs := notification.webhook + if webhookNotifyJobs != nil { registrars = append(registrars, webhookNotifyJobs) } @@ -697,7 +707,7 @@ func main() { // unreachable in practice — kept as a defensive guard against future drift. log.Printf("[hitl] notifier disabled: notification job pipeline not registered") } else { - notifier := hitlnotify.New(store, smtpRelay, approvalSigner, cfg.OutboundSMTP.FromDomain, cfg.Notifications.FromAddress, cfg.Notifications.ReplyTo, cfg.HTTP.PublicURL).WithDKIM(store) + notifier := hitlnotify.New(store, providerSubmitter, approvalSigner, cfg.OutboundSMTP.FromDomain, cfg.Notifications.FromAddress, cfg.Notifications.ReplyTo, cfg.HTTP.PublicURL).WithDKIM(store) // Late-bind the concrete Deliverer onto the registered NotifyWorker (which // has been running since jobsClient.Start; jobs enqueued before this bind // simply retry) and give the hold path its accept-tx enqueuer. The HTTP @@ -718,7 +728,7 @@ func main() { // a BYODKIM custom from-address domain is signed here or not at all. // Fail-open — no stored key (self-host default) sends unsigned. if webhookNotifyJobs != nil { - whNotifier := webhooknotify.New(store, smtpRelay, cfg.OutboundSMTP.FromDomain, cfg.Notifications.FromAddress, cfg.Notifications.ReplyTo, cfg.HTTP.PublicURL).WithDKIM(store) + whNotifier := webhooknotify.New(store, providerSubmitter, cfg.OutboundSMTP.FromDomain, cfg.Notifications.FromAddress, cfg.Notifications.ReplyTo, cfg.HTTP.PublicURL).WithDKIM(store) webhookNotifyJobs.SetDeliverer(whNotifier) log.Printf("[webhook-notify] enabled (from=%s)", whNotifier.FromAddress()) } else { @@ -833,6 +843,7 @@ func main() { // The outbound accept-tx enqueuer is mandatory: DeliverOutbound always // persists+enqueues and returns accepted before provider submission. api.SetOutboundEnqueuer(outboundJobs) + outboundSending.armAPI(api) // Slices 6 + 7: customer-facing events API needs the raw pool to // query webhook_events and write webhook_subscriber_deliveries on // replay. Kept as a separate setter so a future refactor can route diff --git a/cmd/e2a/outbound_wiring.go b/cmd/e2a/outbound_wiring.go index 462cdea66..7ec9b6b38 100644 --- a/cmd/e2a/outbound_wiring.go +++ b/cmd/e2a/outbound_wiring.go @@ -4,9 +4,12 @@ import ( "github.com/jackc/pgx/v5/pgxpool" "github.com/tokencanopy/e2a/internal/agent" + "github.com/tokencanopy/e2a/internal/hitlnotify" + "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/outbound" "github.com/tokencanopy/e2a/internal/outboundsend" "github.com/tokencanopy/e2a/internal/sendingpolicy" + "github.com/tokencanopy/e2a/internal/webhooknotify" ) // outboundSendingDeps is everything the outbound composition root needs. It @@ -49,3 +52,45 @@ func newOutboundSending(d outboundSendingDeps) outboundSending { WithRateGate(d.rate) return outboundSending{gate: gate, submitter: submitter, jobs: jobs} } + +// notificationDeps is what the notification composition needs: the same gate +// and pool the customer path uses, plus the two config gates main applies. +type notificationDeps struct { + store *identity.Store + pool *pgxpool.Pool + gate sendingpolicy.Gate + metrics webhooknotify.Metrics + hitlEnabled bool // outbound_smtp.from_domain and http.public_url set + webhookEnabled bool // outbound_smtp.from_domain set +} + +// notificationJobs are the two notification job bundles, nil when their +// feature is unconfigured (no worker registers, the sweep/hold take the +// plain path). +type notificationJobs struct { + hitl *hitlnotify.Jobs + webhook *webhooknotify.Jobs +} + +// newNotificationJobs composes the notification bundles over the ONE gate. +// Every enqueue prepares a customer_notification operation in the source +// transaction and every worker execution authorizes through the gate; a +// bundle built any other way would fail closed at runtime (empty token) with +// an error that says nothing about wiring, which is why the composition is +// factored here and pinned by the wiring test. +func newNotificationJobs(d notificationDeps) notificationJobs { + var n notificationJobs + if d.hitlEnabled { + n.hitl = hitlnotify.NewJobs(d.store).WithGate(d.gate, d.pool) + } + if d.webhookEnabled { + n.webhook = webhooknotify.NewJobs(d.store).WithMetrics(d.metrics).WithGate(d.gate, d.pool) + } + return n +} + +// armAPI hands the API the authorized seam for the platform mail it sends +// itself (public feedback). +func (s outboundSending) armAPI(api *agent.API) { + api.SetProviderSubmitter(s.submitter, s.gate) +} diff --git a/cmd/e2a/sending_policy.go b/cmd/e2a/sending_policy.go index 02c2efc41..a9d8d733c 100644 --- a/cmd/e2a/sending_policy.go +++ b/cmd/e2a/sending_policy.go @@ -24,6 +24,7 @@ type sendingProtectionFlags struct { register bool attest bool capabilities bool + reconcile bool expectedGeneration int64 expectedPolicySHA string @@ -40,12 +41,12 @@ type sendingProtectionFlags struct { } func (f *sendingProtectionFlags) commandRequested() bool { - return f.inspect || f.activate || f.register || f.attest || f.capabilities + return f.inspect || f.activate || f.register || f.attest || f.capabilities || f.reconcile } func (f *sendingProtectionFlags) selectedCount() int { n := 0 - for _, set := range []bool{f.inspect, f.activate, f.register, f.attest, f.capabilities} { + for _, set := range []bool{f.inspect, f.activate, f.register, f.attest, f.capabilities, f.reconcile} { if set { n++ } @@ -105,6 +106,8 @@ func runSendingProtectionCommand(ctx context.Context, cfg *config.Config, pool * return runRuntimeAttest(ctx, module, f, stdout) case f.capabilities: return runPrintCapabilities(source, secrets, stdout) + case f.reconcile: + return runReconcileLegacySendingJobs(ctx, pool, sendingpolicy.NewGate(pool, secrets, source, policy), stdout) } return errors.New("no sending-protection command selected") } diff --git a/cmd/e2a/sending_policy_test.go b/cmd/e2a/sending_policy_test.go index aa30648eb..9a8beb0d7 100644 --- a/cmd/e2a/sending_policy_test.go +++ b/cmd/e2a/sending_policy_test.go @@ -312,6 +312,17 @@ func TestSendingProtectionCommands(t *testing.T) { } }) + t.Run("reconcile-legacy-sending-jobs dispatches", func(t *testing.T) { + resetRiverJobs(t, pool) + out, err := run(&sendingProtectionFlags{reconcile: true}) + if err != nil { + t.Fatalf("reconcile: %v", err) + } + if !strings.Contains(out, "scanned: 0") || !strings.Contains(out, "remaining: 0") { + t.Errorf("reconcile output = %q", out) + } + }) + t.Run("print-capabilities", func(t *testing.T) { clearEnvForTest(t) out, err := run(&sendingProtectionFlags{capabilities: true}) diff --git a/cmd/e2a/sending_policy_wiring_test.go b/cmd/e2a/sending_policy_wiring_test.go index a7eebc1a4..206113818 100644 --- a/cmd/e2a/sending_policy_wiring_test.go +++ b/cmd/e2a/sending_policy_wiring_test.go @@ -8,10 +8,12 @@ import ( "github.com/riverqueue/river" + "github.com/tokencanopy/e2a/internal/agent" "github.com/tokencanopy/e2a/internal/config" "github.com/tokencanopy/e2a/internal/outbound" "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/testutil/testdb" + "github.com/tokencanopy/e2a/internal/usage" ) // TestSendingPolicyWiring builds the production outbound composition from @@ -76,3 +78,50 @@ func TestSendingPolicyWiring(t *testing.T) { t.Fatal("a never-prepared operation resolved") } } + +// TestNotificationAndPlatformMailWiring pins the three composition-root +// edges the AST closure guard cannot see: both notification bundles hold the +// gate (so their enqueues prepare operations and their workers authorize), +// and the API holds the submitter + gate for public feedback. Dropping any +// of them fails closed at runtime with an opaque "authorization required" +// error; this is where it fails loudly instead. +func TestNotificationAndPlatformMailWiring(t *testing.T) { + pool := testdb.TestDB(t) + relay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{Host: "relay.invalid", Port: 587, FromDomain: "test.e2a.dev"}) + composed := newOutboundSending(outboundSendingDeps{ + pool: pool, + relay: relay, + secrets: sendingpolicy.Secrets{}, + source: sendingpolicy.PolicySourceConfig, + policy: sendingpolicy.DisabledPolicy(), + }) + + n := newNotificationJobs(notificationDeps{pool: pool, gate: composed.gate, hitlEnabled: true, webhookEnabled: true}) + if n.hitl == nil || n.hitl.Gate() != composed.gate { + t.Fatal("hitl notification bundle does not hold the composed gate") + } + if n.webhook == nil || n.webhook.Gate() != composed.gate { + t.Fatal("webhook notification bundle does not hold the composed gate") + } + // The registered workers are what run; they must carry the gate too. + if w := n.hitl.NotifyWorker(); w == nil || w.Gate() != composed.gate { + t.Fatal("hitl notify worker registered without the gate") + } + if w := n.webhook.NotifyWorker(); w == nil || w.Gate() != composed.gate { + t.Fatal("webhook notify worker registered without the gate") + } + + off := newNotificationJobs(notificationDeps{pool: pool, gate: composed.gate}) + if off.hitl != nil || off.webhook != nil { + t.Fatal("unconfigured notifications must register nothing") + } + + api := agent.NewAPI(nil, nil, relay, nil, usage.NewNoopUsageTracker(), "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + if api.ProviderSubmitterWired() { + t.Fatal("a fresh API must not claim a submitter") + } + composed.armAPI(api) + if !api.ProviderSubmitterWired() { + t.Fatal("armAPI did not hand the API the submitter and gate") + } +} diff --git a/cmd/e2a/sending_reconcile.go b/cmd/e2a/sending_reconcile.go new file mode 100644 index 000000000..b56a888b4 --- /dev/null +++ b/cmd/e2a/sending_reconcile.go @@ -0,0 +1,275 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "fmt" + "io" + "slices" + + "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" + "github.com/riverqueue/river/rivertype" + + "github.com/tokencanopy/e2a/internal/hitlnotify" + "github.com/tokencanopy/e2a/internal/jobs" + "github.com/tokencanopy/e2a/internal/outboundsend" + "github.com/tokencanopy/e2a/internal/sendingpolicy" + "github.com/tokencanopy/e2a/internal/webhooknotify" +) + +// legacySendingJobKinds are the River job kinds that submit mail to the +// provider and therefore must carry a sending operation reference. A job of +// one of these kinds without an operation_ref was enqueued by a pre-floor +// slot: the worker resolves it at fire time, but an operator can also settle +// the backlog up front with -reconcile-legacy-sending-jobs so the cutover +// leaves no job whose attribution is decided later than its enqueue. +var legacySendingJobKinds = []string{ + outboundsend.OutboundSendArgs{}.Kind(), + hitlnotify.HITLNotifyArgs{}.Kind(), + webhooknotify.WebhookNotifyArgs{}.Kind(), +} + +// legacyReconcileStates are the job states a reconcile touches: those River +// may still pick up. A running job is left to its worker, and a finalized job +// (completed, cancelled, discarded) has nothing left to authorize. +var legacyReconcileStates = []string{ + string(rivertype.JobStateAvailable), + string(rivertype.JobStatePending), + string(rivertype.JobStateRetryable), + string(rivertype.JobStateScheduled), +} + +// conformingReferenceSQL is true for a river_job row whose operation_ref +// already has the shape its worker derives: the message id for a send, the +// op_hitl_ / op_wh_ derivations for the two notice kinds. +// +// COALESCE keeps the predicate two-valued: a reference with no id (or a JSON +// null) would otherwise make the LIKE NULL and drop the row from a NOT scan. +const conformingReferenceSQL = `COALESCE( + (args ? 'operation_ref') AND ( + (kind = 'outbound_send') + OR (kind = 'hitl_notify' AND args->'operation_ref'->>'id' LIKE 'op\_hitl\_%') + OR (kind = 'webhook_notify' AND args->'operation_ref'->>'id' LIKE 'op\_wh\_%') + ), false)` + +// legacyReconcileCounts is the operator-facing summary of one reconcile pass. +type legacyReconcileCounts struct { + Scanned int + Stamped int + Cancelled int + Paused int + Skipped int // moved on by a worker between the scan and the job's own transaction + Failed int +} + +// remaining is the number of scanned jobs that still carry no operation +// reference after the pass: those the resolver could not decide. A job whose +// account is paused is deliberately left for the worker's hold path, so it is +// not counted as remaining. +func (c legacyReconcileCounts) remaining() int { return c.Failed } + +// runReconcileLegacySendingJobs stamps an operation reference onto every +// pending provider-submitting job that has none, cancelling the ones whose +// source row no longer exists. Each job is handled in its own transaction, +// through exactly the Prepare path its enqueue would have used, so a stamped +// job and a natively enqueued job authorize identically. Exit status is +// nonzero unless every scanned job was decided. +func runReconcileLegacySendingJobs(ctx context.Context, pool *pgxpool.Pool, gate sendingpolicy.Gate, stdout io.Writer) error { + client, err := jobs.New(pool, jobs.Config{}) + if err != nil { + return fmt.Errorf("river client: %w", err) + } + // A job is legacy when it carries no reference, or a pre-derivation one: + // migration 113 stamped adopted notify jobs with op_, which the + // workers now re-key at fire time; this command does the same up front. + rows, err := pool.Query(ctx, ` + SELECT id, kind, args + FROM river_job + WHERE kind = ANY($1) + AND state = ANY($2) + AND NOT `+conformingReferenceSQL+` + ORDER BY id`, legacySendingJobKinds, legacyReconcileStates) + if err != nil { + return fmt.Errorf("scan legacy sending jobs: %w", err) + } + type legacyJob struct { + id int64 + kind string + args []byte + } + var pending []legacyJob + for rows.Next() { + var j legacyJob + if err := rows.Scan(&j.id, &j.kind, &j.args); err != nil { + rows.Close() + return fmt.Errorf("scan legacy sending job: %w", err) + } + pending = append(pending, j) + } + rows.Close() + if err := rows.Err(); err != nil { + return fmt.Errorf("scan legacy sending jobs: %w", err) + } + + var counts legacyReconcileCounts + for _, j := range pending { + counts.Scanned++ + outcome, err := reconcileLegacySendingJob(ctx, pool, client, gate, j.id, j.kind, j.args) + if err != nil { + counts.Failed++ + fmt.Fprintf(stdout, "job %d (%s): %v\n", j.id, j.kind, err) + continue + } + switch outcome { + case legacyOutcomeStamped: + counts.Stamped++ + case legacyOutcomeCancelled: + counts.Cancelled++ + case legacyOutcomePaused: + counts.Paused++ + case legacyOutcomeSkipped: + counts.Skipped++ + } + } + + fmt.Fprintf(stdout, "scanned: %d\n", counts.Scanned) + fmt.Fprintf(stdout, "stamped: %d\n", counts.Stamped) + fmt.Fprintf(stdout, "cancelled: %d\n", counts.Cancelled) + fmt.Fprintf(stdout, "paused: %d (left unstamped for the worker's hold path; rerun after the account resumes)\n", counts.Paused) + fmt.Fprintf(stdout, "skipped: %d (picked up by a worker meanwhile; the worker resolves them)\n", counts.Skipped) + fmt.Fprintf(stdout, "failed: %d\n", counts.Failed) + fmt.Fprintf(stdout, "remaining: %d (undecided; nonzero exit)\n", counts.remaining()) + if counts.remaining() != 0 { + return fmt.Errorf("%d legacy sending job(s) could not be reconciled", counts.remaining()) + } + return nil +} + +type legacyOutcome int + +const ( + legacyOutcomeStamped legacyOutcome = iota + 1 + legacyOutcomeCancelled + legacyOutcomePaused + legacyOutcomeSkipped +) + +// reconcileLegacySendingJob decides one job inside one transaction: the +// source row is locked by the Prepare call, the reference is stamped (or the +// orphan cancelled) in the same transaction, and a failure rolls both back so +// a rerun sees the job untouched. +func reconcileLegacySendingJob(ctx context.Context, pool *pgxpool.Pool, client *jobs.Client, gate sendingpolicy.Gate, jobID int64, kind string, rawArgs []byte) (legacyOutcome, error) { + tx, err := pool.Begin(ctx) + if err != nil { + return 0, fmt.Errorf("begin: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + + // Re-read the job under its row lock: the scan ran outside this + // transaction, and a worker may have claimed the job (or resolved and + // stamped it itself) since. Deciding a job a worker now owns would + // prepare beside it and could cancel it mid-flight, so anything that + // left the reconcilable states is skipped and left to that worker. The + // lock also serializes against the worker's own stamp. + var state string + var conforming bool + err = tx.QueryRow(ctx, + `SELECT state, `+conformingReferenceSQL+` FROM river_job WHERE id = $1 FOR UPDATE`, jobID, + ).Scan(&state, &conforming) + if errors.Is(err, pgx.ErrNoRows) { + return legacyOutcomeSkipped, nil + } + if err != nil { + return 0, fmt.Errorf("lock job: %w", err) + } + if conforming || !slices.Contains(legacyReconcileStates, state) { + return legacyOutcomeSkipped, nil + } + + var ref sendingpolicy.OperationRef + var cancelReason string + switch kind { + case outboundsend.OutboundSendArgs{}.Kind(): + // Decode only the source fields: a malformed stored reference is + // exactly what this command replaces, so it must not fail decoding. + var args struct { + MessageID string `json:"message_id"` + } + if err := json.Unmarshal(rawArgs, &args); err != nil { + return 0, fmt.Errorf("decode args: %w", err) + } + decision, prepared, err := gate.PrepareExternalTx(ctx, tx, args.MessageID) + switch { + case errors.Is(err, sendingpolicy.ErrSourceUnavailable): + cancelReason = "legacy source unavailable" + case err != nil: + return 0, err + case decision == sendingpolicy.AcceptanceSendingPaused: + // The worker's hold path owns a paused account: it records the + // hold on the message and waits for the operator. Nothing to + // stamp yet; the rerun after the resume picks it up. + return legacyOutcomePaused, nil + case prepared.IsZero(): + // The only accepted shape with no operation is an exact + // self-send, which never enqueues; a queued job that resolves to + // nothing cannot be authorized by any worker. + cancelReason = "message has no provider operation" + default: + ref = prepared + } + case hitlnotify.HITLNotifyArgs{}.Kind(): + var args struct { + MessageID string `json:"message_id"` + } + if err := json.Unmarshal(rawArgs, &args); err != nil { + return 0, fmt.Errorf("decode args: %w", err) + } + ref, cancelReason, err = prepareLegacyNotification(ctx, tx, gate, sendingpolicy.NewHITLNotificationRef(args.MessageID)) + if err != nil { + return 0, err + } + case webhooknotify.WebhookNotifyArgs{}.Kind(): + var args struct { + WebhookID string `json:"webhook_id"` + NotifyKind string `json:"kind"` + } + if err := json.Unmarshal(rawArgs, &args); err != nil { + return 0, fmt.Errorf("decode args: %w", err) + } + ref, cancelReason, err = prepareLegacyNotification(ctx, tx, gate, sendingpolicy.NewWebhookHealthNotificationRef(args.WebhookID, args.NotifyKind)) + if err != nil { + return 0, err + } + default: + return 0, fmt.Errorf("unexpected job kind %q", kind) + } + + outcome := legacyOutcomeStamped + if cancelReason != "" { + if err := client.CancelTx(ctx, tx, jobID); err != nil { + return 0, fmt.Errorf("cancel (%s): %w", cancelReason, err) + } + outcome = legacyOutcomeCancelled + } else if err := jobs.SetJobArg(ctx, tx, jobID, "operation_ref", ref); err != nil { + // Unconditional: the row is locked and known non-conforming, and a + // pre-derivation reference must be replaced, not kept. + return 0, err + } + if err := tx.Commit(ctx); err != nil { + return 0, fmt.Errorf("commit: %w", err) + } + return outcome, nil +} + +func prepareLegacyNotification(ctx context.Context, tx pgx.Tx, gate sendingpolicy.Gate, nref sendingpolicy.NotificationRef) (sendingpolicy.OperationRef, string, error) { + ref, err := gate.PrepareNotificationTx(ctx, tx, nref) + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + return sendingpolicy.OperationRef{}, "legacy source unavailable", nil + } + if err != nil { + return sendingpolicy.OperationRef{}, "", err + } + return ref, "", nil +} diff --git a/cmd/e2a/sending_reconcile_test.go b/cmd/e2a/sending_reconcile_test.go new file mode 100644 index 000000000..213c3d1a0 --- /dev/null +++ b/cmd/e2a/sending_reconcile_test.go @@ -0,0 +1,302 @@ +package main + +import ( + "bytes" + "context" + "strings" + "testing" + + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/tokencanopy/e2a/internal/identity" + "github.com/tokencanopy/e2a/internal/jobs" + "github.com/tokencanopy/e2a/internal/sendingpolicy" + "github.com/tokencanopy/e2a/internal/testutil" + "github.com/tokencanopy/e2a/internal/webhooknotify" +) + +// insertLegacyJob enqueues a River job the way a pre-floor slot did: the +// args carry no operation_ref. Raw SQL on purpose — the typed enqueuers +// always prepare a reference now, so the only way to produce a legacy job in +// a test is to write one the old way. +func insertLegacyJob(t *testing.T, pool *pgxpool.Pool, kind, args string) int64 { + t.Helper() + var id int64 + if err := pool.QueryRow(context.Background(), + `INSERT INTO river_job (args, kind, max_attempts) VALUES ($1::jsonb, $2, 3) RETURNING id`, + args, kind).Scan(&id); err != nil { + t.Fatalf("insert legacy %s job: %v", kind, err) + } + return id +} + +// resetRiverJobs empties the shared per-package river_job table: the test DB +// helper leaves River's tables alone, so legacy rows one test writes would +// otherwise be scanned by the next. +func resetRiverJobs(t *testing.T, pool *pgxpool.Pool) { + t.Helper() + if err := jobs.Migrate(context.Background(), pool); err != nil { + t.Fatalf("jobs.Migrate: %v", err) + } + if _, err := pool.Exec(context.Background(), `TRUNCATE river_job RESTART IDENTITY`); err != nil { + t.Fatalf("reset river_job: %v", err) + } +} + +func legacyJobState(t *testing.T, pool *pgxpool.Pool, id int64) (state, opID string) { + t.Helper() + if err := pool.QueryRow(context.Background(), + `SELECT state, COALESCE(args->'operation_ref'->>'id', '') FROM river_job WHERE id = $1`, id, + ).Scan(&state, &opID); err != nil { + t.Fatalf("read job %d: %v", id, err) + } + return state, opID +} + +func seedReconcileSource(t *testing.T, pool *pgxpool.Pool, store *identity.Store, slug string) (*identity.Message, *identity.Webhook) { + t.Helper() + ctx := context.Background() + user, err := store.CreateOrGetUser(ctx, "owner-"+slug+"@reviewer.test", "Owner", "google-reconcile-"+slug) + if err != nil { + t.Fatal(err) + } + if _, err := store.ClaimOrCreateDomain(ctx, slug+".bot.test", user.ID); err != nil { + t.Fatal(err) + } + if err := store.VerifyDomain(ctx, slug+".bot.test", user.ID); err != nil { + t.Fatal(err) + } + a, err := store.CreateAgent(ctx, "bot@"+slug+".bot.test", slug+".bot.test", "", "https://example.com/webhook", "", user.ID) + if err != nil { + t.Fatal(err) + } + msg, err := store.CreatePendingOutboundMessage(ctx, a.ID, + []string{"alice@example.com"}, nil, nil, + "Held draft", "body", "", nil, "send", "conv_"+slug, "", "", 3600) + if err != nil { + t.Fatal(err) + } + wh, err := store.CreateWebhook(ctx, user.ID, "https://hooks.example.com/e2a", "", + []string{"email.received"}, identity.WebhookFilters{}) + if err != nil { + t.Fatal(err) + } + // The sweep stamps the warning episode before it enqueues the notice; + // a legacy warning job's operation is keyed by that stamp. + if _, err := pool.Exec(ctx, `UPDATE webhooks SET warn_notified_at = now() WHERE id = $1`, wh.ID); err != nil { + t.Fatal(err) + } + wh, err = store.GetWebhookByIDInternal(ctx, wh.ID) + if err != nil { + t.Fatal(err) + } + return msg, wh +} + +// TestReconcileLegacySendingJobs: every pending provider-submitting job +// without an operation reference is decided in one pass — stamped when its +// source row exists, cancelled when it does not — and a second pass finds +// nothing left. A job River already finalized is out of scope. +func TestReconcileLegacySendingJobs(t *testing.T) { + ctx := context.Background() + pool := testutil.TestDB(t) + resetRiverJobs(t, pool) + store := identity.NewStore(pool) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + msg, wh := seedReconcileSource(t, pool, store, "reconcile") + + sendLive := insertLegacyJob(t, pool, "outbound_send", `{"message_id":"`+msg.ID+`"}`) + sendGone := insertLegacyJob(t, pool, "outbound_send", `{"message_id":"msg_does_not_exist"}`) + hitlLive := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`"}`) + whLive := insertLegacyJob(t, pool, "webhook_notify", `{"webhook_id":"`+wh.ID+`","kind":"warning"}`) + whGone := insertLegacyJob(t, pool, "webhook_notify", `{"webhook_id":"wh_does_not_exist","kind":"disabled"}`) + finalized := insertLegacyJob(t, pool, "outbound_send", `{"message_id":"msg_finalized"}`) + if _, err := pool.Exec(ctx, `UPDATE river_job SET state = 'completed', finalized_at = now() WHERE id = $1`, finalized); err != nil { + t.Fatal(err) + } + other := insertLegacyJob(t, pool, "outbound_terminal_reconcile", `{"message_id":"`+msg.ID+`"}`) + + var out bytes.Buffer + if err := runReconcileLegacySendingJobs(ctx, pool, gate, &out); err != nil { + t.Fatalf("reconcile: %v\nOUTPUT:\n%s", err, out.String()) + } + for _, want := range []string{"scanned: 5", "stamped: 3", "cancelled: 2", "failed: 0", "remaining: 0"} { + if !strings.Contains(out.String(), want) { + t.Errorf("output missing %q:\n%s", want, out.String()) + } + } + + if state, op := legacyJobState(t, pool, sendLive); state != "available" || op != msg.ID { + t.Errorf("live send job: state=%s op=%q, want available with the message id", state, op) + } + if state, op := legacyJobState(t, pool, hitlLive); state != "available" || op != sendingpolicy.HITLNotificationOperationID(msg.ID) { + t.Errorf("live hitl job: state=%s op=%q, want available with the message's notification operation", state, op) + } + if state, op := legacyJobState(t, pool, whLive); state != "available" || op != webhooknotify.ExpectedOperationID(wh, webhooknotify.KindWarning) { + t.Errorf("live webhook job: state=%s op=%q, want available with the warning episode's operation", state, op) + } + for name, id := range map[string]int64{"send": sendGone, "webhook": whGone} { + if state, op := legacyJobState(t, pool, id); state != "cancelled" || op != "" { + t.Errorf("orphan %s job: state=%s op=%q, want cancelled and unstamped", name, state, op) + } + } + if state, op := legacyJobState(t, pool, finalized); state != "completed" || op != "" { + t.Errorf("finalized job touched: state=%s op=%q", state, op) + } + if state, op := legacyJobState(t, pool, other); state != "available" || op != "" { + t.Errorf("non-submitting kind touched: state=%s op=%q", state, op) + } + + // The stamped reference must round-trip: the same bytes a native enqueue + // would have written, so a worker reading it authorizes identically. + var raw []byte + if err := pool.QueryRow(ctx, `SELECT args->'operation_ref' FROM river_job WHERE id = $1`, sendLive).Scan(&raw); err != nil { + t.Fatal(err) + } + var ref sendingpolicy.OperationRef + if err := ref.UnmarshalJSON(raw); err != nil || ref.ID() != msg.ID { + t.Fatalf("stamped reference does not decode to the message operation: err=%v id=%q", err, ref.ID()) + } + + out.Reset() + if err := runReconcileLegacySendingJobs(ctx, pool, gate, &out); err != nil { + t.Fatalf("second pass: %v", err) + } + if !strings.Contains(out.String(), "scanned: 0") { + t.Errorf("second pass should find nothing:\n%s", out.String()) + } +} + +// TestReconcileLegacySendingJobsReportsUndecided: a job the resolver cannot +// decide is reported, left untouched, and makes the command exit nonzero so a +// cutover script cannot mistake a partial pass for a clean one. +func TestReconcileLegacySendingJobsReportsUndecided(t *testing.T) { + ctx := context.Background() + pool := testutil.TestDB(t) + resetRiverJobs(t, pool) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + broken := insertLegacyJob(t, pool, "outbound_send", `{"message_id":123}`) + + var out bytes.Buffer + err := runReconcileLegacySendingJobs(ctx, pool, gate, &out) + if err == nil || !strings.Contains(err.Error(), "1 legacy sending job(s) could not be reconciled") { + t.Fatalf("err = %v, want the undecided count", err) + } + for _, want := range []string{"failed: 1", "remaining: 1", "decode args"} { + if !strings.Contains(out.String(), want) { + t.Errorf("output missing %q:\n%s", want, out.String()) + } + } + if state, op := legacyJobState(t, pool, broken); state != "available" || op != "" { + t.Errorf("undecided job touched: state=%s op=%q", state, op) + } +} + +// TestReconcileLegacySendingJobsLeavesClaimedJobsToTheirWorker: a job that +// left the reconcilable states (a worker claimed it) or was stamped by its +// worker between the scan and its own transaction is skipped untouched — no +// second operation, no cancel under a running worker. +func TestReconcileLegacySendingJobsLeavesClaimedJobsToTheirWorker(t *testing.T) { + ctx := context.Background() + pool := testutil.TestDB(t) + resetRiverJobs(t, pool) + store := identity.NewStore(pool) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + msg, _ := seedReconcileSource(t, pool, store, "claimed") + + running := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`"}`) + if _, err := pool.Exec(ctx, `UPDATE river_job SET state = 'running', attempted_at = now() WHERE id = $1`, running); err != nil { + t.Fatal(err) + } + orphanRunning := insertLegacyJob(t, pool, "outbound_send", `{"message_id":"msg_gone"}`) + if _, err := pool.Exec(ctx, `UPDATE river_job SET state = 'running', attempted_at = now() WHERE id = $1`, orphanRunning); err != nil { + t.Fatal(err) + } + + var ops int + if err := pool.QueryRow(ctx, `SELECT count(*) FROM sending_provider_operations`).Scan(&ops); err != nil { + t.Fatal(err) + } + var out bytes.Buffer + if err := runReconcileLegacySendingJobs(ctx, pool, gate, &out); err != nil { + t.Fatalf("reconcile: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "scanned: 0") { + t.Fatalf("running jobs must not be scanned:\n%s", out.String()) + } + for name, id := range map[string]int64{"running": running, "orphan running": orphanRunning} { + if state, op := legacyJobState(t, pool, id); state != "running" || op != "" { + t.Errorf("%s job touched: state=%s op=%q", name, state, op) + } + } + var after int + if err := pool.QueryRow(ctx, `SELECT count(*) FROM sending_provider_operations`).Scan(&after); err != nil { + t.Fatal(err) + } + if after != ops { + t.Errorf("operations minted for jobs the command did not own: %d → %d", ops, after) + } + + // The per-job transaction re-checks under lock: simulate a worker that + // claimed the job after the scan by driving the per-job step directly. + claimed := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`"}`) + if _, err := pool.Exec(ctx, `UPDATE river_job SET state = 'running', attempted_at = now() WHERE id = $1`, claimed); err != nil { + t.Fatal(err) + } + client, err := jobs.New(pool, jobs.Config{}) + if err != nil { + t.Fatal(err) + } + outcome, err := reconcileLegacySendingJob(ctx, pool, client, gate, claimed, "hitl_notify", []byte(`{"message_id":"`+msg.ID+`"}`)) + if err != nil || outcome != legacyOutcomeSkipped { + t.Fatalf("claimed job: outcome=%v err=%v, want skipped", outcome, err) + } + stampedByWorker := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`","operation_ref":{"v":1,"id":"op_hitl_`+msg.ID+`"}}`) + outcome, err = reconcileLegacySendingJob(ctx, pool, client, gate, stampedByWorker, "hitl_notify", []byte(`{"message_id":"`+msg.ID+`"}`)) + if err != nil || outcome != legacyOutcomeSkipped { + t.Fatalf("already stamped job: outcome=%v err=%v, want skipped", outcome, err) + } +} + +// TestReconcileLegacySendingJobsReKeysPreDerivationReferences: a notify job +// migration 113 stamped with op_ is scanned, re-resolved through the +// Prepare path and re-keyed to the derived id; a conforming one is left alone. +func TestReconcileLegacySendingJobsReKeysPreDerivationReferences(t *testing.T) { + ctx := context.Background() + pool := testutil.TestDB(t) + resetRiverJobs(t, pool) + store := identity.NewStore(pool) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + msg, wh := seedReconcileSource(t, pool, store, "rekey") + + md5Hitl := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`","operation_ref":{"v":1,"id":"op_0123456789abcdef0123456789abcdef"}}`) + md5Wh := insertLegacyJob(t, pool, "webhook_notify", `{"webhook_id":"`+wh.ID+`","kind":"warning","operation_ref":{"v":1,"id":"op_fedcba9876543210fedcba9876543210"}}`) + conforming := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`","operation_ref":{"v":1,"id":"`+sendingpolicy.HITLNotificationOperationID(msg.ID)+`"}}`) + // A malformed reference (no id) must be scanned and re-keyed, not hidden + // by three-valued logic in the scan predicate. + noID := insertLegacyJob(t, pool, "hitl_notify", `{"message_id":"`+msg.ID+`","operation_ref":{"v":1}}`) + send := insertLegacyJob(t, pool, "outbound_send", `{"message_id":"`+msg.ID+`","operation_ref":{"v":1,"id":"`+msg.ID+`"}}`) + + var out bytes.Buffer + if err := runReconcileLegacySendingJobs(ctx, pool, gate, &out); err != nil { + t.Fatalf("reconcile: %v\n%s", err, out.String()) + } + if !strings.Contains(out.String(), "scanned: 3") || !strings.Contains(out.String(), "stamped: 3") { + t.Fatalf("want the two md5-keyed jobs and the id-less one scanned and re-keyed:\n%s", out.String()) + } + if _, op := legacyJobState(t, pool, noID); op != sendingpolicy.HITLNotificationOperationID(msg.ID) { + t.Errorf("id-less hitl job op = %q, want the derived id", op) + } + if _, op := legacyJobState(t, pool, md5Hitl); op != sendingpolicy.HITLNotificationOperationID(msg.ID) { + t.Errorf("hitl job op = %q, want the derived id", op) + } + if _, op := legacyJobState(t, pool, md5Wh); op != webhooknotify.ExpectedOperationID(wh, webhooknotify.KindWarning) { + t.Errorf("webhook job op = %q, want the warning episode's derived id", op) + } + if _, op := legacyJobState(t, pool, conforming); op != sendingpolicy.HITLNotificationOperationID(msg.ID) { + t.Errorf("conforming hitl job touched: %q", op) + } + if _, op := legacyJobState(t, pool, send); op != msg.ID { + t.Errorf("conforming send job touched: %q", op) + } +} diff --git a/docs/design/async-message-pipeline.md b/docs/design/async-message-pipeline.md index 7aff7c3de..89c4f08f0 100644 --- a/docs/design/async-message-pipeline.md +++ b/docs/design/async-message-pipeline.md @@ -292,3 +292,63 @@ respectively. An account pause has no clock and starts no hold, but a deadline already running keeps running. Terminal reconciliation is settlement-only: an evidence-settled row also settles the attempt that dialed (`Gate.SettleOperation`). + +## Addendum (2026-09-05): every provider call is an authorized attempt (B7) + +Slice B7 closed the seam B5 opened. `outbound.SMTPRelay` no longer exports a +send method: the only way to open a socket to the provider is +`ProviderSubmitter.SubmitOnce` with a `sendingpolicy.ProviderAuthorization`, +and `internal/outbound`'s tracked-closure test parses every production file +to keep it that way (no `net/smtp` import and no call to the relay's socket +core outside the named exceptions). The paths that used to bypass the gate now +cross it: + +- **HITL approval notifications** (`internal/hitlnotify`) and **webhook health + notices** (`internal/webhooknotify`): the enqueue prepares a + `customer_notification` operation in the same transaction as the source + row (`PrepareNotificationTx`, charged to the triggering account, shared + reputation class) and stamps it on the job. The operation id is derived + from the source — `op_hitl_` for an approval request, + `op_wh___` for a health notice, where the + episode is the `warn_notified_at` / `auto_disabled_at` stamp the sweep + wrote in the same transaction — so preparing the same source twice yields + one operation, and the worker cancels a job whose reference is a derived + id for any other source (the binding the message worker enforces). A + reference of any other shape — migration 113 stamped adopted notify jobs + with `op_` — is a pre-derivation reference for the job's own source: + the worker re-resolves it through the same Prepare path and replaces it + once (`jobs.SetJobArg`), so an upgrade that crosses v1.8.7 drains its + backlog instead of cancelling it. The worker + order is compose → Reserve → early hold → ConsumeAttempt → authorized + submit: every fallible, provider-free step (owner lookup, token signing, + MIME, DKIM) runs before an ordinal is charged, and the token is consumed + immediately before the socket opens. A job from a pre-floor slot resolves + its operation at fire time and stamps it once (`jobs.StampJobArg`); with a + source-derived id a repeat resolve is harmless. A health notice older than + seven days is dropped rather than left snoozing behind a pause. +- **Public feedback mail** (`POST /api/feedback`): the operation is keyed by + a server-minted submission id and its envelope is the configured notify + set, never the request, so the form cannot become a relay. No queue owns + this path, so its bounded in-request retry loop is the whole envelope and + every physical attempt is its own charged ordinal; a definite rejection and + a lost acceptance both stop the loop. + +Operators cutting over a slot with a queued backlog run +`e2a -reconcile-legacy-sending-jobs`: it stamps an operation onto every +pending `outbound_send` / `hitl_notify` / `webhook_notify` job that has none +or a pre-derivation one, through exactly the Prepare path its enqueue would +have used, cancels the +ones whose source row is gone, and exits nonzero unless every scanned job was +decided. Each job is re-read under its row lock inside its own transaction, +so one a worker claimed after the scan is skipped and left to that worker; +a paused account's message job is also left unstamped for the worker's hold +path. The workers resolve legacy jobs themselves, so the command is a +convenience for a clean cutover, not a prerequisite. + +Two consequences worth knowing. Notification and feedback mail now cross the +same submitter as customer mail, so it carries `X-SES-CONFIGURATION-SET` +and SES publishes delivery feedback for it; none of it correlates to a +message row, and the SNS consumer acks it as unknown (a log line, no +suppression). And the closure guard fences `net/smtp` and the SES v2 SDK +import; a send through some other HTTP provider API would be a new +dependency, which is where review catches it. diff --git a/internal/agent/api.go b/internal/agent/api.go index fe16be741..c94be1ed7 100644 --- a/internal/agent/api.go +++ b/internal/agent/api.go @@ -2,6 +2,8 @@ package agent import ( "context" + "crypto/rand" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -37,6 +39,7 @@ import ( "github.com/tokencanopy/e2a/internal/outboundsend" "github.com/tokencanopy/e2a/internal/piguard" "github.com/tokencanopy/e2a/internal/ratelimit" + "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/telemetry" "github.com/tokencanopy/e2a/internal/usage" "github.com/tokencanopy/e2a/internal/webhook" @@ -178,7 +181,12 @@ type API struct { // identically to a wire roundtrip of the same message. inboundScreen *piguard.Engine smtpRelay *outbound.SMTPRelay - userAuth *auth.UserAuth + // submitter and gate are the authorized provider seam for platform mail + // this API sends itself (public feedback). Wired via SetProviderSubmitter; + // unset means the platform cannot send feedback mail. + submitter *outbound.ProviderSubmitter + gate sendingpolicy.Gate + userAuth *auth.UserAuth // oidcAuth wires optional, generic OpenID Connect browser login. Nil means // both OIDC routes are absent; it is independent of legacy Google login. oidcAuth *auth.OIDCAuth @@ -1629,6 +1637,17 @@ func (a *API) DeliverOutbound(ctx context.Context, user *identity.User, agent *i return &OutboundResult{MessageID: accepted.ID, Status: acceptStatus, ScheduledAt: scheduledAt, SentAs: comp.SentAs, Method: comp.Method}, nil } +// SetProviderSubmitter wires the authorized provider seam and the gate that +// issues its tokens, for the platform mail this API sends on its own behalf. +func (a *API) SetProviderSubmitter(submitter *outbound.ProviderSubmitter, gate sendingpolicy.Gate) { + a.submitter = submitter + a.gate = gate +} + +// ProviderSubmitterWired reports whether the platform-mail seam is armed, for +// the composition root's wiring test. +func (a *API) ProviderSubmitterWired() bool { return a.submitter != nil && a.gate != nil } + // SendTestCore accepts (or HITL-holds) a platform test email to the agent's // own address. HTTP-free; shared by the legacy handler and the v1 layer. The // caller has already authed, resolved + owned the agent, domain-verified, @@ -1925,7 +1944,7 @@ func (a *API) handleFeedback(w http.ResponseWriter, r *http.Request) { // notification reaches them directly; compose-layer header sanitization // neutralizes any CR/LF in that user-controlled value. func (a *API) sendFeedbackEmail(ctx context.Context, title, category, message, submitterEmail, ghNote string, to, cc []string) error { - if a.smtpRelay == nil || !a.smtpRelay.Configured() || a.fromDomain == "" { + if a.submitter == nil || a.gate == nil || a.smtpRelay == nil || !a.smtpRelay.Configured() || a.fromDomain == "" { return fmt.Errorf("outbound SMTP relay not configured") } @@ -1951,12 +1970,95 @@ func (a *API) sendFeedbackEmail(ctx context.Context, title, category, message, s rcpts = append(rcpts, to...) rcpts = append(rcpts, cc...) - // Send (not SendOnce) — no job queue owns retries for this path, so the - // relay's own transient-4xx backoff is the only retry envelope. - if _, err := a.smtpRelay.SendWithContext(ctx, from, rcpts, raw); err != nil { - return fmt.Errorf("smtp send: %w", err) + // No job queue owns retries for this path, so the request's bounded + // retry loop is the whole envelope — and every physical attempt is its + // own charged ordinal: Reserve, ConsumeAttempt, one authorized submit. + // The operation is keyed by a server-minted submission id and its + // envelope is configuration, never the request, so the form cannot + // become an open relay however it is retried. What goes on the wire is + // the token's canonical recipient set: the configured TO/CC lists may + // overlap or differ in case, and the seam refuses an envelope whose raw + // count disagrees with its normalized one. + submissionID, err := feedbackSubmissionID() + if err != nil { + return err } - return nil + ref, err := a.gate.PreparePublicFeedback(ctx, sendingpolicy.NewPublicFeedbackRef(submissionID, rcpts)) + if err != nil { + return fmt.Errorf("prepare feedback operation: %w", err) + } + var last error + for attempt := 0; attempt < feedbackSendAttempts; attempt++ { + if attempt > 0 { + select { + case <-ctx.Done(): + return errors.Join(ctx.Err(), last) + case <-time.After(feedbackRetryBackoff[attempt-1]): + } + } + early, attemptRef, err := a.gate.Reserve(ctx, ref) + if err != nil { + return fmt.Errorf("reserve feedback attempt: %w", err) + } + if !early.Allow { + return fmt.Errorf("feedback send held by sending policy: %s", early.Reason) + } + decision, auth, err := a.gate.ConsumeAttempt(ctx, attemptRef) + if err != nil { + // The ordinal is reserved and nothing will Reserve it again on + // this path (the request ends here), so give its units back + // rather than leave them charged until midnight. Best effort: + // the gate's day-scoped expiry is the backstop. + releaseCtx, cancel := context.WithTimeout(context.WithoutCancel(ctx), feedbackReleaseTimeout) + cerr := a.gate.CancelAttempt(releaseCtx, attemptRef) + cancel() + if cerr != nil { + log.Printf("[feedback] release reserved attempt after authorize error: %v", cerr) + } + return fmt.Errorf("authorize feedback attempt: %w", err) + } + if !decision.Allow || auth == nil { + return fmt.Errorf("feedback send held by sending policy: %s", decision.Reason) + } + _, err = a.submitter.SubmitOnce(ctx, *auth, outbound.Envelope{From: from, Recipients: auth.AuthorizedRecipients(), Message: raw}) + if err == nil { + return nil + } + last = err + if outbound.IsPermanentSMTPError(err) || errors.Is(err, outbound.ErrProviderAcceptanceUnknown) { + // Definite rejection: retrying resends nothing. Acceptance unknown: + // the provider may hold the message, and a retry would be a + // duplicate copy of platform mail nobody asked for twice. + break + } + } + return fmt.Errorf("smtp send: %w", last) +} + +// feedbackSendAttempts bounds the physical submissions one feedback request +// may make; feedbackRetryBackoff paces them. The sleeps total six of the ten +// seconds feedbackEmailTimeout allows, so all four attempts fit only when +// the relay answers quickly (a refused connection, a fast 4xx); a relay that +// hangs consumes the budget on its first attempt and the deadline exit +// reports that attempt's error. Each attempt is a distinct charged ordinal +// on the feedback operation. +const feedbackSendAttempts = 4 + +var feedbackRetryBackoff = []time.Duration{time.Second, 2 * time.Second, 3 * time.Second} + +// feedbackReleaseTimeout bounds the best-effort release of a reserved +// attempt after an authorize error, so a database that is already failing +// cannot park the handler goroutine. +const feedbackReleaseTimeout = 2 * time.Second + +// feedbackSubmissionID mints the server-side identity one feedback request's +// operation is keyed by. +func feedbackSubmissionID() (string, error) { + var b [12]byte + if _, err := rand.Read(b[:]); err != nil { + return "", fmt.Errorf("feedback submission id: %w", err) + } + return hex.EncodeToString(b[:]), nil } // splitFeedbackAddrs parses a comma-separated address list from env config, diff --git a/internal/agent/api_test.go b/internal/agent/api_test.go index 702208f9e..2235cf563 100644 --- a/internal/agent/api_test.go +++ b/internal/agent/api_test.go @@ -8,6 +8,7 @@ import ( "mime" "net/http" "net/http/httptest" + "sort" "strings" "testing" @@ -19,6 +20,7 @@ import ( "github.com/tokencanopy/e2a/internal/idempotency" "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/testutil" "github.com/tokencanopy/e2a/internal/usage" ) @@ -139,6 +141,9 @@ func setupAPIWithSMTP(t *testing.T) (*httptest.Server, *identity.Store, *pgxpool sender := outbound.NewSender(smtpRelay, "test.e2a.dev") noopUsage := usage.NewNoopUsageTracker() api := agent.NewAPI(store, sender, smtpRelay, nil, noopUsage, "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + // Platform mail (public feedback) crosses the authorized provider seam. + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + api.SetProviderSubmitter(outbound.NewProviderSubmitter(smtpRelay, gate), gate) api.SetIdempotencyStore(idempotency.NewStore(pool)) router := mux.NewRouter() api.RegisterRoutes(router) @@ -364,8 +369,12 @@ func TestFeedback_EmailNotification(t *testing.T) { if m.From != "noreply@test.e2a.dev" { t.Errorf("envelope from = %q, want noreply@test.e2a.dev", m.From) } - wantRcpts := []string{"feedback-to@example.com", "feedback-cc@example.com"} - if strings.Join(m.Recipients, ",") != strings.Join(wantRcpts, ",") { + // RCPT TO is issued from the token's canonical (sorted) recipient set, + // so compare as a set: the wire order is the seam's, not the form's. + gotRcpts := append([]string(nil), m.Recipients...) + sort.Strings(gotRcpts) + wantRcpts := []string{"feedback-cc@example.com", "feedback-to@example.com"} + if strings.Join(gotRcpts, ",") != strings.Join(wantRcpts, ",") { t.Errorf("recipients = %v, want %v", m.Recipients, wantRcpts) } for _, want := range []string{ @@ -413,6 +422,8 @@ func TestFeedback_AllChannelsFail_500(t *testing.T) { deadRelay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{Host: "127.0.0.1", Port: 1}) sender := outbound.NewSender(deadRelay, "test.e2a.dev") api := agent.NewAPI(store, sender, deadRelay, nil, usage.NewNoopUsageTracker(), "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + deadGate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + api.SetProviderSubmitter(outbound.NewProviderSubmitter(deadRelay, deadGate), deadGate) router := mux.NewRouter() api.RegisterRoutes(router) server := httptest.NewServer(router) diff --git a/internal/agent/feedback_github_test.go b/internal/agent/feedback_github_test.go index 3afd92def..fe6d0f8fa 100644 --- a/internal/agent/feedback_github_test.go +++ b/internal/agent/feedback_github_test.go @@ -22,6 +22,8 @@ import ( "github.com/gorilla/mux" "github.com/tokencanopy/e2a/internal/config" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" + "github.com/tokencanopy/e2a/internal/testutil/testdb" "github.com/tokencanopy/e2a/internal/usage" ) @@ -132,6 +134,7 @@ func TestFeedbackGitHubTimeoutStillDeliversEmail(t *testing.T) { relay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{Host: smtpHost, Port: smtpPort}) sender := outbound.NewSender(relay, "test.e2a.dev") api := NewAPI(nil, sender, relay, nil, usage.NewNoopUsageTracker(), "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + wireFeedbackSubmitter(t, api, relay) router := mux.NewRouter() api.RegisterRoutes(router) server := httptest.NewServer(router) @@ -188,6 +191,7 @@ func TestFeedbackEmailTimeoutReturnsAfterGitHubDelivery(t *testing.T) { relay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{Host: smtpHost, Port: smtpPort}) sender := outbound.NewSender(relay, "test.e2a.dev") api := NewAPI(nil, sender, relay, nil, usage.NewNoopUsageTracker(), "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + wireFeedbackSubmitter(t, api, relay) router := mux.NewRouter() api.RegisterRoutes(router) server := httptest.NewServer(router) @@ -239,6 +243,7 @@ func TestFeedbackNoRepoConfigured_RefusesToFileRatherThanDefaultingToOperatorRep relay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{Host: smtpHost, Port: smtpPort}) sender := outbound.NewSender(relay, "test.e2a.dev") api := NewAPI(nil, sender, relay, nil, usage.NewNoopUsageTracker(), "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + wireFeedbackSubmitter(t, api, relay) router := mux.NewRouter() api.RegisterRoutes(router) server := httptest.NewServer(router) @@ -402,3 +407,12 @@ func TestFeedbackGitHubClient_Precedence(t *testing.T) { t.Errorf("bad app key: got client=%v err=%v, want nil,error", c, err) } } + +// wireFeedbackSubmitter gives an API the authorized provider seam the feedback +// path submits through, backed by a disabled-policy gate on the test DB. +func wireFeedbackSubmitter(t *testing.T, api *API, relay *outbound.SMTPRelay) { + t.Helper() + pool := testdb.TestDB(t) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + api.SetProviderSubmitter(outbound.NewProviderSubmitter(relay, gate), gate) +} diff --git a/internal/agent/feedback_seam_test.go b/internal/agent/feedback_seam_test.go new file mode 100644 index 000000000..3318963d9 --- /dev/null +++ b/internal/agent/feedback_seam_test.go @@ -0,0 +1,282 @@ +package agent + +import ( + "bufio" + "context" + "errors" + "fmt" + "net" + "strings" + "sync" + "testing" + "time" + + "github.com/jackc/pgx/v5/pgxpool" + + "github.com/tokencanopy/e2a/internal/config" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" + "github.com/tokencanopy/e2a/internal/testutil/testdb" + "github.com/tokencanopy/e2a/internal/usage" +) + +// scriptedSMTP answers one connection per script entry. The entry is the +// reply the server gives after the message body: an SMTP code ("250", "451", +// "554") or "drop", which closes the socket without any reply — the lost-250 +// shape the relay reports as ErrProviderAcceptanceUnknown. +type scriptedSMTP struct { + host string + port int + + mu sync.Mutex + messages []string + rcpts [][]string // RCPT TO per connection, in wire order + conns int +} + +func startScriptedSMTP(t *testing.T, script ...string) *scriptedSMTP { + t.Helper() + listener, err := net.Listen("tcp", "127.0.0.1:0") + if err != nil { + t.Fatal(err) + } + t.Cleanup(func() { _ = listener.Close() }) + addr := listener.Addr().(*net.TCPAddr) + s := &scriptedSMTP{host: addr.IP.String(), port: addr.Port} + + go func() { + for _, reply := range script { + conn, acceptErr := listener.Accept() + if acceptErr != nil { + return + } + s.mu.Lock() + s.conns++ + s.mu.Unlock() + s.serve(conn, reply) + } + }() + return s +} + +func (s *scriptedSMTP) serve(conn net.Conn, reply string) { + defer conn.Close() + reader := bufio.NewReader(conn) + fmt.Fprint(conn, "220 scripted ready\r\n") + var data []string + var rcpts []string + inData := false + for { + line, err := reader.ReadString('\n') + if err != nil { + return + } + line = strings.TrimRight(line, "\r\n") + if inData { + if line != "." { + data = append(data, line) + continue + } + s.mu.Lock() + s.messages = append(s.messages, strings.Join(data, "\n")) + s.rcpts = append(s.rcpts, rcpts) + s.mu.Unlock() + if reply == "drop" { + return + } + fmt.Fprintf(conn, "%s scripted reply\r\n", reply) + inData = false + continue + } + switch { + case len(line) > 8 && strings.EqualFold(line[:8], "RCPT TO:"): + rcpts = append(rcpts, strings.Trim(strings.TrimSpace(line[8:]), "<>")) + fmt.Fprint(conn, "250 OK\r\n") + case strings.EqualFold(line, "DATA"): + inData = true + fmt.Fprint(conn, "354 Go ahead\r\n") + case strings.EqualFold(line, "QUIT"): + fmt.Fprint(conn, "221 Bye\r\n") + return + default: + fmt.Fprint(conn, "250 OK\r\n") + } + } +} + +func (s *scriptedSMTP) received() ([]string, int) { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string(nil), s.messages...), s.conns +} + +func (s *scriptedSMTP) recipients() [][]string { + s.mu.Lock() + defer s.mu.Unlock() + return append([][]string(nil), s.rcpts...) +} + +func attemptHeader(wire string) string { + for _, line := range strings.Split(wire, "\n") { + if strings.HasPrefix(line, outbound.ProviderAttemptHeader+": ") { + return strings.TrimPrefix(line, outbound.ProviderAttemptHeader+": ") + } + } + return "" +} + +func countFeedbackAttempts(t *testing.T, pool *pgxpool.Pool) int { + t.Helper() + var n int + if err := pool.QueryRow(context.Background(), + `SELECT count(*) FROM sending_budget_reservations WHERE purpose = 'public_feedback_notification' AND call_state = 'started'`, + ).Scan(&n); err != nil { + t.Fatal(err) + } + return n +} + +func newFeedbackSeamAPI(t *testing.T, s *scriptedSMTP) (*API, *pgxpool.Pool) { + t.Helper() + pool := testdb.TestDB(t) + relay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{Host: s.host, Port: s.port}) + api := NewAPI(nil, outbound.NewSender(relay, "test.e2a.dev"), relay, nil, usage.NewNoopUsageTracker(), "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + api.SetProviderSubmitter(outbound.NewProviderSubmitter(relay, gate), gate) + return api, pool +} + +func fastFeedbackBackoff(t *testing.T) { + t.Helper() + old := feedbackRetryBackoff + feedbackRetryBackoff = []time.Duration{time.Millisecond, time.Millisecond, time.Millisecond} + t.Cleanup(func() { feedbackRetryBackoff = old }) +} + +// TestFeedbackSeam_EachPhysicalAttemptIsItsOwnOrdinal: a transient provider +// reply is retried, and the retry is a NEW authorized attempt — a distinct +// ordinal in the ledger and a distinct attempt id on the wire — not a replay +// of the first token. +func TestFeedbackSeam_EachPhysicalAttemptIsItsOwnOrdinal(t *testing.T) { + fastFeedbackBackoff(t) + s := startScriptedSMTP(t, "451", "250") + api, pool := newFeedbackSeamAPI(t, s) + before := countFeedbackAttempts(t, pool) + + err := api.sendFeedbackEmail(context.Background(), "t", "bug", "m", "", "", []string{"feedback@example.test"}, nil) + if err != nil { + t.Fatalf("sendFeedbackEmail: %v", err) + } + msgs, conns := s.received() + if conns != 2 || len(msgs) != 2 { + t.Fatalf("conns=%d messages=%d, want 2/2 (one retry)", conns, len(msgs)) + } + a1, a2 := attemptHeader(msgs[0]), attemptHeader(msgs[1]) + if a1 == "" || a2 == "" || a1 == a2 { + t.Fatalf("attempt ids on the wire = %q / %q, want two distinct non-empty ids", a1, a2) + } + if got := countFeedbackAttempts(t, pool) - before; got != 2 { + t.Fatalf("started feedback attempts = %d, want 2", got) + } +} + +// TestFeedbackSeam_DefiniteRejectionIsNotRetried: a 5xx is the provider's +// answer to the message; retrying it resends nothing. +func TestFeedbackSeam_DefiniteRejectionIsNotRetried(t *testing.T) { + fastFeedbackBackoff(t) + s := startScriptedSMTP(t, "554", "250") + api, pool := newFeedbackSeamAPI(t, s) + before := countFeedbackAttempts(t, pool) + + err := api.sendFeedbackEmail(context.Background(), "t", "bug", "m", "", "", []string{"feedback@example.test"}, nil) + if err == nil || !outbound.IsPermanentSMTPError(err) { + t.Fatalf("err = %v, want the permanent SMTP rejection", err) + } + if _, conns := s.received(); conns != 1 { + t.Fatalf("conns = %d, want 1 (no retry after a definite rejection)", conns) + } + if got := countFeedbackAttempts(t, pool) - before; got != 1 { + t.Fatalf("started feedback attempts = %d, want 1", got) + } +} + +// TestFeedbackSeam_LostAcceptanceIsNotRetried: a body the provider took but +// never answered may already be queued; a retry would be a second copy. +func TestFeedbackSeam_LostAcceptanceIsNotRetried(t *testing.T) { + fastFeedbackBackoff(t) + s := startScriptedSMTP(t, "drop", "250") + api, _ := newFeedbackSeamAPI(t, s) + + err := api.sendFeedbackEmail(context.Background(), "t", "bug", "m", "", "", []string{"feedback@example.test"}, nil) + if !errors.Is(err, outbound.ErrProviderAcceptanceUnknown) { + t.Fatalf("err = %v, want ErrProviderAcceptanceUnknown", err) + } + if _, conns := s.received(); conns != 1 { + t.Fatalf("conns = %d, want 1 (no retry after a lost acceptance)", conns) + } +} + +// TestFeedbackSeam_RetriesAreBounded: transient failures stop at the attempt +// cap, each one charged. +func TestFeedbackSeam_RetriesAreBounded(t *testing.T) { + fastFeedbackBackoff(t) + s := startScriptedSMTP(t, "451", "451", "451", "451", "250") + api, pool := newFeedbackSeamAPI(t, s) + before := countFeedbackAttempts(t, pool) + + err := api.sendFeedbackEmail(context.Background(), "t", "bug", "m", "", "", []string{"feedback@example.test"}, nil) + if err == nil { + t.Fatal("expected the exhausted retry loop to fail") + } + if _, conns := s.received(); conns != feedbackSendAttempts { + t.Fatalf("conns = %d, want %d", conns, feedbackSendAttempts) + } + if got := countFeedbackAttempts(t, pool) - before; got != feedbackSendAttempts { + t.Fatalf("started feedback attempts = %d, want %d", got, feedbackSendAttempts) + } +} + +// TestFeedbackSeam_EnvelopeIsConfigurationNotRequest: the recipients on the +// wire are exactly the configured notify set the operation was prepared with; +// the form's own address only ever appears as Reply-To. +func TestFeedbackSeam_EnvelopeIsConfigurationNotRequest(t *testing.T) { + s := startScriptedSMTP(t, "250") + api, _ := newFeedbackSeamAPI(t, s) + + err := api.sendFeedbackEmail(context.Background(), "t", "bug", "m", "someone@attacker.test", "", []string{"feedback@example.test"}, []string{"ops@example.test"}) + if err != nil { + t.Fatalf("sendFeedbackEmail: %v", err) + } + msgs, _ := s.received() + if len(msgs) != 1 { + t.Fatalf("messages = %d, want 1", len(msgs)) + } + if !strings.Contains(msgs[0], "Reply-To: someone@attacker.test") { + t.Errorf("submitter address should be the Reply-To only") + } + if attemptHeader(msgs[0]) == "" { + t.Errorf("feedback mail left without the provider attempt header: it did not cross the authorized seam") + } + got := s.recipients() + if len(got) != 1 || strings.Join(got[0], ",") != "feedback@example.test,ops@example.test" { + t.Errorf("RCPT TO = %v, want exactly the configured notify set", got) + } +} + +// TestFeedbackSeam_OverlappingNotifyConfigStillSends: TO and CC naming the +// same mailbox (in any case) is a legal configuration that used to send one +// copy; the seam's canonical recipient set keeps it that way instead of +// refusing every attempt. +func TestFeedbackSeam_OverlappingNotifyConfigStillSends(t *testing.T) { + s := startScriptedSMTP(t, "250") + api, _ := newFeedbackSeamAPI(t, s) + + err := api.sendFeedbackEmail(context.Background(), "t", "bug", "m", "", "", []string{"ops@example.test"}, []string{"Ops@example.test"}) + if err != nil { + t.Fatalf("sendFeedbackEmail: %v", err) + } + got := s.recipients() + if len(got) != 1 || len(got[0]) != 1 || !strings.EqualFold(got[0][0], "ops@example.test") { + t.Fatalf("RCPT TO = %v, want the one mailbox once", got) + } +} diff --git a/internal/hitlnotify/e2e_test.go b/internal/hitlnotify/e2e_test.go index ac7691941..54e42b0d2 100644 --- a/internal/hitlnotify/e2e_test.go +++ b/internal/hitlnotify/e2e_test.go @@ -13,6 +13,7 @@ import ( "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/jobs" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/testutil" ) @@ -34,7 +35,8 @@ func TestEndToEnd_AcceptTxThroughRiverToSMTP(t *testing.T) { Host: smtpAddr.Host, Port: smtpAddr.Port, FromDomain: "notify.test", }) signer := approvaltoken.NewSigner("hitl-notify-e2e-secret") - notifier := hitlnotify.New(store, relay, signer, "notify.test", "", "", "https://app.example.test") + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + notifier := hitlnotify.New(store, outbound.NewProviderSubmitter(relay, gate), signer, "notify.test", "", "", "https://app.example.test") // Seed a verified HITL agent + owner. user, err := store.CreateOrGetUser(ctx, "owner-e2e@reviewer.test", "Owner", "google-notify-e2e") @@ -54,7 +56,7 @@ func TestEndToEnd_AcceptTxThroughRiverToSMTP(t *testing.T) { } // Build the integration on a real client and bind the concrete Notifier. - j := hitlnotify.NewJobs(store) + j := hitlnotify.NewJobs(store).WithGate(gate, pool) client, err := jobs.New(pool, jobs.Config{}, j) if err != nil { t.Fatalf("jobs.New: %v", err) diff --git a/internal/hitlnotify/jobs.go b/internal/hitlnotify/jobs.go index c8c510236..554f28820 100644 --- a/internal/hitlnotify/jobs.go +++ b/internal/hitlnotify/jobs.go @@ -3,6 +3,7 @@ package hitlnotify import ( "context" "errors" + "fmt" "sync" "github.com/jackc/pgx/v5" @@ -11,6 +12,8 @@ import ( "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/jobs" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) // Jobs is the HITL-notification integration on the shared River client: a @@ -23,6 +26,8 @@ import ( type Jobs struct { store Store enq jobs.Enqueuer + gate sendingpolicy.Gate + pool *pgxpool.Pool mu sync.RWMutex deliverer Deliverer @@ -31,6 +36,20 @@ type Jobs struct { // NewJobs builds the integration with just its store (no client, no deliverer yet). func NewJobs(store Store) *Jobs { return &Jobs{store: store} } +// WithGate injects the sending-protection gate and the pool its legacy +// resolver and arg stamp use. Every enqueue then prepares a notification +// operation in the hold's transaction and every worker execution authorizes +// through the gate. Chainable; nil keeps the gateless default (tests only). +func (j *Jobs) WithGate(g sendingpolicy.Gate, pool *pgxpool.Pool) *Jobs { + if g != nil { + j.gate = g + } + if pool != nil { + j.pool = pool + } + return j +} + // SetEnqueuer injects the shared client so EnqueueNotifyTx can insert jobs. func (j *Jobs) SetEnqueuer(e jobs.Enqueuer) { j.enq = e } @@ -43,33 +62,98 @@ func (j *Jobs) SetDeliverer(d Deliverer) { j.mu.Unlock() } -// Deliver makes Jobs itself the worker's Deliverer, delegating to the concrete one -// set via SetDeliverer. Until that is wired (the brief startup window before the -// notifier is built) it returns a retryable outcome, so a pending job simply -// retries rather than dropping on a nil deliverer. -func (j *Jobs) Deliver(ctx context.Context, pn *identity.PendingNotify) DeliverOutcome { - j.mu.RLock() - d := j.deliverer - j.mu.RUnlock() +// Compose makes Jobs itself the worker's Deliverer, delegating to the +// concrete one set via SetDeliverer. Until that is wired (the brief startup +// window before the notifier is built) it returns a retryable outcome — and +// because Compose runs before any attempt is charged, that window costs +// nothing. +func (j *Jobs) Compose(ctx context.Context, pn *identity.PendingNotify) (outbound.Envelope, DeliverOutcome) { + d := j.currentDeliverer() + if d == nil { + return outbound.Envelope{}, DeliverOutcome{Err: errors.New("hitl notifier not wired yet — retrying")} + } + return d.Compose(ctx, pn) +} + +// Submit delegates the authorized submission to the concrete Deliverer. +func (j *Jobs) Submit(ctx context.Context, env outbound.Envelope, auth sendingpolicy.ProviderAuthorization) DeliverOutcome { + d := j.currentDeliverer() if d == nil { return DeliverOutcome{Err: errors.New("hitl notifier not wired yet — retrying")} } - return d.Deliver(ctx, pn) + return d.Submit(ctx, env, auth) +} + +func (j *Jobs) currentDeliverer() Deliverer { + j.mu.RLock() + defer j.mu.RUnlock() + return j.deliverer } +// Gate exposes the wired sending-protection gate (nil when gateless), so the +// composition root's wiring test can prove the production bundle is armed. +func (j *Jobs) Gate() sendingpolicy.Gate { return j.gate } + // RegisterJobs adds the NotifyWorker (with Jobs as the late-binding Deliverer). // No periodics — the reconciler is a one-shot startup cutover. Implements // jobs.Registrar. func (j *Jobs) RegisterJobs(w *river.Workers) []*river.PeriodicJob { - river.AddWorker(w, NewNotifyWorker(j.store, j)) + river.AddWorker(w, j.NotifyWorker()) return nil } +// NotifyWorker builds the fully armed worker RegisterJobs registers. +func (j *Jobs) NotifyWorker() *NotifyWorker { + w := NewNotifyWorker(j.store, j).WithGate(j.gate).WithOperationResolver(j.ResolveLegacyOperation) + if j.pool != nil { + w = w.WithArgStamper(func(ctx context.Context, jobID int64, ref sendingpolicy.OperationRef) error { + return jobs.StampJobArg(ctx, j.pool, jobID, "operation_ref", ref) + }).WithArgRestamper(func(ctx context.Context, jobID int64, ref sendingpolicy.OperationRef) error { + return jobs.SetJobArg(ctx, j.pool, jobID, "operation_ref", ref) + }) + } + return w +} + +// ResolveLegacyOperation prepares the notification operation for a job that +// carries no reference, in its own committed transaction, through the same +// PrepareNotificationTx an enqueue runs. +func (j *Jobs) ResolveLegacyOperation(ctx context.Context, messageID string) (sendingpolicy.OperationRef, error) { + if j.gate == nil || j.pool == nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("hitl notify: legacy operation resolver is not wired") + } + tx, err := j.pool.Begin(ctx) + if err != nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("begin legacy resolve: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + ref, err := j.gate.PrepareNotificationTx(ctx, tx, sendingpolicy.NewHITLNotificationRef(messageID)) + if err != nil { + return sendingpolicy.OperationRef{}, err + } + if err := tx.Commit(ctx); err != nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("commit legacy resolve: %w", err) + } + return ref, nil +} + // EnqueueNotifyTx inserts the hitl_notify job in the caller's hold accept-tx (the // same tx as the pending_review insert), returning the River job id to stamp on the // message so a committed pending_review row always has its notification job. +// +// With a gate wired the notification's operation is prepared here, in the +// same transaction, against the locked source row: the triggering account is +// charged, never the platform, and the worker never derives attribution. func (j *Jobs) EnqueueNotifyTx(ctx context.Context, tx pgx.Tx, messageID string) (int64, error) { - res, err := j.enq.InsertTx(ctx, tx, HITLNotifyArgs{MessageID: messageID}, &river.InsertOpts{ + args := HITLNotifyArgs{MessageID: messageID} + if j.gate != nil { + ref, err := j.gate.PrepareNotificationTx(ctx, tx, sendingpolicy.NewHITLNotificationRef(messageID)) + if err != nil { + return 0, fmt.Errorf("prepare notification operation: %w", err) + } + args.OperationRef = &ref + } + res, err := j.enq.InsertTx(ctx, tx, args, &river.InsertOpts{ Queue: jobs.QueueNotify, MaxAttempts: MaxNotifyAttempts, }) diff --git a/internal/hitlnotify/notifier.go b/internal/hitlnotify/notifier.go index 835739cfb..c8503c1ae 100644 --- a/internal/hitlnotify/notifier.go +++ b/internal/hitlnotify/notifier.go @@ -18,7 +18,6 @@ import ( "context" "fmt" "html" - "log" "net/url" "strings" "time" @@ -26,6 +25,7 @@ import ( "github.com/tokencanopy/e2a/internal/approvaltoken" "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) // notifyLocalPart is the default local-part of the notification sender @@ -50,9 +50,9 @@ const tokenGraceAfterTTL = 10 * time.Minute // call NotifyPendingApproval from the HITL gate right after the pending // row is written. Errors are logged, never returned upstream. type Notifier struct { - store *identity.Store - relay *outbound.SMTPRelay - signer *approvaltoken.Signer + store *identity.Store + submitter *outbound.ProviderSubmitter + signer *approvaltoken.Signer // fromAddress is the resolved sender: notifications.from_address when // set, else notifyLocalPart on fromDomain. fromAddress string @@ -80,7 +80,7 @@ type Notifier struct { // distinct and separately filterable. Resolution deliberately mirrors // webhooknotify.New line for line; it is a copy rather than a shared helper, // so changing one means changing the other. -func New(store *identity.Store, relay *outbound.SMTPRelay, signer *approvaltoken.Signer, fromDomain, fromAddress, replyTo, publicURL string) *Notifier { +func New(store *identity.Store, submitter *outbound.ProviderSubmitter, signer *approvaltoken.Signer, fromDomain, fromAddress, replyTo, publicURL string) *Notifier { addr := strings.TrimSpace(fromAddress) if addr == "" { addr = fmt.Sprintf("%s@%s", notifyLocalPart, fromDomain) @@ -91,7 +91,7 @@ func New(store *identity.Store, relay *outbound.SMTPRelay, signer *approvaltoken } return &Notifier{ store: store, - relay: relay, + submitter: submitter, signer: signer, fromAddress: addr, fromDomain: msgIDDomain, @@ -110,26 +110,36 @@ func (n *Notifier) WithDKIM(lookup outbound.DKIMKeyLookup) *Notifier { } // NotifyPendingApproval composes and sends the notification email for a held -// message, submitting once (SendOnce). It is the compose+send core the River -// NotifyWorker drives via Deliver; the returned error is classified there into -// retry/permanent/outage. -func (n *Notifier) NotifyPendingApproval(ctx context.Context, msg *identity.Message, agent *identity.AgentIdentity) error { +// message with an already-authorized attempt: Compose then Submit in one call, +// for callers that hold the token up front (tests, the reconciler drill). The +// worker calls the two phases itself so the token is consumed last. +func (n *Notifier) NotifyPendingApproval(ctx context.Context, msg *identity.Message, agent *identity.AgentIdentity, auth sendingpolicy.ProviderAuthorization) error { if n == nil { return nil } + env, err := n.compose(ctx, msg, agent) + if err != nil { + return err + } + return n.submit(ctx, env, auth) +} + +// compose builds the approval email: owner lookup, magic-link tokens, MIME, +// deterministic Message-ID and DKIM. It touches no provider. +func (n *Notifier) compose(ctx context.Context, msg *identity.Message, agent *identity.AgentIdentity) (outbound.Envelope, error) { if msg == nil || agent == nil { - return fmt.Errorf("notify: msg or agent is nil") + return outbound.Envelope{}, fmt.Errorf("notify: msg or agent is nil") } if msg.ApprovalExpiresAt == nil { - return fmt.Errorf("notify: approval_expires_at is nil on msg %s", msg.ID) + return outbound.Envelope{}, fmt.Errorf("notify: approval_expires_at is nil on msg %s", msg.ID) } owner, err := n.store.GetUserByID(ctx, agent.UserID) if err != nil { - return fmt.Errorf("notify: lookup owner: %w", err) + return outbound.Envelope{}, fmt.Errorf("notify: lookup owner: %w", err) } if owner.Email == "" { - return fmt.Errorf("notify: owner %s has no email on record", owner.ID) + return outbound.Envelope{}, fmt.Errorf("notify: owner %s has no email on record", owner.ID) } tokenExp := msg.ApprovalExpiresAt.Add(tokenGraceAfterTTL) @@ -142,11 +152,11 @@ func (n *Notifier) NotifyPendingApproval(ctx context.Context, msg *identity.Mess approveTok, err := signFn(approvaltoken.ActionApprove) if err != nil { - return fmt.Errorf("notify: sign approve token: %w", err) + return outbound.Envelope{}, fmt.Errorf("notify: sign approve token: %w", err) } rejectTok, err := signFn(approvaltoken.ActionReject) if err != nil { - return fmt.Errorf("notify: sign reject token: %w", err) + return outbound.Envelope{}, fmt.Errorf("notify: sign reject token: %w", err) } subject := fmt.Sprintf("[e2a] approve outbound from %s: %s", @@ -183,7 +193,7 @@ func (n *Notifier) NotifyPendingApproval(ctx context.Context, msg *identity.Mess "", // no conversation_id ) if err != nil { - return fmt.Errorf("notify: compose: %w", err) + return outbound.Envelope{}, fmt.Errorf("notify: compose: %w", err) } // Prepend a DETERMINISTIC Message-ID so a re-sent notification collapses at @@ -225,34 +235,58 @@ func (n *Notifier) NotifyPendingApproval(ctx context.Context, msg *identity.Mess message = signed } - // SendOnce, not Send: this runs inside a River job, so River (not the relay's - // in-process loop) owns retries. The %w keeps the SMTP error classifiable by - // Deliver via internal/outbound's IsPermanentSMTPError / IsConnectionError. - if _, err := n.relay.SendOnce(fromAddr, []string{owner.Email}, message); err != nil { + return outbound.Envelope{From: fromAddr, Recipients: []string{owner.Email}, Message: message}, nil +} + +// submit is the one authorized submission: the submitter redeems the token +// immediately before the socket opens and settles the provider's answer; +// River (not the relay's in-process loop) owns retries, each as a fresh +// attempt. The %w keeps the SMTP error classifiable via internal/outbound's +// IsPermanentSMTPError / IsConnectionError. +func (n *Notifier) submit(ctx context.Context, env outbound.Envelope, auth sendingpolicy.ProviderAuthorization) error { + if _, err := n.submitter.SubmitOnce(ctx, auth, env); err != nil { return fmt.Errorf("notify: smtp send: %w", err) } - - log.Printf("[hitl-notify] sent approval email: msg=%s owner=%s agent=%s", - msg.ID, owner.ID, agent.ID) return nil } -// Deliver composes and sends the approval email for one held message, classifying -// the result for the River NotifyWorker: a 5xx / validation reject is Permanent -// (no retry), an unreachable relay is an Outage (snooze), everything else retries. -// Implements hitlnotify.Deliverer. The classifiers key on the SMTP code / net -// error preserved through NotifyPendingApproval's %w wrapping. -func (n *Notifier) Deliver(ctx context.Context, pn *identity.PendingNotify) DeliverOutcome { - if err := n.NotifyPendingApproval(ctx, pn.Message, pn.Agent); err != nil { - return DeliverOutcome{ - Err: err, - Permanent: outbound.IsPermanentSMTPError(err), - Outage: outbound.IsConnectionError(err), - } +// Compose implements Deliverer: the provider-free half, classified like a +// send so the worker treats a permanent compose failure the same way. +func (n *Notifier) Compose(ctx context.Context, pn *identity.PendingNotify) (outbound.Envelope, DeliverOutcome) { + if n == nil { + return outbound.Envelope{}, DeliverOutcome{Err: fmt.Errorf("notify: notifier is nil")} + } + if pn == nil { + return outbound.Envelope{}, DeliverOutcome{Err: fmt.Errorf("notify: nothing to compose"), Permanent: true} + } + env, err := n.compose(ctx, pn.Message, pn.Agent) + if err != nil { + return outbound.Envelope{}, classify(err) + } + return env, DeliverOutcome{} +} + +// Submit implements Deliverer: one authorized submission, classified for the +// River NotifyWorker — a 5xx / validation reject is Permanent (no retry), an +// unreachable relay is an Outage (snooze), everything else retries. +func (n *Notifier) Submit(ctx context.Context, env outbound.Envelope, auth sendingpolicy.ProviderAuthorization) DeliverOutcome { + if n == nil { + return DeliverOutcome{Err: fmt.Errorf("notify: notifier is nil")} + } + if err := n.submit(ctx, env, auth); err != nil { + return classify(err) } return DeliverOutcome{} } +func classify(err error) DeliverOutcome { + return DeliverOutcome{ + Err: err, + Permanent: outbound.IsPermanentSMTPError(err), + Outage: outbound.IsConnectionError(err), + } +} + func (n *Notifier) magicURL(path, token string) string { if n.publicURL == "" { return path + "?t=" + url.QueryEscape(token) diff --git a/internal/hitlnotify/notifier_test.go b/internal/hitlnotify/notifier_test.go index 17e0b2878..59edeed62 100644 --- a/internal/hitlnotify/notifier_test.go +++ b/internal/hitlnotify/notifier_test.go @@ -5,12 +5,14 @@ import ( "strings" "testing" + "github.com/jackc/pgx/v5/pgxpool" "github.com/tokencanopy/e2a/internal/approvaltoken" "github.com/tokencanopy/e2a/internal/config" "github.com/tokencanopy/e2a/internal/dkim" "github.com/tokencanopy/e2a/internal/hitlnotify" "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/testutil" ) @@ -38,10 +40,54 @@ func newNotifier(t *testing.T) ( FromDomain: notifyFromDomain, }) signer := approvaltoken.NewSigner(notifySecret) - n := hitlnotify.New(store, relay, signer, notifyFromDomain, "", "", publicURL) + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + notifierGates[store] = gatePool{gate: gate, pool: pool} + n := hitlnotify.New(store, outbound.NewProviderSubmitter(relay, gate), signer, notifyFromDomain, "", "", publicURL) return n, store, signer, smtpDone } +type gatePool struct { + gate sendingpolicy.Gate + pool *pgxpool.Pool +} + +// notifierGates remembers the gate each test store was built with, so a test +// can mint the token its notification needs without threading it through +// every helper signature. +var notifierGates = map[*identity.Store]gatePool{} + +// tokenFor prepares the notification operation for a held message and runs +// Reserve + ConsumeAttempt, returning the authorization the notifier redeems. +func tokenFor(t *testing.T, store *identity.Store, messageID string) sendingpolicy.ProviderAuthorization { + t.Helper() + gp, ok := notifierGates[store] + if !ok { + t.Fatal("no gate for this store") + } + ctx := context.Background() + tx, err := gp.pool.Begin(ctx) + if err != nil { + t.Fatal(err) + } + ref, err := gp.gate.PrepareNotificationTx(ctx, tx, sendingpolicy.NewHITLNotificationRef(messageID)) + if err != nil { + _ = tx.Rollback(ctx) + t.Fatalf("prepare notification: %v", err) + } + if err := tx.Commit(ctx); err != nil { + t.Fatal(err) + } + early, attempt, err := gp.gate.Reserve(ctx, ref) + if err != nil || !early.Allow { + t.Fatalf("reserve: decision=%+v err=%v", early, err) + } + decision, auth, err := gp.gate.ConsumeAttempt(ctx, attempt) + if err != nil || auth == nil { + t.Fatalf("authorize: decision=%+v err=%v", decision, err) + } + return *auth +} + // setupPendingMessage creates a verified HITL-enabled agent with one // pending outbound message. Returns (agent, message). func setupPendingMessage(t *testing.T, store *identity.Store, slug string) (*identity.AgentIdentity, *identity.Message) { @@ -83,7 +129,7 @@ func TestNotifierSendsEmailToOwner(t *testing.T) { n, store, _, smtpDone := newNotifier(t) agent, msg := setupPendingMessage(t, store, "send-email") - if err := n.NotifyPendingApproval(context.Background(), msg, agent); err != nil { + if err := n.NotifyPendingApproval(context.Background(), msg, agent, tokenFor(t, store, msg.ID)); err != nil { t.Fatalf("NotifyPendingApproval: %v", err) } @@ -148,7 +194,7 @@ func TestNotifierMagicLinksAreVerifiable(t *testing.T) { n, store, _, smtpDone := newNotifier(t) agent, msg := setupPendingMessage(t, store, "tok-verify") - if err := n.NotifyPendingApproval(context.Background(), msg, agent); err != nil { + if err := n.NotifyPendingApproval(context.Background(), msg, agent, tokenFor(t, store, msg.ID)); err != nil { t.Fatal(err) } data := smtpDone()[0].Data @@ -191,7 +237,7 @@ func TestNotifierBuildsAbsoluteURLs(t *testing.T) { n, store, _, smtpDone := newNotifier(t) agent, msg := setupPendingMessage(t, store, "abs-url") - if err := n.NotifyPendingApproval(context.Background(), msg, agent); err != nil { + if err := n.NotifyPendingApproval(context.Background(), msg, agent, tokenFor(t, store, msg.ID)); err != nil { t.Fatal(err) } data := smtpDone()[0].Data @@ -213,7 +259,7 @@ func TestNotifierRejectsMessageWithNilApprovalExpiresAt(t *testing.T) { agent, msg := setupPendingMessage(t, store, "nil-exp") msg.ApprovalExpiresAt = nil - err := n.NotifyPendingApproval(context.Background(), msg, agent) + err := n.NotifyPendingApproval(context.Background(), msg, agent, tokenFor(t, store, msg.ID)) if err == nil { t.Fatal("expected error for nil ApprovalExpiresAt") } @@ -231,10 +277,10 @@ func TestNotifierDeterministicMessageID(t *testing.T) { n, store, _, smtpDone := newNotifier(t) agent, msg := setupPendingMessage(t, store, "msgid") - if err := n.NotifyPendingApproval(context.Background(), msg, agent); err != nil { + if err := n.NotifyPendingApproval(context.Background(), msg, agent, tokenFor(t, store, msg.ID)); err != nil { t.Fatal(err) } - if err := n.NotifyPendingApproval(context.Background(), msg, agent); err != nil { + if err := n.NotifyPendingApproval(context.Background(), msg, agent, tokenFor(t, store, msg.ID)); err != nil { t.Fatal(err) } @@ -251,8 +297,11 @@ func TestNotifierDeterministicMessageID(t *testing.T) { if n := strings.Count(m.Data, "Message-ID:"); n != 1 { t.Errorf("message %d has %d Message-ID headers, want exactly 1", i, n) } - if !strings.HasPrefix(m.Data, "Message-ID: maxNotifyAge { + // A hold with no TTL on record behind a paused account would otherwise + // snooze forever (River's snooze spends no attempt); a week-old + // approval request is stale by any reading. + log.Printf("[hitl-notify] dropping notice for %s: older than %s", msg.ID, maxNotifyAge) + return nil + } if pn.Notified { return nil // a prior attempt already sent it (crash-after-send re-drive) } @@ -125,8 +205,59 @@ func (w *NotifyWorker) Work(ctx context.Context, job *river.Job[HITLNotifyArgs]) return nil // agent opted out of approval notifications } - out := w.deliverer.Deliver(ctx, pn) + // Compose first: owner lookup, magic-link signing, MIME and DKIM are all + // fallible and none of them touches the provider, so they run before any + // attempt is charged. A failure here is classified exactly like a send + // failure but costs no ordinal. + env, out := w.deliverer.Compose(ctx, pn) + if out.Err != nil { + return w.verdict(job, msg.ID, "compose", out) + } + + // Every provider call is authorized: Reserve the durable attempt, hold + // without I/O when the gate says so, ConsumeAttempt as the LAST decision + // before Submit, whose submitter redeems the token immediately before the + // socket opens. Notifications carry no durable hold class of their own — + // the approval TTL guard above already bounds how long one can wait, and + // a hold past it becomes the no-op the guard returns. + auth := sendingpolicy.ProviderAuthorization{} + if w.gate != nil { + ref, err := w.operationFor(ctx, job) + if err != nil { + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + return nil // the hold is gone — nothing to notify + } + if errors.Is(err, errOperationMismatch) { + return river.JobCancel(err) + } + return err + } + early, attempt, err := w.gate.Reserve(ctx, ref) + if err != nil { + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + return nil + } + return river.JobSnooze(notifyOutageSnooze) + } + if !early.Allow { + return holdVerdict(early) + } + decision, token, err := w.gate.ConsumeAttempt(ctx, attempt) + if err != nil { + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + return nil + } + return river.JobSnooze(notifyOutageSnooze) + } + if !decision.Allow || token == nil { + return holdVerdict(decision) + } + auth = *token + } + + out = w.deliverer.Submit(ctx, env, auth) if out.Err == nil { + log.Printf("[hitl-notify] sent approval email: msg=%s", msg.ID) if merr := w.store.MarkMessageNotified(ctx, msg.ID); merr != nil { // The email is already out; only the dedup marker failed to persist. Do // NOT return an error — a retry would re-send. Completing the job leaves @@ -135,19 +266,88 @@ func (w *NotifyWorker) Work(ctx context.Context, job *river.Job[HITLNotifyArgs]) } return nil } + return w.verdict(job, msg.ID, "send", out) +} + +// verdict turns a classified failure into River's answer: a permanent one +// cancels (the hold still finalizes on its TTL), an outage snoozes without +// spending a River attempt, everything else retries per NextRetry until +// MaxNotifyAttempts. +func (w *NotifyWorker) verdict(job *river.Job[HITLNotifyArgs], messageID, phase string, out DeliverOutcome) error { if out.Permanent { - // e.g. the owner address is rejected 5xx. Unavoidable — the hold still - // finalizes on its TTL. Cancel (no retry) rather than churn the tail. - log.Printf("[hitl-notify] permanent send failure for %s (no retry): %v", msg.ID, out.Err) + log.Printf("[hitl-notify] permanent %s failure for %s (no retry): %v", phase, messageID, out.Err) return river.JobCancel(out.Err) } if out.Outage { - // Relay unreachable. Snooze without burning an attempt. If the hold has - // since passed its TTL, the next attempt's expiry guard above short-circuits - // to a no-op — no need to special-case it here. + // Relay unreachable. If the hold has since passed its TTL, the next + // attempt's expiry guard short-circuits to a no-op. return river.JobSnooze(notifyOutageSnooze) } - // Transient (relay throttle, owner lookup blip, compose error): let River - // reschedule per NextRetry until MaxNotifyAttempts, then discard. - return fmt.Errorf("hitl notify attempt %d failed: %w", job.Attempt, out.Err) + return fmt.Errorf("hitl notify attempt %d %s failed: %w", job.Attempt, phase, out.Err) } + +// operationFor returns the job's durable operation, resolving and stamping a +// legacy job through the accept path. +func (w *NotifyWorker) operationFor(ctx context.Context, job *river.Job[HITLNotifyArgs]) (sendingpolicy.OperationRef, error) { + // The approval request's operation IS derived from the message id, so a + // reference naming any other operation would charge another account: the + // same binding the message worker enforces, checked before Reserve. + want := sendingpolicy.HITLNotificationOperationID(job.Args.MessageID) + stamp := w.stamp + if job.Args.OperationRef != nil && !job.Args.OperationRef.IsZero() { + stored := job.Args.OperationRef.ID() + if stored == want { + return *job.Args.OperationRef, nil + } + if sendingpolicy.IsHITLNotificationOperationID(stored) { + // A derived id for a different message: foreign, never authorize. + // (Any other shape, a wrong-kind derivation included, is re-derived + // from this job's own source below, so no stored id can redirect + // attribution.) + return sendingpolicy.OperationRef{}, errOperationMismatch + } + // A pre-derivation reference — migration 113 stamped adopted jobs + // with op_, and the first build of this seam minted op_. + // Its source is still this job's own message, so re-derive through + // the same Prepare path and replace the reference, once. + log.Printf("[hitl-notify] job %d carries a pre-derivation operation reference %s; re-keying", job.ID, stored) + stamp = w.restamp + } + if w.resolve == nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("hitl notify: legacy job %d carries no operation and no resolver is wired", job.ID) + } + ref, err := w.resolve(ctx, job.Args.MessageID) + if err != nil { + return sendingpolicy.OperationRef{}, err + } + if ref.ID() != want { + return sendingpolicy.OperationRef{}, errOperationMismatch + } + if stamp != nil { + if err := stamp(ctx, job.ID, ref); err != nil { + // Not fatal: the reference is valid for this execution; a retry + // resolves again (idempotently) and stamps then. + log.Printf("[hitl-notify] stamp operation on legacy job %d: %v", job.ID, err) + } + } + return ref, nil +} + +// holdVerdict turns a gate hold into River's answer: a terminal hold cancels +// the job, everything else waits for the gate's retry time or the outage pace. +func holdVerdict(d sendingpolicy.Decision) error { + if d.Terminal { + return river.JobCancel(fmt.Errorf("hitl notify: sending policy: %s", d.Reason)) + } + delay := notifyOutageSnooze + if !d.RetryAt.IsZero() { + if until := time.Until(d.RetryAt); until > delay { + delay = until + } + } + return river.JobSnooze(delay) +} + +// Gate exposes the wired gate (nil when gateless), for the composition +// root's wiring test. +func (w *NotifyWorker) Gate() sendingpolicy.Gate { return w.gate } diff --git a/internal/hitlnotify/worker_test.go b/internal/hitlnotify/worker_test.go index 2f75ec8ea..e8c71a0cc 100644 --- a/internal/hitlnotify/worker_test.go +++ b/internal/hitlnotify/worker_test.go @@ -2,7 +2,9 @@ package hitlnotify_test import ( "context" + "encoding/json" "errors" + "strings" "testing" "time" @@ -12,6 +14,8 @@ import ( "github.com/tokencanopy/e2a/internal/hitlnotify" "github.com/tokencanopy/e2a/internal/identity" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) type fakeStore struct { @@ -34,15 +38,36 @@ func (f *fakeStore) StampNotifyJobIDTx(_ context.Context, _ pgx.Tx, _ string, _ } type fakeDeliverer struct { - out hitlnotify.DeliverOutcome - called int + out hitlnotify.DeliverOutcome // Submit's outcome + composeOut hitlnotify.DeliverOutcome // Compose's outcome + called int // Submit calls + composed int + auths []sendingpolicy.ProviderAuthorization + trace *[]string // shared with fakeGate to pin ordering } -func (f *fakeDeliverer) Deliver(_ context.Context, _ *identity.PendingNotify) hitlnotify.DeliverOutcome { +func (f *fakeDeliverer) Compose(_ context.Context, _ *identity.PendingNotify) (outbound.Envelope, hitlnotify.DeliverOutcome) { + f.composed++ + f.record("compose") + if f.composeOut.Err != nil { + return outbound.Envelope{}, f.composeOut + } + return outbound.Envelope{From: "e2a@notify.test", Recipients: []string{"owner@reviewer.test"}, Message: []byte("Subject: x\r\n\r\nbody")}, hitlnotify.DeliverOutcome{} +} + +func (f *fakeDeliverer) Submit(_ context.Context, _ outbound.Envelope, auth sendingpolicy.ProviderAuthorization) hitlnotify.DeliverOutcome { f.called++ + f.record("submit") + f.auths = append(f.auths, auth) return f.out } +func (f *fakeDeliverer) record(step string) { + if f.trace != nil { + *f.trace = append(*f.trace, step) + } +} + func job(id string, attempt int) *river.Job[hitlnotify.HITLNotifyArgs] { return &river.Job[hitlnotify.HITLNotifyArgs]{ JobRow: &rivertype.JobRow{Attempt: attempt, MaxAttempts: hitlnotify.MaxNotifyAttempts, Kind: hitlnotify.HITLNotifyArgs{}.Kind()}, @@ -212,3 +237,282 @@ func TestNotifyWorker_NextRetryMatchesEnvelope(t *testing.T) { } } } + +// fakeGate is a scriptable sendingpolicy.Gate for the worker-order tests. +type fakeGate struct { + trace *[]string + reserve sendingpolicy.Decision + consume sendingpolicy.Decision + reserves int + consumes int + reserveErr error +} + +func allowAll() *fakeGate { + return &fakeGate{reserve: sendingpolicy.Decision{Allow: true}, consume: sendingpolicy.Decision{Allow: true}} +} + +func (g *fakeGate) PrepareExternalTx(context.Context, pgx.Tx, string) (sendingpolicy.AcceptanceDecision, sendingpolicy.OperationRef, error) { + return sendingpolicy.AcceptanceAccept, sendingpolicy.OperationRef{}, nil +} +func (g *fakeGate) PrepareNotificationTx(context.Context, pgx.Tx, sendingpolicy.NotificationRef) (sendingpolicy.OperationRef, error) { + return refFor("op_prepared"), nil +} +func (g *fakeGate) PrepareProtectionNoticeTx(context.Context, pgx.Tx, sendingpolicy.ProtectionNoticeRef) (sendingpolicy.OperationRef, error) { + return sendingpolicy.OperationRef{}, nil +} +func (g *fakeGate) PreparePublicFeedback(context.Context, sendingpolicy.PublicFeedbackRef) (sendingpolicy.OperationRef, error) { + return sendingpolicy.OperationRef{}, nil +} +func (g *fakeGate) Reserve(context.Context, sendingpolicy.OperationRef) (sendingpolicy.Decision, sendingpolicy.AttemptRef, error) { + g.reserves++ + g.record("reserve") + return g.reserve, sendingpolicy.AttemptRef{}, g.reserveErr +} +func (g *fakeGate) ConsumeAttempt(context.Context, sendingpolicy.AttemptRef) (sendingpolicy.Decision, *sendingpolicy.ProviderAuthorization, error) { + g.consumes++ + g.record("consume") + if !g.consume.Allow { + return g.consume, nil, nil + } + return g.consume, &sendingpolicy.ProviderAuthorization{}, nil +} +func (g *fakeGate) RedeemProviderCall(context.Context, sendingpolicy.ProviderAuthorization) error { + return nil +} +func (g *fakeGate) DeferAttempt(context.Context, sendingpolicy.AttemptRef) error { return nil } +func (g *fakeGate) CancelAttempt(context.Context, sendingpolicy.AttemptRef) error { return nil } +func (g *fakeGate) SettleProvider(context.Context, sendingpolicy.ProviderSettlement) error { + return nil +} +func (g *fakeGate) SettleOperation(context.Context, sendingpolicy.OperationRef, sendingpolicy.SettlementOutcome, string) error { + return nil +} +func (g *fakeGate) LookupOperation(_ context.Context, id string) (sendingpolicy.OperationRef, error) { + return refFor(id), nil +} + +func refFor(id string) sendingpolicy.OperationRef { + var ref sendingpolicy.OperationRef + if err := json.Unmarshal([]byte(`{"v":1,"id":"`+id+`"}`), &ref); err != nil { + panic(err) + } + return ref +} + +func gatedJob(id string, attempt int) *river.Job[hitlnotify.HITLNotifyArgs] { + j := job(id, attempt) + ref := refFor(sendingpolicy.HITLNotificationOperationID(id)) + j.Args.OperationRef = &ref + return j +} + +func isSnooze(err error) bool { + var snooze *river.JobSnoozeError + return errors.As(err, &snooze) +} + +func isCancel(err error) bool { + var cancel *river.JobCancelError + return errors.As(err, &cancel) +} + +func TestNotifyWorker_GatedPathAuthorizesThenDelivers(t *testing.T) { + st := &fakeStore{pn: pending("msg_gated")} + dl := &fakeDeliverer{} + g := allowAll() + if err := hitlnotify.NewNotifyWorker(st, dl).WithGate(g).Work(context.Background(), gatedJob("msg_gated", 1)); err != nil { + t.Fatalf("Work: %v", err) + } + if g.reserves != 1 || g.consumes != 1 || dl.called != 1 || len(st.notified) != 1 { + t.Fatalf("reserves=%d consumes=%d delivers=%d notified=%d, want 1/1/1/1", g.reserves, g.consumes, dl.called, len(st.notified)) + } +} + +func TestNotifyWorker_GateHoldSnoozesWithoutDelivery(t *testing.T) { + for name, g := range map[string]*fakeGate{ + "early hold": {reserve: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonAccountPaused}}, + "late hold": {reserve: sendingpolicy.Decision{Allow: true}, consume: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonAccountSharedBudget, RetryAt: time.Now().Add(2 * time.Hour)}}, + "gate error": {reserveErr: errors.New("policy db down")}, + } { + st := &fakeStore{pn: pending("msg_hold")} + dl := &fakeDeliverer{} + err := hitlnotify.NewNotifyWorker(st, dl).WithGate(g).Work(context.Background(), gatedJob("msg_hold", 1)) + if !isSnooze(err) || dl.called != 0 || len(st.notified) != 0 { + t.Fatalf("%s: err=%v delivers=%d notified=%d, want snooze with no I/O", name, err, dl.called, len(st.notified)) + } + } +} + +func TestNotifyWorker_TerminalHoldCancels(t *testing.T) { + st := &fakeStore{pn: pending("msg_terminal")} + dl := &fakeDeliverer{} + g := &fakeGate{reserve: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonAccountDeleted, Terminal: true}} + if err := hitlnotify.NewNotifyWorker(st, dl).WithGate(g).Work(context.Background(), gatedJob("msg_terminal", 1)); !isCancel(err) || dl.called != 0 { + t.Fatalf("err=%v delivers=%d, want cancel with no I/O", err, dl.called) + } +} + +func TestNotifyWorker_LegacyJobResolvesAndStampsOnce(t *testing.T) { + st := &fakeStore{pn: pending("msg_legacy")} + dl := &fakeDeliverer{} + resolved, stamped := 0, 0 + w := hitlnotify.NewNotifyWorker(st, dl).WithGate(allowAll()). + WithOperationResolver(func(_ context.Context, id string) (sendingpolicy.OperationRef, error) { + resolved++ + return refFor(sendingpolicy.HITLNotificationOperationID(id)), nil + }). + WithArgStamper(func(_ context.Context, _ int64, _ sendingpolicy.OperationRef) error { stamped++; return nil }) + if err := w.Work(context.Background(), job("msg_legacy", 1)); err != nil { + t.Fatalf("Work: %v", err) + } + if resolved != 1 || stamped != 1 || dl.called != 1 { + t.Fatalf("resolved=%d stamped=%d delivers=%d, want 1/1/1", resolved, stamped, dl.called) + } + // A legacy job whose source is gone is a no-op, never a retry loop. + w = hitlnotify.NewNotifyWorker(&fakeStore{pn: pending("msg_gone")}, dl).WithGate(allowAll()). + WithOperationResolver(func(context.Context, string) (sendingpolicy.OperationRef, error) { + return sendingpolicy.OperationRef{}, sendingpolicy.ErrSourceUnavailable + }) + if err := w.Work(context.Background(), job("msg_gone", 1)); err != nil || dl.called != 1 { + t.Fatalf("orphan legacy: err=%v delivers=%d, want nil and no new delivery", err, dl.called) + } +} + +func (g *fakeGate) record(step string) { + if g.trace != nil { + *g.trace = append(*g.trace, step) + } +} + +// TestNotifyWorker_ComposeRunsBeforeAnyChargeAndConsumeIsLast pins the order +// the seam depends on: compose (every fallible, provider-free step) precedes +// Reserve, and ConsumeAttempt is the last call before Submit. +func TestNotifyWorker_ComposeRunsBeforeAnyChargeAndConsumeIsLast(t *testing.T) { + var trace []string + fd := &fakeDeliverer{trace: &trace} + g := allowAll() + g.trace = &trace + st := &fakeStore{pn: pending("msg_1")} + w := hitlnotify.NewNotifyWorker(st, fd).WithGate(g) + if err := w.Work(context.Background(), gatedJob("msg_1", 1)); err != nil { + t.Fatalf("Work: %v", err) + } + if got := strings.Join(trace, ","); got != "compose,reserve,consume,submit" { + t.Fatalf("order = %s, want compose,reserve,consume,submit", got) + } +} + +// TestNotifyWorker_ComposeFailureChargesNothing: a compose failure (owner +// lookup, signing, MIME) happens before Reserve, so it burns no ordinal; it +// is classified exactly like a send failure. +func TestNotifyWorker_ComposeFailureChargesNothing(t *testing.T) { + for name, tc := range map[string]struct { + out hitlnotify.DeliverOutcome + wantErr func(error) bool + wantMsgID bool + }{ + "transient": {out: hitlnotify.DeliverOutcome{Err: errors.New("owner lookup blip")}, wantErr: func(err error) bool { return err != nil && !isCancel(err) && !isSnooze(err) }}, + "permanent": {out: hitlnotify.DeliverOutcome{Err: errors.New("no owner email"), Permanent: true}, wantErr: isCancel}, + "outage": {out: hitlnotify.DeliverOutcome{Err: errors.New("dkim store down"), Outage: true}, wantErr: isSnooze}, + } { + fd := &fakeDeliverer{composeOut: tc.out} + g := allowAll() + st := &fakeStore{pn: pending("msg_1")} + w := hitlnotify.NewNotifyWorker(st, fd).WithGate(g) + err := w.Work(context.Background(), gatedJob("msg_1", 1)) + if !tc.wantErr(err) { + t.Fatalf("%s: err = %v", name, err) + } + if g.reserves != 0 || g.consumes != 0 || fd.called != 0 { + t.Fatalf("%s: reserves=%d consumes=%d submits=%d, want 0/0/0", name, g.reserves, g.consumes, fd.called) + } + if len(st.notified) != 0 { + t.Fatalf("%s: marked notified without a send", name) + } + } +} + +// TestNotifyWorker_ForeignOperationReferenceIsCancelled: a job whose +// reference names another message's operation would charge that operation's +// account; it is cancelled before Reserve, never retried. +func TestNotifyWorker_ForeignOperationReferenceIsCancelled(t *testing.T) { + fd := &fakeDeliverer{} + g := allowAll() + st := &fakeStore{pn: pending("msg_1")} + w := hitlnotify.NewNotifyWorker(st, fd).WithGate(g) + j := job("msg_1", 1) + ref := refFor(sendingpolicy.HITLNotificationOperationID("msg_other")) + j.Args.OperationRef = &ref + if err := w.Work(context.Background(), j); !isCancel(err) { + t.Fatalf("err = %v, want cancel", err) + } + if g.reserves != 0 || fd.called != 0 { + t.Fatalf("reserves=%d submits=%d, want 0/0", g.reserves, fd.called) + } + + // The same binding applies to a legacy resolve that returns a foreign id. + fd, g = &fakeDeliverer{}, allowAll() + w = hitlnotify.NewNotifyWorker(&fakeStore{pn: pending("msg_1")}, fd).WithGate(g). + WithOperationResolver(func(context.Context, string) (sendingpolicy.OperationRef, error) { + return refFor(sendingpolicy.HITLNotificationOperationID("msg_other")), nil + }) + if err := w.Work(context.Background(), job("msg_1", 1)); !isCancel(err) { + t.Fatalf("legacy: err = %v, want cancel", err) + } + if g.reserves != 0 || fd.called != 0 { + t.Fatalf("legacy: reserves=%d submits=%d, want 0/0", g.reserves, fd.called) + } +} + +// TestNotifyWorker_PreDerivationReferenceIsReKeyed: a job stamped before the +// source-derived ids existed (migration 113's op_, or the first build +// of this seam) is re-resolved through the Prepare path and its reference +// replaced, not cancelled — its source is still this job's own message. +func TestNotifyWorker_PreDerivationReferenceIsReKeyed(t *testing.T) { + fd := &fakeDeliverer{} + g := allowAll() + resolved, stamped, restamped := 0, 0, 0 + var restampedWith string + w := hitlnotify.NewNotifyWorker(&fakeStore{pn: pending("msg_1")}, fd).WithGate(g). + WithOperationResolver(func(_ context.Context, id string) (sendingpolicy.OperationRef, error) { + resolved++ + return refFor(sendingpolicy.HITLNotificationOperationID(id)), nil + }). + WithArgStamper(func(context.Context, int64, sendingpolicy.OperationRef) error { stamped++; return nil }). + WithArgRestamper(func(_ context.Context, _ int64, ref sendingpolicy.OperationRef) error { + restamped++ + restampedWith = ref.ID() + return nil + }) + j := job("msg_1", 1) + legacy := refFor("op_0123456789abcdef0123456789abcdef") + j.Args.OperationRef = &legacy + if err := w.Work(context.Background(), j); err != nil { + t.Fatalf("Work: %v", err) + } + if resolved != 1 || restamped != 1 || stamped != 0 || restampedWith != sendingpolicy.HITLNotificationOperationID("msg_1") { + t.Fatalf("resolved=%d restamped=%d stamped=%d with=%q, want 1/1/0 with the derived id", resolved, restamped, stamped, restampedWith) + } + if g.reserves != 1 || fd.called != 1 { + t.Fatalf("reserves=%d submits=%d, want 1/1", g.reserves, fd.called) + } +} + +// TestNotifyWorker_StaleNoticeIsDropped: a request older than the age bound +// is dropped instead of snoozing forever behind a hold. +func TestNotifyWorker_StaleNoticeIsDropped(t *testing.T) { + fd := &fakeDeliverer{} + g := &fakeGate{reserve: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonAccountPaused}} + pn := pending("msg_1") + pn.Message.ApprovalExpiresAt = nil + w := hitlnotify.NewNotifyWorker(&fakeStore{pn: pn}, fd).WithGate(g) + j := gatedJob("msg_1", 1) + j.CreatedAt = time.Now().Add(-8 * 24 * time.Hour) + if err := w.Work(context.Background(), j); err != nil { + t.Fatalf("err = %v, want a silent drop", err) + } + if g.reserves != 0 || fd.composed != 0 || fd.called != 0 { + t.Fatalf("reserves=%d composes=%d submits=%d, want 0/0/0", g.reserves, fd.composed, fd.called) + } +} diff --git a/internal/jobs/argstamp.go b/internal/jobs/argstamp.go new file mode 100644 index 000000000..569b0018c --- /dev/null +++ b/internal/jobs/argstamp.go @@ -0,0 +1,62 @@ +package jobs + +import ( + "context" + "encoding/json" + "fmt" + + "github.com/jackc/pgx/v5/pgconn" +) + +// Execer is the one method StampJobArg needs; both a pool and a transaction +// satisfy it. +type Execer interface { + Exec(ctx context.Context, sql string, args ...any) (pgconn.CommandTag, error) +} + +// StampJobArg adds one key to a River job's args, only when that key is +// absent, leaving every existing field in place. +// +// It exists for the sending-protection compatibility resolvers: a job +// enqueued by a pre-floor slot carries no operation reference, the worker +// derives one through the same Prepare path an enqueue uses, and stamping it +// here makes that derivation happen once per job rather than once per +// execution. Existing fields stay so an older worker can still read the job. +func StampJobArg(ctx context.Context, db Execer, jobID int64, key string, value any) error { + if db == nil { + return fmt.Errorf("stamp job arg: no database") + } + patch, err := json.Marshal(map[string]any{key: value}) + if err != nil { + return fmt.Errorf("stamp job arg: encode %s: %w", key, err) + } + if _, err := db.Exec(ctx, + `UPDATE river_job SET args = args || $2::jsonb WHERE id = $1 AND NOT (args ? $3)`, + jobID, string(patch), key, + ); err != nil { + return fmt.Errorf("stamp job arg %s on job %d: %w", key, jobID, err) + } + return nil +} + +// SetJobArg writes one key into a River job's args unconditionally, leaving +// every other field in place. It is the re-key half of the compatibility +// story: a job whose reference predates the source-derived ids (migration +// 113 stamped `op_`) is re-resolved through the same Prepare path and +// its reference replaced, once. +func SetJobArg(ctx context.Context, db Execer, jobID int64, key string, value any) error { + if db == nil { + return fmt.Errorf("set job arg: no database") + } + patch, err := json.Marshal(map[string]any{key: value}) + if err != nil { + return fmt.Errorf("set job arg: encode %s: %w", key, err) + } + if _, err := db.Exec(ctx, + `UPDATE river_job SET args = args || $2::jsonb WHERE id = $1`, + jobID, string(patch), + ); err != nil { + return fmt.Errorf("set job arg %s on job %d: %w", key, jobID, err) + } + return nil +} diff --git a/internal/jobs/argstamp_test.go b/internal/jobs/argstamp_test.go new file mode 100644 index 000000000..45dc45c42 --- /dev/null +++ b/internal/jobs/argstamp_test.go @@ -0,0 +1,85 @@ +package jobs_test + +import ( + "context" + "testing" + + "github.com/tokencanopy/e2a/internal/jobs" + "github.com/tokencanopy/e2a/internal/testutil" +) + +// TestStampJobArg: the key is added once, existing fields survive, a present +// key is never overwritten, and a missing job is a no-op rather than an +// error (River may have pruned it). +func TestStampJobArg(t *testing.T) { + ctx := context.Background() + pool := testutil.TestDB(t) + if err := jobs.Migrate(ctx, pool); err != nil { + t.Fatalf("Migrate: %v", err) + } + var id int64 + if err := pool.QueryRow(ctx, + `INSERT INTO river_job (args, kind, max_attempts) VALUES ('{"message_id":"msg_1"}'::jsonb, 'argstamp_test', 3) RETURNING id`, + ).Scan(&id); err != nil { + t.Fatal(err) + } + + if err := jobs.StampJobArg(ctx, pool, id, "operation_ref", map[string]any{"v": 1, "id": "op_1"}); err != nil { + t.Fatalf("stamp: %v", err) + } + if err := jobs.StampJobArg(ctx, pool, id, "operation_ref", map[string]any{"v": 1, "id": "op_2"}); err != nil { + t.Fatalf("second stamp: %v", err) + } + var messageID, opID string + if err := pool.QueryRow(ctx, + `SELECT args->>'message_id', args->'operation_ref'->>'id' FROM river_job WHERE id = $1`, id, + ).Scan(&messageID, &opID); err != nil { + t.Fatal(err) + } + if messageID != "msg_1" || opID != "op_1" { + t.Fatalf("args = message_id=%q operation_ref.id=%q, want msg_1 / op_1 (first stamp wins, existing field kept)", messageID, opID) + } + + if err := jobs.StampJobArg(ctx, pool, id+1000, "operation_ref", "x"); err != nil { + t.Fatalf("missing job must be a no-op, got %v", err) + } + + // SetJobArg replaces the key and keeps the rest. + if err := jobs.SetJobArg(ctx, pool, id, "operation_ref", map[string]any{"v": 1, "id": "op_3"}); err != nil { + t.Fatalf("set: %v", err) + } + if err := pool.QueryRow(ctx, + `SELECT args->>'message_id', args->'operation_ref'->>'id' FROM river_job WHERE id = $1`, id, + ).Scan(&messageID, &opID); err != nil { + t.Fatal(err) + } + if messageID != "msg_1" || opID != "op_3" { + t.Fatalf("after set: message_id=%q operation_ref.id=%q, want msg_1 / op_3", messageID, opID) + } + if err := jobs.SetJobArg(ctx, nil, id, "k", "v"); err == nil { + t.Fatal("nil database must be refused") + } + if err := jobs.SetJobArg(ctx, pool, id, "k", make(chan int)); err == nil { + t.Fatal("unencodable value must be refused") + } +} + +// TestStampJobArgRefusesBadInputs: no database and an unencodable value are +// errors before any SQL runs; a failed statement is reported, not swallowed. +func TestStampJobArgRefusesBadInputs(t *testing.T) { + ctx := context.Background() + if err := jobs.StampJobArg(ctx, nil, 1, "k", "v"); err == nil { + t.Fatal("nil database must be refused") + } + pool := testutil.TestDB(t) + if err := jobs.StampJobArg(ctx, pool, 1, "k", make(chan int)); err == nil { + t.Fatal("unencodable value must be refused") + } + if err := jobs.StampJobArg(ctx, pool, 1, "k", "v"); err == nil { + // river_job may not exist on this fresh pool (no Migrate): the + // statement fails and the error must surface. + if _, qerr := pool.Exec(ctx, `SELECT 1 FROM river_job LIMIT 1`); qerr != nil { + t.Fatal("statement failure must be reported") + } + } +} diff --git a/internal/outbound/provider_authorization_guard_test.go b/internal/outbound/provider_authorization_guard_test.go new file mode 100644 index 000000000..b8f50bbad --- /dev/null +++ b/internal/outbound/provider_authorization_guard_test.go @@ -0,0 +1,215 @@ +package outbound + +import ( + "go/ast" + "go/parser" + "go/token" + "os" + "os/exec" + "path/filepath" + "strings" + "testing" +) + +// TestEveryProviderCallRequiresAuthorization is the tracked closure guard for +// the provider seam. It parses every tracked production Go file and rejects: +// +// - any import of net/smtp outside the relay itself and the named exceptions; +// - any call to the relay's private socket-opening core outside the one +// authorized adapter; +// - any exported relay method that could open a socket without a token. +// +// Exceptions are exact file paths (or one exact symbol), never substrings, +// and each is named here with the reason it may exist. Adding a +// provider-bound caller anywhere else fails this test until it goes through +// ProviderSubmitter.SubmitOnce. +// +// What it does not see, stated so nobody over-reads it: a second unexported +// dialer added inside smtp_relay.go under another name (that file may import +// net/smtp; the sentinel check catches a rename of the core, not an addition +// beside it), a mail-capable SDK other than the ones fenced below, and any +// provider reached over plain net/http. Those arrive as a new import or a new +// dependency, which is where review catches them. +func TestEveryProviderCallRequiresAuthorization(t *testing.T) { + root := moduleRoot(t) + files := trackedGoFiles(t, root) + + // Files that may import net/smtp: the relay (the only SES client) and the + // self-test scenarios, which drive a local SMTP conversation against + // e2a's OWN inbound listener to prove delivery end to end — never the + // provider. + smtpImportAllowed := map[string]string{ + "internal/outbound/smtp_relay.go": "the provider relay itself", + "internal/selftest/scenarios.go": "local inbound self-test client, not provider-bound", + } + // The ONE function that may reference the relay's socket-opening core: + // the authorized adapter's SubmitOnce. The exception is a symbol, not a + // file, so a second function added beside it is not exempt. + socketCallAllowed := map[string]string{ + "internal/outbound/provider_submit.go:SubmitOnce": "the one authorized adapter method", + } + // Provider SDKs that can send mail without SMTP, and the one package + // that may import each: sender-identity provisioning uses SES v2 for + // identities and tags, never SendEmail. A send through an HTTP provider + // API is invisible to the socket check, so the import is fenced instead. + providerSDKAllowed := map[string]map[string]string{ + "github.com/aws/aws-sdk-go-v2/service/sesv2": { + "internal/senderidentity/ses.go": "SES identity provisioning", + "internal/senderidentity/tags.go": "SES identity tagging", + }, + } + allowedSocketCalls := 0 + + fset := token.NewFileSet() + for _, rel := range files { + src, err := os.ReadFile(filepath.Join(root, rel)) + if err != nil { + t.Fatalf("read %s: %v", rel, err) + } + f, err := parser.ParseFile(fset, rel, src, parser.ImportsOnly|parser.ParseComments) + if err != nil { + t.Fatalf("parse %s: %v", rel, err) + } + for _, imp := range f.Imports { + path := strings.Trim(imp.Path.Value, `"`) + if path == "net/smtp" { + if _, ok := smtpImportAllowed[rel]; !ok { + t.Errorf("%s imports net/smtp: provider I/O must go through outbound.ProviderSubmitter (or be named in the guard's exception list with its reason)", rel) + } + } + if files, fenced := providerSDKAllowed[path]; fenced { + if _, ok := files[rel]; !ok { + t.Errorf("%s imports %s: a provider SDK may only be used where the guard names it, and never to send", rel, path) + } + } + } + full, err := parser.ParseFile(fset, rel, src, 0) + if err != nil { + t.Fatalf("parse %s: %v", rel, err) + } + // Any reference to the socket core counts, not only a direct call: + // a method value (`f := r.sendOnceContext`) or a method expression + // (`(*SMTPRelay).sendOnceContext`) is a SelectorExpr too, and either + // would otherwise let a caller open the socket one hop away from the + // name this guard looks for. + for _, decl := range full.Decls { + fn, isFunc := decl.(*ast.FuncDecl) + var enclosing string + if isFunc { + enclosing = rel + ":" + fn.Name.Name + } + ast.Inspect(decl, func(n ast.Node) bool { + sel, ok := n.(*ast.SelectorExpr) + if !ok || sel.Sel.Name != "sendOnceContext" { + return true + } + if rel == "internal/outbound/smtp_relay.go" && isFunc && fn.Name.Name == "sendOnceContext" { + return true // the definition's own receiver method is not a reference + } + if _, ok := socketCallAllowed[enclosing]; ok { + allowedSocketCalls++ + return true + } + t.Errorf("%s references the relay's socket-opening core outside ProviderSubmitter.SubmitOnce", fset.Position(sel.Pos())) + return true + }) + } + } + // The sentinel must be real: renaming the socket core would otherwise + // turn the whole reference check into a no-op that still passes. + if allowedSocketCalls == 0 { + t.Fatal("ProviderSubmitter.SubmitOnce no longer references sendOnceContext: the guard's sentinel is stale, update both together") + } + + // The relay's exported surface may not open a socket: Configured is a + // field read, and everything that dials is unexported. A newly exported + // Send* method is exactly the bypass this guard exists to refuse. + relaySrc, err := os.ReadFile(filepath.Join(root, "internal/outbound/smtp_relay.go")) + if err != nil { + t.Fatal(err) + } + relayFile, err := parser.ParseFile(fset, "smtp_relay.go", relaySrc, 0) + if err != nil { + t.Fatal(err) + } + for _, decl := range relayFile.Decls { + fn, ok := decl.(*ast.FuncDecl) + if !ok || fn.Recv == nil || len(fn.Recv.List) == 0 { + continue + } + recv := fn.Recv.List[0].Type + if star, ok := recv.(*ast.StarExpr); ok { + recv = star.X + } + if ident, ok := recv.(*ast.Ident); !ok || ident.Name != "SMTPRelay" { + continue + } + if fn.Name.IsExported() && fn.Name.Name != "Configured" { + t.Errorf("SMTPRelay exports %s: the relay must expose no socket-opening method", fn.Name.Name) + } + } +} + +// moduleRoot walks up from the package directory to the module's go.mod. +// It needs no git: a guard that skipped itself wherever git was absent (a +// source tarball, a container without the binary, a prebuilt test binary) +// would report green exactly where nobody was looking. +func moduleRoot(t *testing.T) string { + t.Helper() + dir, err := os.Getwd() + if err != nil { + t.Fatal(err) + } + for { + if _, err := os.Stat(filepath.Join(dir, "go.mod")); err == nil { + return dir + } + parent := filepath.Dir(dir) + if parent == dir { + t.Fatal("go.mod not found above the package directory") + } + dir = parent + } +} + +// trackedGoFiles lists the production (non-test) Go files under internal/ +// and cmd/. Git's index is the authority when available — it is what ships — +// and a filesystem walk is the fallback so the guard never skips. +func trackedGoFiles(t *testing.T, root string) []string { + t.Helper() + var files []string + cmd := exec.Command("git", "ls-files", "--", "internal/*.go", "internal/**/*.go", "cmd/*.go", "cmd/**/*.go") + cmd.Dir = root + if out, err := cmd.Output(); err == nil { + for _, line := range strings.Split(strings.TrimSpace(string(out)), "\n") { + if line == "" || strings.HasSuffix(line, "_test.go") { + continue + } + files = append(files, line) + } + } else { + for _, top := range []string{"internal", "cmd"} { + err := filepath.WalkDir(filepath.Join(root, top), func(path string, d os.DirEntry, err error) error { + if err != nil { + return err + } + if d.IsDir() || !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + rel, err := filepath.Rel(root, path) + if err != nil { + return err + } + files = append(files, filepath.ToSlash(rel)) + return nil + }) + if err != nil { + t.Fatalf("walk %s: %v", top, err) + } + } + } + if len(files) < 50 { + t.Fatalf("only %d production files found; the guard is scanning the wrong tree", len(files)) + } + return files +} diff --git a/internal/outbound/provider_submit.go b/internal/outbound/provider_submit.go index fa01de163..982b8f717 100644 --- a/internal/outbound/provider_submit.go +++ b/internal/outbound/provider_submit.go @@ -199,7 +199,7 @@ func (s *ProviderSubmitter) SubmitOnce(ctx context.Context, auth sendingpolicy.P // A failure after the body was fully written (ErrProviderAcceptanceUnknown) // is neither accepted nor rejected here: it is returned unsettled, because // the provider may hold the message and only its feedback can say. - providerID, sendErr := s.relay.SendOnceContext(ctx, env.From, auth.AuthorizedRecipients(), wire) + providerID, sendErr := s.relay.sendOnceContext(ctx, env.From, auth.AuthorizedRecipients(), wire) if sendErr != nil { // IsPermanentSMTPError is the worker's retry classifier: any 5xx, // including one raised before DATA (an AUTH 535, say). Settling such a diff --git a/internal/outbound/sender.go b/internal/outbound/sender.go index 769da13f6..388b21fcd 100644 --- a/internal/outbound/sender.go +++ b/internal/outbound/sender.go @@ -299,54 +299,6 @@ type ComposeResult struct { To, CC, BCC []string } -// Send normalizes recipients, composes, and sends an email via SMTP relay -// (the historical retrying submit). Returns a ValidationError for caller errors -// (bad addresses, no visible recipients) and a plain error for transport failures. -func (s *Sender) Send(agent *identity.AgentIdentity, req SendRequest) (*SendResult, error) { - c, err := s.compose(agent, req) - if err != nil { - return nil, err - } - sesMessageID, err := s.smtpRelay.Send(c.envelopeFrom, c.envelope, c.wire) - if err != nil { - return nil, fmt.Errorf("smtp relay: %w", err) - } - return &SendResult{ - MessageID: sesMessageID, - Method: "smtp", - SentAs: c.sentAs, - To: c.to, - CC: c.cc, - BCC: c.bcc, - Raw: c.sentBody, - }, nil -} - -// SendOnce is Send with a SINGLE SMTP submit and no internal retry loop — the -// entry point for a caller that owns its own retry envelope. Behaviorally -// identical to Send except it calls smtpRelay.SendOnce. (The async pipeline does -// NOT use this — it persists ComposeForAccept's bytes and the River worker -// submits them via SubmitOnce — but it is the direct single-attempt analogue.) -func (s *Sender) SendOnce(agent *identity.AgentIdentity, req SendRequest) (*SendResult, error) { - c, err := s.compose(agent, req) - if err != nil { - return nil, err - } - sesMessageID, err := s.smtpRelay.SendOnce(c.envelopeFrom, c.envelope, c.wire) - if err != nil { - return nil, fmt.Errorf("smtp relay: %w", err) - } - return &SendResult{ - MessageID: sesMessageID, - Method: "smtp", - SentAs: c.sentAs, - To: c.to, - CC: c.cc, - BCC: c.bcc, - Raw: c.sentBody, - }, nil -} - // ComposeForAccept composes an outbound message for the async accept path WITHOUT // submitting it. The accept-tx persists the returned bytes + envelope so the River // worker owns the actual SMTP submit; it reuses Send's exact compose stage (same @@ -368,35 +320,6 @@ func (s *Sender) ComposeForAccept(agent *identity.AgentIdentity, req SendRequest }, nil } -// SubmitOnce submits the persisted Sent-folder bytes in a SINGLE SMTP attempt -// (River owns retries) and returns the provider Message-ID. It attaches two -// wire-time headers post-DKIM (never in the signed header set): -// -// - X-E2A-Message-ID (delivery.MessageIDHeader) — the stable e2a correlation -// marker (async-send-contract §3.1). SES overrides supplied Message-ID/Date -// headers, but echoes original headers back in its notifications -// (mail.headers, when "include original headers" is enabled on the -// configuration set), so this is the value that correlates feedback for -// the SMTP-accept↔mark-sent crash window. Unlike the config-set header SES -// does NOT strip it — recipients see it too; it is deliberately a stable -// public marker. Stamped at submit time (not compose time) so messages -// accepted before this header existed still carry it on re-drive. -// -// - X-SES-CONFIGURATION-SET — re-attached because raw_message is stored -// WITHOUT it (SES strips it before delivery; the recipient/Sent-folder -// copy must not carry it). -// -// Keeping the header logic here (not in the worker) means Send and the async -// path share one source of truth for what SES actually receives. -func (s *Sender) SubmitOnce(messageID, envelopeFrom string, recipients []string, sentBody []byte) (string, error) { - return s.SubmitOnceContext(context.Background(), messageID, envelopeFrom, recipients, sentBody) -} - -// SubmitOnceContext is SubmitOnce with caller cancellation propagated to SMTP. -func (s *Sender) SubmitOnceContext(ctx context.Context, messageID, envelopeFrom string, recipients []string, sentBody []byte) (string, error) { - return s.smtpRelay.SendOnceContext(ctx, envelopeFrom, recipients, s.applySESConfigSet(applyCorrelationHeader(sentBody, messageID))) -} - // applyCorrelationHeader prepends the X-E2A-Message-ID marker. The id is // server-minted, but sanitize anyway — this is a header write. Empty id // (defensive) = no header. diff --git a/internal/outbound/smtp_relay.go b/internal/outbound/smtp_relay.go index b67753423..c6d77184b 100644 --- a/internal/outbound/smtp_relay.go +++ b/internal/outbound/smtp_relay.go @@ -5,7 +5,6 @@ import ( "crypto/tls" "errors" "fmt" - "log" "net" "net/smtp" "net/textproto" @@ -14,11 +13,8 @@ import ( "time" "github.com/tokencanopy/e2a/internal/config" - "github.com/tokencanopy/e2a/internal/logredact" ) -var smtpRetryBackoffs = []time.Duration{1 * time.Second, 5 * time.Second, 15 * time.Second} - // ErrProviderAcceptanceUnknown marks a failure that happened AFTER the whole // message body was handed to the provider: the terminating dot was written and // the 250 never arrived. The provider may have accepted the message. No @@ -40,85 +36,12 @@ func (r *SMTPRelay) Configured() bool { return r.cfg.Host != "" } -// Send sends an email to one or more recipients and returns the Message-ID assigned by the remote server (e.g. SES). -func (r *SMTPRelay) Send(from string, recipients []string, message []byte) (string, error) { - return r.SendWithContext(context.Background(), from, recipients, message) -} - -// SendWithContext sends an email while honoring ctx during SMTP I/O and retry -// backoff. It is intended for request-bound callers that cannot allow the -// relay's normal retry envelope to outlive the request budget. -func (r *SMTPRelay) SendWithContext(ctx context.Context, from string, recipients []string, message []byte) (string, error) { - return r.SendWithEnvelopeContext(ctx, from, recipients, message) -} - -// SendWithEnvelope sends an email using envelopeFrom for SMTP MAIL FROM. -// Issues RCPT TO for each recipient. If any RCPT TO is rejected, the transaction is aborted. -// Returns the Message-ID assigned by the remote SMTP server from the DATA response. -// Retries transient SMTP errors (4xx) up to 3 times with backoff. -func (r *SMTPRelay) SendWithEnvelope(envelopeFrom string, recipients []string, message []byte) (string, error) { - return r.SendWithEnvelopeContext(context.Background(), envelopeFrom, recipients, message) -} - -// SendWithEnvelopeContext is SendWithEnvelope with caller-controlled -// cancellation and deadline propagation. -func (r *SMTPRelay) SendWithEnvelopeContext(ctx context.Context, envelopeFrom string, recipients []string, message []byte) (string, error) { - if !r.Configured() { - return "", fmt.Errorf("outbound SMTP relay not configured") - } - - var lastErr error - for attempt := 0; attempt <= len(smtpRetryBackoffs); attempt++ { - msgID, err := r.sendOnceContext(ctx, envelopeFrom, recipients, message) - if err == nil { - return msgID, nil - } - lastErr = err - if ctx.Err() != nil { - return "", ctx.Err() - } - if !isTransientSMTPError(lastErr) { - return "", lastErr - } - if attempt < len(smtpRetryBackoffs) { - // lastErr is an upstream MTA response and cannot be perfectly - // sanitized: rejections routinely quote the recipient back at us - // ("550 5.1.1 : user unknown"), which would - // otherwise defeat the recipient redaction on this same line. Cap - // it so at most a bounded slice of provider text is retained; the - // full error still reaches the caller and the message row. - log.Printf("[smtp-relay] transient error sending to recipient_count=%d recipient_domains=%v (attempt %d/%d), retrying in %s: %s", - len(recipients), logredact.AddressDomains(recipients), attempt+1, len(smtpRetryBackoffs)+1, smtpRetryBackoffs[attempt], logredact.Truncate(lastErr.Error(), 200)) - select { - case <-time.After(smtpRetryBackoffs[attempt]): - case <-ctx.Done(): - return "", ctx.Err() - } - } - } - return "", lastErr -} - -// SendOnce performs a SINGLE SMTP submit — no internal retry loop — and returns -// the provider Message-ID. This is the entry point for the River outbound worker -// (internal/outboundsend), which owns the retry envelope: River reschedules the -// next attempt per the worker's NextRetry, so the relay must NOT loop (a loop here -// would hide the envelope from river_job and make each Work() run up to ~6.5 min). -// Classify the returned error with IsTransientSMTPError — transient (4xx/throttle) -// → let River retry; permanent (5xx/validation) → fail the message terminally. -func (r *SMTPRelay) SendOnce(envelopeFrom string, recipients []string, message []byte) (string, error) { - return r.SendOnceContext(context.Background(), envelopeFrom, recipients, message) -} - -// SendOnceContext is SendOnce with caller cancellation propagated into the -// SMTP dial/command path. River workers use it so remotely cancelling a running -// job can stop provider I/O promptly. -func (r *SMTPRelay) SendOnceContext(ctx context.Context, envelopeFrom string, recipients []string, message []byte) (string, error) { - if !r.Configured() { - return "", fmt.Errorf("outbound SMTP relay not configured") - } - return r.sendOnceContext(ctx, envelopeFrom, recipients, message) -} +// The relay exposes no socket-opening method. Every provider call is made by +// the ProviderSubmitter in this package through sendOnceContext, after the +// caller's authorization token has been redeemed; there is no in-process +// retry loop either, because a retry is a new charged attempt that only the +// sending-protection gate may allocate. The tracked guard test +// (provider_authorization_guard_test.go) keeps it that way. // IsTransientSMTPError reports whether err is a retryable SMTP failure (4xx / // throttle) vs a permanent one. Exported so the River worker's deliverer can set diff --git a/internal/outbound/smtp_relay_test.go b/internal/outbound/smtp_relay_test.go index ee4cb3f12..e03a2035a 100644 --- a/internal/outbound/smtp_relay_test.go +++ b/internal/outbound/smtp_relay_test.go @@ -12,7 +12,7 @@ import ( "github.com/tokencanopy/e2a/internal/config" ) -func TestSMTPRelaySendWithContextCancelsHangingServer(t *testing.T) { +func TestSMTPRelayCancelsHangingServer(t *testing.T) { listener, err := net.Listen("tcp", "127.0.0.1:0") if err != nil { t.Fatal(err) @@ -40,24 +40,24 @@ func TestSMTPRelaySendWithContextCancelsHangingServer(t *testing.T) { defer cancel() started := time.Now() - _, err = relay.SendWithContext(ctx, "noreply@example.com", []string{"feedback@example.com"}, []byte("Subject: test\r\n\r\nbody")) + _, err = relay.sendOnceContext(ctx, "noreply@example.com", []string{"feedback@example.com"}, []byte("Subject: test\r\n\r\nbody")) if !errors.Is(err, context.DeadlineExceeded) { - t.Fatalf("SendWithContext error = %v, want context deadline exceeded", err) + t.Fatalf("sendOnceContext error = %v, want context deadline exceeded", err) } if elapsed := time.Since(started); elapsed > time.Second { - t.Fatalf("SendWithContext returned after %s, want cancellation within 1s", elapsed) + t.Fatalf("sendOnceContext returned after %s, want cancellation within 1s", elapsed) } } -func TestSMTPRelaySendOnceContextHonorsCancellation(t *testing.T) { +func TestSMTPRelayHonorsCancellation(t *testing.T) { relay := NewSMTPRelay(&config.OutboundSMTPConfig{Host: "127.0.0.1", Port: 1}) ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := relay.SendOnceContext(ctx, "noreply@example.com", + _, err := relay.sendOnceContext(ctx, "noreply@example.com", []string{"recipient@example.com"}, []byte("Subject: test\r\n\r\nbody")) if !errors.Is(err, context.Canceled) { - t.Fatalf("SendOnceContext error = %v, want context canceled", err) + t.Fatalf("sendOnceContext error = %v, want context canceled", err) } } diff --git a/internal/sendingpolicy/operations.go b/internal/sendingpolicy/operations.go index 237ad600b..e0781a498 100644 --- a/internal/sendingpolicy/operations.go +++ b/internal/sendingpolicy/operations.go @@ -281,18 +281,41 @@ func (m *Module) PrepareNotificationTx(ctx context.Context, tx pgx.Tx, ref Notif return OperationRef{}, ErrSourceUnavailable } - var userID string + var userID, operationID string var err error switch ref.source { case NotificationHITLMessage: userID, err = lockHITLSourceOwner(ctx, tx, ref.id) + operationID = HITLNotificationOperationID(ref.id) case NotificationWebhookHealth: + // The operation is keyed by the episode the sweep stamped in the + // same transaction that enqueues the notice, so preparing the same + // episode twice (an enqueue and a later legacy resolve, or two + // resolvers racing) yields one operation, and a job whose reference + // names another episode is detectably stale. + var warnedAt, disabledAt *time.Time err = tx.QueryRow(ctx, - `SELECT user_id FROM webhooks WHERE id = $1 FOR UPDATE`, ref.id, - ).Scan(&userID) + `SELECT user_id, warn_notified_at, auto_disabled_at FROM webhooks WHERE id = $1 FOR UPDATE`, ref.id, + ).Scan(&userID, &warnedAt, &disabledAt) if errors.Is(err, pgx.ErrNoRows) { err = ErrSourceUnavailable } + if err == nil { + var episode *time.Time + switch ref.kind { + case WebhookHealthKindWarning: + episode = warnedAt + case WebhookHealthKindDisabled: + episode = disabledAt + } + if episode == nil { + // Unknown kind, or an episode the sweep never stamped: + // there is no notice to send, so there is nothing to + // authorize. + return OperationRef{}, ErrSourceUnavailable + } + operationID = WebhookHealthOperationID(ref.id, ref.kind, *episode) + } default: return OperationRef{}, ErrSourceUnavailable } @@ -308,7 +331,7 @@ func (m *Module) PrepareNotificationTx(ctx context.Context, tx pgx.Tx, ref Notif } row, err := insertOperation(ctx, tx, operationRow{ - OperationID: randomID("op_"), + OperationID: operationID, SourceAccountRef: &userID, PolicySubjectRef: userID, Purpose: PurposeCustomerNotification, diff --git a/internal/sendingpolicy/store_integration_test.go b/internal/sendingpolicy/store_integration_test.go index 2d160a733..d2a53f432 100644 --- a/internal/sendingpolicy/store_integration_test.go +++ b/internal/sendingpolicy/store_integration_test.go @@ -172,8 +172,8 @@ func (f *fixture) webhook(userID string) string { id := fmt.Sprintf("wh_gate_%d", messageSeq+1000) messageSeq++ if _, err := f.pool.Exec(f.ctx, - `INSERT INTO webhooks (id, user_id, url, signing_secret, events) - VALUES ($1, $2, $3, $4, ARRAY['message.received'])`, + `INSERT INTO webhooks (id, user_id, url, signing_secret, events, enabled, auto_disabled_at) + VALUES ($1, $2, $3, $4, ARRAY['message.received'], false, now())`, id, userID, "https://hook.example.test/"+id, "secret", ); err != nil { f.t.Fatalf("insert webhook: %v", err) @@ -421,7 +421,7 @@ func TestSharedMailboxAndNotificationsShareOneAccountCounter(t *testing.T) { var hookRef sendingpolicy.OperationRef f.inTx(func(tx pgx.Tx) error { var err error - hookRef, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook)) + hookRef, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook, sendingpolicy.WebhookHealthKindDisabled)) return err }) d := f.authorize(g, hookRef) @@ -1126,3 +1126,99 @@ func TestReputationClassCannotBecomeCheaperAfterPreparation(t *testing.T) { t.Fatalf("tightening in the safe direction must still send: %q", d.Reason) } } + +// TestNotificationOperationsAreKeyedBySource: preparing the same held +// message or the same webhook health episode twice yields ONE operation, so +// an enqueue and a later legacy resolve (or two resolvers racing) cannot mint +// a second operation that nothing settles; a different episode is a +// different operation; an episode the sweep never stamped has nothing to +// authorize. +func TestNotificationOperationsAreKeyedBySource(t *testing.T) { + f := newFixture(t) + g := f.gate(enforcingPolicy(nil)) + user := f.user("standard") + agent := f.agent(user) + held := f.pendingMessage(agent, "relay") + + var first, second sendingpolicy.OperationRef + f.inTx(func(tx pgx.Tx) error { + var err error + first, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewHITLNotificationRef(held)) + return err + }) + f.inTx(func(tx pgx.Tx) error { + var err error + second, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewHITLNotificationRef(held)) + return err + }) + if first.ID() != sendingpolicy.HITLNotificationOperationID(held) || first.ID() != second.ID() { + t.Fatalf("hitl operation ids = %q / %q, want both %q", first.ID(), second.ID(), sendingpolicy.HITLNotificationOperationID(held)) + } + + hook := f.webhook(user) + var episode time.Time + if err := f.pool.QueryRow(f.ctx, `SELECT auto_disabled_at FROM webhooks WHERE id = $1`, hook).Scan(&episode); err != nil { + t.Fatal(err) + } + var disabled1, disabled2 sendingpolicy.OperationRef + f.inTx(func(tx pgx.Tx) error { + var err error + disabled1, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook, sendingpolicy.WebhookHealthKindDisabled)) + return err + }) + f.inTx(func(tx pgx.Tx) error { + var err error + disabled2, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook, sendingpolicy.WebhookHealthKindDisabled)) + return err + }) + want := sendingpolicy.WebhookHealthOperationID(hook, sendingpolicy.WebhookHealthKindDisabled, episode) + if disabled1.ID() != want || disabled2.ID() != want { + t.Fatalf("webhook operation ids = %q / %q, want both %q", disabled1.ID(), disabled2.ID(), want) + } + + // No warning episode was ever stamped: nothing to authorize. + err := f.tryTx(func(tx pgx.Tx) error { + _, err := g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook, sendingpolicy.WebhookHealthKindWarning)) + return err + }) + if !errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + t.Fatalf("unstamped warning episode: err = %v, want ErrSourceUnavailable", err) + } + err = f.tryTx(func(tx pgx.Tx) error { + _, err := g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook, "bogus")) + return err + }) + if !errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + t.Fatalf("unknown kind: err = %v, want ErrSourceUnavailable", err) + } + + // A later episode (the webhook recovered and was disabled again) is a + // new operation. + if _, err := f.pool.Exec(f.ctx, `UPDATE webhooks SET auto_disabled_at = auto_disabled_at + interval '1 hour' WHERE id = $1`, hook); err != nil { + t.Fatal(err) + } + var disabled3 sendingpolicy.OperationRef + f.inTx(func(tx pgx.Tx) error { + var err error + disabled3, err = g.PrepareNotificationTx(f.ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(hook, sendingpolicy.WebhookHealthKindDisabled)) + return err + }) + if disabled3.ID() == disabled1.ID() { + t.Fatalf("a new episode must be a new operation, got %q twice", disabled3.ID()) + } +} + +// tryTx runs fn in a transaction that is rolled back on error and returns +// fn's error, for the paths a fixture expects to be refused. +func (f *fixture) tryTx(fn func(tx pgx.Tx) error) error { + f.t.Helper() + tx, err := f.pool.Begin(f.ctx) + if err != nil { + f.t.Fatalf("begin: %v", err) + } + defer func() { _ = tx.Rollback(f.ctx) }() + if err := fn(tx); err != nil { + return err + } + return tx.Commit(f.ctx) +} diff --git a/internal/sendingpolicy/types.go b/internal/sendingpolicy/types.go index dad75ab5e..013c2bc9f 100644 --- a/internal/sendingpolicy/types.go +++ b/internal/sendingpolicy/types.go @@ -348,6 +348,60 @@ const ( type NotificationRef struct { source NotificationSource id string + // kind is the webhook health episode kind (WebhookHealthKindWarning or + // WebhookHealthKindDisabled); empty for every other source. + kind string +} + +// Source exposes the notification source, for tests and logging. +func (r NotificationRef) Source() NotificationSource { return r.source } + +// SourceID exposes the source row id. +func (r NotificationRef) SourceID() string { return r.id } + +// Kind exposes the webhook health episode kind; empty for other sources. +func (r NotificationRef) Kind() string { return r.kind } + +// Webhook health episode kinds. They mirror the notification job's own +// vocabulary; the notify package asserts the two agree. +const ( + WebhookHealthKindWarning = "warning" + WebhookHealthKindDisabled = "disabled" +) + +// HITLNotificationOperationID is the operation id of the approval request +// for one held message. Deriving it from the message makes +// PrepareNotificationTx idempotent per hold and lets the worker bind a +// job's reference to its source the way the message worker does. +func HITLNotificationOperationID(messageID string) string { + return hitlOperationPrefix + messageID +} + +const ( + hitlOperationPrefix = "op_hitl_" + webhookHealthOperationPrefix = "op_wh_" +) + +// IsHITLNotificationOperationID reports whether an id has the source-derived +// shape above. An id of any other shape — migration 113 stamped adopted +// notify jobs with `op_` — is a pre-derivation reference: its source is +// still the job's own, so a worker re-derives rather than refuses it. +func IsHITLNotificationOperationID(id string) bool { + return strings.HasPrefix(id, hitlOperationPrefix) +} + +// IsWebhookHealthOperationID reports whether an id has the episode-derived +// shape; see IsHITLNotificationOperationID for what any other shape means. +func IsWebhookHealthOperationID(id string) bool { + return strings.HasPrefix(id, webhookHealthOperationPrefix) +} + +// WebhookHealthOperationID is the operation id of one webhook health +// episode: the kind plus the timestamp the sweep stamped when it flipped +// the state (warn_notified_at or auto_disabled_at). A webhook that recovers +// and fails again is a new episode with a new operation. +func WebhookHealthOperationID(webhookID, kind string, episode time.Time) string { + return fmt.Sprintf("%s%s_%s_%d", webhookHealthOperationPrefix, kind, webhookID, episode.UTC().UnixMicro()) } // NewHITLNotificationRef references a pending outbound message whose approval @@ -358,10 +412,13 @@ func NewHITLNotificationRef(messageID string) NotificationRef { return NotificationRef{source: NotificationHITLMessage, id: messageID} } -// NewWebhookHealthNotificationRef references a webhook whose health episode is -// being reported to its owner. -func NewWebhookHealthNotificationRef(webhookID string) NotificationRef { - return NotificationRef{source: NotificationWebhookHealth, id: webhookID} +// NewWebhookHealthNotificationRef references a webhook whose health episode +// of the given kind (WebhookHealthKindWarning / WebhookHealthKindDisabled) is +// being reported to its owner. PrepareNotificationTx reads the episode's +// timestamp from the locked webhook row; an unknown kind or an episode the +// sweep never stamped is ErrSourceUnavailable. +func NewWebhookHealthNotificationRef(webhookID, kind string) NotificationRef { + return NotificationRef{source: NotificationWebhookHealth, id: webhookID, kind: kind} } // ProtectionNoticeRef names one already-committed notice event and audience. @@ -524,12 +581,13 @@ func (a ProviderAuthorization) Attempt() AttemptRef { return a.attempt } // Purpose exposes the derived purpose, for metrics. func (a ProviderAuthorization) Purpose() Purpose { return a.purpose } -// AuthorizedRecipients returns a defensive copy of the exact final envelope. -// -// Only the protection notifier uses it: that path is the one caller that does -// not already know its recipient, because the address is resolved under lock at -// final authorization and deliberately never persisted in plaintext. Every -// other caller composed its own envelope and must not re-derive one here. +// AuthorizedRecipients is the normalized recipient set this token permits, +// in canonical order. The protection notifier and public feedback compose +// their envelope from it — their recipients are configuration the gate +// already resolved, never a customer-controlled list. Every other caller +// composed its own envelope from the source row and hands that to the seam, +// which proves it names exactly these mailboxes (ValidateEnvelope) before it +// dials; a mismatch there fails closed rather than being re-derived here. func (a ProviderAuthorization) AuthorizedRecipients() []string { out := make([]string, len(a.recipients)) copy(out, a.recipients) diff --git a/internal/testutil/contract_server.go b/internal/testutil/contract_server.go index 63ae44ad5..332603cdd 100644 --- a/internal/testutil/contract_server.go +++ b/internal/testutil/contract_server.go @@ -122,9 +122,10 @@ func StartContractServer(ctx context.Context, dbURL string) (*ContractServer, er // disabled policy (pass-through admission, every attempt still durable) // and the authorized submitter that refuses to dial without its token. sendingGate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + providerSubmitter := outbound.NewProviderSubmitter(smtpRelay, sendingGate) outboundJobs := outboundsend.NewJobs( outboundSendStore, - agent.NewOutboundDeliverer(outbound.NewProviderSubmitter(smtpRelay, sendingGate)), + agent.NewOutboundDeliverer(providerSubmitter), pool, ).WithGate(sendingGate) jobsClient, err := jobs.New(pool, jobs.Config{OutboundWorkers: 1}, outboundJobs) @@ -137,6 +138,7 @@ func StartContractServer(ctx context.Context, dbURL string) (*ContractServer, er router := mux.NewRouter() api := agent.NewAPI(store, sender, smtpRelay, nil, noopUsage, "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + api.SetProviderSubmitter(providerSubmitter, sendingGate) api.SetIdempotencyStore(idempotencyStore) api.SetEnforcer(enforcer) api.SetUsageStore(usageStore) diff --git a/internal/testutil/server.go b/internal/testutil/server.go index 22d37e500..489fd7c42 100644 --- a/internal/testutil/server.go +++ b/internal/testutil/server.go @@ -222,9 +222,10 @@ func TestServer(t *testing.T, pool *pgxpool.Pool, opts ...TestServerOption) *E2A // disabled policy (pass-through admission, every attempt still durable) // and the authorized submitter that refuses to dial without its token. sendingGate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + providerSubmitter := outbound.NewProviderSubmitter(smtpRelay, sendingGate) outboundJobs := outboundsend.NewJobs( outboundSendStore, - agent.NewOutboundDeliverer(outbound.NewProviderSubmitter(smtpRelay, sendingGate)), + agent.NewOutboundDeliverer(providerSubmitter), pool, ).WithGate(sendingGate) jobsClient, err := jobs.New(pool, jobs.Config{OutboundWorkers: 2}, outboundJobs) @@ -247,6 +248,7 @@ func TestServer(t *testing.T, pool *pgxpool.Pool, opts ...TestServerOption) *E2A }, time.Minute) idempotencyStore := idempotency.NewStore(pool) api := agent.NewAPI(store, sender, smtpRelay, nil, noopUsage, "e2a.dev", "test.e2a.dev", "agents.e2a.dev", "", false) + api.SetProviderSubmitter(providerSubmitter, sendingGate) api.SetIdempotencyStore(idempotencyStore) api.SetSubscriberStore(subscriberStore) api.SetOutbox(outbox) diff --git a/internal/webhooknotify/e2e_test.go b/internal/webhooknotify/e2e_test.go index 3f6ed375e..019f7d96f 100644 --- a/internal/webhooknotify/e2e_test.go +++ b/internal/webhooknotify/e2e_test.go @@ -13,6 +13,7 @@ import ( "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/jobs" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/testutil" "github.com/tokencanopy/e2a/internal/webhooknotify" ) @@ -49,9 +50,10 @@ func newE2EHarness(t *testing.T, replyTo string) *e2eHarness { relay := outbound.NewSMTPRelay(&config.OutboundSMTPConfig{ Host: smtpAddr.Host, Port: smtpAddr.Port, FromDomain: "notify.test", }) - notifier := webhooknotify.New(store, relay, "notify.test", "", replyTo, "https://app.example.test") + gate := sendingpolicy.NewGate(pool, sendingpolicy.Secrets{}, sendingpolicy.PolicySourceConfig, sendingpolicy.DisabledPolicy()) + notifier := webhooknotify.New(store, outbound.NewProviderSubmitter(relay, gate), "notify.test", "", replyTo, "https://app.example.test") - j := webhooknotify.NewJobs(store) + j := webhooknotify.NewJobs(store).WithGate(gate, pool) client, err := jobs.New(pool, jobs.Config{}, j) if err != nil { t.Fatalf("jobs.New: %v", err) diff --git a/internal/webhooknotify/jobs.go b/internal/webhooknotify/jobs.go index 954874d49..ce8f9cb7f 100644 --- a/internal/webhooknotify/jobs.go +++ b/internal/webhooknotify/jobs.go @@ -3,13 +3,17 @@ package webhooknotify import ( "context" "errors" + "fmt" "sync" "github.com/jackc/pgx/v5" + "github.com/jackc/pgx/v5/pgxpool" "github.com/riverqueue/river" "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/jobs" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) // Jobs is the webhook health-notification integration on the shared River @@ -29,6 +33,8 @@ type Jobs struct { store Store enq jobs.Enqueuer metrics Metrics + gate sendingpolicy.Gate + pool *pgxpool.Pool mu sync.RWMutex deliverer Deliverer @@ -38,6 +44,18 @@ type Jobs struct { // deliverer yet). func NewJobs(store Store) *Jobs { return &Jobs{store: store} } +// WithGate injects the sending-protection gate and the pool its legacy +// resolver and arg stamp use. Chainable; nil keeps the gateless default. +func (j *Jobs) WithGate(g sendingpolicy.Gate, pool *pgxpool.Pool) *Jobs { + if g != nil { + j.gate = g + } + if pool != nil { + j.pool = pool + } + return j +} + // SetEnqueuer injects the shared client so the EnqueueTx methods can // insert jobs. func (j *Jobs) SetEnqueuer(e jobs.Enqueuer) { j.enq = e } @@ -51,20 +69,38 @@ func (j *Jobs) SetDeliverer(d Deliverer) { j.mu.Unlock() } -// Deliver makes Jobs itself the worker's Deliverer, delegating to the -// concrete one set via SetDeliverer. Until that is wired (the brief -// startup window before the notifier is built) it returns a retryable -// outcome, so a pending job simply retries rather than dropping. -func (j *Jobs) Deliver(ctx context.Context, wh *identity.Webhook, kind string) DeliverOutcome { - j.mu.RLock() - d := j.deliverer - j.mu.RUnlock() +// Compose makes Jobs itself the worker's Deliverer, delegating to the +// concrete one set via SetDeliverer. Until that is wired (the brief startup +// window before the notifier is built) it returns a retryable outcome — and +// because Compose runs before any attempt is charged, that window costs +// nothing. +func (j *Jobs) Compose(ctx context.Context, wh *identity.Webhook, kind string) (outbound.Envelope, DeliverOutcome) { + d := j.currentDeliverer() + if d == nil { + return outbound.Envelope{}, DeliverOutcome{Err: errors.New("webhook notifier not wired yet — retrying")} + } + return d.Compose(ctx, wh, kind) +} + +// Submit delegates the authorized submission to the concrete Deliverer. +func (j *Jobs) Submit(ctx context.Context, env outbound.Envelope, auth sendingpolicy.ProviderAuthorization) DeliverOutcome { + d := j.currentDeliverer() if d == nil { return DeliverOutcome{Err: errors.New("webhook notifier not wired yet — retrying")} } - return d.Deliver(ctx, wh, kind) + return d.Submit(ctx, env, auth) } +func (j *Jobs) currentDeliverer() Deliverer { + j.mu.RLock() + defer j.mu.RUnlock() + return j.deliverer +} + +// Gate exposes the wired sending-protection gate (nil when gateless), so the +// composition root's wiring test can prove the production bundle is armed. +func (j *Jobs) Gate() sendingpolicy.Gate { return j.gate } + // WithMetrics wires the observability backend the NotifyWorker emits the // notification-outcome counter on. Nil-safe; call before RegisterJobs. func (j *Jobs) WithMetrics(m Metrics) *Jobs { @@ -76,15 +112,62 @@ func (j *Jobs) WithMetrics(m Metrics) *Jobs { // Deliverer). No periodics — the maintenance sweep is the only producer. // Implements jobs.Registrar. func (j *Jobs) RegisterJobs(w *river.Workers) []*river.PeriodicJob { - river.AddWorker(w, NewNotifyWorker(j.store, j).WithMetrics(j.metrics)) + river.AddWorker(w, j.NotifyWorker()) return nil } +// NotifyWorker builds the fully armed worker RegisterJobs registers. +func (j *Jobs) NotifyWorker() *NotifyWorker { + w := NewNotifyWorker(j.store, j).WithMetrics(j.metrics).WithGate(j.gate).WithOperationResolver(j.ResolveLegacyOperation) + if j.pool != nil { + w = w.WithArgStamper(func(ctx context.Context, jobID int64, ref sendingpolicy.OperationRef) error { + return jobs.StampJobArg(ctx, j.pool, jobID, "operation_ref", ref) + }).WithArgRestamper(func(ctx context.Context, jobID int64, ref sendingpolicy.OperationRef) error { + return jobs.SetJobArg(ctx, j.pool, jobID, "operation_ref", ref) + }) + } + return w +} + +// ResolveLegacyOperation prepares the notification operation for a job that +// carries no reference, in its own committed transaction, through the same +// PrepareNotificationTx the sweep's enqueue runs. +func (j *Jobs) ResolveLegacyOperation(ctx context.Context, webhookID, kind string) (sendingpolicy.OperationRef, error) { + if j.gate == nil || j.pool == nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("webhook notify: legacy operation resolver is not wired") + } + tx, err := j.pool.Begin(ctx) + if err != nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("begin legacy resolve: %w", err) + } + defer func() { _ = tx.Rollback(ctx) }() + ref, err := j.gate.PrepareNotificationTx(ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(webhookID, kind)) + if err != nil { + return sendingpolicy.OperationRef{}, err + } + if err := tx.Commit(ctx); err != nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("commit legacy resolve: %w", err) + } + return ref, nil +} + // EnqueueWebhookNotifyTx inserts one webhook_notify job in the caller's // transaction — the maintenance sweep's, so the state transition and its // notification job commit atomically (the design's SC2 argument). +// +// With a gate wired the notification's operation is prepared here, against +// the locked webhook row, so the owning account is charged and the worker +// never derives attribution. func (j *Jobs) EnqueueWebhookNotifyTx(ctx context.Context, tx pgx.Tx, webhookID, kind string) (int64, error) { - res, err := j.enq.InsertTx(ctx, tx, WebhookNotifyArgs{WebhookID: webhookID, NotifyKind: kind}, &river.InsertOpts{ + args := WebhookNotifyArgs{WebhookID: webhookID, NotifyKind: kind} + if j.gate != nil { + ref, err := j.gate.PrepareNotificationTx(ctx, tx, sendingpolicy.NewWebhookHealthNotificationRef(webhookID, kind)) + if err != nil { + return 0, fmt.Errorf("prepare notification operation: %w", err) + } + args.OperationRef = &ref + } + res, err := j.enq.InsertTx(ctx, tx, args, &river.InsertOpts{ Queue: jobs.QueueNotify, MaxAttempts: MaxNotifyAttempts, }) diff --git a/internal/webhooknotify/notifier.go b/internal/webhooknotify/notifier.go index 659d24f4b..dc095e8d3 100644 --- a/internal/webhooknotify/notifier.go +++ b/internal/webhooknotify/notifier.go @@ -5,13 +5,13 @@ import ( "errors" "fmt" "html" - "log" "net/url" "strings" "time" "github.com/tokencanopy/e2a/internal/identity" "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) // notifyLocalPart is the fallback local-part of the sender address, used @@ -51,15 +51,15 @@ type NotifierStore interface { // relay is the narrow send surface (*outbound.SMTPRelay satisfies it). // SendOnce, not Send: this runs inside a River job, so River owns retries. -type relay interface { - SendOnce(envelopeFrom string, recipients []string, message []byte) (string, error) +type submitter interface { + SubmitOnce(ctx context.Context, auth sendingpolicy.ProviderAuthorization, env outbound.Envelope) (outbound.ProviderResult, error) } // Notifier composes and sends the two webhook health emails. Construct // with New; the NotifyWorker drives Deliver. type Notifier struct { - store NotifierStore - relay relay + store NotifierStore + submitter submitter // dkim, when non-nil, signs each email for the From-header domain // before it reaches the relay (see WithDKIM). dkim outbound.DKIMKeyLookup @@ -89,7 +89,7 @@ type Notifier struct { // local part on fromDomain; replyTo is the optional // notifications.reply_to config value, empty = no Reply-To header. // publicURL builds the dashboard link; empty degrades to generic copy. -func New(store NotifierStore, r relay, fromDomain, fromAddress, replyTo, publicURL string) *Notifier { +func New(store NotifierStore, s submitter, fromDomain, fromAddress, replyTo, publicURL string) *Notifier { addr := strings.TrimSpace(fromAddress) if addr == "" { addr = fmt.Sprintf("%s@%s", notifyLocalPart, fromDomain) @@ -100,7 +100,7 @@ func New(store NotifierStore, r relay, fromDomain, fromAddress, replyTo, publicU } return &Notifier{ store: store, - relay: r, + submitter: s, fromAddress: addr, fromDomain: msgIDDomain, replyTo: strings.TrimSpace(replyTo), @@ -129,33 +129,63 @@ func (n *Notifier) WithDKIM(lookup outbound.DKIMKeyLookup) *Notifier { return n } -// Deliver composes and sends one health email, classifying the result for -// the NotifyWorker. Implements Deliverer. -func (n *Notifier) Deliver(ctx context.Context, wh *identity.Webhook, kind string) DeliverOutcome { - if err := n.send(ctx, wh, kind); err != nil { - return DeliverOutcome{ - Err: err, - Permanent: outbound.IsPermanentSMTPError(err) || errors.Is(err, errNoOwnerEmail), - Outage: outbound.IsConnectionError(err), - } +// Compose implements Deliverer: the provider-free half (owner lookup, failure +// stats, MIME, Message-ID, DKIM), classified like a send so the worker +// treats a permanent compose failure the same way. +func (n *Notifier) Compose(ctx context.Context, wh *identity.Webhook, kind string) (outbound.Envelope, DeliverOutcome) { + if n == nil { + return outbound.Envelope{}, DeliverOutcome{Err: fmt.Errorf("webhook notify: notifier is nil")} } - return DeliverOutcome{} + env, err := n.compose(ctx, wh, kind) + if err != nil { + return outbound.Envelope{}, classify(err) + } + return env, DeliverOutcome{} } -func (n *Notifier) send(ctx context.Context, wh *identity.Webhook, kind string) error { +// Submit implements Deliverer: one authorized submission, classified for the +// NotifyWorker. +func (n *Notifier) Submit(ctx context.Context, env outbound.Envelope, auth sendingpolicy.ProviderAuthorization) DeliverOutcome { if n == nil { - return nil + return DeliverOutcome{Err: fmt.Errorf("webhook notify: notifier is nil")} } + if _, err := n.submitter.SubmitOnce(ctx, auth, env); err != nil { + return classify(fmt.Errorf("webhook notify: smtp send: %w", err)) + } + return DeliverOutcome{} +} + +// Deliver composes and sends one health email with an already-authorized +// attempt: Compose then Submit in one call, for callers that hold the token +// up front (tests). The worker runs the two phases itself so the token is +// consumed last. +func (n *Notifier) Deliver(ctx context.Context, wh *identity.Webhook, kind string, auth sendingpolicy.ProviderAuthorization) DeliverOutcome { + env, out := n.Compose(ctx, wh, kind) + if out.Err != nil { + return out + } + return n.Submit(ctx, env, auth) +} + +func classify(err error) DeliverOutcome { + return DeliverOutcome{ + Err: err, + Permanent: outbound.IsPermanentSMTPError(err) || errors.Is(err, errNoOwnerEmail), + Outage: outbound.IsConnectionError(err), + } +} + +func (n *Notifier) compose(ctx context.Context, wh *identity.Webhook, kind string) (outbound.Envelope, error) { if wh == nil { - return fmt.Errorf("webhook notify: webhook is nil") + return outbound.Envelope{}, fmt.Errorf("webhook notify: webhook is nil") } owner, err := n.store.GetUserByID(ctx, wh.UserID) if err != nil { - return fmt.Errorf("webhook notify: lookup owner: %w", err) + return outbound.Envelope{}, fmt.Errorf("webhook notify: lookup owner: %w", err) } if owner.Email == "" { - return fmt.Errorf("webhook notify: owner %s: %w", owner.ID, errNoOwnerEmail) + return outbound.Envelope{}, fmt.Errorf("webhook notify: owner %s: %w", owner.ID, errNoOwnerEmail) } window := identity.WarnWindow @@ -164,7 +194,7 @@ func (n *Notifier) send(ctx context.Context, wh *identity.Webhook, kind string) } stats, err := n.store.RecentWebhookFailureStats(ctx, wh.ID, window) if err != nil { - return fmt.Errorf("webhook notify: failure stats: %w", err) + return outbound.Envelope{}, fmt.Errorf("webhook notify: failure stats: %w", err) } reason := stats.LastError @@ -207,7 +237,7 @@ func (n *Notifier) send(ctx context.Context, wh *identity.Webhook, kind string) "", // no conversation_id ) if err != nil { - return fmt.Errorf("webhook notify: compose: %w", err) + return outbound.Envelope{}, fmt.Errorf("webhook notify: compose: %w", err) } // Deterministic Message-ID so a crash-after-send re-drive collapses at @@ -235,12 +265,7 @@ func (n *Notifier) send(ctx context.Context, wh *identity.Webhook, kind string) message = signed } - if _, err := n.relay.SendOnce(n.fromAddress, []string{owner.Email}, message); err != nil { - return fmt.Errorf("webhook notify: smtp send: %w", err) - } - - log.Printf("[webhook-notify] sent %s email: webhook=%s owner=%s", kind, wh.ID, owner.ID) - return nil + return outbound.Envelope{From: n.fromAddress, Recipients: []string{owner.Email}, Message: message}, nil } // endpointLabel condenses the webhook URL for the subject line: host when diff --git a/internal/webhooknotify/notifier_test.go b/internal/webhooknotify/notifier_test.go index c3913f34c..5a2493be4 100644 --- a/internal/webhooknotify/notifier_test.go +++ b/internal/webhooknotify/notifier_test.go @@ -9,6 +9,8 @@ import ( "github.com/tokencanopy/e2a/internal/dkim" "github.com/tokencanopy/e2a/internal/identity" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) type stubStore struct { @@ -33,9 +35,14 @@ type captureRelay struct { err error } -func (r *captureRelay) SendOnce(from string, to []string, msg []byte) (string, error) { - r.from, r.to, r.message = from, to, msg - return "queued-id", r.err +// SubmitOnce satisfies the notifier's submitter seam: it captures the envelope +// the notifier hands over and returns the scripted error. +func (r *captureRelay) SubmitOnce(_ context.Context, _ sendingpolicy.ProviderAuthorization, env outbound.Envelope) (outbound.ProviderResult, error) { + r.from, r.to, r.message = env.From, env.Recipients, env.Message + if r.err != nil { + return outbound.ProviderResult{}, r.err + } + return outbound.ProviderResult{ProviderMessageID: "queued-id"}, nil } func testWebhook() *identity.Webhook { @@ -63,7 +70,7 @@ func TestNotifier_DisabledEmailContent(t *testing.T) { relay := &captureRelay{} n := New(okStore(), relay, "send.example.com", "", "", "https://app.example.com") - out := n.Deliver(context.Background(), testWebhook(), KindDisabled) + out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}) if out.Err != nil { t.Fatalf("Deliver: %v", out.Err) } @@ -111,7 +118,7 @@ func TestNotifier_WarningEmailContent(t *testing.T) { wh.Enabled = true wh.AutoDisabledAt = nil wh.AutoDisableReason = "" - out := n.Deliver(context.Background(), wh, KindWarning) + out := n.Deliver(context.Background(), wh, KindWarning, sendingpolicy.ProviderAuthorization{}) if out.Err != nil { t.Fatalf("Deliver: %v", out.Err) } @@ -140,7 +147,7 @@ func TestNotifier_ConfiguredFromAddress(t *testing.T) { if got := n.FromAddress(); got != "support@corp.example" { t.Fatalf("FromAddress = %q", got) } - out := n.Deliver(context.Background(), testWebhook(), KindDisabled) + out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}) if out.Err != nil { t.Fatalf("Deliver: %v", out.Err) } @@ -172,7 +179,7 @@ func TestNotifier_ConfiguredReplyTo(t *testing.T) { relay := &captureRelay{} n := New(okStore(), relay, "send.example.com", "support@send.example.com", "support@agents.example.com", "") - if out := n.Deliver(context.Background(), testWebhook(), KindDisabled); out.Err != nil { + if out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}); out.Err != nil { t.Fatalf("Deliver: %v", out.Err) } msg := string(relay.message) @@ -192,7 +199,7 @@ func TestNotifier_NoOwnerEmailIsPermanent(t *testing.T) { st.owner = &identity.User{ID: "user_1", Email: ""} n := New(st, &captureRelay{}, "send.example.com", "", "", "") - out := n.Deliver(context.Background(), testWebhook(), KindDisabled) + out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}) if out.Err == nil { t.Fatal("expected an error for a missing owner email") } @@ -206,7 +213,7 @@ func TestNotifier_TransientStoreErrorIsRetryable(t *testing.T) { st.statsErr = errors.New("db blip") n := New(st, &captureRelay{}, "send.example.com", "", "", "") - out := n.Deliver(context.Background(), testWebhook(), KindDisabled) + out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}) if out.Err == nil { t.Fatal("expected an error") } @@ -244,7 +251,7 @@ func TestNotifier_SignsWithDKIMWhenKeyExists(t *testing.T) { relay := &captureRelay{} n := New(okStore(), relay, "send.example.com", "support@corp.example", "", "").WithDKIM(lookup) - if out := n.Deliver(context.Background(), testWebhook(), KindDisabled); out.Err != nil { + if out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}); out.Err != nil { t.Fatalf("Deliver: %v", out.Err) } msg := string(relay.message) @@ -264,7 +271,7 @@ func TestNotifier_SendsUnsignedWhenNoDKIMKey(t *testing.T) { relay := &captureRelay{} n := New(okStore(), relay, "send.example.com", "", "", "").WithDKIM(lookup) - if out := n.Deliver(context.Background(), testWebhook(), KindDisabled); out.Err != nil { + if out := n.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}); out.Err != nil { t.Fatalf("Deliver must succeed unsigned: %v", out.Err) } if strings.Contains(string(relay.message), "DKIM-Signature:") { @@ -273,7 +280,7 @@ func TestNotifier_SendsUnsignedWhenNoDKIMKey(t *testing.T) { // And with no lookup wired at all (zero-config self-host). relay2 := &captureRelay{} n2 := New(okStore(), relay2, "send.example.com", "", "", "") - if out := n2.Deliver(context.Background(), testWebhook(), KindDisabled); out.Err != nil { + if out := n2.Deliver(context.Background(), testWebhook(), KindDisabled, sendingpolicy.ProviderAuthorization{}); out.Err != nil { t.Fatalf("Deliver must succeed without a DKIM lookup: %v", out.Err) } } @@ -288,7 +295,7 @@ func TestNotifier_ReasonIsHTMLEscaped(t *testing.T) { wh := testWebhook() wh.AutoDisableReason = "" - if out := n.Deliver(context.Background(), wh, KindDisabled); out.Err != nil { + if out := n.Deliver(context.Background(), wh, KindDisabled, sendingpolicy.ProviderAuthorization{}); out.Err != nil { t.Fatalf("Deliver: %v", out.Err) } // The text/plain part may carry the raw string (harmless in plain diff --git a/internal/webhooknotify/worker.go b/internal/webhooknotify/worker.go index e33e5bf14..4d5ad400f 100644 --- a/internal/webhooknotify/worker.go +++ b/internal/webhooknotify/worker.go @@ -22,6 +22,8 @@ import ( "github.com/riverqueue/river" "github.com/tokencanopy/e2a/internal/identity" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" ) // Notification kinds. One worker, two templates: the guards and the @@ -56,6 +58,10 @@ const notifyOutageSnooze = 5 * time.Minute // of truth) each attempt, so the guards always see current state. type WebhookNotifyArgs struct { WebhookID string `json:"webhook_id"` + // OperationRef is the durable sending operation the sweep's transaction + // prepared; a job from a pre-floor slot carries none and is resolved at + // fire time, then stamped. + OperationRef *sendingpolicy.OperationRef `json:"operation_ref,omitempty"` // NotifyKind ∈ {warning, disabled}. (Named NotifyKind because river's // JobArgs interface reserves the Kind() method name.) NotifyKind string `json:"kind"` @@ -72,12 +78,36 @@ type DeliverOutcome struct { Outage bool // relay unreachable — snooze without spending an attempt } -// Deliverer composes and sends one health email. Implemented by *Notifier -// (compose + SMTPRelay.SendOnce + classify). +// Deliverer is the two-phase send of one health email. Compose does every +// fallible, provider-free step (owner lookup, failure stats, MIME, DKIM) and +// returns the envelope; Submit hands that envelope and a freshly consumed +// authorization to the provider seam. The split lets the worker +// ConsumeAttempt immediately before the socket opens, so a compose failure +// costs no charged ordinal. Implemented by *Notifier. type Deliverer interface { - Deliver(ctx context.Context, wh *identity.Webhook, kind string) DeliverOutcome + Compose(ctx context.Context, wh *identity.Webhook, kind string) (outbound.Envelope, DeliverOutcome) + Submit(ctx context.Context, env outbound.Envelope, auth sendingpolicy.ProviderAuthorization) DeliverOutcome } +// OperationResolver recovers the durable operation for a job that carries no +// reference, through the same Prepare path the sweep's enqueue runs. The kind +// selects the episode (warning or disable) the operation is keyed by. +type OperationResolver func(ctx context.Context, webhookID, kind string) (sendingpolicy.OperationRef, error) + +// errOperationMismatch marks a job whose operation reference names another +// episode's (or another webhook's) operation: authorizing it would charge the +// wrong operation, and a reference for a superseded episode is stale anyway. +var errOperationMismatch = errors.New("webhook notify: job operation reference does not name this episode") + +// maxNotifyAge bounds how long a health notice may wait behind a gate hold. +// A pause has no clock of its own, and a disabled webhook never self-clears, +// so without this a held notice would snooze forever; a week-old health +// notice is stale by any reading. +const maxNotifyAge = 7 * 24 * time.Hour + +// ArgStamper persists a resolved reference into the job's args. +type ArgStamper func(ctx context.Context, jobID int64, ref sendingpolicy.OperationRef) error + // Store is the read surface the worker needs. *identity.Store satisfies it. type Store interface { // GetWebhookByIDInternal loads the webhook with no ownership check — @@ -112,6 +142,10 @@ type NotifyWorker struct { river.WorkerDefaults[WebhookNotifyArgs] store Store deliverer Deliverer + gate sendingpolicy.Gate + resolve OperationResolver + stamp ArgStamper + restamp ArgStamper metrics Metrics // nil ⇒ no emission (nil-safe via emitNotify) } @@ -121,6 +155,40 @@ func NewNotifyWorker(store Store, deliverer Deliverer) *NotifyWorker { // WithMetrics swaps in a metrics backend. Nil-safe: unset (or nil) means no // emission, so tests and self-host builds don't have to wire anything. +// WithGate injects the sending-protection gate every notification must pass. +func (w *NotifyWorker) WithGate(g sendingpolicy.Gate) *NotifyWorker { + if g != nil { + w.gate = g + } + return w +} + +// WithOperationResolver injects the legacy-argument resolver. +func (w *NotifyWorker) WithOperationResolver(r OperationResolver) *NotifyWorker { + if r != nil { + w.resolve = r + } + return w +} + +// WithArgStamper injects the job-args stamp used after a legacy resolution +// (adds the reference only when absent). +func (w *NotifyWorker) WithArgStamper(s ArgStamper) *NotifyWorker { + if s != nil { + w.stamp = s + } + return w +} + +// WithArgRestamper injects the unconditional re-key used when a job carries +// a pre-derivation reference. +func (w *NotifyWorker) WithArgRestamper(s ArgStamper) *NotifyWorker { + if s != nil { + w.restamp = s + } + return w +} + func (w *NotifyWorker) WithMetrics(m Metrics) *NotifyWorker { w.metrics = m return w @@ -184,27 +252,186 @@ func (w *NotifyWorker) Work(ctx context.Context, job *river.Job[WebhookNotifyArg w.emitNotify(kind, outcomeSkipped) return nil } + if kind == KindDisabled && wh.AutoDisabledAt == nil { + // Guard 5: disabled by hand, not by the breaker — there is no + // auto-disable episode to report. + w.emitNotify(kind, outcomeSkipped) + return nil + } + if !job.CreatedAt.IsZero() && time.Since(job.CreatedAt) > maxNotifyAge { + // Guard 6: a notice that waited a week behind a hold is stale; drop + // it rather than snooze forever behind a paused account. + log.Printf("[webhook-notify] dropping %s notice for %s: older than %s", kind, wh.ID, maxNotifyAge) + w.emitNotify(kind, outcomeSkipped) + return nil + } - out := w.deliverer.Deliver(ctx, wh, kind) + // Compose first: the owner lookup, failure stats, MIME and DKIM are + // fallible and provider-free, so they run before any attempt is charged. + env, out := w.deliverer.Compose(ctx, wh, kind) + if out.Err != nil { + return w.verdict(job, wh.ID, kind, "compose", out) + } + + // Every provider call is authorized: Reserve, hold without I/O, then + // ConsumeAttempt as the LAST decision before Submit, whose submitter + // redeems the token immediately before the socket opens. A health notice + // has no durable hold class; the guards above re-run on every execution + // and drop a notice that went stale while it waited. + auth := sendingpolicy.ProviderAuthorization{} + if w.gate != nil { + ref, err := w.operationFor(ctx, job, wh) + if err != nil { + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + w.emitNotify(kind, outcomeSkipped) + return nil + } + if errors.Is(err, errOperationMismatch) { + w.emitNotify(kind, outcomeSkipped) + return river.JobCancel(err) + } + w.emitNotify(kind, outcomeRetryable) + return err + } + early, attempt, err := w.gate.Reserve(ctx, ref) + if err != nil { + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + w.emitNotify(kind, outcomeSkipped) + return nil + } + w.emitNotify(kind, outcomeOutage) + return river.JobSnooze(notifyOutageSnooze) + } + if !early.Allow { + return w.holdVerdict(kind, early) + } + decision, token, err := w.gate.ConsumeAttempt(ctx, attempt) + if err != nil { + if errors.Is(err, sendingpolicy.ErrSourceUnavailable) { + w.emitNotify(kind, outcomeSkipped) + return nil + } + w.emitNotify(kind, outcomeOutage) + return river.JobSnooze(notifyOutageSnooze) + } + if !decision.Allow || token == nil { + return w.holdVerdict(kind, decision) + } + auth = *token + } + + out = w.deliverer.Submit(ctx, env, auth) if out.Err == nil { + log.Printf("[webhook-notify] sent %s email: webhook=%s", kind, wh.ID) w.emitNotify(kind, outcomeSent) return nil } + return w.verdict(job, wh.ID, kind, "send", out) +} + +// verdict turns a classified failure into River's answer. +func (w *NotifyWorker) verdict(job *river.Job[WebhookNotifyArgs], webhookID, kind, phase string, out DeliverOutcome) error { if out.Permanent { // e.g. the owner address is rejected 5xx, or there is no owner email // on record. Cancel (no retry) rather than churn the tail. - log.Printf("[webhook-notify] permanent send failure for %s (%s, no retry): %v", wh.ID, kind, out.Err) + log.Printf("[webhook-notify] permanent %s failure for %s (%s, no retry): %v", phase, webhookID, kind, out.Err) w.emitNotify(kind, outcomePermanent) return river.JobCancel(out.Err) } if out.Outage { // Relay unreachable — snooze without burning an attempt. The guards - // above re-run on the next attempt, so a notification that goes - // stale during the outage still drops correctly. + // re-run on the next attempt, so a notification that goes stale + // during the outage still drops correctly. w.emitNotify(kind, outcomeOutage) return river.JobSnooze(notifyOutageSnooze) } // Transient: let River reschedule per NextRetry until MaxNotifyAttempts. w.emitNotify(kind, outcomeRetryable) - return fmt.Errorf("webhook notify attempt %d failed: %w", job.Attempt, out.Err) + return fmt.Errorf("webhook notify attempt %d %s failed: %w", job.Attempt, phase, out.Err) } + +// operationFor returns the job's durable operation, resolving and stamping a +// legacy job through the sweep's Prepare path. +func (w *NotifyWorker) operationFor(ctx context.Context, job *river.Job[WebhookNotifyArgs], wh *identity.Webhook) (sendingpolicy.OperationRef, error) { + // The episode's operation is derived from the webhook, the kind and the + // timestamp the sweep stamped, so a reference naming any other operation + // is either another account's (never authorize it) or a superseded + // episode's (nothing left to say): the binding the message worker + // enforces, checked before Reserve. + want := ExpectedOperationID(wh, job.Args.NotifyKind) + stamp := w.stamp + if job.Args.OperationRef != nil && !job.Args.OperationRef.IsZero() { + stored := job.Args.OperationRef.ID() + if stored == want { + return *job.Args.OperationRef, nil + } + if sendingpolicy.IsWebhookHealthOperationID(stored) { + // A derived id for another webhook or a superseded episode. (Any + // other shape is re-derived from this job's own source below, so no + // stored id can redirect attribution.) + return sendingpolicy.OperationRef{}, errOperationMismatch + } + // A pre-derivation reference (migration 113's op_, or the first + // build of this seam): its source is still this job's own webhook, + // so re-derive through the same Prepare path and replace it, once. + log.Printf("[webhook-notify] job %d carries a pre-derivation operation reference %s; re-keying", job.ID, stored) + stamp = w.restamp + } + if w.resolve == nil { + return sendingpolicy.OperationRef{}, fmt.Errorf("webhook notify: legacy job %d carries no operation and no resolver is wired", job.ID) + } + ref, err := w.resolve(ctx, job.Args.WebhookID, job.Args.NotifyKind) + if err != nil { + return sendingpolicy.OperationRef{}, err + } + if ref.ID() != want { + return sendingpolicy.OperationRef{}, errOperationMismatch + } + if stamp != nil { + if err := stamp(ctx, job.ID, ref); err != nil { + log.Printf("[webhook-notify] stamp operation on legacy job %d: %v", job.ID, err) + } + } + return ref, nil +} + +// holdVerdict turns a gate hold into River's answer: a terminal hold cancels +// the job; everything else waits for the gate's retry time or the outage pace. +func (w *NotifyWorker) holdVerdict(kind string, d sendingpolicy.Decision) error { + if d.Terminal { + w.emitNotify(kind, outcomePermanent) + return river.JobCancel(fmt.Errorf("webhook notify: sending policy: %s", d.Reason)) + } + w.emitNotify(kind, outcomeOutage) + delay := notifyOutageSnooze + if !d.RetryAt.IsZero() { + if until := time.Until(d.RetryAt); until > delay { + delay = until + } + } + return river.JobSnooze(delay) +} + +// ExpectedOperationID is the operation a notice of the given kind for this +// webhook's current episode must carry: the same derivation the gate's +// PrepareNotificationTx uses. Empty when the episode was never stamped. +func ExpectedOperationID(wh *identity.Webhook, kind string) string { + if wh == nil { + return "" + } + var episode *time.Time + switch kind { + case KindWarning: + episode = wh.WarnNotifiedAt + case KindDisabled: + episode = wh.AutoDisabledAt + } + if episode == nil { + return "" + } + return sendingpolicy.WebhookHealthOperationID(wh.ID, kind, *episode) +} + +// Gate exposes the wired gate (nil when gateless), for the composition +// root's wiring test. +func (w *NotifyWorker) Gate() sendingpolicy.Gate { return w.gate } diff --git a/internal/webhooknotify/worker_test.go b/internal/webhooknotify/worker_test.go index 4990ada0d..d40909125 100644 --- a/internal/webhooknotify/worker_test.go +++ b/internal/webhooknotify/worker_test.go @@ -2,15 +2,19 @@ package webhooknotify_test import ( "context" + "encoding/json" "errors" "strings" "testing" "time" + "github.com/jackc/pgx/v5" "github.com/riverqueue/river" "github.com/riverqueue/river/rivertype" "github.com/tokencanopy/e2a/internal/identity" + "github.com/tokencanopy/e2a/internal/outbound" + "github.com/tokencanopy/e2a/internal/sendingpolicy" "github.com/tokencanopy/e2a/internal/webhooknotify" ) @@ -24,17 +28,38 @@ func (f *fakeStore) GetWebhookByIDInternal(_ context.Context, _ string) (*identi } type fakeDeliverer struct { - out webhooknotify.DeliverOutcome - called int - kinds []string + out webhooknotify.DeliverOutcome // Submit's outcome + composeOut webhooknotify.DeliverOutcome // Compose's outcome + called int // Submit calls + composed int + kinds []string + auths []sendingpolicy.ProviderAuthorization + trace *[]string } -func (f *fakeDeliverer) Deliver(_ context.Context, _ *identity.Webhook, kind string) webhooknotify.DeliverOutcome { - f.called++ +func (f *fakeDeliverer) Compose(_ context.Context, _ *identity.Webhook, kind string) (outbound.Envelope, webhooknotify.DeliverOutcome) { + f.composed++ f.kinds = append(f.kinds, kind) + f.record("compose") + if f.composeOut.Err != nil { + return outbound.Envelope{}, f.composeOut + } + return outbound.Envelope{From: "e2a@notify.test", Recipients: []string{"owner@reviewer.test"}, Message: []byte("Subject: x\r\n\r\nbody")}, webhooknotify.DeliverOutcome{} +} + +func (f *fakeDeliverer) Submit(_ context.Context, _ outbound.Envelope, auth sendingpolicy.ProviderAuthorization) webhooknotify.DeliverOutcome { + f.called++ + f.record("submit") + f.auths = append(f.auths, auth) return f.out } +func (f *fakeDeliverer) record(step string) { + if f.trace != nil { + *f.trace = append(*f.trace, step) + } +} + func job(webhookID, kind string, attempt int) *river.Job[webhooknotify.WebhookNotifyArgs] { return &river.Job[webhooknotify.WebhookNotifyArgs]{ JobRow: &rivertype.JobRow{Attempt: attempt, MaxAttempts: webhooknotify.MaxNotifyAttempts, Kind: webhooknotify.WebhookNotifyArgs{}.Kind()}, @@ -42,14 +67,24 @@ func job(webhookID, kind string, attempt int) *river.Job[webhooknotify.WebhookNo } } +// episodeAt is the fixed auto-disable timestamp every disabled fixture +// carries: the breaker stamps it when it flips a webhook, and the operation +// a disable notice authorizes under is keyed by it. +var episodeAt = time.Date(2026, 9, 1, 12, 0, 0, 0, time.UTC) + func hook(enabled bool, warnedAt *time.Time) *identity.Webhook { - return &identity.Webhook{ + wh := &identity.Webhook{ ID: "wh_test", UserID: "user_test", URL: "https://hooks.example.com/inbox", Enabled: enabled, WarnNotifiedAt: warnedAt, } + if !enabled { + at := episodeAt + wh.AutoDisabledAt = &at + } + return wh } func now() *time.Time { t := time.Now(); return &t } @@ -232,3 +267,275 @@ func TestNotifyWorker_ErrorTriage(t *testing.T) { fm.only(t, webhooknotify.KindDisabled, "retryable") }) } + +// fakeGate is a scriptable sendingpolicy.Gate for the worker-order tests. +type fakeGate struct { + trace *[]string + reserve sendingpolicy.Decision + consume sendingpolicy.Decision + reserveErr error + reserves int + consumes int +} + +func allowAll() *fakeGate { + return &fakeGate{reserve: sendingpolicy.Decision{Allow: true}, consume: sendingpolicy.Decision{Allow: true}} +} + +func (g *fakeGate) PrepareExternalTx(context.Context, pgx.Tx, string) (sendingpolicy.AcceptanceDecision, sendingpolicy.OperationRef, error) { + return sendingpolicy.AcceptanceAccept, sendingpolicy.OperationRef{}, nil +} +func (g *fakeGate) PrepareNotificationTx(context.Context, pgx.Tx, sendingpolicy.NotificationRef) (sendingpolicy.OperationRef, error) { + return refFor("op_prepared"), nil +} +func (g *fakeGate) PrepareProtectionNoticeTx(context.Context, pgx.Tx, sendingpolicy.ProtectionNoticeRef) (sendingpolicy.OperationRef, error) { + return sendingpolicy.OperationRef{}, nil +} +func (g *fakeGate) PreparePublicFeedback(context.Context, sendingpolicy.PublicFeedbackRef) (sendingpolicy.OperationRef, error) { + return sendingpolicy.OperationRef{}, nil +} +func (g *fakeGate) Reserve(context.Context, sendingpolicy.OperationRef) (sendingpolicy.Decision, sendingpolicy.AttemptRef, error) { + g.reserves++ + g.record("reserve") + return g.reserve, sendingpolicy.AttemptRef{}, g.reserveErr +} +func (g *fakeGate) ConsumeAttempt(context.Context, sendingpolicy.AttemptRef) (sendingpolicy.Decision, *sendingpolicy.ProviderAuthorization, error) { + g.consumes++ + g.record("consume") + if !g.consume.Allow { + return g.consume, nil, nil + } + return g.consume, &sendingpolicy.ProviderAuthorization{}, nil +} +func (g *fakeGate) RedeemProviderCall(context.Context, sendingpolicy.ProviderAuthorization) error { + return nil +} +func (g *fakeGate) DeferAttempt(context.Context, sendingpolicy.AttemptRef) error { return nil } +func (g *fakeGate) CancelAttempt(context.Context, sendingpolicy.AttemptRef) error { return nil } +func (g *fakeGate) SettleProvider(context.Context, sendingpolicy.ProviderSettlement) error { + return nil +} +func (g *fakeGate) SettleOperation(context.Context, sendingpolicy.OperationRef, sendingpolicy.SettlementOutcome, string) error { + return nil +} +func (g *fakeGate) LookupOperation(_ context.Context, id string) (sendingpolicy.OperationRef, error) { + return refFor(id), nil +} + +func refFor(id string) sendingpolicy.OperationRef { + var ref sendingpolicy.OperationRef + if err := json.Unmarshal([]byte(`{"v":1,"id":"`+id+`"}`), &ref); err != nil { + panic(err) + } + return ref +} + +// gatedJob carries the operation a notice of this kind for the disabled +// fixture (hook(false, …)) is keyed by; a warning fixture passes its own +// webhook through gatedJobFor. +func gatedJob(webhookID, kind string, attempt int) *river.Job[webhooknotify.WebhookNotifyArgs] { + wh := hook(false, nil) + wh.ID = webhookID + if kind == webhooknotify.KindWarning { + wh.Enabled = true + wh.WarnNotifiedAt = now() + } + return gatedJobFor(wh, kind, attempt) +} + +func gatedJobFor(wh *identity.Webhook, kind string, attempt int) *river.Job[webhooknotify.WebhookNotifyArgs] { + j := job(wh.ID, kind, attempt) + ref := refFor(webhooknotify.ExpectedOperationID(wh, kind)) + j.Args.OperationRef = &ref + return j +} + +func isSnooze(err error) bool { + var snooze *river.JobSnoozeError + return errors.As(err, &snooze) +} + +func TestNotifyWorker_GatedPathAuthorizesThenDelivers(t *testing.T) { + fd := &fakeDeliverer{} + fm := &fakeMetrics{} + g := allowAll() + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(fm).WithGate(g) + if err := w.Work(context.Background(), gatedJob("wh_test", webhooknotify.KindDisabled, 1)); err != nil { + t.Fatalf("Work: %v", err) + } + if g.reserves != 1 || g.consumes != 1 || fd.called != 1 { + t.Fatalf("reserves=%d consumes=%d delivers=%d, want 1/1/1", g.reserves, g.consumes, fd.called) + } +} + +func TestNotifyWorker_GateHoldSnoozesWithoutDelivery(t *testing.T) { + for name, g := range map[string]*fakeGate{ + "early hold": {reserve: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonAccountPaused}}, + "late hold": {reserve: sendingpolicy.Decision{Allow: true}, consume: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonGlobalAllBudget, RetryAt: time.Now().Add(time.Hour)}}, + "gate error": {reserveErr: errors.New("policy db down")}, + } { + fd := &fakeDeliverer{} + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(g) + if err := w.Work(context.Background(), gatedJob("wh_test", webhooknotify.KindDisabled, 1)); !isSnooze(err) || fd.called != 0 { + t.Fatalf("%s: err=%v delivers=%d, want snooze with no I/O", name, err, fd.called) + } + } +} + +func TestNotifyWorker_LegacyJobResolvesAndStampsOnce(t *testing.T) { + fd := &fakeDeliverer{} + resolved, stamped := 0, 0 + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(allowAll()). + WithOperationResolver(func(_ context.Context, id, kind string) (sendingpolicy.OperationRef, error) { + resolved++ + wh := hook(false, nil) + wh.ID = id + return refFor(webhooknotify.ExpectedOperationID(wh, kind)), nil + }). + WithArgStamper(func(context.Context, int64, sendingpolicy.OperationRef) error { stamped++; return nil }) + if err := w.Work(context.Background(), job("wh_test", webhooknotify.KindDisabled, 1)); err != nil { + t.Fatalf("Work: %v", err) + } + if resolved != 1 || stamped != 1 || fd.called != 1 { + t.Fatalf("resolved=%d stamped=%d delivers=%d, want 1/1/1", resolved, stamped, fd.called) + } +} + +func (g *fakeGate) record(step string) { + if g.trace != nil { + *g.trace = append(*g.trace, step) + } +} + +// TestNotifyWorker_ComposeRunsBeforeAnyChargeAndConsumeIsLast pins the order +// the seam depends on: compose precedes Reserve, ConsumeAttempt is the last +// call before Submit. +func TestNotifyWorker_ComposeRunsBeforeAnyChargeAndConsumeIsLast(t *testing.T) { + var trace []string + fd := &fakeDeliverer{trace: &trace} + g := allowAll() + g.trace = &trace + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(g) + if err := w.Work(context.Background(), gatedJob("wh_test", webhooknotify.KindDisabled, 1)); err != nil { + t.Fatalf("Work: %v", err) + } + if got := strings.Join(trace, ","); got != "compose,reserve,consume,submit" { + t.Fatalf("order = %s, want compose,reserve,consume,submit", got) + } +} + +// TestNotifyWorker_ComposeFailureChargesNothing: a compose failure precedes +// Reserve, so it burns no ordinal. +func TestNotifyWorker_ComposeFailureChargesNothing(t *testing.T) { + for name, tc := range map[string]struct { + out webhooknotify.DeliverOutcome + wantErr func(error) bool + }{ + "transient": {out: webhooknotify.DeliverOutcome{Err: errors.New("stats blip")}, wantErr: func(err error) bool { return err != nil && !isSnooze(err) && !isCancel(err) }}, + "permanent": {out: webhooknotify.DeliverOutcome{Err: errors.New("no owner email"), Permanent: true}, wantErr: isCancel}, + "outage": {out: webhooknotify.DeliverOutcome{Err: errors.New("dkim store down"), Outage: true}, wantErr: isSnooze}, + } { + fd := &fakeDeliverer{composeOut: tc.out} + g := allowAll() + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(g) + err := w.Work(context.Background(), gatedJob("wh_test", webhooknotify.KindDisabled, 1)) + if !tc.wantErr(err) { + t.Fatalf("%s: err = %v", name, err) + } + if g.reserves != 0 || g.consumes != 0 || fd.called != 0 { + t.Fatalf("%s: reserves=%d consumes=%d submits=%d, want 0/0/0", name, g.reserves, g.consumes, fd.called) + } + } +} + +// TestNotifyWorker_ForeignOrStaleOperationReferenceIsCancelled: a reference +// naming another webhook's operation, or a superseded episode of this one, +// is cancelled before Reserve. +func TestNotifyWorker_ForeignOrStaleOperationReferenceIsCancelled(t *testing.T) { + other := hook(false, nil) + other.ID = "wh_other" + stale := hook(false, nil) + at := episodeAt.Add(-time.Hour) + stale.AutoDisabledAt = &at + for name, ref := range map[string]sendingpolicy.OperationRef{ + "foreign webhook": refFor(webhooknotify.ExpectedOperationID(other, webhooknotify.KindDisabled)), + "stale episode": refFor(webhooknotify.ExpectedOperationID(stale, webhooknotify.KindDisabled)), + } { + fd := &fakeDeliverer{} + g := allowAll() + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(g) + j := job("wh_test", webhooknotify.KindDisabled, 1) + r := ref + j.Args.OperationRef = &r + if err := w.Work(context.Background(), j); !isCancel(err) { + t.Fatalf("%s: err = %v, want cancel", name, err) + } + if g.reserves != 0 || fd.called != 0 { + t.Fatalf("%s: reserves=%d submits=%d, want 0/0", name, g.reserves, fd.called) + } + } +} + +// TestNotifyWorker_StaleNoticeIsDropped: a notice older than the age bound +// is dropped instead of snoozing forever behind a hold. +func TestNotifyWorker_StaleNoticeIsDropped(t *testing.T) { + fd := &fakeDeliverer{} + g := &fakeGate{reserve: sendingpolicy.Decision{Allow: false, Reason: sendingpolicy.ReasonAccountPaused}} + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(g) + j := gatedJob("wh_test", webhooknotify.KindDisabled, 1) + j.CreatedAt = time.Now().Add(-8 * 24 * time.Hour) + if err := w.Work(context.Background(), j); err != nil { + t.Fatalf("err = %v, want a silent drop", err) + } + if g.reserves != 0 || fd.composed != 0 || fd.called != 0 { + t.Fatalf("reserves=%d composes=%d submits=%d, want 0/0/0", g.reserves, fd.composed, fd.called) + } +} + +// TestKindVocabularyMatchesGate: the job's kinds are the gate's episode kinds. +func TestKindVocabularyMatchesGate(t *testing.T) { + if webhooknotify.KindWarning != sendingpolicy.WebhookHealthKindWarning || webhooknotify.KindDisabled != sendingpolicy.WebhookHealthKindDisabled { + t.Fatal("webhooknotify kinds and sendingpolicy webhook health kinds disagree") + } +} + +func isCancel(err error) bool { + var cancel *river.JobCancelError + return errors.As(err, &cancel) +} + +// TestNotifyWorker_PreDerivationReferenceIsReKeyed: a job stamped before the +// episode-derived ids existed (migration 113's op_) is re-resolved and +// its reference replaced, not cancelled. +func TestNotifyWorker_PreDerivationReferenceIsReKeyed(t *testing.T) { + fd := &fakeDeliverer{} + g := allowAll() + resolved, stamped, restamped := 0, 0, 0 + var restampedWith string + w := webhooknotify.NewNotifyWorker(&fakeStore{wh: hook(false, nil)}, fd).WithMetrics(&fakeMetrics{}).WithGate(g). + WithOperationResolver(func(_ context.Context, id, kind string) (sendingpolicy.OperationRef, error) { + resolved++ + wh := hook(false, nil) + wh.ID = id + return refFor(webhooknotify.ExpectedOperationID(wh, kind)), nil + }). + WithArgStamper(func(context.Context, int64, sendingpolicy.OperationRef) error { stamped++; return nil }). + WithArgRestamper(func(_ context.Context, _ int64, ref sendingpolicy.OperationRef) error { + restamped++ + restampedWith = ref.ID() + return nil + }) + j := job("wh_test", webhooknotify.KindDisabled, 1) + legacy := refFor("op_0123456789abcdef0123456789abcdef") + j.Args.OperationRef = &legacy + if err := w.Work(context.Background(), j); err != nil { + t.Fatalf("Work: %v", err) + } + want := webhooknotify.ExpectedOperationID(hook(false, nil), webhooknotify.KindDisabled) + if resolved != 1 || restamped != 1 || stamped != 0 || restampedWith != want { + t.Fatalf("resolved=%d restamped=%d stamped=%d with=%q, want 1/1/0 with %q", resolved, restamped, stamped, restampedWith, want) + } + if g.reserves != 1 || fd.called != 1 { + t.Fatalf("reserves=%d submits=%d, want 1/1", g.reserves, fd.called) + } +}