improvement(billing): team upgrade + session management #2751
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
icecrasher321
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
PR Overview
This PR aims to improve billing reliability by adding proper error handling for team subscription upgrades and implementing session management to set the active organization ID when users upgrade to a team plan.
Key Changes
organization.ts:
- Refactored imports from
import * as schemato named imports (improves code readability and follows established patterns) - Added session management to
createOrganizationWithOwnerto update user sessions with the new organization ID - Added session management to
ensureOrganizationForTeamSubscriptionto update sessions when using an existing organization
auth.ts:
- Wrapped
ensureOrganizationForTeamSubscriptioncalls in error handlers in both subscription webhooks onSubscriptionComplete: Re-throws errors to signal Stripe webhook failure for retryonSubscriptionUpdate: Catches and logs errors but continues execution
Critical Issues Found
1. Race Condition in Session Updates (organization.ts, line 76-80)
The session update in createOrganizationWithOwner is performed outside the database transaction. If the transaction succeeds but the session update fails, the organization exists but user sessions aren't updated. In Stripe webhook handlers with retries, this can lead to orphaned organizations or repeated creation attempts.
2. Non-Transactional Subscription + Session Updates (organization.ts, line 164-172)
In ensureOrganizationForTeamSubscription, the subscription referenceId update and session activeOrganizationId update are separate operations. A failure between them leaves the subscription pointing to the organization but user sessions pointing elsewhere, creating an inconsistent state.
3. Asymmetric Error Handling (auth.ts, line 2227-2243)
The onSubscriptionUpdate handler differs from onSubscriptionComplete by catching and ignoring organization creation failures instead of triggering a webhook retry. This creates a scenario where:
- First subscription event fails and retries (Complete handler)
- Subsequent updates fail silently without retry (Update handler)
- Subscriptions remain in inconsistent state with userId as referenceId while organization-based operations execute
These issues can cause subscriptions to be left with incomplete organization associations, leading to billing and UX problems in team upgrade scenarios.
Confidence Score: 2/5
- This PR has critical transaction and atomicity issues that can cause data inconsistency in team upgrade scenarios, particularly in Stripe webhook handlers. Not safe to merge in current form.
- The PR introduces three significant logic errors related to transaction boundaries and error handling consistency. Session updates are performed outside database transactions in two locations, creating race conditions in Stripe webhook handlers where retries are expected. Additionally, the asymmetric error handling between onSubscriptionComplete (which retries) and onSubscriptionUpdate (which silently continues) can leave subscriptions in an inconsistent state. These issues directly impact the reliability of team billing operations, which is the stated goal of this PR.
- Both files require attention: (1) apps/sim/lib/billing/organization.ts needs transactional wrapping of all related database operations including session updates, (2) apps/sim/lib/auth/auth.ts needs consistent error handling between subscription event handlers.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/lib/billing/organization.ts | 2/5 | Refactored imports and added session management to organization creation. However, session updates are performed outside transactions, creating race conditions in webhook handlers where failures trigger retries. Two critical atomicity issues: (1) Session update outside transaction in createOrganizationWithOwner (line 76), (2) Subscription and session updates not transactional in ensureOrganizationForTeamSubscription (lines 164-172). |
| apps/sim/lib/auth/auth.ts | 2/5 | Added error handling for organization creation in subscription webhooks. Good: onSubscriptionComplete re-throws to trigger Stripe retries. Concern: onSubscriptionUpdate catches errors silently without retrying, creating asymmetric error handling that can leave subscriptions in inconsistent state if org creation fails during updates. |
Sequence Diagram
sequenceDiagram
participant Stripe
participant Webhook as Webhook Handler
participant EnsureOrg as ensureOrganizationForTeamSubscription
participant CreateOrg as createOrganizationWithOwner
participant DB as Database
participant Sessions as Session Table
Stripe->>Webhook: Subscription Complete/Update Event
Webhook->>EnsureOrg: ensureOrganizationForTeamSubscription(subscription)
EnsureOrg->>DB: Check existing memberships
DB-->>EnsureOrg: No existing org
EnsureOrg->>CreateOrg: createOrganizationForTeamPlan()
CreateOrg->>DB: BEGIN TRANSACTION
CreateOrg->>DB: INSERT organization
CreateOrg->>DB: INSERT member
CreateOrg->>DB: COMMIT TRANSACTION
Note over CreateOrg,DB: ISSUE: Session update outside transaction!
CreateOrg->>Sessions: UPDATE activeOrganizationId<br/>(NOT IN TRANSACTION)
alt Sessions update succeeds
Sessions-->>CreateOrg: OK
else Sessions update fails
Sessions-->>CreateOrg: ERROR
Note over CreateOrg: Org created but sessions not updated!
end
CreateOrg-->>EnsureOrg: orgId
Note over EnsureOrg,DB: ISSUE: Multiple non-transactional updates
EnsureOrg->>DB: UPDATE subscription.referenceId
EnsureOrg->>DB: UPDATE session.activeOrganizationId
alt Update succeeds
EnsureOrg-->>Webhook: resolvedSubscription
else First update succeeds, second fails
Note over EnsureOrg: Subscription → org, but sessions not updated!
end
Webhook->>DB: syncSubscriptionUsageLimits(resolvedSubscription)
DB-->>Webhook: Limits synced
alt onSubscriptionComplete path
Note over Webhook: ERROR: RE-THROW<br/>Stripe will RETRY
else onSubscriptionUpdate path
Note over Webhook: ERROR: LOG & CONTINUE<br/>No retry!
end
Additional Comments (1)
This asymmetry creates a state consistency issue. If organization creation fails during the first subscription event (complete), the retry will attempt to fix it. But if a subsequent update happens and org creation fails again:
For updates to team plans, consider whether the same retry-on-error pattern should apply to maintain consistency with the Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/lib/auth/auth.ts
Line: 2227:2243
Comment:
The error handling in `onSubscriptionUpdate` differs from `onSubscriptionComplete` in a problematic way. When `ensureOrganizationForTeamSubscription` fails:
1. In `onSubscriptionComplete`: error is re-thrown, triggering Stripe webhook retry
2. In `onSubscriptionUpdate`: error is caught and logged, but execution continues with the original subscription
This asymmetry creates a state consistency issue. If organization creation fails during the first subscription event (complete), the retry will attempt to fix it. But if a subsequent update happens and org creation fails again:
- The subscription may have a userId as `referenceId` (not an org ID)
- The error is silently logged but not retried
- `syncSubscriptionUsageLimits` receives the original subscription with wrong referenceId
- Usage limits sync for the user instead of the organization
For updates to team plans, consider whether the same retry-on-error pattern should apply to maintain consistency with the `onSubscriptionComplete` behavior. If not retrying is intentional, the function should validate that `resolvedSubscription.referenceId` actually starts with 'org_' before proceeding with organization-specific operations.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
Overview
This PR adds error handling and session management improvements for team subscription upgrades:
-
In
auth.ts: WrapsensureOrganizationForTeamSubscriptioncalls in try-catch blocks in bothonSubscriptionCompleteandonSubscriptionUpdatehandlers with explicit error logging. -
In
organization.ts:- Refactors imports from wildcard to named imports (improves tree-shaking)
- Adds session updates in
createOrganizationWithOwner()to set the new organization as active - Adds session updates when using existing organization membership
- Improves code organization with transaction wrappers
Issues Found
Critical Issues
-
Error Handling Asymmetry (auth.ts, line 2224-2236): The error from
ensureOrganizationForTeamSubscriptionis not re-thrown inonSubscriptionUpdate, unlike inonSubscriptionComplete. This means Stripe won't retry if organization creation fails during an update, leading to inconsistent subscription state where the referenceId could be a userId instead of an organization ID. -
Transaction Atomicity Gap (organization.ts, line 196-205): When creating a new organization, session updates occur inside a transaction, but the subsequent subscription referenceId update happens outside any transaction. This creates a consistency gap where sessions point to a new organization but the subscription record still has the old userId if the update fails.
Medium Issues
- Session Assignment Logic (auth.ts, line 346-380): The session creation hook retrieves only the first organization without considering subscription context. Users who log in after a team subscription upgrade may be assigned a different organization than the one they subscribed to, since the code doesn't prioritize or select based on subscription status.
Testing Notes
The PR mentions "tested manually" but the critical error handling asymmetry suggests the failure cases for organization creation during subscription updates may not have been fully tested.
Recommendations
- Re-throw errors in
onSubscriptionUpdateto maintain consistency withonSubscriptionComplete - Wrap subscription updates in the same transaction as organization creation for atomicity
- Consider improving session assignment to respect subscription relationships
Confidence Score: 2/5
- This PR is NOT safe to merge without fixes. Critical error handling asymmetry prevents Stripe webhook retries on subscription update failures, creating potential for data inconsistency.
- Score of 2/5 reflects multiple critical issues: (1) Error handling in
onSubscriptionUpdatedoesn't re-throw, breaking retry mechanism that exists inonSubscriptionComplete, (2) Transaction atomicity gap between session and subscription updates creates consistency vulnerability, (3) Session assignment logic may assign wrong organization to new sessions created after upgrade. While the overall PR intent is good, the execution has significant reliability and correctness issues that need to be addressed before merging. - Both files require attention:
apps/sim/lib/auth/auth.tsneeds critical error handling fix inonSubscriptionUpdate, andapps/sim/lib/billing/organization.tsneeds transaction wrapping improvements to ensure atomicity of subscription updates.
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/lib/auth/auth.ts | 2/5 | Added try-catch wrapper around ensureOrganizationForTeamSubscription in both onSubscriptionComplete and onSubscriptionUpdate. However, critical asymmetry: onSubscriptionComplete re-throws errors (triggering Stripe retry), but onSubscriptionUpdate silently swallows them. This prevents retry on failure during updates. Additionally, the session creation hook picks the first organization without considering subscription context, which could assign wrong org to new sessions created after upgrade. |
| apps/sim/lib/billing/organization.ts | 2/5 | Refactored to use named imports and added session updates when creating/linking organizations. Good improvements to atomicity with transactions in two places. However, there's a consistency gap: when creating a new organization, the session updates happen inside createOrganizationWithOwner transaction, but the subscription referenceId update (lines 202-205) happens outside any transaction. This creates a window where data could be inconsistent if the subscription update fails. Additionally, the PR aims to update sessions to set active organization, but the implementation is incomplete - only existing sessions are updated, new sessions will get whatever organization the query returns first. |
Sequence Diagram
sequenceDiagram
participant S as Stripe
participant H as Handler
participant O as Organization Module
participant D as DB
S->>H: subscription update event
H->>O: ensure organization
alt Organization Creation Fails
O-->>D: org creation fails
O-->>H: throw error
H->>H: catch error
Note over H: No re-throw
H->>D: sync limits
Note over S: No retry
else Success Path
O->>D: create org
O->>D: update sessions TX
O-->>H: return org ID
H->>D: update subscription
Note over H: Sessions in TX<br/>Subscription not in TX
end
Additional Comments (1)
Although there is code in organization.ts that updates existing sessions to the correct organization (lines 76-82 and 174-176), this only affects sessions that existed before the subscription update. A user who logs in AFTER the subscription is upgraded may get assigned a different organization than the one they just paid for. This could be improved by:
Consider whether the session should be set to a specific organization based on subscription status rather than just taking the first organization. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/lib/auth/auth.ts
Line: 346:380
Comment:
Potential issue with session creation hook: The `session.create.before` hook (line 349) retrieves only the FIRST organization the user is a member of using `.limit(1)`. While the organization.ts file updates EXISTING sessions when a subscription changes, newly created sessions after a team subscription will get whichever organization comes first from the database query, not necessarily the organization linked to the active subscription.
Although there is code in organization.ts that updates existing sessions to the correct organization (lines 76-82 and 174-176), this only affects sessions that existed before the subscription update. A user who logs in AFTER the subscription is upgraded may get assigned a different organization than the one they just paid for.
This could be improved by:
1. Ordering the query to prioritize organizations with active team subscriptions (if needed)
2. Or, storing the subscription's preferred organization on the user or session object
Consider whether the session should be set to a specific organization based on subscription status rather than just taking the first organization.
How can I resolve this? If you propose a fix, please make it concise. |
There was a problem hiding this comment.
Greptile Overview
Greptile Summary
This PR improves reliability for team subscription upgrades by adding proper error handling and session management. The changes ensure that organization creation failures trigger Stripe webhook retries, and user sessions are updated atomically when upgrading to a team plan.
Major Changes:
- Added try-catch with error logging and re-throw in
onSubscriptionCompleteandonSubscriptionUpdateto enable Stripe retry on org creation failure - Wrapped session updates in transactions alongside org/member creation for atomic updates
- Refactored imports from wildcard
schema.*to named imports for better tree-shaking
Issues Found:
- Critical: Subscription
referenceIdupdate happens outside the transaction after org creation (line 202-205 in organization.ts), creating potential for orphaned organizations - Critical: Race condition on concurrent team upgrades - multiple requests could pass the membership check and violate the
member_user_id_uniqueconstraint - Minor: Session creation hook selects first org without ordering, which may not pick the org with active subscription
- Minor: Variable shadowing where loop variable
membershadows the importedmembertable - Minor: Sessions created during the transaction window won't get
activeOrganizationIdset until next login
Confidence Score: 2/5
- Not safe to merge without addressing critical transaction and race condition issues
- While the error handling improvements are good, there are two critical logic issues that could cause data inconsistency: (1) subscription update outside transaction can create orphaned organizations, and (2) concurrent upgrade requests can cause database constraint violations. The minor issues around session ordering are edge cases but should still be addressed.
- apps/sim/lib/billing/organization.ts requires immediate attention for transaction boundary and race condition fixes
Important Files Changed
File Analysis
| Filename | Score | Overview |
|---|---|---|
| apps/sim/lib/auth/auth.ts | 3/5 | Added error handling with retry for team subscription org creation in webhook handlers. Minor issue: session.create.before hook doesn't order orgs when selecting activeOrganizationId. |
| apps/sim/lib/billing/organization.ts | 2/5 | Added session updates in transactions for team upgrades. Critical issues: subscription update outside transaction, race condition on concurrent upgrades, variable shadowing in loop. |
Sequence Diagram
sequenceDiagram
participant User
participant Stripe
participant Auth
participant Org
participant DB
User->>Stripe: Upgrade to Team
Stripe->>Auth: Webhook
Auth->>Org: ensureOrganization
alt Has membership
Org->>DB: Update subscription
Org->>DB: Update sessions
else No membership
Org->>DB: Create org
Org->>DB: Create member
Org->>DB: Update sessions
Org->>DB: Update subscription
end
Org->>Auth: Success
Auth->>Stripe: Complete
Additional Comments (1)
Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/lib/auth/auth.ts
Line: 350:354
Comment:
No ordering on organization selection for new sessions. The query picks the FIRST organization a user is a member of (`.limit(1)`) without any ordering. If a user is member of multiple orgs (e.g., edge case or future feature), this may not select the org with the active team subscription. Consider ordering by subscription status or creation date, or storing the "primary" org ID on the user record.
How can I resolve this? If you propose a fix, please make it concise. |
Greptile OverviewGreptile SummaryThis PR improves the reliability and user experience of team plan upgrades by addressing two key areas: Changes Made
Issues Found
Overall AssessmentThe changes successfully implement the stated goals of making team upgrades more reliable through Stripe retries and improving UX through session refresh. The transaction-based approach for organization creation is a significant improvement in consistency. However, the missing re-throw in Confidence Score: 3/5
Important Files ChangedFile Analysis
Sequence DiagramsequenceDiagram
participant User
participant Browser
participant Frontend
participant Stripe
participant Webhook
participant Backend
participant Database
User->>Frontend: Click Upgrade to Team
Frontend->>Frontend: Check existing organization
alt Has organization
Frontend->>Backend: Set organization active
else No organization
Note over Frontend: Will create org after payment
end
Frontend->>Stripe: Redirect with success URL
User->>Stripe: Complete payment
Stripe->>Browser: Redirect with upgrade flag
par Frontend Processing
Browser->>Frontend: Page loads with flag
Frontend->>Frontend: Remove flag from URL
Frontend->>Backend: Fetch session (bypass cache)
and Webhook Processing
Stripe->>Webhook: Subscription event
Webhook->>Backend: Process subscription
Backend->>Database: Check membership
alt Create new organization
Backend->>Database: Transaction start
Backend->>Database: Create org and member
Backend->>Database: Update sessions
Backend->>Database: Transaction commit
else Use existing organization
Backend->>Database: Transaction start
Backend->>Database: Update subscription
Backend->>Database: Update sessions
Backend->>Database: Transaction commit
end
Backend->>Database: Sync usage limits
Webhook->>Stripe: Success response
end
Backend-->>Frontend: Fresh session with org
Frontend->>Frontend: Invalidate cached queries
Frontend->>Frontend: Render updated UI
|
Additional Comments (1)
In contrast, when This inconsistency means:
Consider re-throwing the error after logging to ensure Stripe retries the webhook until usage limits are successfully synced. Prompt To Fix With AIThis is a comment left during a code review.
Path: apps/sim/lib/auth/auth.ts
Line: 2240:2248
Comment:
Error handling inconsistency: If `syncSubscriptionUsageLimits` fails, the error is caught and logged but NOT re-thrown. This means the Stripe webhook will return HTTP 200 (success) even though the usage limit sync failed.
In contrast, when `ensureOrganizationForTeamSubscription` fails (line 2237), the error is re-thrown, triggering Stripe to retry the webhook.
This inconsistency means:
- Organization creation failures → webhook retried (good)
- Usage limit sync failures → webhook succeeds, limits never synced (bad)
Consider re-throwing the error after logging to ensure Stripe retries the webhook until usage limits are successfully synced.
```suggestion
try {
await syncSubscriptionUsageLimits(resolvedSubscription)
} catch (error) {
logger.error('[onSubscriptionUpdate] Failed to sync usage limits', {
subscriptionId: resolvedSubscription.id,
referenceId: resolvedSubscription.referenceId,
error,
})
throw error // Re-throw to trigger Stripe retry
}
```
How can I resolve this? If you propose a fix, please make it concise. |
Summary
Type of Change
Testing
Tested manually
Checklist