Repository navigation
fix(minimald): close the client channel when shedding a stalled binding - #1747
Conversation
A binding whose stdout forward could not send within the 2 s host probe deadline was shed with `JoinHandle::abort`. That skipped the binding's exit path, and russh sends no close for dropped channel halves, so the client stayed connected with no output, swallowed keystrokes, and no way to detach. Any terminal draining below roughly 800 KB/s while a session printed a few megabytes hit it. Shed a binding by cancelling a token the binding races against every await that can park on a stalled client. The binding then leaves through its own exit path: a notice (bounded), EOF, exit status 255 so the client restores the terminal, and a close. The session is untouched. Replace the 2 s bounded send with backpressure that does not park the host loop. While the binding's mailbox is full the host stops reading the pty, so the shell waits on the terminal, and the loop keeps serving its mailbox, kills, attaches and exits. A binding that takes no output for OUTPUT_STALL_TIMEOUT (30 s) is shed. The supersede, shutdown and detach handoffs and the leader bell are bounded as well, so a stalled binding cannot park the host through them either.
|
Warning Review limit reached
This review includes 2 billable files and costs up to $0.50.
Reviews can continue after your included limit without a manual trigger. An admin must approve usage-based billing. Or wait 48 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (2)
Comment |
|
Reviewed: 1 finding, none blocking at Address the comments on the diff. Clobber: none: |
| Err(e) => { | ||
| tracing::warn!("failed sending detach signal to remote: {e}"); | ||
| tracing::warn!("shedding binding that could not take the detach: {e}"); | ||
| self.shed_binding(); | ||
| 8000 | } |
There was a problem hiding this comment.
LOW · api-design · confidence 0.60
When a user deliberately presses the detach chord while output is stalled, the binding's mailbox is full, send_timeout expires after HOST_PROBE_TIMEOUT, and shed_binding() is called. The binding then exits through the shed path, which prints SHED_NOTICE ("the terminal stopped keeping up with session output; re-attach...") and reports exit status 255, triggering client_must_unwind's blind terminal unwind. But the user is present and typed the chord — their input path (the shared stdin channel) is alive, so this is not a dark transport — and the notice is factually wrong. The deliberate detach also loses the daemon's own unwind codes (captured into uc but not sent on the shed path), so the terminal gets the blunt 255 blind unwind instead of the precise codes a clean detach would send. This is the one path where a shed is triggered by an explicitly-typed command rather than a genuinely stalled client, and it is mislabeled as a transport failure. Consider distinguishing a detach-timeout shed from an output-stall shed (e.g. exit status 0 + the normal detach notice, or a distinct message) so a user who actively detaches is not told their terminal stopped keeping up. Not applicable to the supercede/shutdown retire_binding timeouts, which are genuinely dark-client cases.
What the reviewer checked
session_host.rs:3371-3374 Err(e) => { tracing::warn!("shedding binding that could not take the detach: {e}"); self.shed_binding(); } -- shed path prints SHED_NOTICE and exit 255 (SHED_EXIT_STATUS) at 668-682
Hand this to an agent
Act on a code-review finding.
File: crates/minimald/src/session_host.rs
Lines: 3371-3374
Category: api-design
Severity: low (reviewer confidence 0.60)
Finding:
When a user deliberately presses the detach chord while output is stalled, the binding's mailbox is full, `send_timeout` expires after `HOST_PROBE_TIMEOUT`, and `shed_binding()` is called. The binding then exits through the shed path, which prints `SHED_NOTICE` ("the terminal stopped keeping up with session output; re-attach...") and reports exit status 255, triggering `client_must_unwind`'s blind terminal unwind. But the user is present and typed the chord — their input path (the shared stdin channel) is alive, so this is not a dark transport — and the notice is factually wrong. The deliberate detach also loses the daemon's own unwind codes (captured into `uc` but not sent on the shed path), so the terminal gets the blunt 255 blind unwind instead of the precise codes a clean detach would send. This is the one path where a shed is triggered by an explicitly-typed command rather than a genuinely stalled client, and it is mislabeled as a transport failure. Consider distinguishing a detach-timeout shed from an output-stall shed (e.g. exit status 0 + the normal detach notice, or a distinct message) so a user who actively detaches is not told their terminal stopped keeping up. Not applicable to the supercede/shutdown `retire_binding` timeouts, which are genuinely dark-client cases.
First verify this against the code yourself. It is model output and
may be wrong. If it is wrong, or the tradeoff was deliberate, say so
and change nothing.
👍 useful · 👎 noise — react to tell the reviewer. Replying works too, and says more.
There was a problem hiding this comment.
Deliberate, so no change. The detach send only times out when the binding has freed no mailbox slot for HOST_PROBE_TIMEOUT (2 s), which means the client is not draining output at all. A client that reads even slowly frees a slot in milliseconds and gets the normal detach. In the timeout case the notice is accurate: output really has stopped reaching the terminal. The narrow unwind codes cannot help either. They would travel the same stalled output path, behind the queued output, so they may never land, and the stream may have been cut mid-escape-sequence. Exit status 255 has min restore the terminal itself, which is the safe choice when the daemon's own codes cannot be trusted to arrive.
Routing-Key: inbox-route/I_kwDOSUhdos8AAAABTSyQxg
Problem
A session attach sheds its binding when the stdout forward cannot send within
HOST_PROBE_TIMEOUT(2 s). The shed calledJoinHandle::abort()on the binding task. That skipped the binding's exit path, and russh 0.63 sends no close for dropped channel halves. The client stayed connected with no output behind it: keystrokes were swallowed, and even the detach chord could not get out. Only ssh's~.escape or killing the terminal did.Any terminal draining slower than about 700 to 800 KB/s for 2 s hit this while the session printed a few megabytes, on both the macOS VM and Linux native. 0.5.4 used plain backpressure and did not have the problem.
The 2 s bound was added by #1408 to stop a dark client from parking the host loop in an unbounded send. A parked loop never pumps the pty, answers its mailbox, or sees the shell exit. The fix below keeps that guarantee.
Fix
A shed closes the channel. Each binding now carries a
CancellationToken. The binding races the token against every await that can park on a stalled client: the channel write, the idle select, and its sends into the host's stdin queue.Host::shed_bindingcancels the token instead of aborting the task, so the binding leaves through its own exit path:Disconnecting - the terminal stopped keeping up with session output; re-attach with `min session attach`), bounded so a dark transport cannot hold it;255 is the status on which
minrestores the terminal itself (client_must_unwind). That is needed here, because the output stream was cut mid-flight and no unwind codes followed it. The session is untouched, and a re-attach works as before.on_detachhooks do not fire for a shed.A slow reader gets backpressure. The host no longer awaits the forward at all. While the binding's mailbox is full, the host stops reading the pty, so the shell waits on the terminal. The loop keeps serving its mailbox: probes, kills and re-attaches. A
reserve()arm wakes the loop when a slot frees. Only a binding that takes no output forOUTPUT_STALL_TIMEOUT(30 s, separate from the 2 s probe deadline) is shed.No other path parks the loop behind a stalled binding. Because a stall can now last up to 30 s, the host's other awaited sends to the binding are bounded too:
retire_binding. It still waits for the old binding to finish its farewell, but sheds it afterHOST_PROBE_TIMEOUT.send_timeoutand sheds the binding on failure. The detach chord is also how a user leaves a stalled attach.try_send.Tests
In
crates/minimald/src/session_host/tests.rs, driven through a real in-process russh server and client, with a small client window so a client that stops reading stalls the server within a few KiB:shedding_a_stalled_binding_closes_the_client_channel: the client stops reading until the stall bound passes. When it reads again, it must see EOF, exit status 255 and a close, and the host must still be serving. Under the old abort, no close ever arrives.a_slow_reader_is_held_back_not_shed: the client stops reading for twice the probe deadline with the production stall bound. The host must answer every probe meanwhile, and once the client reads again, over 1 MiB more output must arrive without EOF or close. Under the old 2 s shed, output stops.a_stalled_binding_does_not_wedge_the_host_loopis adapted to the new flow and still proves the loop keeps serving, sheds, and drops the shed generation's input.