Repository navigation
Revert PR #6558: fix SSH transfer regression in v1.16.0 - #6608
Merged
Merged
Conversation
… of ending the session" This reverts commit 3cee3a1. Fixes v1.16.0 regression where continuous SSH transfers larger than ~1 MiB kill the session. 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 VS Code server transfers several MB during startup), and any SSH tunnel transfer larger than 1 MiB in either direction. The reverted code did bundle two independent fixes (pre-swap handover resilience and SSH telemetry categories) alongside the window limit. A targeted fix-forward (degrade full window to non-resumable instead of killing session) is being prepared in parallel as the preferred path. Co-authored-by: Isaac <no-reply@databricks.com>
anton-107
marked this pull request as ready for review
September 10, 2026 15:15
janniklasrose
approved these changes
Sep 10, 2026
anton-107
marked this pull request as draft
September 10, 2026 15:20
anton-107
marked this pull request as ready for review
September 10, 2026 15:25
Collaborator
Integration test reportCommit: 673b11c
Top 6 slowest tests (at least 2 minutes):
|
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>
janniklasrose
approved these changes
Sep 10, 2026
Already part of patch release notes
janniklasrose
enabled auto-merge
September 10, 2026 16:35
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>
Collaborator
Integration test reportCommit: aaab7ff
494 interesting tests: 488 FAIL, 5 KNOWN, 1 SKIP
Top 50 slowest tests (at least 2 minutes):
|
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>
pranshupand-db
pushed a commit
to pranshupand-db/cli
that referenced
this pull request
Sep 11, 2026
…atabricks#6626) ## Why databricks#6558 capped the tunnel's unacknowledged replay window at 1 MiB and read a full window as a peer that had stopped acknowledging, so ordinary in-flight data ended the session: ``` Proxy server error: proxy websocket dropped failed to send message: resume send buffer is full: the peer stopped acknowledging: 1045563 unacknowledged bytes, limit 1048576 ``` It shipped in v1.16.0, broke `ssh connect --ide` outright (the remote IDE server pushes several megabytes as it starts), and was reverted in databricks#6608. **No test noticed, in pre- or post-merge CI** — this PR is that missing coverage, so a re-introduction is caught before users are. ## What it does `acceptance/ssh/bulk-transfer` moves 8 MiB in each direction over a real `ssh connect` session and asserts the byte counts. Measured against a real workspace, on the tree as it was before the revert and after: | | result | |---|---| | local (test server's sshd over `ws://`) | pass, 3.4s | | cloud, serverless CPU, with databricks#6558 present | **fail** — `received 1032192 of 4194304`, `delivered 0 of 4194304` | | cloud, serverless CPU, with databricks#6558 out of the path | pass, 23s cold start | | cloud, on this PR's own CI run | pass, 72s (cold serverless start plus the one-time binary upload) | Some details worth flagging for review: - **8 MiB, and it must stay under 10,000,000.** The root `test.toml` rewrites any run of 8+ digits to `[NUMID]`, which would silently erase the counts and leave the test asserting nothing. 8 MiB is also the size that matters — roughly what the IDE server pushes at startup. - **The byte counts are the assertion, not the exit code.** The regression could end the session with truncated output *and* exit code 0, so an exit-code check would have passed on it. - **First ssh test to run on cloud since databricks#4838** disabled the old cloud-only ones as too flaky. Serverless CPU has no cluster to provision and no accelerator to wait for, which is what makes this one dependable. - **Deliberately not `CloudSlow`.** PR cloud runs pass `-short` (`task cloud-select`), so `CloudSlow` would keep this off every PR — including one that changes the test itself — and leave it only to the full suite on main. At ~70s in CI - a cold serverless start plus the one-time upload of the two release archives, not the transfer - it does not belong with the multi-minute infra tests that carry that flag. - **Locally the test server never negotiates the resume protocol**, so the local run covers the plumbing over a real ssh session (real sshd, real handshake, 16 MiB through the websocket) and the cloud run is what would catch the window coming back. ## Tests - `go test ./acceptance -run TestAccept/ssh` — pass - `go test ./experimental/ssh/...` — pass - cloud runs on serverless CPU in both configurations (table above) - this PR's own `Integration Tests` check ran it and nothing else: the cloud leg's selection resolves to `^ssh$/^bulk-transfer$`, and its log shows `PASS acceptance.TestAccept/ssh/bulk-transfer (72.08s)` against a real serverless tunnel This pull request and its description were written by Isaac. Co-authored-by: Isaac <no-reply@databricks.com>
janniklasrose
added a commit
that referenced
this pull request
Sep 15, 2026
## Summary Reverts commit 3cee3a1 to fix CLI v1.16.0 regression: any continuous transfer larger than ~1 MiB over the SSH tunnel kills the session. **Impact:** - `databricks ssh connect --ide` hits it 100% of the time (VS Code server pushes several MB during startup, session drops after 15-30s with reconnect loops) - Any SSH tunnel transfer larger than 1 MiB in either direction fails - v1.15.0 moved 7 MB fine; regression is specific to v1.16.0 **Root Cause:** The replay buffer limit (`proxyResumeBufferLimit = 1<<20` in `experimental/ssh/internal/proxy/proxy.go`) misreads ordinary in-flight data as a dead peer. When `sendBuffer.append` exhausts the unacknowledged window, it returns `errSendWindowExhausted` and kills the sending loop, even though the peer is actively sending acks. Instrumentation showed the client sends acks and every write succeeds, but the peer never applies them during continuous bursts — the window is reached by ordinary in-flight data and misinterpreted as connection death. --------- Co-authored-by: Isaac <no-reply@databricks.com> Co-authored-by: Jan N Rose <janniklas.rose@gmail.com>
janniklasrose
pushed a commit
that referenced
this pull request
Sep 15, 2026
…6626) ## Why #6558 capped the tunnel's unacknowledged replay window at 1 MiB and read a full window as a peer that had stopped acknowledging, so ordinary in-flight data ended the session: ``` Proxy server error: proxy websocket dropped failed to send message: resume send buffer is full: the peer stopped acknowledging: 1045563 unacknowledged bytes, limit 1048576 ``` It shipped in v1.16.0, broke `ssh connect --ide` outright (the remote IDE server pushes several megabytes as it starts), and was reverted in #6608. **No test noticed, in pre- or post-merge CI** — this PR is that missing coverage, so a re-introduction is caught before users are. ## What it does `acceptance/ssh/bulk-transfer` moves 8 MiB in each direction over a real `ssh connect` session and asserts the byte counts. Measured against a real workspace, on the tree as it was before the revert and after: | | result | |---|---| | local (test server's sshd over `ws://`) | pass, 3.4s | | cloud, serverless CPU, with #6558 present | **fail** — `received 1032192 of 4194304`, `delivered 0 of 4194304` | | cloud, serverless CPU, with #6558 out of the path | pass, 23s cold start | | cloud, on this PR's own CI run | pass, 72s (cold serverless start plus the one-time binary upload) | Some details worth flagging for review: - **8 MiB, and it must stay under 10,000,000.** The root `test.toml` rewrites any run of 8+ digits to `[NUMID]`, which would silently erase the counts and leave the test asserting nothing. 8 MiB is also the size that matters — roughly what the IDE server pushes at startup. - **The byte counts are the assertion, not the exit code.** The regression could end the session with truncated output *and* exit code 0, so an exit-code check would have passed on it. - **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, which is what makes this one dependable. - **Deliberately not `CloudSlow`.** PR cloud runs pass `-short` (`task cloud-select`), so `CloudSlow` would keep this off every PR — including one that changes the test itself — and leave it only to the full suite on main. At ~70s in CI - a cold serverless start plus the one-time upload of the two release archives, not the transfer - it does not belong with the multi-minute infra tests that carry that flag. - **Locally the test server never negotiates the resume protocol**, so the local run covers the plumbing over a real ssh session (real sshd, real handshake, 16 MiB through the websocket) and the cloud run is what would catch the window coming back. ## Tests - `go test ./acceptance -run TestAccept/ssh` — pass - `go test ./experimental/ssh/...` — pass - cloud runs on serverless CPU in both configurations (table above) - this PR's own `Integration Tests` check ran it and nothing else: the cloud leg's selection resolves to `^ssh$/^bulk-transfer$`, and its log shows `PASS acceptance.TestAccept/ssh/bulk-transfer (72.08s)` against a real serverless tunnel This pull request and its description were written by Isaac. Co-authored-by: Isaac <no-reply@databricks.com>
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
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.
Summary
Reverts commit 3cee3a1 to fix CLI v1.16.0 regression: any continuous transfer larger than ~1 MiB over the SSH tunnel kills the session.
Impact:
databricks ssh connect --idehits it 100% of the time (VS Code server pushes several MB during startup, session drops after 15-30s with reconnect loops)Root Cause:
The replay buffer limit (
proxyResumeBufferLimit = 1<<20inexperimental/ssh/internal/proxy/proxy.go) misreads ordinary in-flight data as a dead peer. WhensendBuffer.appendexhausts the unacknowledged window, it returnserrSendWindowExhaustedand kills the sending loop, even though the peer is actively sending acks. Instrumentation showed the client sends acks and every write succeeds, but the peer never applies them during continuous bursts — the window is reached by ordinary in-flight data and misinterpreted as connection death.