Conversation
Ensures that errors in logging do not disrupt the request-response cycle. Adds try-catch blocks around logger calls to prevent potential logger failures from affecting the main application flow. This ensures resilience in the face of logging issues.
✨ 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 |
There was a problem hiding this comment.
Pull request overview
This PR adds defensive error handling around logger calls to prevent potential logger failures from disrupting the request-response cycle. The changes wrap logger method invocations in try-catch blocks and fall back to console.error if the logger fails.
Changes:
- Added try-catch blocks around all logger method calls (
logger.error,logger.warn,logger.debug) - Introduced optional chaining (
logger?.) for logger calls inexpress-callback.js - Added
console.errorfallback when logger failures occur
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 9 comments.
| File | Description |
|---|---|
| src/handlers/response-validation.js | Wrapped the response validation failure logging in a try-catch block with console.error fallback |
| src/express-callback.js | Added try-catch blocks around access logging and error logging calls, with optional chaining and console.error fallback |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| try { | ||
| logger?.debug({ | ||
| url, | ||
| parameters, | ||
| post: request.body, | ||
| response: responseBody, | ||
| method, | ||
| ip, | ||
| userAgent, | ||
| responseTime, | ||
| statusCode: response.statusCode || 200, | ||
| message: 'access' | ||
| }) | ||
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) | ||
| } |
There was a problem hiding this comment.
Inconsistent null-safety pattern with logger parameter. The code uses optional chaining (logger?.debug) inside a try-catch block, but optional chaining already prevents errors if logger is null/undefined. Either remove the optional chaining operator (since the try-catch will handle any errors), or remove the try-catch block (since optional chaining prevents the error). The current implementation provides double protection that is redundant.
| try { | ||
| if (errorCodeStatus >= 500) { | ||
| logger?.error(error) | ||
| } else { | ||
| logger?.warn(error) | ||
| } | ||
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) | ||
| } |
There was a problem hiding this comment.
Inconsistent null-safety pattern with logger parameter. The code uses optional chaining (logger?.error and logger?.warn) inside a try-catch block, but optional chaining already prevents errors if logger is null/undefined. Either remove the optional chaining operator (since the try-catch will handle any errors), or remove the try-catch block (since optional chaining prevents the error). The current implementation provides double protection that is redundant.
| if (logger) { | ||
| logger.error({ | ||
| message: 'Response validation failed', | ||
| errors: valid.errors, | ||
| operation: context.operation, | ||
| statusCode: response.statusCode, | ||
| response: context.response | ||
| }) | ||
| try { | ||
| logger.error({ | ||
| message: 'Response validation failed', | ||
| errors: valid.errors, | ||
| operation: context.operation, | ||
| statusCode: response.statusCode, | ||
| response: context.response | ||
| }) | ||
| } catch (logError) { | ||
| // Prevent logging errors from affecting the response | ||
| console.error('Logger failed:', logError) | ||
| } |
There was a problem hiding this comment.
Inconsistent logger null-safety approach between files. In response-validation.js, the code uses if (logger) check followed by a try-catch, while in express-callback.js, it uses optional chaining (logger?.) with try-catch. For consistency and clarity, the same pattern should be used throughout the codebase. Consider standardizing on one approach across both files.
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) |
There was a problem hiding this comment.
Using console.error as a fallback when the logger fails creates a circular issue: if the logger failed due to a general logging infrastructure problem, console.error might also fail or be unavailable in certain environments. Additionally, this creates inconsistent logging output (some logs go through the configured logger, others through console), making log aggregation and monitoring more difficult. Consider either: 1) silently swallowing the error if resilience is the primary goal, or 2) using a more robust fallback mechanism.
| // Prevent logging errors from affecting the response | ||
| console.error('Logger failed:', logError) |
There was a problem hiding this comment.
Using console.error as a fallback when the logger fails creates a circular issue: if the logger failed due to a general logging infrastructure problem, console.error might also fail or be unavailable in certain environments. Additionally, this creates inconsistent logging output (some logs go through the configured logger, others through console), making log aggregation and monitoring more difficult. Consider either: 1) silently swallowing the error if resilience is the primary goal, or 2) using a more robust fallback mechanism.
| // Prevent logging errors from affecting the response | |
| console.error('Logger failed:', logError) | |
| // Intentionally ignore logging errors to avoid interfering with the response |
| try { | ||
| logger?.debug({ | ||
| url, | ||
| parameters, | ||
| post: request.body, | ||
| response: responseBody, | ||
| method, | ||
| ip, | ||
| userAgent, | ||
| responseTime, | ||
| statusCode: response.statusCode || 200, | ||
| message: 'access' | ||
| }) | ||
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) | ||
| } |
There was a problem hiding this comment.
The new error handling behavior for logger failures is not covered by tests. Since express-callback.test.js exists and tests other aspects of this function, consider adding test cases to verify that: 1) the application continues to work correctly when the logger throws an error, and 2) the fallback console.error is called when the logger fails. This ensures the resilience behavior works as intended.
| try { | ||
| logger?.debug({ | ||
| url, | ||
| parameters, | ||
| post: request.body, | ||
| response: responseBody, | ||
| method, | ||
| ip, | ||
| userAgent, | ||
| responseTime, | ||
| statusCode: response.statusCode || 200, | ||
| message: 'access' | ||
| }) | ||
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) | ||
| } |
There was a problem hiding this comment.
Consider whether this defensive error handling is necessary. Most production-grade logging libraries (Winston, Pino, Bunyan) handle errors internally and don't throw synchronous exceptions. If the logger is throwing errors, it typically indicates a configuration issue or bug that should be fixed rather than silenced. The added try-catch blocks increase code complexity and may hide underlying problems. Consider documenting which specific logger failure scenarios this protects against, or whether the logger interface requirements should be documented instead.
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) |
There was a problem hiding this comment.
Using console.error as a fallback when the logger fails creates a circular issue: if the logger failed due to a general logging infrastructure problem, console.error might also fail or be unavailable in certain environments. Additionally, this creates inconsistent logging output (some logs go through the configured logger, others through console), making log aggregation and monitoring more difficult. Consider either: 1) silently swallowing the error if resilience is the primary goal, or 2) using a more robust fallback mechanism.
| try { | ||
| if (errorCodeStatus >= 500) { | ||
| logger?.error(error) | ||
| } else { | ||
| logger?.warn(error) | ||
| } | ||
| } catch (logError) { | ||
| // Prevent logging errors from affecting the request | ||
| console.error('Logger failed:', logError) | ||
| } |
There was a problem hiding this comment.
The new error handling behavior for logger failures in the error handling path is not covered by tests. Since express-callback.test.js exists and tests error scenarios, consider adding test cases to verify that: 1) the application continues to handle errors correctly when the logger throws an error, and 2) the fallback console.error is called when the logger fails during error logging. This ensures the resilience behavior works as intended even in error scenarios.
2 participants
Ensures that errors in logging do not disrupt the request-response cycle.
Adds try-catch blocks around logger calls to prevent potential logger failures from affecting the main application flow. This ensures resilience in the face of logging issues.