Repository navigation
fix: recover Thrift CloudFetch downloads from rejected URLs, 4xx and stalls - #610
Open
vuanhphung wants to merge 2 commits into
Open
vuanhphung wants to merge 2 commits into
vuanhphung wants to merge 2 commits into
Conversation
…stalls A one-shot 401, 403 or 404 from cloud storage failed the query: CloudFetchResultHandler threw on any non-OK response and HttpRetryPolicy retries only 429 and 5xx. Other 4xx responses failed the same way, and a stalled attempt was never timed out, so it waited until retriesTimeout instead of being retried. On 401/403/404 the presigned URL itself was rejected, so fetch a fresh link for the same rows with FetchResults(startRowOffset) and download it, counting each refresh against retryMaxAttempts. RowSetProvider serializes its FetchResults calls and pages by explicit offset after a refresh, since the refresh may move the server cursor. Retry other 4xx with the same URL, and apply socketTimeout to each download attempt so a stalled one is retried. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
vuanhphung
temporarily deployed
to
azure-prod
November 13, 2026 08:47 — with 
GitHub Actions
Inactive
GitHub Actions
Inactive
vuanhphung
temporarily deployed
to
azure-prod
November 13, 2026 08:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
vuanhphung
temporarily deployed
to
azure-prod
November 13, 2026 08:47 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
![peco-review-bot[bot]](https://avatars.githubusercontent.com/in/3801177?s=60&v=4){kind=small}
There was a problem hiding this comment.
Verdict: 1 Medium · 1 Low
Mostly solid, with 2 concerns about timeout semantics. The per-attempt socketTimeout measures total elapsed time, while on Thrift requests it is an inactivity timeout, so lowering it can now fail large or slow downloads. Separately, link refreshes start a fresh retriesTimeout deadline each time, which loosens the end-to-end bound added in #520.
… refreshes Review follow-up. The per-attempt download timer now restarts on every received chunk, so socketTimeout aborts only an attempt that stops receiving data, not a large download still in progress. Link refreshes reuse the download's retriesTimeout deadline instead of starting a new one each. Signed-off-by: Vu Anh Phung <vu.phung@databricks.com> Co-authored-by: Isaac <no-reply@databricks.com>
vuanhphung
temporarily deployed
to
azure-prod
November 13, 2026 09:05 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
vuanhphung
temporarily deployed
to
azure-prod
November 13, 2026 09:05 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
vuanhphung
temporarily deployed
to
azure-prod
November 13, 2026 09:05 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
This branch was previously deployed
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.
On the Thrift backend, a one-shot 401, 403 or 404 from cloud storage on a CloudFetch download failed the query with
CloudFetch HTTP error 403 Forbidden.CloudFetchResultHandlerthrows on any non-OK response, andHttpRetryPolicyretries only 429 and 5xx, so the link was neither refreshed nor retried. Other 4xx responses (400, 405, 412) failed the same way. A stalled attempt was never timed out, so it waited untilretriesTimeout(15 minutes by default) instead of being retried. The cross-driver CloudFetch spec expects recovery in all three cases, and C#, JDBC and the kernel recover. Found by databricks-driver-test CLOUDFETCH-001..003 and 006..009, live against a warehouse with proxy-injected failures.The fix:
FetchResults(startRowOffset)and downloads it, as C# does. Each refresh counts as an attempt againstretryMaxAttempts. A refresh can move the server cursor, soRowSetProviderserializes itsFetchResultscalls and, after a refresh, requests later pages by explicit offset.HttpRetryPolicygets an opt-in for this, and only CloudFetch downloads use it.socketTimeoutnow also bounds each download attempt, and a timed-out attempt is retried like a network timeout (ETIMEDOUT, with no URL in the message).Testing: new unit tests run against a local HTTP server: refresh after 401/403/404, the refresh budget, same-URL 4xx retry, and the per-attempt timeout. Also covered: refresh offsets and serialization in
RowSetProvider, and theHttpRetryPolicyopt-in. The full unit suite passes (1763), and so do lint, type-check and prettier. Live against a warehouse, the databricks-driver-test CloudFetch spec tests on Thrift (CLOUDFETCH-001..003 and 006..009, in databricks/databricks-driver-test#2070) go from xfail to passing every assertion against this build, in 3 of 3 runs. For 401/403/404, the retry downloads the same blob with a new signature, one refreshFetchResultsafter the failure. The other CloudFetch spec tests still pass on both backends.This pull request and its description were written by Isaac.