-
Notifications
You must be signed in to change notification settings - Fork 8
feat: add robust error handling for transient network failures #395
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: development
Are you sure you want to change the base?
Conversation
Coverage report for commit: dbb940b
Summary - Lines: 82.17% | Methods: 95.81% | Branches: 68.25%
🤖 comment via lucassabreu/comment-coverage-clover |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR adds robust error handling for transient network failures to the Contentstack SDK, implementing enhanced retry mechanisms with exponential backoff and fine-grained control over different types of network errors. The changes include comprehensive retry logic for DNS failures, socket errors, timeouts, and HTTP server errors, along with SSRF protection for URL validation.
Key changes include:
- Enhanced retry configuration with network-specific retry options (DNS, socket, HTTP 5xx errors)
- Exponential backoff strategy with jitter for network retries
- SSRF protection through URL validation utilities
Reviewed Changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
File | Description |
---|---|
lib/core/concurrency-queue.js | Implements core network retry logic with transient error detection and enhanced retry mechanisms |
lib/core/Util.js | Adds URL validation utilities for SSRF prevention |
test/unit/concurrency-Queue-test.js | Updates test cases to handle new retry configuration and error handling patterns |
examples/robust-error-handling.js | Provides comprehensive examples of the new error handling configuration options |
.talismanrc | Adds security scanning exemption for the new example file |
Comments suppressed due to low confidence (2)
lib/core/concurrency-queue.js:328
- The retry condition check for error.response.status === 408 may fail for timeout errors that don't have a response object. This could cause the retry mechanism to not work properly for ECONNABORTED timeout errors that are later transformed to have a 408 status.
this.config.authorization = token.authorization
test/unit/concurrency-Queue-test.js:352
- The test assertion expects at least 2 log calls but doesn't verify the specific retry behavior or validate that the correct retry logic was triggered. Consider adding more specific assertions about retry attempts and error types.
expect(logHandlerStub.callCount).to.be.at.least(2) // Should have retry attempts
No description provided.