fix(email): close the d5 email queue debts - #62
Merged
Conversation
mise.toml pinned pnpm 10.33.2 while package.json#packageManager pinned 11.0.9 — corepack follows one, mise follows the other, and which wins was accidental. Both now state pnpm@11.24.0. Node goes to 24.20.0 LTS (26.x is Current, and Node only runs tooling here), Bun to 1.4.0 — the runtime apps/api ships on, proven by the 718-test API suite and the Bun build. .nvmrc moves with them: CI resolves its Node from that file, so leaving it behind would have kept the pipeline on 24.15.0 while every other surface moved. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
Three ranges genuinely blocked -- typescript ^6.0.3 -> ^7.0.2 across the 11 workspaces that declare it, @hono/zod-validator ^0.8 -> ^0.9 and @types/pg ^8.20 -> ^8.21, neither of which a caret crosses. Everything else was a stale lockfile, not a stale range, and moved via pnpm up -r --latest. packages/ddd-kit keeps typescript pinned to ^6.0.3 -- its tsup build emits .d.ts via a bundled rollup-plugin-dts that isn't yet updated for TS7's rewritten compiler-host API and crashes under 7.0.2. Every other workspace runs its type-check against 7.0.2; this is the one place the Go port isn't 1:1 yet, exactly as expected going in. pnpm up also pulled @better-auth/scim/sso/passkey/stripe/core to 1.7.2 via their caret ranges. @better-auth/scim@1.7.0 turns out to be a full config rewrite (requiredRole/providerOwnership/storeSCIMToken all removed) -- a real breaking change behind a semver-minor bump, and out of scope for a toolchain refresh since it would mean re-implementing the SCIM provisioning integration against a different contract. Rolled the whole better-auth family back to 1.6.30 (the newest 1.6.x patch, still ahead of where the repo started) across apps/api, apps/app and packages/access-control. The refresh also surfaced a lib.dom.d.ts change in TS7: Response gained a textStream member that hono's ClientResponse doesn't implement, breaking every RPC call site that fed a response into throwApiError(res: Response). Narrowed that function's parameter to what it actually reads instead of the full DOM Response type. Stripe's apiVersion literal moved with the 22.6.0 bump. packages/ui's react-hook-form/@hookform/resolvers peer ranges were bumped to match what apps/app now resolves to, closing a duplicate- instance split the refresh otherwise left behind (peer ranges aren't touched by `pnpm up -r --latest`). The biome $schema URL moves with the binary -- it pointed at 2.5.1 and was silently under-validating the config; now points at 2.5.10, the resolved version. Biome 2.5.10 also flags noUnsafeOptionalChaining more eagerly than 2.5.1 did, catching three pre-existing (x?.y as T).z casts in test files that would throw at runtime if x were ever undefined; fixed with explicit narrowing guards rather than suppressing the rule. The eight deprecated z.string().email()/.url() call sites move to the standalone form, deprecated in Zod 4 and gone in 5. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
Compose, the CI service image and the two README mentions all move together, plus the three postgres:17-alpine examples in docs/DISASTER-RECOVERY.md that would otherwise document a runbook against a major the repo no longer ships. DEPLOY-RAILWAY.md turned out to pin no version at all (it runs `railway add --database postgres-ssl` and lets Railway pick), so it gains an explicit minimum instead of a bump. docker-compose.yaml's postgres service also moves its volume mount from /var/lib/postgresql/data to /var/lib/postgresql: the 18+ Docker images store data one directory up (pg_ctlcluster-style layout) and refuse to start against the old mount point. Undocumented by the image's changelog at the version this was bumped to; discovered by the container looping in Restarting after the volume recreate in Step 6. Primary keys stay text: every PK in packages/drizzle/src/schema is filled by BetterAuth, which generates its own ids, so uuidv7() is a BetterAuth question rather than a Postgres one. It is documented in MODULES.md as the shape for new cloner-owned tables instead. The payoff here is operational (async I/O, B-tree skip scan), so the outbox drain and the notification fan-out were EXPLAIN (ANALYZE, BUFFERS)'d on 17 and again on 18; both plans are in HISTORY.md. The outbox drain (the query that matters most here) kept its index scan on outbox_event_pending_idx on both versions. The fan-out probe's member-table access path also changed (seq scan -> bitmap index scan), but the two captures ran against different data volumes (11h-old dev data vs a freshly reseeded DB per Step 6), so that diff is flagged as inconclusive rather than attributed to Postgres 18. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
tsup's bundled rollup-plugin-dts crashes under TypeScript 7 (peer-resolved from ddd-kit's own devDependency, unreachable by any pnpm override -- see the correction round notes in task-2-report.md for the four mechanisms tried and why each fails against an optional peerDependency). Pinning ddd-kit's typescript to ^6.0.3 avoided the crash but downgraded its tsc --noEmit type-check gate along with it, leaving it as the only one of 11 workspaces off TS7. tsup no longer needs typescript at all once it stops emitting .d.ts (dts: false replaces the dts.compilerOptions block, and its ignoreDeprecations: "6.0" pansement goes with it) -- so the optional peer that pinned tsup's resolution stops applying, and ddd-kit's own typescript can move to ^7.0.2 like every other workspace. Declarations are now emitted by tsc against tsconfig.build.json (a config that already existed, unwired, from the publishing setup in 7518f61) with emitDeclarationOnly instead of declaration+noEmit:false, so tsc adds only .d.ts/.d.ts.map into dist/ without emitting parallel .js files tsup already produces. incremental stays false there deliberately: tsup's clean:true wipes dist/ on every build, and an incremental .tsbuildinfo left over from tsc would otherwise report the declarations as already up to date and silently emit nothing. publishConfig and the tsup JS bundling step are both kept as-is per an explicit scope decision -- only the declaration emitter changes. The build script chains tsup && tsc -p tsconfig.build.json so a single `pnpm build` still produces dist/index.js, dist/index.cjs and dist/index.d.ts. Declarations are no longer bundled into one file -- tsc emits one .d.ts per source module (dist/domain/entity.d.ts, dist/primitives/result.d.ts, etc.) re-exported from dist/index.d.ts, instead of tsup's single flattened index.d.ts. Acceptable: nothing in this monorepo consumes dist/ (internal packages import from src/ directly per rule #4), this only matters the day the package is actually published. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
…ount move Round 1 of review on the G.1c Postgres 18 bump: the original grep only matched the postgres:17-alpine image-tag form, so four prose mentions in README.md (138, 172, 315), docs/FEATURES.md (176) and CLAUDE.md (29) still said "Postgres 17" while the infra table right next to two of them already said postgres:18-alpine. Fixed all four; docs/HISTORY.md's dated "before" EXPLAIN captures on Postgres 17 are left untouched, since those describe a measurement taken at a point in time, not the current stack. Also documents the operational hazard behind the docker-compose.yaml volume mount change from a prior commit (/var/lib/postgresql/data -> /var/lib/ postgresql, required by Postgres 18+ images): anyone who already had a postgres_data volume populated under the old layout gets a silent empty database on their next `docker compose up`, not a visible crash. README.md's Database section (Volume row + a callout above the db:* script block) and docs/HISTORY.md's G.1c section both now say plainly: docker volume rm clean-stack_postgres_data once before the first up on this branch, then pnpm db:push && pnpm db:seed. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
The tree was composed by hand: 40 createRoute() calls, a router.tsx
addChildren block, and 9 nodes crammed into router/layouts.tsx. E.1 plans a
$lang layout re-parenting every one of them, so migrating first avoids doing
that work twice.
An explicit virtualRouteConfig, not physical() and not a flattened
src/routes/: the 5 pathless layouts wrap routes drawn from 8 different
features, which a directory mount cannot express, and flattening would
destroy the vertical slices. Every features/<x>/<x>.route.tsx stays exactly
where it is; routes.ts declares the tree once. Empirically confirmed that
virtualRouteConfig paths resolve relative to routesDirectory ("./src"), not
to vite.config.ts, by generating the tree from a single route before writing
the other 39.
Route ids are unchanged, which is what keeps the 12 getRouteApi call sites
valid — they are string literals checked through Register, so type-check is
the gate that proves it. autoCodeSplitting stays off here and the 32
lazyRouteComponent call sites are untouched; merging the pages is separate.
knip.json gains three apps/app entry patterns (routes.ts, src/router/*.tsx,
src/features/**/*.route.tsx): routeTree.gen.ts is gitignored and is the only
file that imports each Route export, and the vite.config.ts -> routes.ts link
goes through a string (virtualRouteConfig) knip can't follow statically.
Without these entries every converted route file reads as dead code.
Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
autoCodeSplitting only splits a component internal to its route file — a statically imported one silently stays in the main bundle. So the 32 <x>.page.tsx files merge into their <x>.route.tsx and the 32 lazyRouteComponent wrappers go. The 2-file pattern existed solely because the bundler splits only modules reachable exclusively via a dynamic import(), and router.tsx imported every route file statically. The plugin removes that constraint, so the rule that rests on it no longer holds; features/CLAUDE.md is rewritten in the docs commit that closes this phase. The 12 getRouteApi(...) call sites collapse to Route.* now that page and route share a module. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
The 2-file routing rule in features/CLAUDE.md is rewritten rather than amended: it was justified by manual code-splitting, and the plugin now owns that, so per cross-cutting rule #1 the rule goes when its property does. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
.gitignore excluded apps/app/src/routeTree.gen.ts while nothing in turbo.json's type-check task depends on the app build that generates it (type-check only depends on ^build, the upstream packages, not the app's own build). A fresh clone running `pnpm type-check` hit 52 errors, one per route, and CI/pre-push could flake on the same race between Vite generating the file and tsc reading it. Follow TanStack Router's own recommendation and commit the generated file instead. Regenerated it via `pnpm --filter app build` before committing so it reflects the current route tree (40 routes). The three apps/app entry points added to knip.json for the routing migration (routes.ts, src/router/*.tsx, src/features/**/*.route.tsx) stay required even though the tree is now versioned: knip can't resolve the string paths passed to virtualRouteConfig. Documented that explicitly in apps/app/CLAUDE.md and src/features/CLAUDE.md so it isn't mistaken for now-redundant config. Also fixes doc/code drift left over from the file-based routing migration and the toolchain refresh: - README.md and docs/OVERVIEW.md described a codegen-free 2-file router pattern that no longer exists. - README.md's initial-bundle figure (~588 KB) was stale; the real post-merge figure is 363,798 bytes (~355 KB). - docs/REMOVABILITY.md's removal recipe pointed at router.tsx route registrations and *.page.tsx files that no longer exist — updated to apps/app/routes.ts entries and *.route.tsx. - ROADMAP.md and docs/HISTORY.md claimed biome 2.5.11; the pinned and installed version is 2.5.10 (the $schema URL was already correct). - apps/app/CLAUDE.md's landmark rule and docs/FEATURES.md's path references still pointed at removed *.page.tsx files. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
routeTree.gen.ts is now committed (previous commit) so a fresh clone type-checks without a build step. TanStack's own header on the file says to exclude it from lint/format tooling, and biome.json already does (routeTree.gen.ts override, linter+assist disabled). jscpd was the one tool still scanning it, and its repetitive per-route import blocks add one extra self-clone (29 vs the expected 28) that has nothing to do with hand-written duplication. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
…0/pnpm 11.24.0 G.1 bumped mise.toml/.nvmrc/package.json but left the four Dockerfiles pinned to node:24.15.0-alpine, oven/bun:1.3.6-alpine and pnpm@10.33.2. apps/api/dev.Dockerfile and apps/app/dev.Dockerfile are what docker compose builds, so dev:docker would install with pnpm 10 against a lockfile regenerated by pnpm 11 - resolution no longer guaranteed to match native dev. Also fixes stale pnpm 10 / floor version mentions in README, CLAUDE.md, docs/MODULES.md and docs/FEATURES.md describing current tooling (dated baselines in CHANGELOG.md/ROADMAP.md/docs/HISTORY.md are left untouched). Verified node:24.20.0-alpine and oven/bun:1.4.0-alpine exist on Docker Hub, both docker compose build api/app succeed with pnpm v11.24.0, and the full gate (ci:check, turbo build/type-check/test, check:duplication, check:unused) is green. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
CLAUDE.md still advertised "Bun 1.3+ / Node 24.15+" as the floor while package.json#engines.node now requires >=24.20.0 and mise.toml pins Bun 1.4.0. The Bun 1.3+ mentions in OBSERVABILITY.md and HISTORY.md are left alone: they describe OTel behaviour under that version, not a repo floor. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
Phase G.1 — toolchain refresh + file-based routing. pnpm 11.24.0, Node 24.20.0, Bun 1.4.0, TypeScript 7.0.2 across all 11 workspaces that declare it, Postgres 18.6-alpine, dependency floor refreshed. apps/app moves from hand-wired createRoute() to TanStack Router's file-based routing via an explicit virtualRouteConfig, keeping every feature slice where it was; the 40 route ids are unchanged and the 32 pages merged into their route files, cutting the main chunk from 478 KB to 364 KB. Reviewed per task plus three independent pre-merge passes (security, correctness, build/CI). Two defects the plan had missed were caught and fixed before merge: routeTree.gen.ts was gitignored with nothing generating it before type-check (52 errors on a fresh clone), and the four Dockerfiles never followed the toolchain bump. Open debts recorded in HISTORY.md: the Stripe API version moved with the SDK and no CI test can see it (verify against a real account before a production deploy), and the better-auth family is pinned exact at 1.6.30 until the SCIM 1.7.x rewrite is migrated. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
English and French, with `fr-BE` collapsing to `fr` the same way i18next's `load: "languageOnly"` does, so resolution and the library never disagree about which catalog a browser language maps to.
Catalogs are `.ts … as const`, not JSON: TypeScript widens JSON string values to `string`, which silently disables interpolation typing, and a variable-path dynamic import across a workspace specifier is not analysable by Rolldown. `CustomTypeOptions.resources` binds to the English catalog, so an unknown key is a compile error at every call site. `loadCatalog`'s own `Resources` type widens every string leaf so both locale catalogs (necessarily different literal values) satisfy the same shape; the strict literal binding lives only in `CustomTypeOptions`, where it is what makes `t()` reject unknown keys. The parity test runs under `turbo run test`, which pre-push already executes, and fails on the symmetric difference between the two key sets.
The catalog is awaited before the first render so no language flash is possible, and only the active locale's catalog is fetched. `<html lang>` follows `languageChanged`, and the a11y suite asserts it: axe's `html-has-lang` passes on a stale `lang="en"` whatever the content says, so the rule engine alone would never catch a broken sync.
…lient The a11y suite compared lang against /^(en|fr)$/ and never forced a fr run, so a stale lang="en" passed trivially; it now asserts equality against the locale each run seeds via the locale cookie, with one fr-forced case added. initI18n's top-level await had no error path; a rejected catalog fetch now reports to telemetry and retries against DEFAULT_LOCALE, re-throwing only if that retry also fails.
`user.locale` stays nullable so "never chose" remains distinguishable from "chose English" — a NOT NULL default would erase that, and it cannot be recovered afterwards. The field is exposed with `input: false` so the only writer is the route that emits the domain event. `email_message.locale` is nullable because every row in flight at deploy time will have none; the worker falls back to the default locale.
Catalog 80 -> 81, public 34 -> 35. `previousLocale` is explicitly nullable because the column is, and §7 wants that stated rather than implied. Subject and actor coincide — the route carries `denyImpersonated` — so `userId` needs no separate `actorUserId`.
A dedicated module rather than the `auth-queries.ts` plain-function precedent: this is a route-owned write, so rule #8 applies in full and the store carries constructor-injected instrumentation with spans on both methods. `denyImpersonated` is what lets the event payload treat subject and actor as the same person. Also declares `@packages/i18n` as a real dependency of `apps/api` - required to import `Locale`/`LOCALES`/`isLocale`, previously missing from the plan.
…e session The server value wins when it exists; when it does not, the browser-resolved locale is persisted once. Without that write a user who never opens their settings keeps a null locale forever and reads a French UI while every email arrives in English. The reconciliation and the switcher both skip the write during impersonation, matching the denyImpersonated guard on PUT /me/locale.
`SendTemplateOptions.locale` is removed rather than activated: one options
object serves N recipients, and the two genuinely multi-recipient callers
are exactly the ones that must not be forced into a single language.
The subject is rendered during enqueue and stored on the row, so the locale
has to arrive there too — passing it only to the worker would have shipped
translated bodies under English subjects. Rows predating the column fall
back to English. Organization invitations use the inviter's locale.
Also fixes a `mock.module("@packages/emails", ...)` leak in the delivery
worker test (process-global per bun test, documented in shared/CLAUDE.md)
that would otherwise mask the new email-locale.test.ts assertions in the
full suite: the worker never reads the mocked renderer's subject, so it
now exercises the real renderer instead of a stub.
…, and i18n fallback Three review findings closed: the BetterAuth verify/reset/change-email hooks had a recipient locale available on `user` and never read it, the notification-digest flush never selected `user.locale` so every digest shipped in English, and `createI18n` declared an English fallback that never actually loaded English resources — so a key missing from a non-English catalog would have rendered as a raw key, not silently as English, defeating the premise partial translations rely on.
BetterAuth errors stop surfacing the raw server string and go through the same code-keyed store as the API errors, so there is one translation store rather than two. The Zod map is re-applied on every language change: z.config holds one map process-wide, and a stale closure would keep emitting the previous language. Every real caller of formatApiError/resolveAuthError is updated so the build stays green with the new t-carrying signatures, including the non-component call sites (toast.ts, the global query-error-handler) which read the active i18next instance via a small accessor since they run outside the React tree. The i18n package now exposes its catalog subpaths so tests can build a t() fixture directly from the English catalog.
toLocaleDateString()/toLocaleString() use the browser locale, so a French UI could still render American dates. formatDate/formatDateTime now take an explicit locale and every call site (the fourteen from the plan plus four more formatDate() callers found using the same pattern) passes i18n.language, so date formatting always follows the active UI language.
Per-issue message literals on schema checks and refines always win over z.config's customError, so 17 inline `message:` strings across the auth, account, security and webhooks schemas were silently shadowing the localized error map — a French UI still read "Password is required" on an empty sign-in password. Removed the literals, extended the global map to cover the constraint shapes they used to hardcode (too_small/too_big by origin, invalid_format for url/other, and a params.i18nKey escape hatch for refine()-based custom checks), and added the matching English and French catalog keys. Also pin TZ=UTC in utils.test.ts: the 14:05Z fixture instant straddles a day boundary at UTC+14/UTC-12, which is a latent CI flake in a zero-failures gate.
The consent sentence uses named `<Trans>` tags rather than numbered ones so French word order is free to differ from English. Every key lands in the English catalog first — it is the type source, so a call site cannot reference a key that does not exist there yet.
… keys Map the data-rights link to the innermost element instead of NavLink's Radix Slot, which threw on the Trans-injected text child. Also split the command-palette theme hint, the verify-email copy, and the emailField placeholder off keys they had been silently borrowing.
…omponent The round-1 test hand-copied the Trans composition instead of importing it, so a regression in the production file would leave it green. Extract the notice into its own exported DataRightsNotice component and have the test render that import directly, mocking only the router Link.
The cookie page claimed no functional cookies existed, which the locale cookie made false. It is filed as necessary, not functional: a language preference is consent-exempt, and filing it otherwise would imply it can be refused — gating it would break resolution for anyone who declines.
`/settings/account` was the one screen where the accepted partial-translation state read as broken rather than as work in progress: `ProfileCard`, `LanguageCard` and `ChangePasswordCard` in French, with `PasskeysCard`, `TwoFactorCard`, `RecoveryCodesCard` and the deletion card in English directly beneath them — on the very page that hosts the language switcher. Those four cards move together with the dialogs they open, the forms inside them and the toasts their hooks raise, since a French card that toasts English is the same defect one interaction later. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
The validation rejection is an `AppErrorException` now, and only Hono's `onError` turns one into a 400 — a bare sub-router reports it as an unhandled 500. Mounting the routes the way `index.ts` does is what makes the test assert the contract the client actually sees. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
broadcastAuthChange() reset the just-picked locale marker on every one of its ~25 call sites, most of which refresh the same person's session (profile save, avatar upload, org switch, ...). That let a language switch get silently reverted by the very next unrelated mutation, since the session refetch still read BetterAuth's cookie-cached row. Add an identityChanged flag, defaulting to false, so only sign-out and the impersonation start/stop switch clear the marker - locally and across tabs via the broadcast payload. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
Last remaining isLocale(...) ? ... : DEFAULT_LOCALE ternary that the toLocale() helper was promoted to replace. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
validator.ts's zV throws AppErrorException({ code: "REQUEST_INVALID" }),
not HTTPException(400). toastError's raw-server-message fallback is used
as a last resort for uncataloged 4xx responses, not never.
Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
E.1a i18n foundation: typed en/fr catalogs, cookie + user-record locale resolution, language switcher, localized errors, validation, dates and per-recipient emails. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
ROADMAP gains a shipped milestone row for E.1a and its "what's left" entry becomes E.1b (remaining extraction) instead of the pre-decision "locale routes + Lingui" framing. README, OVERVIEW and MODULES carried that same stale framing and are corrected; OVERVIEW gains a guided-tour section for the feature. Event counts refreshed to 81 / 35 public / 46 internal — EVENTS.md and FEATURES.md were still on 80 and 67 respectively. MODULES moves E.1a into the shipped table with its subtotals rebalanced. HISTORY's dated `SendTemplateOptions.locale?` entry gets the reversal note the repo already uses for superseded decisions. Claude-Session: https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
IEmailQueue.enqueue returned Result<void, EmailQueueError>, so a batch that
suppressed every row via onConflictDoNothing was indistinguishable from one
that wrote every row. flush-notification-emails.route.ts and rgpd.service.ts
both commit follow-up state (emailSentAt, the wipe transaction) on that
boolean alone. enqueue now returns Result<{ written: number }, ...> so
callers can see how many rows actually landed; a short write still succeeds
(a suppressed duplicate is not a failure) and still logs its existing
warning.
Also resets `insertReturns` in a beforeEach so a failing assertion in one
enqueue test can no longer leak mock state into the next test.
The unit suite mocks onConflictDoNothing as a no-op and asserts on a return
value the test itself chooses, so deleting .onConflictDoNothing(...) from
DrizzleEmailQueue.enqueue leaves it green. Add check-enqueue.ts (wired as
check:enqueue), modelled on check-marksent.ts / check-wipe-rollback.ts, to
prove against a real database that: a mixed batch of {existing key, fresh
key} inserts only the fresh row and reports written: 1; the conflict target
is idempotency_key and not the primary key; and two NULL idempotency keys
both insert, since Postgres does not treat NULL = NULL as a conflict under
a UNIQUE index.
executeAccountWipe's catch block reported ACCOUNT_WIPE_NOTIFY_PROVIDER_FAILURE whenever notifyFailure was set, regardless of what was actually caught. If Postgres fails to ROLLBACK itself (e.g. connection loss) after the sentinel throw, a different error surfaces from the transaction — that path was silently mapped to the notify-failure code with no instrumentation.capture, losing the infra signal. Gate the branch on the sentinel (e instanceof Error && e.message === "rollback"); anything else now falls through to the existing capture-and-report path.
Three inaccuracies in the docs/HISTORY.md entry: markSent uses inArray (IN (...)), not WHERE id = ANY(...); six route files were migrated onto the promoted sweep runner (the five runRetentionSweep callers plus sweep-audit-log, which had been bypassing it via runBatchedSweep), not five; and the digest flush route already keyed on a digest of the full row-id list, not a per-call timestamp. Also records the separator change (/ -> #) as the one-time cross-deploy dedup window it actually is, bounded by EMAIL_MESSAGE_RETENTION_DAYS=7, rather than describing it as cosmetic. docs/FEATURES.md's retention sweep line is reworded to say the cutoff is measured from created_at (enqueue time), not from the sent/failed transition — a failed row's age at purge time tracks the retry ceiling (minutes) closely in practice but is not literally "kept 90 days".
…k locks ALTER TABLE ... ADD CONSTRAINT takes an ACCESS EXCLUSIVE lock on the shared email_message table with no bound on the wait, no guard against pointing it at a non-local database, and its temporary constraint could survive a SIGKILL. Add SET lock_timeout before the ALTER so a held lock fails fast instead of hanging, refuse to run when DATABASE_URL is not localhost/ 127.0.0.1, and add a header note to stop the dev API worker first.
sweep-email-messages.test.ts mocked ../sweep-runner without runBatchedSweep, MAX_BATCHES or INTER_BATCH_SLEEP_MS. bun runs test files in parallel and mock.module leaks process-wide, so a partial mock here was latent only because the sibling sweep-runner.test.ts happens to import the real module first — reordering tests could surface a missing-export error in an unrelated file. Expose the full export surface so this can never happen.
The sweep gained a second pass purging failed rows past EMAIL_MESSAGE_FAILED_RETENTION_DAYS (default 90d), but CRON.md and INTEGRATIONS.md still claimed failed rows were kept forever. Document both passes and both cutoff bases (sent_at vs created_at) so an on-call engineer reading these at 3am gets accurate retention info.
check-enqueue.ts and check-marksent.ts wrote real rows through the real code path with no protection against a shared DATABASE_URL — check-enqueue.ts in particular inserts status: "pending" rows that a live EmailDeliveryWorker would pick up and actually attempt to send. Extract the guard already present in check-wipe-rollback.ts into a shared require-local-database.ts helper (third occurrence) and use it in all three scripts.
DrizzleEmailQueue.enqueue reports { written } specifically so callers
can tell a fully-suppressed enqueue from a real one, but
QueuedEmailService.enqueue only checked result.isFailure and discarded
written — a 0-of-N write was indistinguishable from success at every
call site, including the RGPD deletion confirmation, which would
commit the account wipe believing the notice had been queued.
Return Result.fail (reusing EMAIL_PROVIDER_FAILURE, since both current
consumers already act correctly on that code) when rows.length > 0 and
written === 0. A partial suppression (0 < written < rows.length) stays
success — some rows were genuinely duplicates, already logged as a
warning at the queue level.
This changes RGPD behavior: a fully-suppressed confirmation enqueue
now rolls back the wipe instead of committing it. Verified against a
real Postgres with check:wipe-rollback; rgpd.service.test.ts needed no
changes since it mocks IEmailService directly and already asserts on
isFailure/EMAIL_PROVIDER_FAILURE.
|
🎉 This PR is included in version 1.24.0 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the three D.5 email-queue debts from
ROADMAP.md, plus two defects that design review surfaced — one of them live in production.What review found that the roadmap didn't
The first design draft was wrong about the mechanism. It claimed the
email_message.idempotency_keyUNIQUE constraint closed the duplicate-email window. It does not: the constraint protects an INSERT, while the failure was a re-send of an already-inserted row (the worker re-claims it after the 120 s claim window lapses — no INSERT occurs). What actually closes it is the provider-sideIdempotency-Keyheader.And that header was broken.
chunkIdempotencyKeytruncated the joined keys to 256 characters. The notification digest flush already passes real keys, ~66 chars each, so past ~4 rows per chunk two different sends produced the same key — Resend returned the cached response of the first,sendChunksaw no error, and every recipient of the second was markedsentwithout ever being sent. Silent mail loss, live today.The RGPD confirmation could be lost permanently. The wipe committed in its own transaction and the confirmation was sent after the whole batch loop. Since
findUsersReadyForWipefiltersisNull(deletedAt), a crash in between lost that user's deletion confirmation forever — the inverse of the documented duplicate, and worse for a mandatory notice.Changes
fix(email)chunkIdempotencyKeyhashes instead of truncatingfix(email)enqueuesuppresses duplicates instead of failing the batch, warns on a short write, and reportswrittenfix(email)#on both batch pathsfix(rgpd)throw new Error("rollback")conventionrefactor(sweep)runRetentionSweeppromoted to N passes; six route files migratedfix(email)failedrows purge on their own 90-day cutoff with a dedicated partial indexperf(email)markSentcollapses from N UPDATEs to oneUPDATE … CASE WHENdocsDrizzle knows nothing about
Result<T, E>—db.transactionCOMMITs whenever the callback resolves and ROLLBACKs only when it throws. Returning a failure would have wiped the account, reported it as failed, and —deletedAtnow set — never retried it.Verification
Three of these changes live in SQL a mocked
txcannot evaluate (apps/api/src/shared/CLAUDE.md:41), so each got a script proving it against real Postgres rather than a test asserting on generated SQL:pnpm --filter api check:enqueue— the ON CONFLICT targetsidempotency_key(not the PK), NULL keys stay distinct, a mixed duplicate+fresh batch inserts the fresh rowpnpm --filter api check:marksent— theCASEmaps distinct provider ids, falls back to NULL, and incrementsattempts2→3 (not 0→1, which a wrongly-hardcoded1would also pass)pnpm --filter api check:wipe-rollback— a failing enqueue genuinely leavesdeleted_atNULLPlus
pnpm --filter api test749/749,type-check,ci:check,knip,jscpd— all green.No new event types. Catalog stays 81 / 35 public / 46 internal, verified by importing
visibility-map.ts.One debt recorded rather than fixed
Neither the sweep routes nor the cron client sets an explicit timeout —
internalLayerssets none,Bun.servehas noidleTimeout,signedInternalFetchissues a barefetchwith noAbortSignal. Doubling the sweep passes brings that closer to mattering. Found during review, left unfixed rather than widened in silence; it is now a known debt inROADMAP.md.https://claude.ai/code/session_01GHPzZiA3cHT9XBdoZrjMmY
Scope note, after review
This closes the truncation collision — two different sends producing the same provider key. It does not close every double-send path:
chunkIdempotencyKeystill returnsOption.none()when any row in a chunk lacks a key, so a chunk containing an unkeyed row goes to the provider with noIdempotency-Keyand a slow provider plus a lapsed claim window can still double-send it. That is pre-existing behaviour, unchanged here, and most callers now do carry keys — but the framing above should not be read as closing the class.