From 517ce9abd35614484b2f782a450ebeda23b8284a Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Sat, 29 Aug 2026 12:31:24 -0400 Subject: [PATCH 1/3] Treat realtime and deferrable mail differently when quota runs out Verification codes and upload-failure notifications are not the same kind of mail, and the difference only shows up at the daily quota. A code is realtime -- someone is watching a form, and it expires in 24 hours -- so a code delivered tomorrow is a failure with extra steps. "Your data stopped arriving" is just as true an hour later. Until now both were queued and neither was retried, which produced the worst of both. The verification endpoint returned 200 before delivery was even attempted, so a researcher was told to check an inbox nothing was coming to, and was given a 60-second cooldown on asking again. And a queued upload-failure mail that died on quota was retried by nobody, while upload-failure-notify.ts had already armed the episode as "told them" in the same transaction that enqueued it -- so the researcher was never notified and the experiment document asserted that they had been. THE BREAKER (mail-availability.ts, systemStatus/mail) Resend returns x-resend-daily-quota -- the quota USED today -- on SUCCESSFUL responses, not only on 429s. So the breaker is proactive: we learn we are at 94/100 while sending still works, rather than by failing. Verification stops at a ceiling of 90; upload-failure notifications keep going to the full 100. That asymmetry is the design decision. A researcher waiting on a code can come back later; a notification that data has stopped arriving is the only signal they get, so it should have the last sends of the day. The rate-limit headers are no help here -- ratelimit-reset describes the per-second window -- and Resend documents no daily reset time, so nothing depends on knowing one. unavailableUntil is next UTC midnight as a CEILING; what actually reopens sending is the sweeper landing a success. The deferrable path probes, the realtime path only reads. The header is free-plan-only, so it vanishes on a paid plan. Absent reads as "no daily cap", which means this logic turns itself off on upgrade rather than needing removal. VERIFICATION IS NOW SYNCHRONOUS The breaker is checked before a code is minted, a record is written, or a cooldown is armed -- so a researcher clicking during an outage costs zero Resend requests. The send is then awaited, and a failure clears the verification record so they can retry immediately instead of waiting out a cooldown for a code that does not exist. onMailCreated stands down for inline mail: the claim would make the race safe, but the loser gets skipped-in-flight and could not report an outcome, which is the ambiguity the inline path exists to remove. Inline failures are terminal, never retryable. Nothing will retry them, and a retryable error is never given delivery.expireAt -- so calling them retryable would park an address outside the TTL's reach indefinitely. The message is vague on purpose, and the same for every cause: quota, an unverified domain, a revoked key. None of it is actionable by a researcher. THE SWEEPER (scheduled-mail-retry.ts, every 10 minutes) Re-drives retryable mail, and refuses three things. Inline mail, ever. Ambiguous errors past 24 hours, because mail-delivery.ts only made timeouts retryable on the strength of Resend's Idempotency-Key and Resend honours it for a day -- REFUSED_ERRORS (refused, or never reached Resend) carry no such risk and sweep at any age. And anything at all while the breaker is shut, because sweeping into an exhausted quota fails every document AND spends one of its three MAX_ATTEMPTS doing it, so a day-long outage would exhaust every queued mail's budget and turn them all terminal. It also ages out mail nobody could deliver in three days, marking it terminal so delivery.expireAt is finally written. That closes the hole runbook 4 notes: a retryable ERROR otherwise holds an address forever, outside the TTL policy, with no sweeper to catch it. Alerting is documented rather than built (runbook 6): two log-based metrics, no code, routed somewhere Google delivers -- you cannot email yourself that you are out of email. Adds a composite index for the sweep query, a firestore.rules note that systemStatus stays unmatched and therefore closed, and 33 tests: 21 pure (both predicates as tables) and 12 emulator-backed. The emulator suite uses its own status document, because systemStatus/mail is a singleton and a suite that shut the real breaker would fail every other suite that sends mail, differently each run. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D1dNU7EnmBBMTn8MWfojDL --- components/account/ContactEmail.js | 12 + docs/deploy-contact-email.md | 73 ++++ firestore.indexes.json | 14 + firestore.rules | 8 + .../src/__tests__/mail-availability.test.js | 336 ++++++++++++++++ .../src/__tests__/mail-retry-emulator.test.js | 374 ++++++++++++++++++ functions/src/index.ts | 6 + functions/src/mail-availability.ts | 245 ++++++++++++ functions/src/mail-delivery.ts | 130 +++++- functions/src/scheduled-mail-retry.ts | 240 +++++++++++ .../src/send-contact-email-verification.ts | 109 ++++- 11 files changed, 1542 insertions(+), 5 deletions(-) create mode 100644 functions/src/__tests__/mail-availability.test.js create mode 100644 functions/src/__tests__/mail-retry-emulator.test.js create mode 100644 functions/src/mail-availability.ts create mode 100644 functions/src/scheduled-mail-retry.ts diff --git a/components/account/ContactEmail.js b/components/account/ContactEmail.js index 58a8639..297c41f 100644 --- a/components/account/ContactEmail.js +++ b/components/account/ContactEmail.js @@ -17,6 +17,18 @@ import { isValidEmailFormat, buildContactEmailUpdate } from "../../lib/contact-e const SEND_ERROR_COPY = { "no-contact-email": "Add a contact email address before requesting a code.", "rate-limited": "Please wait a moment before requesting another code.", + // Sending is paused -- an exhausted daily quota, an unverified sending + // domain, a revoked key. Deliberately vague, and deliberately one message for + // all of them: the cause is operational detail a researcher cannot act on + // differently, and naming it ("we are out of email quota") would be both + // meaningless to them and needlessly revealing. What they CAN act on is the + // only thing said: not now, try later. + // + // The code form is NOT opened for this one (see requestCode) -- there is no + // code out there to enter, and offering the field would be a second, quieter + // lie on top of the first. + "mail-unavailable": + "We can't send verification codes right now. Please try again in a little while.", }; const VERIFY_ERROR_COPY = { "invalid-code": "That code is incorrect. Check it and try again.", diff --git a/docs/deploy-contact-email.md b/docs/deploy-contact-email.md index c6988af..1225ab2 100644 --- a/docs/deploy-contact-email.md +++ b/docs/deploy-contact-email.md @@ -424,6 +424,79 @@ experiments and queue entries. That query is unaffected by delivery: handled (the send resolves, the receipt has nowhere to land, one warning is logged). +## 5. When sending is unavailable: the breaker + +The two things DataPipe mails fail differently, and the difference only matters +when quota runs out: + +| | Verification code | Upload-failure notification | +|---|---|---| +| Nature | **Realtime.** Someone is watching a form. | **Deferrable.** Still true an hour later. | +| Delivered by | The request itself, synchronously | `onmailcreated`, then the sweeper | +| Retried? | **Never** | Yes, `scheduledmailretry` | +| On failure | 503, vague message, cooldown cleared | Stays queued, swept later | + +**`systemStatus/mail` is the breaker.** `mail-delivery.ts` writes it after every +send: the daily quota reading on success, a shut breaker on +`daily_quota_exceeded`. It is server-only — no `firestore.rules` match, so it is +default-denied to every client, and the account page learns nothing from it +directly. + +**The proactive part is a response header.** Resend returns +`x-resend-daily-quota` — the quota *used* today — on ordinary **successful** +responses, so DataPipe learns it is at 94/100 while sending still works rather +than by failing. Verification stops at a ceiling (currently 90 of 100) while +upload-failure notifications keep going to the full limit. That asymmetry is the +point: a researcher waiting on a code can come back later, but a notification +that their data has stopped arriving is the only signal they get. + +The header is documented as free-plan-only, so it disappears on a paid plan. +Absent reads as "no daily cap applies", which is correct — the reserve logic +turns itself off on upgrade instead of needing to be removed. + +**When does it reopen?** Resend publishes no daily-reset time, and nothing here +depends on knowing one. `unavailableUntil` is set to the next UTC midnight as a +*ceiling*; what actually reopens sending is the sweeper landing a successful +send. The deferrable path probes, the realtime path only ever reads. If the real +reset is later than midnight UTC the sweeper's next attempt re-arms the breaker; +if it is earlier, the sweeper finds out first. + +**What a researcher sees:** *"We can't send verification codes right now. Please +try again in a little while."* Deliberately vague, and deliberately the same +message for an exhausted quota, an unverified domain and a revoked key — the +cause is operational detail they cannot act on differently. The code-entry form +is not opened, because there is no code out there to enter, and the resend +cooldown is cleared so they can retry the moment it comes back. + +**To force the feature open or shut by hand**, edit +`systemStatus/mail.unavailableUntil` (a Timestamp, or null). Useful for testing +the message, and for shutting off verification during a Resend incident without +a deploy. + +## 6. Alerting: two metrics, no code + +**You cannot email yourself that you are out of email.** Same account, same +quota. Alerting has to be out-of-band, which in practice means Cloud Monitoring +delivering it rather than DataPipe. + +No code is needed — `mail-delivery.ts` already logs everything at error level. +Create log-based metrics on the functions' logs and alert on them: + +| Match | Why | +|---|---| +| `MailConfigMissingError` | **First.** Every notification the deployment sends is being dropped. | +| `daily_quota_exceeded` | Sending has stopped. Reactive — it is already happening. | +| `mail-availability` + `dailyQuotaUsed` above ~80 | **The useful one.** Leading indicator, from the success-response header, while there is still time to act. | + +Route them to a channel Google delivers — Slack, PagerDuty, or an email address +that is **not** on the `jspsych.org` sending domain, so an alert about mail +being broken does not depend on mail working. + +Worth watching alongside, though neither needs an alert: `scheduled-mail-retry` +lines reporting a non-zero `agedOut` (notifications that expired undelivered), +and any accumulation of `delivery.state == "ERROR"` with `retryable == true`, +which is the sweeper's backlog. + ## Not covered here Firestore index changes (none expected — see the design doc §3.4/§7 on why diff --git a/firestore.indexes.json b/firestore.indexes.json index 3d90964..e2f306c 100644 --- a/firestore.indexes.json +++ b/firestore.indexes.json @@ -67,6 +67,20 @@ "order": "ASCENDING" } ] + }, + { + "collectionGroup": "mail", + "queryScope": "COLLECTION", + "fields": [ + { + "fieldPath": "delivery.state", + "order": "ASCENDING" + }, + { + "fieldPath": "delivery.retryable", + "order": "ASCENDING" + } + ] } ], "fieldOverrides": [ diff --git a/firestore.rules b/firestore.rules index af71c7e..94838af 100644 --- a/firestore.rules +++ b/firestore.rules @@ -84,6 +84,14 @@ service cloud.firestore { // Same for the `mail` collection (functions/src/mail.ts): unmatched, // therefore already closed. Written down so nobody "fixes" the absence // later by granting access. + // + // Same again for `systemStatus/mail` (functions/src/mail-availability.ts), + // which records how much of the Resend daily quota has been used and until + // when sending is paused. A client that could read it would learn nothing + // about any researcher, but it would learn DataPipe's operational posture, + // and the account page does not need it to: the verification endpoint + // answers 503 with `code: "mail-unavailable"` and the UI renders from that. + // Keeping this unmatched keeps a purely operational document off the wire. match /experiments/{experimentId} { function baseFields() { return request.resource.data.keys().hasAll(['active', 'activeBase64', 'activeConditionAssignment', 'id', 'owner', 'title', 'sessions', 'nConditions', 'currentCondition', 'useValidation', 'allowJSON', 'allowCSV', 'requiredFields', 'maxSessions', 'limitSessions']) diff --git a/functions/src/__tests__/mail-availability.test.js b/functions/src/__tests__/mail-availability.test.js new file mode 100644 index 0000000..cffd6b7 --- /dev/null +++ b/functions/src/__tests__/mail-availability.test.js @@ -0,0 +1,336 @@ +/** + * @jest-environment node + */ + +// Pure coverage for the two predicates that decide whether DataPipe sends. +// +// verificationAvailability() may the REALTIME path send right now? +// sweepDecision() may this failed mail be retried right now? +// +// Both are functions of their arguments, and both are the entire safety +// argument of the feature they belong to -- so they are asserted as tables +// here rather than provoked through Firestore, a scheduler and a mail +// transport. The end-to-end behaviour lives in mail-retry-emulator.test.js. +// +// Imports the COMPILED modules (functions/lib/), so `npm --prefix functions run +// build` must run first. Same convention as mail-delivery.test.js, including +// the emulator bootstrap -- these modules reach app.js transitively for `db`, +// and app.js calls initializeApp() with no arguments. Nothing here talks to +// Firestore. + +process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080"; +process.env.GCLOUD_PROJECT = "datapipe-test"; +process.env.TOKEN_ENCRYPTION_KEY ||= "aa".repeat(32); +process.env.FIREBASE_CONFIG = JSON.stringify({ + projectId: "datapipe-test", + storageBucket: "datapipe-test.appspot.com", +}); + +let verificationAvailability; +let deliveryPaused; +let nextUtcMidnight; +let isQuotaReadingStale; +let VERIFICATION_CEILING; +let FREE_PLAN_DAILY_LIMIT; + +let sweepDecision; +let MAX_SWEEP_AGE_MS; +let IDEMPOTENCY_WINDOW_MS; + +beforeAll(async () => { + ({ + verificationAvailability, + deliveryPaused, + nextUtcMidnight, + isQuotaReadingStale, + VERIFICATION_CEILING, + FREE_PLAN_DAILY_LIMIT, + } = await import("../../lib/mail-availability.js")); + + ({ sweepDecision, MAX_SWEEP_AGE_MS, IDEMPOTENCY_WINDOW_MS } = await import( + "../../lib/scheduled-mail-retry.js" + )); +}); + +// Stored Timestamps, as the Admin SDK hands them back. +const ts = (ms) => ({ toMillis: () => ms }); + +// A fixed instant well inside a UTC day, so "next midnight" arithmetic is not +// accidentally satisfied by being near a boundary. +const NOON = Date.UTC(2026, 7, 29, 12, 0, 0); // 2026-08-29T12:00:00Z +const MIDNIGHT_AFTER = Date.UTC(2026, 7, 30, 0, 0, 0); + +// --------------------------------------------------------------------------- +// 1. The daily-reset guess, isolated +// --------------------------------------------------------------------------- + +describe("nextUtcMidnight", () => { + test("is the next UTC day boundary, not 24 hours out", () => { + expect(nextUtcMidnight(NOON)).toBe(MIDNIGHT_AFTER); + // One second before midnight -> that midnight is already past, so the next + // one is the following day. The off-by-one that would break the breaker. + expect(nextUtcMidnight(MIDNIGHT_AFTER - 1000)).toBe(MIDNIGHT_AFTER); + expect(nextUtcMidnight(MIDNIGHT_AFTER)).toBe( + Date.UTC(2026, 7, 31, 0, 0, 0) + ); + }); + + test("rolls over month and year boundaries", () => { + expect(nextUtcMidnight(Date.UTC(2026, 7, 31, 23, 0, 0))).toBe( + Date.UTC(2026, 8, 1, 0, 0, 0) + ); + expect(nextUtcMidnight(Date.UTC(2026, 11, 31, 23, 0, 0))).toBe( + Date.UTC(2027, 0, 1, 0, 0, 0) + ); + }); +}); + +describe("isQuotaReadingStale", () => { + test("a reading from earlier today is fresh", () => { + expect(isQuotaReadingStale(NOON - 60_000, NOON)).toBe(false); + }); + + test("a reading from before the last midnight is stale", () => { + // Yesterday's 100/100 must not hold today's verification path shut. + expect(isQuotaReadingStale(NOON - 24 * 60 * 60 * 1000, NOON)).toBe(true); + }); + + test("no reading at all is stale, not fresh", () => { + expect(isQuotaReadingStale(null, NOON)).toBe(true); + }); +}); + +// --------------------------------------------------------------------------- +// 2. May the realtime path send? +// --------------------------------------------------------------------------- + +describe("verificationAvailability", () => { + test("an absent status document means available", () => { + // Fails OPEN. A missing or unreadable status document must never be the + // reason a researcher cannot verify their address -- the worst case is one + // send that discovers the real state and records it. + expect(verificationAvailability(undefined, NOON).available).toBe(true); + expect(verificationAvailability({}, NOON).available).toBe(true); + }); + + test("a live unavailableUntil closes it", () => { + const result = verificationAvailability( + { unavailableUntil: ts(MIDNIGHT_AFTER) }, + NOON + ); + expect(result.available).toBe(false); + expect(result.reason).toBe("quota-exhausted"); + expect(result.until).toBe(MIDNIGHT_AFTER); + }); + + test("an expired unavailableUntil reopens it without anyone clearing it", () => { + // The breaker has to reopen on its own. If it only ever reopened on a + // successful send, and the realtime path is the only sender, nothing would + // ever send again. + expect( + verificationAvailability({ unavailableUntil: ts(NOON - 1) }, NOON).available + ).toBe(true); + }); + + test("the reserve closes it before the quota is actually gone", () => { + // The whole point of reading x-resend-daily-quota off SUCCESS responses: + // stop the realtime path with headroom left, rather than discovering the + // limit by failing. + const atCeiling = verificationAvailability( + { dailyQuotaUsed: VERIFICATION_CEILING, dailyQuotaObservedAt: ts(NOON - 1000) }, + NOON + ); + expect(atCeiling.available).toBe(false); + expect(atCeiling.reason).toBe("quota-reserve"); + + // ...and one below it is still open. + expect( + verificationAvailability( + { + dailyQuotaUsed: VERIFICATION_CEILING - 1, + dailyQuotaObservedAt: ts(NOON - 1000), + }, + NOON + ).available + ).toBe(true); + }); + + test("the reserve leaves real headroom for upload-failure mail", () => { + // The asymmetry is the design decision, so assert it rather than trusting + // the constants to stay sane: verification must stop with sends to spare, + // because an upload-failure notification is the one nobody can recover + // from missing. + expect(VERIFICATION_CEILING).toBeLessThan(FREE_PLAN_DAILY_LIMIT); + expect(FREE_PLAN_DAILY_LIMIT - VERIFICATION_CEILING).toBeGreaterThanOrEqual(5); + }); + + test("yesterday's reading does not close today", () => { + // Without the staleness check the breaker would latch permanently: a + // reading of 100/100 taken yesterday would keep refusing forever, because + // nothing rewrites it until a send succeeds and no send is attempted. + expect( + verificationAvailability( + { + dailyQuotaUsed: FREE_PLAN_DAILY_LIMIT, + dailyQuotaObservedAt: ts(NOON - 24 * 60 * 60 * 1000), + }, + NOON + ).available + ).toBe(true); + }); + + test("a missing quota reading is not treated as zero", () => { + // On a paid plan Resend stops sending x-resend-daily-quota. Absent must + // read as "no daily cap applies", which is what makes this module turn + // itself off on upgrade instead of needing to be removed. + expect( + verificationAvailability({ dailyQuotaObservedAt: ts(NOON) }, NOON).available + ).toBe(true); + expect( + verificationAvailability( + { dailyQuotaUsed: "lots", dailyQuotaObservedAt: ts(NOON) }, + NOON + ).available + ).toBe(true); + }); +}); + +describe("deliveryPaused", () => { + test("tracks unavailableUntil only, and ignores the verification reserve", () => { + // The reserve exists to keep the realtime path off the last few sends. The + // sweeper is the consumer those sends are being reserved FOR, so it must + // not be stopped by them. + expect( + deliveryPaused( + { + dailyQuotaUsed: VERIFICATION_CEILING, + dailyQuotaObservedAt: ts(NOON - 1000), + }, + NOON + ) + ).toBe(false); + + expect(deliveryPaused({ unavailableUntil: ts(MIDNIGHT_AFTER) }, NOON)).toBe(true); + expect(deliveryPaused({ unavailableUntil: ts(NOON - 1) }, NOON)).toBe(false); + expect(deliveryPaused(undefined, NOON)).toBe(false); + }); +}); + +// --------------------------------------------------------------------------- +// 3. May this failed mail be swept? +// --------------------------------------------------------------------------- + +describe("sweepDecision", () => { + const doc = (delivery, extra = {}) => ({ + to: ["researcher@example.edu"], + datapipe: { kind: "upload-failure", owner: "uid-1", ...(extra.datapipe ?? {}) }, + delivery: { state: "ERROR", retryable: true, ...delivery }, + }); + + test("sweeps a refused error at any age inside the age bound", () => { + // Quota is the case the sweeper exists for, and it is provably un-sent: + // Resend refused the request, so there is no message to duplicate. + expect( + sweepDecision( + doc({ + error: { name: "daily_quota_exceeded", message: "quota" }, + lastAttemptAt: ts(NOON - 2 * 60 * 60 * 1000), + }), + NOON + ) + ).toBe("deliver"); + }); + + test("sweeps a refused error even PAST the idempotency window", () => { + // The distinction that makes the sweeper safe. A refusal means Resend + // never accepted anything, so the 24-hour Idempotency-Key window is + // irrelevant -- there is nothing for it to deduplicate. + expect( + sweepDecision( + doc({ + error: { name: "ECONNREFUSED", message: "refused" }, + lastAttemptAt: ts(NOON - 2 * IDEMPOTENCY_WINDOW_MS), + }), + NOON + ) + ).toBe("deliver"); + }); + + test("sweeps a 5xx only INSIDE the idempotency window", () => { + // A 500 may or may not have been accepted. Inside the window the + // Idempotency-Key makes a retry a no-op; outside it, the retry is a coin + // flip on a second copy, so we decline. + const inside = doc({ + error: { name: "application_error", message: "boom" }, + lastAttemptAt: ts(NOON - 60 * 60 * 1000), + }); + expect(sweepDecision(inside, NOON)).toBe("deliver"); + + const outside = doc({ + error: { name: "application_error", message: "boom" }, + lastAttemptAt: ts(NOON - IDEMPOTENCY_WINDOW_MS - 60_000), + }); + expect(sweepDecision(outside, NOON)).toBe("skip"); + }); + + test("NEVER sweeps inline mail, whatever the error says", () => { + // A verification code is realtime. Delivering one an hour late is not a + // late success -- it may already have expired, and the researcher has long + // since given up or requested another. + expect( + sweepDecision( + doc( + { + error: { name: "daily_quota_exceeded", message: "quota" }, + lastAttemptAt: ts(NOON - 60_000), + }, + { datapipe: { kind: "contact-email-verification", deliverInline: true } } + ), + NOON + ) + ).toBe("skip"); + }); + + test("ages out anything past the age bound, rather than skipping it", () => { + // Terminal, not skip, and the difference matters: a retryable ERROR never + // gets delivery.expireAt, so skipping would leave the document holding a + // researcher's address outside the TTL policy's reach forever. + expect( + sweepDecision( + doc({ + error: { name: "daily_quota_exceeded", message: "quota" }, + lastAttemptAt: ts(NOON - MAX_SWEEP_AGE_MS - 60_000), + }), + NOON + ) + ).toBe("age-out"); + }); + + test("falls back to startTime when lastAttemptAt is absent", () => { + // Documents written before lastAttemptAt existed still have to age out. + expect( + sweepDecision( + doc({ + error: { name: "daily_quota_exceeded", message: "quota" }, + startTime: ts(NOON - MAX_SWEEP_AGE_MS - 60_000), + }), + NOON + ) + ).toBe("age-out"); + }); + + test("ignores anything that is not a live retryable error", () => { + expect(sweepDecision(doc({ state: "SUCCESS", retryable: false }), NOON)).toBe( + "skip" + ); + expect(sweepDecision(doc({ retryable: false }), NOON)).toBe("skip"); + expect(sweepDecision({ to: ["x@example.edu"] }, NOON)).toBe("skip"); + }); + + test("skips an error with no usable name rather than guessing", () => { + expect( + sweepDecision(doc({ error: {}, lastAttemptAt: ts(NOON - 1000) }), NOON) + ).toBe("skip"); + expect(sweepDecision(doc({ lastAttemptAt: ts(NOON - 1000) }), NOON)).toBe("skip"); + }); +}); diff --git a/functions/src/__tests__/mail-retry-emulator.test.js b/functions/src/__tests__/mail-retry-emulator.test.js new file mode 100644 index 0000000..4b8982d --- /dev/null +++ b/functions/src/__tests__/mail-retry-emulator.test.js @@ -0,0 +1,374 @@ +/** + * @jest-environment node + */ + +// The quota breaker and the retry sweeper, against the Firestore emulator. +// +// Harness conventions are mail-delivery-emulator.test.js's, for the same +// reasons: emulator env at module scope before any import that reaches app.js, +// a NAMED admin app, a dynamic import of the COMPILED module from +// functions/lib/, and scoped cleanup via a registrar of created refs rather +// than a collection-wide wipe. +// +// TWO SEAMS ARE INJECTED HERE, AND THE SECOND ONE IS NOT OPTIONAL. +// +// _setMailSenderForTests the transport, so nothing reaches api.resend.com. +// _setMailStatusDocForTests the breaker document. +// +// The breaker lives at `systemStatus/mail`, which is a SINGLETON shared by +// every suite. Half the assertions below require the breaker to be SHUT, and a +// shut breaker makes every other suite that sends mail fail -- including +// contact-email-verify-emulator.test.js, which drives the real deployed +// endpoint over HTTP and would start getting 503s. Under `--maxWorkers=2` that +// is a suite failing in a different place each run, which is the worst kind of +// bug to chase. Pointing this suite at its own status document is what prevents +// it, and it is why the seam exists in production code at all. + +process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080"; +process.env.GCLOUD_PROJECT = "datapipe-test"; +process.env.TOKEN_ENCRYPTION_KEY ||= "aa".repeat(32); +process.env.FIREBASE_CONFIG = JSON.stringify({ + projectId: "datapipe-test", + storageBucket: "datapipe-test.appspot.com", +}); + +process.env.RESEND_API_KEY = "re_test_not_a_real_key"; +process.env.MAIL_FROM = "DataPipe (test) "; + +import { initializeApp, getApp } from "firebase-admin/app"; +import { getFirestore, Timestamp } from "firebase-admin/firestore"; +import { randomUUID } from "crypto"; + +jest.setTimeout(30000); + +let db; +let deliverMailDocument; +let _setMailSenderForTests; +let sweepRetryableMail; +let _setMailStatusDocForTests; +let MAX_SWEEP_AGE_MS; +let MAIL_RETENTION_MS; + +// This suite's private breaker document. +const STATUS_DOC_ID = `mail-test-${randomUUID()}`; + +beforeAll(async () => { + let app; + try { + app = getApp("mail-retry-test"); + } catch { + app = initializeApp({ projectId: "datapipe-test" }, "mail-retry-test"); + } + db = getFirestore(app); + + ({ deliverMailDocument, _setMailSenderForTests, MAIL_RETENTION_MS } = await import( + "../../lib/mail-delivery.js" + )); + ({ sweepRetryableMail, MAX_SWEEP_AGE_MS } = await import( + "../../lib/scheduled-mail-retry.js" + )); + ({ _setMailStatusDocForTests } = await import("../../lib/mail-availability.js")); + + _setMailStatusDocForTests(STATUS_DOC_ID); +}); + +afterAll(async () => { + _setMailStatusDocForTests(null); + await db.collection("systemStatus").doc(STATUS_DOC_ID).delete().catch(() => {}); +}); + +const created = []; +const RECIPIENT = "researcher@example.edu"; + +async function seedMail({ delivery, inline = false, kind = "upload-failure" } = {}) { + const ref = db.collection("mail").doc(); + created.push(ref); + await ref.set({ + to: [RECIPIENT], + message: { + subject: "DataPipe couldn't upload data for Working Memory Span", + text: "The file is not lost.", + html: "

The file is not lost.

", + }, + datapipe: { + kind, + owner: `mr-user-${randomUUID()}`, + experimentID: `mr-exp-${randomUUID()}`, + queuedAt: Timestamp.now(), + ...(inline ? { deliverInline: true } : {}), + }, + ...(delivery ? { delivery } : {}), + }); + return { ref, id: ref.id }; +} + +const statusRef = () => db.collection("systemStatus").doc(STATUS_DOC_ID); +const deliveryOf = async (ref) => (await ref.get()).data()?.delivery; + +function sendingOk(id = "resend-ok", headers = {}) { + return jest.fn().mockResolvedValue({ id, ...headers }); +} + +function sendingError(name, extra = {}) { + return jest.fn().mockRejectedValue( + Object.assign(new Error(`${name} happened`), { name }, extra) + ); +} + +// A retryable failure already on the document, as finish() would have left it. +function failedDelivery(name, agoMs = 60_000, attempts = 1) { + const at = Timestamp.fromMillis(Date.now() - agoMs); + return { + state: "ERROR", + retryable: true, + attempts, + error: { name, message: `${name} happened` }, + startTime: at, + lastAttemptAt: at, + leaseExpiresAt: null, + endTime: null, + }; +} + +let errorSpy; +let warnSpy; +let logSpy; + +beforeEach(() => { + errorSpy = jest.spyOn(console, "error").mockImplementation(() => {}); + warnSpy = jest.spyOn(console, "warn").mockImplementation(() => {}); + logSpy = jest.spyOn(console, "log").mockImplementation(() => {}); +}); + +afterEach(async () => { + _setMailSenderForTests(null); + jest.restoreAllMocks(); + await statusRef().delete().catch(() => {}); + const batch = db.batch(); + while (created.length) batch.delete(created.pop()); + await batch.commit(); +}); + +// --------------------------------------------------------------------------- +// 1. The breaker gets written by delivery +// --------------------------------------------------------------------------- + +describe("recording quota state", () => { + test("a successful send records the daily quota reading from the header", async () => { + // The proactive signal. x-resend-daily-quota rides on SUCCESS responses, so + // the breaker learns we are at 94/100 while sending still works. + _setMailSenderForTests(sendingOk("resend-1", { dailyQuotaUsed: 94 })); + const { id } = await seedMail(); + + expect(await deliverMailDocument(id)).toBe("sent"); + + const status = (await statusRef().get()).data(); + expect(status.dailyQuotaUsed).toBe(94); + expect(status.dailyQuotaObservedAt.toMillis()).toBeGreaterThan(0); + expect(status.unavailableUntil).toBeNull(); + }); + + test("a quota failure shuts the breaker until the next UTC midnight", async () => { + _setMailSenderForTests(sendingError("daily_quota_exceeded", { status: 429 })); + const { id } = await seedMail(); + + expect(await deliverMailDocument(id)).toBe("retryable-error"); + + const status = (await statusRef().get()).data(); + expect(status.reason).toBe("daily_quota_exceeded"); + expect(status.unavailableUntil.toMillis()).toBeGreaterThan(Date.now()); + // Pinned at the limit: a stale lower reading from earlier in the day must + // not keep claiming the verification reserve is untouched. + expect(status.dailyQuotaUsed).toBe(100); + }); + + test("a later success reopens the breaker -- this is the half-open close", async () => { + _setMailSenderForTests(sendingError("daily_quota_exceeded", { status: 429 })); + const first = await seedMail(); + await deliverMailDocument(first.id); + expect((await statusRef().get()).data().unavailableUntil).not.toBeNull(); + + _setMailSenderForTests(sendingOk("resend-2", { dailyQuotaUsed: 3 })); + const second = await seedMail(); + expect(await deliverMailDocument(second.id)).toBe("sent"); + + const status = (await statusRef().get()).data(); + expect(status.unavailableUntil).toBeNull(); + expect(status.dailyQuotaUsed).toBe(3); + }); + + test("a send with no quota header leaves no reading, rather than recording zero", async () => { + // Resend omits the header on paid plans. Absent must read as "no daily cap + // applies", not as "0 used" and certainly not as a stale 0 that survives. + _setMailSenderForTests(sendingOk("resend-3")); + const { id } = await seedMail(); + await deliverMailDocument(id); + + const status = (await statusRef().get()).data(); + expect(status.dailyQuotaUsed).toBeUndefined(); + expect(status.unavailableUntil).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 2. Inline mail is terminal, because nothing will ever retry it +// --------------------------------------------------------------------------- + +describe("inline mail", () => { + test("a retryable-class error is TERMINAL when the mail is delivered inline", async () => { + // A verification code is realtime: its sender is holding an HTTP request + // open and the sweeper skips it forever. Calling it "retryable" would be a + // lie that also costs something -- a retryable error is never given + // delivery.expireAt, so the document would sit outside the TTL's reach + // holding an address indefinitely. + _setMailSenderForTests(sendingError("daily_quota_exceeded", { status: 429 })); + const { ref, id } = await seedMail({ + inline: true, + kind: "contact-email-verification", + }); + + expect(await deliverMailDocument(id)).toBe("terminal-error"); + + const delivery = await deliveryOf(ref); + expect(delivery.state).toBe("ERROR"); + expect(delivery.retryable).toBe(false); + // Terminal means the TTL can reap it, which is the whole point. + expect(delivery.endTime.toMillis()).toBeGreaterThan(0); + expect(delivery.expireAt.toMillis()).toBeGreaterThan(Date.now()); + }); + + test("the same error on QUEUED mail stays retryable", async () => { + // The contrast that proves the branch is about inline-ness, not about the + // error. + _setMailSenderForTests(sendingError("daily_quota_exceeded", { status: 429 })); + const { ref, id } = await seedMail(); + + expect(await deliverMailDocument(id)).toBe("retryable-error"); + const delivery = await deliveryOf(ref); + expect(delivery.retryable).toBe(true); + expect(delivery.endTime).toBeNull(); + }); +}); + +// --------------------------------------------------------------------------- +// 3. The sweep +// --------------------------------------------------------------------------- + +describe("sweepRetryableMail", () => { + test("sweeps NOTHING while the breaker is shut, and burns no attempts", async () => { + // The most important assertion in this file. Sweeping into an exhausted + // quota fails every document and spends one of its three MAX_ATTEMPTS + // doing it -- so a day-long outage would exhaust the retry budget of every + // queued mail and turn all of them terminal, which is the exact opposite of + // what the sweeper is for. + const send = sendingOk(); + _setMailSenderForTests(send); + const { ref } = await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + await statusRef().set({ + unavailableUntil: Timestamp.fromMillis(Date.now() + 60 * 60 * 1000), + reason: "daily_quota_exceeded", + }); + + const report = await sweepRetryableMail(); + + expect(report.paused).toBe(true); + expect(report.scanned).toBe(0); + expect(send).not.toHaveBeenCalled(); + // Untouched: attempts did not creep up by being looked at. + expect((await deliveryOf(ref)).attempts).toBe(1); + }); + + test("delivers a quota-failed mail once the breaker has expired", async () => { + const send = sendingOk("resend-swept", { dailyQuotaUsed: 5 }); + _setMailSenderForTests(send); + const { ref } = await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + // Yesterday's breaker, already expired. + await statusRef().set({ + unavailableUntil: Timestamp.fromMillis(Date.now() - 1000), + }); + + const report = await sweepRetryableMail(); + + expect(report.paused).toBe(false); + expect(report.delivered).toBeGreaterThanOrEqual(1); + expect(send).toHaveBeenCalled(); + + const delivery = await deliveryOf(ref); + expect(delivery.state).toBe("SUCCESS"); + expect(delivery.attempts).toBe(2); + expect(delivery.info.transport).toBe("resend"); + // ...and the successful probe reopened the realtime path. + expect((await statusRef().get()).data().unavailableUntil).toBeNull(); + }); + + test("never sweeps inline mail, even when it somehow looks retryable", async () => { + // Belt and braces: inline failures are already marked terminal, so this + // document should not exist. If one ever does -- a hand edit, an older + // deploy's write -- the sweeper still must not resurrect a verification + // code that expired hours ago. + const send = sendingOk(); + _setMailSenderForTests(send); + const { ref } = await seedMail({ + inline: true, + kind: "contact-email-verification", + delivery: failedDelivery("daily_quota_exceeded"), + }); + + const report = await sweepRetryableMail(); + + expect(send).not.toHaveBeenCalled(); + expect(report.skipped).toBeGreaterThanOrEqual(1); + expect((await deliveryOf(ref)).state).toBe("ERROR"); + }); + + test("ages out a mail nobody delivered in time, and lets the TTL have it", async () => { + // Terminal rather than skipped, deliberately: a retryable ERROR never gets + // delivery.expireAt, so skipping would leave this document holding a + // researcher's address outside the TTL policy's reach forever. + const send = sendingOk(); + _setMailSenderForTests(send); + const { ref } = await seedMail({ + delivery: failedDelivery("daily_quota_exceeded", MAX_SWEEP_AGE_MS + 60_000), + }); + + const report = await sweepRetryableMail(); + + expect(report.agedOut).toBe(1); + expect(send).not.toHaveBeenCalled(); + + const delivery = await deliveryOf(ref); + expect(delivery.retryable).toBe(false); + expect(delivery.endTime.toMillis()).toBeGreaterThan(0); + expect(delivery.expireAt.toMillis()).toBeCloseTo( + delivery.endTime.toMillis() + MAIL_RETENTION_MS, + -4 + ); + }); + + test("stops the pass when a send trips the breaker mid-sweep", async () => { + // Without this the remaining documents each fail and each spend an attempt, + // converting one quota outage into an exhausted retry budget. + const send = sendingError("daily_quota_exceeded", { status: 429 }); + _setMailSenderForTests(send); + await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + + const report = await sweepRetryableMail(); + + expect(report.paused).toBe(true); + // One attempt discovers the quota; the rest of the pass stands down. + expect(send).toHaveBeenCalledTimes(1); + }); + + test("an empty mail collection is a quiet no-op", async () => { + const send = sendingOk(); + _setMailSenderForTests(send); + + const report = await sweepRetryableMail(); + + expect(send).not.toHaveBeenCalled(); + expect(report.delivered).toBe(0); + expect(report.agedOut).toBe(0); + }); +}); diff --git a/functions/src/index.ts b/functions/src/index.ts index 416eeea..75adf20 100644 --- a/functions/src/index.ts +++ b/functions/src/index.ts @@ -18,6 +18,11 @@ import { onUploadFailure } from "./upload-failure-notify.js"; // denied production access); mail.ts's document contract is unchanged through // both swaps, so nothing on the write side moved. import { onMailCreated } from "./mail-delivery.js"; +// Re-drives mail whose failure has since stopped being true (a quota that has +// rolled over, a blip that has passed). onMailCreated cannot: an +// onDocumentCreated trigger does not re-fire on updates, so before this existed +// a `retryable` ERROR was retried by nobody. +import { scheduledMailRetry } from "./scheduled-mail-retry.js"; // The verification round trip (plan §2.2, §5 package P3): a resend-capable // send + a hash-checked verify, both bearer-token onRequest endpoints in the // same shape as deleteAccount / apiQueueStatus below. @@ -53,6 +58,7 @@ export { onUploadQueueChanged as onuploadqueuechanged, onUploadFailure as onuploadfailure, onMailCreated as onmailcreated, + scheduledMailRetry as scheduledmailretry, sendContactEmailVerification as sendcontactemailverification, verifyContactEmail as verifycontactemail, apiQueueStatus as apiqueuestatus, diff --git a/functions/src/mail-availability.ts b/functions/src/mail-availability.ts new file mode 100644 index 0000000..f917b36 --- /dev/null +++ b/functions/src/mail-availability.ts @@ -0,0 +1,245 @@ +// Is DataPipe able to send mail right now, and if not, until when? +// +// --------------------------------------------------------------------------- +// WHY THIS EXISTS +// --------------------------------------------------------------------------- +// +// The two things DataPipe mails are not the same kind of thing, and the +// difference only shows up when quota runs out: +// +// contact-email verification REALTIME. A researcher clicked a button and is +// watching for a 6-digit code that expires in 24 +// hours. A code delivered tomorrow is not a late +// success, it is a failure with extra steps. +// Never retried. +// upload-failure notification DEFERRABLE. "Your data stopped arriving" is +// just as true an hour later. Retried by +// scheduled-mail-retry.ts. +// +// So the realtime path needs to know, BEFORE it does anything, whether a send +// can succeed -- otherwise it mints a code, arms its own resend cooldown, tells +// the researcher to check their inbox, and only then discovers that nothing can +// be sent. That is the state this module exists to prevent, and it is why the +// check has to happen ahead of the send rather than being inferred from it. +// +// --------------------------------------------------------------------------- +// WHERE THE NUMBER COMES FROM +// --------------------------------------------------------------------------- +// +// Resend returns `x-resend-daily-quota` -- the quota USED so far today -- on +// ordinary successful responses, not only on 429s. That is the whole reason +// this can be proactive: we learn we are at 94/100 while sending is still +// working, instead of finding out by failing. +// +// The rate-limit headers (`ratelimit-reset`, `retry-after`) are NOT useful +// here. They describe the per-second request limit (10/s per team), so +// `ratelimit-reset` counts down seconds to the next second. Resend documents no +// header or endpoint for when the DAILY quota rolls over, so nothing here may +// depend on knowing it -- see "the reset time is a guess" below. +// +// `x-resend-daily-quota` is documented as "only sent to free plan users". Its +// absence therefore means the daily cap does not apply, which is exactly right: +// on a paid plan this module quietly stops tripping instead of needing to be +// removed. + +import { Timestamp } from "firebase-admin/firestore"; +import { db } from "./app.js"; + +export const STATUS_COLLECTION = "systemStatus"; +export const MAIL_STATUS_DOC = "mail"; + +// Resend's free plan: 3,000/month with a 100/day ceiling. Only used to derive +// the reserve below -- nothing here fails closed if the real limit differs, +// because an actual `daily_quota_exceeded` sets the breaker regardless. +export const FREE_PLAN_DAILY_LIMIT = 100; + +// Sends held back from the REALTIME path. +// +// Verification stops at the ceiling; upload-failure notifications keep going to +// the full limit. The asymmetry is deliberate and it is the important design +// decision in this file: a researcher waiting on a verification code can come +// back in an hour, but an upload-failure notification is the only signal that a +// researcher's data has stopped arriving. If anything gets the last ten sends +// of the day, it should be the one nobody can recover from missing. +export const VERIFICATION_RESERVE = 10; +export const VERIFICATION_CEILING = FREE_PLAN_DAILY_LIMIT - VERIFICATION_RESERVE; + +export interface MailStatus { + dailyQuotaUsed?: unknown; + dailyQuotaObservedAt?: unknown; + unavailableUntil?: unknown; + reason?: unknown; +} + +export type UnavailableReason = "quota-exhausted" | "quota-reserve"; + +export type Availability = + | { available: true } + | { available: false; reason: UnavailableReason; until: number | null }; + +function millisOrNull(value: unknown): number | null { + if (!value || typeof (value as { toMillis?: unknown }).toMillis !== "function") { + return null; + } + return (value as { toMillis: () => number }).toMillis(); +} + +/** + * Midnight UTC after the given instant. + * + * THE RESET TIME IS A GUESS, and this is where the guess lives. Resend does not + * publish when a daily quota rolls over; UTC midnight is the conventional + * answer and is almost certainly right, but nothing here is allowed to DEPEND + * on it being right. It is used only as a ceiling on how long the breaker stays + * shut. What actually reopens sending early is a successful send discovered by + * scheduled-mail-retry.ts -- the deferrable path probes, the realtime path only + * ever reads. If the real reset is an hour later than this, the sweeper's next + * attempt fails and re-arms the breaker; if it is earlier, the sweeper finds + * out and clears it. Either way the guess costs nothing. + */ +export function nextUtcMidnight(nowMs: number): number { + const d = new Date(nowMs); + return Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate() + 1); +} + +/** + * Has a quota reading aged out? + * + * A reading taken before the most recent UTC midnight describes yesterday's + * usage and must not hold today's verification path shut. This is the same + * guess as above, in the opposite direction, and it fails SAFE: treating a + * still-valid reading as stale merely lets a send through, and that send + * returns a fresh number. + */ +export function isQuotaReadingStale(observedAtMs: number | null, nowMs: number): boolean { + if (observedAtMs === null) return true; + return nextUtcMidnight(observedAtMs) <= nowMs; +} + +/** + * May the REALTIME path send right now? + * + * Pure, and exported for it: this is the predicate a researcher's account page + * ultimately renders, and it needs no Firestore to assert. + */ +export function verificationAvailability( + status: MailStatus | undefined, + nowMs: number +): Availability { + const until = millisOrNull(status?.unavailableUntil); + if (until !== null && until > nowMs) { + return { available: false, reason: "quota-exhausted", until }; + } + + const used = status?.dailyQuotaUsed; + const observedAt = millisOrNull(status?.dailyQuotaObservedAt); + if ( + typeof used === "number" && + Number.isFinite(used) && + !isQuotaReadingStale(observedAt, nowMs) && + used >= VERIFICATION_CEILING + ) { + return { + available: false, + reason: "quota-reserve", + until: observedAt === null ? null : nextUtcMidnight(observedAt), + }; + } + + return { available: true }; +} + +/** + * Is delivery paused outright? Used by scheduled-mail-retry.ts to decide + * whether to sweep at all. + * + * Deliberately does NOT consider the verification reserve: the reserve exists + * to keep the realtime path off the last few sends, and the sweeper is the + * consumer those sends are being reserved FOR. + */ +export function deliveryPaused(status: MailStatus | undefined, nowMs: number): boolean { + const until = millisOrNull(status?.unavailableUntil); + return until !== null && until > nowMs; +} + +let statusDocId = MAIL_STATUS_DOC; + +/** + * Test seam: point the breaker at a different document. + * + * `systemStatus/mail` is a SINGLETON, and the emulator is shared by suites + * running in parallel. A test that trips the breaker on the real document would + * make every concurrently-running suite that sends mail fail -- intermittently, + * in a different place each run, which is the worst kind of failure to chase. + * Pointing this suite at its own document is what keeps that from happening. + * Pass null to restore. + */ +export function _setMailStatusDocForTests(docId: string | null): void { + statusDocId = docId ?? MAIL_STATUS_DOC; +} + +function statusRef(): FirebaseFirestore.DocumentReference { + return db.collection(STATUS_COLLECTION).doc(statusDocId); +} + +/** + * Read the breaker. Never throws: a status read that fails must not be the + * reason a researcher cannot verify their address, so an unreadable document is + * treated as "no reason to believe anything is wrong". + */ +export async function readMailStatus(): Promise { + try { + const snap = await statusRef().get(); + return snap.exists ? (snap.data() as MailStatus) : undefined; + } catch (error) { + console.error( + "mail-availability: could not read mail status, assuming available:", + error instanceof Error ? error.message : "Unknown error" + ); + return undefined; + } +} + +/** + * Record the outcome of a send. + * + * Never throws, and never fails a delivery: this is bookkeeping alongside the + * real work, and a mail that was actually sent must not be reported as failed + * because a status write lost a race. + */ +export async function recordSendOutcome(outcome: { + dailyQuotaUsed?: number; + quotaExhausted?: boolean; + errorName?: string; +}): Promise { + const now = Date.now(); + const updates: Record = { updatedAt: Timestamp.fromMillis(now) }; + + if (outcome.quotaExhausted) { + updates.unavailableUntil = Timestamp.fromMillis(nextUtcMidnight(now)); + updates.reason = outcome.errorName ?? "quota-exhausted"; + // Pin the counter at the limit. Without this a stale, lower reading from + // earlier in the day would keep saying the reserve is untouched. + updates.dailyQuotaUsed = FREE_PLAN_DAILY_LIMIT; + updates.dailyQuotaObservedAt = Timestamp.fromMillis(now); + } else { + // A send got through, so whatever the breaker believed is now out of date. + // This is the half-open close: the sweeper probes, and its success is what + // reopens the realtime path. + updates.unavailableUntil = null; + updates.reason = null; + if (typeof outcome.dailyQuotaUsed === "number") { + updates.dailyQuotaUsed = outcome.dailyQuotaUsed; + updates.dailyQuotaObservedAt = Timestamp.fromMillis(now); + } + } + + try { + await statusRef().set(updates, { merge: true }); + } catch (error) { + console.error( + "mail-availability: could not record send outcome:", + error instanceof Error ? error.message : "Unknown error" + ); + } +} diff --git a/functions/src/mail-delivery.ts b/functions/src/mail-delivery.ts index 07d5ffb..5d8b2c0 100644 --- a/functions/src/mail-delivery.ts +++ b/functions/src/mail-delivery.ts @@ -74,6 +74,7 @@ import { Timestamp } from "firebase-admin/firestore"; import { randomUUID } from "crypto"; import { db } from "./app.js"; import { MAIL_COLLECTION } from "./mail.js"; +import { recordSendOutcome } from "./mail-availability.js"; // --------------------------------------------------------------------------- // Timings. The relationship between these three numbers is load-bearing. @@ -399,6 +400,33 @@ const AMBIGUOUS_ERRORS = new Set([ "UND_ERR_SOCKET", ]); +// Quota exhaustion, as opposed to the per-second rate limit. These are what +// trip the breaker in mail-availability.ts, because they are the ones that stay +// true for the rest of the day. +export const QUOTA_ERRORS = new Set(["daily_quota_exceeded", "monthly_quota_exceeded"]); + +// Errors where Resend provably did NOT send: it refused the request, or we +// never reached it. Safe to retry at ANY age, because there is no message to +// duplicate. +// +// This set is the sweeper's licence. Everything else that is merely `retryable` +// is only safe to retry inside the 24-hour window in which Resend still honours +// the Idempotency-Key -- past that, a 5xx that might have been accepted becomes +// a coin flip on a second copy. See scheduled-mail-retry.ts. +export const REFUSED_ERRORS = new Set([ + "daily_quota_exceeded", + "monthly_quota_exceeded", + "rate_limit_exceeded", + "concurrent_idempotent_requests", + "resource_locked", + "ENOTFOUND", + "ECONNREFUSED", + "EAI_AGAIN", + "EHOSTUNREACH", + "ENETUNREACH", + "UND_ERR_CONNECT_TIMEOUT", +]); + /** * A non-2xx answer from Resend, or an unusable one. Carries the HTTP status so * classification can fall back on it when the body named nothing familiar. @@ -563,6 +591,13 @@ export interface SendResult { // Resend's `id` for the accepted message. Recorded as // delivery.info.messageId, which is the extension's field name. id?: string; + // `x-resend-daily-quota` -- the quota USED today, read off the SUCCESS + // response. This is what makes the breaker proactive instead of reactive: + // we learn we are at 94/100 while sending still works. Absent on paid plans + // (Resend only sends it to free-plan accounts), which correctly reads as + // "no daily cap applies". + dailyQuotaUsed?: number; + monthlyQuotaUsed?: number; } export interface SendOptions { @@ -580,6 +615,14 @@ export type MailSender = ( const RESEND_ENDPOINT = "https://api.resend.com/emails"; +/** A response header as a number, or undefined if absent or unparseable. */ +function headerInt(response: Response, name: string): number | undefined { + const raw = response.headers.get(name); + if (raw === null) return undefined; + const n = Number.parseInt(raw, 10); + return Number.isFinite(n) ? n : undefined; +} + let injectedSender: MailSender | null = null; /** @@ -648,14 +691,29 @@ function resendSender(config: MailConfig): MailSender { throw new MailTransportError(name, message, response.status); } + // Quota headers ride on the SUCCESS response, which is the whole point -- + // see mail-availability.ts. Read before the body so a malformed body cannot + // cost us the reading. + const quota = { + ...(headerInt(response, "x-resend-daily-quota") !== undefined + ? { dailyQuotaUsed: headerInt(response, "x-resend-daily-quota") } + : {}), + ...(headerInt(response, "x-resend-monthly-quota") !== undefined + ? { monthlyQuotaUsed: headerInt(response, "x-resend-monthly-quota") } + : {}), + }; + // 2xx means Resend accepted it. An unreadable body after that point costs // us the message id, which is a worse audit trail -- not a failed send, so // it must not throw. delivery.info.messageId simply lands null. try { const body = (await response.json()) as { id?: unknown }; - return { id: typeof body?.id === "string" ? body.id : undefined }; + return { + ...quota, + id: typeof body?.id === "string" ? body.id : undefined, + }; } catch { - return {}; + return quota; } }; } @@ -685,6 +743,20 @@ const SKIP_OUTCOMES: Record, DeliveryOutcome> = "skip-attempts-exhausted": "skipped-attempts-exhausted", }; +/** + * Is this mail delivered inline by its caller, rather than by the trigger? + * + * Set by send-contact-email-verification.ts. Two consequences, both here and + * both deliberate: onMailCreated leaves the document alone (so the trigger and + * the inline caller cannot race for the claim, which would leave the caller + * unable to report an outcome), and a failure is terminal rather than + * retryable. + */ +export function isInline(data: FirebaseFirestore.DocumentData | undefined): boolean { + const meta = data?.datapipe as { deliverInline?: unknown } | undefined; + return meta?.deliverInline === true; +} + interface Claim { data: FirebaseFirestore.DocumentData; attempts: number; @@ -832,11 +904,26 @@ export async function deliverMailDocument(docId: string): Promise= MAX_ATTEMPTS; - const retryable = classified.retryable && !exhausted; + // An inline send is a realtime one: its caller is holding an HTTP request + // open and will report the outcome to a researcher who is watching. Nothing + // will ever retry it -- scheduled-mail-retry.ts skips these on purpose -- + // so calling it "retryable" would be a lie that also costs something real: + // a retryable error is never given delivery.expireAt, so the document would + // sit outside the TTL's reach holding an address forever. + const retryable = classified.retryable && !exhausted && !isInline(claim.data); // No recipient in this line: the docId is the handle, and the neighbours // (upload-failure-notify.ts, send-contact-email-verification.ts) log ids // and uids, never addresses. @@ -866,6 +953,17 @@ export async function deliverMailDocument(docId: string): Promise ${outcome}`); diff --git a/functions/src/scheduled-mail-retry.ts b/functions/src/scheduled-mail-retry.ts new file mode 100644 index 0000000..d9d25fd --- /dev/null +++ b/functions/src/scheduled-mail-retry.ts @@ -0,0 +1,240 @@ +// Re-drive mail that failed for a reason that has since stopped being true. +// +// --------------------------------------------------------------------------- +// THE GAP THIS CLOSES +// --------------------------------------------------------------------------- +// +// mail-delivery.ts's onDocumentCreated trigger does not re-fire when a document +// is UPDATED, so a `retryable` ERROR was, until this file existed, retried by +// nobody. It sat marked "still deliverable" forever and nothing ever delivered +// it. +// +// That is worse than it sounds, because of what happens upstream. +// upload-failure-notify.ts writes `uploadFailure.notifiedAt` in the SAME +// transaction that enqueues the mail, and `lastNotifiedAt` is a 24-hour floor +// ACROSS episodes. So a mail that dies on quota leaves an episode armed as "we +// told them": the researcher is never told their data stopped arriving, nothing +// re-notifies, and the experiment document positively asserts that they were +// informed. The only dissent is a mail document nobody reads. +// +// --------------------------------------------------------------------------- +// WHAT IT DELIBERATELY DOES NOT SWEEP +// --------------------------------------------------------------------------- +// +// 1. INLINE MAIL (contact-email verification). Never. A verification code is +// realtime -- it expires in 24 hours and its recipient is watching a form +// right now -- so a code delivered an hour late is not a late success, it is +// a confusing failure. Those are marked terminal at failure time +// (mail-delivery.ts's isInline), so they should never match the query here; +// the explicit skip below is belt and braces. +// +// 2. AMBIGUOUS ERRORS, past the idempotency window. mail-delivery.ts makes +// timeouts retryable ONLY because Resend honours an Idempotency-Key, and it +// honours it for 24 hours. A sweeper is by nature a late retry -- quota +// resets daily -- so retrying a maybe-delivered send past that window is a +// coin flip on a second copy of a notification whose whole value is arriving +// once. REFUSED_ERRORS (the request was refused, or never reached Resend at +// all) carry no such risk and are swept at any age; everything else is swept +// only inside the window. +// +// 3. ANYTHING, while the breaker is shut. If mail-availability.ts says the +// quota is exhausted, sweeping would fail every document AND burn one of its +// three MAX_ATTEMPTS doing it -- so a day-long outage would exhaust the +// retry budget of every queued mail and turn all of them terminal, which is +// the precise opposite of this file's purpose. Checking the breaker first is +// what makes the sweep free to run often. + +import { onSchedule } from "firebase-functions/v2/scheduler"; +import { Timestamp } from "firebase-admin/firestore"; +import { db } from "./app.js"; +import { MAIL_COLLECTION } from "./mail.js"; +import { + deliverMailDocument, + isInline, + REFUSED_ERRORS, + MAIL_RETENTION_MS, +} from "./mail-delivery.js"; +import { deliveryPaused, readMailStatus } from "./mail-availability.js"; + +// How long Resend honours an Idempotency-Key. The bound on retrying anything +// that is not provably un-sent. Raising this without checking Resend's docs +// would silently reintroduce double-sends. +export const IDEMPOTENCY_WINDOW_MS = 24 * 60 * 60 * 1000; + +// Past this, a notification has stopped being worth sending. Three days is +// chosen against what the mail SAYS: "your uploads are failing" is still true +// and still actionable a day or two later, but a researcher who has not noticed +// in three days is better served by the dashboard than by an email about a +// failure episode that has probably long since drained. +// +// Reaching it is TERMINAL, not a skip, and that matters for more than tidiness: +// a retryable ERROR is never given delivery.expireAt, so it sits outside the +// TTL policy's reach holding a researcher's address indefinitely. Ageing these +// out is what closes that hole (docs/deploy-contact-email.md §4). +export const MAX_SWEEP_AGE_MS = 3 * 24 * 60 * 60 * 1000; + +// One pass' budget. Small on purpose: the steady state is zero documents, the +// bad case is a quota outage that queued a few dozen, and a sweep that tried to +// drain hundreds would run into Resend's per-second rate limit and convert a +// recoverable backlog into a burned retry budget. +export const SWEEP_LIMIT = 25; + +export interface SweepReport { + scanned: number; + delivered: number; + failed: number; + skipped: number; + agedOut: number; + paused: boolean; +} + +function millisOrZero(value: unknown): number { + if (!value || typeof (value as { toMillis?: unknown }).toMillis !== "function") { + return 0; + } + return (value as { toMillis: () => number }).toMillis(); +} + +/** + * May this document be retried right now? + * + * Pure, and exported for it -- this predicate is the entire safety argument of + * the file, and it should be assertable as a table rather than provoked through + * Firestore and a mail transport. + */ +export function sweepDecision( + data: FirebaseFirestore.DocumentData, + nowMs: number +): "deliver" | "age-out" | "skip" { + if (isInline(data)) return "skip"; + + const delivery = (data.delivery ?? {}) as Record; + if (delivery.state !== "ERROR" || delivery.retryable !== true) return "skip"; + + // Age is measured from the last ATTEMPT, not from when the document was + // queued: a mail that has been retried into this morning is young, however + // long ago it was first written. startTime is the fallback for documents + // written before lastAttemptAt existed. + const lastAttempt = + millisOrZero(delivery.lastAttemptAt) || millisOrZero(delivery.startTime); + if (lastAttempt > 0 && nowMs - lastAttempt > MAX_SWEEP_AGE_MS) return "age-out"; + + const name = (delivery.error as { name?: unknown } | undefined)?.name; + if (typeof name !== "string") return "skip"; + + // Provably un-sent: retry at any age. + if (REFUSED_ERRORS.has(name)) return "deliver"; + + // Everything else (a 5xx that may or may not have been accepted) only inside + // the window where the Idempotency-Key still deduplicates it. + if (lastAttempt > 0 && nowMs - lastAttempt <= IDEMPOTENCY_WINDOW_MS) return "deliver"; + + return "skip"; +} + +/** + * One sweep. + * + * Exported as the test seam this codebase already uses for scheduled work + * (scheduled-upload-retry.ts's retryPendingUploads, scheduled-pending- + * recovery.ts's recoverPendingUploads): tests drive it in-process rather than + * trying to provoke a real scheduler tick out of the emulator. + */ +export async function sweepRetryableMail(nowMs = Date.now()): Promise { + const report: SweepReport = { + scanned: 0, + delivered: 0, + failed: 0, + skipped: 0, + agedOut: 0, + paused: false, + }; + + // The breaker, FIRST. See the header: sweeping into an exhausted quota fails + // every document and spends a MAX_ATTEMPTS on each failure. + const status = await readMailStatus(); + if (deliveryPaused(status, nowMs)) { + report.paused = true; + console.log( + "scheduled-mail-retry: delivery is paused (quota), sweeping nothing this pass" + ); + return report; + } + + const snap = await db + .collection(MAIL_COLLECTION) + .where("delivery.state", "==", "ERROR") + .where("delivery.retryable", "==", true) + .limit(SWEEP_LIMIT) + .get(); + + report.scanned = snap.size; + + for (const doc of snap.docs) { + const decision = sweepDecision(doc.data(), nowMs); + + if (decision === "skip") { + report.skipped += 1; + continue; + } + + if (decision === "age-out") { + // Terminal, so delivery.expireAt is written and the TTL can finally reap + // the address this document has been holding. + await doc.ref.update({ + "delivery.retryable": false, + "delivery.endTime": Timestamp.fromMillis(nowMs), + "delivery.expireAt": Timestamp.fromMillis(nowMs + MAIL_RETENTION_MS), + }); + report.agedOut += 1; + console.log( + `scheduled-mail-retry: ${doc.id} aged out undelivered, marked terminal` + ); + continue; + } + + const outcome = await deliverMailDocument(doc.id); + if (outcome === "sent") { + report.delivered += 1; + } else { + report.failed += 1; + } + console.log(`scheduled-mail-retry: ${doc.id} -> ${outcome}`); + + // A quota failure mid-sweep means the rest of this pass would fail too, and + // each failure costs a retry attempt. deliverMailDocument has already + // tripped the breaker; honour it immediately rather than after 24 more + // wasted attempts. + if (outcome === "retryable-error" || outcome === "terminal-error") { + const fresh = await readMailStatus(); + if (deliveryPaused(fresh, nowMs)) { + report.paused = true; + console.log( + "scheduled-mail-retry: quota tripped mid-sweep, stopping this pass" + ); + break; + } + } + } + + // A delivery during this pass already cleared the breaker via + // recordSendOutcome. Nothing to do here -- stated so nobody adds a redundant + // clear that would reopen the realtime path on a pass that sent nothing. + return report; +} + +// Every 10 minutes. Cheap by construction: the steady state is one indexed +// query returning nothing, and a shut breaker makes it one document read. +// +// No `retry` config. A failed sweep is a logged line and the next pass tries +// again ten minutes later, which is what a sweeper is for -- replaying a failed +// sweep would stack passes on top of each other for no benefit. +export const scheduledMailRetry = onSchedule( + { schedule: "*/10 * * * *", memory: "256MiB" }, + async () => { + const report = await sweepRetryableMail(); + if (report.scanned > 0 || report.paused) { + console.log(`scheduled-mail-retry: ${JSON.stringify(report)}`); + } + } +); diff --git a/functions/src/send-contact-email-verification.ts b/functions/src/send-contact-email-verification.ts index be44cd8..3e2161b 100644 --- a/functions/src/send-contact-email-verification.ts +++ b/functions/src/send-contact-email-verification.ts @@ -2,6 +2,8 @@ import { onRequest } from "firebase-functions/v2/https"; import { randomInt, createHash } from "crypto"; import { db, auth } from "./app.js"; import { contactEmailRecipient, sendMail } from "./mail.js"; +import { deliverMailDocument } from "./mail-delivery.js"; +import { readMailStatus, verificationAvailability } from "./mail-availability.js"; import { ACCOUNT_URL } from "./email/upload-failure-copy.js"; // Send (or resend) a 6-digit code that confirms users/{uid}.contactEmail. @@ -143,6 +145,36 @@ export const sendContactEmailVerification = onRequest( return; } + // --------------------------------------------------------------------- + // CAN WE SEND AT ALL? ASKED BEFORE ANYTHING IS SPENT. + // --------------------------------------------------------------------- + // + // Deliberately ahead of minting a code, writing the verification record, + // and arming the resend cooldown. Ask afterwards and a researcher gets + // told to check their inbox, gets a 60-second cooldown on requesting + // another, and never receives anything -- the failure invisible on both + // ends. Asking here also means a researcher clicking the button + // repeatedly during a quota outage costs zero Resend requests, which is + // the difference between a paused feature and a feature that makes the + // outage worse. + // + // The message is vague on purpose. "Quota" is operational detail that + // means nothing to a researcher, and one honest sentence covers every + // terminal cause -- exhausted quota, an unverified domain, a revoked key + // -- none of which they can act on differently anyway. + const availability = verificationAvailability(await readMailStatus(), Date.now()); + if (!availability.available) { + console.warn( + `send-contact-email-verification: refused for ${uid}, mail unavailable (${availability.reason})` + ); + res.status(503).json({ + error: + "We can't send verification codes right now. Please try again in a little while.", + code: "mail-unavailable", + }); + return; + } + const existing = await verificationRef(uid).get(); const sentAt = existing.exists ? (existing.data()?.sentAt as number | undefined) @@ -175,14 +207,87 @@ export const sendContactEmailVerification = onRequest( // there is nothing else to keep consistent with the send -- this is a // direct response to a request the researcher just made, which is // exactly the case mail.ts's sendMail doc comment names. - await sendMail({ + const mailRef = await sendMail({ to: recipient, subject, text, html, - meta: { kind: "contact-email-verification", owner: uid }, + meta: { + kind: "contact-email-verification", + owner: uid, + // Delivered below, synchronously, by THIS request -- not by the + // onmailcreated trigger, which stands down for inline mail. The + // document is still written first, so the audit trail, the + // purge-user-data handle and the TTL all work exactly as they do for + // queued mail; only who performs the send changes. + deliverInline: true, + }, }); + // --------------------------------------------------------------------- + // THE EMULATOR NEVER SENDS, SO IT NEVER REPORTS A SEND FAILURE EITHER. + // --------------------------------------------------------------------- + // + // onMailCreated returns before doing anything at all when + // FUNCTIONS_EMULATOR is set, and the inline path has to make the same + // promise -- otherwise it becomes the one route by which a local test run + // could mail a real person, which is precisely the hole that gate exists + // to close. + // + // It also has to answer 200. The mail document is written either way, and + // contact-email-verify-emulator.test.js recovers the code from it exactly + // as it did when the trigger owned delivery; answering 503 here would + // fail every emulator-backed verification test on a send that was never + // going to be attempted. + // + // Coverage does not suffer: the inline delivery path below is exercised + // in-process, against the emulator, with an injected transport, by + // mail-retry-emulator.test.js. + if (process.env.FUNCTIONS_EMULATOR === "true") { + console.log( + `send-contact-email-verification: emulator instance, leaving mail ${mailRef.id} unsent` + ); + res.status(200).json({ success: true }); + return; + } + + // --------------------------------------------------------------------- + // AWAITED, BECAUSE SOMEBODY IS WATCHING A FORM. + // --------------------------------------------------------------------- + // + // The queue's asynchrony is right for a failure notification and wrong + // here: a verification code that arrives tomorrow is not a late success, + // it is a code that expired (EXPIRY_MS) before it landed. So this request + // does not return until the send has actually happened or actually + // failed. deliverMailDocument never throws -- every expected failure is a + // returned outcome and a field on the document. + const outcome = await deliverMailDocument(mailRef.id); + if (outcome !== "sent") { + // The code was never delivered, so the record that proves it exists + // must go -- and with it the resend cooldown it arms. A researcher + // whose code never arrived should be able to press the button again + // immediately, not wait out a minute for a code that does not exist. + try { + await verificationRef(uid).delete(); + } catch (cleanupError) { + // Non-fatal, but worth a line: the researcher is now sitting on a + // cooldown for a code they never got. + console.error( + `send-contact-email-verification: could not clear the unsent record for ${uid}:`, + cleanupError instanceof Error ? cleanupError.message : "Unknown error" + ); + } + console.error( + `send-contact-email-verification: delivery for ${uid} ended as ${outcome}, mail ${mailRef.id}` + ); + res.status(503).json({ + error: + "We couldn't send a verification code right now. Please try again in a little while.", + code: "mail-unavailable", + }); + return; + } + res.status(200).json({ success: true }); } catch (error) { console.error( From caa6a2418ac2df177c15590d02059bf91e688b9c Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Sat, 29 Aug 2026 13:07:55 -0400 Subject: [PATCH 2/3] Stop deleting data the researcher was never told about Two ways the queue cleanup lost data nobody meant to lose. It deleted on `createdAt <= now - 7d` with no check on status and no check on whether anyone had been told -- and since there is no GCS lifecycle rule on the bucket, that sweep is the entire retention policy. 1. A STORAGE PROVIDER OUTAGE. The entry is still pending with retries left, so it would have uploaded fine on day eight. Deleting it on day seven throws away data that was never actually lost. 2. A NOTIFICATION THAT NEVER ARRIVED. The seven days count from SUBMISSION, so part of the window is spent before anything goes wrong -- and if the notification died on a quota outage, the window closes without the researcher ever learning there was one. upload-retention.ts now decides, per entry: delete past 14 days from createdAt whatever else is true; retain while pending with retries left; retain while retainUntil is in the future; otherwise delete, unchanged. Its own module and a pure function, because it decides the least reversible thing here and scheduled-upload-retry.ts pulls in the whole provider stack -- a test wanting only this predicate would have to load all of it. scheduled-mail-retry.ts writes retainUntil while a notification is undelivered, covering EVERY unresolved entry for the experiment rather than the one that tripped the episode. datapipe.queueDocId records only the trigger; extending that alone would leave the rest of the episode's files expiring on schedule, which is the same bug in miniature. The retention pass runs BEFORE the breaker check and therefore also while sending is paused. Extending is a Firestore write that costs no quota, and a quota outage is precisely when data ages towards deletion behind a notification that never arrived -- putting it after the check would disable the protection in the only case that needs it. An experiment whose owner has no contact email is never extended, and that falls out rather than being special-cased: upload-failure-notify.ts records suppressedReason "no-contact-email" and returns before enqueuing, so no mail document exists for the sweeper to find. Plain seven days, which is right when there is nobody to tell. The 14-day ceiling is load-bearing. Without it an experiment whose provider is dead and whose owner never reads their mail would hold research payloads forever, silently, at DataPipe's cost. THE WARNING BANNER Targets UNVERIFIED addresses, not missing ones. ContactEmailGate already walls off every admin route until a usable address exists, so "no contact email" is nearly extinct; what the gate does not check is whether the address works, since hasContactEmail() tests format only. A typo passes it, so does anything the 2026-08 backfill seeded from Auth, and upload-failure-notify.ts mails the address regardless of verified status -- so it bounces and is marked terminally failed somewhere nobody looks. Not dismissible and no flag to maintain: contactEmailVerified is written server-side by verify-contact-email.ts and only there, so the banner removes itself the moment it is acted on. 10 new tests, 1162 total. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D1dNU7EnmBBMTn8MWfojDL --- components/account/UnverifiedEmailBanner.js | 91 ++++++++++++++ docs/deploy-contact-email.md | 60 +++++++++ .../src/__tests__/mail-availability.test.js | 81 ++++++++++++ .../src/__tests__/mail-retry-emulator.test.js | 95 ++++++++++++-- functions/src/scheduled-mail-retry.ts | 119 ++++++++++++++++-- functions/src/scheduled-upload-retry.ts | 20 ++- functions/src/upload-retention.ts | 92 ++++++++++++++ pages/admin/index.js | 2 + 8 files changed, 538 insertions(+), 22 deletions(-) create mode 100644 components/account/UnverifiedEmailBanner.js create mode 100644 functions/src/upload-retention.ts diff --git a/components/account/UnverifiedEmailBanner.js b/components/account/UnverifiedEmailBanner.js new file mode 100644 index 0000000..353c7e5 --- /dev/null +++ b/components/account/UnverifiedEmailBanner.js @@ -0,0 +1,91 @@ +import { useContext } from "react"; +import { Box, Text, VStack, Button, HStack } from "@chakra-ui/react"; +import Link from "next/link"; +import { doc } from "firebase/firestore"; +import { useDocumentData } from "react-firebase-hooks/firestore"; +import { UserContext } from "../../lib/context"; +import { db } from "../../lib/firebase"; +import { hasContactEmail } from "../../lib/contact-email"; + +// Shown to researchers whose contact address has never been confirmed. +// +// --------------------------------------------------------------------------- +// WHY *UNVERIFIED* AND NOT *MISSING* +// --------------------------------------------------------------------------- +// +// "No contact email" is very nearly extinct, and deliberately so: +// ContactEmailGate (components/AuthCheck.js) walls off every admin route until +// a usable address exists, so a researcher cannot reach this dashboard without +// one. What the gate does NOT check is whether the address works -- +// hasContactEmail() tests format only, never contactEmailVerified. +// +// That gap is the entire failure mode. A typo passes the gate. So does an +// address the 2026-08 backfill seeded from Firebase Auth, which is real but +// was never confirmed against the person now using the account. And +// upload-failure-notify.ts mails the address regardless of verified status -- +// so the notification goes out, hard-bounces, and is marked terminally failed +// somewhere nobody looks. The researcher's data stopped arriving and the only +// system that could tell them believes it did. +// +// This banner exists because that failure is invisible from the researcher's +// side by construction: the symptom of a notification you cannot receive is +// silence, which is indistinguishable from everything being fine. +// +// --------------------------------------------------------------------------- +// NOT DISMISSIBLE, AND NO FLAG TO MAINTAIN +// --------------------------------------------------------------------------- +// +// Same reasoning as AddSignInMethodBanner: this is not a nag, it is the one +// warning about a silent data-loss path, and it removes itself the moment it is +// acted on. `contactEmailVerified` flipping to true is written server-side by +// verify-contact-email.ts and ONLY there, so the condition is exact and there +// is no dismissal state to store, expire, or reset when the address changes. +export default function UnverifiedEmailBanner() { + const { user } = useContext(UserContext); + + const [userDoc, loading] = useDocumentData( + user ? doc(db, "users", user.uid) : null + ); + + if (!user || loading || !userDoc) return null; + + // Nothing to say to someone who is already reachable. + if (userDoc.contactEmailVerified === true) return null; + + // No address at all is ContactEmailGate's job, not this banner's -- and a + // researcher seeing this dashboard has already been past it. Staying quiet + // here means the two never argue about the same account. + if (!hasContactEmail(userDoc)) return null; + + return ( + + + + + Confirm your email address + + + DataPipe emails you if data stops uploading for one of your + experiments — but {userDoc.contactEmail} has never been confirmed, + so we have no way to know it reaches you. If it does not, that + notification is the one you would never see. + + + + + + + + + ); +} diff --git a/docs/deploy-contact-email.md b/docs/deploy-contact-email.md index 1225ab2..23729f6 100644 --- a/docs/deploy-contact-email.md +++ b/docs/deploy-contact-email.md @@ -497,6 +497,66 @@ lines reporting a non-zero `agedOut` (notifications that expired undelivered), and any accumulation of `delivery.state == "ERROR"` with `retryable == true`, which is the sweeper's backlog. +## 7. Payload retention: the clock now measures the right thing + +`scheduled-upload-retry.ts` deletes queue entries and their Cloud Storage +payloads seven days after **submission**. That is the only thing that deletes +them — there is no GCS lifecycle rule on the bucket — so this sweep is the whole +retention policy. + +Counting from submission is subtly wrong in two ways, and both lose data nobody +meant to lose: + +- **A storage-provider outage.** The entry is still `pending` with retries left, + so it would have uploaded fine on day eight. Deleting it on day seven throws + away data that was never actually lost. +- **A notification that never arrived.** Part of the seven days is spent before + anything goes wrong, and if the notification died (a quota outage, a bounced + address) the window closes without the researcher ever learning there was one. + +`upload-retention.ts` now decides, and the sweep asks it per entry: + +| Condition | Outcome | +|---|---| +| Older than **14 days** from `createdAt` | **delete** — the ceiling wins over everything | +| `status: "pending"` with retries left | retain — the upload may yet succeed | +| `retainUntil` in the future | retain — the researcher has not been told | +| otherwise | delete — unchanged from before | + +`retainUntil` is written by `scheduled-mail-retry.ts` while an upload-failure +notification is undelivered, and it covers **every unresolved entry for the +experiment**, not just the one that tripped the episode. It is written even +while the breaker is shut, because extending is a Firestore write that costs no +quota and an outage is exactly when the data is at risk. + +**What deliberately does not extend:** an experiment whose owner has no contact +email. `upload-failure-notify.ts` records `suppressedReason: "no-contact-email"` +and returns before enqueuing any mail, so no mail document exists, so nothing +extends it — it keeps the plain seven days. That is the right answer when there +is nobody to tell, and it falls out of the design rather than being special-cased. + +**The 14-day ceiling is not optional.** Without it, an experiment whose provider +is dead and whose owner never reads their mail would hold research payloads +forever, silently, at DataPipe's cost. + +## 8. The unverified-address warning + +`ContactEmailGate` walls off every admin route until a usable address exists, so +"no contact email" is nearly extinct among active researchers. What the gate +does not check is whether the address **works** — `hasContactEmail()` tests +format only, never `contactEmailVerified`. + +That gap is the failure mode. A typo passes the gate. So does anything the +2026-08 backfill seeded from Firebase Auth. And upload-failure notifications go +to the address regardless of verified status, so the mail bounces and is marked +terminally failed somewhere nobody looks. + +`components/account/UnverifiedEmailBanner.js` renders on the dashboard whenever +`contactEmailVerified` is false and an address exists. Not dismissible, and with +no flag to maintain: `contactEmailVerified` is written server-side by +`verify-contact-email.ts` and only there, so the banner removes itself the moment +it is acted on. + ## Not covered here Firestore index changes (none expected — see the design doc §3.4/§7 on why diff --git a/functions/src/__tests__/mail-availability.test.js b/functions/src/__tests__/mail-availability.test.js index cffd6b7..15a3913 100644 --- a/functions/src/__tests__/mail-availability.test.js +++ b/functions/src/__tests__/mail-availability.test.js @@ -37,6 +37,8 @@ let sweepDecision; let MAX_SWEEP_AGE_MS; let IDEMPOTENCY_WINDOW_MS; +let retentionDecision; + beforeAll(async () => { ({ verificationAvailability, @@ -50,6 +52,8 @@ beforeAll(async () => { ({ sweepDecision, MAX_SWEEP_AGE_MS, IDEMPOTENCY_WINDOW_MS } = await import( "../../lib/scheduled-mail-retry.js" )); + + ({ retentionDecision } = await import("../../lib/upload-retention.js")); }); // Stored Timestamps, as the Admin SDK hands them back. @@ -334,3 +338,80 @@ describe("sweepDecision", () => { expect(sweepDecision(doc({ lastAttemptAt: ts(NOON - 1000) }), NOON)).toBe("skip"); }); }); + +// --------------------------------------------------------------------------- +// 4. May this researcher's unuploaded data be destroyed? +// --------------------------------------------------------------------------- + +describe("retentionDecision", () => { + const DAY = 24 * 60 * 60 * 1000; + // The sweep only ever asks about entries already older than seven days. + const entry = (over = {}) => ({ + status: "failed", + retryCount: 5, + maxRetries: 5, + createdAt: ts(NOON - 8 * DAY), + ...over, + }); + + test("deletes an ordinary aged-out entry, exactly as before", () => { + // The unchanged default. Nothing below should make the common case keep + // data longer than it used to. + expect(retentionDecision(entry(), NOON)).toBe("delete"); + }); + + test("retains an entry whose upload is still being retried", () => { + // The storage-provider outage case: this would have uploaded fine on day + // eight, so deleting it on day seven throws away data that was never + // actually lost. + expect( + retentionDecision(entry({ status: "pending", retryCount: 3 }), NOON) + ).toBe("retain"); + }); + + test("deletes a pending entry whose retries are exhausted", () => { + // "Pending" alone is not a reason to keep it -- an entry that has spent its + // whole retry budget is not live work, it is a corpse with a hopeful status. + expect( + retentionDecision(entry({ status: "pending", retryCount: 5 }), NOON) + ).toBe("delete"); + }); + + test("retains an entry the researcher has not been told about", () => { + expect( + retentionDecision(entry({ retainUntil: ts(NOON + 2 * DAY) }), NOON) + ).toBe("retain"); + }); + + test("deletes once the extension itself has expired", () => { + expect( + retentionDecision(entry({ retainUntil: ts(NOON - 1000) }), NOON) + ).toBe("delete"); + }); + + test("the absolute ceiling beats every reason to keep it", () => { + // Without this, an experiment whose provider is dead and whose owner never + // reads their mail would hold research payloads in Cloud Storage forever, + // silently, at DataPipe's cost. Both extension paths are tested against it + // because either one alone would otherwise be unbounded. + const ancient = { createdAt: ts(NOON - 15 * DAY) }; + expect( + retentionDecision( + entry({ ...ancient, status: "pending", retryCount: 0 }), + NOON + ) + ).toBe("delete"); + expect( + retentionDecision( + entry({ ...ancient, retainUntil: ts(NOON + 5 * DAY) }), + NOON + ) + ).toBe("delete"); + }); + + test("a missing or unreadable createdAt does not disable the ceiling check", () => { + // Defensive: a hand-written or partially-migrated entry must not become + // immortal by lacking a field. + expect(retentionDecision({ status: "failed" }, NOON)).toBe("delete"); + }); +}); diff --git a/functions/src/__tests__/mail-retry-emulator.test.js b/functions/src/__tests__/mail-retry-emulator.test.js index 4b8982d..27182f0 100644 --- a/functions/src/__tests__/mail-retry-emulator.test.js +++ b/functions/src/__tests__/mail-retry-emulator.test.js @@ -102,6 +102,21 @@ async function seedMail({ delivery, inline = false, kind = "upload-failure" } = return { ref, id: ref.id }; } +async function seedQueueEntry(experimentID, { status = "failed", ageMs = 8 * 24 * 60 * 60 * 1000 } = {}) { + const ref = db.collection("uploadQueue").doc(); + created.push(ref); + await ref.set({ + experimentID, + owner: `mr-user-${randomUUID()}`, + status, + retryCount: 5, + maxRetries: 5, + storagePath: `pending-data/${experimentID}/subject-1.json`, + createdAt: Timestamp.fromMillis(Date.now() - ageMs), + }); + return ref; +} + const statusRef = () => db.collection("systemStatus").doc(STATUS_DOC_ID); const deliveryOf = async (ref) => (await ref.get()).data()?.delivery; @@ -255,12 +270,12 @@ describe("inline mail", () => { // --------------------------------------------------------------------------- describe("sweepRetryableMail", () => { - test("sweeps NOTHING while the breaker is shut, and burns no attempts", async () => { - // The most important assertion in this file. Sweeping into an exhausted - // quota fails every document and spends one of its three MAX_ATTEMPTS - // doing it -- so a day-long outage would exhaust the retry budget of every - // queued mail and turn all of them terminal, which is the exact opposite of - // what the sweeper is for. + test("SENDS nothing while the breaker is shut, and burns no attempts", async () => { + // The most important send-side assertion in this file. Sweeping into an + // exhausted quota fails every document and spends one of its three + // MAX_ATTEMPTS doing it -- so a day-long outage would exhaust the retry + // budget of every queued mail and turn all of them terminal, which is the + // exact opposite of what the sweeper is for. const send = sendingOk(); _setMailSenderForTests(send); const { ref } = await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); @@ -272,7 +287,6 @@ describe("sweepRetryableMail", () => { const report = await sweepRetryableMail(); expect(report.paused).toBe(true); - expect(report.scanned).toBe(0); expect(send).not.toHaveBeenCalled(); // Untouched: attempts did not creep up by being looked at. expect((await deliveryOf(ref)).attempts).toBe(1); @@ -361,6 +375,73 @@ describe("sweepRetryableMail", () => { expect(send).toHaveBeenCalledTimes(1); }); + test("holds the data back EVEN WHILE PAUSED -- the case that needs it most", async () => { + // Retention is a Firestore write, not a send: it costs no quota, so an + // exhausted quota is no reason to skip it. The opposite, in fact. A quota + // outage is exactly when a researcher's unuploaded data is ageing towards + // deletion behind a notification that never arrived, so putting this after + // the breaker check would switch the protection off in the only situation + // that needs it. + const send = sendingOk(); + _setMailSenderForTests(send); + const experimentID = `mr-exp-${randomUUID()}`; + const entry = await seedQueueEntry(experimentID); + const { ref } = await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + await ref.update({ "datapipe.experimentID": experimentID }); + await statusRef().set({ + unavailableUntil: Timestamp.fromMillis(Date.now() + 60 * 60 * 1000), + }); + + const report = await sweepRetryableMail(); + + expect(report.paused).toBe(true); + expect(send).not.toHaveBeenCalled(); + expect(report.retained).toBe(1); + const retainUntil = (await entry.get()).data().retainUntil; + expect(retainUntil.toMillis()).toBeGreaterThan(Date.now()); + }); + + test("holds back EVERY unresolved entry for the experiment, not just the one that tripped it", async () => { + // A notification is per EPISODE, and an episode belongs to an experiment, + // not to one file. datapipe.queueDocId records only the entry that tripped + // it -- extending just that one would leave the rest of the episode's data + // expiring on schedule, which is the original bug in miniature. + _setMailSenderForTests(sendingOk()); + const experimentID = `mr-exp-${randomUUID()}`; + const entries = [ + await seedQueueEntry(experimentID), + await seedQueueEntry(experimentID), + await seedQueueEntry(experimentID, { status: "pending" }), + ]; + const { ref } = await seedMail({ delivery: failedDelivery("daily_quota_exceeded") }); + await ref.update({ "datapipe.experimentID": experimentID }); + + const report = await sweepRetryableMail(); + + expect(report.retained).toBe(3); + for (const entry of entries) { + expect((await entry.get()).data().retainUntil.toMillis()).toBeGreaterThan( + Date.now() + ); + } + }); + + test("holds nothing back for a verification code -- there is no data behind it", async () => { + _setMailSenderForTests(sendingOk()); + const experimentID = `mr-exp-${randomUUID()}`; + const entry = await seedQueueEntry(experimentID); + const { ref } = await seedMail({ + kind: "contact-email-verification", + delivery: failedDelivery("daily_quota_exceeded"), + }); + await ref.update({ "datapipe.experimentID": experimentID }); + + const report = await sweepRetryableMail(); + + expect(report.retained).toBe(0); + expect((await entry.get()).data().retainUntil).toBeUndefined(); + }); + test("an empty mail collection is a quiet no-op", async () => { const send = sendingOk(); _setMailSenderForTests(send); diff --git a/functions/src/scheduled-mail-retry.ts b/functions/src/scheduled-mail-retry.ts index d9d25fd..10165cc 100644 --- a/functions/src/scheduled-mail-retry.ts +++ b/functions/src/scheduled-mail-retry.ts @@ -55,6 +55,7 @@ import { MAIL_RETENTION_MS, } from "./mail-delivery.js"; import { deliveryPaused, readMailStatus } from "./mail-availability.js"; +import { UNRESOLVED_STATUSES } from "./upload-failure-notify.js"; // How long Resend honours an Idempotency-Key. The bound on retrying anything // that is not provably un-sent. Raising this without checking Resend's docs @@ -79,6 +80,24 @@ export const MAX_SWEEP_AGE_MS = 3 * 24 * 60 * 60 * 1000; // recoverable backlog into a burned retry budget. export const SWEEP_LIMIT = 25; +// How much longer a researcher's unuploaded data is kept while the +// notification about it is still undelivered. +// +// Seven days is not arbitrary: it is the SAME window the researcher would have +// had if the notification had worked. scheduled-upload-retry.ts's sweep counts +// from `createdAt` -- submission -- so by the time a notification fails, part +// of that window is already spent on a problem the researcher could not have +// known about. This gives it back, and keeps giving it back until they are +// actually told. The absolute ceiling lives in scheduled-upload-retry.ts's +// ABSOLUTE_MAX_RETENTION_MS, where the deletion happens, so there is exactly +// one place that can decide data is gone. +export const RETENTION_GRACE_MS = 7 * 24 * 60 * 60 * 1000; + +// Unresolved entries touched per undelivered notification. An episode is +// usually a handful of files; the cap is there so one pathological experiment +// cannot make a sweep unbounded. +export const RETENTION_BATCH = 100; + export interface SweepReport { scanned: number; delivered: number; @@ -86,6 +105,9 @@ export interface SweepReport { skipped: number; agedOut: number; paused: boolean; + // Queue entries whose deletion was pushed back because the researcher has + // not been told about them yet. + retained: number; } function millisOrZero(value: unknown): number { @@ -132,6 +154,48 @@ export function sweepDecision( return "skip"; } +/** + * Hold back the data an undelivered notification is ABOUT. + * + * The notification is per EPISODE, and an episode belongs to an experiment, not + * to one file -- `datapipe.queueDocId` merely records the entry that tripped + * it. So the extension has to cover every unresolved entry for the experiment; + * extending only the one that tripped it would leave the other nineteen failed + * files in the same episode expiring on schedule, which is the original bug in + * miniature. + * + * Returns how many entries were held back. + */ +async function extendRetentionFor( + mailData: FirebaseFirestore.DocumentData, + nowMs: number +): Promise { + const meta = (mailData.datapipe ?? {}) as Record; + // Verification codes have no data behind them to keep. + if (meta.kind !== "upload-failure") return 0; + const experimentID = typeof meta.experimentID === "string" ? meta.experimentID : null; + if (!experimentID) return 0; + + // `experimentID ==` plus `status in` is served as a prefix by the existing + // (experimentID, status, providerErrorCode) composite -- the same shape + // upload-failure-notify.ts's drain query already runs. + const entries = await db + .collection("uploadQueue") + .where("experimentID", "==", experimentID) + .where("status", "in", UNRESOLVED_STATUSES) + .limit(RETENTION_BATCH) + .get(); + if (entries.empty) return 0; + + const retainUntil = Timestamp.fromMillis(nowMs + RETENTION_GRACE_MS); + const batch = db.batch(); + for (const entry of entries.docs) { + batch.update(entry.ref, { retainUntil }); + } + await batch.commit(); + return entries.size; +} + /** * One sweep. * @@ -148,19 +212,9 @@ export async function sweepRetryableMail(nowMs = Date.now()): Promise [doc, sweepDecision(doc.data(), nowMs)] as const + ); + + // ------------------------------------------------------------------------- + // RETENTION FIRST, AND DELIBERATELY BEFORE THE BREAKER. + // ------------------------------------------------------------------------- + // + // Holding a queue entry back is a Firestore write, not a send: it costs no + // quota, so nothing about an exhausted quota is a reason to skip it. The + // opposite, in fact -- a quota outage is precisely when a researcher's data + // is quietly ageing towards deletion behind a notification that never + // arrived. Putting this after the breaker check would switch the protection + // off in the only situation that needs it. + for (const [doc, decision] of decisions) { + if (decision !== "deliver") continue; + try { + report.retained += await extendRetentionFor(doc.data(), nowMs); + } catch (error) { + // Never fatal. Failing to extend costs the researcher time, but throwing + // here would cost them the sweep as well. + console.error( + `scheduled-mail-retry: could not extend retention for ${doc.id}:`, + error instanceof Error ? error.message : "Unknown error" + ); + } + } + + // The breaker. See the header: sweeping into an exhausted quota fails every + // document and spends a MAX_ATTEMPTS on each failure. + const status = await readMailStatus(); + if (deliveryPaused(status, nowMs)) { + report.paused = true; + console.log( + "scheduled-mail-retry: delivery is paused (quota), sending nothing this pass" + ); + return report; + } + for (const [doc, decision] of decisions) { if (decision === "skip") { report.skipped += 1; continue; diff --git a/functions/src/scheduled-upload-retry.ts b/functions/src/scheduled-upload-retry.ts index cd9d597..62eec18 100644 --- a/functions/src/scheduled-upload-retry.ts +++ b/functions/src/scheduled-upload-retry.ts @@ -9,6 +9,7 @@ import { ExperimentData, UserData } from "./interfaces.js"; import { isFastRetry } from "./queue-upload.js"; import { isCompactionInFlight, COMPACTION_HOLD_REASON } from "./compaction-gate.js"; import { decryptPayload } from "./payload-crypto.js"; +import { retentionDecision } from "./upload-retention.js"; const SEVEN_DAYS_MS = 7 * 24 * 60 * 60 * 1000; const MAX_BACKOFF_MS = 24 * 60 * 60 * 1000; // 24 hours (slow tier cap, unchanged) @@ -417,17 +418,32 @@ async function cleanupOldEntries() { return; } - console.log(`Cleaning up ${oldEntries.size} old queue entries.`); - const bucket = storage.bucket(); + const now = Date.now(); + let deleted = 0; + let retained = 0; for (const doc of oldEntries.docs) { const data = doc.data(); + + // The query above finds entries by AGE. Whether one may actually be + // destroyed is a separate question, and not one an age answers on its own + // -- see upload-retention.ts. + if (retentionDecision(data, now) === "retain") { + retained += 1; + continue; + } + try { await bucket.file(data.storagePath).delete(); } catch { // File may already be deleted } await doc.ref.delete(); + deleted += 1; } + + console.log( + `Cleanup: ${deleted} old queue entries deleted, ${retained} retained (still retrying, or the researcher has not been notified yet).` + ); } diff --git a/functions/src/upload-retention.ts b/functions/src/upload-retention.ts new file mode 100644 index 0000000..d20b4a4 --- /dev/null +++ b/functions/src/upload-retention.ts @@ -0,0 +1,92 @@ +// Whether a researcher's unuploaded data may be destroyed yet. +// +// Its own module, and a pure function, for two reasons. It decides the least +// reversible thing in this codebase -- deleting research data the researcher +// has not got back -- so it should be assertable as a table rather than +// provoked through a scheduler. And scheduled-upload-retry.ts, where the +// deletion actually happens, imports the whole provider stack; a test that +// wanted only this predicate would have to load all of it. +// +// --------------------------------------------------------------------------- +// WHY THE PLAIN AGE TEST WAS NOT ENOUGH +// --------------------------------------------------------------------------- +// +// The cleanup sweep used to delete on `createdAt <= now - 7d` alone -- no check +// on status, and no check on whether anyone had been told. Two ways that loses +// data nobody meant to lose: +// +// 1. A STORAGE PROVIDER OUTAGE. The entry is still `pending` with retries +// left, so it would have uploaded fine on day eight. Deleting it on day +// seven throws away data that was never actually lost. +// +// 2. A NOTIFICATION THAT NEVER ARRIVED. The seven days are counted from +// SUBMISSION, so part of the window is already spent before anything goes +// wrong -- and if the notification died (a Resend quota outage, say) the +// researcher's window closes without them ever learning there was one. +// +// `retainUntil` is written by scheduled-mail-retry.ts while an upload-failure +// notification for the experiment is still undelivered and still deliverable. +// +// NOTE WHAT DOES NOT WRITE IT. An experiment whose owner has no contact email +// never produces a mail document at all -- upload-failure-notify.ts records +// `suppressedReason: "no-contact-email"` and returns before enqueuing -- so it +// is never extended and keeps the plain seven days. That is the right answer +// when there is nobody to tell, and it falls out of the design rather than +// being special-cased. + +// The ceiling on everything below. An entry is deleted once it reaches this +// age no matter what else is still true about it. +// +// The extensions exist so a researcher gets a fair chance to act on data that +// has not uploaded. This exists so that chance cannot become permanent storage +// of research payloads DataPipe was never able to deliver: an experiment whose +// provider is dead and whose owner never reads their mail would otherwise +// accumulate forever, silently, at DataPipe's cost. Fourteen days is the plain +// seven a researcher was always promised, plus another seven to absorb an +// outage or a missed notification. +export const ABSOLUTE_MAX_RETENTION_MS = 14 * 24 * 60 * 60 * 1000; + +function millisOrZero(value: unknown): number { + if (!value || typeof (value as { toMillis?: unknown }).toMillis !== "function") { + return 0; + } + return (value as { toMillis: () => number }).toMillis(); +} + +/** + * Is this aged-out queue entry safe to delete? + * + * Only ever asked about entries the sweep has already found by age, so + * "delete" here means "the seven days are up AND none of the reasons to keep it + * apply". + */ +export function retentionDecision( + data: FirebaseFirestore.DocumentData, + nowMs: number +): "delete" | "retain" { + const createdAt = millisOrZero(data.createdAt); + + // The ceiling wins over every reason to keep it. Checked FIRST, and checked + // even when createdAt is missing or unreadable -- an entry must not become + // immortal by lacking a field. + if (createdAt === 0 || nowMs - createdAt >= ABSOLUTE_MAX_RETENTION_MS) { + return "delete"; + } + + // Still live work. The upload itself may yet succeed, which makes this data + // not merely un-notified but not actually lost. "Pending" alone is not + // enough: an entry that has spent its whole retry budget is not live work, + // it is a corpse with a hopeful status. + const retryCount = typeof data.retryCount === "number" ? data.retryCount : 0; + const maxRetries = typeof data.maxRetries === "number" ? data.maxRetries : 0; + if (data.status === "pending" && retryCount < maxRetries) { + return "retain"; + } + + // The researcher has not been told yet, and still might be. + if (millisOrZero(data.retainUntil) > nowMs) { + return "retain"; + } + + return "delete"; +} diff --git a/pages/admin/index.js b/pages/admin/index.js index c5bd488..f01809e 100644 --- a/pages/admin/index.js +++ b/pages/admin/index.js @@ -18,6 +18,7 @@ import { } from "@chakra-ui/react"; import { Trash2 } from "lucide-react"; import AddSignInMethodBanner from "../../components/account/AddSignInMethodBanner"; +import UnverifiedEmailBanner from "../../components/account/UnverifiedEmailBanner"; import OsfSunsetNotice from "../../components/OsfSunsetNotice"; import { isLegacyOsfExperiment } from "../../lib/osf-sunset"; import { STORAGE_PROVIDERS } from "../../lib/provider-config"; @@ -36,6 +37,7 @@ export default function AdminPage({}) { routes (960 / 540 / 1200). */} + From 64687184bcbdcaf33bb3c6353a0d1875d4009bf8 Mon Sep 17 00:00:00 2001 From: Josh de Leeuw Date: Sat, 29 Aug 2026 15:08:02 -0400 Subject: [PATCH 3/3] Close the ways mail could stop for a month, or stall forever The breaker and the sweeper both assumed the only way sending fails is a daily quota that resets at midnight. Neither assumption survives contact with the other failure modes. WHY SENDING STOPPED DECIDES HOW LONG IT STAYS STOPPED The free plan has two caps -- 100/day and 3,000/month -- on different clocks. Reusing the daily reset for a monthly exhaustion is not a small error: hit the monthly cap on the 20th and the breaker reopens at midnight, the sweeper probes into a cap with eleven days left to run, and spends one of each queued document's MAX_ATTEMPTS doing it. Three nights of that and every queued notification is terminal -- the exact retry-budget exhaustion the breaker exists to prevent. `PauseKind` names the three reasons and `pauseUntil` maps each to a reset: daily-quota to the next UTC midnight, monthly-quota to the next month, and systemic to a 15-minute cooldown. `pauseKindFor` is the table that turns a Resend error name into one, and both call sites go through it. SYSTEMIC FAILURES TRIP IT TOO A revoked key or an unverified domain is not exhaustion, and previously nothing shut on it: verificationAvailability answered "available" forever while every click minted a code, wrote a document, spent a real request and failed, with no server-side ceiling. Tripping bounds that to one request per SYSTEMIC_PAUSE_MS however many researchers are pressing the button. `validation_error` is deliberately NOT in SYSTEMIC_ERRORS. Resend uses it both for an unverified sending domain and for one researcher's typo, and a typo must not switch verification off for everybody. THE HEADER READING IS A GUESS, AND IT IS BOUNDED Resend documents x-resend-daily-quota's existence, not its semantics -- "used today" and "remaining today" are the same shape. If it is really the plan limit, every reading is 100, the reserve rule is true forever, and each send rewrites dailyQuotaObservedAt so the staleness escape hatch never fires either. `usableQuotaReading` refuses anything outside [0, 100] at the write and ignores it at the read, refusing loudly; refusing a verification on the reserve alone now logs at ERROR with a stable token, so the condition is alertable rather than silent. The runbook says how to check the number against the dashboard before trusting it. THE VERIFICATION RECORD IS THE RATE LIMIT, SO IT STAYS The failure branch used to delete it, reasoning that a researcher whose code never arrived should not wait out a cooldown for a code that does not exist. Right about the researcher, wrong about the endpoint: `sentAt` is the only server-side throttle on that path, and deleting it removed the throttle from exactly the case that needs one. The record now survives with `codeHash` cleared -- a cooldown, not a secret nobody received -- and verify-contact-email.ts answers "request a new one" rather than spending one of the five attempts on a code that was never sent. EVERY DOCUMENT THE SWEEP FINDS LEAVES BY A DOOR The queries are unordered limits, so anything the pass can look at without changing it will look at again next pass, forever, occupying the budget a deliverable notification needed. The decision table has no permanent skip left in it: every outcome either sends or writes a terminal state that takes the document out of both queries. The only skips are documents another invocation holds, which resolve within LEASE_MS. That matters beyond starvation. A retryable ERROR never gets delivery.expireAt, so it sits outside the TTL policy holding a researcher's address indefinitely. Ageing one out is how that address is finally deleted. A second query finds stranded claims. deliverMailDocument's claim rewrites the document to PROCESSING with `retryable: null` BEFORE the send, so an instance that dies mid-send left a document neither the retryable-ERROR query nor the TTL could see -- a preempted instance or a mid-deploy roll stranded an address permanently. claimDecision already knew how to recover an expired lease; nothing ever asked it to. The idempotency window is now measured from `startTime`, which is when the key was first used and never moves. Measuring from the last attempt slid the window forward with every retry, so a document retried at +20h and again at +40h was sent on a key that expired at +24h and Resend treated it as a new message. RETENTION MOVES NEXT TO THE PREDICATE THAT READS IT `extendRetentionForExperiment` lives in upload-retention.ts now. Splitting "may this data be destroyed, and what holds it back" across the mail sweeper, that module and the deletion sweep meant three files in two subsystems had to be read together to answer one question, and the mail subsystem had to know the uploadQueue's schema. The sweeper now says only what it knows -- this researcher has not been told -- and retention decides what that means. Inverting it completely was considered and rejected: asking "is a notification still undelivered?" at deletion time makes the least reversible operation in the codebase depend on a query that can fail, and a failing query there deletes data. `retainUntil` fails the other way. It covers documents about to be AGED OUT as well as sent, which is the more important half -- giving up is when the researcher will never be told at all, and it must not also be when their data goes back on the original clock. Once per experiment rather than once per document, and skipped entirely when the stored value is already more than half the grace window out: the sweeper runs every ten minutes, so without that floor a day-long outage rewrites the same field tens of thousands of times to no effect, against a 20,000/day free tier. THE DELETION SWEEP PAGES PAST WHAT IT MAY NOT DELETE cleanupOldEntries found entries by age, ascending, with one limit(50) -- but whether one may be destroyed is a separate question, and a retained entry is not removed from the result set by being looked at. Fifty retained entries at the head meant a pass that deleted nothing, forever: one experiment stuck behind a dead provider stopped every other experiment's payloads from being deleted until the blockers crossed the 14-day ceiling. It now scans up to 500 with a cursor to find its 50. ALSO - mailCollection() indirection, so a suite can isolate the collection the sweep queries. Isolating the status document was never enough on its own; the collection is the other shared singleton. The trigger still binds to MAIL_COLLECTION, which is deploy-time configuration a test may not move. - One users/{uid} subscription on /admin, passed down, replacing two. - leaseIsExpired, pauseKindFor and unresolvedQueueEntriesQuery each own a rule that was previously written out twice. - Age-out and retention writes run concurrently; a backlog was up to fifty serialized round-trips. Not batched -- a batch is atomic, and one document purged mid-sweep would take the rest with it. - Composite index for delivery.state + delivery.leaseExpiresAt. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01D1dNU7EnmBBMTn8MWfojDL --- components/account/UnverifiedEmailBanner.js | 27 +- docs/deploy-contact-email.md | 102 ++++- firestore.indexes.json | 14 + .../src/__tests__/mail-availability.test.js | 258 +++++++++++- functions/src/__tests__/mail-delivery.test.js | 116 +++++ .../src/__tests__/mail-retry-emulator.test.js | 354 +++++++++++++++- functions/src/mail-availability.ts | 125 +++++- functions/src/mail-delivery.ts | 126 +++++- functions/src/mail.ts | 29 +- functions/src/purge-user-data.ts | 5 +- functions/src/scheduled-mail-retry.ts | 396 +++++++++++------- functions/src/scheduled-upload-retry.ts | 100 +++-- .../src/send-contact-email-verification.ts | 56 ++- functions/src/upload-failure-notify.ts | 26 +- functions/src/upload-retention.ts | 133 +++++- functions/src/verify-contact-email.ts | 32 +- pages/admin/index.js | 44 +- 17 files changed, 1615 insertions(+), 328 deletions(-) diff --git a/components/account/UnverifiedEmailBanner.js b/components/account/UnverifiedEmailBanner.js index 353c7e5..369c163 100644 --- a/components/account/UnverifiedEmailBanner.js +++ b/components/account/UnverifiedEmailBanner.js @@ -1,10 +1,5 @@ -import { useContext } from "react"; import { Box, Text, VStack, Button, HStack } from "@chakra-ui/react"; import Link from "next/link"; -import { doc } from "firebase/firestore"; -import { useDocumentData } from "react-firebase-hooks/firestore"; -import { UserContext } from "../../lib/context"; -import { db } from "../../lib/firebase"; import { hasContactEmail } from "../../lib/contact-email"; // Shown to researchers whose contact address has never been confirmed. @@ -40,14 +35,20 @@ import { hasContactEmail } from "../../lib/contact-email"; // acted on. `contactEmailVerified` flipping to true is written server-side by // verify-contact-email.ts and ONLY there, so the condition is exact and there // is no dismissal state to store, expire, or reset when the address changes. -export default function UnverifiedEmailBanner() { - const { user } = useContext(UserContext); - - const [userDoc, loading] = useDocumentData( - user ? doc(db, "users", user.uid) : null - ); - - if (!user || loading || !userDoc) return null; +// +// --------------------------------------------------------------------------- +// NO SUBSCRIPTION OF ITS OWN +// --------------------------------------------------------------------------- +// +// `userDoc` is the users/{uid} document, passed down from the dashboard's +// single subscription -- the same convention ContactEmail, ProviderConnections +// and SelectAuth follow on the account page. This component used to open its +// own live listener, which made three on one document on /admin (AuthCheck's +// gate, the experiment list's provider check, and this), each with its own +// loading flicker. Undefined means "not loaded yet" and renders nothing, +// exactly as the in-component loading flag did. +export default function UnverifiedEmailBanner({ userDoc }) { + if (!userDoc) return null; // Nothing to say to someone who is already reachable. if (userDoc.contactEmailVerified === true) return null; diff --git a/docs/deploy-contact-email.md b/docs/deploy-contact-email.md index 23729f6..0fb87ba 100644 --- a/docs/deploy-contact-email.md +++ b/docs/deploy-contact-email.md @@ -397,9 +397,14 @@ Three things about this policy that are worth knowing: `ERROR` that will not be retried. A document in a retryable error state has `delivery.endTime: null` and is therefore *never* eligible for deletion, which is deliberate: the TTL must not reap a mail that is still deliverable. - The flip side is that a document stuck in retryable `ERROR` lives forever. - Those are worth a periodic look (`delivery.state == "ERROR"` and - `delivery.retryable == true`); there is no automatic sweeper. + The flip side used to be that a document stuck in retryable `ERROR` lived + forever, holding an address the TTL could not reach. + `scheduled-mail-retry.ts` is what closes that: every document it looks at + either gets sent or gets a terminal state, so nothing it can see stays + outside the TTL's reach. It sweeps two shapes — a retryable `ERROR`, and a + `PROCESSING` document whose lease has expired, which is what an instance + killed *inside* a send leaves behind. Both need an index; both are in + `firestore.indexes.json`. - **The retention window is seven days, by design.** `mail-delivery.ts` writes `delivery.expireAt = endTime + 7 days` on every terminal outcome (delivered or permanently failed), and the TTL policy above keys on it. @@ -434,13 +439,39 @@ when quota runs out: | Nature | **Realtime.** Someone is watching a form. | **Deferrable.** Still true an hour later. | | Delivered by | The request itself, synchronously | `onmailcreated`, then the sweeper | | Retried? | **Never** | Yes, `scheduledmailretry` | -| On failure | 503, vague message, cooldown cleared | Stays queued, swept later | +| On failure | 503, vague message, cooldown **kept** | Stays queued, swept later | + +**The cooldown is kept on failure, and that is deliberate.** It used to be +cleared, so that a researcher whose code never arrived was not stuck waiting a +minute for a code that does not exist. That is right about the researcher and +wrong about the endpoint: `contactEmailVerifications/{uid}.sentAt` is the only +server-side rate limit on a path that spends a real Resend request, and clearing +it removed the limit from exactly the case that needs one — a signed-in +researcher holding the button while sends fail. What is cleared now is the +`codeHash`, not the record: no code is left standing, the throttle is, and +`verify-contact-email.ts` answers such a record with "request a new code" +instead of spending one of the five attempts on it. **`systemStatus/mail` is the breaker.** `mail-delivery.ts` writes it after every -send: the daily quota reading on success, a shut breaker on -`daily_quota_exceeded`. It is server-only — no `firestore.rules` match, so it is -default-denied to every client, and the account page learns nothing from it -directly. +send: the daily quota reading on success, and a shut breaker on any failure that +is not this one message's problem. It is server-only — no `firestore.rules` +match, so it is default-denied to every client, and the account page learns +nothing from it directly. + +**Three reasons it shuts, and they last for different lengths of time:** + +| Cause | Shut until | Why that long | +|---|---|---| +| `daily_quota_exceeded` | next UTC midnight | the daily cap resets there (a guess — see below) | +| `monthly_quota_exceeded` | start of the next UTC month | the 3,000/month cap does **not** reset at midnight. Reopening nightly means probing into a cap with days left to run, and each probe spends one of a queued mail's three attempts — three nights and every queued notification is terminal | +| a revoked key, an unverified domain, missing config (`SYSTEMIC_ERRORS`) | 15 minutes | nothing resets; a human has to act. The pause exists only to stop a loop of failing sends, and to keep the realtime path from minting codes for mail that cannot go anywhere | + +`validation_error` is deliberately **not** in the systemic set: Resend uses it +both for an unverified sending domain and for one malformed recipient address, +and one researcher's typo must not switch verification off for everybody. + +**If you upgrade the plan mid-month**, clear `systemStatus/mail` by hand — the +monthly pause is a wait for a reset that has just stopped applying. **The proactive part is a response header.** Resend returns `x-resend-daily-quota` — the quota *used* today — on ordinary **successful** @@ -454,6 +485,27 @@ The header is documented as free-plan-only, so it disappears on a paid plan. Absent reads as "no daily cap applies", which is correct — the reserve logic turns itself off on upgrade instead of needing to be removed. +**Check what that number actually means before you trust it.** Resend documents +the header's existence, not its semantics, and "used today" and "remaining +today" are the same shape. If it is really the plan *limit*, every reading is +100, the reserve rule is true forever, and verification is off for good — and +because every send rewrites `dailyQuotaObservedAt`, the staleness escape hatch +never fires either. Capture one for yourself: + +``` +curl -sS -D - -o /dev/null -X POST https://api.resend.com/emails \ + -H "Authorization: Bearer $RESEND_API_KEY" \ + -H "Content-Type: application/json" \ + -d '{"from":"DataPipe ", + "to":["you@example.edu"],"subject":"quota header check","text":"."}' \ + | grep -i x-resend +``` + +Send it twice and watch which way the number moves, then compare it with the +Resend dashboard's usage for the day. Two guards stand in the meantime: a +reading outside `0..100` is refused at the write and ignored at the read, and a +refusal on the reserve alone logs at ERROR (§6). + **When does it reopen?** Resend publishes no daily-reset time, and nothing here depends on knowing one. `unavailableUntil` is set to the next UTC midnight as a *ceiling*; what actually reopens sending is the sweeper landing a successful @@ -487,15 +539,18 @@ Create log-based metrics on the functions' logs and alert on them: | `MailConfigMissingError` | **First.** Every notification the deployment sends is being dropped. | | `daily_quota_exceeded` | Sending has stopped. Reactive — it is already happening. | | `mail-availability` + `dailyQuotaUsed` above ~80 | **The useful one.** Leading indicator, from the success-response header, while there is still time to act. | +| `MailVerificationUnavailable` | Verification is being refused for everyone. On `quota-reserve` it is also the check on the header's meaning: if this fires all day while sending works, the reading is not what we think it is (§5). | +| `refusing an implausible x-resend-daily-quota` | The reading is out of range — the header is not a count of today's sends. | Route them to a channel Google delivers — Slack, PagerDuty, or an email address that is **not** on the `jspsych.org` sending domain, so an alert about mail being broken does not depend on mail working. -Worth watching alongside, though neither needs an alert: `scheduled-mail-retry` +Worth watching alongside, though none needs an alert: `scheduled-mail-retry` lines reporting a non-zero `agedOut` (notifications that expired undelivered), -and any accumulation of `delivery.state == "ERROR"` with `retryable == true`, -which is the sweeper's backlog. +any accumulation of `delivery.state == "ERROR"` with `retryable == true` (the +sweeper's backlog), and `MailSweepAbandoned` on a document, which means an +instance died inside a send — one is noise, a run of them is a platform problem. ## 7. Payload retention: the clock now measures the right thing @@ -523,11 +578,26 @@ meant to lose: | `retainUntil` in the future | retain — the researcher has not been told | | otherwise | delete — unchanged from before | -`retainUntil` is written by `scheduled-mail-retry.ts` while an upload-failure -notification is undelivered, and it covers **every unresolved entry for the -experiment**, not just the one that tripped the episode. It is written even -while the breaker is shut, because extending is a Firestore write that costs no -quota and an outage is exactly when the data is at risk. +`retainUntil` is written by `upload-retention.ts` — which owns this whole rule, +predicate and write together — and `scheduled-mail-retry.ts` is what calls it, +while an upload-failure notification is undelivered. It covers **every +unresolved entry for the experiment**, not just the one that tripped the +episode, and once per experiment per pass however many notifications name it. + +Two things about when it is written: + +- **Even while the breaker is shut.** Extending is a Firestore write that costs + no quota, and an outage is exactly when the data is at risk. +- **Including the pass that gives up on the notification.** Ageing a + notification out is the case where the researcher will never be told at all, + so it must not also be the moment their data quietly goes back on the + original clock. The 14-day ceiling is what bounds this. + +**The deletion sweep pages.** It looks at up to 500 aged entries to find its 50 +deletions, rather than taking the oldest 50 and stopping. Without that, fifty +retained entries at the head of the queue — one experiment behind a dead +provider — meant a pass that deleted nothing at all, for every other experiment +too, until the blockers crossed the 14-day ceiling up to a week later. **What deliberately does not extend:** an experiment whose owner has no contact email. `upload-failure-notify.ts` records `suppressedReason: "no-contact-email"` diff --git a/firestore.indexes.json b/firestore.indexes.json index e2f306c..06c9f01 100644 --- a/firestore.indexes.json +++ b/firestore.indexes.json @@ -81,6 +81,20 @@ "order": "ASCENDING" } ] + }, + { + "collectionGroup": "mail", + "queryScope": "COLLECTION", + "fields": [ + { + "fieldPath": "delivery.state", + "order": "ASCENDING" + }, + { + "fieldPath": "delivery.leaseExpiresAt", + "order": "ASCENDING" + } + ] } ], "fieldOverrides": [ diff --git a/functions/src/__tests__/mail-availability.test.js b/functions/src/__tests__/mail-availability.test.js index 15a3913..c847126 100644 --- a/functions/src/__tests__/mail-availability.test.js +++ b/functions/src/__tests__/mail-availability.test.js @@ -29,13 +29,19 @@ process.env.FIREBASE_CONFIG = JSON.stringify({ let verificationAvailability; let deliveryPaused; let nextUtcMidnight; +let nextUtcMonthStart; +let pauseUntil; +let usableQuotaReading; let isQuotaReadingStale; let VERIFICATION_CEILING; let FREE_PLAN_DAILY_LIMIT; +let SYSTEMIC_PAUSE_MS; let sweepDecision; +let pauseKindFor; let MAX_SWEEP_AGE_MS; let IDEMPOTENCY_WINDOW_MS; +let MAX_ATTEMPTS; let retentionDecision; @@ -44,15 +50,21 @@ beforeAll(async () => { verificationAvailability, deliveryPaused, nextUtcMidnight, + nextUtcMonthStart, + pauseUntil, + usableQuotaReading, isQuotaReadingStale, VERIFICATION_CEILING, FREE_PLAN_DAILY_LIMIT, + SYSTEMIC_PAUSE_MS, } = await import("../../lib/mail-availability.js")); ({ sweepDecision, MAX_SWEEP_AGE_MS, IDEMPOTENCY_WINDOW_MS } = await import( "../../lib/scheduled-mail-retry.js" )); + ({ pauseKindFor, MAX_ATTEMPTS } = await import("../../lib/mail-delivery.js")); + ({ retentionDecision } = await import("../../lib/upload-retention.js")); }); @@ -89,6 +101,88 @@ describe("nextUtcMidnight", () => { }); }); +describe("nextUtcMonthStart", () => { + test("is the start of the next UTC month, not thirty days out", () => { + expect(nextUtcMonthStart(NOON)).toBe(Date.UTC(2026, 8, 1)); + // The 20th of the month is the motivating case: the monthly cap is hit + // with eleven days still to run, and a daily reset would reopen the + // breaker that night. + expect(nextUtcMonthStart(Date.UTC(2026, 7, 20, 3, 0, 0))).toBe( + Date.UTC(2026, 8, 1) + ); + // December rolls the year. + expect(nextUtcMonthStart(Date.UTC(2026, 11, 31, 23, 59, 59))).toBe( + Date.UTC(2027, 0, 1) + ); + }); +}); + +describe("pauseUntil", () => { + test("a daily cap holds until midnight; a MONTHLY cap holds until the month turns", () => { + // The distinction the breaker used to lack. Treating a monthly exhaustion + // as a daily one reopened sending every midnight into a cap with days left + // to run -- and each probe spent one of a queued mail's three attempts, so + // three nights turned every queued notification terminal. + expect(pauseUntil("daily-quota", NOON)).toBe(MIDNIGHT_AFTER); + expect(pauseUntil("monthly-quota", NOON)).toBe(Date.UTC(2026, 8, 1)); + expect(pauseUntil("monthly-quota", NOON)).toBeGreaterThan( + pauseUntil("daily-quota", NOON) + ); + }); + + test("a systemic failure is a short cooldown, not a wait for a reset", () => { + // Nothing resets: a revoked key or an unverified domain needs a human. The + // pause is only there to stop a loop of failing sends, so it is minutes. + expect(pauseUntil("systemic", NOON)).toBe(NOON + SYSTEMIC_PAUSE_MS); + expect(pauseUntil("systemic", NOON)).toBeLessThan(MIDNIGHT_AFTER); + }); +}); + +describe("pauseKindFor", () => { + test("tells the two quota caps apart, and names the systemic failures", () => { + expect(pauseKindFor("daily_quota_exceeded")).toBe("daily-quota"); + expect(pauseKindFor("monthly_quota_exceeded")).toBe("monthly-quota"); + for (const name of [ + "suspended_api_key", + "missing_api_key", + "restricted_api_key", + "invalid_permission", + "MailConfigMissingError", + ]) { + expect(pauseKindFor(name)).toBe("systemic"); + } + }); + + test("does NOT shut the breaker for a per-message failure", () => { + // validation_error is Resend's name both for an unverified sending domain + // and for one malformed recipient address. One researcher's typo must not + // switch verification off for everybody, so the ambiguous name is left out + // and the unambiguous ones carry the rule. + expect(pauseKindFor("validation_error")).toBeNull(); + expect(pauseKindFor("rate_limit_exceeded")).toBeNull(); + expect(pauseKindFor("application_error")).toBeNull(); + expect(pauseKindFor("ECONNRESET")).toBeNull(); + }); +}); + +describe("usableQuotaReading", () => { + test("refuses a reading that cannot be a count of today's sends", () => { + // The bound exists because the header's MEANING is assumed, not documented. + // If x-resend-daily-quota turns out to be the monthly counter or a plan + // limit, an unbounded reading would sit above the reserve ceiling forever + // and hold verification shut with nothing in the logs saying why. + expect(usableQuotaReading(0)).toBe(0); + expect(usableQuotaReading(94)).toBe(94); + expect(usableQuotaReading(FREE_PLAN_DAILY_LIMIT)).toBe(FREE_PLAN_DAILY_LIMIT); + expect(usableQuotaReading(FREE_PLAN_DAILY_LIMIT + 1)).toBeNull(); + expect(usableQuotaReading(2900)).toBeNull(); + expect(usableQuotaReading(-1)).toBeNull(); + expect(usableQuotaReading("94")).toBeNull(); + expect(usableQuotaReading(Number.NaN)).toBeNull(); + expect(usableQuotaReading(undefined)).toBeNull(); + }); +}); + describe("isQuotaReadingStale", () => { test("a reading from earlier today is fresh", () => { expect(isQuotaReadingStale(NOON - 60_000, NOON)).toBe(false); @@ -199,6 +293,21 @@ describe("verificationAvailability", () => { }); }); +describe("verificationAvailability, on an implausible reading", () => { + test("an out-of-range reading does not hold verification shut", () => { + // A stored 2,900 is not "today's sends on a free plan" whatever else it + // is -- the monthly counter, say. Acting on it would refuse every + // researcher a code indefinitely, and because each send rewrites + // dailyQuotaObservedAt, the staleness escape hatch would never fire. + expect( + verificationAvailability( + { dailyQuotaUsed: 2900, dailyQuotaObservedAt: ts(NOON - 60_000) }, + NOON + ) + ).toEqual({ available: true }); + }); +}); + describe("deliveryPaused", () => { test("tracks unavailableUntil only, and ignores the verification reserve", () => { // The reserve exists to keep the realtime path off the last few sends. The @@ -263,38 +372,139 @@ describe("sweepDecision", () => { test("sweeps a 5xx only INSIDE the idempotency window", () => { // A 500 may or may not have been accepted. Inside the window the // Idempotency-Key makes a retry a no-op; outside it, the retry is a coin - // flip on a second copy, so we decline. + // flip on a second copy, so we give up rather than gamble. const inside = doc({ error: { name: "application_error", message: "boom" }, + startTime: ts(NOON - 60 * 60 * 1000), lastAttemptAt: ts(NOON - 60 * 60 * 1000), }); expect(sweepDecision(inside, NOON)).toBe("deliver"); const outside = doc({ error: { name: "application_error", message: "boom" }, + startTime: ts(NOON - IDEMPOTENCY_WINDOW_MS - 60_000), lastAttemptAt: ts(NOON - IDEMPOTENCY_WINDOW_MS - 60_000), }); - expect(sweepDecision(outside, NOON)).toBe("skip"); + // Terminal, not skipped: it can never become deliverable again -- the key + // has expired and only gets older -- so leaving it "retryable" would keep + // expireAt off the document and hold the address forever, while occupying + // a slot in every future pass. + expect(sweepDecision(outside, NOON)).toBe("age-out"); }); - test("NEVER sweeps inline mail, whatever the error says", () => { + test("measures the idempotency window from startTime, NOT from the last attempt", () => { + // THE BUG THIS PINS. The Idempotency-Key is the document id, and Resend + // expires it 24 hours after its FIRST use. Measuring the window from + // lastAttemptAt slides it forward with every retry: a mail first attempted + // at T0 and retried at T0+20h is "20 hours old" at T0+40h and would be + // sent again on a key that expired at T0+24h -- which Resend treats as a + // new message, and the researcher gets a second copy. + const retriedTwice = doc({ + error: { name: "application_error", message: "boom" }, + startTime: ts(NOON - 40 * 60 * 60 * 1000), + lastAttemptAt: ts(NOON - 20 * 60 * 60 * 1000), + }); + expect(sweepDecision(retriedTwice, NOON)).toBe("age-out"); + + // ...and the same document while the key is genuinely still live. + const stillInside = doc({ + error: { name: "application_error", message: "boom" }, + startTime: ts(NOON - 20 * 60 * 60 * 1000), + lastAttemptAt: ts(NOON - 60 * 60 * 1000), + }); + expect(sweepDecision(stillInside, NOON)).toBe("deliver"); + }); + + test("NEVER sends inline mail, whatever the error says -- it ends it instead", () => { // A verification code is realtime. Delivering one an hour late is not a // late success -- it may already have expired, and the researcher has long - // since given up or requested another. + // since given up or requested another. Terminal rather than skipped: a + // skipped document keeps `retryable: true`, never gets expireAt, and holds + // its recipient's address outside the TTL's reach for good. expect( sweepDecision( doc( { error: { name: "daily_quota_exceeded", message: "quota" }, + startTime: ts(NOON - 60_000), lastAttemptAt: ts(NOON - 60_000), }, { datapipe: { kind: "contact-email-verification", deliverInline: true } } ), NOON ) + ).toBe("age-out"); + }); + + test("recovers a claim that was abandoned mid-send", () => { + // deliverMailDocument's claim writes PROCESSING with `retryable: null` + // BEFORE the send, so an instance killed inside the send leaves a document + // that the retryable-ERROR query cannot see and the TTL cannot reap. An + // expired lease is the proof the claimant is dead. + const stranded = { + to: ["researcher@example.edu"], + datapipe: { kind: "upload-failure", owner: "uid-1" }, + delivery: { + state: "PROCESSING", + retryable: null, + attempts: 1, + startTime: ts(NOON - 30 * 60 * 1000), + leaseExpiresAt: ts(NOON - 60_000), + }, + }; + expect(sweepDecision(stranded, NOON)).toBe("deliver"); + + // A LIVE lease is the one case that is genuinely somebody else's: another + // invocation may be inside the send right now. + expect( + sweepDecision( + { + ...stranded, + delivery: { ...stranded.delivery, leaseExpiresAt: ts(NOON + 60_000) }, + }, + NOON + ) ).toBe("skip"); }); + test("ends a stranded claim rather than resending it once the key has expired", () => { + // Same document, a day and a half later. Nothing knows whether the send + // went out, and the Idempotency-Key that would have made a retry safe is + // gone -- so this is exactly the coin flip the sweeper declines. + expect( + sweepDecision( + { + to: ["researcher@example.edu"], + datapipe: { kind: "upload-failure", owner: "uid-1" }, + delivery: { + state: "PROCESSING", + attempts: 1, + startTime: ts(NOON - IDEMPOTENCY_WINDOW_MS - 60_000), + leaseExpiresAt: ts(NOON - IDEMPOTENCY_WINDOW_MS), + }, + }, + NOON + ) + ).toBe("age-out"); + }); + + test("ends a document that has spent its whole attempt budget", () => { + // Otherwise deliverMailDocument answers "skipped-attempts-exhausted" on + // every pass forever, and the document sits in the query eating the budget + // a deliverable notification needed. + expect( + sweepDecision( + doc({ + error: { name: "daily_quota_exceeded", message: "quota" }, + attempts: MAX_ATTEMPTS, + startTime: ts(NOON - 60_000), + lastAttemptAt: ts(NOON - 60_000), + }), + NOON + ) + ).toBe("age-out"); + }); + test("ages out anything past the age bound, rather than skipping it", () => { // Terminal, not skip, and the difference matters: a retryable ERROR never // gets delivery.expireAt, so skipping would leave the document holding a @@ -323,6 +533,20 @@ describe("sweepDecision", () => { ).toBe("age-out"); }); + test("a document with NO usable clock ages out rather than living forever", () => { + // An older deploy's write, a hand edit during an incident, a partially + // applied update. With neither timestamp there is no age at which it would + // ever cross the age bound, so a "skip" here is permanent: no expireAt is + // ever written, the TTL never reaps it, and it holds a researcher's address + // for good -- the precise hole ageing out exists to close. + expect( + sweepDecision( + doc({ error: { name: "application_error", message: "boom" } }), + NOON + ) + ).toBe("age-out"); + }); + test("ignores anything that is not a live retryable error", () => { expect(sweepDecision(doc({ state: "SUCCESS", retryable: false }), NOON)).toBe( "skip" @@ -331,11 +555,29 @@ describe("sweepDecision", () => { expect(sweepDecision({ to: ["x@example.edu"] }, NOON)).toBe("skip"); }); - test("skips an error with no usable name rather than guessing", () => { + test("treats an error with no usable name as ambiguous, not as unswept work", () => { + // Not knowing what happened is the definition of ambiguous, and ambiguity + // is what the Idempotency-Key resolves: inside the window a retry is a + // no-op at Resend, so it is safe; outside it, this becomes terminal like + // every other ambiguity. What it must never be is a permanent skip -- that + // is a document nothing ever writes again, in a query nothing ever + // finishes. expect( - sweepDecision(doc({ error: {}, lastAttemptAt: ts(NOON - 1000) }), NOON) - ).toBe("skip"); - expect(sweepDecision(doc({ lastAttemptAt: ts(NOON - 1000) }), NOON)).toBe("skip"); + sweepDecision( + doc({ error: {}, startTime: ts(NOON - 1000), lastAttemptAt: ts(NOON - 1000) }), + NOON + ) + ).toBe("deliver"); + expect( + sweepDecision( + doc({ + error: {}, + startTime: ts(NOON - IDEMPOTENCY_WINDOW_MS - 1000), + lastAttemptAt: ts(NOON - IDEMPOTENCY_WINDOW_MS - 1000), + }), + NOON + ) + ).toBe("age-out"); }); }); diff --git a/functions/src/__tests__/mail-delivery.test.js b/functions/src/__tests__/mail-delivery.test.js index 369fa90..a9950a1 100644 --- a/functions/src/__tests__/mail-delivery.test.js +++ b/functions/src/__tests__/mail-delivery.test.js @@ -41,6 +41,8 @@ let LEASE_MS; let MAX_ATTEMPTS; let CONFIG_MISSING_ERROR; let INVALID_DOCUMENT_ERROR; +let quotaFromHeaders; +let resendSender; beforeAll(async () => { ({ @@ -54,6 +56,8 @@ beforeAll(async () => { MAX_ATTEMPTS, CONFIG_MISSING_ERROR, INVALID_DOCUMENT_ERROR, + quotaFromHeaders, + resendSender, } = await import("../../lib/mail-delivery.js")); }); @@ -474,3 +478,115 @@ describe("configuration", () => { expect(classifyMailError(error).retryable).toBe(false); }); }); + +// --------------------------------------------------------------------------- +// 5. The transport, and the two header names the breaker depends on +// --------------------------------------------------------------------------- +// +// WHY THIS IS WORTH ITS OWN SECTION. Everything else in the mail suite mocks +// the sender, which means it hands the delivery path an ALREADY-PARSED +// `dailyQuotaUsed` -- the exact value the transport is supposed to produce. So +// the header names, their casing on a real Headers object, and what +// Number.parseInt does to a value that is not a bare integer were the one part +// of the proactive breaker that no test touched: misspell "x-resend-daily-quota" +// and every send returns no reading, mail-availability.ts never learns a +// number, verification never stops at the reserve, and every other test still +// passes. + +describe("quotaFromHeaders", () => { + test("reads both quota headers, case-insensitively, off a real Headers", () => { + // Headers lower-cases its keys, which is why a real one is used here rather + // than a plain object with a get(). + const headers = new Headers({ + "X-Resend-Daily-Quota": "94", + "x-resend-monthly-quota": "2412", + }); + expect(quotaFromHeaders(headers)).toEqual({ + dailyQuotaUsed: 94, + monthlyQuotaUsed: 2412, + }); + }); + + test("absent headers leave the field out, rather than reporting zero", () => { + // Resend sends the daily header to free-plan accounts only, so absent has + // to read as "no daily cap applies". A 0 here would be a permanent claim + // that nothing has been sent today. + expect(quotaFromHeaders(new Headers())).toEqual({}); + expect(quotaFromHeaders(new Headers({ "x-resend-daily-quota": "0" }))).toEqual({ + dailyQuotaUsed: 0, + }); + }); + + test("an unparseable value is no reading at all", () => { + expect(quotaFromHeaders(new Headers({ "x-resend-daily-quota": "" }))).toEqual({}); + expect( + quotaFromHeaders(new Headers({ "x-resend-daily-quota": "unlimited" })) + ).toEqual({}); + }); +}); + +describe("resendSender", () => { + const okResponse = (body, headers = {}) => + new Response(JSON.stringify(body), { status: 200, headers }); + + afterEach(() => { + jest.restoreAllMocks(); + }); + + test("sends the id and the idempotency key, and returns the quota reading", async () => { + const fetchMock = jest + .spyOn(globalThis, "fetch") + .mockResolvedValue( + okResponse({ id: "resend-1" }, { "x-resend-daily-quota": "94" }) + ); + + const result = await resendSender(CONFIG)( + { from: CONFIG.from, to: ["researcher@example.edu"], subject: "s", text: "t" }, + { timeoutMs: 1000, idempotencyKey: "mail-doc-1" } + ); + + expect(result).toEqual({ id: "resend-1", dailyQuotaUsed: 94 }); + + const [url, init] = fetchMock.mock.calls[0]; + expect(url).toBe("https://api.resend.com/emails"); + // The document id as the Idempotency-Key is what makes an ambiguous + // timeout safe to retry at all (see AMBIGUOUS_ERRORS). + expect(init.headers["Idempotency-Key"]).toBe("mail-doc-1"); + expect(init.headers.Authorization).toBe(`Bearer ${CONFIG.apiKey}`); + }); + + test("a 2xx with an unreadable body is still a send, and still a reading", async () => { + // Losing the message id costs an audit trail, not a delivery -- and the + // headers are read before the body precisely so a bad body cannot cost the + // quota reading too. + jest.spyOn(globalThis, "fetch").mockResolvedValue( + new Response("not json", { + status: 200, + headers: { "x-resend-daily-quota": "12" }, + }) + ); + + await expect( + resendSender(CONFIG)( + { from: CONFIG.from, to: ["researcher@example.edu"], subject: "s", text: "t" }, + { timeoutMs: 1000, idempotencyKey: "mail-doc-2" } + ) + ).resolves.toEqual({ dailyQuotaUsed: 12 }); + }); + + test("a refusal throws with Resend's own name, so the taxonomy can classify it", async () => { + jest.spyOn(globalThis, "fetch").mockResolvedValue( + new Response( + JSON.stringify({ name: "daily_quota_exceeded", message: "over quota" }), + { status: 429 } + ) + ); + + await expect( + resendSender(CONFIG)( + { from: CONFIG.from, to: ["researcher@example.edu"], subject: "s", text: "t" }, + { timeoutMs: 1000, idempotencyKey: "mail-doc-3" } + ) + ).rejects.toMatchObject({ name: "daily_quota_exceeded", status: 429 }); + }); +}); diff --git a/functions/src/__tests__/mail-retry-emulator.test.js b/functions/src/__tests__/mail-retry-emulator.test.js index 27182f0..846c56e 100644 --- a/functions/src/__tests__/mail-retry-emulator.test.js +++ b/functions/src/__tests__/mail-retry-emulator.test.js @@ -10,19 +10,28 @@ // functions/lib/, and scoped cleanup via a registrar of created refs rather // than a collection-wide wipe. // -// TWO SEAMS ARE INJECTED HERE, AND THE SECOND ONE IS NOT OPTIONAL. +// THREE SEAMS ARE INJECTED HERE, AND ONLY THE FIRST IS ABOUT CONVENIENCE. // -// _setMailSenderForTests the transport, so nothing reaches api.resend.com. -// _setMailStatusDocForTests the breaker document. +// _setMailSenderForTests the transport, so nothing reaches api.resend.com. +// _setMailStatusDocForTests the breaker document. +// _setMailCollectionForTests the mail collection. // -// The breaker lives at `systemStatus/mail`, which is a SINGLETON shared by -// every suite. Half the assertions below require the breaker to be SHUT, and a -// shut breaker makes every other suite that sends mail fail -- including -// contact-email-verify-emulator.test.js, which drives the real deployed -// endpoint over HTTP and would start getting 503s. Under `--maxWorkers=2` that -// is a suite failing in a different place each run, which is the worst kind of -// bug to chase. Pointing this suite at its own status document is what prevents -// it, and it is why the seam exists in production code at all. +// The last two are about the same hazard, which is SHARED SINGLETONS in an +// emulator that suites run against in parallel. +// +// The breaker lives at `systemStatus/mail`. Half the assertions below require +// it to be SHUT, and a shut breaker makes every other suite that sends mail +// fail -- including contact-email-verify-emulator.test.js, which drives the +// real deployed endpoint over HTTP and would start getting 503s. +// +// The `mail` COLLECTION is the other one, and it took a review to notice. +// sweepRetryableMail is a QUERY over the whole collection, so under +// `--maxWorkers=2` this suite would pick up mail-delivery-emulator.test.js's +// fixtures, deliver them through THIS suite's injected transport, and rewrite +// them mid-assertion -- while its own exact-count assertions (`retained` is 3, +// `agedOut` is 1, the sender was called once) failed for reasons that had +// nothing to do with the code under test. Both are suites failing in a +// different place each run, which is the worst kind of bug to chase. process.env.FIRESTORE_EMULATOR_HOST = "localhost:8080"; process.env.GCLOUD_PROJECT = "datapipe-test"; @@ -32,6 +41,11 @@ process.env.FIREBASE_CONFIG = JSON.stringify({ storageBucket: "datapipe-test.appspot.com", }); +// cleanupOldEntries deletes the payload before the queue entry, so the sweep +// has to have somewhere to send that delete. 404 from the emulator is fine -- +// the delete is wrapped precisely because the object may already be gone. +process.env.FIREBASE_STORAGE_EMULATOR_HOST = "localhost:9199"; + process.env.RESEND_API_KEY = "re_test_not_a_real_key"; process.env.MAIL_FROM = "DataPipe (test) "; @@ -39,6 +53,16 @@ import { initializeApp, getApp } from "firebase-admin/app"; import { getFirestore, Timestamp } from "firebase-admin/firestore"; import { randomUUID } from "crypto"; +// Block 4 imports the compiled scheduled-upload-retry.js, which pulls in every +// provider adapter, each importing ESM-only "node-fetch" at module scope -- +// which Jest's CJS transform cannot parse. Stubbed exactly as +// payload-encryption-emulator.test.js and upload-queue.test.js do. Nothing here +// reaches a provider; the deletion sweep only touches Firestore and Storage. +jest.mock("node-fetch", () => ({ + __esModule: true, + default: jest.fn(), +})); + jest.setTimeout(30000); let db; @@ -46,11 +70,17 @@ let deliverMailDocument; let _setMailSenderForTests; let sweepRetryableMail; let _setMailStatusDocForTests; +let _setMailCollectionForTests; +let cleanupOldEntries; let MAX_SWEEP_AGE_MS; let MAIL_RETENTION_MS; +let IDEMPOTENCY_WINDOW_MS; +let SWEEP_ABANDONED_ERROR; +let RETENTION_GRACE_MS; -// This suite's private breaker document. +// This suite's private breaker document, and its private mail collection. const STATUS_DOC_ID = `mail-test-${randomUUID()}`; +const MAIL_COLLECTION_ID = `mail-test-${randomUUID()}`; beforeAll(async () => { let app; @@ -64,16 +94,24 @@ beforeAll(async () => { ({ deliverMailDocument, _setMailSenderForTests, MAIL_RETENTION_MS } = await import( "../../lib/mail-delivery.js" )); - ({ sweepRetryableMail, MAX_SWEEP_AGE_MS } = await import( - "../../lib/scheduled-mail-retry.js" - )); + ({ + sweepRetryableMail, + MAX_SWEEP_AGE_MS, + IDEMPOTENCY_WINDOW_MS, + SWEEP_ABANDONED_ERROR, + } = await import("../../lib/scheduled-mail-retry.js")); ({ _setMailStatusDocForTests } = await import("../../lib/mail-availability.js")); + ({ _setMailCollectionForTests } = await import("../../lib/mail.js")); + ({ cleanupOldEntries } = await import("../../lib/scheduled-upload-retry.js")); + ({ RETENTION_GRACE_MS } = await import("../../lib/upload-retention.js")); _setMailStatusDocForTests(STATUS_DOC_ID); + _setMailCollectionForTests(MAIL_COLLECTION_ID); }); afterAll(async () => { _setMailStatusDocForTests(null); + _setMailCollectionForTests(null); await db.collection("systemStatus").doc(STATUS_DOC_ID).delete().catch(() => {}); }); @@ -81,7 +119,7 @@ const created = []; const RECIPIENT = "researcher@example.edu"; async function seedMail({ delivery, inline = false, kind = "upload-failure" } = {}) { - const ref = db.collection("mail").doc(); + const ref = db.collection(MAIL_COLLECTION_ID).doc(); created.push(ref); await ref.set({ to: [RECIPIENT], @@ -102,17 +140,27 @@ async function seedMail({ delivery, inline = false, kind = "upload-failure" } = return { ref, id: ref.id }; } -async function seedQueueEntry(experimentID, { status = "failed", ageMs = 8 * 24 * 60 * 60 * 1000 } = {}) { +// One owner for every queue entry this suite creates. uploadQueue is shared +// with every other suite, and cleanupOldEntries below sweeps it BY AGE with no +// other filter -- so the deletion tests are scoped to this owner the same way +// retryPendingUploads' ownerScope seam scopes the retry tests. +const OWNER_ID = `mr-user-${randomUUID()}`; + +async function seedQueueEntry( + experimentID, + { status = "failed", ageMs = 8 * 24 * 60 * 60 * 1000, ...rest } = {} +) { const ref = db.collection("uploadQueue").doc(); created.push(ref); await ref.set({ experimentID, - owner: `mr-user-${randomUUID()}`, + owner: OWNER_ID, status, retryCount: 5, maxRetries: 5, storagePath: `pending-data/${experimentID}/subject-1.json`, createdAt: Timestamp.fromMillis(Date.now() - ageMs), + ...rest, }); return ref; } @@ -197,6 +245,60 @@ describe("recording quota state", () => { expect(status.dailyQuotaUsed).toBe(100); }); + test("a MONTHLY cap shuts sending until the month turns, not until midnight", async () => { + // The two free-plan caps reset on different clocks. Treated as a daily + // one, a monthly exhaustion reopens the breaker at midnight, the sweeper + // probes into a cap with days left to run, and each probe spends one of a + // queued mail's three attempts -- three nights and every queued + // notification is terminal. + _setMailSenderForTests(sendingError("monthly_quota_exceeded", { status: 429 })); + const { id } = await seedMail(); + + expect(await deliverMailDocument(id)).toBe("retryable-error"); + + const status = (await statusRef().get()).data(); + expect(status.reason).toBe("monthly_quota_exceeded"); + const nextMidnight = Date.UTC( + new Date().getUTCFullYear(), + new Date().getUTCMonth(), + new Date().getUTCDate() + 1 + ); + expect(status.unavailableUntil.toMillis()).toBeGreaterThanOrEqual(nextMidnight); + // ...and the DAILY counter is left alone. A monthly cap says nothing about + // today's sends, so pinning it at 100 would go on blocking verification on + // the reserve rule after the daily counter had reset. + expect(status.dailyQuotaUsed).toBeUndefined(); + }); + + test("a revoked key shuts the breaker too, briefly -- quota is not the only way to be unable to send", async () => { + // Without this the breaker only ever shut on quota, so a revoked key left + // verificationAvailability answering "available" indefinitely: every click + // minted a code, wrote a mail document and spent a real Resend request, and + // nothing anywhere counted them. + _setMailSenderForTests(sendingError("suspended_api_key", { status: 403 })); + const { id } = await seedMail(); + + expect(await deliverMailDocument(id)).toBe("terminal-error"); + + const status = (await statusRef().get()).data(); + expect(status.reason).toBe("suspended_api_key"); + expect(status.unavailableUntil.toMillis()).toBeGreaterThan(Date.now()); + // Minutes, not "until the quota resets": nothing resets, so the pause is + // only there to stop a loop of failing sends until a human sees the logs. + expect(status.unavailableUntil.toMillis()).toBeLessThan(Date.now() + 60 * 60 * 1000); + }); + + test("a per-message failure does NOT shut sending for everybody", async () => { + // validation_error is Resend's name both for an unverified sending domain + // and for one malformed recipient address. One researcher's typo must not + // switch verification off for every other researcher. + _setMailSenderForTests(sendingError("validation_error", { status: 422 })); + const { id } = await seedMail(); + + expect(await deliverMailDocument(id)).toBe("terminal-error"); + expect((await statusRef().get()).exists).toBe(false); + }); + test("a later success reopens the breaker -- this is the half-open close", async () => { _setMailSenderForTests(sendingError("daily_quota_exceeded", { status: 429 })); const first = await seedMail(); @@ -315,11 +417,13 @@ describe("sweepRetryableMail", () => { expect((await statusRef().get()).data().unavailableUntil).toBeNull(); }); - test("never sweeps inline mail, even when it somehow looks retryable", async () => { + test("never SENDS inline mail -- it ends it, so the TTL can have the address", async () => { // Belt and braces: inline failures are already marked terminal, so this // document should not exist. If one ever does -- a hand edit, an older // deploy's write -- the sweeper still must not resurrect a verification - // code that expired hours ago. + // code that expired hours ago. Ending it rather than skipping it is what + // stops it sitting in the query forever with no expireAt, holding a + // researcher's address outside the TTL policy's reach. const send = sendingOk(); _setMailSenderForTests(send); const { ref } = await seedMail({ @@ -331,8 +435,91 @@ describe("sweepRetryableMail", () => { const report = await sweepRetryableMail(); expect(send).not.toHaveBeenCalled(); - expect(report.skipped).toBeGreaterThanOrEqual(1); - expect((await deliveryOf(ref)).state).toBe("ERROR"); + expect(report.agedOut).toBe(1); + const delivery = await deliveryOf(ref); + expect(delivery.state).toBe("ERROR"); + expect(delivery.retryable).toBe(false); + expect(delivery.expireAt.toMillis()).toBeGreaterThan(Date.now()); + }); + + test("recovers a claim that was abandoned mid-send", async () => { + // THE HOLE THIS CLOSES. The claim transaction rewrites the document to + // PROCESSING with `retryable: null` BEFORE the send, so an instance + // preempted (or rolled by a deploy) inside the send left a document that + // the retryable-ERROR query could not see and the TTL could not reap -- + // holding a researcher's address indefinitely, which is the exact failure + // the sweeper exists to prevent. + const send = sendingOk("resend-recovered", { dailyQuotaUsed: 7 }); + _setMailSenderForTests(send); + const at = Timestamp.fromMillis(Date.now() - 30 * 60 * 1000); + const { ref } = await seedMail({ + delivery: { + state: "PROCESSING", + attempts: 1, + retryable: null, + startTime: at, + // Expired: LEASE_MS is five minutes, so its claimant is provably dead. + leaseExpiresAt: Timestamp.fromMillis(Date.now() - 60_000), + endTime: null, + }, + }); + + const report = await sweepRetryableMail(); + + expect(report.delivered).toBe(1); + const delivery = await deliveryOf(ref); + expect(delivery.state).toBe("SUCCESS"); + expect(delivery.attempts).toBe(2); + }); + + test("leaves a claim alone while its lease is still live", async () => { + // The other half: an invocation may be inside the send right now, and two + // senders on one document is the double-send the whole claim machinery + // exists to prevent. + const send = sendingOk(); + _setMailSenderForTests(send); + const { ref } = await seedMail({ + delivery: { + state: "PROCESSING", + attempts: 1, + startTime: Timestamp.now(), + leaseExpiresAt: Timestamp.fromMillis(Date.now() + 4 * 60 * 1000), + }, + }); + + const report = await sweepRetryableMail(); + + expect(send).not.toHaveBeenCalled(); + expect(report.agedOut).toBe(0); + expect((await deliveryOf(ref)).state).toBe("PROCESSING"); + }); + + test("ends an abandoned claim once the idempotency key has expired", async () => { + // Nothing knows whether the send went out, and the key that would have made + // a retry safe is gone -- so this is the coin flip the sweeper declines. + // Terminal, and recorded as such, rather than left as a PROCESSING document + // that the next pass would look at and leave again. + const send = sendingOk(); + _setMailSenderForTests(send); + const old = Timestamp.fromMillis(Date.now() - IDEMPOTENCY_WINDOW_MS - 60_000); + const { ref } = await seedMail({ + delivery: { + state: "PROCESSING", + attempts: 1, + startTime: old, + leaseExpiresAt: old, + }, + }); + + const report = await sweepRetryableMail(); + + expect(send).not.toHaveBeenCalled(); + expect(report.agedOut).toBe(1); + const delivery = await deliveryOf(ref); + expect(delivery.state).toBe("ERROR"); + expect(delivery.retryable).toBe(false); + expect(delivery.error.name).toBe(SWEEP_ABANDONED_ERROR); + expect(delivery.expireAt.toMillis()).toBeGreaterThan(Date.now()); }); test("ages out a mail nobody delivered in time, and lets the TTL have it", async () => { @@ -426,6 +613,82 @@ describe("sweepRetryableMail", () => { } }); + test("holds the data back on the pass that GIVES UP on the notification", async () => { + // The case that matters most, and the one this used to miss. Retention was + // only extended for mail the sweeper was about to try again, so the moment + // a notification became undeliverable the extensions stopped -- and the + // data it was about went back on the original clock and was deleted, with + // the researcher never having been told anything at all. Giving up on + // telling them is not a reason to shorten their window; it is the reason + // they need it. + _setMailSenderForTests(sendingOk()); + const experimentID = `mr-exp-${randomUUID()}`; + const entry = await seedQueueEntry(experimentID); + const { ref } = await seedMail({ + delivery: failedDelivery("daily_quota_exceeded", MAX_SWEEP_AGE_MS + 60_000), + }); + await ref.update({ "datapipe.experimentID": experimentID }); + + const report = await sweepRetryableMail(); + + expect(report.agedOut).toBe(1); + expect(report.retained).toBe(1); + expect((await entry.get()).data().retainUntil.toMillis()).toBeGreaterThan( + Date.now() + ); + }); + + test("extends an experiment ONCE a pass, however many notifications name it", async () => { + // Two failure episodes for one experiment are two mail documents. Running + // the query and the batch twice would double the writes and count the same + // entries twice in the report -- and the sweep runs every ten minutes for + // as long as the outage lasts, so "twice" is really "tens of thousands of + // redundant writes a day" against a 20,000/day free tier. + _setMailSenderForTests(sendingOk()); + const experimentID = `mr-exp-${randomUUID()}`; + const entry = await seedQueueEntry(experimentID); + for (const _ of [1, 2]) { + const { ref } = await seedMail({ + delivery: failedDelivery("daily_quota_exceeded"), + }); + await ref.update({ "datapipe.experimentID": experimentID }); + } + + const report = await sweepRetryableMail(); + + expect(report.scanned).toBe(2); + expect(report.retained).toBe(1); + expect((await entry.get()).data().retainUntil.toMillis()).toBeGreaterThan( + Date.now() + ); + }); + + test("does not rewrite a retainUntil that is already most of the way out", async () => { + // The same reasoning one level down: a pass every ten minutes must not + // rewrite the same field to nearly the same value each time. The stored + // value can never be closer than half the grace window to expiring, which + // is days of slack on a ten-minute sweep. + _setMailSenderForTests(sendingOk()); + const experimentID = `mr-exp-${randomUUID()}`; + const alreadyExtended = Timestamp.fromMillis( + Date.now() + RETENTION_GRACE_MS - 60_000 + ); + const entry = await seedQueueEntry(experimentID, { + retainUntil: alreadyExtended, + }); + const { ref } = await seedMail({ + delivery: failedDelivery("daily_quota_exceeded"), + }); + await ref.update({ "datapipe.experimentID": experimentID }); + + // Still reported as held back -- it IS held back; it just did not need a + // write to stay that way. + expect((await sweepRetryableMail()).retained).toBe(1); + expect((await entry.get()).data().retainUntil.toMillis()).toBe( + alreadyExtended.toMillis() + ); + }); + test("holds nothing back for a verification code -- there is no data behind it", async () => { _setMailSenderForTests(sendingOk()); const experimentID = `mr-exp-${randomUUID()}`; @@ -453,3 +716,50 @@ describe("sweepRetryableMail", () => { expect(report.agedOut).toBe(0); }); }); + +// --------------------------------------------------------------------------- +// 4. The other end of the story: the sweep that actually deletes +// --------------------------------------------------------------------------- + +describe("cleanupOldEntries", () => { + test("deletes past a queue whose head is all retained entries", async () => { + // THE STARVATION THIS FIXES. The query finds entries by age, ascending, + // and a retained entry is not removed from the result set by being looked + // at -- so with a single limit(50) on the query, fifty retained entries at + // the head of the queue meant a pass that deleted nothing, forever. One + // experiment stuck behind a dead storage provider could stop every OTHER + // experiment's payloads from ever being deleted, until the blockers finally + // crossed the 14-day ceiling up to a week later. + const blocked = `mr-exp-${randomUUID()}`; + const deletable = `mr-exp-${randomUUID()}`; + + // 55 older entries that must be kept: still pending, with retries left. + const batch = db.batch(); + for (let i = 0; i < 55; i += 1) { + const ref = db.collection("uploadQueue").doc(); + created.push(ref); + batch.set(ref, { + experimentID: blocked, + owner: OWNER_ID, + status: "pending", + retryCount: 0, + maxRetries: 5, + storagePath: `pending-data/${blocked}/subject-${i}.json`, + createdAt: Timestamp.fromMillis(Date.now() - 10 * 24 * 60 * 60 * 1000), + }); + } + await batch.commit(); + + // ...and behind them, younger but still aged out, five that may go. + const doomed = []; + for (let i = 0; i < 5; i += 1) { + doomed.push(await seedQueueEntry(deletable)); + } + + await cleanupOldEntries(OWNER_ID); + + for (const entry of doomed) { + expect((await entry.get()).exists).toBe(false); + } + }); +}); diff --git a/functions/src/mail-availability.ts b/functions/src/mail-availability.ts index f917b36..6fd92af 100644 --- a/functions/src/mail-availability.ts +++ b/functions/src/mail-availability.ts @@ -41,6 +41,23 @@ // absence therefore means the daily cap does not apply, which is exactly right: // on a paid plan this module quietly stops tripping instead of needing to be // removed. +// +// THAT READING IS AN ASSUMPTION, AND IT IS LOAD-BEARING. Resend documents the +// header's existence, not its semantics, and "used today" and "remaining today" +// are the same shape. If it is really the plan LIMIT, every reading is 100, the +// reserve rule below is true forever, and verification is off for good -- and +// because each send rewrites dailyQuotaObservedAt, the staleness escape hatch +// never fires either. Three things bound that: +// +// 1. A reading outside [0, FREE_PLAN_DAILY_LIMIT] is refused at the write +// (recordSendOutcome) and ignored at the read. That catches a header that +// turns out to be the monthly counter, or a remaining-quota value on a +// paid plan. +// 2. Refusing a verification on the reserve alone logs at ERROR with a stable +// token, so the condition is alertable rather than silent +// (docs/deploy-contact-email.md §6). +// 3. The runbook says how to capture a real response header and check the +// number against the Resend dashboard (§5). Do that before trusting it. import { Timestamp } from "firebase-admin/firestore"; import { db } from "./app.js"; @@ -102,6 +119,68 @@ export function nextUtcMidnight(nowMs: number): number { return Date.UTC(d.getUTCFullYear(), d.getUTCMonth(), d.getUTCDate() + 1); } +/** + * Start of the UTC month after the given instant. + * + * The free plan has TWO caps -- 100/day and 3,000/month -- and they reset on + * different clocks. Reusing the daily reset for a monthly exhaustion is not a + * small error: hit the monthly cap on the 20th and the breaker reopens at + * midnight, the sweeper probes into a cap that has eleven days left to run, + * fails, and spends one of each queued document's MAX_ATTEMPTS doing it. Three + * nights of that and every queued notification is terminal -- which is the + * exact retry-budget exhaustion the breaker exists to prevent. + * + * Same guess-status as nextUtcMidnight, and the same escape hatch: an operator + * who upgrades the plan mid-month clears `systemStatus/mail` by hand rather + * than waiting this out (docs/deploy-contact-email.md §5). + */ +export function nextUtcMonthStart(nowMs: number): number { + const d = new Date(nowMs); + return Date.UTC(d.getUTCFullYear(), d.getUTCMonth() + 1, 1); +} + +// Why sending stopped, and therefore how long it stays stopped. +// +// daily-quota resets at the next UTC midnight. +// monthly-quota resets at the start of the next UTC month. +// systemic nothing about the account is exhausted; the deployment +// cannot send at all (a revoked key, an unverified domain, +// missing configuration). No reset time exists, so this is a +// short cooldown and nothing more -- see SYSTEMIC_PAUSE_MS. +export type PauseKind = "daily-quota" | "monthly-quota" | "systemic"; + +// How long a systemic failure holds sending shut. +// +// Short on purpose. A systemic failure needs a human, and this cannot wait for +// one -- but it must not let a loop of failing sends run either. Fifteen +// minutes bounds a revoked key to ~4 wasted Resend requests an hour no matter +// how many researchers press the button, and it is long enough that the +// sweeper (every 10 minutes) skips at most two passes if the diagnosis was +// wrong. +export const SYSTEMIC_PAUSE_MS = 15 * 60 * 1000; + +/** When may sending be tried again, given why it stopped? */ +export function pauseUntil(kind: PauseKind, nowMs: number): number { + if (kind === "daily-quota") return nextUtcMidnight(nowMs); + if (kind === "monthly-quota") return nextUtcMonthStart(nowMs); + return nowMs + SYSTEMIC_PAUSE_MS; +} + +/** + * A stored daily-quota reading, or null if there isn't a usable one. + * + * The bound is the point. `dailyQuotaUsed` comes from a response header whose + * semantics are assumed rather than documented (see the header), and a reading + * that cannot be what we think it is must not be allowed to hold the + * verification path shut forever. Anything outside [0, FREE_PLAN_DAILY_LIMIT] + * is not a count of today's sends on a free plan, whatever else it is. + */ +export function usableQuotaReading(value: unknown): number | null { + if (typeof value !== "number" || !Number.isFinite(value)) return null; + if (value < 0 || value > FREE_PLAN_DAILY_LIMIT) return null; + return value; +} + /** * Has a quota reading aged out? * @@ -131,11 +210,10 @@ export function verificationAvailability( return { available: false, reason: "quota-exhausted", until }; } - const used = status?.dailyQuotaUsed; + const used = usableQuotaReading(status?.dailyQuotaUsed); const observedAt = millisOrNull(status?.dailyQuotaObservedAt); if ( - typeof used === "number" && - Number.isFinite(used) && + used !== null && !isQuotaReadingStale(observedAt, nowMs) && used >= VERIFICATION_CEILING ) { @@ -209,28 +287,47 @@ export async function readMailStatus(): Promise { */ export async function recordSendOutcome(outcome: { dailyQuotaUsed?: number; - quotaExhausted?: boolean; + pause?: PauseKind; errorName?: string; }): Promise { const now = Date.now(); const updates: Record = { updatedAt: Timestamp.fromMillis(now) }; - if (outcome.quotaExhausted) { - updates.unavailableUntil = Timestamp.fromMillis(nextUtcMidnight(now)); - updates.reason = outcome.errorName ?? "quota-exhausted"; - // Pin the counter at the limit. Without this a stale, lower reading from - // earlier in the day would keep saying the reserve is untouched. - updates.dailyQuotaUsed = FREE_PLAN_DAILY_LIMIT; - updates.dailyQuotaObservedAt = Timestamp.fromMillis(now); + if (outcome.pause) { + updates.unavailableUntil = Timestamp.fromMillis(pauseUntil(outcome.pause, now)); + updates.reason = outcome.errorName ?? outcome.pause; + if (outcome.pause === "daily-quota") { + // Pin the counter at the limit. Without this a stale, lower reading from + // earlier in the day would keep saying the reserve is untouched. + // + // ONLY for the daily cap. A monthly exhaustion says nothing about + // today's counter, and pinning it there would block verification on the + // reserve rule for a reason that has nothing to do with the reserve -- + // and would go on doing so after the daily counter resets. The monthly + // pause above already holds everything shut for as long as it needs to. + updates.dailyQuotaUsed = FREE_PLAN_DAILY_LIMIT; + updates.dailyQuotaObservedAt = Timestamp.fromMillis(now); + } } else { // A send got through, so whatever the breaker believed is now out of date. // This is the half-open close: the sweeper probes, and its success is what // reopens the realtime path. updates.unavailableUntil = null; updates.reason = null; - if (typeof outcome.dailyQuotaUsed === "number") { - updates.dailyQuotaUsed = outcome.dailyQuotaUsed; - updates.dailyQuotaObservedAt = Timestamp.fromMillis(now); + if (outcome.dailyQuotaUsed !== undefined) { + const used = usableQuotaReading(outcome.dailyQuotaUsed); + if (used === null) { + // Loud, because the alternative is silent: an implausible reading + // stored here would hold the verification path shut on the reserve + // rule with nothing in the logs saying why. See the header -- this is + // what a wrong guess about the header's meaning looks like. + console.error( + `mail-availability: refusing an implausible x-resend-daily-quota reading (${outcome.dailyQuotaUsed}); expected 0..${FREE_PLAN_DAILY_LIMIT}. Check what the header actually means before trusting it.` + ); + } else { + updates.dailyQuotaUsed = used; + updates.dailyQuotaObservedAt = Timestamp.fromMillis(now); + } } } diff --git a/functions/src/mail-delivery.ts b/functions/src/mail-delivery.ts index 5d8b2c0..cbd64ce 100644 --- a/functions/src/mail-delivery.ts +++ b/functions/src/mail-delivery.ts @@ -73,8 +73,9 @@ import { onDocumentCreated } from "firebase-functions/v2/firestore"; import { Timestamp } from "firebase-admin/firestore"; import { randomUUID } from "crypto"; import { db } from "./app.js"; -import { MAIL_COLLECTION } from "./mail.js"; +import { MAIL_COLLECTION, mailCollection } from "./mail.js"; import { recordSendOutcome } from "./mail-availability.js"; +import type { PauseKind } from "./mail-availability.js"; // --------------------------------------------------------------------------- // Timings. The relationship between these three numbers is load-bearing. @@ -402,9 +403,49 @@ const AMBIGUOUS_ERRORS = new Set([ // Quota exhaustion, as opposed to the per-second rate limit. These are what // trip the breaker in mail-availability.ts, because they are the ones that stay -// true for the rest of the day. +// true for the rest of the day -- or, for the monthly cap, the rest of the +// month, which is why they are told apart rather than lumped together. export const QUOTA_ERRORS = new Set(["daily_quota_exceeded", "monthly_quota_exceeded"]); +// Nothing is exhausted; this deployment cannot send AT ALL until a human +// changes something. A revoked or rescoped key, an unverified sending domain, +// missing configuration, a wrong endpoint. +// +// These trip the breaker too, and the reason is the realtime path. Without it +// the breaker only ever shuts on quota, so a revoked key leaves +// verificationAvailability answering "available" forever: every click mints a +// code, writes a document, spends a real Resend request and fails -- with no +// server-side ceiling on how often. Tripping here bounds that to one request +// per SYSTEMIC_PAUSE_MS no matter how many researchers are pressing the button. +// +// WHAT IS DELIBERATELY NOT HERE: `validation_error`. Resend uses it both for +// "your sending domain is unverified" (systemic) and for "this recipient +// address is malformed" (one researcher's typo). One researcher's typo must +// not switch verification off for everybody, so the ambiguous name stays out +// and the unambiguous ones carry the rule. +export const SYSTEMIC_ERRORS = new Set([ + "missing_api_key", + "restricted_api_key", + "suspended_api_key", + "invalid_permission", + "not_found", + "method_not_allowed", + CONFIG_MISSING_ERROR, +]); + +/** + * How long should this failure stop DataPipe sending, if at all? + * + * Pure, and exported for it: "how long is the breaker shut" is a decision worth + * asserting as a table rather than provoking through a transport. + */ +export function pauseKindFor(errorName: string): PauseKind | null { + if (errorName === "daily_quota_exceeded") return "daily-quota"; + if (errorName === "monthly_quota_exceeded") return "monthly-quota"; + if (SYSTEMIC_ERRORS.has(errorName)) return "systemic"; + return null; +} + // Errors where Resend provably did NOT send: it refused the request, or we // never reached it. Safe to retry at ANY age, because there is no message to // duplicate. @@ -541,6 +582,23 @@ function millisOrZero(value: unknown): number { return (value as { toMillis: () => number }).toMillis(); } +/** + * Is a PROCESSING claim's lease dead? + * + * The single source of truth for "the claimant is provably gone". Exported + * because scheduled-mail-retry.ts's sweepDecision asks the same question about + * the same field for the opposite reason -- claimDecision asks so it may take + * the claim, the sweep asks so it may recover the document -- and two + * independent copies of the lease rule is exactly how one of them silently + * stops matching LEASE_MS. + */ +export function leaseIsExpired( + delivery: { leaseExpiresAt?: unknown } | undefined, + nowMs: number +): boolean { + return millisOrZero(delivery?.leaseExpiresAt) <= nowMs; +} + function attemptsOf(delivery: DeliveryRecord | undefined): number { const n = delivery?.attempts; return typeof n === "number" && Number.isFinite(n) && n > 0 ? n : 0; @@ -566,7 +624,7 @@ export function claimDecision( if (state === "PROCESSING") { // Someone holds the claim and may be inside the send right now. - if (millisOrZero(delivery?.leaseExpiresAt) > nowMs) return "skip-in-flight"; + if (!leaseIsExpired(delivery, nowMs)) return "skip-in-flight"; // Lease expired: the claimant is provably dead (see LEASE_MS). Recoverable. } else if (state === "ERROR") { if (delivery?.retryable !== true) return "skip-terminal"; @@ -616,13 +674,33 @@ export type MailSender = ( const RESEND_ENDPOINT = "https://api.resend.com/emails"; /** A response header as a number, or undefined if absent or unparseable. */ -function headerInt(response: Response, name: string): number | undefined { - const raw = response.headers.get(name); +function headerInt(headers: Headers, name: string): number | undefined { + const raw = headers.get(name); if (raw === null) return undefined; const n = Number.parseInt(raw, 10); return Number.isFinite(n) ? n : undefined; } +// The quota headers, named ONCE each. +// +// Exported because these two string literals are the entire proactive half of +// the breaker and nothing else can tell you they are wrong: misspell one and +// every send returns no reading, mail-availability.ts never learns a number, +// verificationAvailability answers "available" forever, and every test still +// passes. Taking a plain Headers makes that assertable without a network call +// (mail-delivery.test.js). +export function quotaFromHeaders(headers: Headers): { + dailyQuotaUsed?: number; + monthlyQuotaUsed?: number; +} { + const daily = headerInt(headers, "x-resend-daily-quota"); + const monthly = headerInt(headers, "x-resend-monthly-quota"); + return { + ...(daily === undefined ? {} : { dailyQuotaUsed: daily }), + ...(monthly === undefined ? {} : { monthlyQuotaUsed: monthly }), + }; +} + let injectedSender: MailSender | null = null; /** @@ -647,7 +725,7 @@ export function _setMailSenderForTests(sender: MailSender | null): void { * apidata (index.ts imports every module in this codebase). There is now no * mail dependency to keep off it. */ -function resendSender(config: MailConfig): MailSender { +export function resendSender(config: MailConfig): MailSender { return async (input, { timeoutMs, idempotencyKey }) => { const response = await fetch(RESEND_ENDPOINT, { method: "POST", @@ -694,14 +772,7 @@ function resendSender(config: MailConfig): MailSender { // Quota headers ride on the SUCCESS response, which is the whole point -- // see mail-availability.ts. Read before the body so a malformed body cannot // cost us the reading. - const quota = { - ...(headerInt(response, "x-resend-daily-quota") !== undefined - ? { dailyQuotaUsed: headerInt(response, "x-resend-daily-quota") } - : {}), - ...(headerInt(response, "x-resend-monthly-quota") !== undefined - ? { monthlyQuotaUsed: headerInt(response, "x-resend-monthly-quota") } - : {}), - }; + const quota = quotaFromHeaders(response.headers); // 2xx means Resend accepted it. An unreadable body after that point costs // us the message id, which is a worse audit trail -- not a failed send, so @@ -808,7 +879,7 @@ async function writeIfStillOurs( * real trigger deliveries out of the emulator. */ export async function deliverMailDocument(docId: string): Promise { - const ref = db.collection(MAIL_COLLECTION).doc(docId); + const ref = mailCollection().doc(docId); const leaseOwner = randomUUID(); // ---------------- CLAIM ------------------------------------------------- @@ -870,6 +941,17 @@ export async function deliverMailDocument(docId: string): Promise { // them. Deleting these is why account deletion must not leave an address // behind: an undelivered or already-processed mail document is still a // record of where DataPipe last tried to reach this person. - const queuedMail = await db - .collection(MAIL_COLLECTION) + const queuedMail = await mailCollection() .where("datapipe.owner", "==", uid) .get(); counts.mailDocuments = await deleteInBatches( diff --git a/functions/src/scheduled-mail-retry.ts b/functions/src/scheduled-mail-retry.ts index 10165cc..9002fa2 100644 --- a/functions/src/scheduled-mail-retry.ts +++ b/functions/src/scheduled-mail-retry.ts @@ -18,48 +18,69 @@ // informed. The only dissent is a mail document nobody reads. // // --------------------------------------------------------------------------- -// WHAT IT DELIBERATELY DOES NOT SWEEP +// EVERY DOCUMENT THIS FINDS LEAVES BY A DOOR +// --------------------------------------------------------------------------- +// +// The sweep's queries are unordered `limit()`s, so any document the pass can +// look at without changing is a document it will look at again next pass, and +// forever -- occupying the budget that a deliverable notification needed. So +// the decision table has no permanent "skip" in it. Every outcome either sends +// (leaving SUCCESS or a fresh error), or writes a TERMINAL state that takes the +// document out of both queries. The only skips left are documents that another +// invocation is actively working on, which resolve themselves within LEASE_MS. +// +// Terminal matters for more than starvation. A retryable ERROR is never given +// `delivery.expireAt`, so it sits outside the TTL policy's reach holding a +// researcher's address indefinitely (docs/deploy-contact-email.md §4). Ageing +// one out is how that address finally gets deleted. +// +// --------------------------------------------------------------------------- +// WHAT IT DELIBERATELY DOES NOT SEND // --------------------------------------------------------------------------- // // 1. INLINE MAIL (contact-email verification). Never. A verification code is // realtime -- it expires in 24 hours and its recipient is watching a form // right now -- so a code delivered an hour late is not a late success, it is // a confusing failure. Those are marked terminal at failure time -// (mail-delivery.ts's isInline), so they should never match the query here; -// the explicit skip below is belt and braces. +// (mail-delivery.ts's isInline), so they should never match the queries +// here; one that somehow does is aged out rather than skipped, because a +// skipped one would sit there holding an address forever. // // 2. AMBIGUOUS ERRORS, past the idempotency window. mail-delivery.ts makes // timeouts retryable ONLY because Resend honours an Idempotency-Key, and it -// honours it for 24 hours. A sweeper is by nature a late retry -- quota -// resets daily -- so retrying a maybe-delivered send past that window is a -// coin flip on a second copy of a notification whose whole value is arriving -// once. REFUSED_ERRORS (the request was refused, or never reached Resend at -// all) carry no such risk and are swept at any age; everything else is swept -// only inside the window. +// honours it for 24 hours FROM ITS FIRST USE. A sweeper is by nature a late +// retry -- quota resets daily -- so retrying a maybe-delivered send past +// that window is a coin flip on a second copy of a notification whose whole +// value is arriving once. REFUSED_ERRORS (the request was refused, or never +// reached Resend at all) carry no such risk and are swept at any age; +// everything else is swept only inside the window. // -// 3. ANYTHING, while the breaker is shut. If mail-availability.ts says the -// quota is exhausted, sweeping would fail every document AND burn one of its -// three MAX_ATTEMPTS doing it -- so a day-long outage would exhaust the -// retry budget of every queued mail and turn all of them terminal, which is -// the precise opposite of this file's purpose. Checking the breaker first is -// what makes the sweep free to run often. +// 3. ANYTHING, while the breaker is shut. If mail-availability.ts says sending +// is paused, sending would fail every document AND burn one of its three +// MAX_ATTEMPTS doing it -- so an outage would exhaust the retry budget of +// every queued mail and turn all of them terminal, which is the precise +// opposite of this file's purpose. Checking the breaker first is what makes +// the sweep free to run often. The two passes that are only WRITES -- +// retention and ageing out -- run ahead of that check, because neither +// spends any quota and both matter most during an outage. import { onSchedule } from "firebase-functions/v2/scheduler"; import { Timestamp } from "firebase-admin/firestore"; -import { db } from "./app.js"; -import { MAIL_COLLECTION } from "./mail.js"; +import { mailCollection } from "./mail.js"; import { deliverMailDocument, isInline, + leaseIsExpired, REFUSED_ERRORS, MAIL_RETENTION_MS, + MAX_ATTEMPTS, } from "./mail-delivery.js"; import { deliveryPaused, readMailStatus } from "./mail-availability.js"; -import { UNRESOLVED_STATUSES } from "./upload-failure-notify.js"; +import { extendRetentionForExperiment } from "./upload-retention.js"; -// How long Resend honours an Idempotency-Key. The bound on retrying anything -// that is not provably un-sent. Raising this without checking Resend's docs -// would silently reintroduce double-sends. +// How long Resend honours an Idempotency-Key, measured FROM ITS FIRST USE. The +// bound on retrying anything that is not provably un-sent. Raising this without +// checking Resend's docs would silently reintroduce double-sends. export const IDEMPOTENCY_WINDOW_MS = 24 * 60 * 60 * 1000; // Past this, a notification has stopped being worth sending. Three days is @@ -67,36 +88,19 @@ export const IDEMPOTENCY_WINDOW_MS = 24 * 60 * 60 * 1000; // and still actionable a day or two later, but a researcher who has not noticed // in three days is better served by the dashboard than by an email about a // failure episode that has probably long since drained. -// -// Reaching it is TERMINAL, not a skip, and that matters for more than tidiness: -// a retryable ERROR is never given delivery.expireAt, so it sits outside the -// TTL policy's reach holding a researcher's address indefinitely. Ageing these -// out is what closes that hole (docs/deploy-contact-email.md §4). export const MAX_SWEEP_AGE_MS = 3 * 24 * 60 * 60 * 1000; -// One pass' budget. Small on purpose: the steady state is zero documents, the -// bad case is a quota outage that queued a few dozen, and a sweep that tried to -// drain hundreds would run into Resend's per-second rate limit and convert a -// recoverable backlog into a burned retry budget. +// One pass' budget, per query. Small on purpose: the steady state is zero +// documents, the bad case is a quota outage that queued a few dozen, and a +// sweep that tried to drain hundreds would run into Resend's per-second rate +// limit and convert a recoverable backlog into a burned retry budget. export const SWEEP_LIMIT = 25; -// How much longer a researcher's unuploaded data is kept while the -// notification about it is still undelivered. -// -// Seven days is not arbitrary: it is the SAME window the researcher would have -// had if the notification had worked. scheduled-upload-retry.ts's sweep counts -// from `createdAt` -- submission -- so by the time a notification fails, part -// of that window is already spent on a problem the researcher could not have -// known about. This gives it back, and keeps giving it back until they are -// actually told. The absolute ceiling lives in scheduled-upload-retry.ts's -// ABSOLUTE_MAX_RETENTION_MS, where the deletion happens, so there is exactly -// one place that can decide data is gone. -export const RETENTION_GRACE_MS = 7 * 24 * 60 * 60 * 1000; - -// Unresolved entries touched per undelivered notification. An episode is -// usually a handful of files; the cap is there so one pathological experiment -// cannot make a sweep unbounded. -export const RETENTION_BATCH = 100; +// What a document that was abandoned mid-send is recorded as. It has no error +// of its own -- the claim cleared the previous one -- and "we do not know what +// happened to this" is worth saying out loud in the audit trail rather than +// leaving a terminal document with an empty `error`. +export const SWEEP_ABANDONED_ERROR = "MailSweepAbandoned"; export interface SweepReport { scanned: number; @@ -123,77 +127,115 @@ function millisOrZero(value: unknown): number { * Pure, and exported for it -- this predicate is the entire safety argument of * the file, and it should be assertable as a table rather than provoked through * Firestore and a mail transport. + * + * "age-out" means TERMINAL: stop trying, and let the TTL have the address. + * "skip" is reserved for documents that are somebody else's right now. */ export function sweepDecision( data: FirebaseFirestore.DocumentData, nowMs: number ): "deliver" | "age-out" | "skip" { - if (isInline(data)) return "skip"; - const delivery = (data.delivery ?? {}) as Record; - if (delivery.state !== "ERROR" || delivery.retryable !== true) return "skip"; - // Age is measured from the last ATTEMPT, not from when the document was - // queued: a mail that has been retried into this morning is young, however - // long ago it was first written. startTime is the fallback for documents - // written before lastAttemptAt existed. - const lastAttempt = - millisOrZero(delivery.lastAttemptAt) || millisOrZero(delivery.startTime); - if (lastAttempt > 0 && nowMs - lastAttempt > MAX_SWEEP_AGE_MS) return "age-out"; + if (delivery.state === "PROCESSING") { + // Someone holds the claim and may be inside the send right now. Same rule + // as claimDecision's, and deliberately the same function: see leaseIsExpired. + if (!leaseIsExpired(delivery, nowMs)) return "skip"; + // Lease expired: the claimant is provably dead (mail-delivery.ts's + // LEASE_MS). Nothing else would ever look at this document again -- the + // claim rewrote it to PROCESSING with retryable null, which is outside the + // retryable-ERROR query that used to be the only one here, and outside the + // TTL too. That is how a preempted instance or a mid-deploy roll used to + // strand a researcher's address permanently. + } else if (delivery.state !== "ERROR" || delivery.retryable !== true) { + return "skip"; + } + + // Inline mail is realtime and nothing may re-send it late (see the header). + // Terminal rather than skipped: a verification code that failed is finished + // work, and leaving it "retryable" keeps expireAt off the document. + if (isInline(data)) return "age-out"; - const name = (delivery.error as { name?: unknown } | undefined)?.name; - if (typeof name !== "string") return "skip"; + // Out of attempts. mail-delivery.ts marks these terminal itself, so this is + // for the document it never got to write -- and without it, deliverMailDocument + // would answer "skipped-attempts-exhausted" on every pass forever. + const attempts = delivery.attempts; + if (typeof attempts === "number" && attempts >= MAX_ATTEMPTS) return "age-out"; + + // TWO CLOCKS, AND THEY MEASURE DIFFERENT THINGS. + // + // startTime when delivery FIRST began, and therefore when this + // document's Idempotency-Key (its own id) was first used. + // Never moves. This is what the 24-hour window is measured + // from -- measuring it from the last attempt slides the + // window forward with every retry, so a document retried at + // +20h and again at +40h would be sent on a key that expired + // at +24h, and Resend would treat it as a new message. + // lastAttemptAt when anything last happened. Age-out is measured from + // this, so a mail that has been retried into this morning is + // young however long ago it was first written. + const startedAt = millisOrZero(delivery.startTime); + const lastAttempt = millisOrZero(delivery.lastAttemptAt); + + // A document with neither is undatable, and an undatable document must not + // become immortal by lacking a field: with no clock to measure, there is no + // age at which it would ever age out, and it would hold its address forever. + if (startedAt === 0 && lastAttempt === 0) return "age-out"; - // Provably un-sent: retry at any age. - if (REFUSED_ERRORS.has(name)) return "deliver"; + if (nowMs - Math.max(startedAt, lastAttempt) > MAX_SWEEP_AGE_MS) return "age-out"; - // Everything else (a 5xx that may or may not have been accepted) only inside - // the window where the Idempotency-Key still deduplicates it. - if (lastAttempt > 0 && nowMs - lastAttempt <= IDEMPOTENCY_WINDOW_MS) return "deliver"; + // Provably un-sent: retry at any age, because there is no message to + // duplicate and so nothing for the Idempotency-Key to do. + const name = (delivery.error as { name?: unknown } | undefined)?.name; + if (typeof name === "string" && REFUSED_ERRORS.has(name)) return "deliver"; - return "skip"; + // Everything else -- a 5xx that may or may not have been accepted, an error + // this deploy has no name for, or a claim abandoned mid-send -- is ambiguous, + // and ambiguity is exactly what the Idempotency-Key resolves. Inside the + // window a retry is a no-op at Resend; outside it, the key has expired and a + // retry is a coin flip on a second copy, so we stop rather than gamble. + if (nowMs - (startedAt || lastAttempt) <= IDEMPOTENCY_WINDOW_MS) return "deliver"; + return "age-out"; } /** - * Hold back the data an undelivered notification is ABOUT. + * The experiment whose data this notification is about, if it is about any. * - * The notification is per EPISODE, and an episode belongs to an experiment, not - * to one file -- `datapipe.queueDocId` merely records the entry that tripped - * it. So the extension has to cover every unresolved entry for the experiment; - * extending only the one that tripped it would leave the other nineteen failed - * files in the same episode expiring on schedule, which is the original bug in - * miniature. - * - * Returns how many entries were held back. + * Verification codes have no data behind them to keep. */ -async function extendRetentionFor( - mailData: FirebaseFirestore.DocumentData, - nowMs: number -): Promise { +function retentionTargetOf(mailData: FirebaseFirestore.DocumentData): string | null { const meta = (mailData.datapipe ?? {}) as Record; - // Verification codes have no data behind them to keep. - if (meta.kind !== "upload-failure") return 0; - const experimentID = typeof meta.experimentID === "string" ? meta.experimentID : null; - if (!experimentID) return 0; - - // `experimentID ==` plus `status in` is served as a prefix by the existing - // (experimentID, status, providerErrorCode) composite -- the same shape - // upload-failure-notify.ts's drain query already runs. - const entries = await db - .collection("uploadQueue") - .where("experimentID", "==", experimentID) - .where("status", "in", UNRESOLVED_STATUSES) - .limit(RETENTION_BATCH) - .get(); - if (entries.empty) return 0; + if (meta.kind !== "upload-failure") return null; + return typeof meta.experimentID === "string" ? meta.experimentID : null; +} - const retainUntil = Timestamp.fromMillis(nowMs + RETENTION_GRACE_MS); - const batch = db.batch(); - for (const entry of entries.docs) { - batch.update(entry.ref, { retainUntil }); +/** The terminal write. Takes the document out of both sweep queries. */ +function ageOutUpdates( + data: FirebaseFirestore.DocumentData, + nowMs: number +): Record { + const delivery = (data.delivery ?? {}) as Record; + const at = Timestamp.fromMillis(nowMs); + const updates: Record = { + // ERROR and not PROCESSING, so a document abandoned mid-send cannot be + // matched by the stranded-claim query again next pass. + "delivery.state": "ERROR", + "delivery.retryable": false, + "delivery.leaseExpiresAt": null, + "delivery.endTime": at, + // The field the TTL policy keys on. Writing it is the whole point of + // ageing out rather than skipping. + "delivery.expireAt": Timestamp.fromMillis(nowMs + MAIL_RETENTION_MS), + }; + // Never overwrite a real failure with a generic one: the recorded error is + // usually the only account of why this mail never arrived. + if (!delivery.error || typeof delivery.error !== "object") { + updates["delivery.error"] = { + name: SWEEP_ABANDONED_ERROR, + message: "Delivery was abandoned in flight and could not safely be retried.", + }; } - await batch.commit(); - return entries.size; + return updates; } /** @@ -215,20 +257,33 @@ export async function sweepRetryableMail(nowMs = Date.now()): Promise { + try { + report.retained += await extendRetentionForExperiment(experimentID, nowMs); + } catch (error) { + // Never fatal, and per experiment: failing to extend one costs that + // researcher time, but throwing would cost every other one the sweep. + console.error( + `scheduled-mail-retry: could not extend retention for ${experimentID}:`, + error instanceof Error ? error.message : "Unknown error" + ); + } + }) + ); + + // ------------------------------------------------------------------------- + // AGE-OUT, ALSO BEFORE THE BREAKER, AND FOR THE SAME REASON. + // ------------------------------------------------------------------------- + // + // Giving up is a write, not a send. Leaving it behind the breaker would mean + // a month-long monthly-quota outage held every abandoned document -- and the + // researcher address in each one -- for the whole outage, which is the exact + // retention hole this pass exists to close. + // + // Concurrently: these are independent single-document writes, and a backlog + // that has just aged out can be the whole of both queries -- fifty serialized + // round-trips for writes that have nothing to say to each other. Each keeps + // its own error handling rather than becoming one batch, because a batch is + // atomic and one vanished document would take the other forty-nine with it. + await Promise.all( + decisions + .filter(([, decision]) => decision === "age-out") + .map(async ([doc]) => { + try { + await doc.ref.update(ageOutUpdates(doc.data(), nowMs)); + report.agedOut += 1; + console.log( + `scheduled-mail-retry: ${doc.id} aged out undelivered, marked terminal` + ); + } catch (error) { + // Not fatal, and the likeliest cause is benign: purge-user-data.ts + // deleting the researcher's mail out from under the pass, which is an + // expected event rather than a fault. Throwing here would cost the + // rest of the sweep -- including deliveries -- for a document that no + // longer needs anything done to it. + console.error( + `scheduled-mail-retry: could not age out ${doc.id}:`, + error instanceof Error ? error.message : "Unknown error" + ); + } + }) + ); - // The breaker. See the header: sweeping into an exhausted quota fails every + // The breaker. See the header: sending into a shut breaker fails every // document and spends a MAX_ATTEMPTS on each failure. const status = await readMailStatus(); if (deliveryPaused(status, nowMs)) { report.paused = true; + report.skipped += decisions.filter(([, d]) => d === "skip").length; console.log( - "scheduled-mail-retry: delivery is paused (quota), sending nothing this pass" + "scheduled-mail-retry: delivery is paused, sending nothing this pass" ); return report; } for (const [doc, decision] of decisions) { - if (decision === "skip") { - report.skipped += 1; - continue; - } - - if (decision === "age-out") { - // Terminal, so delivery.expireAt is written and the TTL can finally reap - // the address this document has been holding. - await doc.ref.update({ - "delivery.retryable": false, - "delivery.endTime": Timestamp.fromMillis(nowMs), - "delivery.expireAt": Timestamp.fromMillis(nowMs + MAIL_RETENTION_MS), - }); - report.agedOut += 1; - console.log( - `scheduled-mail-retry: ${doc.id} aged out undelivered, marked terminal` - ); + if (decision !== "deliver") { + if (decision === "skip") report.skipped += 1; continue; } @@ -294,16 +395,16 @@ export async function sweepRetryableMail(nowMs = Date.now()): Promise ${outcome}`); - // A quota failure mid-sweep means the rest of this pass would fail too, and + // A failure mid-sweep may mean the rest of this pass would fail too, and // each failure costs a retry attempt. deliverMailDocument has already - // tripped the breaker; honour it immediately rather than after 24 more - // wasted attempts. + // tripped the breaker if the cause was one that shuts it; honour that + // immediately rather than after 24 more wasted attempts. if (outcome === "retryable-error" || outcome === "terminal-error") { const fresh = await readMailStatus(); if (deliveryPaused(fresh, nowMs)) { report.paused = true; console.log( - "scheduled-mail-retry: quota tripped mid-sweep, stopping this pass" + "scheduled-mail-retry: sending was paused mid-sweep, stopping this pass" ); break; } @@ -316,8 +417,9 @@ export async function sweepRetryableMail(nowMs = Date.now()): Promise= CLEANUP_DELETE_LIMIT) break; + scanned += 1; + const data = doc.data(); + if (ownerScope && data.owner !== ownerScope) continue; + + // The query above finds entries by AGE. Whether one may actually be + // destroyed is a separate question, and not one an age answers on its own + // -- see upload-retention.ts. + if (retentionDecision(data, now) === "retain") { + retained += 1; + continue; + } - // The query above finds entries by AGE. Whether one may actually be - // destroyed is a separate question, and not one an age answers on its own - // -- see upload-retention.ts. - if (retentionDecision(data, now) === "retain") { - retained += 1; - continue; + try { + await bucket.file(data.storagePath).delete(); + } catch { + // File may already be deleted + } + await doc.ref.delete(); + deleted += 1; } - try { - await bucket.file(data.storagePath).delete(); - } catch { - // File may already be deleted - } - await doc.ref.delete(); - deleted += 1; + if (page.size < CLEANUP_PAGE_SIZE) break; } + if (deleted === 0 && retained === 0) return; + console.log( `Cleanup: ${deleted} old queue entries deleted, ${retained} retained (still retrying, or the researcher has not been notified yet).` ); diff --git a/functions/src/send-contact-email-verification.ts b/functions/src/send-contact-email-verification.ts index 3e2161b..e0a3baf 100644 --- a/functions/src/send-contact-email-verification.ts +++ b/functions/src/send-contact-email-verification.ts @@ -36,13 +36,19 @@ const CODE_SPACE = 10 ** CODE_DIGITS; // 1_000_000 possible codes // plan §2.2: "24-hour expiry, 5 attempts." export const EXPIRY_MS = 24 * 60 * 60 * 1000; -// Floor between two sends to the same account. The plan does not pin a -// number beyond "rate-limited" and putting `sentAt` in the +// Floor between two ATTEMPTS to send to the same account. The plan does not pin +// a number beyond "rate-limited" and putting `sentAt` in the // contactEmailVerifications schema for exactly this purpose; one minute is // enough to stop a resend-mail-bomb loop (an impatient double-click, or // someone hammering the endpoint) without making a researcher who mistyped // and wants a fresh code wait anywhere near as long as the 24-hour code // expiry itself. +// +// ATTEMPTS, not deliveries, and that distinction is the rate limit. This +// endpoint spends a real Resend request every time it gets past this check, so +// a path that reaches the send is a path that has to be throttled whatever the +// send then does. See the failure branch below, which used to delete the very +// record this reads. export const RESEND_COOLDOWN_MS = 60 * 1000; export const VERIFICATIONS_COLLECTION = "contactEmailVerifications"; @@ -164,8 +170,13 @@ export const sendContactEmailVerification = onRequest( // -- none of which they can act on differently anyway. const availability = verificationAvailability(await readMailStatus(), Date.now()); if (!availability.available) { - console.warn( - `send-contact-email-verification: refused for ${uid}, mail unavailable (${availability.reason})` + // ERROR, not WARN, and with a stable token: "verification is refused + // for everybody" is a condition an operator wants to hear about, + // especially in the `quota-reserve` case, where the only evidence that + // the daily-quota header means what this code thinks it means is that + // this line is NOT firing all day (docs/deploy-contact-email.md §6). + console.error( + `send-contact-email-verification: MailVerificationUnavailable (${availability.reason}) -- refused for ${uid}` ); res.status(503).json({ error: @@ -263,17 +274,38 @@ export const sendContactEmailVerification = onRequest( // returned outcome and a field on the document. const outcome = await deliverMailDocument(mailRef.id); if (outcome !== "sent") { - // The code was never delivered, so the record that proves it exists - // must go -- and with it the resend cooldown it arms. A researcher - // whose code never arrived should be able to press the button again - // immediately, not wait out a minute for a code that does not exist. + // --------------------------------------------------------------------- + // THE RECORD STAYS. IT IS THE RATE LIMIT. + // --------------------------------------------------------------------- + // + // This branch used to delete it, so that a researcher whose code never + // arrived would not be stuck behind a cooldown for a code that does not + // exist. That reasoning is right about the researcher and wrong about + // the endpoint: `sentAt` is the ONLY server-side throttle on this path, + // and deleting it here removed the throttle from exactly the case that + // needs one. A send that fails still costs a Resend request and still + // writes a mail document, so a signed-in researcher holding the button + // -- or a loop on one valid ID token -- drove both without any bound at + // all, and the pre-send breaker did not cover it either, because it only + // shut on quota errors. + // + // Two changes make that safe rather than merely throttled: the breaker + // now also shuts on a systemic failure (mail-delivery.ts's + // SYSTEMIC_ERRORS), so a revoked key or an unverified domain stops + // costing a request per click within one attempt; and the code itself is + // cleared here, so what survives is a cooldown and not a secret nobody + // received. The researcher waits at most RESEND_COOLDOWN_MS, which is the + // same minute they would wait after a send that worked. try { - await verificationRef(uid).delete(); + await verificationRef(uid).update({ + codeHash: null, + deliveryFailedAt: now, + }); } catch (cleanupError) { - // Non-fatal, but worth a line: the researcher is now sitting on a - // cooldown for a code they never got. + // Non-fatal. The code is undeliverable either way and expires in 24 + // hours; what matters is that `sentAt` is still standing. console.error( - `send-contact-email-verification: could not clear the unsent record for ${uid}:`, + `send-contact-email-verification: could not clear the unsent code for ${uid}:`, cleanupError instanceof Error ? cleanupError.message : "Unknown error" ); } diff --git a/functions/src/upload-failure-notify.ts b/functions/src/upload-failure-notify.ts index d37e127..ba302a3 100644 --- a/functions/src/upload-failure-notify.ts +++ b/functions/src/upload-failure-notify.ts @@ -47,6 +47,7 @@ import { contactEmailRecipient, enqueueMail, newMailRef } from "./mail.js"; import type { MailMeta } from "./mail.js"; import { buildUploadFailureEmail } from "./email/upload-failure-copy.js"; import type { UploadFailureState } from "./interfaces.js"; +import { unresolvedQueueEntriesQuery } from "./upload-retention.js"; // One mail per experiment per 24 hours, maximum, no matter how often the queue // flaps clear->fail. A study that breaks every hour for a week produces seven @@ -54,20 +55,6 @@ import type { UploadFailureState } from "./interfaces.js"; // transactions below cannot close -- see clearIfDrained. export const RATE_LIMIT_MS = 24 * 60 * 60 * 1000; -// "Not resolved yet, from the researcher's point of view." Deliberately the -// predicate the dashboard already uses (pages/admin/[experiment_id].js:46-53 -// and api-queue-status.ts:143-144), INCLUDING `failed`, and deliberately not -// finalization.ts's, which excludes `failed` so that a permanently dead file -// cannot block sealing forever. -// -// Including `failed` is what stops the notification re-arming while a dead -// file is still sitting there: that file is an unresolved problem the -// researcher has not dealt with, and re-arming would mail them again about a -// queue that never actually got better. The episode closes when the last -// unresolved entry either completes or is swept away by cleanupOldEntries at -// seven days. -export const UNRESOLVED_STATUSES = ["pending", "processing", "failed"]; - // The Admin SDK default is 5, which is not enough here. The motivating case is // the ordinary one: a metadataActive submission produces a raw file, a main // CSV and one sidecar per extracted column, so a single participant can put @@ -337,14 +324,9 @@ async function clearIfDrained(experimentID: string): Promise // limit(1) rather than count(): a plain query inside a transaction is // universally supported by the Admin SDK and the emulator, costs one // document read, and answers the only question being asked -- "is anything - // still unresolved?". Index-wise this is `experimentID ==` + `status in`, - // which the existing composite (experimentID, status, providerErrorCode) - // serves as a prefix, and finalization.ts already runs the same shape. - const stillUnresolved = db - .collection("uploadQueue") - .where("experimentID", "==", experimentID) - .where("status", "in", UNRESOLVED_STATUSES) - .limit(1); + // still unresolved?". The shape, and the composite index it rests on, are + // upload-retention.ts's -- which owns what "unresolved" means. + const stillUnresolved = unresolvedQueueEntriesQuery(experimentID).limit(1); return db.runTransaction(async (tx) => { // ---------------- ALL READS FIRST (Firestore transaction law) ---------- diff --git a/functions/src/upload-retention.ts b/functions/src/upload-retention.ts index d20b4a4..95d55e4 100644 --- a/functions/src/upload-retention.ts +++ b/functions/src/upload-retention.ts @@ -24,8 +24,11 @@ // wrong -- and if the notification died (a Resend quota outage, say) the // researcher's window closes without them ever learning there was one. // -// `retainUntil` is written by scheduled-mail-retry.ts while an upload-failure -// notification for the experiment is still undelivered and still deliverable. +// `retainUntil` is written at the bottom of this file, and +// scheduled-mail-retry.ts is what calls it: while an upload-failure +// notification for the experiment is undelivered -- including the pass that +// finally gives up on it, so that abandoning a notification does not also +// quietly shorten the window for the data it was about. // // NOTE WHAT DOES NOT WRITE IT. An experiment whose owner has no contact email // never produces a mail document at all -- upload-failure-notify.ts records @@ -34,6 +37,9 @@ // when there is nobody to tell, and it falls out of the design rather than // being special-cased. +import { Timestamp } from "firebase-admin/firestore"; +import { db } from "./app.js"; + // The ceiling on everything below. An entry is deleted once it reaches this // age no matter what else is still true about it. // @@ -90,3 +96,126 @@ export function retentionDecision( return "delete"; } + +// --------------------------------------------------------------------------- +// Holding data back while the researcher has not been told about it. +// --------------------------------------------------------------------------- +// +// The WRITE lives here, next to the predicate that reads it, rather than in +// scheduled-mail-retry.ts where it started. The rule "may this data be +// destroyed, and what holds it back" is one decision; splitting it across the +// mail sweeper, this module and the deletion sweep meant three files in two +// subsystems had to be read together to answer a single question, and the mail +// subsystem had to know the uploadQueue's schema to do it. +// +// The mail sweeper now says only the thing it actually knows -- "this +// experiment's researcher has not been told yet" -- and this module decides +// what that means for their data. +// +// WHAT WAS CONSIDERED AND NOT DONE: inverting the dependency completely, so +// that cleanupOldEntries asks "is a notification for this experiment still +// undelivered?" at the moment of deletion and nothing is written ahead of time. +// It is the tidier shape, but it makes the least reversible operation in the +// codebase depend on a query that can fail, and a failing query there deletes +// data. `retainUntil` fails the other way: a write that does not happen costs +// the researcher an extension, not their data. + + +// "Not resolved yet, from the researcher's point of view." Deliberately the +// predicate the dashboard already uses (pages/admin/[experiment_id].js:46-53 +// and api-queue-status.ts:143-144), INCLUDING `failed`, and deliberately not +// finalization.ts's, which excludes `failed` so that a permanently dead file +// cannot block sealing forever. +// +// Including `failed` is what stops the upload-failure notification re-arming +// while a dead file is still sitting there: that file is an unresolved problem +// the researcher has not dealt with, and re-arming would mail them again about +// a queue that never actually got better. The episode closes when the last +// unresolved entry either completes or is swept away by cleanupOldEntries. +export const UNRESOLVED_STATUSES = ["pending", "processing", "failed"]; + +/** + * Every uploadQueue entry for one experiment that the researcher would still + * call unresolved. + * + * One builder rather than two, because both callers rest on the same index + * argument: `experimentID ==` plus `status in` is served as a prefix by the + * existing (experimentID, status, providerErrorCode) composite. The caller adds + * its own limit -- upload-failure-notify.ts only asks whether anything is left, + * this module has to touch all of them. + */ +export function unresolvedQueueEntriesQuery( + experimentID: string +): FirebaseFirestore.Query { + return db + .collection("uploadQueue") + .where("experimentID", "==", experimentID) + .where("status", "in", UNRESOLVED_STATUSES); +} + +// How much longer a researcher's unuploaded data is kept while the +// notification about it is still undelivered. +// +// Seven days is not arbitrary: it is the SAME window the researcher would have +// had if the notification had worked. scheduled-upload-retry.ts's sweep counts +// from `createdAt` -- submission -- so by the time a notification fails, part +// of that window is already spent on a problem the researcher could not have +// known about. This gives it back, and keeps giving it back until they are +// actually told. The absolute ceiling is ABSOLUTE_MAX_RETENTION_MS above, and +// the deletion itself is scheduled-upload-retry.ts's, so there is exactly one +// place that can decide data is gone. +export const RETENTION_GRACE_MS = 7 * 24 * 60 * 60 * 1000; + +// Unresolved entries touched per undelivered notification. An episode is +// usually a handful of files; the cap is there so one pathological experiment +// cannot make a sweep unbounded. +export const RETENTION_BATCH = 100; + +// Don't rewrite a `retainUntil` that is already most of the way out. +// +// The extension is driven by the mail sweeper, which runs every ten minutes for +// as long as a notification stays undelivered -- so without this, a day-long +// outage with a couple of dozen queued notifications rewrites the same field +// tens of thousands of times to the same effect. Firestore's free tier is +// 20,000 writes a day. Half the grace window is the threshold because the +// consequence of skipping a write is bounded by it: the stored value cannot be +// closer than RETENTION_GRACE_MS/2 to expiring, which is three and a half days +// of slack on a sweep that runs every ten minutes. +export const RETENTION_REWRITE_FLOOR_MS = RETENTION_GRACE_MS / 2; + +/** + * Hold back the data an undelivered notification is ABOUT. + * + * The notification is per EPISODE, and an episode belongs to an experiment, not + * to one file -- `datapipe.queueDocId` merely records the entry that tripped + * it. So the extension has to cover every unresolved entry for the experiment; + * extending only the one that tripped it would leave the other nineteen failed + * files in the same episode expiring on schedule, which is the original bug in + * miniature. + * + * Returns how many entries are being held back (whether or not this call was + * the one that had to write them). + */ +export async function extendRetentionForExperiment( + experimentID: string, + nowMs: number +): Promise { + const entries = await unresolvedQueueEntriesQuery(experimentID) + .limit(RETENTION_BATCH) + .get(); + if (entries.empty) return 0; + + const floor = nowMs + RETENTION_REWRITE_FLOOR_MS; + const stale = entries.docs.filter( + (entry) => millisOrZero(entry.get("retainUntil")) < floor + ); + if (stale.length > 0) { + const retainUntil = Timestamp.fromMillis(nowMs + RETENTION_GRACE_MS); + const batch = db.batch(); + for (const entry of stale) { + batch.update(entry.ref, { retainUntil }); + } + await batch.commit(); + } + return entries.size; +} diff --git a/functions/src/verify-contact-email.ts b/functions/src/verify-contact-email.ts index 7f835e8..32969fa 100644 --- a/functions/src/verify-contact-email.ts +++ b/functions/src/verify-contact-email.ts @@ -42,6 +42,18 @@ function verificationRef(uid: string): FirebaseFirestore.DocumentReference { return db.collection(VERIFICATIONS_COLLECTION).doc(uid); } +// One answer, two ways to arrive at it: no record at all, and a record whose +// code was cleared because it could not be delivered. The researcher's next +// move is the same either way, so the wording is defined once rather than kept +// in step by hand. +const NO_CODE_REQUESTED = { + status: 400, + body: { + error: "Request a new verification code.", + code: "no-code-requested", + }, +}; + export const verifyContactEmail = onRequest({ cors: true }, async (req, res) => { if (req.method !== "POST") { res.status(405).json({ error: "Method not allowed" }); @@ -104,16 +116,22 @@ export const verifyContactEmail = onRequest({ cors: true }, async (req, res) => } if (!vSnap.exists) { - return { - status: 400, - body: { - error: "Request a new verification code.", - code: "no-code-requested", - }, - }; + return NO_CODE_REQUESTED; } const v = vSnap.data()!; + + // A record whose code was never delivered. send-contact-email- + // verification.ts clears `codeHash` and keeps the rest of the record -- + // the record is that endpoint's rate limit, so it may not be deleted, but + // there is no code out there to enter. Answering "request a new one" is + // both true and the same answer a missing record gets above; falling + // through would spend one of the five attempts on a code that does not + // exist. + if (typeof v.codeHash !== "string") { + return NO_CODE_REQUESTED; + } + const expiresAt = typeof v.expiresAt === "number" ? v.expiresAt : 0; if (Date.now() > expiresAt) { return { diff --git a/pages/admin/index.js b/pages/admin/index.js index f01809e..a5ee2c0 100644 --- a/pages/admin/index.js +++ b/pages/admin/index.js @@ -31,20 +31,41 @@ import ConfirmDialog from "../../components/ui/ConfirmDialog"; export default function AdminPage({}) { return ( - {/* DESIGN.md §4 allows exactly two measures: 560px for a single-subject - settings column, 1100px for dashboard and marketing pages. This was - 960px, one of three different widths across the three dashboard - routes (960 / 540 / 1200). */} - - - - - + ); } -function ExperimentList() { +// Inside AuthCheck rather than in AdminPage itself, because everything here +// reads `auth.currentUser` and AuthCheck is what guarantees there is one. +// +// ONE subscription to users/{uid} for this page, passed down -- the convention +// pages/admin/account.js already follows (ProviderConnections, ContactEmail, +// OAuthTokenStatus and SelectAuth all take `data` as a prop and none of them +// subscribes). Two components here read that document for different reasons, +// and before this they opened a listener each: two live listeners on one +// document, two independent loading flickers, and two chances for them to +// render disagreeing states for a frame. +function Dashboard() { + const user = auth.currentUser; + const [userDoc] = useDocumentData( + user?.uid ? doc(db, "users", user.uid) : null + ); + + return ( + /* DESIGN.md §4 allows exactly two measures: 560px for a single-subject + settings column, 1100px for dashboard and marketing pages. This was + 960px, one of three different widths across the three dashboard + routes (960 / 540 / 1200). */ + + + + + + ); +} + +function ExperimentList({ userDoc }) { const user = auth.currentUser; const experiments = collection(db, `experiments`); const q = query(experiments, where("owner", "==", user.uid)); @@ -56,9 +77,6 @@ function ExperimentList() { // step, not a maintenance screen), and firestore.rules enforces it. Offering // "Create your first experiment" to someone who will be refused is the dead // end the critique traces persona Jordan into. - const [userDoc] = useDocumentData( - user?.uid ? doc(db, "users", user.uid) : null - ); const hasProvider = Object.values(STORAGE_PROVIDERS).some((p) => p.isConnected(userDoc) );