Repository navigation
Use the shared Pro seat release when turning a seat off - #3157
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| ), | ||
| ); | ||
| ) | ||
| .for("update"); |
There was a problem hiding this comment.
When two organizations turn off the same user's seats at once, each transaction first locks its own membership row. This new locking read then needs the row held by the other transaction. Each can wait for the other, so MySQL aborts one transaction. toggleProSeat does not retry, leaving one seat enabled and returning an error.
Serialize changes for the same user before locking individual memberships, or retry the whole transaction after a deadlock.
Prompt To Fix With AI
This is a comment left during a code review.
Path: packages/database/pro-seat-subscription.ts
Line: 30
Comment:
**Concurrent seat removals can fail**
When two organizations turn off the same user's seats at once, each transaction first locks its own membership row. This new locking read then needs the row held by the other transaction. Each can wait for the other, so MySQL aborts one transaction. `toggleProSeat` does not retry, leaving one seat enabled and returning an error.
Serialize changes for the same user before locking individual memberships, or retry the whole transaction after a deadlock.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| }, | ||
| ]); | ||
| mockDb.limit.mockResolvedValueOnce([]); | ||
| mockDb.limit.mockReturnValueOnce(mockDb); |
There was a problem hiding this comment.
The updated test still returns no membership, so it never reaches the new releaseProSeatSubscription call. These tests can pass even if declining an invite fails to move Pro onto a remaining paid seat.
Add a case with hasProSeat: true and a remaining paid seat. Extend the mock to support the helper's joins, then assert that the subscription changes while the invite and membership are deleted.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/web/__tests__/unit/invite-decline.test.ts
Line: 157
Comment:
**Invite tests skip Pro release**
The updated test still returns no membership, so it never reaches the new `releaseProSeatSubscription` call. These tests can pass even if declining an invite fails to move Pro onto a remaining paid seat.
Add a case with `hasProSeat: true` and a remaining paid seat. Extend the mock to support the helper's joins, then assert that the subscription changes while the invite and membership are deleted.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
@greptileai review |
This comment has been minimized.
This comment has been minimized.
| const proSeatUserIds = [ | ||
| ...new Set( | ||
| memberRows | ||
| .filter((member) => member.hasProSeat) | ||
| .map((member) => member.userId), | ||
| ), | ||
| ].sort(); | ||
| for (const proSeatUserId of proSeatUserIds) { | ||
| await releaseProSeatSubscription(tx, proSeatUserId); |
There was a problem hiding this comment.
Org deletion locks user rows in inconsistent order across its two loops
Medium Severity
The real defect is the unsorted affectedUserIds update loop in @packages/web-backend/src/Organisations/index.ts (lines 284-317): its per-user update(Db.users) takes user-row X locks in insertion order (org owner first, then membership-row order), while the new sorted release loop above it locks the same rows in ascending id order through the SELECT ... FROM users ... FOR UPDATE in releaseProSeatSubscription. With two concurrent organization deletions sharing two users whose orders oppose — org A has Pro-seat members U and V so its release loop locks U then V, while org B's affectedUserIds lists V before U (V joined first, or V is the org owner) — transaction 1 holds U and waits for V's lock, and transaction 2's affected-user update holds V and waits for U, forming an InnoDB deadlock; one deletion transaction is rolled back and the whole delete fails. This flow purges the org's S3 media before the DB transaction, so the failed deletion leaves the organization and its video rows alive with their files already deleted. The base had no seat release in this flow at all, so the sorted user-row locking here is new. Fix by making the whole transaction acquire user rows in one global order: sort affectedUserIds the same way as proSeatUserIds, or take all needed user-row locks up front in sorted order before either loop.
…moval transaction
|
@greptileai review |
# Conflicts: # .github/workflows/ci.yml
Follow-up to #3064. That PR added
releaseProSeatSubscriptionand used it for the two org-removal paths. Every other path that takes a Pro seat away still had its own copy of the old logic.What was wrong
toggleProSeat(dashboard) and the agent APIupdateOrganizationMemberSeathandler, when turning a seat off:.limit(1), with no ordering;stripeSubscriptionId, setthirdPartyStripeSubscriptionIdto null.So a user who still had a manager-sponsored seat lost Pro. A user with a sponsored seat and a paid seat could also lose Pro, depending on which row came back first. The remaining-seat reads were also plain reads, so a concurrent seat change could be missed.
Two more paths removed seats without going through the shared logic:
Changes
releaseProSeatSubscriptionto@cap/database/pro-seat-subscriptionsopackages/web-backendcan use it too. It now locks the user row with.for("update")before it reads the remaining seats. That serializes releases for one user. Locking the remaining seat rows instead would deadlock two orgs turning off the same user's seats at once. Every removal path now deletes or turns off the seat and calls the helper before its first plain read. Under REPEATABLE READ, an earlier plain read pins the snapshot, and the helper would miss a release that committed while it waited on the user lock. Remove-member (both the action and the agent API), invite decline and org deletion were reordered for this. The final state of each is unchanged.hasProSeat: false, then call the helper.Unchanged:
No Stripe calls are touched.
Tests
New
toggle-pro-seat.test.tscovers turning a seat off with:It also covers turning a seat on: assign, out of seats, and no active subscription. On the old code, the sponsored, mixed and lock cases fail.
invite-decline.test.tsmock updated for the locked membership read.New
invite-decline-pro-seat.test.tscovers declining with a Pro seat: last seat, manager-sponsored seat remaining, paid seat remaining, and the row locks.toggle-pro-seat,invite-decline-pro-seat,remove-organization-member(from fix(web): revoke Pro access when removing the last organization seat #3064) andinvite-declineare added to the CI web test list.
No new blocking findings were added, and both numbered previous findings are addressed.Summary
Routes Pro seat removal through the shared database helper.
Both numbered findings from the previous review are addressed. No new comments were added.
Reviews (3) · Last reviewed commit: "fix(web): release Pro seats before the f..." · Reviewed by Greptile