Skip to content

SSH: deliver the channel close instead of attempting it - #2521

Open
janisdombr wants to merge 1 commit into
warp-tech:mainfrom
janisdombr:fix/deliver-channel-close
Open

SSH: deliver the channel close instead of attempting it#2521
janisdombr wants to merge 1 commit into
warp-tech:mainfrom
janisdombr:fix/deliver-channel-close

Conversation

@janisdombr

@janisdombr janisdombr commented Aug 30, 2026

Copy link
Copy Markdown
Contributor

Description

This is what fails Tests on main about a quarter of the time.
test_recordings_index.py::Test::test_terminal_recording_index is the visible
symptom; the cause is below, and it is not in the test. Every pull request pays
for it, including ones that have nothing to do with SSH.

A session that has streamed a large amount of output can end without the client ever being told its channel is closed. ssh then waits on a session that has already finished, until something kills it.

events_tx holds 1024 events and every chunk of target output takes one, so the queue is full at exactly the moment a high-output channel ends. close() used try_send, whose failure was discarded, and the close was lost. The call at the end of run() is in an async context and can wait for room; Drop keeps try_send, having nothing to await with.

How it shows up. That test streams ~1.5 MB through a recorded PTY and times out waiting for the client. In every failure the whole transfer had arrived — 1,489,491 bytes, last line 200000 — so the data is not the problem; the client is simply never released.

The gateway log names the difference. A successful run ends:

Event event=Eof(<channel>)
Event event=ExitStatus(<channel>, 0)
Event event=Close(<channel>)
Closed session

A failing one ends ExitStatus then Eof, and then nothing at all until the client is killed and teardown follows.

Measured. Each run below repeats the transfer twenty times and reports, for any that hangs, how many bytes had arrived.

commit result
without this change c5f4106f 4 of 20 hung
with it facada2a 0 of 20
with it, again facada2a 0 of 20

At the observed rate, forty consecutive clean transfers is about one chance in eight thousand.

The reproduction and the loop that measures it are on tooling/recording-stall in my fork, based on main with nothing else merged, and are deliberately not part of this change. GitHub truncates the larger logs, so the loop's exit status is what carries the result — it fails on the first hang.

AI Usage

Choose the level of AI involvement for this PR.

  • Fully vibe coded
  • AI-designed, AI-coded, manually checked
  • Human-designed, AI-coded
  • Human-designed, human-coded (includes AI autocompletions and boilerplate gen)

This is not to block AI contributions but rather to speed up PR review (saves time on trying to deduce the logic behind AI hallucinations).

A session that has streamed a large amount of output can end without the
client ever being told its channel is closed, and `ssh` then waits on a
session that has already finished until something kills it.

`events_tx` holds 1024 events and every chunk of target output takes one, so
the queue is full at exactly the moment a high-output channel ends. `close()`
used `try_send`, whose failure was discarded, and the close was lost.

The call at the end of `run()` is in an async context and can wait for room.
`Drop` keeps `try_send`, having nothing to await with.
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.

2 participants