experimental/ssh: survive a dropped tunnel connection instead of ending the session - #6558
Merged
Merged
Conversation
…ng the session A single TCP reset anywhere between the client and the workspace ended an `ssh connect` session, and both ends destroyed their state within milliseconds, so nothing survived to reconnect to. It happens several times a session for some users, badly enough that they avoid the terminal entirely. The proxy already knew how to swap a websocket underneath a running sshd - that is what the periodic auth handover does - but only for a connection that still works: the close frame is written after the last data frame, and TCP ordering is the whole safety argument. An abrupt reset provides no such barrier, so this adds the byte accounting that replaces it. Three changes, smallest first. ## A failed handover dial no longer ends the session The handover dials a replacement websocket every `--handover-timeout` (30m by default). Any failure of that dial ended the session, even though nothing had been swapped yet and the connection being replaced was still carrying traffic - so one transient blip dropped a healthy tunnel on a 30-minute clock. The pre-swap failure is now tagged so the client stays on its current connection and waits for the next tick. Deferring the refresh is safe: the driver proxy authenticates a websocket at upgrade time, so a live connection is not re-checked. Retrying the dial in place would not be safe. A dial can fail after the server has already accepted it and begun `acceptHandover`, and a second dial would then race the first for the same connection. ## A dropped session is no longer reported as a successful one `isSuccess` is set before the proxy loop starts and `category()` short-circuited on it, so every mid-session drop reached telemetry as a clean, successful session: the drop rate was unmeasurable and a fix for it unverifiable. The sites that end a session now carry sentinels (`ErrConnectFailed`, `ErrWebsocketDropped`, `ErrHandoverFailed`) mapped onto three new categories. `isSuccess` keeps its meaning - the tunnel was established - so `is_success` separates a failed connection attempt from a session that connected and was later cut short, and `error_category` gives the cause of either. `category()` no longer discards a category just because the tunnel came up, but still refuses to invent one: an established session that ended unattributed stays `TYPE_UNSPECIFIED`, because the ssh client and the user's own remote command both exit non-zero there and neither is a tunnel failure. Only a failed connection attempt falls back to `UNKNOWN`, as before. ## A dropped connection is reattached rather than mourned Both ends now count the payload they have handed to the websocket and the payload they have written to their destination, and keep a bounded tail of the former for replay. The buffer, not the socket, is the source of truth: bytes are appended before they are written, so a write that fails on a dying connection loses nothing. On a drop the client redials with the offset it has delivered, the server answers with its own, and each side replays exactly the difference. SSH verifies a MAC over the byte stream, so this has to be exact - one lost or duplicated byte would disconnect the session rather than repair it. Delivered counts are acknowledged every 64 KB so neither replay buffer grows. Server side, a dropped connection parks the session rather than reaping it: sshd stays alive, the client slot stays held (which is also what keeps the shutdown timer cancelled) and a grace timer tears it down exactly as before if nobody comes back. The three timings are each derived from a constraint rather than picked: the client gives up after 60s because ssh itself does at roughly 90s (`ServerAliveInterval` 30 x OpenSSH's default `ServerAliveCountMax` 3); the server's grace is 90s so it never reaps a client still trying; the 1 MB replay cap is far above the unacknowledged window that acks every 64 KB can produce, so reaching it means the peer stopped acknowledging and the connection is already beyond saving. ### Three ordering hazards, each handled - The write mutex cannot be held across the wait for the peer, because that is the lock the other side needs to complete the reattach - it deadlocks the server. `sendGate` throttles the sending loop instead; correctness rests on the write mutex plus appending before writing. - A reattach must not report a delivered count that can still move. The client often notices a drop first, and the server may still be draining data buffered on the dying connection, so `acceptReattach` retires that connection and waits for the receiving loop to stop delivering before it answers. Otherwise the client replays from a stale offset and the server writes those bytes to sshd twice. - A handover tick landing during a client reattach would wait out its own timeout for a receiving loop that is busy reattaching, and end the session. The client holds the write mutex across its whole reattach so the two serialize. ### Both ends must agree, and version skew is real The generated ssh config pins `--metadata` into the `ProxyCommand`, which skips the version-scoped server lookup, so an upgraded client can reach a server an older CLI started. The client therefore probes a new `/capabilities` endpoint first; an older server has no such route, 404s, and the session behaves exactly as it does today. A resume-capable server also refuses a reattach for a session it no longer holds (410) rather than starting a fresh sshd, since replaying into that would fail the SSH stream with a corrupted MAC - a worse error than the drop. The test server reports `resume: false`, because it drives sshd directly over the websocket rather than running the CLI's own proxy server and has none of the session bookkeeping a reattach needs. ## Tests Byte-exact delivery across single and repeated resets, driven through a TCP relay that sends real RSTs (`httptest`'s `CloseClientConnections` is no use: it does not touch the hijacked connections a websocket upgrade leaves behind). The reattach exchange is asserted frame by frame with a deliberately non-empty replay, so the handshake ordering is pinned rather than incidentally satisfied. Plus the replay arithmetic in isolation, the non-resumable path unchanged, the grace period releasing an abandoned session, and 410/400 on malformed reattach requests. `TestADropIsNeverReportedAsACleanExit` guards the errgroup race fixed in 75fcc00 from the drop side rather than the keepalive side. That fix keeps a cancellation from masking the error that caused it; reverting it fails this test within a few of its 25 attempts. It was found independently here while chasing an intermittent failure of the new drop test, and landed upstream first, so only the test remains. `category()` needed a real merge with the extension-category split in 3adaa48. Its precedence was cyclic across the two changes: an interruption had to outrank the category recorded at the failure site, `isSuccess` had to outrank an interruption, and a session-end category has to outrank `isSuccess`. Broken by making the split explicit instead - once the tunnel is up, the only reportable category is a session-end one, so an established session no longer consults the connection-attempt rules at all. Still to validate against real compute: whether the driver proxy passes the reattach query parameter through a websocket upgrade unchanged. `?id=` already shows query parameters survive, so this is a confirmation rather than a risk, but it is load-bearing for the handshake. Co-authored-by: Isaac <no-reply@databricks.com>
Collaborator
Integration test reportCommit: 873e42a
Top 29 slowest tests (at least 2 minutes):
|
rugpanov
reviewed
Sep 8, 2026
rugpanov
reviewed
Sep 8, 2026
rugpanov
reviewed
Sep 8, 2026
rugpanov
requested changes
Sep 8, 2026
Contributor
Author
|
Finding - cosmetic - #6558 warns "connection dropped" on every clean exit Every successful session end prints a false warning, with no --debug needed: $ databricks ssh connect --name bv-prewarm-lf -- true # exits 0, nothing went wrong Cause. On teardown OpenSSH SIGTERMs the ProxyCommand child; its websocket read then fails with something other than CloseNormalClosure, and because resumable() is true, proxy.go:428 enters reattach(), which logs resume.go:187 before failing fast on the already-cancelled context. Impact is presentation only - exit code and teardown timing are unaffected (measured: immediate). But it tells users their connection dropped when it did not, on every single session, which will generate support noise and erode trust in the genuine warning. Reproduced on every run including all seven passing e2e sessions. Suggested fix. Skip the reattach (and its warning) when the context is already cancelled or a shutdown is in progress. |
Three blocking review fixes on the resume/handover machinery: - proxy.go: a failed write on a resumable connection now closes the connection and returns errSendFailedResumable for ANY message type, not only binary payloads. A failed ack (a text control frame) previously only logged, so on a one-way-from-server flow the connection stayed write-poisoned: the receiving loop kept running, the peer stopped getting acks, its 1 MB replay buffer filled, and the session ended instead of reattaching. It now drives the same reattach path as a failed binary write. Buffering semantics are unchanged (only binary payloads are appended to the replay buffer). Adds a test covering the failed-ack case. - client_server_test.go: TestHandoverDialFailureKeepsSessionAlive no longer relies on the handover-tick send to imply the dial started. The fake dialer now signals when the handover dial is attempted (it runs while initiateHandover holds handoverMutex), and the test waits for that signal before sending post-handover traffic and asserting the dial count. This removes the race behind the macOS "dials == 1" flake. - .nextchanges: append the required trailing PR link to both new changelog fragments so the changelog preview check passes. Co-authored-by: Isaac <no-reply@databricks.com>
Every successful `ssh connect` session ended with a false warning, no --debug
needed:
$ databricks ssh connect --name bv-prewarm-lf -- true # exits 0
Connected!
Received termination signal, cleaning up...
Warn: SSH tunnel connection dropped, reattaching to the session...
On teardown the context is cancelled, start's context watcher closes the
websocket to unblock the receiving loop's read, and that read then fails with
something other than a normal closure. The loop's own check for cancellation
sits at the top, before the read, so a cancellation that lands while the read is
in flight was missed and the failure was taken for an unexpected drop: with
resume negotiated it entered reattach(), logged the warning, and only then
failed fast on the already-cancelled context.
Presentation was the whole impact - exit code and teardown timing were
unaffected - but it told users their connection had dropped on every single
session, which would erode trust in the genuine warning.
The receiving loop now returns the cancellation instead. Placed before the
resumable branch, because neither branch fits a teardown: the reattach cannot
succeed on a cancelled context anyway, and ErrWebsocketDropped would bill an
ordinary exit to a tunnel failure in telemetry - which matters now that an
established session no longer discards its error category.
The test pins the ordering that makes this exact rather than incidental: it
waits for a ping handler to fire from inside ReadMessage, which proves the read
cannot return until the connection is closed, then cancels and closes in the
same order the watcher does. Reverting the guard fails it, warning and all.
Co-authored-by: Isaac <no-reply@databricks.com>
rclarey
reviewed
Sep 8, 2026
| @@ -0,0 +1 @@ | |||
| * Keep an `ssh connect` session alive when the periodic auth handover cannot open its replacement websocket. A transient dial failure previously ended a healthy session, even though the connection being replaced was still working. ([#6558](https://github.com/databricks/cli/pull/6558)) | |||
Contributor
There was a problem hiding this comment.
I think we can combine the two changelog lines together and be more general about what was fixed
Contributor
Author
There was a problem hiding this comment.
Done in 873e42a: combined into a single fragment, .nextchanges/cli/ssh-tunnel-connection-drops.md, stated as what the user gets rather than the two mechanisms behind it:
ssh connectsessions no longer end when the tunnel's websocket connection is lost. The CLI reattaches to the running session and replays the bytes that were missed, so the shell and everything running in it stay intact, and a transient failure to open a replacement connection for the periodic auth refresh is retried rather than ending the session. Reattaching requires an SSH server started by a CLI that supports it; against an older server the connection behaves as before.
Comment on lines
+922
to
+929
| // | ||
| // This must be established before the session starts, not discovered when a connection drops. | ||
| // A server from an older CLI has no /capabilities endpoint and, worse, would answer a reattach | ||
| // request by starting a fresh sshd: replaying into that fails the SSH stream with a corrupted MAC | ||
| // instead of the clear error the user gets today. A 404 (or any other failure) therefore means | ||
| // "assume not supported". Version skew is real here because the generated ssh config pins | ||
| // --metadata into the ProxyCommand, which skips the version-scoped server lookup, so an upgraded | ||
| // client can reach a server an older CLI started. |
Contributor
There was a problem hiding this comment.
Don't need 7 lines explaining this. It can be assumed that if the server doesn't support resuming then we can't resume
Suggested change
| // | |
| // This must be established before the session starts, not discovered when a connection drops. | |
| // A server from an older CLI has no /capabilities endpoint and, worse, would answer a reattach | |
| // request by starting a fresh sshd: replaying into that fails the SSH stream with a corrupted MAC | |
| // instead of the clear error the user gets today. A 404 (or any other failure) therefore means | |
| // "assume not supported". Version skew is real here because the generated ssh config pins | |
| // --metadata into the ProxyCommand, which skips the version-scoped server lookup, so an upgraded | |
| // client can reach a server an older CLI started. |
Contributor
Author
There was a problem hiding this comment.
Done in 873e42a: dropped the block, kept the one-line doc comment (your suggestion range was 922-929, which is exactly the block below it). Nothing is lost - the hazard it described, an older server answering a reattach by starting a fresh sshd that a replay then corrupts, is already documented on RunClientProxys resumable parameter, where it governs the decision.
| // the auth refresh is safe: the driver proxy authenticates a websocket | ||
| // at upgrade time, so a live connection is not re-checked. | ||
| if errors.Is(err, errHandoverDialFailed) { | ||
| log.Warnf(gCtx, "Could not open a replacement connection for the auth handover, staying on the current one: %v", err) |
Contributor
There was a problem hiding this comment.
This will print into the interactive SSH session on top of whatever is already there. This warning is not actionable so at most it should be a debug log instead, or completely removed
Contributor
Author
There was a problem hiding this comment.
Done in 873e42a: now log.Debugf. Kept the message text so it is still there under --debug, rather than removing it entirely.
- proxy/client.go: log a failed handover dial at debug, not warn. The session carries on with the connection it already has, so there is nothing for the user to act on, and a warn writes into their interactive terminal on top of whatever the session is showing. - client.go: drop the rationale block on serverSupportsResume. Why an older server cannot resume needs no explaining, and the hazard it described - an old server answering a reattach by starting a fresh sshd, which a replay then corrupts - is already documented on RunClientProxy's resumable parameter, where it governs the decision. - .nextchanges: one fragment for the whole change instead of two, stated in terms of what the user gets rather than the two mechanisms behind it. Co-authored-by: Isaac <no-reply@databricks.com>
Contributor
Author
|
Fixed in 6553a80 (the false "connection dropped" warning on every clean exit). On teardown the context is cancelled, The guard sits before the
@rclarey one judgment call left open from your logging comment: it applies equally to the two warns this PR adds on a genuine drop - I left those at warn. Unlike the handover one they fire only on a real drop, and they are the difference between "my session froze for four seconds" and knowing why - the terminal is already disrupted at that point. By the strict not-actionable test they would be debug too, so happy to move them if you would rather. (The third new warn, "Reattach signal could not be delivered", is server-side and only reaches the driver logs.) |
rugpanov
approved these changes
Sep 8, 2026
rugpanov
left a comment
rugpanov
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed the latest version. The prior blocking findings are addressed, and CI is green.
anton-107
deleted the
deco-28433-ws-stability-investigate-connections
branch
September 8, 2026 17:57
Collaborator
Integration test reportCommit: 3cee3a1
373 interesting tests: 261 MISS, 110 FAIL, 1 KNOWN, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
rclarey
reviewed
Sep 9, 2026
Comment on lines
+99
to
+104
| // A handover that never got past its dial leaves the current connection | ||
| // untouched and still carrying traffic, so ending the session over it | ||
| // would throw away a working tunnel - the failure mode customers see as | ||
| // a drop every handover interval. The next tick tries again. Deferring | ||
| // the auth refresh is safe: the driver proxy authenticates a websocket | ||
| // at upgrade time, so a live connection is not re-checked. Logged at |
Contributor
There was a problem hiding this comment.
A couple things here:
- nit: the handover is to reset the websocket max connection duration of 1h, not for auth
- since the max connection duration is 1h and the handover interval is 30m, a failed dial here means the next tick will be racing against the connection being closed by the server
I think we should either shorten the retry interval after the first missed handover, or just get rid of handovers all together since the resume protocal should now handle the connection being closed due to exceeding the max connection duration
deco-sdk-tagging Bot
added a commit
that referenced
this pull request
Sep 9, 2026
## Release v1.16.0 ### CLI * `aitools install` now registers the official Claude marketplace if it is missing before installing the Databricks Claude plugin. ([#6485](#6485)) * `databricks aitools install --output json` now reports an `error_category` for a failed or skipped install (per agent, and at the top level for a failure with no per-agent entry), giving coding agents and CI a stable classification of why an install did not complete. ([#6482](#6482)) * `databricks aitools install` honors `--output json`, emitting a structured `{scope, agents[...]}` document that reports each agent's delivery and install status so coding agents and CI can consume the result without scraping the text output. JSON mode requires `--scope` and `--agents` so the command runs without interactive prompts. ([#6481](#6481)) * `databricks bundle sync` now prints sync progress (`Action: PUT`, `Uploaded ...`) by default, matching `databricks sync`. Previously it was silent unless `--output` was passed. Use `--output json` for machine-readable output. ([#6568](#6568)) * Support major-only DBR runtime versions such as `19.x-scala2.13` in the cluster picker used by `databricks auth login --configure-cluster` and `databricks labs`. ([#6574](#6574)) * Deprecated the `databricks environments setup-local --constraints-only` flag in favour of the orthogonal `--no-dbconnect`; the flag still works as a hidden alias but is hidden from `--help` and prints a one-line deprecation notice, and will be removed in a later release. ([#6470](#6470)) * Add orthogonal `--no-constraints` and `--no-dbconnect` flags to `databricks environments setup-local`: `--no-constraints` skips writing the remote Python-version and dependency pins, and `--no-dbconnect` skips the databricks-connect dependency. ([#6464](#6464)) * `databricks environments setup-local` now reports a distinct `E_PROVISION_CONFLICT` error code in `--output json` when the project's dependencies conflict with the pins written for the target environment, making the requirements unsatisfiable (the same conflict surfaced as a `W_USER_CONSTRAINT_CONFLICT` warning); it is reported after the project files are written, without attempting the doomed provisioning, while other provisioning failures continue to report `E_PROVISION`. ([#6479](#6479)) * `databricks ssh connect` and `ssh setup` now verify the tunnel's SSH host key against the key the workspace published for the connection, recorded in `~/.databricks/ssh-tunnel-known-hosts/<name>` instead of `~/.ssh/known_hosts`. Reconnecting with a name used before no longer fails with `Host key verification failed` when the compute behind that name changed, and no longer needs a manual `ssh-keygen -R`; host blocks written by an earlier `databricks ssh setup` pick this up once you re-run it. ([#6557](#6557)) * Stop `databricks ssh connect --ide` from adding a duplicate entry to the IDE's Remote Explorer on every connect: the remote authority is now the SSH host alias alone, instead of embedding the per-instance remote OS user. ([#6550](#6550)) * Add `--max-clients` and `--server-timeout` flags to `databricks ssh setup`, and `--server-timeout` to `databricks ssh connect`. Both are fixed when the SSH tunnel server job is submitted, so `ssh setup` now serializes them into the generated `ProxyCommand` instead of falling back to the built-in defaults. ([#6547](#6547)) * `ssh connect` sessions no longer end when the tunnel's websocket connection is lost. The CLI reattaches to the running session and replays the bytes that were missed, so the shell and everything running in it stay intact, and a transient failure to open a replacement connection for the periodic auth refresh is retried rather than ending the session. Reattaching requires an SSH server started by a CLI that supports it; against an older server the connection behaves as before. ([#6558](#6558)) ### Bundles * Added PyDABs (Python) support for secrets: `Resources.add_secret` and the `secret_mutator` decorator. ([#6553](#6553)) * Fix job and pipeline environment dependencies with a `*` version wildcard (e.g. `numpy==2.5.*`) being treated as local file paths. ([#6555](#6555)) * Add the `postgres_snapshot_schedules` bundle resource for managing a Lakebase Postgres branch's automatic-snapshot schedule (direct deployment engine only). ([#6449](#6449)) ### Dependency Updates * Bump `github.com/databricks/databricks-sdk-go` from v0.175.0 to v0.177.0. ([#6448](#6448)) * Bump Terraform provider from v1.128.0 to v1.131.0. ([#6544](#6544))
This was referenced Sep 10, 2026
janniklasrose
pushed a commit
that referenced
this pull request
Sep 10, 2026
….16.0 (#6612) ## Changes Cherry-picks #6608 onto `release/v1.16.x` for the v1.16.1 patch release: - Revert of #6558 (`experimental/ssh: survive a dropped tunnel connection instead of ending the session`), reverting commit 3cee3a1, which is contained in v1.16.0. - The matching changelog fragment under `.nextchanges/cli/`, so v1.16.1's rendered changelog carries the entry. The revert applied to this branch with no conflicts and produces a patch identical to the one on `main`. ## Why v1.16.0 kills the SSH session on any continuous transfer larger than ~1 MiB. The replay buffer limit (`proxyResumeBufferLimit = 1<<20`) in `proxy.go` misreads in-flight data as a dead peer and exhausts the send window, ending the session prematurely. Affected: `databricks ssh connect --ide` (100% repro after 15-30s, when the VS Code server transfers several MB during startup), and any SSH tunnel transfer larger than 1 MiB in either direction. The reverted change bundled two independent fixes (pre-swap handover resilience and SSH telemetry categories) alongside the window limit. A targeted fix-forward on `main` (degrade a full window to non-resumable instead of killing the session) is the preferred long-term path; this patch-release branch takes the revert. ## Tests - `go build ./...` - `go test ./experimental/ssh/... ./libs/telemetry/...` — pass - `go test ./acceptance -run 'TestAccept/ssh'` — pass - `tools/validate_nextchanges.py` — pass This pull request and its description were written by Isaac. --------- Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
added a commit
that referenced
this pull request
Sep 10, 2026
Conflict in experimental/ssh/internal/proxy/keepalive_test.go: main's #6598 restructured keepaliveTestDialer so the pausable socket is handed to the test only after the websocket handshake completes, while this branch's revert of #6558 restores the pre-resume createWebsocketConnectionFunc signature (connID string instead of DialRequest). Kept both: #6598's dialer restructure and async keystroke write, expressed against the reverted signature. Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
added a commit
that referenced
this pull request
Sep 11, 2026
#6558 capped the tunnel's unacknowledged replay window at 1 MiB and read a full window as a peer that had stopped acknowledging, so any transfer past that ended the session. It shipped in v1.16.0, broke `ssh connect --ide` outright, and was reverted in #6608. No test noticed, in pre- or post-merge CI. This test moves 8 MiB in each direction and asserts the byte counts. It is the first ssh test to run on cloud since #4838 disabled the old cloud-only ones as too flaky; serverless CPU has no cluster to provision and no accelerator to wait for, and the cloud run costs about 25s, most of it serverless startup rather than the transfer. The byte counts are the assertion rather than the exit code, because the session could end with truncated output and exit code 0. Locally the test server never negotiates the resume protocol, so the local run covers the plumbing over a real ssh session and the cloud run is what would catch the window coming back. Co-authored-by: Isaac <no-reply@databricks.com>
This was referenced Sep 11, 2026
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
4 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.
A single TCP reset anywhere between the client and the workspace ended an
ssh connectsession, and both ends destroyed their state within milliseconds, sonothing survived to reconnect to. It happens several times a session for some
users, badly enough that they avoid the terminal entirely.
The proxy already knew how to swap a websocket underneath a running sshd — that is
what the periodic auth handover does — but only for a connection that still works:
the close frame is written after the last data frame, and TCP ordering is the whole
safety argument. An abrupt reset provides no such barrier, so this adds the byte
accounting that replaces it.
Three changes, smallest first.
A failed handover dial no longer ends the session
The handover dials a replacement websocket every
--handover-timeout(30m bydefault). Any failure of that dial ended the session, even though nothing had been
swapped yet and the connection being replaced was still carrying traffic — so one
transient blip dropped a healthy tunnel on a 30-minute clock. The pre-swap failure
is now tagged so the client stays on its current connection and waits for the next
tick. Deferring the refresh is safe: the driver proxy authenticates a websocket at
upgrade time, so a live connection is not re-checked.
Retrying the dial in place would not be safe. A dial can fail after the server has
already accepted it and begun
acceptHandover, and a second dial would then racethe first for the same connection.
A dropped session is no longer reported as a successful one
isSuccessis set before the proxy loop starts andcategory()short-circuited onit, so every mid-session drop reached telemetry as a clean, successful session: the
drop rate was unmeasurable and a fix for it unverifiable. The sites that end a
session now carry sentinels (
ErrConnectFailed,ErrWebsocketDropped,ErrHandoverFailed) mapped onto three new categories.isSuccesskeeps its meaning — the tunnel was established — sois_successseparates a failed connection attempt from a session that connected and was later
cut short, and
error_categorygives the cause of either.category()no longerdiscards a category just because the tunnel came up, but still refuses to invent
one: an established session that ended unattributed stays
TYPE_UNSPECIFIED,because the ssh client and the user's own remote command both exit non-zero there
and neither is a tunnel failure. Only a failed connection attempt falls back to
UNKNOWN, as before.The three new
SshTunnelErrorCategoryvalues (WEBSOCKET_CONNECT_FAILED,WEBSOCKET_DROPPED,HANDOVER_FAILED) get a companion PR in universe adding them toproto/logs/frontend/databricks_cli/enum.proto. Ingestion ignores unknown enumvalues, so the two can land in either order.
A dropped connection is reattached rather than mourned
Both ends now count the payload they have handed to the websocket and the payload
they have written to their destination, and keep a bounded tail of the former for
replay. The buffer, not the socket, is the source of truth: bytes are appended
before they are written, so a write that fails on a dying connection loses nothing.
On a drop the client redials with the offset it has delivered, the server answers
with its own, and each side replays exactly the difference. SSH verifies a MAC over
the byte stream, so this has to be exact — one lost or duplicated byte would
disconnect the session rather than repair it. Delivered counts are acknowledged
every 64 KB so neither replay buffer grows.
Server side, a dropped connection parks the session rather than reaping it: sshd
stays alive, the client slot stays held (which is also what keeps the shutdown timer
cancelled) and a grace timer tears it down exactly as before if nobody comes back.
The three timings are each derived from a constraint rather than picked: the client
gives up after 60s because ssh itself does at roughly 90s
(
ServerAliveInterval30 x OpenSSH's defaultServerAliveCountMax3); the server'sgrace is 90s so it never reaps a client still trying; the 1 MB replay cap is far
above the unacknowledged window that acks every 64 KB can produce, so reaching it
means the peer stopped acknowledging and the connection is already beyond saving.
Three ordering hazards, each handled
lock the other side needs to complete the reattach — it deadlocks the server.
sendGatethrottles the sending loop instead; correctness rests on the writemutex plus appending before writing.
notices a drop first, and the server may still be draining data buffered on the
dying connection, so
acceptReattachretires that connection and waits for thereceiving loop to stop delivering before it answers. Otherwise the client replays
from a stale offset and the server writes those bytes to sshd twice.
for a receiving loop that is busy reattaching, and end the session. The client
holds the write mutex across its whole reattach so the two serialize.
Both ends must agree, and version skew is real
The generated ssh config pins
--metadatainto theProxyCommand, which skips theversion-scoped server lookup, so an upgraded client can reach a server an older CLI
started. The client therefore probes a new
/capabilitiesendpoint first; an olderserver has no such route, 404s, and the session behaves exactly as it does today. A
resume-capable server also refuses a reattach for a session it no longer holds (410)
rather than starting a fresh sshd, since replaying into that would fail the SSH
stream with a corrupted MAC — a worse error than the drop.
The test server reports
resume: false, because it drives sshd directly over thewebsocket rather than running the CLI's own proxy server and has none of the session
bookkeeping a reattach needs.
Tests
Byte-exact delivery across single and repeated resets, driven through a TCP relay
that sends real RSTs (
httptest'sCloseClientConnectionsis no use: it does nottouch the hijacked connections a websocket upgrade leaves behind). The reattach
exchange is asserted frame by frame with a deliberately non-empty replay, so the
handshake ordering is pinned rather than incidentally satisfied. Plus the replay
arithmetic in isolation, the non-resumable path unchanged, the grace period
releasing an abandoned session, and 410/400 on malformed reattach requests.
TestADropIsNeverReportedAsACleanExitguards the errgroup race fixed in 75fcc00from the drop side rather than the keepalive side. That fix keeps a cancellation
from masking the error that caused it; reverting it fails this test within a few of
its 25 attempts. It was found independently here while chasing an intermittent
failure of the new drop test, and landed upstream first, so only the test remains.
category()needed a real merge with the extension-category split in 3adaa48. Itsprecedence was cyclic across the two changes: an interruption had to outrank the
category recorded at the failure site,
isSuccesshad to outrank an interruption,and a session-end category has to outrank
isSuccess. Broken by making the splitexplicit instead — once the tunnel is up, the only reportable category is a
session-end one, so an established session no longer consults the connection-attempt
rules at all.
Still to validate against real compute: whether the driver proxy passes the reattach
query parameter through a websocket upgrade unchanged.
?id=already shows queryparameters survive, so this is a confirmation rather than a risk, but it is
load-bearing for the handshake.
This pull request and its description were written by Isaac.