Improve timeout handling and error recovery#667
Conversation
- Enhanced Lightning Network error handling with specific timeout detection - Made pathfinding and bot handler timeouts configurable via environment variables - Implemented exponential backoff retry strategy for failed payments - Added timeout monitoring and structured logging for better debugging - Enhanced PendingPayment model with error tracking and retry scheduling Fixes 60-second timeout issues that were causing bot unresponsiveness.
## Walkthrough
Timeouts for Lightning Network pathfinding and Telegram bot handling are now configurable via environment variables. The payment retry logic introduces exponential backoff with scheduled retries and enhanced error tracking. Logging utilities for operation duration and timeouts are added. The pending payment model is extended to store error and retry metadata. A new CLAUDE.md documentation file was added describing the project, architecture, and development setup.
## Changes
| File(s) | Change Summary |
|----------------------------------------------|------------------------------------------------------------------------------------------------|
| `.env-sample` | Added `LN_PATHFINDING_TIMEOUT` and `BOT_HANDLER_TIMEOUT` environment variables. |
| `app.ts` | Made Telegram bot handler timeout configurable via `BOT_HANDLER_TIMEOUT` environment variable. |
| `jobs/pending_payments.ts` | Added exponential backoff for retries, improved error handling, and updated retry conditions. |
| `ln/pay_request.ts` | Introduced configurable pathfinding timeout, detailed error handling, and enhanced logging. |
| `logger.ts` | Added `logTimeout` and `logOperationDuration` utility functions for structured logging. |
| `models/pending_payment.ts` | Extended model with `last_error` and `next_retry` fields for error and retry tracking. |
| `CLAUDE.md` | Added new documentation explaining project overview, architecture, and development setup. |
## Poem
> In the warren, timeouts now obey,
> Configurable, they hop and sway.
> With errors tracked and retries wise,
> The payments wait, then re-arise.
> Logs now sing of time elapsed,
> As bunnies code, no detail lapsed.
> 🐇✨ System hops ahead—no surprise! Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
npm error Exit handler never called! ✨ Finishing Touches
🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🔭 Outside diff range comments (1)
app.ts (1)
28-38: Critical: Fix options object overwriting bug.The
handlerTimeoutconfiguration is lost whenSOCKS_PROXY_HOSTis set because the options object is completely overwritten. The proxy configuration should be merged with the existing options.- let options: Partial<Telegraf.Options<CommunityContext>> = { handlerTimeout }; + let options: Partial<Telegraf.Options<CommunityContext>> = { handlerTimeout }; if (process.env.SOCKS_PROXY_HOST) { const agent = new SocksProxyAgent(process.env.SOCKS_PROXY_HOST); - options = { + options = { + ...options, telegram: { agent, }, }; }
♻️ Duplicate comments (1)
jobs/pending_payments.ts (1)
159-164: Duplicate exponential backoff logic.This is identical to the logic in the first function. Consider extracting to a shared utility function as suggested above.
🧹 Nitpick comments (6)
models/pending_payment.ts (1)
42-43: Consider the default value calculation for next_retry.The arrow function for
next_retrydefault works correctly, but consider if this calculation should be consistent with the retry logic elsewhere in the codebase.logger.ts (1)
37-51: Consider input validation for startTime parameter.The
logOperationDurationfunction is well-implemented with appropriate logging levels and structured data. Consider adding validation to ensurestartTimeis a valid timestamp to prevent negative durations.export const logOperationDuration = (operation: string, startTime: number, success: boolean = true) => { + if (startTime <= 0 || startTime > Date.now()) { + logger.warn(`Invalid startTime provided for operation ${operation}: ${startTime}`); + return; + } const duration = Date.now() - startTime;jobs/pending_payments.ts (2)
25-30: Consider extracting exponential backoff logic to a utility function.The exponential backoff calculation is well-implemented with reasonable base delay (5 minutes) and maximum delay (1 hour). However, this logic is duplicated in both functions.
+// Utility function for exponential backoff calculation +const calculateNextRetryDelay = (attempts: number): number => { + const baseDelay = 5 * 60 * 1000; // 5 minutes + const exponentialDelay = baseDelay * Math.pow(2, attempts - 1); + const maxDelay = 60 * 60 * 1000; // 1 hour max + return Math.min(exponentialDelay, maxDelay); +}; // Calculate exponential backoff delay -const baseDelay = 5 * 60 * 1000; // 5 minutes -const exponentialDelay = baseDelay * Math.pow(2, pending.attempts - 1); -const maxDelay = 60 * 60 * 1000; // 1 hour max -const nextRetryDelay = Math.min(exponentialDelay, maxDelay); +const nextRetryDelay = calculateNextRetryDelay(pending.attempts);
97-111: Enhance error handling with type safety.The enhanced error handling is a good improvement, but consider adding type safety for the payment object structure and error types.
// Enhanced error handling for different payment failure types -if (payment && typeof payment === 'object' && 'error' in payment) { +if (payment && typeof payment === 'object' && 'error' in payment && typeof payment.error === 'string') { pending.last_error = payment.error as string; - if (payment.error === 'TIMEOUT') { + const errorType = payment.error.toUpperCase(); + if (errorType === 'TIMEOUT') { logger.warn(`Payment timeout for order ${order._id}, attempt ${pending.attempts}`); - } else if (payment.error === 'ROUTING_FAILED') { + } else if (errorType === 'ROUTING_FAILED') { logger.warn(`Routing failed for order ${order._id}, attempt ${pending.attempts}`); } else { logger.error(`Payment failed for order ${order._id}, attempt ${pending.attempts}, error: ${payment.error}`); }ln/pay_request.ts (2)
26-91: Consider improving type safety for the return value.The function returns different types:
false, invoice object, payment object, or error object. This inconsistent typing could lead to runtime errors and makes it harder for callers to handle the response correctly.Consider defining a consistent return type:
type PayRequestResult = | { success: true; payment: any } | { success: false; error: 'INVALID_INVOICE' | 'EXPIRED' | 'TIMEOUT' | 'ROUTING_FAILED' | 'INSUFFICIENT_BALANCE' | 'UNKNOWN'; message?: string };This would make error handling more predictable and type-safe throughout the codebase.
163-164: Simplify the complex ternary expression for better readability.The nested ternary expression is hard to read and maintain.
attempts: 1, - last_error: payment && typeof payment === 'object' && 'error' in payment ? payment.error as string : 'UNKNOWN', + last_error: (payment && typeof payment === 'object' && 'error' in payment) + ? (payment.error as string) + : 'UNKNOWN',Or even better, extract the logic:
+ const errorType = (payment && typeof payment === 'object' && 'error' in payment) + ? payment.error as string + : 'UNKNOWN'; const pp = new PendingPayment({ amount: order.amount, payment_request: order.buyer_invoice, user_id: buyerUser._id, description: order.description, hash: order.hash, order_id: order._id, attempts: 1, - last_error: payment && typeof payment === 'object' && 'error' in payment ? payment.error as string : 'UNKNOWN', + last_error: errorType, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
.env-sample(1 hunks)app.ts(1 hunks)jobs/pending_payments.ts(4 hunks)ln/pay_request.ts(5 hunks)logger.ts(1 hunks)models/pending_payment.ts(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (2)
app.ts (1)
bot/modules/community/communityContext.ts (1)
CommunityContext(10-14)
jobs/pending_payments.ts (2)
models/index.ts (1)
Order(11-11)logger.ts (1)
logger(53-53)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Analyze (javascript)
🔇 Additional comments (7)
.env-sample (1)
64-68: LGTM! Clean timeout configuration additions.The new environment variables for configurable timeouts are well-documented with clear comments and reasonable default values. The naming convention is consistent with existing variables.
models/pending_payment.ts (1)
16-17: LGTM! Good additions for error tracking and retry scheduling.The new fields
last_errorandnext_retryproperly support the enhanced retry mechanism with error tracking.logger.ts (1)
26-35: LGTM! Well-structured timeout logging utility.The
logTimeoutfunction provides excellent structured logging for timeout events with all necessary context data.jobs/pending_payments.ts (2)
17-17: LGTM! Proper retry scheduling implementation.The
next_retryfilter ensures pending payments are only processed when their retry time has arrived, supporting the exponential backoff strategy.
115-115: LGTM! Correct operator change for attempt limit.Changing from
===to>=ensures that no attempts beyond the configured limit are processed, which is the correct behavior.ln/pay_request.ts (2)
137-154: Well-implemented error handling with proper categorization!The differentiated handling of timeout, routing failures, and other errors provides good visibility into payment failure reasons, which will help with debugging and monitoring. This aligns perfectly with the PR objectives for improved error recovery.
42-44: Verify that the LN_PATHFINDING_TIMEOUT environment variable is documented.Since this is a new configurable timeout, ensure it's properly documented in the environment configuration.
#!/bin/bash # Description: Check if LN_PATHFINDING_TIMEOUT is documented in environment files # Check for documentation in .env files and README fd -t f -e md -e env -e sample | xargs rg -l "LN_PATHFINDING_TIMEOUT" # Check the actual usage and default value rg -A 2 -B 2 "LN_PATHFINDING_TIMEOUT"
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
CLAUDE.md (2)
60-65: Minor grammar polish in the escrow flow stepsStep texts miss determiners (“The”, “a/an”) which slightly hurts readability.
-1. Seller creates order -2. Buyer takes order, seller pays hold invoice +1. The seller creates an order +2. The buyer takes the order and the seller pays a hold invoice
25-29: Duplicate “Testing” sections may confuse readersThere is a testing subsection under both Development Commands and Architecture. Consider merging or renaming one (e.g., “Test Setup”) to avoid redundancy.
Also applies to: 88-90
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
CLAUDE.md(1 hunks)ln/pay_request.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- ln/pay_request.ts
🧰 Additional context used
🪛 LanguageTool
CLAUDE.md
[uncategorized] ~61-~61: You might be missing the article “an” here.
Context: ...for trustless escrow: 1. Seller creates order 2. Buyer takes order, seller pays hold ...
(AI_EN_LECTOR_MISSING_DETERMINER_AN)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: Analyze (javascript)
- GitHub Check: ci_to_main
| const exponentialDelay = baseDelay * Math.pow(2, pending.attempts - 1); | ||
| const maxDelay = 60 * 60 * 1000; // 1 hour max | ||
| const nextRetryDelay = Math.min(exponentialDelay, maxDelay); | ||
| pending.next_retry = new Date(Date.now() + nextRetryDelay); |
There was a problem hiding this comment.
@grunch this piece of code is duplicated, should it be extracted as a function?
3 participants
Fixes 60-second timeout issues that were causing bot unresponsiveness.
Summary by CodeRabbit
New Features
Bug Fixes
Chores