mirror of
https://github.com/stack-auth/stack.git
synced 2026-07-20 21:29:36 +08:00
Fix recurring production Sentry errors (#1768)
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 <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## 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. <!-- end of auto-generated comment: release notes by coderabbit.ai -->
This commit is contained in:
parent
32daff06f5
commit
afa2d74ba0
@ -64,6 +64,7 @@ export const resetPasswordVerificationCodeHandler = createVerificationCodeHandle
|
||||
data: {
|
||||
password,
|
||||
},
|
||||
allowedErrorTypes: [KnownErrors.UserNotFound],
|
||||
});
|
||||
|
||||
|
||||
|
||||
@ -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] } => {
|
||||
|
||||
118
apps/backend/src/lib/external-db-sync-queue.test.ts
Normal file
118
apps/backend/src/lib/external-db-sync-queue.test.ts
Normal file
@ -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);
|
||||
}
|
||||
});
|
||||
});
|
||||
@ -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
|
||||
`;
|
||||
}
|
||||
|
||||
@ -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,
|
||||
|
||||
@ -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",
|
||||
<some fields may have been hidden>,
|
||||
},
|
||||
}
|
||||
`);
|
||||
});
|
||||
|
||||
it.todo("should not be able to reset password if password authentication is disabled on the project after the verification code was sent");
|
||||
|
||||
it.todo("should not be able to reset password if email authentication is disabled on the user after the verification code was sent");
|
||||
|
||||
Loading…
Reference in New Issue
Block a user