fix(DX-10060): retry transient network errors to prevent build crashes - #385
Draft
OMpawar-21 wants to merge 4 commits into
Draft
fix(DX-10060): retry transient network errors to prevent build crashes#385OMpawar-21 wants to merge 4 commits into
OMpawar-21 wants to merge 4 commits into
Conversation
Transient network-layer errors (ENOTFOUND, ENETUNREACH, ECONNRESET, ECONNREFUSED, EAI_AGAIN, ETIMEDOUT, EHOSTUNREACH, ENETDOWN) now trigger the SDK's configured retry policy instead of failing immediately. A combinedRetryCondition composes the user-supplied retryCondition with the new default network-error check. The user condition runs first; if it throws, a warning is emitted via logHandler and the SDK falls back to the default. The original config object is never mutated. ECONNABORTED is excluded — @contentstack/core classifies it as a structured TIMEOUT error and handles it separately. Resolves: SF Case #00060601 (SentinelOne) Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
✅ Snyk checks have passed. No issues have been found so far.
💻 Catch issues earlier using the plugins for VS Code, JetBrains IDEs, Visual Studio, and Eclipse. |
Coverage report
Show files with reduced coverage 🔻
Test suite run success717 tests passing in 36 suites. Report generated by 🧪jest coverage report action from 0d45b5b |
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
netrajpatel
previously approved these changes
Aug 3, 2026
…warnings Lines 182 and 184 in contentstack.ts (the catch block and logHandler warn path) were flagged uncovered by jest-coverage-report-action. Added test (g) which exercises a retryCondition that throws, verifies the SDK falls back to default retry behaviour, and asserts the warning is emitted via logHandler. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
🟡 Not ready to approve
There are correctness and consistency issues in the new retry helper/documentation and logging payload that should be addressed before merging.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.
Pull request overview
Adds default retry behavior for transient network-layer failures in the Delivery SDK by composing a new network-error retry check with any user-provided retryCondition, and documents/releases the change as a minor version bump.
Changes:
- Composes a
combinedRetryConditionin the stack client to retry common transient network error codes while preserving the original userretryConditionreference. - Adds a utility helper (
isTransientNetworkError) + constant set of retryable network error codes. - Introduces unit tests covering the new retry behavior and updates version/changelog to 5.6.0.
File summaries
| File | Description |
|---|---|
| test/unit/network-error-retry.spec.ts | Adds unit tests validating retries for specific transient network error codes and opt-out behaviors. |
| src/stack/contentstack.ts | Composes retry behavior with user retryCondition and adds warning logging when user logic throws. |
| src/common/utils.ts | Adds transient network error code set and helper used by the new retry logic. |
| package.json | Bumps SDK version to 5.6.0. |
| CHANGELOG.md | Documents the retry behavior change and release date for 5.6.0. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 3
- Review effort level: Lite
We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.
Comment on lines
+175
to
+179
| // Retry policy handlers. | ||
| // Network-layer errors (DNS failures, connection resets, etc.) are retried | ||
| // by default, composed on top of any user-supplied retryCondition. `config` | ||
| // itself is never mutated, so stack.config / client.defaults keep reflecting | ||
| // exactly what the consumer passed in. |
Comment on lines
+183
to
+185
| } catch (e) { | ||
| config.logHandler?.('warn', `[Contentstack SDK] retryCondition callback threw: "${(e as Error)?.message ?? e}". Check your retryCondition implementation. Falling back to default network-error retry behavior.`); | ||
| } |
Comment on lines
+43
to
+45
| export function isTransientNetworkError(error: any): boolean { | ||
| return !!error && typeof error.code === 'string' && TRANSIENT_NETWORK_ERROR_CODES.has(error.code); | ||
| } |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ENOTFOUND,ENETUNREACH,ECONNRESET,ECONNREFUSED,EAI_AGAIN,ETIMEDOUT,EHOSTUNREACH,ENETDOWN) are now retried using the SDK's configured retry policy instead of failing immediately.combinedRetryConditioncomposes any user-suppliedretryConditionwith the new default network-error check — the user condition runs first, the original config is never mutated.retryConditionthrows, a warning is emitted vialogHandlerand the SDK falls back to default retry behaviour.ECONNABORTEDis intentionally excluded —@contentstack/coreclassifies it as a structuredTIMEOUTerror.5.5.0→5.6.0(minor — crux retry logic changed).Test coverage
8 unit tests added covering: ENOTFOUND retry, ENETUNREACH/ETIMEDOUT (customer-reported codes), EAI_AGAIN, ECONNRESET composition with user retryCondition, ECONNABORTED exclusion,
retryOnError: false, andretryLimit: 0.🤖 Generated with Claude Code