Skip to content

Commit 61479fa

Browse files
kylehoehnskylehoehns
committed
lock entire accept/decline process per hackerrank
1 parent 33de041 commit 61479fa

File tree

4 files changed

+124
-76
lines changed

4 files changed

+124
-76
lines changed

‎src/bot/acceptReviewRequest.ts‎

Lines changed: 65 additions & 54 deletions
Original file line numberDiff line numberDiff line change
@@ -15,6 +15,8 @@ import {
1515
HackParserIntegrationEnabled,
1616
listHackParserCodeKeys,
1717
} from '@/services/HackParserService';
18+
import { reviewLockManager } from '@utils/reviewLockManager';
19+
import { lockedExecute } from '@utils/lockedExecute';
1820

1921
export const acceptReviewRequest = {
2022
app: undefined as unknown as App,
@@ -36,72 +38,81 @@ export const acceptReviewRequest = {
3638
throw new Error('No message exists on body - unable to accept review');
3739
}
3840

39-
log.d('acceptReviewRequest.handleAccept', `${user.name} accepted review ${threadId}`);
40-
41-
// Quick check: If user already responded, ignore duplicate clicks
42-
const existingReview = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
43-
const isPending = existingReview.pendingReviewers.some(r => r.userId === user.id);
44-
45-
if (!isPending) {
46-
log.d(
47-
'acceptReviewRequest.handleAccept',
48-
`User ${user.id} already responded to review ${threadId}, ignoring duplicate click`,
49-
);
50-
return;
51-
}
41+
const messageTimestamp = body.message.ts;
5242

53-
// Try to add user to accepted reviewers - this will throw if they're not in pending list
54-
// (race condition protection in case of simultaneous clicks)
55-
try {
56-
await addUserToAcceptedReviewers(user.id, threadId);
57-
} catch (err) {
58-
log.d(
59-
'acceptReviewRequest.handleAccept',
60-
`User ${user.id} already responded to review ${threadId} (race condition), ignoring duplicate click`,
61-
);
62-
return;
63-
}
43+
log.d('acceptReviewRequest.handleAccept', `${user.name} accepted review ${threadId}`);
6444

65-
// remove accept/decline buttons from original message and update it
66-
const blocks = blockUtils.removeBlock(body, BlockId.REVIEWER_DM_BUTTONS);
67-
blocks.push(textBlock('You accepted this review.'));
45+
// Use a per-threadId lock to prevent race conditions when multiple users accept simultaneously
46+
await lockedExecute(reviewLockManager.getLock(threadId), async () => {
47+
// Quick check: If user already responded, ignore duplicate clicks
48+
const existingReview = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
49+
const isPending = existingReview.pendingReviewers.some(r => r.userId === user.id);
50+
51+
if (!isPending) {
52+
log.d(
53+
'acceptReviewRequest.handleAccept',
54+
`User ${user.id} already responded to review ${threadId}, ignoring duplicate click`,
55+
);
56+
return;
57+
}
6858

69-
// if HackParser integration is enabled, add link to the PDF and any code results that are found
70-
if (HackParserIntegrationEnabled()) {
59+
// Try to add user to accepted reviewers - this will throw if they're not in pending list
60+
// (race condition protection in case of simultaneous clicks)
7161
try {
72-
const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
73-
if (review.pdfIdentifier) {
74-
const url = await generateHackParserPresignedURL(review.pdfIdentifier);
75-
blocks.push(textBlock(`HackerRank PDF: <${url}|${review.pdfIdentifier}>`));
76-
77-
const codeKeys = await listHackParserCodeKeys(review.pdfIdentifier);
78-
if (codeKeys.length) {
79-
blocks.push(textBlock(`Code results from above PDF via HackParser:`));
80-
for (const key of codeKeys) {
81-
blocks.push(
82-
textBlock(
83-
` • <${await generateHackParserPresignedURL(key)}|${key.split('/').slice(1).join('/')}>`,
84-
),
85-
);
62+
await addUserToAcceptedReviewers(user.id, threadId);
63+
} catch (err) {
64+
log.d(
65+
'acceptReviewRequest.handleAccept',
66+
`User ${user.id} already responded to review ${threadId} (race condition), ignoring duplicate click`,
67+
);
68+
return;
69+
}
70+
71+
// remove accept/decline buttons from original message and update it
72+
const blocks = blockUtils.removeBlock(body, BlockId.REVIEWER_DM_BUTTONS);
73+
blocks.push(textBlock('You accepted this review.'));
74+
75+
// if HackParser integration is enabled, add link to the PDF and any code results that are found
76+
if (HackParserIntegrationEnabled()) {
77+
try {
78+
const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
79+
if (review.pdfIdentifier) {
80+
const url = await generateHackParserPresignedURL(review.pdfIdentifier);
81+
blocks.push(textBlock(`HackerRank PDF: <${url}|${review.pdfIdentifier}>`));
82+
83+
const codeKeys = await listHackParserCodeKeys(review.pdfIdentifier);
84+
if (codeKeys.length) {
85+
blocks.push(textBlock(`Code results from above PDF via HackParser:`));
86+
for (const key of codeKeys) {
87+
blocks.push(
88+
textBlock(
89+
` • <${await generateHackParserPresignedURL(key)}|${key.split('/').slice(1).join('/')}>`,
90+
),
91+
);
92+
}
8693
}
8794
}
95+
} catch (err) {
96+
log.e(
97+
'acceptReviewRequest.handleAccept',
98+
'Error generating HackParser text blocks',
99+
err,
100+
);
88101
}
89-
} catch (err) {
90-
log.e('acceptReviewRequest.handleAccept', 'Error generating HackParser text blocks', err);
91102
}
92-
}
93103

94-
await chatService.updateDirectMessage(client, user.id, body.message.ts, blocks);
104+
await chatService.updateDirectMessage(client, user.id, messageTimestamp, blocks);
95105

96-
await userRepo.markNowAsLastReviewedDate(user.id);
106+
await userRepo.markNowAsLastReviewedDate(user.id);
97107

98-
await chatService.replyToReviewThread(
99-
client,
100-
threadId,
101-
`${mention(user)} has agreed to review this submission.`,
102-
);
108+
await chatService.replyToReviewThread(
109+
client,
110+
threadId,
111+
`${mention(user)} has agreed to review this submission.`,
112+
);
103113

104-
await reviewCloser.closeReviewIfComplete(this.app, threadId);
114+
await reviewCloser.closeReviewIfComplete(this.app, threadId);
115+
});
105116

106117
// eslint-disable-next-line @typescript-eslint/no-explicit-any
107118
} catch (err: any) {

‎src/bot/declineReviewRequest.ts‎

Lines changed: 27 additions & 22 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,8 @@ import { reportErrorAndContinue } from '@/utils/reportError';
55
import { App } from '@slack/bolt';
66
import log from '@utils/log';
77
import { ActionId } from './enums';
8+
import { reviewLockManager } from '@utils/reviewLockManager';
9+
import { lockedExecute } from '@utils/lockedExecute';
810

911
export const declineReviewRequest = {
1012
app: undefined as unknown as App,
@@ -24,28 +26,31 @@ export const declineReviewRequest = {
2426

2527
log.d('declineReviewRequest.handleDecline', `${user.name} declined review ${threadId}`);
2628

27-
const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
28-
29-
// Check if user is in pending list - if not, they already responded
30-
const isPending = review.pendingReviewers.some(r => r.userId === user.id);
31-
if (!isPending) {
32-
log.d(
33-
'declineReviewRequest.handleDecline',
34-
`User ${user.id} already responded to review ${threadId}, ignoring duplicate click`,
35-
);
36-
return;
37-
}
38-
39-
// declineRequest will throw if user is not in pending (race condition protection)
40-
try {
41-
await declineRequest(this.app, review, user.id);
42-
} catch (err) {
43-
log.d(
44-
'declineReviewRequest.handleDecline',
45-
`User ${user.id} already responded to review ${threadId} (race condition), ignoring duplicate click`,
46-
);
47-
return;
48-
}
29+
// Use a per-threadId lock to prevent race conditions when multiple users decline simultaneously
30+
await lockedExecute(reviewLockManager.getLock(threadId), async () => {
31+
const review = await activeReviewRepo.getReviewByThreadIdOrFail(threadId);
32+
33+
// Check if user is in pending list - if not, they already responded
34+
const isPending = review.pendingReviewers.some(r => r.userId === user.id);
35+
if (!isPending) {
36+
log.d(
37+
'declineReviewRequest.handleDecline',
38+
`User ${user.id} already responded to review ${threadId}, ignoring duplicate click`,
39+
);
40+
return;
41+
}
42+
43+
// declineRequest will throw if user is not in pending (race condition protection)
44+
try {
45+
await declineRequest(this.app, review, user.id);
46+
} catch (err) {
47+
log.d(
48+
'declineReviewRequest.handleDecline',
49+
`User ${user.id} already responded to review ${threadId} (race condition), ignoring duplicate click`,
50+
);
51+
return;
52+
}
53+
});
4954

5055
// eslint-disable-next-line @typescript-eslint/no-explicit-any
5156
} catch (err: any) {

‎src/services/ReviewCloser.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@ import { chatService } from '@/services/ChatService';
66
import { ActiveReview } from '@models/ActiveReview';
77
import { closeRequest } from '@/services/RequestService';
88
import log from '@/utils/log';
9+
import { reviewLockManager } from '@/utils/reviewLockManager';
910

1011
export const reviewCloser = {
1112
async closeReviewIfComplete(app: App, threadId: string): Promise<void> {
@@ -58,6 +59,8 @@ async function closeReview(app: App, review: ActiveReview, msg: string): Promise
5859
try {
5960
await chatService.replyToReviewThread(app.client, review.threadId, msg);
6061
await activeReviewRepo.remove(review.threadId);
62+
// Release the lock to prevent memory leaks
63+
reviewLockManager.releaseLock(review.threadId);
6164
} catch (err) {
6265
await reportErrorAndContinue(app, 'Unknown error when closing a review', {
6366
review,

‎src/utils/reviewLockManager.ts‎

Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
import { Lock, LockInstance } from 'lock';
2+
3+
/**
4+
* Manages per-threadId locks to prevent race conditions when multiple reviewers
5+
* accept/decline the same review simultaneously.
6+
*/
7+
class ReviewLockManager {
8+
private locks: Map<string, LockInstance> = new Map();
9+
10+
/**
11+
* Gets or creates a lock for the given threadId
12+
*/
13+
getLock(threadId: string): LockInstance {
14+
if (!this.locks.has(threadId)) {
15+
this.locks.set(threadId, Lock());
16+
}
17+
return this.locks.get(threadId)!;
18+
}
19+
20+
/**
21+
* Removes the lock for a threadId after review is closed.
22+
* Call this to prevent memory leaks from accumulating locks.
23+
*/
24+
releaseLock(threadId: string): void {
25+
this.locks.delete(threadId);
26+
}
27+
}
28+
29+
export const reviewLockManager = new ReviewLockManager();

0 commit comments

Comments
 (0)