feat(networking): retry transient transport errors behind opt-in [IDE-2419] - #690
Conversation
…-2419] Extend the network-retry opt-in (NETWORK_REQUEST_RETRIES_ENABLED, or PREVIEW_FEATURES_ENABLED) to also cover transient transport-level failures - connection resets and network timeouts - which were previously wrapped in backoff.Permanent and never retried regardless of retry configuration. Adds a two-axis allow-list mirroring net/http.Transport's own isReplayable: - error axis: connection reset (POSIX ECONNRESET and Windows WSAECONNRESET, matched by numeric errno since Go does not map the latter onto the former) or a network timeout (net.Error.Timeout()); DNS NotFound, TLS failures, and caller cancellation/deadline are denied. context.DeadlineExceeded also satisfies net.Error.Timeout(), so the deny-check runs before the timeout allow-check. - request axis: safe methods (GET/HEAD/OPTIONS/TRACE, or no method set) are always replayable; other methods are replayable only when they carry an Idempotency-Key or X-Idempotency-Key header. New file pkg/networking/middleware/retry_transport_error.go holds the two unexported predicates. retry_middleware.go's RoundTrip now checks them (gated behind the opt-in and the remaining attempt budget) before falling back to the existing unconditional backoff.Permanent behavior. Outside-in TDD: acceptance tests exercise the real composition root (CreateAppEngineWithOptions -> GetUnauthorizedHttpClient) against a real TCP server that simulates a connection reset; integration tests wire the real RetryMiddleware; unit tests cover the two predicates in isolation.
✅ 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. |
|
Warning This pull request is not mergeable via GitHub because a downstack PR is open. Once all requirements are satisfied, merge this PR as a stack on Graphite.
This stack of pull requests is managed by Graphite. Learn more about stacking. |
✅ 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. |
This comment has been minimized.
This comment has been minimized.
…IDE-2419] Log the transport-error retry at Warn instead of Debug so operators see it without raising verbosity, per explicit requirement. Driven by TDD: extended Test_RetryMiddleware_TransportError_ConnResetRetriedWhenOptedIn to capture zerolog output via a buffer and assert on `"level":"warn"`, confirmed RED against the prior Debug() call, then changed Debug() to Warn() in retry_middleware.go and confirmed GREEN. Remove WHAT-only comments (restating what the following code obviously does, or just repeating a test's name plus a plan-ID) across retry_middleware.go, retry_middleware_test.go, retry_transport_error.go, retry_transport_error_test.go, app_test.go, and constants.go. Genuine WHY comments (non-obvious design reasons, ordering constraints, numeric literals, external references) are kept. Remove duplicate test coverage now that unit tests fully exercise the classification functions: - Test_RetryMiddleware_TransportError_ErrorAxis and Test_RetryMiddleware_TransportError_RequestAxis trimmed to two rows each, enough to prove the middleware is wired to isRetryableTransportError/isReplayableRequest without re-testing their full branch coverage (already covered by Test_isRetryableTransportError and Test_isReplayableRequest). - Test_RetryMiddleware_TransportError_BodyReplayedOnEveryAttempt deleted: Test_TransportRetry_OptIn_RetriedRequestSentInFull now asserts every recorded body (not just the last) against expectedBody, subsuming it at the acceptance level.
Correction to the prior cleanup commit against this repo's stricter Rule 8 (comments only for non-obvious WHY, no reasoning/alternatives, no ticket IDs, no big blocks): - Cut the connResetErr and fakeTimeoutError rationale comments in retry_middleware_test.go: both fixtures are self-explanatory from their names and bodies. - Cut the WHAT-only resettingServerLog doc comment and the precedence-matrix table label in app_test.go; both restated what is already evident from the field names. - Condensed the networkRequestRetryAfterSecondsKey and newResettingServer comments in app_test.go to their essential WHY clause, dropping restated context. - Shrunk the NETWORK_REQUEST_RETRIES_ENABLED godoc in constants.go from a four-line block to a single trailing comment matching its neighbors' style; the precedence nuance with an explicit attempts count belongs in the ticket, not source. No ticket-ID references (IDE-1890-*/IDE-2419-*) remain in any of the six touched files. The errWSAEConnReset rationale, the context.DeadlineExceeded-before-timeout ordering note, and the SetLinger(0) RST-vs-FIN note are retained as genuine load-bearing WHY.
PR Reviewer Guide 🔍
|
5064317
into
feat/IDE-1890-network-retry-opt-in
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Automated verification
Ran a 4-lens review (semantic analysis, adversarial review, security scan, independent code review) against the diff between d117fa3 and 6a883e7.
Approval policy: this PR modifies actual Go source/test files (retry_middleware.go, retry_transport_error.go, constants.go, and their tests) rather than only dependency manifests (go.mod/go.sum). Per this automation's policy, only pure dependency-bump PRs are auto-approved — everything else requires human review, so this review does not approve the PR.
Summary
- Critical: none
- Should Fix: 2 (both minor — doc-comment accuracy and log-level consistency; see inline comments)
- Suggestion: 1 (dead/unreachable guard clause)
- Security: no exploitable issues found (opt-in gating, replayability check mirrors Go stdlib's own
isReplayable, bounded attempt budget, no sensitive data in the new log line). Corroborated by the PR's own green Snyk CI checks. - Build/Lint/Test:
go build ./...,make lint(0 issues), andgo test -race -coveron the touched packages all pass, with strong coverage (88–95%) across the new retry-classification and integration tests.
Overall this is a well-scoped, well-tested, additive change (two-axis allow-list gated behind the existing opt-in flags, no breaking changes to public API surface). The two Should-Fix items below are worth addressing before merge but are not blockers.
Sent by Cursor Automation: Automatic PR verification
| INPUT_DIRECTORY string = "targetDirectory" // INPUT_DIRECTORY ([]string) sets/returns the input directories that the application shall process | ||
| FLAG_EXPERIMENTAL string = "experimental" // FLAG_EXPERIMENTAL (boolean) returns if experimental features shall be enabled or not, workflows should register this value as a flag to indicate that they might change before being GAed | ||
| PREVIEW_FEATURES_ENABLED string = "internal_preview_features_enabled" // PREVIEW_FEATURES_ENABLED (boolean) indicates if preview features shall be enabled, this can be used to limit features to the preview version only | ||
| NETWORK_REQUEST_RETRIES_ENABLED string = "internal_network_request_retries_enabled" // NETWORK_REQUEST_RETRIES_ENABLED (boolean) opts in to the framework's resilient network-retry policy without requiring PREVIEW_FEATURES_ENABLED |
There was a problem hiding this comment.
Should Fix: This doc comment regressed compared to the pre-PR version — it dropped two facts a consumer of this exported constant needs: (1) that an explicitly configured attempts count always takes precedence over this switch, and (2) that this PR extends the flag's effect to also unlock transport-error retries in RetryMiddleware.RoundTrip (not just the default-attempts bump). Since pkg/ is this repo's public, load-bearing API surface, the godoc should stay accurate.
Suggested restore:
// NETWORK_REQUEST_RETRIES_ENABLED (boolean) opts the application in to the framework's
// resilient network-retry policy, without requiring PREVIEW_FEATURES_ENABLED. When true,
// it also raises the default middleware.ConfigurationKeyRequestAttempts and enables
// retrying transient transport-level errors (e.g. connection resets, timeouts) for
// idempotent/replayable requests. An explicitly configured attempts count always takes
// precedence over this switch.| actualAttempts < attemptLimit && | ||
| isRetryableTransportError(rtErr) && | ||
| isReplayableRequest(&localRequest) { | ||
| rm.logger.Warn().Err(rtErr).Msgf("Retrying request after transient transport error (attempt %d/%d)", actualAttempts, attemptLimit) |
There was a problem hiding this comment.
Should Fix: This logs at Warn on every in-progress transport-error retry, while the existing status-code retry path logs at Debug while retrying and only escalates to Warn once retries are ultimately exhausted (see filterRetryError, ~line 310). With this opt-in enabled, a request that hits one transient reset and recovers on the next attempt now emits a Warn for what is a routine, successful resilience action — noisier than an equivalent 503/429 retry, and inconsistent with the rest of this file's logging convention.
Suggested fix — log at Debug here (matching the status-code path) and reserve Warn for exhaustion:
rm.logger.Debug().Err(rtErr).Msgf("Retrying request after transient transport error (attempt %d/%d)", actualAttempts, attemptLimit)|
|
||
| // isReplayableRequest mirrors net/http.Transport's own isReplayable. | ||
| func isReplayableRequest(req *http.Request) bool { | ||
| if req.Body != nil && req.Body != http.NoBody && req.GetBody == nil { |
There was a problem hiding this comment.
Suggestion: This req.GetBody == nil deny-check is unreachable from its only call site. isReplayableRequest(&localRequest) is always called after ensureGetBodyExists(req) has already run unconditionally in RoundTrip, which guarantees GetBody is non-nil whenever Body is non-nil/non-NoBody — that guarantee is carried into localRequest via the shallow copy. Not a bug (it's still correctly tested via Test_isReplayableRequest, which constructs a request that bypasses ensureGetBodyExists), but as written it implies this call site is protected against unreadable bodies when the real guarantee comes entirely from the upstream preprocessing. Consider a short comment noting this guard is for callers other than RetryMiddleware.RoundTrip, or drop it if isReplayableRequest is never meant to be called standalone.
1 participant
Description
Extends the opt-in from #686 (
configuration.NETWORK_REQUEST_RETRIES_ENABLED) to also retry transient transport-level connection errors (e.g.ECONNRESET), not just HTTP status codes. This closes a gap documented in IDE-2050: today, every transport error is wrapped inbackoff.Permanent(rtErr)and never retried, regardless of any retry configuration.Two-axis allow-list (both must hold to retry), per IDE-2050's resolved design:
ECONNRESET(including Windows'WSAECONNRESET/10054, which Go does not map onto the Unix errno) andnet.Errortimeouts (including DNS timeouts) — never DNSNotFound, context cancellation/deadline, or TLS/auth failures.net/http.Transport.isReplayable. A reset/timeout is ambiguous about whether the server already processed the request, so non-idempotent requests are never resent.Gating:
PREVIEW_FEATURES_ENABLED || NETWORK_REQUEST_RETRIES_ENABLEDactivates transport-level retry — mirroring the existing OR-condition already used for the attempt-count default indefaultMaxNetworkRequestAttempts(). Zero behavior change for anyone who doesn't set either flag.On a transport-level retry, a warn-level log line is emitted (no separate user-facing notification, consistent with how 5xx/408/425 retries already behave silently today — only 429 has a notification).
Correctness pitfalls specifically pinned by tests:
context.DeadlineExceededalso satisfiesnet.Error.Timeout()— the deny-list (context cancellation, DNSNotFound, TLS/auth) runs before the allow-list, so a caller-abandoned request is never retried just because it looks like a timeout.WSAECONNRESET(syscall.Errno(10054)) is checked explicitly alongsidesyscall.ECONNRESET, sinceerrors.Isalone does not bridge them — CI runs onwin/server-2022and this would otherwise pass on Linux while silently failing to retry on Windows.Implements IDE-2419, sub-task of IDE-1890, sibling of IDE-2412 (#686, base of this stacked PR) and IDE-2415 (deferred CLI wiring).
ADR-1amended to record this as an explicit decision-maker override of its original "real captured trace" gate on this change, based on documented customer demand rather than a satisfying trace.Note on size: this diff is ~900 lines (82 production, ~820 test) against the repo's 700-line PR guideline. Deliberately shipped as one PR rather than split — the bulk is a genuine 3-layer TDD pyramid (acceptance against the real composition root with a real TCP server simulating RST, integration wiring tests, and a full two-axis classifier matrix including the deny-before-allow-list and Windows-errno edge cases) for a correctness-sensitive retry-safety feature. Splitting production code from the tests that pin it would leave an interim PR either under-tested or carrying dead code.
Test plan
RST, opted-in consumer recovers from a connection reset on an idempotent requestTest_TransportRetry_PreviewFeaturesOnly_ConnectionResetRecoversRetryMiddlewarewiring, including a dedicated test that goes RED if thePREVIEW_FEATURES_ENABLED || NETWORK_REQUEST_RETRIES_ENABLEDOR-condition is reverted to flag-onlyWSAECONNRESEThandlingChecklist
make test)make generate)make lint)go get github.com/snyk/go-application-framework@YOUR_LATEST_GAF_COMMITin thecliv2directory.go.modto point to your local GAF code.go mod tidyin thecliv2directory.go.modandgo.sumchanges.Note
Medium Risk
Changes core HTTP retry behavior for transport errors (duplicate-request and exhaustion semantics), but it is gated behind opt-in/preview flags and covered by extensive safety tests including non-replayable POST and attempt limits.
Overview
Extends network retry middleware so opted-in apps (or preview features) can recover from transient transport failures—connection reset and network timeouts—not only retryable HTTP status codes.
Gating matches the existing resilient-retry opt-in:
PREVIEW_FEATURES_ENABLEDorNETWORK_REQUEST_RETRIES_ENABLED. No change for callers that set neither flag.Retry rules (both must pass):
ECONNRESET(including Windows WSA 10054),net.Errortimeouts; never context cancel/deadline, DNS not found, TLS/auth, or generic errors. Deny-list runs before timeout allow-list soDeadlineExceededis not retried.net/httpreplay rules); non-idempotent POSTs without a key stay single-attempt.On transport retry, a warn log is emitted; attempt budget and
Snyk-Request-Attempt-Countbehave like status-code retries. Newretry_transport_errorhelpers classify errors and replayability.Tests add a raw TCP server that forces RST, composition-root acceptance cases, and middleware/unit matrices for the two-axis policy.
Reviewed by Cursor Bugbot for commit 6a883e7. Bugbot is set up for automated code reviews on this repo. Configure here.