experimental/ssh: retry tunnel binary upload on transient errors#5836
Open
TanishqDatabricks wants to merge 1 commit into
Open
experimental/ssh: retry tunnel binary upload on transient errors#5836TanishqDatabricks wants to merge 1 commit into
TanishqDatabricks wants to merge 1 commit into
Conversation
The binary-upload phase of ssh connect did a single Stat and single Write against the workspace-files API, so one transient failure aborted the whole connect. In practice this fires often: a cold get-status request routinely takes ~60s (right at the SDK's per-request inactivity timeout), and the import endpoint intermittently returns 5xx. Wrap the stat and the upload in a bounded retry (5 min) that re-runs on transient errors — SDK-classified retriable API errors (429/503/...) and request timeouts. The reader is re-fetched each attempt since Write drains it. fs.ErrNotExist (the signal to upload) and stream resets (a proxy body-size rejection, where retrying won't help) are not retried. Co-authored-by: Isaac
TanishqDatabricks
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
TanishqDatabricks
force-pushed
the
ssh-retry-binary-upload
branch
from
db6a7d6 to
33cd676
Compare
TanishqDatabricks
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
TanishqDatabricks
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
TanishqDatabricks
temporarily deployed
to
test-trigger-is
GitHub Actions
Inactive
Contributor
Waiting for approvalBased on git history, these people are best suited to review:
Eligible reviewers: Suggestions based on git history. See OWNERS for ownership rules. |
Collaborator
Integration test reportCommit: 33cd676
19 interesting tests: 15 SKIP, 2 RECOVERED, 2 flaky
Top 5 slowest tests (at least 2 minutes):
|
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
2 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.
Changes
The binary-upload phase of
databricks ssh connect(UploadTunnelReleases) did a singleStatfollowed by a singleWriteagainst the workspace-files API per architecture. A single transient failure on either call aborted the whole connect.In practice this fires often on a busy workspace:
get-statusrequest routinely takes ~60s — right at the SDK's per-request inactivity timeout — so the existence check tripsrequest timed out after 1m0s of inactivityand aborts. Repeating the same stat immediately after is sub-second (the endpoint is warm).Write) endpoint intermittently returns a 5xxInternal error.This wraps both operations in a bounded retry (5 min) that re-runs only on transient errors:
apierr.APIError.IsRetriable— 429, 503, etc.), andcontext.DeadlineExceeded/os.ErrDeadlineExceeded, plus the SDK's stringified"request timed out after ...").Not retried:
fs.ErrNotExistfromStat— that's the signal that the binary isn't there yet and must be uploaded; it resolves immediately.The upload retry re-fetches the release reader on each attempt, since
Writedrains it.Why
Transient workspace-files blips (cold get-status latency, intermittent 5xx) were the most common real-world failure when connecting: a single 60s stat timeout or one 5xx would kill an otherwise-healthy connect, and the fix was always just "run it again." A bounded retry absorbs these transparently.
Tests
New unit tests in
releases_test.gousing a scripted stub filer:TestIsRetriableUploadError— timeout string,context.DeadlineExceeded, retriable 503/429, non-retriable 404, plain error.TestBinaryExistsRetriesTransientStat— transient stat timeout then success.TestBinaryExistsNotFoundNoRetry—fs.ErrNotExistresolves in one call (no retry).TestBinaryExistsHaltsOnNonRetriable— a 403 halts immediately.TestUploadReleaseRetriesTransientWrite— transient 503 then success, re-fetching a fresh reader.TestUploadReleaseStreamResetNoRetry— stream reset fails fast with the proxy hint, no retry.go test ./experimental/ssh/...passes;gofmtandgo vetclean.Notes
This is an independent robustness fix, separate from the
--base-environmentenv-version work. It targets only the tunnel binary-upload path; the server-sidemetadata.jsonwrite (which can hit the same transient 5xx) is a candidate for the same treatment in a follow-up.This pull request and its description were written by Isaac.