[vuanhphung]

A Thrift CloudFetch download that stalls (the cloud-storage GET is accepted but response headers or body never arrive) blocked the fetch forever. The GET had no timeout: the agent's socketTimeout only notifies on idle sockets, and node-fetch got no timeout. HttpRetryPolicy checks retriesTimeout only between attempts, so the overall budget never applied to an attempt that never finished. Reproduced on main against a local server: with socketTimeout: 2000 and retriesTimeout: 3000, the download was still pending after 8–15 s, for both header and body stalls.

This PR enforces retriesTimeout as the end-to-end deadline of each chunk download: every attempt (connect, headers, body) plus the retry backoff between attempts. When the deadline expires, the in-flight request is aborted, no further attempt is sent, and the fetch fails with RetryError (TIMEOUT_EXCEEDED). The message, CloudFetch result download failed: Retry timeout exceeded, follows the #567 format, including (last error: …) when an earlier attempt failed. The default stays 15 minutes, which is the existing retriesTimeout default and also the lifetime of a CloudFetch link (measured: 900 s from issue). A healthy download, even a slow one, only fails if it can't finish within its link's lifetime. A tighter fixed default would fail healthy downloads on slow links: ten concurrent ~21 MB chunks (the measured chunk size) need about 14 Mbit/s to finish in 2 minutes. Callers who want a tighter bound set retriesTimeout. A non-positive value turns the deadline off, and a value too large for a timer is clamped to the timer limit.

Verification: new unit tests use a local server with the real HttpRetryPolicy and node-fetch. They cover a header stall, a body stall, a deadline that expires during retry backoff (no further request is sent), a slow-but-progressing download, and retriesTimeout: 0. The stall tests fail on main and pass here. A new e2e test redirects real warehouse CloudFetch links to a stalled local server: it fails with TIMEOUT_EXCEEDED in about 3 s with retriesTimeout: 3000, and hangs on main. Existing CloudFetch e2e tests pass. npm test, lint, prettier, type-check and build pass.

Live check in the style of CLOUDFETCH-018: a local CONNECT proxy stalls every cloud-storage connection, with a 150 s budget over execute + drain. With retriesTimeout: 60000, this branch fails with RetryError TIMEOUT_EXCEEDED after 63 s. main with the same config has still not returned at 150 s. Both runs recorded 6 stalled downloads.

Performance, medians over alternating fresh-process runs: a live CloudFetch drain of 3M lineitem rows took 39.7 s on main vs 39.2 s here (4 runs each). A local 200 × 20 MB drain took 5328 ms vs 5319 ms (7 runs each). 5000 × 10 KB local chunks took 625 ms vs 684 ms, about 12 µs of fixed cost per chunk (AbortController + timer), which is negligible against a real ~1 s chunk download.

Not addressed here: the [sea] finding. The kernel owns that download loop. The pinned kernel (KERNEL_REV 6f4b6d6) already includes databricks/databricks-sql-kernel#323. In the same live stall check, useKernel: true failed with a TIMEOUT error after 21 s ("exceeded the 20.0s end-to-end deadline").

Addresses #520

This pull request and its description were written by Isaac.

[peco-review-bot]

Verdict: 1 Low

Looks good. I found one low-severity issue: a retriesTimeout too large for setTimeout turns the deadline off without any notice instead of being clamped. The race/abort logic is sound: the timeout error is checked before each attempt and in the attempt's catch, HttpRetryPolicy does not retry it, and Promise.race handles the rejection when download loses. One thing to consider as follow-up: retriesTimeout now also caps healthy CloudFetch downloads, so users who set a low value to fail fast on Thrift retries may see large chunk downloads aborted. The CHANGELOG and JSDoc document this; it is not a defect.

[peco-review-bot]

✅ No issues identified by the review bot.

[vuanhphung]

@bot Could the deadline also cancel the retry policy’s pending backoff? Promise.race returns TIMEOUT_EXCEEDED, but the referenced sleep timer keeps Node alive until backoff finishes. The current test prevents another request but explicitly waits for that surviving sleep.

[peco-engineer-bot]

Pushed 7f8d687 to fix/issue-520-cloudfetch-chunk-deadline addressing your request.

Started from 5c6d5c5. Yes, done. The deadline now cancels a pending backoff: invokeWithRetry takes an optional AbortSignal, and aborting it clears the sleep timer and rejects. CloudFetch now uses one controller per download, which the deadline aborts with the timeout error, so no backoff timer outlives it. The 503 test asserts that timer is cleared and no second request goes out, instead of waiting 2.2s. Added two HttpRetryPolicy tests: abort during a backoff, and abort before one starts. Unit tests (1535), type-check and prettier pass. npm run lint fails here because ESLint can't find the airbnb-base config, so lint still needs a run. E2E not run (no credentials).

[peco-review-bot]

Verdict: 1 Nit

Looks good, no blocking issues. I traced the deadline/abort flow through HttpRetryPolicy and node-fetch and found it correct: aborting during a backoff sleep rejects with the RetryError, an aborted attempt rethrows timeoutError and is not retried, and the RetryError payload keeps its {request, response} shape. On Node ≥22 (the engines range), global AbortController and signal.reason are available.

Other findings

• ⚪ Nit — CHANGELOG: a download that is slow but still making progress now fails once it runs past retriesTimeout. Users who set a low retriesTimeout only to tune Thrift RPC retries could start seeing CloudFetch failures on large chunks. Consider adding a short Compatibility: note, like the one on the #485 entry next to it.