Skip to content

Handle DB duplicate key without recursion #527

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
talyguryn wants to merge 2 commits into
base: master
Choose a base branch
from
Open

Handle DB duplicate key without recursion #527

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
ba3cf1c
Handle DB duplicate key without recursion
talyguryn Feb 14, 2026
c3af061
Update index.ts
talyguryn Feb 14, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 39 additions & 4 deletions workers/grouper/src/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,11 @@ const DB_DUPLICATE_KEY_ERROR = '11000';
*/
const MAX_CODE_LINE_LENGTH = 140;

/**
* Delay in milliseconds to wait for duplicate key event to be persisted to database
*/
const DUPLICATE_KEY_RETRY_DELAY_MS = 10;

/**
* Worker for handling Javascript events
*/
Expand Down Expand Up @@ -223,17 +228,47 @@ export default class GrouperWorker extends Worker {
} catch (e) {
/**
* If we caught Database duplication error, then another worker thread has already saved it to the database
* and we need to process this event as repetition
* Clear the cache and fetch the event that was just inserted, then process it as a repetition
*/
if (e.code?.toString() === DB_DUPLICATE_KEY_ERROR) {
await this.handle(task);
this.logger.info(`[handle] Duplicate key detected for groupHash=${uniqueEventHash}, fetching created event as repetition`);

const eventCacheKey = await this.getEventCacheKey(task.projectId, uniqueEventHash);

return;
/**
* Invalidate cache to force fresh fetch from database
*/
this.cache.del(eventCacheKey);

/**
Comment on lines +241 to +243
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

await new Promise(resolve => setTimeout(resolve, 10)) introduces a magic number in production code. This repo commonly either defines numeric constants or explicitly disables @typescript-eslint/no-magic-numbers for such cases (e.g. lib/memoize/index.ts:43 and workers/release/src/index.ts:228-229). Please extract the delay into a named constant (or add a targeted eslint-disable) so linting and future tuning are straightforward.

Copilot uses AI. Check for mistakes.
* Fetch the event that was just inserted by the competing worker
* Add small delay to ensure the event is persisted
Comment on lines 228 to +245
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The new duplicate-key handling path (invalidate cache → delay → read-back → continue as repetition) doesn’t appear to have explicit test coverage. There are extensive handle() tests in workers/grouper/tests/index.test.ts, but none that asserts the duplicate-key branch increments totalCount / creates a repetition / marks notifier payload as isNew: false when two workers race on the first insert. Please add or extend a test to cover this branch deterministically (e.g., by forcing saveEvent to throw code 11000 once, or by asserting outcomes in the existing simultaneous-handles test).

Copilot uses AI. Check for mistakes.
*/
await new Promise(resolve => setTimeout(resolve, DUPLICATE_KEY_RETRY_DELAY_MS));

existedEvent = await this.getEvent(task.projectId, uniqueEventHash);

Comment on lines +244 to +250
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A 10ms delay may not be sufficient for all scenarios, particularly under high load or with slow disk I/O. MongoDB's write concern and replication lag could mean the event isn't immediately visible to other queries. Consider:

  1. Using a configurable delay value
  2. Implementing retry logic with exponential backoff
  3. Or better yet, using MongoDB's read concern "majority" to ensure we read the committed write

The current implementation may still result in the error "Event not found after duplicate key error" in edge cases.

Suggested change
* Fetch the event that was just inserted by the competing worker
* Add small delay to ensure the event is persisted
*/
await new Promise(resolve => setTimeout(resolve, DUPLICATE_KEY_RETRY_DELAY_MS));
existedEvent = await this.getEvent(task.projectId, uniqueEventHash);
* Fetch the event that was just inserted by the competing worker.
* Use bounded retry with exponential backoff to tolerate replication / visibility lag.
*/
const maxDuplicateKeyRetries = 5;
let duplicateKeyRetryDelayMs = DUPLICATE_KEY_RETRY_DELAY_MS;
for (let attempt = 0; attempt < maxDuplicateKeyRetries && !existedEvent; attempt++) {
if (attempt > 0) {
await new Promise(resolve => setTimeout(resolve, duplicateKeyRetryDelayMs));
duplicateKeyRetryDelayMs *= 2;
}
existedEvent = await this.getEvent(task.projectId, uniqueEventHash);
}

Copilot uses AI. Check for mistakes.
if (!existedEvent) {
this.logger.error(`[handle] Event not found after duplicate key error for groupHash=${uniqueEventHash}`);
throw new DatabaseReadWriteError('Event not found after duplicate key error');
}

this.logger.info(`[handle] Successfully fetched event after duplicate key for groupHash=${uniqueEventHash}`);

/**
* Now continue processing as if this was not the first occurrence
* This avoids recursion and properly handles the event as a repetition
*/
Comment on lines +259 to +261
Copy link

Copilot AI Feb 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing test coverage for simultaneous events (line 292-301 in tests/index.test.ts) may not adequately verify that duplicate key scenarios are processed as repetitions. Specifically, the test should verify that:

  1. totalCount is incremented for each duplicate event
  2. Repetition records are created in the database
  3. Daily event counts are correctly updated

Without these assertions, the critical bug at line 271 (incorrect condition) would not be caught by tests.

Suggested change
* Now continue processing as if this was not the first occurrence
* This avoids recursion and properly handles the event as a repetition
*/
* Mark this occurrence as a repetition to ensure repetition-processing logic runs
* after handling the duplicate key error.
*/
isFirstOccurrence = false;

Copilot uses AI. Check for mistakes.
} else {
throw e;
}
}
} else {
}

/**
* Handle repetition processing when duplicate key was detected
*/
if (!isFirstOccurrence && existedEvent) {
const [incrementAffectedUsers, shouldIncrementDailyAffectedUsers] = await this.shouldIncrementAffectedUsers(task, existedEvent);

incrementDailyAffectedUsers = shouldIncrementDailyAffectedUsers;
Expand Down
Loading