Fix SSH session resume with bounded backpressure - #6650
Merged
Merged
Conversation
DECO-28433: restore dropped-session reattachment without the full-window failure introduced by #6558 or the protocol degradation attempted in #6609. Wait for acknowledgments outside the write lock, send acknowledgments on a separate loop, and resume reads before replaying full windows. Negotiate a new protocol version to avoid enabling resume against the faulty server. Preserve final payload before EOF, distinguish session completion from a dropped socket, retry dropped handshakes, and stop retrying expired sessions. Regression tests reproduce the original 1 MiB truncation, an acknowledgment deadlock during handover, and a simultaneous replay deadlock. They verify 8 MiB transfers in both directions through handover and repeated resets. Validation: SSH unit and local acceptance suites; race tests for changed SSH and telemetry packages; targeted lint; Windows test compilation. Live driver-proxy validation remains required before release.
Collaborator
Integration test reportCommit: 29da087
Top 3 slowest tests (at least 2 minutes):
|
Validation of #6650 found three unbounded or unreported paths, each with a test that fails without the fix: - EOF on the source waited indefinitely for the peer to acknowledge the tail. A peer that reads without acknowledging (its own receiving loop wedged, or a half-open connection) kept the CLI alive after the ssh client that spawned it was gone; on the server side it kept sshd, a client slot and the compute alive with nobody attached. Bounded by proxyEOFDrainTimeout. - The capabilities probe had no timeout of its own, so a driver proxy that accepts the request and never answers blocked the connect path before the tunnel was even dialled. - Every session that ended before the SSH server's first byte was reported as a clean exit: proxy.start's deferred cancel always fires before g.Wait delivers the outcome, so the exit path returned nil. Deterministic, not a race - 40 of 40 runs. The exit path now waits for the outcome, bounded. Also report a tunnel that kept dropping as a drop rather than as a missing openssh-server, which is what the handshake timeout's message claims. Added tests: byte-exactness across a reset storm, no goroutine growth across repeated reattaches, client-slot retention for abandoned sessions, and both readings of the close frame's "finished" reason. Co-authored-by: Isaac <no-reply@databricks.com>
rugpanov
reviewed
Sep 14, 2026
rugpanov
left a comment
rugpanov
left a comment
Contributor
There was a problem hiding this comment.
Reviewed the resume/backpressure concurrency and protocol paths (resume.go, proxy.go, server.go, client.go, websockets.go). The design is careful and internally consistent — I could not substantiate any data race, deadlock, goroutine leak, unbounded-memory path, or byte/sequence off-by-one. Offset stability via awaitParked, the 1 MiB window as a true single-producer bound, conservative exact-v2 negotiation, and the EOF drain all check out.
Two issues worth addressing before merge, both on the drop-recovery path that resume exists to protect. Left inline.
rugpanov
approved these changes
Sep 15, 2026
rugpanov
left a comment
rugpanov
left a comment
Contributor
There was a problem hiding this comment.
Re-reviewed after 29da0875d. Both prior reservations are resolved:
- Drop during handover now falls through to
reattach()for a resumable session (verified no deadlock between the receiving loop and the handover goroutine, no goroutine leak, and thenewConnleak on the handover error paths is plugged). NewTestResumeAfterHandoverDropdrives during-upgrade / failed-dial / lost-close-reply with byte-exact streams both directions. - Transient HTTP 408 now retries within the reattach budget; only 409/410 are treated as permanent, matching the server's actual terminal responses. Covered by
TestCreateWebsocketConnectionRetriesRequestTimeout.
Verified locally: the two new tests pass, and the full proxy + client packages pass 3× under -race with no data races. The underlying resume/backpressure design was already sound (offset stability via awaitParked, the 1 MiB single-producer window bound, conservative exact-v2 negotiation, correct EOF drain).
One optional, non-blocking note left inline: a persistent non-terminal reattach rejection (e.g. a malformed 400) now consumes the full ~60s budget before the session ends rather than failing fast — bounded and acceptable. LGTM.
rclarey
reviewed
Sep 15, 2026
rclarey
left a comment
rclarey
left a comment
Contributor
There was a problem hiding this comment.
It would be good to also document this resume protocol somewhere so it's easier to understand at a glance and doesn't require reading the source code
| } | ||
|
|
||
| // capabilitiesProbeTimeout caps the pre-connect capabilities probe. A var so tests can shorten it. | ||
| var capabilitiesProbeTimeout = 10 * time.Second |
Contributor
There was a problem hiding this comment.
How did you decide on this value? Can we make it smaller? 10s is a while to wait for a failed probe
Contributor
Author
There was a problem hiding this comment.
what do you mean by a failed probe here?
| select { | ||
| case resumable := <-done: | ||
| require.False(t, resumable, "a server that never answered cannot be assumed to support resume") | ||
| case <-time.After(30 * time.Second): |
Contributor
There was a problem hiding this comment.
Why wait 30s if the timeout is 1s?
Comment on lines
+108
to
+110
| // Total payload bytes written to the destination. The peer replays from this offset, so it | ||
| // only advances after a successful write. | ||
| delivered atomic.Int64 |
Contributor
There was a problem hiding this comment.
I think this naming is really confusing, I needed to repeatedly jump back to re-read the doc comment and still the first 2 times I read through this PR I misunderstood what this meant.
Naming is hard, but I think handled might be a better name. At the least I would like the doc comment to be clearer since "destination" is vague and I misunderstood what that meant. Maybe something like
Suggested change
| // Total payload bytes written to the destination. The peer replays from this offset, so it | |
| // only advances after a successful write. | |
| delivered atomic.Int64 | |
| // Total payload bytes received from the peer and written to sshd's stdin (server) | |
| // or stdout (client). The peer replays from this offset, so it only advances after a | |
| // successful write. | |
| handled atomic.Int64 |
Collaborator
Integration test reportCommit: da11ec1
55 interesting tests: 45 FAIL, 6 flaky, 3 KNOWN, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
sunishsheth2009
pushed a commit
to sunishsheth2009/cli
that referenced
this pull request
Sep 15, 2026
## Changes - Document SSH resume protocol v2, including negotiation, acknowledgments, replay, retry limits, and clean shutdown. - Rename the internal receive offset from `delivered` to `handled` and clarify what it counts. - Tighten the stalled capabilities-probe test deadline while keeping the production timeout at 10 seconds. ## Why Follow up on Russell Clarey's post-merge review of databricks#6650. The protocol should be understandable without tracing the implementation, and the receive offset and timeout test should state their intent clearly. ## Tests - `./task fmt` - `./task checks` - `./task lint` - `./task test-exp-ssh` (426 package tests and 13 SSH acceptance tests) - `go test -race ./experimental/ssh/internal/client ./experimental/ssh/internal/proxy` - The unit portion of `./task test` passed 10,148 tests. The full acceptance suite is left to CI; a partial local run reproduced unrelated harness user-agent snapshot differences and the known completion SIGPIPE. _This PR was written by Codex._
janniklasrose
pushed a commit
that referenced
this pull request
Sep 15, 2026
## Changes Restore SSH session resume with bounded backpressure: a full replay window pauses the sender until acknowledgments free space. Separate acknowledgment handling and concurrent replay keep handover from deadlocking. Negotiate protocol version 2, preserve final bytes before EOF, and bound reconnect retries and abandoned-session retention. ## Why Transient websocket resets terminate customers' active terminal and IDE sessions. #6558 truncated healthy transfers when its 1 MiB replay window filled; #6609 would disable resume during large transfers and mishandle subsequent acknowledgments. This fix keeps resume enabled while applying backpressure. ## Tests - Reproduced the prior 1 MiB truncation and handover/replay deadlocks. Regression tests now verify byte-exact 8 MiB transfers each way through handover and repeated resets, plus negotiation, cancellation, handshake drops, and session cleanup. - SSH unit/local acceptance tests, full unit tests, targeted race tests, Windows compilation, full lint, and repository checks passed. The full acceptance run failed outside SSH (79 failures among 5,130 tests), including user-agent snapshot differences from `meta-harness/omnigent`, completion SIGPIPE, and bundle test timeouts; the full suite is not green. - Changed Go files are formatted and passed targeted lint. Python/YAML formatting passed; the full Go formatter was stopped without completion. A broader race run reproduced an existing VS Code settings backup test race on the original checkout. - Live-workspace validation remains required before release. Local acceptance covers existing SSH behavior; new proxy tests exercise resume directly because the local acceptance server does not negotiate resume. _This PR was written by Codex._ [DECO-28433]: https://databricks.atlassian.net/browse/DECO-28433?atlOrigin=eyJpIjoiNWRkNTljNzYxNjVmNDY3MDlhMDU5Y2ZhYzA5YTRkZjUiLCJwIjoiZ2l0aHViLWNvbS1KU1cifQ --------- Co-authored-by: Isaac <no-reply@databricks.com>
janniklasrose
pushed a commit
that referenced
this pull request
Sep 15, 2026
## Changes - Document SSH resume protocol v2, including negotiation, acknowledgments, replay, retry limits, and clean shutdown. - Rename the internal receive offset from `delivered` to `handled` and clarify what it counts. - Tighten the stalled capabilities-probe test deadline while keeping the production timeout at 10 seconds. ## Why Follow up on Russell Clarey's post-merge review of #6650. The protocol should be understandable without tracing the implementation, and the receive offset and timeout test should state their intent clearly. ## Tests - `./task fmt` - `./task checks` - `./task lint` - `./task test-exp-ssh` (426 package tests and 13 SSH acceptance tests) - `go test -race ./experimental/ssh/internal/client ./experimental/ssh/internal/proxy` - The unit portion of `./task test` passed 10,148 tests. The full acceptance suite is left to CI; a partial local run reproduced unrelated harness user-agent snapshot differences and the known completion SIGPIPE. _This PR was written by Codex._
deco-sdk-tagging Bot
added a commit
that referenced
this pull request
Sep 16, 2026
## Release v1.17.0 ### Notable Changes * Bump the direct deployment state version to 3. Clients older than v1.8.0 will reject bundles deployed with this release. ([#6713](#6713)) ### CLI * Add an `INVALID_REFRESH_TOKEN` error code to `databricks auth token --output json` failures. ([#6684](#6684)) * Add experimental `databricks auth docker configure` to configure Docker credential helper access for Databricks Artifact Registry. ([#6700](#6700)) * Add experimental `databricks auth docker token` to generate Docker credentials for Databricks Artifact Registry. ([#6699](#6699)) * `databricks environments setup-local` now reports the `E_PROVISION_CONFLICT` error code instead of the generic `E_PROVISION` when `uv sync` fails to resolve a dependency conflict. ([#6666](#6666)) * Preserve SSH sessions across temporary tunnel disconnects, with bounded replay and backpressure for large transfers. ([#6650](#6650)) * Allow OAuth U2M logins to override the CLI client ID with `--client-id`, profile `client_id`, or `DATABRICKS_CLIENT_ID`. ([#6594](#6594)) ### Bundles * direct: Store a dashboard's `serialized_dashboard` in state as a content hash instead of its full contents. ([#6105](#6105)) * direct: Fix pipelines recreation when the whole `ingestion_definition` block is added or removed. ([#6589](#6589)) * `bundle plan`, `deploy`, and `destroy` no longer report removing `permissions`, `grants`, or secret scope ACLs from a bundle as a deletion, since it leaves the resource untouched. ([#6647](#6647)) * `bundle plan` and `deploy` no longer list or count a resource that was already deleted remotely as a deletion, matching `bundle destroy`; applying still cleans up its stale state entry. ([#6675](#6675)) * Fix `bundle run` failing with `expected an int, found a string` when an unrelated resource references another resource that is not deployed. `bundle run` now resolves `${resources.*}` references only within the resource being run. ([#6690](#6690)) * Add grants support for the AI Gateway `model_service`, `mcp_service`, and `model_provider_service` resources (direct engine). ([#6635](#6635)) * Add bundle support for the AI Gateway `mcp_service` resource (direct engine). ([#6633](#6633)) * Add bundle support for the AI Gateway `model_provider_service` resource (direct engine). ([#6634](#6634)) * Add bundle support for the AI Gateway `model_service` resource (direct engine). ([#6525](#6525)) * Prevent resource drift on catalogs if `storage_root` contained a trailing slash in the URL. ([#6622](#6622)) * Fixed a "lineage mismatch in state files" error that could occur after destroying a bundle and redeploying it from another machine. `bundle destroy` now removes the local state file so no stale lineage is left behind, and prunes the state directories it leaves empty (such as `.internal/` and `sync-snapshots/`). ([#6210](#6210), [#6685](#6685)) * direct: `bundle plan` no longer reports a permanent update on a cluster that uses a cluster policy: when the cluster spec sets `policy_id`, a field present in the remote but absent from the bundle config is not treated as drift. ([#6531](#6531)) * `bundle deploy` on the direct engine now reports each resource as soon as it is deployed, instead of listing them all after the deployment finishes. A deploy that fails part way through now reports the resources it did apply. ([#6361](#6361)) * Direct-engine bundles no longer flag phantom drift on server-populated nested fields under reused config types (e.g. `external_locations` file-event-queue resource IDs, `database_instances` parent-instance refs, `apps` git credential ID). ([#6618](#6618)) * `databricks bundle generate app` now reproduces a git-backed app's `git_repository` and `git_source` configuration instead of emitting a workspace `source_code_path`, so generating from a Git-deployed app no longer silently converts it to workspace source. ([#6656](#6656)) * Improved configuration load time for bundles with many included files. ([#6195](#6195)) * `bundle destroy` no longer deletes triggered job runs, leaving them untouched on the backend. ([#6672](#6672)) * direct: resources.job\_runs: new lifecycle.triggers.on\_file\_change setting to restart the run when monitored files change. Can be set to a series of paths or globs. ([#6309](#6309)) * Bundle summary now shows a name for Postgres branches, endpoints, databases, and roles instead of a blank Name field. ([#6663](#6663)) * Added PyDABs (Python) support for cluster policies, dashboards, and Genie spaces. ([#6585](#6585)) * CLI commands no longer imply that a resource whose type has no workspace URL is merely not deployed yet. ([#6583](#6583)) * Capture the implicit dependency a vector search index has on a catalog or schema defined in the same bundle, so the catalog and schema are deployed first. ([#6655](#6655)) ### Dependency Updates * Bump dependencies with known vulnerabilities. ([#6695](#6695)) * Bump `github.com/databricks/databricks-sdk-go` from v0.177.0 to v0.178.0. ([#6673](#6673)) * Bump Terraform provider from v1.131.0 to v1.132.0. ([#6671](#6671))
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.
Changes
Restore SSH session resume with bounded backpressure: a full replay window pauses the sender until acknowledgments free space. Separate acknowledgment handling and concurrent replay keep handover from deadlocking. Negotiate protocol version 2, preserve final bytes before EOF, and bound reconnect retries and abandoned-session retention.
Why
Transient websocket resets terminate customers' active terminal and IDE sessions. #6558 truncated healthy transfers when its 1 MiB replay window filled; #6609 would disable resume during large transfers and mishandle subsequent acknowledgments. This fix keeps resume enabled while applying backpressure.
Tests
meta-harness/omnigent, completion SIGPIPE, and bundle test timeouts; the full suite is not green.This PR was written by Codex.