fix: server down due to infinite pagination bug#1128
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📝 WalkthroughWalkthroughAdds a confirmed-tx deduplication guard and cache cleanup in WebSocket handling, replaces block-sync linear checks with Set-based filtering, rewrites price sync to upsert only recent missing daily prices, and removes stray tokens in constants. ChangesProcessing & Price-sync Updates
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
services/chronikService.ts (1)
717-737:⚠️ Potential issue | 🟠 Major | ⚡ Quick winTransient failures can permanently suppress confirmed-tx reprocessing (Line 717).
isAlreadyBeingProcessed(msg.txid, true)marks the txid before thetry. If fetching/processing fails (non-404), the dedup flag remains set, so repeatedTX_CONFIRMEDmessages are ignored and the tx may never be added toconfirmedTxsHashesFromLastBlockfor block sync.Suggested fix
} else if (msg.msgType === 'TX_CONFIRMED') { - if (this.isAlreadyBeingProcessed(msg.txid, true)) return + if (this.isAlreadyBeingProcessed(msg.txid, true)) return try { const transaction = await this.fetchTxWithRetry(msg.txid) const addressesWithTransactions = await this.getAddressesForTransaction(transaction) console.log(`${this.CHRONIK_MSG_PREFIX}: [${msg.msgType}] ${msg.txid}`) this.confirmedTxsHashesFromLastBlock = [...this.confirmedTxsHashesFromLastBlock, msg.txid] for (const addressWithTransaction of addressesWithTransactions) { const { amount, opReturn } = addressWithTransaction.transaction await this.handleUpdateClientPaymentStatus(amount, opReturn, 'CONFIRMED' as ClientPaymentStatus, addressWithTransaction.address.address) } } catch (e: any) { const msg404 = String(e?.message ?? e) const is404 = /not found in the index|404/.test(msg404) if (is404) { console.log(`${this.CHRONIK_MSG_PREFIX}: [${msg.msgType}] tx ${msg.txid} not found in chronik, marking as orphaned`) await markTransactionsOrphaned(msg.txid) } else { + delete this.lastProcessedMessages.confirmed[msg.txid] console.error(`${this.CHRONIK_MSG_PREFIX}: confirmed tx handler failed for ${msg.txid}`, e) } } }🤖 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 `@services/chronikService.ts` around lines 717 - 737, The code marks a tx as "being processed" by calling isAlreadyBeingProcessed(msg.txid, true) before the try block, which leaves the dedup flag set on transient failures so future TX_CONFIRMED messages are ignored; change the flow so the tx is only marked-as-processing after a successful fetch/start or ensure the flag is cleared on non-404 failures: either move the isAlreadyBeingProcessed(..., true) call to just before/inside successful processing (e.g., after fetchTxWithRetry or just before awaiting handleUpdateClientPaymentStatus), or add a finally block that calls the corresponding clear/unmark operation for the dedup flag when an exception occurs (but keep 404/orphan handling intact and do not clear for true orphan case if desired); reference functions/fields: isAlreadyBeingProcessed, fetchTxWithRetry, handleUpdateClientPaymentStatus, confirmedTxsHashesFromLastBlock, and markTransactionsOrphaned to locate the code to change.
🤖 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.
Outside diff comments:
In `@services/chronikService.ts`:
- Around line 717-737: The code marks a tx as "being processed" by calling
isAlreadyBeingProcessed(msg.txid, true) before the try block, which leaves the
dedup flag set on transient failures so future TX_CONFIRMED messages are
ignored; change the flow so the tx is only marked-as-processing after a
successful fetch/start or ensure the flag is cleared on non-404 failures: either
move the isAlreadyBeingProcessed(..., true) call to just before/inside
successful processing (e.g., after fetchTxWithRetry or just before awaiting
handleUpdateClientPaymentStatus), or add a finally block that calls the
corresponding clear/unmark operation for the dedup flag when an exception occurs
(but keep 404/orphan handling intact and do not clear for true orphan case if
desired); reference functions/fields: isAlreadyBeingProcessed, fetchTxWithRetry,
handleUpdateClientPaymentStatus, confirmedTxsHashesFromLastBlock, and
markTransactionsOrphaned to locate the code to change.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69b72758-0ad5-4d5a-a295-957551e438ad
📒 Files selected for processing (1)
services/chronikService.ts
|
Failing test? @chedieck |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
services/priceService.ts (1)
156-220: 💤 Low valueOtherwise the rewrite reads well.
Per-network gating of the API fetch (lines 198–199), Set-based missing-day lookup (lines 202, 211), and using
moment.utc(price.day).unix()to align the inserted timestamp with the windowedstartOf('day')keys are all correct and a clear improvement over the prior implementation.Two small, optional notes (skip if intentional):
- The window loop on line 182 is inclusive of both endpoints, so
N_DAYS_LOOK_FOR_PRICE_GAPS = 30actually checks 31 days. The log on line 157/192 says "last 30 days". Either changeisSameOrAftertoisAfter, or update the log wording.getAllPricesByNetworkTicker(..., false)swallows errors and returnsnull; the resulting "no rows upserted" path is silent. Consider an explicit warning log whenallXECPrices/allBCHPricesisnullso an API outage is distinguishable from "no gaps" in operations.🤖 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 `@services/priceService.ts` around lines 156 - 220, The date-window loop in syncPastDaysNewerPrices uses cursor.isSameOrAfter(windowStart) which makes the range inclusive of both endpoints (so N_DAYS_LOOK_FOR_PRICE_GAPS = 30 yields 31 days) — change the loop to use cursor.isAfter(windowStart) or update the console messages to reflect the inclusive count; additionally, detect when getAllPricesByNetworkTicker(...) returns null for allXECPrices or allBCHPrices and emit a warning (use processLogger or console.warn) naming the variables allXECPrices/allBCHPrices and the network (NETWORK_TICKERS.ecash / NETWORK_TICKERS.bitcoincash) so API outages are visible instead of silently doing nothing.
🤖 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 `@services/priceService.ts`:
- Around line 162-188: The gap detection is only querying rows with quoteId:
USD_QUOTE_ID so days where USD upsert succeeded but CAD failed will be treated
as present; fix by making the dual-quote upsert atomic in
upsertPricesForNetworkId: wrap the two Prisma upserts (USD and CAD) inside a
single prisma.$transaction(...) so both succeed or both rollback (use
CAD_QUOTE_ID and USD_QUOTE_ID inside the transaction), and keep the
gap-detection logic (existingPrices / xecTimestamps / bchTimestamps) unchanged;
alternatively, if you prefer detection-side fix, change the query that builds
existingPrices to group by timestamp+networkId and require count === N_OF_QUOTES
so only fully-populated (both quotes) days are considered present.
---
Nitpick comments:
In `@services/priceService.ts`:
- Around line 156-220: The date-window loop in syncPastDaysNewerPrices uses
cursor.isSameOrAfter(windowStart) which makes the range inclusive of both
endpoints (so N_DAYS_LOOK_FOR_PRICE_GAPS = 30 yields 31 days) — change the loop
to use cursor.isAfter(windowStart) or update the console messages to reflect the
inclusive count; additionally, detect when getAllPricesByNetworkTicker(...)
returns null for allXECPrices or allBCHPrices and emit a warning (use
processLogger or console.warn) naming the variables allXECPrices/allBCHPrices
and the network (NETWORK_TICKERS.ecash / NETWORK_TICKERS.bitcoincash) so API
outages are visible instead of silently doing nothing.
🪄 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
Run ID: 3f0db9bd-d81d-4dad-ace9-47811a4197c9
📒 Files selected for processing (3)
constants/index.tsservices/chronikService.tsservices/priceService.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- services/chronikService.ts
| const existingPrices = await prisma.price.findMany({ | ||
| where: { | ||
| timestamp: { | ||
| gte: windowStart.unix(), | ||
| lte: today.unix() | ||
| }, | ||
| quoteId: USD_QUOTE_ID | ||
| }, | ||
| select: { timestamp: true, networkId: true } | ||
| }) | ||
| if (lastPrice === null) throw new Error('No prices found, initial database seed did not complete successfully') | ||
|
|
||
| const lastDateInDB = moment.unix(lastPrice.timestamp) | ||
| const date = moment().startOf('day') | ||
| const daysToRetrieve: string[] = [] | ||
| const xecTimestamps = new Set( | ||
| existingPrices.filter(p => p.networkId === XEC_NETWORK_ID).map(p => p.timestamp) | ||
| ) | ||
| const bchTimestamps = new Set( | ||
| existingPrices.filter(p => p.networkId === BCH_NETWORK_ID).map(p => p.timestamp) | ||
| ) | ||
|
|
||
| const expectedDays: Array<{ formatted: string, timestamp: number }> = [] | ||
| const cursor = today.clone() | ||
| while (cursor.isSameOrAfter(windowStart)) { | ||
| expectedDays.push({ formatted: cursor.format(PRICE_API_DATE_FORMAT), timestamp: cursor.unix() }) | ||
| cursor.subtract(1, 'day') | ||
| } | ||
|
|
||
| const missingXECDays = expectedDays.filter(d => !xecTimestamps.has(d.timestamp)) | ||
| const missingBCHDays = expectedDays.filter(d => !bchTimestamps.has(d.timestamp)) |
There was a problem hiding this comment.
Gap detection only checks USD; CAD gaps from a partial prior upsert will be missed.
upsertPricesForNetworkId upserts USD and CAD as two separate, non-transactional Prisma calls (see lines 32–68). If the USD upsert succeeds but CAD throws (network blip, timeout, etc.), the row set ends up with USD-only for that day. This new gap-filler queries existing rows with quoteId: USD_QUOTE_ID only, so any USD-without-CAD day will be considered "already present" and will never be backfilled.
Two reasonable options:
- Wrap the USD+CAD upserts in
prisma.$transaction(...)so they always succeed/fail together (preferred — also fixes the underlying invariant). - Detect gaps based on
(timestamp, networkId)having both quotes present, e.g. group by timestamp+networkId and require count =N_OF_QUOTES.
🛡️ Option 1: make the dual upsert atomic (root-cause fix)
export async function upsertPricesForNetworkId (responseData: IResponseData, networkId: number, timestamp: number): Promise<void> {
try {
- await prisma.price.upsert({
- where: { ... USD ... },
- ...
- })
-
- await prisma.price.upsert({
- where: { ... CAD ... },
- ...
- })
+ await prisma.$transaction([
+ prisma.price.upsert({
+ where: {
+ Price_timestamp_quoteId_networkId_unique_constraint: { quoteId: USD_QUOTE_ID, networkId, timestamp }
+ },
+ create: { quoteId: USD_QUOTE_ID, networkId, timestamp, value: new Prisma.Decimal(responseData.Price_in_USD) },
+ update: { value: new Prisma.Decimal(responseData.Price_in_USD) }
+ }),
+ prisma.price.upsert({
+ where: {
+ Price_timestamp_quoteId_networkId_unique_constraint: { quoteId: CAD_QUOTE_ID, networkId, timestamp }
+ },
+ create: { quoteId: CAD_QUOTE_ID, networkId, timestamp, value: new Prisma.Decimal(responseData.Price_in_CAD) },
+ update: { value: new Prisma.Decimal(responseData.Price_in_CAD) }
+ })
+ ])🤖 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 `@services/priceService.ts` around lines 162 - 188, The gap detection is only
querying rows with quoteId: USD_QUOTE_ID so days where USD upsert succeeded but
CAD failed will be treated as present; fix by making the dual-quote upsert
atomic in upsertPricesForNetworkId: wrap the two Prisma upserts (USD and CAD)
inside a single prisma.$transaction(...) so both succeed or both rollback (use
CAD_QUOTE_ID and USD_QUOTE_ID inside the transaction), and keep the
gap-detection logic (existingPrices / xecTimestamps / bchTimestamps) unchanged;
alternatively, if you prefer detection-side fix, change the query that builds
existingPrices to group by timestamp+networkId and require count === N_OF_QUOTES
so only fully-populated (both quotes) days are considered present.
There was a problem hiding this comment.
The bot is correct on that one. You could run the same prisma query for both quote ids and intersect them for finding the gaps, so you will override any price with a missing quote as well.
There was a problem hiding this comment.
@Fabcien, great suggestion — querying both quote IDs and intersecting gives you the tightest gap definition: a day is only "complete" if both USD and CAD rows exist. Here is a concrete implementation:
// Fetch rows for both quote IDs in a single query
const existingPrices = await prisma.price.findMany({
where: {
timestamp: {
gte: windowStart.unix(),
lte: today.unix()
},
quoteId: { in: [USD_QUOTE_ID, CAD_QUOTE_ID] } // ← both quotes
},
select: { timestamp: true, networkId: true, quoteId: true }
})
// Count distinct quotes per (networkId, timestamp)
const xecQuoteCounts = new Map<number, number>()
const bchQuoteCounts = new Map<number, number>()
for (const p of existingPrices) {
if (p.networkId === XEC_NETWORK_ID) {
xecQuoteCounts.set(p.timestamp, (xecQuoteCounts.get(p.timestamp) ?? 0) + 1)
} else if (p.networkId === BCH_NETWORK_ID) {
bchQuoteCounts.set(p.timestamp, (bchQuoteCounts.get(p.timestamp) ?? 0) + 1)
}
}
// A day is only "present" when all N_OF_QUOTES quotes exist
const xecTimestamps = new Set(
[...xecQuoteCounts.entries()]
.filter(([, count]) => count >= N_OF_QUOTES)
.map(([ts]) => ts)
)
const bchTimestamps = new Set(
[...bchQuoteCounts.entries()]
.filter(([, count]) => count >= N_OF_QUOTES)
.map(([ts]) => ts)
)The rest of the function (missingXECDays, missingBCHDays, fetch + upsert logic) stays unchanged. Any day where one quote succeeded and the other failed will have a count of 1, fall below N_OF_QUOTES, and be treated as a gap — triggering a full re-upsert of both quotes.
3 participants
Fix 1 — Line 717
Added if (this.isAlreadyBeingProcessed(msg.txid, true)) return to the TX_CONFIRMED handler. The dedup mechanism already existed (and was already used for TX_ADDED_TO_MEMPOOL) but was never called for confirmed txs. Now when the same txid fires 253 times (one per subscribed address), only the first one actually runs. This is safe because getAddressesForTransaction already fetches ALL related addresses for the tx — you don't need N websocket events to cover N addresses.
Fix 2 — Lines 807-809
Changed syncBlockTransactions pagination to use a Set instead of the raw array:
With this deployed, the DOIT airdrop scenario goes from 253 sequential Chronik fetches + 253 DB query rounds + broken pagination → 1 fetch + 1 query round + correct early termination.
Summary by CodeRabbit
Bug Fixes
Refactor
Chores