fix(teams): prevent concurrent slug collision with sequential retry#3
fix(teams): prevent concurrent slug collision with sequential retry#3Ridanshi wants to merge 2 commits into
Conversation
Replace non-deterministic random suffix generation with sequential numeric candidates (my-team → my-team-1 → my-team-2, capped at 10). Wrap team creation in a bounded retry loop (5 attempts) so P2002 constraint violations from concurrent inserts trigger re-allocation rather than surfacing as a 409. The database-level @unique constraint on Team.slug remains the authoritative guard; application logic now recovers gracefully when it fires. Adds slug utility tests (createSlug, generateUniqueSlug determinism and bounds) and team route tests for retry-on-race-condition and retry-exhaustion paths. Closes Dev-Card#499
Apply the same bounded retry loop (5 attempts) used for team slug allocation to event creation, so P2002 unique-constraint violations from concurrent inserts trigger re-allocation rather than surfacing as 500 errors. Also aligns GET /:slug response shape (organizerId instead of organizer join), fixes paginated attendees to use attendees array length for total, and cleans up auth to use request.jwtVerify() inline — consistent with the team routes approach. Co-Authored-By: Ridanshi <ridanshiagarwal2@gmail.com>
|
Hi @Ridanshi, Thanks for opening this pull request. This PR has been automatically classified based on the files modified. Applied Labels
Primary Review Area
Reviewer@Harxhit has been identified as the primary reviewer for this pull request. If you have any questions regarding the affected area or implementation details, feel free to reach out to the assigned reviewer. Thank you for your contribution! |
📝 WalkthroughWalkthroughThe PR replaces an unbounded random-suffix slug collision loop with a deterministic bounded retry approach ( ChangesSlug Generation and Auth Refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
CI — All Checks PassedBackend — PASS
Mobile — SKIP
Web — SKIP
Last updated: |
![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
🧹 Nitpick comments (2)
apps/backend/src/routes/team.ts (2)
251-266: ⚡ Quick winRedundant condition and missing
return.The
if (isOwner || isSelfRemove)check on line 251 is always true at this point—the guard at lines 243-245 already returned 403 when both were false. Additionally, line 261 is missing areturnbeforereply.status(200).send(...), which is inconsistent with other handlers and could cause subtle issues if code is added after this block.♻️ Suggested fix
- if (isOwner || isSelfRemove) { - try { - await app.prisma.teamMember.delete({ - where: { - userId_teamId: { - teamId: teamDetails.id, - userId: paramsUserId, - }, + try { + await app.prisma.teamMember.delete({ + where: { + userId_teamId: { + teamId: teamDetails.id, + userId: paramsUserId, }, - }); - reply.status(200).send('Member removed'); - } catch (error) { - app.log.error(error); - return reply.status(500).send('DB query failed'); - } + }, + }); + return reply.status(200).send('Member removed'); + } catch (error) { + app.log.error(error); + return reply.status(500).send('DB query failed'); }🤖 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 `@apps/backend/src/routes/team.ts` around lines 251 - 266, The if condition checking isOwner || isSelfRemove on line 251 is redundant because the guard clause at lines 243-245 already returns early when both conditions are false, meaning the code always enters this block. Remove this redundant condition and unindent the try-catch block that follows it. Additionally, add a return statement before reply.status(200).send on the success path to ensure the function exits after sending the response, making it consistent with error handling patterns in other handlers.
78-78: ⚡ Quick winLog the actual error object for debugging.
Unlike other handlers in this file (e.g., lines 143, 204, 302), this error log omits the error object, making it harder to diagnose non-P2002/P2003 failures in production.
🛠️ Suggested fix
- app.log.error('Failed to create a team'); + app.log.error(error, 'Failed to create a team');🤖 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 `@apps/backend/src/routes/team.ts` at line 78, The error logging statement in the team creation handler omits the actual error object, making it inconsistent with other error handlers in the same file and harder to debug production issues. Modify the app.log.error call to include the error object as a second parameter, following the same pattern used in other handlers in this file such as those at lines 143, 204, and 302, so that the actual error details are captured in the logs.
🤖 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 `@apps/backend/src/__tests__/slug.test.ts`:
- Around line 61-73: The test "produces consistent slugs across concurrent calls
for different inputs" is misleading as it only tests different inputs, not
actual concurrency scenarios. The takenSlugs Set is never populated, so it
doesn't verify that concurrent calls with the same input produce distinct
suffixed slugs. Add a new test case that calls generateUniqueSlug twice
concurrently with the same input (e.g., 'My Team'), implement the slugExists
mock to properly track and detect collisions by adding slugs to the Set after
checking, and verify that the two concurrent calls produce distinct results with
one returning the base slug and the other returning a suffixed variant (e.g.,
'my-team' and 'my-team-1').
In `@apps/backend/src/routes/event.ts`:
- Line 209: HTTP 204 No Content responses must not include a message body per
HTTP specification. In the event route handler where the status is set to 204,
remove the message body being passed to the send() method by either calling
send() without arguments or send(undefined), or alternatively change the status
code to 200 if you need to include the confirmation message in the response
body.
- Around line 264-271: The pagination.total field in the
PaginatedAttendeesResponse is incorrectly set to event.attendees.length, which
only reflects the count of attendees on the current page after pagination, not
the actual total count. To fix this, retrieve the total attendee count
separately using a count query (such as using _count in your database query)
before applying pagination skip/take operations, then use this actual total
count value instead of event.attendees.length when setting pagination.total in
the response object.
In `@apps/backend/src/utils/slug.ts`:
- Line 26: The error message thrown in the slug generation function undercounts
the actual attempts made. The function performs one initial check for the base
slug before the loop that generates suffixed candidates, resulting in
MAX_SLUG_ATTEMPTS + 1 total checks rather than just MAX_SLUG_ATTEMPTS. Update
the error message on the throw statement to correctly reflect the actual number
of attempts by either changing MAX_SLUG_ATTEMPTS to MAX_SLUG_ATTEMPTS + 1 in the
message, or adjusting the message text to accurately state the total number of
slug candidates evaluated.
---
Nitpick comments:
In `@apps/backend/src/routes/team.ts`:
- Around line 251-266: The if condition checking isOwner || isSelfRemove on line
251 is redundant because the guard clause at lines 243-245 already returns early
when both conditions are false, meaning the code always enters this block.
Remove this redundant condition and unindent the try-catch block that follows
it. Additionally, add a return statement before reply.status(200).send on the
success path to ensure the function exits after sending the response, making it
consistent with error handling patterns in other handlers.
- Line 78: The error logging statement in the team creation handler omits the
actual error object, making it inconsistent with other error handlers in the
same file and harder to debug production issues. Modify the app.log.error call
to include the error object as a second parameter, following the same pattern
used in other handlers in this file such as those at lines 143, 204, and 302, so
that the actual error details are captured in the logs.
🪄 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: a15998c3-c7f3-483f-ab13-b65e9fe3dd14
📒 Files selected for processing (5)
apps/backend/src/__tests__/slug.test.tsapps/backend/src/__tests__/team.test.tsapps/backend/src/routes/event.tsapps/backend/src/routes/team.tsapps/backend/src/utils/slug.ts
| it('produces consistent slugs across concurrent calls for different inputs', async () => { | ||
| const takenSlugs = new Set<string>(); | ||
| const slugExists = vi.fn(async (slug: string) => takenSlugs.has(slug)); | ||
|
|
||
| const [a, b] = await Promise.all([ | ||
| generateUniqueSlug('Alpha Team', slugExists), | ||
| generateUniqueSlug('Beta Team', slugExists), | ||
| ]); | ||
|
|
||
| expect(a).toBe('alpha-team'); | ||
| expect(b).toBe('beta-team'); | ||
| expect(a).not.toBe(b); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Test name is misleading and doesn't verify the core concurrency scenario.
The test is named "produces consistent slugs across concurrent calls" but doesn't actually test race conditions or concurrent slug generation for the same input. The takenSlugs Set is never populated (line 62), so both calls always receive their base slugs. This test only verifies that different inputs produce different slugs, not that concurrent creation with the same name produces distinct suffixed slugs.
Given that the PR objectives specifically mention "concurrent team creation failures," consider adding a test that verifies concurrent calls with the same input (e.g., both creating "My Team") produce distinct results (e.g., my-team and my-team-1).
💡 Suggested additional test case
it('produces distinct slugs when concurrent calls use the same input', async () => {
const takenSlugs = new Set<string>();
const slugExists = vi.fn(async (slug: string) => {
if (takenSlugs.has(slug)) {
return true;
}
takenSlugs.add(slug);
return false;
});
const [a, b] = await Promise.all([
generateUniqueSlug('My Team', slugExists),
generateUniqueSlug('My Team', slugExists),
]);
expect(a).toBe('my-team');
expect(b).toBe('my-team-1');
expect(a).not.toBe(b);
});🤖 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 `@apps/backend/src/__tests__/slug.test.ts` around lines 61 - 73, The test
"produces consistent slugs across concurrent calls for different inputs" is
misleading as it only tests different inputs, not actual concurrency scenarios.
The takenSlugs Set is never populated, so it doesn't verify that concurrent
calls with the same input produce distinct suffixed slugs. Add a new test case
that calls generateUniqueSlug twice concurrently with the same input (e.g., 'My
Team'), implement the slugExists mock to properly track and detect collisions by
adding slugs to the Set after checking, and verify that the two concurrent calls
produce distinct results with one returning the base slug and the other
returning a suffixed variant (e.g., 'my-team' and 'my-team-1').
| }, | ||
| }, | ||
| }); | ||
| return reply.status(204).send({ message: 'User left' }); |
There was a problem hiding this comment.
HTTP 204 should not include a response body.
Per HTTP specification, 204 No Content responses must not contain a message body. Either remove the body or change to 200 if a confirmation message is needed.
Proposed fix
- return reply.status(204).send({ message: 'User left' });
+ return reply.status(204).send();📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| return reply.status(204).send({ message: 'User left' }); | |
| return reply.status(204).send(); |
🤖 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 `@apps/backend/src/routes/event.ts` at line 209, HTTP 204 No Content responses
must not include a message body per HTTP specification. In the event route
handler where the status is set to 204, remove the message body being passed to
the send() method by either calling send() without arguments or send(undefined),
or alternatively change the status code to 200 if you need to include the
confirmation message in the response body.
| const response: PaginatedAttendeesResponse = { | ||
| attendees, | ||
| pagination: { | ||
| page, | ||
| limit, | ||
| total: (event as any).attendees.length, | ||
| }, | ||
| }; |
There was a problem hiding this comment.
pagination.total returns page length instead of total attendee count.
event.attendees.length is the count of items on the current page (after skip/take), not the total number of attendees. This breaks pagination UI—clients cannot determine total pages or display "showing 1-10 of 500".
Use a separate count query or include _count on the event to get the actual total.
Proposed fix using _count
const event = await app.prisma.event.findUnique({
where: { slug: paramsSlug },
include: {
+ _count: {
+ select: { attendees: true },
+ },
attendees: {
include: {
user: {Then update the response:
const response: PaginatedAttendeesResponse = {
attendees,
pagination: {
page,
limit,
- total: (event as any).attendees.length,
+ total: event._count.attendees,
},
};🤖 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 `@apps/backend/src/routes/event.ts` around lines 264 - 271, The
pagination.total field in the PaginatedAttendeesResponse is incorrectly set to
event.attendees.length, which only reflects the count of attendees on the
current page after pagination, not the actual total count. To fix this, retrieve
the total attendee count separately using a count query (such as using _count in
your database query) before applying pagination skip/take operations, then use
this actual total count value instead of event.attendees.length when setting
pagination.total in the response object.
| if (!(await slugExists(candidate))) { return candidate; } | ||
| } | ||
|
|
||
| throw new Error(`Unable to generate unique slug for "${name}" after ${MAX_SLUG_ATTEMPTS} attempts`); |
There was a problem hiding this comment.
Error message undercounts the actual attempts.
The error message states "after 10 attempts" but the function actually performs 11 checks: one for the base slug (line 19) plus 10 suffixed candidates (lines 21-24). This could mislead debugging and operations.
📝 Proposed fix
- throw new Error(`Unable to generate unique slug for "${name}" after ${MAX_SLUG_ATTEMPTS} attempts`);
+ throw new Error(`Unable to generate unique slug for "${name}" after ${MAX_SLUG_ATTEMPTS + 1} attempts`);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| throw new Error(`Unable to generate unique slug for "${name}" after ${MAX_SLUG_ATTEMPTS} attempts`); | |
| throw new Error(`Unable to generate unique slug for "${name}" after ${MAX_SLUG_ATTEMPTS + 1} attempts`); |
🤖 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 `@apps/backend/src/utils/slug.ts` at line 26, The error message thrown in the
slug generation function undercounts the actual attempts made. The function
performs one initial check for the base slug before the loop that generates
suffixed candidates, resulting in MAX_SLUG_ATTEMPTS + 1 total checks rather than
just MAX_SLUG_ATTEMPTS. Update the error message on the throw statement to
correctly reflect the actual number of attempts by either changing
MAX_SLUG_ATTEMPTS to MAX_SLUG_ATTEMPTS + 1 in the message, or adjusting the
message text to accurately state the total number of slug candidates evaluated.
|
Superseded by Dev-Card#557 which targets the correct repository. |
1 participant
Summary
P2002unique-constraint violations and reallocates a new slug, so concurrent team creation no longer surfaces as a 500 errorgenerateUniqueSlugwith sequential numeric candidates (my-team→my-team-1→my-team-2, capped at 10); the database-level@uniqueconstraint onTeam.slugremains the authoritative race guardslug.test.ts(utility unit tests) and extendsteam.test.tswith retry-on-race-condition and retry-exhaustion pathsCloses Dev-Card#499
Changes
src/utils/slug.ts—generateUniqueSlugnow uses sequential numeric suffixes instead of random ones, making retries deterministic and testable.src/routes/team.ts—POST /wraps$transactionin aforloop (max 5 attempts);P2002→ retry,P2003→ 400, other errors → 500. Returns 409 only after exhausting all attempts.src/routes/event.ts— Same P2002 retry loop applied to event creation; aligned GET response shape and auth approach with the rest of the codebase.src/__tests__/slug.test.ts— New file: unit tests forcreateSlugandgenerateUniqueSlug(determinism, bounds, collision resolution).src/__tests__/team.test.ts— Two new cases: retry-succeeds-on-second-attempt and retry-exhaustion-returns-409.Test plan
npx vitest run src/__tests__/team.test.ts— all 57 tests passnpx vitest run src/__tests__/slug.test.ts— all tests passnpx vitest run src/__tests__/event.test.ts— all 36 tests passnpx eslint src/routes/team.ts src/routes/event.ts src/utils/slug.ts src/__tests__/team.test.ts src/__tests__/slug.test.ts— 0 errorsSummary by cubic
Stops concurrent slug collisions during team and event creation by using sequential slug candidates and a 5-attempt retry on unique-constraint errors. This makes slug allocation deterministic and prevents 500s from race conditions, returning 409 only after all attempts fail.
Bug Fixes
P2002up to 5 times with sequential slugs (base,base-1…base-10); teams mapP2003to 400; 409 only after retry exhaustion.Refactors
generateUniqueSlugis deterministic and capped; the DB@uniqueconstraint remains the race guard.organizerId; auth usesrequest.jwtVerify(); addedslug.test.tsand team tests for retry and exhaustion paths.Written for commit d39f618. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Tests