feat(networking): opt-in network retry resilience [IDE-1890] - #686
Open
basti-snyk wants to merge 30 commits into
Open
feat(networking): opt-in network retry resilience [IDE-1890]#686basti-snyk wants to merge 30 commits into
basti-snyk wants to merge 30 commits into
Conversation
…-1890] Application authors can now enable the framework's multi-attempt network retry default via NETWORK_REQUEST_RETRIES_ENABLED, independently of PREVIEW_FEATURES_ENABLED. Behaviour is unchanged for consumers that do not set it, and an explicitly configured attempt count still wins. Additive only; no exported symbol or default behaviour is changed.
✅ 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. |
Contributor
Author
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.
…890] Delete the local boolPtr test helper in app_test.go and use the existing generic pkg/utils.Ptr helper instead, avoiding duplication. Pure dedup, no behavior change.
This comment has been minimized.
This comment has been minimized.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (d117fa3) |
…-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.
9 tasks
…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.
This comment has been minimized.
This comment has been minimized.
…t [IDE-2419] Replace the method/idempotency-key request axis with an endpoint deny-list: retry every request when transport-error retry is opted in, except calls to the monitor endpoint, which creates a snapshot resource and must not be silently duplicated. The method-based guard refused to retry POST /v1/test-dep-graph, the customer failure this retry work exists for; that request creates nothing and is safe to retry. isReplayableRequest is renamed to isRetryableRequest and now checks the request path instead of method/headers. The req.Body != nil && req.GetBody == nil guard is kept as-is (physical replay constraint, not a policy choice). Inverted Test_TransportRetry_OptIn_PostNotRetried into Test_TransportRetry_OptIn_PostRetried and added Test_TransportRetry_OptIn_MonitorPathNotRetried. Replaced the two Test_RetryMiddleware_TransportError_RequestAxis rows and rewrote Test_isReplayableRequest as Test_isRetryableRequest for the new rule.
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (d15612f) |
This comment has been minimized.
This comment has been minimized.
…eak [IDE-1890] Two automated PR-reviewer findings on #686: - The PREVIEW_FEATURES_ENABLED || NETWORK_REQUEST_RETRIES_ENABLED opt-in predicate was duplicated between defaultMaxNetworkRequestAttempts (app.go) and the transport-error retry check (retry_middleware.go). A future change to one site could silently desync the two policies. Extracted to internal/utils.NetworkRetriesEnabled, used by both call sites. - The transport-error retry branch returned a response alongside a retryable error without draining/closing its body. backoff.Retry discards that response before calling the round tripper again, so any http.RoundTripper implementation returning a non-nil response with a non-nil error (permitted by the interface contract) would leak the body and its connection on every retry. Guarded on response != nil before calling drainAndClose, since a transport error commonly carries a nil response and drainAndClose only nil-checks the body it's given.
An SSL-inspecting proxy can flush 200 OK headers/status and then have the upstream connection reset mid-body, leaving RoundTrip returning a truncated body that gets silently parsed as empty JSON. When the transport-retry opt-in is active, the request is retryable, and the response is JSON, RetryMiddleware now reads the body fully and swaps in a buffered io.ReadCloser before returning. If the read fails, it returns a *RetryAttemptError so the existing backoff.Retry loop retries (or fails loudly via backoff.Permanent on the last attempt) instead of returning a truncated body. Response is always non-nil alongside the error. With the opt-in unset, this branch is never entered: no read, no buffer, no added latency, no changed error surface, pinned by a regression test.
basti-snyk
pushed a commit
that referenced
this pull request
Aug 5, 2026
…eak [IDE-1890] Two automated PR-reviewer findings on #686: - The PREVIEW_FEATURES_ENABLED || NETWORK_REQUEST_RETRIES_ENABLED opt-in predicate was duplicated between defaultMaxNetworkRequestAttempts (app.go) and the transport-error retry check (retry_middleware.go). A future change to one site could silently desync the two policies. Extracted to internal/utils.NetworkRetriesEnabled, used by both call sites. - The transport-error retry branch returned a response alongside a retryable error without draining/closing its body. backoff.Retry discards that response before calling the round tripper again, so any http.RoundTripper implementation returning a non-nil response with a non-nil error (permitted by the interface contract) would leak the body and its connection on every retry. Guarded on response != nil before calling drainAndClose, since a transport error commonly carries a nil response and drainAndClose only nil-checks the body it's given.
Contributor
Author
|
/describe |
basti-snyk
changed the title
|
PR Description updated to latest commit (59b3025) |
1 similar comment
|
PR Description updated to latest commit (59b3025) |
Resolve dependency and connection-reset detector conflicts while preserving current main behavior and the shared cross-platform retry classifier.
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
PR Reviewer Guide 🔍
|
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
Contributor
Author
|
/describe |
|
PR Description updated to latest commit (102bfe6) |
rrama
reviewed
Aug 13, 2026
|
|
||
| assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) | ||
| assert.Equal(t, int32(3), atomic.LoadInt32(requestCount)) | ||
| } |
Contributor
There was a problem hiding this comment.
Nitpick: the 4 tests above are quite similar, consider converting into a table test like this:
func Test_NetworkRetryOptIn(t *testing.T) {
tests := []struct {
name string
statusSequence []int
retriesEnabled bool
requestAttempts int // 0 = leave unset
wantStatus int
wantCount int32
}{
{
name: "not opted in, single attempt",
statusSequence: []int{http.StatusServiceUnavailable, http.StatusOK},
wantStatus: http.StatusServiceUnavailable,
wantCount: 1,
},
{
name: "explicit attempt count wins over opt-in",
statusSequence: []int{http.StatusServiceUnavailable, http.StatusOK},
retriesEnabled: true,
requestAttempts: 1,
wantStatus: http.StatusServiceUnavailable,
wantCount: 1,
},
{
name: "non-retryable response not retried",
statusSequence: []int{http.StatusNotFound},
retriesEnabled: true,
wantStatus: http.StatusNotFound,
wantCount: 1,
},
{
name: "gives up after policy limit",
statusSequence: []int{http.StatusServiceUnavailable},
retriesEnabled: true,
wantStatus: http.StatusServiceUnavailable,
wantCount: 3,
},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
server, requestCount := newSequencedStatusServer(t, tt.statusSequence)
config := configuration.NewWithOpts()
if tt.retriesEnabled {
config.Set(configuration.NETWORK_REQUEST_RETRIES_ENABLED, true)
}
if tt.requestAttempts != 0 {
config.Set(middleware.ConfigurationKeyRequestAttempts, tt.requestAttempts)
}
config.Set(middleware.ConfigurationKeyRetryAfter, 1)
engine := CreateAppEngineWithOptions(WithConfiguration(config))
client := engine.GetNetworkAccess().GetUnauthorizedHttpClient()
resp, err := client.Get(server.URL)
require.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, tt.wantStatus, resp.StatusCode)
assert.Equal(t, tt.wantCount, atomic.LoadInt32(requestCount))
})
}
}
rrama
reviewed
Aug 13, 2026
|
|
||
| configFile, err := configuration.CreateConfigurationFile("gaf-retry-test.json") | ||
| require.NoError(t, err) | ||
| require.NoError(t, os.WriteFile(configFile, []byte(`{"internal_network_request_retry_allowed_paths": ["custom-retryable"]}`), 0600)) |
Contributor
There was a problem hiding this comment.
Nitpicks: Remove the hardcodings of the internal key:
require.NoError(t, os.WriteFile(configFile, fmt.Appendf(nil,
`{%q: ["custom-retryable"]}`, configuration.NETWORK_REQUEST_RETRY_ALLOWED_PATHS), 0600))
rrama
reviewed
Aug 13, 2026
|
|
||
| assert.Equal(t, http.StatusServiceUnavailable, resp.StatusCode) | ||
| assert.Equal(t, int32(1), atomic.LoadInt32(requestCount)) | ||
| } |
Contributor
There was a problem hiding this comment.
Nitpick: can table test with the test above:
func Test_NetworkRetryOptIn_AllowedPathsFromPersistedJSONConfig(t *testing.T) {
tests := []struct {
name string
allowlist []string
path string
wantStatus int
wantRetried bool
}{
{"declared path is retried", []string{"custom-retryable"}, "/v1/custom-retryable", http.StatusOK, true},
{"blank entry allows nothing", []string{""}, "/v1/monitor/npm", http.StatusServiceUnavailable, false},
}
for _, tt := range tests {
t.Run(tt.name, func(t *testing.T) {
fakehome := t.TempDir()
t.Setenv("HOME", fakehome)
t.Setenv("USERPROFILE", fakehome)
configFile, err := configuration.CreateConfigurationFile("gaf-retry-test.json")
require.NoError(t, err)
contents, err := json.Marshal(map[string][]string{
configuration.NETWORK_REQUEST_RETRY_ALLOWED_PATHS: tt.allowlist,
})
require.NoError(t, err)
require.NoError(t, os.WriteFile(configFile, contents, 0600))
server, requestCount := newSequencedStatusServer(t, []int{http.StatusServiceUnavailable, http.StatusOK})
config := configuration.NewWithOpts(
configuration.WithFiles("gaf-retry-test"),
configuration.WithSupportedEnvVarPrefixes("snyk_", "internal_"),
configuration.WithCachingEnabled(configuration.NoCacheExpiration),
)
config.Set(configuration.NETWORK_REQUEST_RETRIES_ENABLED, true)
config.Set(middleware.ConfigurationKeyRetryAfter, 1)
engine := CreateAppEngineWithOptions(WithConfiguration(config))
client := engine.GetNetworkAccess().GetUnauthorizedHttpClient()
req, err := http.NewRequest(http.MethodPost, server.URL+tt.path, bytes.NewReader([]byte(`{}`)))
require.NoError(t, err)
resp, err := client.Do(req)
require.NoError(t, err)
defer resp.Body.Close()
assert.Equal(t, tt.wantStatus, resp.StatusCode)
if tt.wantRetried {
assert.GreaterOrEqual(t, atomic.LoadInt32(requestCount), int32(2))
} else {
assert.Equal(t, int32(1), atomic.LoadInt32(requestCount))
}
})
}
}
rrama
reviewed
Aug 13, 2026
| for _, item := range v { | ||
| output = append(output, fmt.Sprintf("%v", item)) | ||
| } | ||
| return output |
Contributor
There was a problem hiding this comment.
This change will make this comment a lie:
go-application-framework/pkg/auth/authHost.go
Lines 62 to 65 in a23b761
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
3 participants
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.
User description
Description
Everything for IDE-1890, consolidated into one PR. All of it sits behind a single opt-in —
configuration.NETWORK_REQUEST_RETRIES_ENABLED. With that unset, behaviour is byte-identical to today, pinned by regression tests at every layer.Four pieces, in dependency order:
1. The opt-in itself (IDE-2412) — new
NETWORK_REQUEST_RETRIES_ENABLEDconfig key. When set, the retry middleware's default attempt count becomes 3 without also requiringPREVIEW_FEATURES_ENABLED. An explicitly configured attempt count still wins over both.2. Transport-error retry (IDE-2419) — today every transport error is wrapped in
backoff.Permanentand never retried, whatever the config says. NowECONNRESETand network timeouts (including DNS timeouts) are retried; DNSNotFound, context cancellation/deadline, and TLS/auth failures are not.Two details worth a reviewer's attention: the deny-check runs before the timeout allow-check, because
context.DeadlineExceededalso satisfiesnet.Error.Timeout()and a caller-abandoned request must never be retried. And WindowsWSAECONNRESET(10054) is matched explicitly — Go does not map it ontosyscall.ECONNRESET, so a Unix-only check would pass CI on Linux while silently never retrying on Windows.Retry eligibility is method- and configuration-based. Safe methods retry by default; mutating methods retry only when their path matches
NETWORK_REQUEST_RETRY_ALLOWED_PATHS. The default includes the monitor endpoint paths needed by current consumers, and applications such as the CLI can override the list through Configuration. Matching uses contiguous full path segments, so similarly named segments are unaffected.3. Truncated-response recovery — the part that fixes the reported customer. Behind an SSL-inspecting corporate proxy, the upstream HTTP/2 stream is reset mid-body after
RoundTriphas already returned200 OK. The truncated body is then parsed as an empty result and the scan reports zero vulnerabilities on a project that has them, roughly 1 session in 5. BecauseRoundTripreturns before the body is read, no amount of retry configuration reaches this — which is why pieces 1 and 2 alone did not fix it.The fix reads retryable 2xx JSON bodies inside the existing retry loop; a truncated read then becomes an ordinary retryable error the existing
backoff.Retryalready handles. The integrity-check buffer has a dedicated 4 MiB cap. Larger JSON bodies preserve the inspected prefix and continue streaming from the original body, while non-JSON responses remain untouched. Three attempts take session success from ~80% to ~99%.4. Review fixes — the opt-in predicate is extracted to
internal/utils.NetworkRetriesEnabledso the two call sites cannot drift, and a nil-guardeddrainAndClosecloses a response-body leak on the retried transport-error path.Design context: IDE-2050. CLI flag wiring is deferred to IDE-2415.
Automated review findings
The accumulated review findings have been addressed or evaluated against the final design:
internal/utils, not exported, to avoid growing GAF's public APIdrainAndClose(response.Body), which panics on the common nil-response transport-error path; the guard checks the response, not just the bodyKnown limits, stated deliberately
PREVIEW_FEATURES_ENABLEDalso activates all of this, matching the pre-existing attempt-count default. Intentional and tested.getErrorListswallows the identical read error on non-2xx responses. A real adjacent gap, left alone because changing it would alter flag-off behaviour.Size
~1380 added lines across 9 files, well over the 700-line guideline. Deliberate, and consolidated on request: this is one coherent opt-in feature whose pieces do not stand alone — 2 and 3 are inert without 1, and 3 is the only one that fixes the customer. The large majority is a three-layer test pyramid for correctness-sensitive retry-safety; production code is a small fraction of the diff.
Test plan
make format && make lint && make test && make generate;snyk code test— no new findingsChecklist
make testcurrently reports two unrelated repository/environment failures (Test_UfmPresenter_HumanReadablefixture drift andTestDetectProxyConfigdue the sandbox CA environment); the changed acceptance, integration, and unit paths passmake generate)make lint)Note
Medium Risk
Changes shared HTTP client retry semantics and can re-send POSTs on allowlisted paths; incorrect allowlist or buffering could affect scan results or duplicate side effects, though monitor paths are excluded and the feature is opt-in.
Overview
Adds an opt-in resilient network retry policy behind
NETWORK_REQUEST_RETRIES_ENABLED(or existing preview features), with flag-off behavior unchanged from today.Configuration: New keys for the opt-in flag and a path allowlist for retries on unsafe HTTP methods (defaults like
test-dep-graph,verify/token,feature_flags/evaluation). Default max attempts (3 vs 1) now follows sharedNetworkRetriesEnabled; env CSV and persisted JSON arrays are normalized for the allowlist.GetStringSlicenow handles JSON[]interface{}from config files.Retry middleware (when opted in): Retries transient transport errors (e.g. connection reset, network timeouts) instead of always treating RoundTripper errors as permanent. Status-code retries on POST/etc. are limited to allowlisted path segments; safe methods still retry broadly. Successful JSON responses up to ~4MB are buffered in the retry loop so truncated bodies after
200 OKcan trigger another attempt; monitor-style paths stay excluded.Tests: Large integration/unit coverage for opt-in gates, path rules, transport vs truncation recovery, and config wiring through the real app engine.
Reviewed by Cursor Bugbot for commit cbf8bd1. Bugbot is set up for automated code reviews on this repo. Configure here.
PR Type
Enhancement
Description
Introduce opt-in for network retry resilience.
Enable retries for transient transport errors.
Configure retry eligibility by method and path.
Maintain byte-identical behavior when disabled.
Diagram Walkthrough
flowchart LR A[Client Request] --> B{Retry Middleware}; B -- Transient Transport Error --> C{Retryable Transport Error?}; C -- Yes --> D{Request Allowed to Retry?}; D -- Yes --> B; D -- No --> E[Return Error]; C -- No --> E; B -- HTTP Response --> F{Retryable Status Code?}; F -- Yes --> G{Request Allowed to Retry?}; G -- Yes --> B; G -- No --> H[Return Response]; F -- No --> H;File Walkthrough
6 files
Add helper function for network retry enablement check.Integrate retry enablement and define default retry paths.Handle []interface{} for GetStringSlice configuration.Define new configuration constants for network retries.Implement opt-in network retry logic for transport errors and statuscodes.Define retryable transport errors and request criteria.6 files
Test network retry enablement helper function.Add comprehensive tests for network retry logic.Test GetStringSlice handling of []interface{}.Test Windows WSAECONNRESET error handling.Add extensive tests for retry middleware functionality.Test retryable transport error and request logic.1 files
Include Windows WSAECONNRESET in connection reset errors.