Skip to content

fix: retry Thrift OpenSession and server-rejected ExecuteStatement - #605

Open
vuanhphung wants to merge 2 commits into
mainfrom
fix/thrift-retry-open-session-and-rejected-execute
Open

vuanhphung wants to merge 2 commits into
mainfrom
fix/thrift-retry-open-session-and-rejected-execute

Conversation

@vuanhphung

@vuanhphung vuanhphung commented Nov 13, 2026 •

Copy link
Copy Markdown
Collaborator

Problem

On the Thrift backend a single transient HTTP error fails the connection or the statement, while the kernel (SEA) backend retries it:

  • OpenSession is not in retryableThriftMethods, so a 503, 429, 502, 504 or 408 on session open is surfaced at once (1 attempt), and retriesTimeout has no effect on it.
  • ExecuteStatement is retried only when the connection fails before the request is sent (fix: retry ExecuteStatement when the connection fails before the request is sent #598). A 429, or a 503 with Retry-After, means the server did not run the statement, but it is still surfaced.
  • HttpRetryPolicy does not retry 408 on any request.

Found by the cross-driver error-recovery spec (ERRORRECOV-001, 007–012): live runs against main (7be705e) through the test proxy showed exactly 1 Thrift attempt in every case, while the SEA leg retried and passed.

Change

  • Add OpenSession to the idempotent methods. A repeated open leaves at most an unused session, which the server expires.
  • Retry 408 in HttpRetryPolicy (idempotent requests, CloudFetch and feature-flag fetches).
  • ExecuteStatement now also retries 429 and 503 with Retry-After, the same rule as the kernel's non-idempotent strategy (src/client/retry.rs). 500, 502, 504, 408 and 503 without Retry-After are still not retried, so a statement can't run twice. UnsentRequestRetryPolicy is renamed NonIdempotentRetryPolicy.

All retries use the existing retryMaxAttempts / retriesTimeout budget.

Verification

  • npm test (1764 passing), npm run lint, npm run type-check, npm run prettier.
  • New unit tests cover OpenSession retry on 503/429/408, ExecuteStatement retry on 429 and 503+Retry-After, no retry on 503 without Retry-After, 500/502/504/408, and RetryError on exhaustion.
  • With this branch built, the databricks-driver-test ERRORRECOV-001/007/008/009/010/011/012 Thrift tests (databricks/databricks-driver-test#2074) pass the spec assertions live (their xfail tripwires throw XPASS), and ERRORRECOV-013/014 (Reyden KP001 recovery) still pass.

Not changed here: getRetryAfterHeader compares the header's seconds with retryDelayMin in milliseconds, so a Retry-After below 1000 seconds is ignored and the plain exponential backoff applies.

This pull request and its description were written by Isaac.

Thrift OpenSession was never retried, so a single 503, 429, 502, 504
or 408 failed the connection. ExecuteStatement was retried only when
the connection failed before the request was sent, so a 429 or a 503
with Retry-After (the server did not run the statement) failed too.
The kernel backend retries all of these.

Add OpenSession to the idempotent methods (a repeated open leaves at
most an unused session), retry 408 on idempotent requests, and retry
ExecuteStatement on 429 and on 503 with Retry-After. Other errors on
ExecuteStatement are still not retried. UnsentRequestRetryPolicy is
renamed NonIdempotentRetryPolicy.

Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: 1 Low

Looks good: one low concern about changelog wording. The retry classification is right: 429 and 503+Retry-After retry on ExecuteStatement, the ambiguous statuses don't, and OpenSession now uses the idempotent policy. Tests cover each case at the policy level and in the end-to-end connection tests. Follow-up work outside this PR: the existing Retry-After seconds-vs-milliseconds mismatch in getRetryAfterHeader, which the description already mentions.

Comment thread CHANGELOG.md Resolved
Co-authored-by: Isaac <no-reply@databricks.com>
Signed-off-by: Vu Anh Phung <vu.phung@databricks.com>

@peco-review-bot peco-review-bot Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ No issues identified by the review bot.

This branch was previously deployed

1 inactive deployment
azure-prod — 2cbb8c0c Deployed Nov 13, 2026 by vuanhphung via e2e-test (24.22.1, 24_22_1) #1998
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant