SessionChannel drops buffered output when CHANNEL_CLOSE arrives before the consumer receives it (delivery job cancelled mid-send)

[GlassOnTin]

SessionChannel silently drops output that a consumer has not received by the time CHANNEL_CLOSE arrives. The consumer observes a cleanly-closed empty channel — no exception, no truncation signal — so the data loss is indistinguishable from "the command produced no output".

Reproduced deterministically on 0.4.0 and 0.4.1 (the closeResources() path is unchanged between them).

Mechanism

SessionChannel buffers arriving data in an UNLIMITED ingress channel and forwards it to the RENDEZVOUS channel the caller reads, via a delivery-job pump:

private val stderrIngress = Channel<ByteArray>(Channel.UNLIMITED)
private val _stderr = Channel<ByteArray>(Channel.RENDEZVOUS)

private val stderrDeliveryJob = connectionScope.launch {
    deliverData(stderrIngress, _stderr) { it.size }
}

Because _stderr is RENDEZVOUS, the pump suspends inside output.send(value) until the caller parks in receive(). That is deliberate for flow control — the window credit is only released once the consumer has actually taken the data.

But closeResources() then cancels that pump outright:

stderrIngress.close()
...
stderrDeliveryJob.cancel()   // kills the suspended send()
...
_stderr.close()              // consumer now sees a normal, empty, closed channel

So any data the pump is holding mid-handoff when CHANNEL_CLOSE is processed is discarded. close() (caller-initiated) does the same thing, which is more defensible; the problem case is onClose() → closeResources(), i.e. a server-initiated close, which is the normal end of every short-lived command.

The window is "between the data arriving and the consumer parking in receive()". For a command that writes a line and exits immediately, that window contains a full network round trip on the server side, so a consumer that starts reading after issuing requestExec can easily lose the race.

Reproduction

Against a MINA sshd whose command writes oops\n to stderr and exits 3:

val session = client.openSession()!!
check(session.requestExec("fail"))
delay(750)                               // let the server's CHANNEL_CLOSE land first
val buf = ByteArrayOutputStream()
runCatching { for (chunk in session.stderr) buf.write(chunk) }
println(buf.toByteArray().decodeToString())   // prints "" — the "oops" is gone

8/8 runs lose the output. Parking the drain before requestExec instead (async(start = CoroutineStart.UNDISPATCHED) { drain(session.stderr) }) recovers it 8/8. The delay only makes the race deterministic — it is not required to hit it.

This surfaced as an intermittent CI failure for us: a contract test asserting a command's stderr failed roughly 1 run in 100 with expected:<[oops\n]> but was:<[]>, only ever on loaded runners. Reproducing it needed nothing more than inserting the delay above into the old ordering. stdout is affected identically — we see expected:<[hello\n]> but was:<[]> from the same mutation.

Impact

Any consumer that is not already parked in receive() when the server closes can lose the tail of a stream. Short-lived exec is the easy case to hit, but an interactive shell reading through a blocking InputStream bridge has a wider window between reads, so trailing output at session end looks vulnerable too (not separately verified).

The silence is the sharp edge: there is no way for a caller to tell dropped output from no output.

Possible directions

Not proposing a patch, since the flow-control design is yours — but the options I can see:

1. On close, close the ingress channels and let each pump exit its for loop naturally (deliverData already closes output in its finally), instead of cancelling the job. Risk: the pump stays suspended in send() forever if the consumer abandons the channel.
2. Same, but bound it — cancel after a grace period, or cancel only once the consumer side is known to be gone.
3. Signal the truncation rather than hiding it, e.g. output.close(cause) when the pump is cancelled with data still in flight, so a caller at least sees that output was lost.

Happy to test a patch against our contract suite if that helps.

Workaround

We park both drains UNDISPATCHED before issuing requestExec, so the receivers are already waiting before the command can produce anything. That removes the dominant window but not the residual one (between successive chunk receives on a multi-chunk stream), so it is a mitigation rather than a fix.

[GlassOnTin]

Follow-up with measurements from the shell path, which turns out to be a worse case than the exec one I opened this with — and one useful nuance in the library's favour.

Steady-state reading is safe

The rendezvous channel does its job: a server cannot outrun a consumer that is actually consuming. A reader taking 30ms between 512-byte reads received all 14800 bytes of a payload intact. Backpressure holds, so this is not a general "sshlib loses data" problem.

A consumer that is not yet parked loses everything

The damage is confined to output produced while no one is parked on the receive — but there it is total, not a truncated tail. Against MINA sshd, a shell that writes 740 bytes and exits immediately, varying only how long the consumer waits before its first read:

  0ms -> 740/740     20ms -> 740/740
  5ms -> 740/740     50ms ->   0/740
150ms ->   0/740    750ms ->   0/740

Nothing partial in between — either the consumer is parked when CHANNEL_CLOSE is processed and gets everything, or it isn't and gets nothing.

Why the shell path is more exposed than exec

For exec I could fix it caller-side by parking both drains before requestExec. A shell can't be closed the same way as easily: the natural shape is request the shell, wrap stdout in a blocking InputStream, and hand that to a reader thread that starts later. Every millisecond of that is unguarded, and a shell that fails immediately — /bin/false, a rejected forced command, a login script that exits — writes its explanation and closes inside the window. The user sees an empty terminal rather than the reason.

That is also why it stays plausible for a consumer to hit this despite the backpressure result: the window is at channel start, before any steady state exists.

Where I think this lands

The caller-side mitigations available to me all amount to "make sure something is parked on the channel at all times", which is exactly what the rendezvous plus pump was meant to abstract away. Cancelling stdoutDeliveryJob while it still holds a value is the part that turns a slow consumer into a silent data loss, and only the library can avoid that — by letting the pump drain what it has before the output channel closes, or by closing the output channel with a cause so the consumer can at least tell truncation from EOF.

Happy to test a patch against these cases; the repro is a few lines against MINA and I can push it somewhere if it would be useful as a regression test.