fix: changed generation of multiple checkout link to one, so that the user doesnt accidently pays multiple times - #48
Conversation
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
📝 WalkthroughWalkthroughThis PR introduces a checkout session management feature that reuses existing sessions within 24 hours to avoid repeated link generation, tracks session completion through payment webhooks, makes storage handlers idempotent for resilience, and improves error context propagation. ChangesCheckout Session Management
Sequence Diagram(s)sequenceDiagram
participant Client
participant createCheckoutLink as gRPC Endpoint
participant db as Postgres DB
Client->>createCheckoutLink: Request checkout link
createCheckoutLink->>db: Query existing session (24h, is_completed=false)
alt Found active session
db-->>createCheckoutLink: Return existing link
createCheckoutLink-->>Client: Return cached link
else No active session
createCheckoutLink->>createCheckoutLink: Generate link & proxyUrl
createCheckoutLink->>db: Upsert checkout_sessions (ON CONFLICT)
db-->>createCheckoutLink: Confirm
createCheckoutLink-->>Client: Return new link
end
sequenceDiagram
participant Webhook as Payment Webhook
participant Transaction
participant db as Postgres DB
Webhook->>Transaction: Begin transaction
Transaction->>db: Update checkout_sessions.is_completed = true (WHERE user_id=? AND is_completed=false)
db-->>Transaction: Confirm
Transaction->>db: Update user billed_at
db-->>Transaction: Confirm
Transaction->>db: Record payment via handleAddPayment
db-->>Transaction: Confirm
Transaction-->>Webhook: Commit
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint skipped: no ESLint configuration detected in root package.json. To enable, add Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/routes/gRPC/payment/createCheckoutLink.ts`:
- Line 76: The TTL math mixes DB now() and app time; in createCheckoutLink.ts
update the upsert/update path to set checkoutSessionsTable.created_at using the
database current time (DB now()) instead of DateTime.utc() so both reads and
writes use the same clock; locate the upsert/update that writes created_at
(references to checkoutSessionsTable and createdAt/created_at in the
createCheckoutLink function) and replace the app-side DateTime.utc() assignment
with a DB-side now()/CURRENT_TIMESTAMP expression so the 24-hour comparison
(sql`${checkoutSessionsTable.createdAt} > now() - interval '24 hours'`) is
consistent.
- Around line 69-85: The current existence check using checkoutSessionsTable and
validatedData.userId happens outside the provider call and can race; wrap the
flow in a Drizzle transaction and acquire a user-scoped advisory lock (or
otherwise serialize by userId) before creating the provider session: inside the
transaction/lock re-run the same select on checkoutSessionsTable for
validatedData.userId to short-circuit if a payable session now exists, otherwise
either insert a reserved row (status=reserved) to claim the slot before calling
the external provider or create the provider session then upsert within the same
transaction; additionally catch and handle unique constraint violations on the
checkoutSessionsTable upsert to return the existing link via
CreateCheckoutLinkResponse and callback when conflicts occur.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 160-201: The batch INSERT uses onConflictDoNothing which can
silently drop rows that share idempotencyKey across different models; update
addAiTokenUsage handler to explicitly detect and handle unique-constraint
conflicts by (1) replacing the blind success check on inserted (variables:
inserted, aiTokenUsageValues, aiTokenUsageEventsTable, txn) with a verification
that inserted.length === aiTokenUsageValues.length, (2) if lengths differ, query
the DB for all idempotency keys in the batch (use txn.select(...)
.where(in(...)) on aiTokenUsageEventsTable.idempotencyKey) to reconcile which
rows already exist, and (3) either perform an upsert (onConflictDoUpdate) keyed
on the correct unique constraint that includes model/user as needed or return a
clear StorageError listing the missing/duplicated idempotencyKeys; ensure you
reference buildAiTokenInsertValues output to map returned/existing rows back to
the original events instead of only checking the first idempotencyKey.
In `@src/storage/adapter/postgres/handlers/addBasicUsage.ts`:
- Around line 58-69: The current fallback accepts any row with the same
idempotencyKey as an idempotent success; instead, when the txn.select(...) on
basicUsageEventsTable returns existing, explicitly validate that that existing
row's critical fields (e.g., event type/enum, owner_id, usage metrics,
timestamp/period) match the incoming event_data values before treating it as
success; if they match, set resultId = existing.id, but if they differ, throw a
clear conflict StorageError (do not silently drop the new record) so callers
know an idempotency key was reused with different payloads; ensure this logic is
applied where existing is read and references variables basicUsageEventsTable,
event_data, existing, resultId, and follow unique constraint handling guidance
in the surrounding transaction.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: c532d883-d24a-4021-889f-e04c4bac06cb
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locksrc/gen/event/v1/event_pb.jsis excluded by!**/gen/**
📒 Files selected for processing (8)
drizzle/0001_add_checkout_sessions.sqlpackage.jsonsrc/errors/storage.tssrc/routes/gRPC/payment/createCheckoutLink.tssrc/routes/http/createdCheckout.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addBasicUsage.tssrc/storage/db/postgres/schema.ts
| const [existing] = await db | ||
| .select() | ||
| .from(checkoutSessionsTable) | ||
| .where( | ||
| and( | ||
| eq(checkoutSessionsTable.userId, validatedData.userId), | ||
| eq(checkoutSessionsTable.isCompleted, false), | ||
| sql`${checkoutSessionsTable.createdAt} > now() - interval '24 hours'` | ||
| ) | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (existing) { | ||
| const response = new CreateCheckoutLinkResponse(); | ||
| response.setCheckoutlink(existing.link); | ||
| return callback?.(null, response); | ||
| } |
There was a problem hiding this comment.
Non-atomic reuse flow can still create multiple payable checkout sessions.
Line 69-85 does the existence check before the provider call, but Line 95-123 creates the provider session and upserts afterward. Concurrent requests for the same user can both miss the row and both create provider checkout sessions, so duplicate payable links are still possible.
Please serialize this path per user (transaction + user-scoped lock/advisory lock, then re-check inside lock before provider creation), or reserve state before the external call.
As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly."
Also applies to: 95-123
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/gRPC/payment/createCheckoutLink.ts` around lines 69 - 85, The
current existence check using checkoutSessionsTable and validatedData.userId
happens outside the provider call and can race; wrap the flow in a Drizzle
transaction and acquire a user-scoped advisory lock (or otherwise serialize by
userId) before creating the provider session: inside the transaction/lock re-run
the same select on checkoutSessionsTable for validatedData.userId to
short-circuit if a payable session now exists, otherwise either insert a
reserved row (status=reserved) to claim the slot before calling the external
provider or create the provider session then upsert within the same transaction;
additionally catch and handle unique constraint violations on the
checkoutSessionsTable upsert to return the existing link via
CreateCheckoutLinkResponse and callback when conflicts occur.
| and( | ||
| eq(checkoutSessionsTable.userId, validatedData.userId), | ||
| eq(checkoutSessionsTable.isCompleted, false), | ||
| sql`${checkoutSessionsTable.createdAt} > now() - interval '24 hours'` |
There was a problem hiding this comment.
Use a single clock source for checkout-session TTL math.
Line 76 compares against DB now(), while Line 122 updates created_at from app time (DateTime.utc()). Clock skew between app and DB can make the 24-hour reuse window inconsistent.
Prefer setting created_at via DB now() in the upsert update too.
Also applies to: 122-122
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/routes/gRPC/payment/createCheckoutLink.ts` at line 76, The TTL math mixes
DB now() and app time; in createCheckoutLink.ts update the upsert/update path to
set checkoutSessionsTable.created_at using the database current time (DB now())
instead of DateTime.utc() so both reads and writes use the same clock; locate
the upsert/update that writes created_at (references to checkoutSessionsTable
and createdAt/created_at in the createCheckoutLink function) and replace the
app-side DateTime.utc() assignment with a DB-side now()/CURRENT_TIMESTAMP
expression so the 24-hour comparison (sql`${checkoutSessionsTable.createdAt} >
now() - interval '24 hours'`) is consistent.
| const aiTokenUsageValues = buildAiTokenInsertValues( | ||
| aggregatedEvents, | ||
| auth | ||
| ); | ||
|
|
||
| const inserted = await txn | ||
| .insert(aiTokenUsageEventsTable) | ||
| .values(aiTokenUsageValues) | ||
| .onConflictDoNothing({ | ||
| target: aiTokenUsageEventsTable.idempotencyKey, | ||
| }) | ||
| .returning({ id: aiTokenUsageEventsTable.id }); | ||
|
|
||
| if (!inserted[0] || !inserted[0].id) { | ||
| throw StorageError.insertFailed( | ||
| "Missing or invalid ID for the first inserted event", | ||
| new Error(`Invalid first event ID: ${JSON.stringify(inserted[0])}`) | ||
| ); | ||
| const firstIdempotencyKey = aiTokenUsageValues[0]?.idempotencyKey; | ||
| if (!firstIdempotencyKey) { | ||
| throw StorageError.insertFailed( | ||
| "Missing idempotency key in the first event", | ||
| new Error("First event idempotency key is undefined") | ||
| ); | ||
| } | ||
|
|
||
| const [existing] = await txn | ||
| .select({ id: aiTokenUsageEventsTable.id }) | ||
| .from(aiTokenUsageEventsTable) | ||
| .where( | ||
| eq(aiTokenUsageEventsTable.idempotencyKey, firstIdempotencyKey) | ||
| ) | ||
| .limit(1); | ||
|
|
||
| if (!existing) { | ||
| throw StorageError.insertFailed( | ||
| "Missing or invalid ID for the first inserted event, and no existing record found", | ||
| new Error( | ||
| `Invalid first event ID: ${JSON.stringify(inserted[0])}` | ||
| ) | ||
| ); | ||
| } | ||
| return { id: existing.id }; | ||
| } | ||
|
|
||
| return { id: inserted[0].id }; |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In PostgreSQL / Drizzle ORM, when a multi-row INSERT ... ON CONFLICT DO NOTHING RETURNINGcontains duplicate values for the conflict target, are the conflicting rows omitted fromRETURNING, leaving only successfully inserted rows?
💡 Result:
Yes. In PostgreSQL, with INSERT ... ON CONFLICT DO NOTHING RETURNING ..., only rows that are actually inserted (or updated) are returned; rows for which the DO NOTHING alternative action is taken produce no RETURNING output [1]. Therefore, if your multi-row VALUES list contains duplicates for the conflict target, the rows that conflict (including duplicates that would conflict with an already-inserted row in the same statement) are omitted from RETURNING, leaving only the rows that successfully insert. Drizzle ORM’s onConflictDoNothing().returning(...) ultimately uses PostgreSQL’s semantics, so the returned array corresponds to PostgreSQL’s RETURNING result (empty for conflicts) [2][3].
Citations:
- 1: https://www.postgresql.org/docs/current/sql-insert.html
- 2: [BUG]: using
returningwithonConflictDoNothingdoes not return anything drizzle-team/drizzle-orm#2474 - 3: https://orm.drizzle.team/docs/insert
🏁 Script executed:
# Find the aggregateAiTokenEvents function and check its grouping logic
rg -A 20 "function aggregateAiTokenEvents|const aggregateAiTokenEvents" src/storage/adapter/postgres/handlers/addAiTokenUsage.tsRepository: ScrawnDotDev/Scrawn
Length of output: 1078
🏁 Script executed:
# Also check what the buildAiTokenInsertValues function does
rg -B 5 -A 15 "function buildAiTokenInsertValues|const buildAiTokenInsertValues" src/storage/adapter/postgres/handlers/addAiTokenUsage.tsRepository: ScrawnDotDev/Scrawn
Length of output: 630
🏁 Script executed:
# Check the table schema to understand the unique constraint
rg -B 5 -A 5 "aiTokenUsageEventsTable" src/ --type ts | grep -A 10 "idempotencyKey"Repository: ScrawnDotDev/Scrawn
Length of output: 3202
onConflictDoNothing silently drops rows with duplicate idempotencyKey within the same batch, causing data loss.
The aggregation at line 69 groups by ${userId}:${model}:${idempotencyKey}, so multiple rows can share the same idempotencyKey when they have different models. When the batch insert uses ON CONFLICT DO NOTHING (lines 168–170), PostgreSQL silently omits those conflicting rows from the RETURNING result, leaving the handler with an incomplete insert and a false success response. The fallback logic (lines 176–201) only checks the first idempotencyKey and does not validate that all expected rows were inserted.
This violates the coding guideline: "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly."
Proposed guard until aggregation and uniqueness are aligned
const aiTokenUsageValues = buildAiTokenInsertValues(
aggregatedEvents,
auth
);
+
+ const seenIdempotencyKeys = new Set<string>();
+ for (const value of aiTokenUsageValues) {
+ if (seenIdempotencyKeys.has(value.idempotencyKey)) {
+ throw StorageError.constraintViolation(
+ `Duplicate AI token usage idempotencyKey in batch: ${value.idempotencyKey}`
+ );
+ }
+ seenIdempotencyKeys.add(value.idempotencyKey);
+ }
const inserted = await txn
.insert(aiTokenUsageEventsTable)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts` around lines 160 -
201, The batch INSERT uses onConflictDoNothing which can silently drop rows that
share idempotencyKey across different models; update addAiTokenUsage handler to
explicitly detect and handle unique-constraint conflicts by (1) replacing the
blind success check on inserted (variables: inserted, aiTokenUsageValues,
aiTokenUsageEventsTable, txn) with a verification that inserted.length ===
aiTokenUsageValues.length, (2) if lengths differ, query the DB for all
idempotency keys in the batch (use txn.select(...) .where(in(...)) on
aiTokenUsageEventsTable.idempotencyKey) to reconcile which rows already exist,
and (3) either perform an upsert (onConflictDoUpdate) keyed on the correct
unique constraint that includes model/user as needed or return a clear
StorageError listing the missing/duplicated idempotencyKeys; ensure you
reference buildAiTokenInsertValues output to map returned/existing rows back to
the original events instead of only checking the first idempotencyKey.
| const [existing] = await txn | ||
| .select({ id: basicUsageEventsTable.id }) | ||
| .from(basicUsageEventsTable) | ||
| .where(eq(basicUsageEventsTable.idempotencyKey, event_data.idempotencyKey)) | ||
| .limit(1); | ||
|
|
||
| if (!existing) { | ||
| throw StorageError.emptyResult( | ||
| "Basic usage event insert returned no ID and no existing record found" | ||
| ); | ||
| } | ||
| resultId = existing.id; |
There was a problem hiding this comment.
Validate the conflicting row before treating it as idempotent success.
Lines 58-69 accept any existing row with the same idempotencyKey. If a different BASIC_USAGE event ever reuses that key, this path silently drops the new usage record and returns success. For billing data, that should be a hard mismatch, not an idempotent replay.
Proposed mismatch check on the fallback path
const [existing] = await txn
- .select({ id: basicUsageEventsTable.id })
+ .select({
+ id: basicUsageEventsTable.id,
+ eventId: basicUsageEventsTable.eventId,
+ userId: basicUsageEventsTable.userId,
+ type: basicUsageEventsTable.type,
+ debitAmount: basicUsageEventsTable.debitAmount,
+ })
.from(basicUsageEventsTable)
.where(eq(basicUsageEventsTable.idempotencyKey, event_data.idempotencyKey))
.limit(1);
if (!existing) {
throw StorageError.emptyResult(
"Basic usage event insert returned no ID and no existing record found"
);
}
+
+ if (
+ existing.userId !== event_data.userId ||
+ existing.eventId !== event_data.eventId ||
+ existing.type !== event_data.data.basicUsageType ||
+ existing.debitAmount !== event_data.data.debitAmount
+ ) {
+ throw StorageError.constraintViolation(
+ `basic usage idempotency mismatch for key ${event_data.idempotencyKey}`
+ );
+ }
resultId = existing.id;As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/adapter/postgres/handlers/addBasicUsage.ts` around lines 58 - 69,
The current fallback accepts any row with the same idempotencyKey as an
idempotent success; instead, when the txn.select(...) on basicUsageEventsTable
returns existing, explicitly validate that that existing row's critical fields
(e.g., event type/enum, owner_id, usage metrics, timestamp/period) match the
incoming event_data values before treating it as success; if they match, set
resultId = existing.id, but if they differ, throw a clear conflict StorageError
(do not silently drop the new record) so callers know an idempotency key was
reused with different payloads; ensure this logic is applied where existing is
read and references variables basicUsageEventsTable, event_data, existing,
resultId, and follow unique constraint handling guidance in the surrounding
transaction.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/storage/adapter/postgres/handlers/addAiTokenUsage.ts (1)
165-198:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReconcile every conflicted key, not just the first one.
ON CONFLICT DO NOTHINGcan return a partialinsertedarray here. When that happens, this fallback only loadsaiTokenUsageValues[0].idempotencyKey, so later rows in the batch can be dropped silently, and a reused key with different aggregate data would still be treated as success. Compareinserted.lengthwithaiTokenUsageValues.length, load all conflicted keys when they differ, and validate the existing rows match the incoming aggregates before returning success.As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts` around lines 165 - 198, The current fallback after .onConflictDoNothing only checks the first aiTokenUsageValues item and may silently drop later conflicted rows; update the logic in addAiTokenUsage (referencing aiTokenUsageValues, inserted, aiTokenUsageEventsTable, and txn) to compare inserted.length to aiTokenUsageValues.length, and if they differ collect all idempotency keys from aiTokenUsageValues that were not inserted, query the DB for those keys, ensure you retrieve a matching row for each missing key, and validate that each existing row's aggregate data matches the corresponding incoming aiTokenUsageValues; if any key is missing or data mismatches, throw StorageError.insertFailed with a clear message (keep using StorageError.insertFailed for failures) otherwise return the set of existing/inserted ids.src/storage/adapter/postgres/handlers/addBasicUsage.ts (1)
53-69:⚠️ Potential issue | 🟠 Major | ⚡ Quick winReject idempotency-key mismatches on the fallback path.
If a different
BASIC_USAGEpayload reuses the sameidempotencyKey, this branch still returns the existing row id and drops the new billing event. Fetch the fields that define idempotent equivalence and compare them toevent_databefore treating this as a replay; otherwise throwStorageError.constraintViolation(...).As per coding guidelines, "Use Drizzle ORM with transactions; validate all inputs before DB operations; handle unique constraint violations explicitly".
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/storage/adapter/postgres/handlers/addBasicUsage.ts` around lines 53 - 69, The fallback branch that finds an existing row by idempotencyKey must verify the row is actually the same event: query the same set of columns used to determine idempotent equivalence (the columns you include when inserting a BASIC_USAGE event — e.g., the org/project/feature/timestamp/amount fields or whatever columns are used for equivalence) from basicUsageEventsTable via txn (use the same where eq(basicUsageEventsTable.idempotencyKey, event_data.idempotencyKey)), compare each fetched column to the corresponding value on event_data, and only treat it as a replay and set resultId = existing.id when all fields match; if any field differs, throw StorageError.constraintViolation(...) with a clear message; also validate event_data fields before DB ops per guidelines. Ensure you reference basicUsageEventsTable, event_data, txn, and StorageError.constraintViolation in the change.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/storage/db/postgres/schema.ts`:
- Around line 254-265: The schema stores only a mutable link so webhooks cannot
target a specific provider checkout — add a stable provider identifier column
(e.g. providerCheckoutId or sessionId) to this table and make it either a string
column or a foreign key referencing sessionsTable.id; update the table
definition alongside existing fields (id, userId, link, createdAt, isCompleted)
and add the corresponding migration to populate/backfill values, then change
webhook/completion logic to mark isCompleted on the row matching the
providerCheckoutId (or sessionsTable FK) rather than matching by userId or link
so the exact checkout session is completed.
- Around line 251-271: The checkout_sessions schema currently only scopes active
uniqueness by userId; add a new column "mode" to checkoutSessionsTable (e.g.,
text or enum as appropriate) and make it non-null (and default if desired), then
update the uniqueActiveCheckout uniqueIndex ("unique_active_checkout_per_user")
to include table.mode in its .on(...) clause so the partial unique constraint
becomes userId+mode where isCompleted = false; update any references to
checkoutSessionsTable or uniqueActiveCheckout accordingly.
---
Duplicate comments:
In `@src/storage/adapter/postgres/handlers/addAiTokenUsage.ts`:
- Around line 165-198: The current fallback after .onConflictDoNothing only
checks the first aiTokenUsageValues item and may silently drop later conflicted
rows; update the logic in addAiTokenUsage (referencing aiTokenUsageValues,
inserted, aiTokenUsageEventsTable, and txn) to compare inserted.length to
aiTokenUsageValues.length, and if they differ collect all idempotency keys from
aiTokenUsageValues that were not inserted, query the DB for those keys, ensure
you retrieve a matching row for each missing key, and validate that each
existing row's aggregate data matches the corresponding incoming
aiTokenUsageValues; if any key is missing or data mismatches, throw
StorageError.insertFailed with a clear message (keep using
StorageError.insertFailed for failures) otherwise return the set of
existing/inserted ids.
In `@src/storage/adapter/postgres/handlers/addBasicUsage.ts`:
- Around line 53-69: The fallback branch that finds an existing row by
idempotencyKey must verify the row is actually the same event: query the same
set of columns used to determine idempotent equivalence (the columns you include
when inserting a BASIC_USAGE event — e.g., the
org/project/feature/timestamp/amount fields or whatever columns are used for
equivalence) from basicUsageEventsTable via txn (use the same where
eq(basicUsageEventsTable.idempotencyKey, event_data.idempotencyKey)), compare
each fetched column to the corresponding value on event_data, and only treat it
as a replay and set resultId = existing.id when all fields match; if any field
differs, throw StorageError.constraintViolation(...) with a clear message; also
validate event_data fields before DB ops per guidelines. Ensure you reference
basicUsageEventsTable, event_data, txn, and StorageError.constraintViolation in
the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: df339ea5-ed2e-4a2f-90e7-3083c1aec1bb
⛔ Files ignored due to path filters (2)
bun.lockis excluded by!**/*.locksrc/gen/event/v1/event_pb.jsis excluded by!**/gen/**
📒 Files selected for processing (8)
drizzle/0001_add_checkout_sessions.sqlpackage.jsonsrc/errors/storage.tssrc/routes/gRPC/payment/createCheckoutLink.tssrc/routes/http/createdCheckout.tssrc/storage/adapter/postgres/handlers/addAiTokenUsage.tssrc/storage/adapter/postgres/handlers/addBasicUsage.tssrc/storage/db/postgres/schema.ts
| export const checkoutSessionsTable = pgTable( | ||
| "checkout_sessions", | ||
| { | ||
| id: uuid("id").primaryKey().defaultRandom(), | ||
| userId: USER_ID_CONFIG.dbType("user_id") | ||
| .references(() => usersTable.id) | ||
| .notNull(), | ||
| link: text("link").notNull(), | ||
| createdAt: timestamp("created_at", { | ||
| withTimezone: true, | ||
| mode: "string", | ||
| }) | ||
| .defaultNow() | ||
| .notNull(), | ||
| isCompleted: boolean("is_completed").notNull().default(false), | ||
| }, | ||
| (table) => ({ | ||
| uniqueActiveCheckout: uniqueIndex("unique_active_checkout_per_user") | ||
| .on(table.userId) | ||
| .where(sql`${table.isCompleted} = false`), | ||
| }) |
There was a problem hiding this comment.
Scope active checkout uniqueness by mode.
checkout_sessions is keyed only by userId, so the reuse/completion flow cannot distinguish test from production. A test checkout for a user can be reused by a production request for the same user, and a successful payment in one mode can retire the other mode’s active row. Add mode to this table and make the partial unique index user+mode scoped instead of user-only.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/db/postgres/schema.ts` around lines 251 - 271, The
checkout_sessions schema currently only scopes active uniqueness by userId; add
a new column "mode" to checkoutSessionsTable (e.g., text or enum as appropriate)
and make it non-null (and default if desired), then update the
uniqueActiveCheckout uniqueIndex ("unique_active_checkout_per_user") to include
table.mode in its .on(...) clause so the partial unique constraint becomes
userId+mode where isCompleted = false; update any references to
checkoutSessionsTable or uniqueActiveCheckout accordingly.
| id: uuid("id").primaryKey().defaultRandom(), | ||
| userId: USER_ID_CONFIG.dbType("user_id") | ||
| .references(() => usersTable.id) | ||
| .notNull(), | ||
| link: text("link").notNull(), | ||
| createdAt: timestamp("created_at", { | ||
| withTimezone: true, | ||
| mode: "string", | ||
| }) | ||
| .defaultNow() | ||
| .notNull(), | ||
| isCompleted: boolean("is_completed").notNull().default(false), |
There was a problem hiding this comment.
Persist the paid checkout’s identity, not just its link.
This row has no stable reference to the provider checkout/session it represents. Once the row is refreshed with a newer link, the webhook can only complete by userId, so paying an older provider session will mark the current row completed and reopen link generation while the newer link is still payable. Store the provider checkout session id, or a foreign key to sessionsTable, and complete that exact record instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/storage/db/postgres/schema.ts` around lines 254 - 265, The schema stores
only a mutable link so webhooks cannot target a specific provider checkout — add
a stable provider identifier column (e.g. providerCheckoutId or sessionId) to
this table and make it either a string column or a foreign key referencing
sessionsTable.id; update the table definition alongside existing fields (id,
userId, link, createdAt, isCompleted) and add the corresponding migration to
populate/backfill values, then change webhook/completion logic to mark
isCompleted on the row matching the providerCheckoutId (or sessionsTable FK)
rather than matching by userId or link so the exact checkout session is
completed.
1 participant
This pull request introduces a new
checkout_sessionstable to manage user checkout flows, enforces idempotency in event insertions, and improves error handling for usage events. It also includes minor dependency and formatting updates. The most important changes are summarized below:Checkout Session Management:
checkout_sessionstable with a unique index to ensure only one active (not completed) session per user. This is reflected in both the SQL migration (drizzle/0001_add_checkout_sessions.sql) and the Drizzle schema (checkoutSessionsTableinsrc/storage/db/postgres/schema.ts). [1] [2]createCheckoutLinkhandler to check for an existing active session within 24 hours and reuse it, or create/update one atomically, ensuring only one active session per user. [1] [2]Summary by CodeRabbit
New Features
Bug Fixes
Chores