From afa2d74ba0e5ffb9f821976c58772fcb32904ebd Mon Sep 17 00:00:00 2001 From: BilalG1 Date: Thu, 16 Jul 2026 17:20:49 -0700 Subject: [PATCH] Fix recurring production Sentry errors (#1768) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes four recurring production Sentry issues (triaged from the last 24h of `vercel-production`): ## X OAuth: 403 client-not-enrolled → 400 (STACK-BACKEND-1JE) When a customer's X developer App isn't attached to an X API Project, X deterministically rejects `GET /2/users/me` with 403 `client-not-enrolled` even though the token exchange succeeds. This previously surfaced as a `HexclaveAssertionError` (opaque 500 + Sentry alert) on every sign-in attempt for the misconfigured project. It's now a 400 `StatusError` telling the developer to attach their App to a Project, mirroring the existing `invalid_client` handling in `providers/base.tsx`. Other non-OK statuses still fail loud. ## Stripe: ignore `refund.updated` (STACK-BACKEND-1D9) Stripe emits `refund.updated` asynchronously when the card network fills in `destination_details` on refunds we created — and already ledgered — synchronously in the internal refund route, so it carries no actionable information. Added it to `ignoredEvents` (same approach as #1667/#1461). `refund.failed` deliberately stays fail-loud, since a refund failing after it's ledgered is actionable. ## external-db-sync: fix enqueue deadlocks (STACK-BACKEND-16P) `enqueueExternalDbSyncBatch` batch-inserts deduplication keys with `ON CONFLICT DO NOTHING`; two concurrent batches inserting overlapping keys in different orders could deadlock (Postgres 40P01, surfaced as an empty-titled `PrismaClientKnownRequestError`). Worse than the noise: the flag-clearing UPDATE and the enqueue run in separate transactions, so a deadlocked enqueue silently lost that batch's sync trigger until the next change for the tenancy. Fixed by deduplicating and inserting keys in canonical (sorted) order, so the circular wait between two batches can't form. Deliberately minimal: deadlocks against other `OutgoingRequest` writers (e.g. `recoverStaleOutgoingRequests`) are theoretically still possible but not evidenced in the Sentry events; if 40P01 reappears, a retry can be added then. ## Password reset: 404 instead of 500 for deleted users (STACK-BACKEND-1JD) Password reset codes embed the `user_id` at issuance time, so a code can be redeemed after the user was deleted (observed in prod from a scripted client replaying a reset code). The `usersCrudHandlers.adminUpdate` call didn't declare `UserNotFound` via `allowedErrorTypes`, so the CRUD handler wrapped it in `CrudHandlerInvocationError` and the endpoint returned an internal 500. Now declares it and lets the client-safe `USER_NOT_FOUND` KnownError propagate as a 404, matching the ~10 sibling call sites that already do this. ## Testing - New e2e test: resetting the password of a deleted user returns 404 `USER_NOT_FOUND` (`auth/password/reset.test.ts`) — verified passing locally against the full stack - Backend + e2e typecheck and lint pass - No new tests for the other three: the ignore-list entry is covered by `satisfies Stripe.Event.Type[]` and the existing unknown-type webhook tests; the X fix would need the X API base URL to be injectable plus a mock user-info endpoint (possible follow-up); the deadlock is a Postgres timing race that can't be deterministically reproduced through the e2e API surface ## Summary by CodeRabbit * **Bug Fixes** * Improved password reset handling when the account no longer exists. * Added clearer guidance for X API configuration errors. * Prevented unsupported Stripe refund update events from triggering processing. * Ensured external database synchronization requests are deduplicated and consistently ordered. * **Tests** * Added coverage for password reset attempts involving deleted accounts. --- .../reset/verification-code-handler.tsx | 1 + .../integrations/stripe/webhooks/route.tsx | 1 + .../src/lib/external-db-sync-queue.test.ts | 118 ++++++++++++++++++ .../backend/src/lib/external-db-sync-queue.ts | 5 +- apps/backend/src/oauth/providers/x.tsx | 5 +- .../api/v1/auth/password/reset.test.ts | 32 +++++ 6 files changed, 160 insertions(+), 2 deletions(-) create mode 100644 apps/backend/src/lib/external-db-sync-queue.test.ts diff --git a/apps/backend/src/app/api/latest/auth/password/reset/verification-code-handler.tsx b/apps/backend/src/app/api/latest/auth/password/reset/verification-code-handler.tsx index fdda33955..f8f428647 100644 --- a/apps/backend/src/app/api/latest/auth/password/reset/verification-code-handler.tsx +++ b/apps/backend/src/app/api/latest/auth/password/reset/verification-code-handler.tsx @@ -64,6 +64,7 @@ export const resetPasswordVerificationCodeHandler = createVerificationCodeHandle data: { password, }, + allowedErrorTypes: [KnownErrors.UserNotFound], }); diff --git a/apps/backend/src/app/api/latest/integrations/stripe/webhooks/route.tsx b/apps/backend/src/app/api/latest/integrations/stripe/webhooks/route.tsx index 5c0927881..f291b5a3d 100644 --- a/apps/backend/src/app/api/latest/integrations/stripe/webhooks/route.tsx +++ b/apps/backend/src/app/api/latest/integrations/stripe/webhooks/route.tsx @@ -55,6 +55,7 @@ const ignoredEvents = [ "payout.created", "payout.paid", "payout.reconciliation_completed", + "refund.updated", ] as const satisfies Stripe.Event.Type[]; const isSubscriptionChangedEvent = (event: Stripe.Event): event is Stripe.Event & { type: (typeof subscriptionChangedEvents)[number] } => { diff --git a/apps/backend/src/lib/external-db-sync-queue.test.ts b/apps/backend/src/lib/external-db-sync-queue.test.ts new file mode 100644 index 000000000..a168d9a8c --- /dev/null +++ b/apps/backend/src/lib/external-db-sync-queue.test.ts @@ -0,0 +1,118 @@ +import { randomUUID } from "node:crypto"; +import { afterEach, describe, expect, it } from "vitest"; +import { globalPrismaClient } from "@/prisma-client"; +import { enqueueExternalDbSync, enqueueExternalDbSyncBatch } from "./external-db-sync-queue"; + +const DEDUP_PREFIX = "sentinel-sync-key-"; + +// Track every tenancy ID a test enqueues so we can delete the rows afterwards. +// These tests run against the shared dev database, and leftover pending rows +// would be picked up by the poller (which would then fail on the nonexistent +// tenancies and pollute logs). +const enqueuedTenancyIds: string[] = []; + +function freshTenancyIds(count: number): string[] { + const ids = Array.from({ length: count }, () => randomUUID()); + enqueuedTenancyIds.push(...ids); + return ids; +} + +async function findRowsForTenancies(tenancyIds: string[]) { + return await globalPrismaClient.outgoingRequest.findMany({ + where: { deduplicationKey: { in: tenancyIds.map((id) => DEDUP_PREFIX + id) } }, + orderBy: { deduplicationKey: "asc" }, + }); +} + +afterEach(async () => { + await globalPrismaClient.outgoingRequest.deleteMany({ + where: { deduplicationKey: { in: enqueuedTenancyIds.splice(0).map((id) => DEDUP_PREFIX + id) } }, + }); +}); + +describe("enqueueExternalDbSyncBatch (real DB)", () => { + it("inserts one pending row per tenancy with the expected qstash options", async ({ expect }) => { + const [tenancyId] = freshTenancyIds(1); + + await enqueueExternalDbSync(tenancyId); + + const rows = await findRowsForTenancies([tenancyId]); + expect(rows).toHaveLength(1); + expect(rows[0].startedFulfillingAt).toBeNull(); + expect(rows[0].qstashOptions).toMatchObject({ + url: "/api/latest/internal/external-db-sync/sync-engine", + body: { tenancyId }, + flowControl: { key: "sentinel-sync-key", parallelism: 20 }, + }); + }); + + it("produces the same rows regardless of input order and deduplicates within the batch", async ({ expect }) => { + const ids = freshTenancyIds(5); + const shuffled = [...ids].reverse(); + // Duplicates within one call must collapse to a single row per tenancy. + const withDuplicates = [...shuffled, ...ids, ids[2]]; + + await enqueueExternalDbSyncBatch(withDuplicates); + + const rows = await findRowsForTenancies(ids); + expect(rows).toHaveLength(ids.length); + expect(new Set(rows.map((r) => r.deduplicationKey))).toEqual(new Set(ids.map((id) => DEDUP_PREFIX + id))); + }); + + it("skips tenancies that already have a pending row, even when re-enqueued in a different order", async ({ expect }) => { + const ids = freshTenancyIds(4); + + await enqueueExternalDbSyncBatch(ids); + await enqueueExternalDbSyncBatch([...ids].reverse()); + + const rows = await findRowsForTenancies(ids); + expect(rows).toHaveLength(ids.length); + }); + + it("enqueues a new pending row once the previous one has been claimed", async ({ expect }) => { + const [tenancyId] = freshTenancyIds(1); + + await enqueueExternalDbSync(tenancyId); + // Simulate the poller claiming the row: the partial unique index only + // covers rows WHERE startedFulfillingAt IS NULL, so a claimed row must not + // block a fresh sync request for the same tenancy. + await globalPrismaClient.outgoingRequest.updateMany({ + where: { deduplicationKey: DEDUP_PREFIX + tenancyId }, + data: { startedFulfillingAt: new Date() }, + }); + + await enqueueExternalDbSync(tenancyId); + + const rows = await findRowsForTenancies([tenancyId]); + expect(rows).toHaveLength(2); + expect(rows.filter((r) => r.startedFulfillingAt === null)).toHaveLength(1); + }); + + it("rejects non-UUID tenancy IDs", async ({ expect }) => { + await expect(enqueueExternalDbSyncBatch(["not-a-uuid"])).rejects.toThrow("tenancyId must be a valid UUID"); + await expect(enqueueExternalDbSyncBatch([randomUUID(), "'; DROP TABLE \"OutgoingRequest\"; --"])).rejects.toThrow("tenancyId must be a valid UUID"); + }); + + // Regression test for a production deadlock (SQLSTATE 40P01): two concurrent + // batches inserting overlapping tenancies in different orders acquired the + // partial unique index locks in opposite orders. The fix canonicalizes the + // insert order (JS sort + SQL ORDER BY), so concurrent batches always lock + // in the same order and can never deadlock against each other. This test + // hammers exactly that scenario; without the sort it deadlocks flakily, + // with it a deadlock is impossible, so the test cannot be flaky post-fix. + it("does not deadlock when concurrent batches enqueue the same tenancies in opposite orders", async ({ expect }) => { + const ITERATIONS = 10; + const BATCH_SIZE = 50; + + for (let i = 0; i < ITERATIONS; i++) { + const ids = freshTenancyIds(BATCH_SIZE); + await Promise.all([ + enqueueExternalDbSyncBatch(ids), + enqueueExternalDbSyncBatch([...ids].reverse()), + ]); + + const rows = await findRowsForTenancies(ids); + expect(rows).toHaveLength(BATCH_SIZE); + } + }); +}); diff --git a/apps/backend/src/lib/external-db-sync-queue.ts b/apps/backend/src/lib/external-db-sync-queue.ts index cbd0415e1..700e50b17 100644 --- a/apps/backend/src/lib/external-db-sync-queue.ts +++ b/apps/backend/src/lib/external-db-sync-queue.ts @@ -24,6 +24,8 @@ export async function enqueueExternalDbSyncBatch(tenancyIds: string[]): Promise< assertUuid(id, "tenancyId"); } + const sortedTenancyIds = [...new Set(tenancyIds)].sort(); + // Use unnest to pass array of UUIDs and insert all in one query await globalPrismaClient.$executeRaw` INSERT INTO "OutgoingRequest" ("id", "createdAt", "qstashOptions", "startedFulfillingAt", "deduplicationKey") @@ -37,7 +39,8 @@ export async function enqueueExternalDbSyncBatch(tenancyIds: string[]): Promise< ), NULL, 'sentinel-sync-key-' || t.tenancy_id - FROM unnest(${tenancyIds}::uuid[]) AS t(tenancy_id) + FROM unnest(${sortedTenancyIds}::uuid[]) AS t(tenancy_id) + ORDER BY t.tenancy_id ON CONFLICT ("deduplicationKey") WHERE "startedFulfillingAt" IS NULL DO NOTHING `; } diff --git a/apps/backend/src/oauth/providers/x.tsx b/apps/backend/src/oauth/providers/x.tsx index a48223d5f..634a5e4d8 100644 --- a/apps/backend/src/oauth/providers/x.tsx +++ b/apps/backend/src/oauth/providers/x.tsx @@ -1,4 +1,4 @@ -import { HexclaveAssertionError } from "@hexclave/shared/dist/utils/errors"; +import { HexclaveAssertionError, StatusError } from "@hexclave/shared/dist/utils/errors"; import { OAuthUserInfo, validateUserInfo } from "../utils"; import { OAuthBaseProvider, TokenSet } from "./base"; @@ -34,6 +34,9 @@ export class XProvider extends OAuthBaseProvider { ); if (!fetchRes.ok) { const text = await fetchRes.text(); + if (fetchRes.status === 403 && text.includes("client-not-enrolled")) { + throw new StatusError(400, "X rejected the user info request because the X developer App is not attached to an X API Project. Attach the App to a Project in the X developer portal (https://developer.x.com/en/docs/projects/overview), then try signing in again."); + } throw new HexclaveAssertionError(`Failed to fetch user info from X: ${fetchRes.status} ${text}`, { status: fetchRes.status, text, diff --git a/apps/e2e/tests/backend/endpoints/api/v1/auth/password/reset.test.ts b/apps/e2e/tests/backend/endpoints/api/v1/auth/password/reset.test.ts index 8299898ca..4ffd25a87 100644 --- a/apps/e2e/tests/backend/endpoints/api/v1/auth/password/reset.test.ts +++ b/apps/e2e/tests/backend/endpoints/api/v1/auth/password/reset.test.ts @@ -163,6 +163,38 @@ it("should be able to check the password reset code without using it", async ({ `); }); +it("should return USER_NOT_FOUND if the user was deleted after the reset code was sent", async ({ expect }) => { + const { userId } = await Auth.Password.signUpWithEmail(); + await Auth.signOut(); + const resetCode = await getResetCode(); + const deleteResponse = await niceBackendFetch(`/api/v1/users/${userId}`, { + method: "DELETE", + accessType: "server", + }); + expect(deleteResponse.status).toBe(200); + const response = await niceBackendFetch("/api/v1/auth/password/reset", { + method: "POST", + accessType: "client", + body: { + code: resetCode, + password: "this-is-a-new-password", + }, + }); + expect(response).toMatchInlineSnapshot(` + NiceResponse { + "status": 404, + "body": { + "code": "USER_NOT_FOUND", + "error": "User not found.", + }, + "headers": Headers { + "x-stack-known-error": "USER_NOT_FOUND", +