fix(invites): distinguish CAS contention from a real use-cap (retry vs reject)
claimInviteUse returned null both when the cap was truly reached and when it lost the compare-and-swap 5× under load, so a legitimate joiner on a busy high-cap invite got a false 'invite has reached its use cap'. Return a discriminated result; map contention to a distinct 'try again' message. Co-Authored-By: Claude Opus 4.8 (1M context) <[email protected]>
This commit is contained in:
co-authored by
Claude Opus 4.8
parent
33b2475e67
commit
61ffc2e01f
@@ -54,7 +54,7 @@ describe('claimInviteUse (invite max_uses TOCTOU fix)', () => {
|
|||||||
it('claims a use when under the cap and returns the new count', async () => {
|
it('claims a use when under the cap and returns the new count', async () => {
|
||||||
const admin = makeAdmin({ uses_count: 0, max_uses: 1 })
|
const admin = makeAdmin({ uses_count: 0, max_uses: 1 })
|
||||||
const result = await claimInviteUse(admin, 'inv-1', 0, 1)
|
const result = await claimInviteUse(admin, 'inv-1', 0, 1)
|
||||||
expect(result).toBe(1)
|
expect(result).toEqual({ ok: true, count: 1 })
|
||||||
expect(admin._row.uses_count).toBe(1)
|
expect(admin._row.uses_count).toBe(1)
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -62,7 +62,7 @@ describe('claimInviteUse (invite max_uses TOCTOU fix)', () => {
|
|||||||
const admin = makeAdmin({ uses_count: 1, max_uses: 1 })
|
const admin = makeAdmin({ uses_count: 1, max_uses: 1 })
|
||||||
// Second redeemer read uses_count=0 stale, but the row is already at 1.
|
// Second redeemer read uses_count=0 stale, but the row is already at 1.
|
||||||
const result = await claimInviteUse(admin, 'inv-1', 0, 1)
|
const result = await claimInviteUse(admin, 'inv-1', 0, 1)
|
||||||
expect(result).toBeNull()
|
expect(result).toEqual({ ok: false, reason: 'cap_reached' })
|
||||||
expect(admin._row.uses_count).toBe(1) // unchanged — not incremented past cap
|
expect(admin._row.uses_count).toBe(1) // unchanged — not incremented past cap
|
||||||
})
|
})
|
||||||
|
|
||||||
@@ -70,23 +70,67 @@ describe('claimInviteUse (invite max_uses TOCTOU fix)', () => {
|
|||||||
// max_uses=5, another redeemer already bumped it to 1 after our stale read of 0.
|
// max_uses=5, another redeemer already bumped it to 1 after our stale read of 0.
|
||||||
const admin = makeAdmin({ uses_count: 1, max_uses: 5 })
|
const admin = makeAdmin({ uses_count: 1, max_uses: 5 })
|
||||||
const result = await claimInviteUse(admin, 'inv-1', 0, 5)
|
const result = await claimInviteUse(admin, 'inv-1', 0, 5)
|
||||||
expect(result).toBe(2) // re-read to 1, then CAS-claimed to 2
|
expect(result).toEqual({ ok: true, count: 2 }) // re-read to 1, then CAS-claimed to 2
|
||||||
expect(admin._row.uses_count).toBe(2)
|
expect(admin._row.uses_count).toBe(2)
|
||||||
})
|
})
|
||||||
|
|
||||||
it('treats an unlimited invite (max_uses null) as always claimable', async () => {
|
it('treats an unlimited invite (max_uses null) as always claimable', async () => {
|
||||||
const admin = makeAdmin({ uses_count: 99, max_uses: null })
|
const admin = makeAdmin({ uses_count: 99, max_uses: null })
|
||||||
const result = await claimInviteUse(admin, 'inv-1', 99, null)
|
const result = await claimInviteUse(admin, 'inv-1', 99, null)
|
||||||
expect(result).toBe(100)
|
expect(result).toEqual({ ok: true, count: 100 })
|
||||||
})
|
})
|
||||||
|
|
||||||
it('gives up (null) if it cannot win the CAS within the attempt budget', async () => {
|
it('reports contention (not cap_reached) when it loses the CAS every attempt under a high cap', async () => {
|
||||||
|
// A perpetually-contended invite with plenty of headroom (cap 1000): every
|
||||||
|
// CAS loses because a competing writer keeps advancing the row between our
|
||||||
|
// re-read and our next update, so we never win within the attempt budget.
|
||||||
|
// The row stays well under cap, so the failure is contention — a false
|
||||||
|
// 'cap_reached' here would wrongly turn away a legitimate joiner.
|
||||||
|
const row = { uses_count: 5, max_uses: 1000 as number | null }
|
||||||
|
const admin = {
|
||||||
|
from() {
|
||||||
|
return {
|
||||||
|
update() {
|
||||||
|
const builder: Record<string, unknown> = {
|
||||||
|
eq() {
|
||||||
|
return builder
|
||||||
|
},
|
||||||
|
select() {
|
||||||
|
// Always lose the CAS: a competitor also bumped the row.
|
||||||
|
row.uses_count += 1
|
||||||
|
return Promise.resolve({ data: [] })
|
||||||
|
},
|
||||||
|
}
|
||||||
|
return builder
|
||||||
|
},
|
||||||
|
select() {
|
||||||
|
const builder: Record<string, unknown> = {
|
||||||
|
eq() {
|
||||||
|
return builder
|
||||||
|
},
|
||||||
|
maybeSingle() {
|
||||||
|
return Promise.resolve({ data: { uses_count: row.uses_count } })
|
||||||
|
},
|
||||||
|
}
|
||||||
|
return builder
|
||||||
|
},
|
||||||
|
}
|
||||||
|
},
|
||||||
|
} as unknown as Parameters<typeof claimInviteUse>[0]
|
||||||
|
|
||||||
|
const result = await claimInviteUse(admin, 'inv-1', 5, 1000, 5)
|
||||||
|
expect(result).toEqual({ ok: false, reason: 'contention' })
|
||||||
|
expect(row.uses_count).toBeLessThan(1000) // never hit the cap
|
||||||
|
})
|
||||||
|
|
||||||
|
it('gives up (contention) if it cannot win the CAS within the attempt budget', async () => {
|
||||||
// Caller's known count (0) is behind the live row (3), so the first CAS
|
// Caller's known count (0) is behind the live row (3), so the first CAS
|
||||||
// guard misses; with a 1-attempt budget it re-reads and then gives up
|
// guard misses; with a 1-attempt budget it re-reads and then gives up
|
||||||
// rather than looping — proving the retry is bounded, not infinite.
|
// rather than looping — proving the retry is bounded, not infinite. Cap is
|
||||||
|
// far off (10), so this is contention, not a reached cap.
|
||||||
const admin = makeAdmin({ uses_count: 3, max_uses: 10 })
|
const admin = makeAdmin({ uses_count: 3, max_uses: 10 })
|
||||||
const result = await claimInviteUse(admin, 'inv-1', 0, 10, 1)
|
const result = await claimInviteUse(admin, 'inv-1', 0, 10, 1)
|
||||||
expect(result).toBeNull()
|
expect(result).toEqual({ ok: false, reason: 'contention' })
|
||||||
expect(admin._row.uses_count).toBe(3) // untouched
|
expect(admin._row.uses_count).toBe(3) // untouched
|
||||||
})
|
})
|
||||||
})
|
})
|
||||||
|
|||||||
+10
-2
@@ -182,8 +182,16 @@ export async function joinSpace(formData: {
|
|||||||
// (The pre-check above is a fast UX reject; this is the authoritative guard.)
|
// (The pre-check above is a fast UX reject; this is the authoritative guard.)
|
||||||
let claimedCount: number | null = null
|
let claimedCount: number | null = null
|
||||||
if (invite) {
|
if (invite) {
|
||||||
claimedCount = await claimInviteUse(admin, invite.id, invite.uses_count, invite.max_uses)
|
const claim = await claimInviteUse(admin, invite.id, invite.uses_count, invite.max_uses)
|
||||||
if (claimedCount === null) return { error: 'This invite has reached its use cap.' }
|
if (!claim.ok) {
|
||||||
|
return {
|
||||||
|
error:
|
||||||
|
claim.reason === 'contention'
|
||||||
|
? 'Too many people are joining at once — please try again.'
|
||||||
|
: 'This invite has reached its use cap.',
|
||||||
|
}
|
||||||
|
}
|
||||||
|
claimedCount = claim.count
|
||||||
}
|
}
|
||||||
|
|
||||||
const { data: newMember, error: memberErr } = await admin
|
const { data: newMember, error: memberErr } = await admin
|
||||||
|
|||||||
+16
-8
@@ -12,20 +12,27 @@ type Admin = ReturnType<typeof createAdminClient>
|
|||||||
* 0-row result, re-reads, and retries while capacity remains. `uses_count` only
|
* 0-row result, re-reads, and retries while capacity remains. `uses_count` only
|
||||||
* ever increases, so the CAS has no ABA hazard.
|
* ever increases, so the CAS has no ABA hazard.
|
||||||
*
|
*
|
||||||
* Returns the new `uses_count` on success (the caller stores it to roll the
|
* Returns a discriminated result: `{ ok: true, count }` with the new
|
||||||
* claim back if the subsequent membership insert fails), or `null` if the cap
|
* `uses_count` on success (the caller stores it to roll the claim back if the
|
||||||
* is reached.
|
* subsequent membership insert fails), or `{ ok: false, reason }` where the
|
||||||
|
* reason distinguishes a genuinely reached cap (`'cap_reached'`) from losing
|
||||||
|
* the compare-and-swap on every attempt under contention (`'contention'`) so
|
||||||
|
* the caller can surface a retryable message instead of a false cap error.
|
||||||
*/
|
*/
|
||||||
|
export type ClaimInviteResult =
|
||||||
|
| { ok: true; count: number }
|
||||||
|
| { ok: false; reason: 'cap_reached' | 'contention' }
|
||||||
|
|
||||||
export async function claimInviteUse(
|
export async function claimInviteUse(
|
||||||
admin: Admin,
|
admin: Admin,
|
||||||
inviteId: string,
|
inviteId: string,
|
||||||
knownCount: number,
|
knownCount: number,
|
||||||
maxUses: number | null,
|
maxUses: number | null,
|
||||||
maxAttempts = 5,
|
maxAttempts = 5,
|
||||||
): Promise<number | null> {
|
): Promise<ClaimInviteResult> {
|
||||||
let current = knownCount
|
let current = knownCount
|
||||||
for (let attempt = 0; attempt < maxAttempts; attempt++) {
|
for (let attempt = 0; attempt < maxAttempts; attempt++) {
|
||||||
if (maxUses !== null && current >= maxUses) return null
|
if (maxUses !== null && current >= maxUses) return { ok: false, reason: 'cap_reached' }
|
||||||
const next = current + 1
|
const next = current + 1
|
||||||
const { data } = await admin
|
const { data } = await admin
|
||||||
.from('space_invites')
|
.from('space_invites')
|
||||||
@@ -33,15 +40,16 @@ export async function claimInviteUse(
|
|||||||
.eq('id', inviteId)
|
.eq('id', inviteId)
|
||||||
.eq('uses_count', current) // compare-and-swap guard
|
.eq('uses_count', current) // compare-and-swap guard
|
||||||
.select('id')
|
.select('id')
|
||||||
if (data && data.length > 0) return next
|
if (data && data.length > 0) return { ok: true, count: next }
|
||||||
// Lost the race (or the row changed): re-read and retry while under cap.
|
// Lost the race (or the row changed): re-read and retry while under cap.
|
||||||
const { data: fresh } = await admin
|
const { data: fresh } = await admin
|
||||||
.from('space_invites')
|
.from('space_invites')
|
||||||
.select('uses_count')
|
.select('uses_count')
|
||||||
.eq('id', inviteId)
|
.eq('id', inviteId)
|
||||||
.maybeSingle()
|
.maybeSingle()
|
||||||
if (!fresh) return null
|
if (!fresh) return { ok: false, reason: 'cap_reached' }
|
||||||
current = (fresh as { uses_count: number }).uses_count
|
current = (fresh as { uses_count: number }).uses_count
|
||||||
}
|
}
|
||||||
return null
|
// Exhausted the attempt budget while still under cap: pure contention.
|
||||||
|
return { ok: false, reason: 'contention' }
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user