retry transient CCloud errors when fetching Flink statement results - #3427
Draft
Dave Shoup (shouples) wants to merge 5 commits into
Draft
retry transient CCloud errors when fetching Flink statement results#3427Dave Shoup (shouples) wants to merge 5 commits into
Dave Shoup (shouples) wants to merge 5 commits into
Conversation
A single 429 or 5xx from the results endpoint marked the stream "completed", so the viewer showed "Failed to load results" permanently even though the next poll would have succeeded. Retry those statuses on a bounded backoff, preferring the Retry-After delay Confluent Cloud sends on 429 over a locally-guessed one. Also clone the error response before parsing it, so logError() can still read the body for Sentry rather than throwing on a consumed body. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR hardens Flink statement results fetching against transient Confluent Cloud failures (429/5xx) by adding bounded retries with backoff (including Retry-After support) and fixes response-body consumption so transient failures are properly logged/reported.
Changes:
- Add transient retry classification (
isTransientResponseError) and use it when fetching statement results. - Implement
Retry-After-aware backoff window selection plus jittered exponential fallback for 5xx. - Expand unit test coverage for transient retries, backoff behavior, and response-body cloning; update changelog.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| tests/unit/testUtils.ts | Extends ResponseError test helpers to support headers and single-use responses. |
| src/flinkSql/flinkStatementResultsManager.ts | Adds transient retry/backoff logic and fixes response body extraction before logging. |
| src/flinkSql/flinkStatementResultsManager.test.ts | Adds unit tests for transient retries, Retry-After handling, and body cloning behavior. |
| src/errors.ts | Adds isTransientResponseError() helper for 429/5xx retry classification. |
| src/errors.test.ts | Adds test coverage for isTransientResponseError(). |
| CHANGELOG.md | Documents the user-visible fix for transient results loading errors. |
Retry-After is only sent once a rate limit is actually hit, so a 429 without it would have dropped straight to the guessed exponential delay. X-RateLimit-Reset rides along on every response and carries the same relative seconds. No epoch-normalization guard here: only /oauth/token reports Reset as an absolute epoch, and the sidecar owns that path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
The loop counter advanced on every iteration, so a transient retry also burned one of the 60 conflict attempts. Track each class separately and leave the loop header as a ceiling that should never bind. Caught by Copilot on #3427. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With Retry-After honored the per-sleep cap is 8s, so up to 4 retries could keep the manager alive and calling the API for ~34s after dispose(). Race the sleep against the abort signal instead. Only the results fetch opts into transient retries, so watching that controller can't cut stopStatement()'s own 409 retries short - it aborts the same controller before running them. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
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 of Changes
Submit a Flink statement and the Results Viewer sometimes shows "Failed to load results. Something
went wrong." and never recovers, even though the statement itself completes fine. This is the flake
behind the
flink-statementsE2E job.Confluent Cloud briefly rejects requests for a statement's results in the first seconds after it is
submitted, before the statement name is resolvable. We gave up on the very first rejection and
permanently stopped fetching, so the pane got stuck on the error even though the next poll (800ms
later) would have worked.
Now those rejections are retried a few times before giving up. When Confluent Cloud tells us how
long to wait, we wait exactly that long; otherwise we back off 500ms, 1s, 2s, 4s. Retries stop
immediately if you close the results pane.
Also fixes a logging bug in the same code path that was hiding these failures from Sentry entirely.
Click-testing instructions
important regression check.
retry()insrc/flinkSql/flinkStatementResultsManager.tsand make the firstgetSqlv1StatementResult()call reject with a 429 or 500. The pane should recover instead of showing the error.
Forcing the real failure isn't practical, since it depends on Confluent Cloud timing. The unit tests
carry the real coverage: each new behavior test was confirmed to fail against the unfixed code.
Optional: Any additional details or context that should be provided?
Implementation detail
Retried statuses are 429 and 5xx, via a new
isTransientResponseError()insrc/errors.ts. Delaycomes from
Retry-Afterif present, elseX-RateLimit-Reset, else a jittered exponential curve,all capped at 8s (
transientBackoffWindow()). 409 keeps its existing 60-attempt budget, and the twobudgets no longer consume each other. Transient retries are opt-in per call site, so
stopStatement()still fails fast on anything but a 409.Backoff sleeps race against the results
AbortController, so closing the pane stops retries ratherthan letting them run up to ~34s.
The logging fix: the error handler read the response body without cloning it, so
logError()thenthrew
Response.clone: Body has already been consumedand swallowed it. It now uses the existingextractResponseBody().Not doing proactive rate limiting off
X-RateLimit-Limit/Remaininghere. The sidecar already hasthat machinery in
CCloudApiRateLimiter; extending it to cover proxied requests is the better fix.That also explains why the sidecar update in #3419 didn't already cover this. Its limiter only
guards the calls the sidecar makes on its own behalf, not the ones it proxies for us. The failing
run was already on 0.273.0.
Follow-ups, one line each:
src/consume.tshas the same give-up-on-first-error shape and the same un-cloned body read.sanitizeHeaders()insrc/sidecar/middlewares.tscallsObject.entries()on aHeadersinstance, which always returns
[], so response headers never appear in our logs.Pull request checklist
Please check if your PR fulfills the following (if applicable):
Tests
Release notes