feat: job to clean up pending old payments #1066
Conversation
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughThis PR refactors client payment management by establishing a dedicated Changes
Sequence DiagramsequenceDiagram
participant API as Payment API
participant CPS as ClientPaymentService
participant DB as Database
participant Chronik as MultiBlockchainClient
participant Cleanup as Cleanup Job<br/>(Scheduled)
API->>CPS: generatePaymentId(address, amount)
CPS->>DB: Check if address exists
alt Address not registered
DB-->>CPS: Address not found
CPS->>Chronik: syncAndSubscribeAddresses
Chronik-->>CPS: Subscribed
end
CPS->>DB: Create clientPayment (PENDING)
DB-->>CPS: paymentId
CPS-->>API: Return paymentId
Note over Cleanup: Every CLIENT_PAYMENT_EXPIRATION_TIME<br/>(7 days)
Cleanup->>CPS: cleanupExpiredClientPayments()
CPS->>DB: Query PENDING payments<br/>older than cutoff
DB-->>CPS: Expired payment records
CPS->>DB: Delete expired payments
CPS-->>Cleanup: Cleanup complete (logged)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes The changes span multiple heterogeneous modifications: a new service module with straightforward CRUD operations, job orchestration setup, and non-trivial Chronik service restructuring (initialization simplification and new public APIs). Most changes follow consistent patterns, but the Chronik refactoring and service migration require careful verification of moved responsibilities and renamed imports across multiple files. Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧬 Code graph analysis (1)jobs/workers.ts (3)
⏰ 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)
🔇 Additional comments (1)
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.
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)
scripts/docker-exec-shortcuts.sh (1)
52-77: Restore login shell invocation for database/node shells.
bash -c bash -lruns a plainbashbecause-lbecomes the$0placeholder, so users no longer get a login shell (profile/env setup is skipped). The same regression affectsnodeshellandrootnodeshell. Please switch back to invokingbash -ldirectly (or quote the-ccommand properly) to keep the expected environment.Apply this diff:
- 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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
constants/index.ts(1 hunks)jobs/initJobs.ts(2 hunks)jobs/workers.ts(2 hunks)pages/api/payments/paymentId/index.ts(1 hunks)scripts/docker-exec-shortcuts.sh(2 hunks)services/chronikService.ts(4 hunks)services/clientPaymentService.ts(1 hunks)services/transactionService.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
jobs/workers.ts (5)
services/chronikService.ts (1)
multiBlockchainClient(1114-1114)services/transactionService.ts (1)
connectAllTransactionsToPrices(474-487)redis/clientInstance.ts (1)
redisBullMQ(54-54)constants/index.ts (1)
DEFAULT_WORKER_LOCK_DURATION(177-177)services/clientPaymentService.ts (1)
cleanupExpiredClientPayments(60-82)
services/chronikService.ts (1)
types/chronikTypes.ts (1)
SyncAndSubscriptionReturn(50-53)
jobs/initJobs.ts (3)
redis/clientInstance.ts (1)
redisBullMQ(54-54)jobs/workers.ts (2)
syncBlockchainAndPricesWorker(34-63)cleanupClientPaymentsWorker(65-86)constants/index.ts (1)
CLIENT_PAYMENT_EXPIRATION_TIME(295-295)
services/clientPaymentService.ts (4)
utils/validators.ts (1)
parseAddress(31-48)constants/index.ts (2)
NETWORK_IDS_FROM_SLUGS(132-135)CLIENT_PAYMENT_EXPIRATION_TIME(295-295)services/addressService.ts (1)
addressExists(127-141)services/chronikService.ts (1)
multiBlockchainClient(1114-1114)
🪛 GitHub Actions: Pull Request Tests
services/clientPaymentService.ts
[error] 54-54: PrismaClientInitializationError: The provided database string is invalid. Error parsing connection string: invalid port number in database URL.
🪛 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)
🔇 Additional comments (9)
jobs/workers.ts (2)
4-6: LGTM!The new imports are correctly wired to support the blockchain sync and client payment cleanup workers.
65-86: LGTM!The cleanup worker is well-structured with proper error handling and logging.
services/clientPaymentService.ts (3)
46-51: LGTM!The status update function is straightforward and correctly delegates error handling to callers.
60-82: LGTM!The cleanup function is efficient and well-structured:
- Minimizes data transfer by selecting only
paymentId- Uses bulk delete operation
- Provides clear logging for observability
53-58: Invalid DATABASE_URL in CI config — not a code issue. Ensure your CI pipeline setsMAIN_DB_USER,MAIN_DB_PASSWORD,MAIN_DB_HOST,MAIN_DB_PORT, andMAIN_DB_NAME(e.g., via GitHub Secrets) soDATABASE_URLresolves correctly.services/chronikService.ts (4)
13-18: LGTM!The import changes correctly reflect the relocation of client payment functions to the new service module.
Note: Line 13 imports
getSimplifiedTrasaction(typo in function name), but this is a pre-existing issue not introduced by this PR.
860-876: LGTM!The sync method properly handles errors with logging and continues execution, which is appropriate for background synchronization tasks.
960-968: LGTM!The constructor refactoring simplifies initialization by collecting all async operations and awaiting them together before marking as initialized. This is a cleaner approach.
1080-1086: LGTM!The parallel synchronization of both networks is efficient and correctly waits for initialization before proceeding.
| 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.
Add error handling for destroy() call.
The destroy() call is fire-and-forget with no error handling. If teardown fails, resources (e.g., WebSocket connections) may leak.
Apply this diff to add error handling:
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`)
+ try {
+ console.log('Cleaning up MultiBlockchainClient global instance...')
+ await multiBlockchainClient.destroy()
+ console.log(`job ${job.id as string}: blockchain + prices sync finished — cleanup done.`)
+ } catch (err: any) {
+ console.error(`job ${job.id as string}: cleanup FAILED — ${err.message}`)
+ }
})()
})📝 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`) | |
| })() | |
| }) | |
| worker.on('completed', (job) => { | |
| void (async () => { | |
| try { | |
| console.log('Cleaning up MultiBlockchainClient global instance...') | |
| await multiBlockchainClient.destroy() | |
| console.log(`job ${job.id as string}: blockchain + prices sync finished — cleanup done.`) | |
| } catch (err: any) { | |
| console.error(`job ${job.id as string}: cleanup FAILED — ${err.message}`) | |
| } | |
| })() | |
| }) |
🤖 Prompt for AI Agents
In jobs/workers.ts around lines 48 to 56, the async teardown invoked on worker
'completed' fire-and-forgets multiBlockchainClient.destroy() with no error
handling; wrap the await multiBlockchainClient.destroy() in a try/catch inside
the async IIFE (or convert to a named async function) so any thrown errors are
caught and logged (processLogger or console.error) and do not crash the worker,
and optionally perform a fallback/force cleanup in the catch; ensure the
function still resolves after logging the error.
| address = parseAddress(address) | ||
| const prefix = address.split(':')[0].toLowerCase() | ||
| const networkId = NETWORK_IDS_FROM_SLUGS[prefix] |
There was a problem hiding this comment.
Validate networkId before database operation.
If the address prefix is not in NETWORK_IDS_FROM_SLUGS, networkId will be undefined, causing a database constraint violation when creating the clientPayment.
Apply this diff to add validation:
address = parseAddress(address)
const prefix = address.split(':')[0].toLowerCase()
const networkId = NETWORK_IDS_FROM_SLUGS[prefix]
+ if (networkId === undefined) {
+ throw new Error(`Invalid network prefix: ${prefix}`)
+ }
const isAddressRegistered = await addressExists(address)📝 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.
| address = parseAddress(address) | |
| const prefix = address.split(':')[0].toLowerCase() | |
| const networkId = NETWORK_IDS_FROM_SLUGS[prefix] | |
| address = parseAddress(address) | |
| const prefix = address.split(':')[0].toLowerCase() | |
| const networkId = NETWORK_IDS_FROM_SLUGS[prefix] | |
| if (networkId === undefined) { | |
| throw new Error(`Invalid network prefix: ${prefix}`) | |
| } | |
| const isAddressRegistered = await addressExists(address) |
🤖 Prompt for AI Agents
In services/clientPaymentService.ts around lines 14 to 16, the computed
networkId may be undefined when the address prefix isn't found in
NETWORK_IDS_FROM_SLUGS which will cause a DB constraint violation; after
computing const networkId = NETWORK_IDS_FROM_SLUGS[prefix], validate that
networkId is defined and if not, immediately throw or return a clear validation
error (e.g., BadRequestError or similar used in the codebase) that includes the
invalid prefix/address, and stop further processing so the create clientPayment
DB call never runs with an undefined networkId.
| if (!isAddressRegistered) { | ||
| void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) | ||
| } |
There was a problem hiding this comment.
Add error handling for fire-and-forget synchronization.
The syncAndSubscribeAddresses call is fire-and-forget. If synchronization fails silently, the payment won't be tracked properly, but the function returns successfully.
Apply this diff to add error handling:
if (!isAddressRegistered) {
- void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address])
+ multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address])
+ .catch(err => console.error(`[CLIENT_PAYMENT] Failed to sync address ${clientPayment.address.address}: ${err.message}`))
}📝 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.
| if (!isAddressRegistered) { | |
| void multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) | |
| } | |
| if (!isAddressRegistered) { | |
| multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) | |
| .catch(err => console.error(`[CLIENT_PAYMENT] Failed to sync address ${clientPayment.address.address}: ${err.message}`)) | |
| } |
🤖 Prompt for AI Agents
In services/clientPaymentService.ts around lines 39 to 41, the fire-and-forget
call to multiBlockchainClient.syncAndSubscribeAddresses may fail silently;
change it to handle errors by either awaiting the promise inside a try/catch or
attaching a .catch handler that logs the error (and optionally records a metric
or retries). Specifically, replace the void call with an awaited call inside a
try { await
multiBlockchainClient.syncAndSubscribeAddresses([clientPayment.address]) } catch
(err) { logger.error("Failed to sync/subscribe address", { address:
clientPayment.address, error: err }) } or, if you must remain non-blocking, call
multiBlockchainClient.syncAndSubscribeAddresses(...).catch(err =>
logger.error(...)) so failures are surfaced and retriable/monitored.
chedieck
force-pushed
the
feat/clean-client-payment-instances-after-a-while
branch
from
0bc8fba to
7c43b62
Compare
There was a problem hiding this comment.
Pull Request Overview
Creates a recurrent job to clean up pending client payments older than 7 days, while refactoring client payment handling into a dedicated service and improving blockchain client lifecycle management.
- Extracts client payment functions into a dedicated service (
clientPaymentService.ts) - Adds a cleanup job that removes
PENDINGpayments older than 7 days - Refactors blockchain initialization to support explicit sync and teardown operations
Reviewed Changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| services/clientPaymentService.ts | New service containing client payment operations and cleanup functionality |
| services/transactionService.ts | Removes client payment functions that were moved to the new service |
| services/chronikService.ts | Updates imports and adds explicit sync/destroy methods for better lifecycle management |
| jobs/workers.ts | Adds new workers for blockchain sync and client payment cleanup |
| jobs/initJobs.ts | Initializes the new background jobs with appropriate scheduling |
| constants/index.ts | Defines the 7-day expiration time constant for client payments |
| pages/api/payments/paymentId/index.ts | Updates import to use new client payment service |
| tests/unittests/handleUpdateClientPaymentStatus.test.ts | Updates test mocks to reference new service |
| scripts/docker-exec-shortcuts.sh | Fixes shell command syntax for container access |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ;; | ||
| "databaseshell" | "dbs") | ||
| eval "$base_command_db" bash -l "$@" | ||
| eval "$base_command_db" bash -c bash -l "$@" |
There was a problem hiding this comment.
The bash command syntax is incorrect. It should be bash -l without the -c flag, or if using -c, it should execute a proper command string.
| eval "$base_command_db" bash -c bash -l "$@" | |
| eval "$base_command_db" bash -l "$@" |
| ;; | ||
| "rootnodeshell" | "rns") | ||
| eval "$base_command_node_root" bash -l | ||
| eval "$base_command_node_root" bash -c bash -l |
There was a problem hiding this comment.
The bash command syntax is incorrect. It should be bash -l without the -c flag, or if using -c, it should execute a proper command string.
| eval "$base_command_node_root" bash -c bash -l | |
| eval "$base_command_node_root" bash -l |
| }) | ||
|
|
||
| worker.on('failed', (job, err) => { | ||
| console.error(`[CLIENT_PAYMENT CLEANUP] job ${job?.id as string}: FAILED — ${err.message}`) |
There was a problem hiding this comment.
Using as string on a potentially undefined value can cause runtime errors. The job parameter can be null/undefined in failed events.
| console.error(`[CLIENT_PAYMENT CLEANUP] job ${job?.id as string}: FAILED — ${err.message}`) | |
| console.error(`[CLIENT_PAYMENT CLEANUP] job ${(job?.id ?? "unknown")}: FAILED — ${err.message}`) |
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
chedieck
force-pushed
the
feat/clean-client-payment-instances-after-a-while
branch
from
e4debef to
5690c86
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
services/chronikService.ts(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
services/chronikService.ts (1)
types/chronikTypes.ts (1)
SyncAndSubscriptionReturn(50-53)
⏰ 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 (3)
services/chronikService.ts (3)
6-18: LGTM! Import restructuring aligns with service separation.The import changes correctly reflect the extraction of client payment logic into
clientPaymentService, supporting the PR's goal of adding client payment cleanup functionality.
1080-1086: LGTM! Clean implementation of missed transaction sync.The method correctly waits for initialization via
waitForStart()before syncing both blockchain clients in parallel. This provides a clean public API for the new sync job functionality.
1093-1104: LGTM! Proper error handling in cleanup method.The
destroy()method correctly handles connection cleanup with error logging, addressing the previous review concern. The try-catch ensures that failures in closing one client's connections don't prevent cleanup of others.
chedieck
dismissed
lissavxo’s stale review
The merge-base changed after approval.
Related to #
Description
Creates a recurrent job that will clean
ClientPayments older than 7 days with the status'PENDING'.Test plan
Create some client payments, change the constant at the end of
constants/indexto a smaller time (instead of a week, use e.g.10_000for 10 seconds). Make sure payments are deleted as expectedSummary by CodeRabbit