fix: fix collision handling in comlink username task#3293
Conversation
WalkthroughReplace previous username-insert flow with an insert-that-returns-count approach; the generator iterates target accounts from a new getter, uses Changes
Sequence Diagram(s)sequenceDiagram
participant Generator as UsernameGenerator
participant Store as SubaccountUsernamesTable
participant DB as Postgres
Generator->>Store: getSubaccountZerosWithoutUsernames(batchSize)
Store-->>Generator: targetAccounts[]
loop for each account
Generator->>Store: insertAndReturnCount(username, subaccountId, {txId})
Store->>DB: INSERT ... ON CONFLICT DO NOTHING
DB-->>Store: result.rows (len 0 or 1)
Store-->>Generator: count (0 or 1)
alt count == 1
Generator->>Generator: successCount++
Generator->>DB: (later) COMMIT txId
else count == 0
Generator->>Generator: collision -> generate fallback username and retry
note right of Generator `#f9f7d9`: treat 0 as handled collision\n(do not throw), log and continue
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.tsindexer/services/roundtable/src/tasks/subaccount-username-generator.ts
🧰 Additional context used
🧠 Learnings (2)
📚 Learning: 2024-11-22T18:12:04.606Z
Learnt from: hwray
Repo: dydxprotocol/v4-chain PR: 2597
File: indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql:16-20
Timestamp: 2024-11-22T18:12:04.606Z
Learning: Avoid suggesting changes to deprecated functions such as `dydx_update_perpetual_v1_handler` in `indexer/services/ender/src/scripts/handlers/dydx_update_perpetual_v1_handler.sql` if they are unchanged in the PR.
Applied to files:
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts
📚 Learning: 2025-07-17T14:40:45.384Z
Learnt from: lukabuz
Repo: dydxprotocol/v4-chain PR: 2989
File: indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts:171-177
Timestamp: 2025-07-17T14:40:45.384Z
Learning: In the dydx v4-chain indexer comlink service, the Transaction abstraction from the postgres package is intended for use within the postgres package itself, not in service layers like comlink. Using it in comlink can cause unstable behavior where other database calls fail due to default transaction ID issues.
Applied to files:
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: check-build-auxo
- GitHub Check: build-and-push-testnet
- GitHub Check: run_command
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Analyze (go)
- GitHub Check: Summary
🔇 Additional comments (5)
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts (1)
58-62: LGTM! Consistent read replica usage.Using
readReplica: truefor both pre- and post-generation queries improves consistency and properly separates read traffic from write operations.indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (4)
13-13: LGTM! Clearer variable naming.The rename from
subaccountZerosWithoutUsernametotargetSubaccountsis more concise and clearly indicates these are the subaccounts targeted for username generation.
22-42: Excellent fix! Transaction removal prevents collision cascades.Removing the transaction wrapper correctly isolates username insertions. Now a
UniqueViolationErrorcollision on one subaccount won't rollback successful inserts for other subaccounts, which aligns perfectly with the PR objective.The addition of
successCounttracking (line 22, 41) provides valuable observability into the success rate.Based on learnings, the Transaction abstraction should not be used in service layers like roundtable, as it can cause unstable behavior with default transaction ID issues.
24-70: Verify behavior when all retry attempts are exhausted.The retry loop correctly handles collisions and other errors, but there's no explicit handling when all
ATTEMPT_PER_SUBACCOUNTattempts fail for a given subaccount. In this case:
- No username is created
- Errors are logged during iterations
successCountis not incremented- The task continues to the next subaccount
Please confirm this silent failure behavior is acceptable and that monitoring/alerting is in place to detect when
successCount < targetSubaccounts.lengthconsistently.
72-87: LGTM! Good observability improvements.The logging captures essential metrics:
batchSize: Total subaccounts processedsuccessCount: Successful username creationsaddressSample: Sample for debuggingduration: Performance trackingThis provides excellent visibility into the success rate and performance of the task.
| const subaccountsWithUsernames = await SubaccountUsernamesTable.findAll( | ||
| {}, | ||
| [], | ||
| { readReplica: true }, | ||
| ); | ||
| expect(subaccountsWithUsernames.length).toEqual(2); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the test file and examine seed functions
# First, locate the test file
echo "=== Looking for the test file ==="
find . -name "subaccount-username-generator.test.ts" -type f 2>/dev/null | head -5
# Search for seed function implementations in test support/mocks files
echo -e "\n=== Searching for seedData and seedAdditionalSubaccounts functions ==="
rg -l "seedData|seedAdditionalSubaccounts" --type=ts -g "**/*test*" -g "**/*mock*" 2>/dev/null | head -10
# Search for SubaccountUsernamesTable.create calls in test files
echo -e "\n=== Searching for SubaccountUsernamesTable.create calls ==="
rg "SubaccountUsernamesTable\.create" --type=ts -A 3 2>/dev/null | head -30Repository: dydxprotocol/v4-chain
Length of output: 3913
🏁 Script executed:
#!/bin/bash
# Get the test file content around lines 50-55
echo "=== Test file lines 1-70 ==="
head -70 ./indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts | cat -n
echo -e "\n=== Mock generators file to see seedData implementation ==="
grep -n "seedData\|seedAdditionalSubaccounts\|SubaccountUsernamesTable.create" indexer/packages/postgres/__tests__/helpers/mock-generators.ts | head -20Repository: dydxprotocol/v4-chain
Length of output: 2913
🏁 Script executed:
#!/bin/bash
# Read the mock-generators.ts file to see the full implementation
echo "=== Full seedData and seedAdditionalSubaccounts implementations ==="
sed -n '51,100p' indexer/packages/postgres/__tests__/helpers/mock-generators.ts | cat -n
echo -e "\n=== Also check what testConstants defines ==="
rg "defaultSubaccountId|defaultSubaccountId2" indexer/packages/postgres/src/test-constants.ts -A 1 2>/dev/null | head -20Repository: dydxprotocol/v4-chain
Length of output: 2308
🏁 Script executed:
#!/bin/bash
# Let me verify the complete beforeEach and the surrounding test context
echo "=== Full beforeEach and test context ==="
sed -n '18,70p' ./indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts | cat -n
echo -e "\n=== Check if there are other tests in the file ==="
grep -n "it(" ./indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.tsRepository: dydxprotocol/v4-chain
Length of output: 2224
🏁 Script executed:
#!/bin/bash
# Verify the actual assertion at line 38 matches the provided snippet
echo "=== Exact line 38 in the test ==="
sed -n '38p' ./indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts
echo -e "\n=== Verify seedData creates exactly how many usernames ==="
sed -n '58,68p' indexer/packages/postgres/__tests__/helpers/mock-generators.ts | cat -nRepository: dydxprotocol/v4-chain
Length of output: 653
🏁 Script executed:
#!/bin/bash
# Double-check the actual line numbers - get the full test structure
echo "=== Lines 18-60 of the actual test file ==="
sed -n '18,60p' ./indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts | cat -n
echo -e "\n=== Verify what deleteBySubaccountId does - is it really deleting? ==="
rg "deleteBySubaccountId" indexer/packages/postgres/src/stores/subaccount-usernames-table.ts -B 2 -A 10 | head -40Repository: dydxprotocol/v4-chain
Length of output: 1975
The test assertion at line 38 is incorrect and should expect 0 usernames, not 2.
The test seeds 2 subaccount usernames via seedData() and then immediately deletes both of them in the beforeEach hook (lines 22-27). Therefore, when SubaccountUsernamesTable.findAll() is called at line 50, there are 0 seeded usernames remaining, not 2. The expectation on line 55 should be toEqual(0) instead of toEqual(2).
🤖 Prompt for AI Agents
In
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts
around lines 50 to 55, the test incorrectly expects two usernames after seeding
because seedData() entries are deleted in the beforeEach hook; change the
assertion to expect 0 usernames (use toEqual(0)) so it matches the test setup
where both seeded usernames were removed before the findAll call.
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (1)
26-70: Consider logging when all collision retry attempts are exhausted.If all
ATTEMPT_PER_SUBACCOUNTattempts result in collisions (count == 0), the inner loop exits without incrementingsuccessCount, and the subaccount is silently skipped. Adding a warning log after the inner loop whensuccessCounthasn't increased would help identify subaccounts that couldn't get a username after all retries.🔎 Suggested enhancement
for (const subaccount of targetAccounts) { + const beforeCount = successCount; for (let i = 0; i < config.ATTEMPT_PER_SUBACCOUNT; i++) { const username: string = generateUsernameForSubaccount( subaccount.address, // Always use subaccountNum 0 for generation. Effectively we are // generating one username per address. The fact that we are storing // in the `subaccount_usernames` table is a tech debt. 0, // generation nonce i, ); try { const count: number = await SubaccountUsernamesTable.insertAndReturnCount( username, subaccount.subaccountId, { txId }, ); if (count > 0) { successCount += 1; break; } else { // if count is 0, log error and continue to next iteration // which will bump the nonce and try again with a new username logger.error({ at: 'subaccount-username-generator#runTask', message: 'Failed to insert username for subaccount', address: subaccount.address, subaccountId: subaccount.subaccountId, username, error: new Error('Username already exists'), }); } } catch (e) { logger.error({ at: 'subaccount-username-generator#runTask', message: 'Failed to insert username for subaccount', address: subaccount.address, subaccountId: subaccount.subaccountId, username, error: e, }); throw e; } } + if (successCount === beforeCount) { + logger.warning({ + at: 'subaccount-username-generator#runTask', + message: 'Exhausted all retry attempts for subaccount', + address: subaccount.address, + subaccountId: subaccount.subaccountId, + attempts: config.ATTEMPT_PER_SUBACCOUNT, + }); + } }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
indexer/packages/postgres/src/stores/subaccount-usernames-table.tsindexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.tsindexer/services/roundtable/src/tasks/subaccount-username-generator.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-17T14:40:45.384Z
Learnt from: lukabuz
Repo: dydxprotocol/v4-chain PR: 2989
File: indexer/services/comlink/src/controllers/api/v4/affiliates-controller.ts:171-177
Timestamp: 2025-07-17T14:40:45.384Z
Learning: In the dydx v4-chain indexer comlink service, the Transaction abstraction from the postgres package is intended for use within the postgres package itself, not in service layers like comlink. Using it in comlink can cause unstable behavior where other database calls fail due to default transaction ID issues.
Applied to files:
indexer/services/roundtable/src/tasks/subaccount-username-generator.ts
🧬 Code graph analysis (2)
indexer/packages/postgres/src/stores/subaccount-usernames-table.ts (2)
indexer/packages/postgres/src/types/utility-types.ts (1)
Options(15-25)indexer/packages/postgres/src/helpers/stores-helpers.ts (1)
rawQuery(62-80)
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts (2)
indexer/packages/postgres/src/types/db-model-types.ts (1)
SubaccountFromDatabase(22-27)indexer/services/roundtable/src/helpers/usernames-helper.ts (1)
generateUsernameForSubaccount(9-26)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (29)
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Public Testnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-comlink / (comlink) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-roundtable / (roundtable) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-auxo-lambda / (auxo) Build and Push Lambda
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-ender / (ender) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-vulcan / (vulcan) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-ecs-service-socks / (socks) Build and Push
- GitHub Check: (Mainnet) Build and Push ECS Services / call-build-and-push-bazooka-lambda / (bazooka) Build and Push Lambda
- GitHub Check: test / run_command
- GitHub Check: call-build-ecs-service-roundtable / (roundtable) Check docker image build
- GitHub Check: call-build-ecs-service-vulcan / (vulcan) Check docker image build
- GitHub Check: check-build-bazooka
- GitHub Check: call-build-ecs-service-socks / (socks) Check docker image build
- GitHub Check: call-build-ecs-service-comlink / (comlink) Check docker image build
- GitHub Check: call-build-ecs-service-ender / (ender) Check docker image build
- GitHub Check: check-build-auxo
- GitHub Check: build-and-push-mainnet
- GitHub Check: lint
- GitHub Check: build-and-push-testnet
- GitHub Check: run_command
- GitHub Check: Analyze (go)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Summary
🔇 Additional comments (3)
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts (1)
75-122: Excellent test coverage for collision handling!This test effectively validates the fallback behavior by:
- Pre-inserting a conflicting username for a different subaccount
- Verifying the target subaccount receives the fallback username (nonce 1)
- Confirming the original conflict persists for the other subaccount
The test design is comprehensive and provides good coverage for the collision retry logic.
indexer/packages/postgres/src/stores/subaccount-usernames-table.ts (1)
88-105: Clean implementation for collision-aware insertion!The
insertAndReturnCountfunction correctly usesON CONFLICT (username) DO NOTHINGwith aRETURNINGclause. This approach allows the caller to detect collisions via the returned row count (0 for conflict, 1 for success) without throwing exceptions, enabling the retry logic with fallback usernames.indexer/services/roundtable/src/tasks/subaccount-username-generator.ts (1)
58-95: Verify batch rollback behavior on unexpected errors.The catch block at lines 58-68 rethrows any unexpected exception (not collision-related), which triggers a rollback of the entire batch at line 89. This means if a single subaccount encounters an unexpected error (e.g., constraint violation, connection issue), all usernames inserted in the current batch are rolled back.
This fail-fast approach is conservative and may be intended, but please confirm this is the desired behavior rather than allowing partial batch success.
| const before = await SubaccountUsernamesTable.findAll( | ||
| {}, | ||
| [], | ||
| { readReplica: true }, | ||
| ); | ||
| expect(before.length).toEqual(0); | ||
|
|
||
| await subaccountUsernameGenerator(); | ||
| const subaccountsWithUsernamesAfter: SubaccountUsernamesFromDatabase[] = await | ||
| SubaccountUsernamesTable.findAll( | ||
| {}, [], {}); | ||
| const after = await SubaccountUsernamesTable.findAll( | ||
| {}, | ||
| [], | ||
| { readReplica: true }, | ||
| ); |
There was a problem hiding this comment.
Consider removing readReplica: true from test assertions.
Using readReplica: true in tests can cause intermittent failures due to replication lag. In test environments, the replica may not have caught up with writes from the primary database, leading to assertions reading stale data.
For reliable tests, consider reading from the primary database by omitting the readReplica option.
🔎 Suggested fix
const before = await SubaccountUsernamesTable.findAll(
{},
[],
- { readReplica: true },
+ {},
);
expect(before.length).toEqual(0);
await subaccountUsernameGenerator();
const after = await SubaccountUsernamesTable.findAll(
{},
[],
- { readReplica: true },
+ {},
);📝 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.
| const before = await SubaccountUsernamesTable.findAll( | |
| {}, | |
| [], | |
| { readReplica: true }, | |
| ); | |
| expect(before.length).toEqual(0); | |
| await subaccountUsernameGenerator(); | |
| const subaccountsWithUsernamesAfter: SubaccountUsernamesFromDatabase[] = await | |
| SubaccountUsernamesTable.findAll( | |
| {}, [], {}); | |
| const after = await SubaccountUsernamesTable.findAll( | |
| {}, | |
| [], | |
| { readReplica: true }, | |
| ); | |
| const before = await SubaccountUsernamesTable.findAll( | |
| {}, | |
| [], | |
| {}, | |
| ); | |
| expect(before.length).toEqual(0); | |
| await subaccountUsernameGenerator(); | |
| const after = await SubaccountUsernamesTable.findAll( | |
| {}, | |
| [], | |
| {}, | |
| ); |
🤖 Prompt for AI Agents
In
indexer/services/roundtable/__tests__/tasks/subaccount-username-generator.test.ts
around lines 50 to 62, the test uses { readReplica: true } when calling
SubaccountUsernamesTable.findAll which can read stale data and cause
intermittent failures; remove the { readReplica: true } option from both findAll
calls so the test reads from the primary database (ensuring reliable, up-to-date
results), leaving the rest of the test assertions unchanged.
| ); | ||
| expect(conflict.length).toBe(1); | ||
| expect(conflict[0].subaccountId).not.toEqual(targetSubaccount.id); | ||
| }); |
There was a problem hiding this comment.
Can you add a test case if there is 2 usernames in a batch, one works and the other doesnt?
|
@Mergifyio backport release/indexer/v9.x |
✅ Backports have been createdDetails
|
(cherry picked from commit 7586d7e)
Changelist
Test Plan
This was tested locally using the test suite
Author/Reviewer Checklist
state-breakinglabel.indexer-postgres-breakinglabel.PrepareProposalorProcessProposal, manually add the labelproposal-breaking.feature:[feature-name].backport/[branch-name].refactor,chore,bug.Summary by CodeRabbit
Tests
Refactor
Chores
✏️ Tip: You can customize this high-level summary in your review settings.