Session channels are not deregistered on close: a second session channel fails (null on MINA, "Remote channel N is already registered" + connection teardown on OpenSSH)

[GlassOnTin]

Opening a second session channel on the same connection fails once the first has been closed. Found while promoting sshlib 0.4.0 from SFTP-only to a whole-connection engine in Haven (follow-on from #231 — thanks again for that fix, 0.4.0 is solid on rekey).

It surfaces differently depending on the server, which is why I'm reporting both:

Against Apache MINA sshd 2.19.0 — the second openSession() simply returns null:

val client = SshClient(SshClientConfig {
    host = "127.0.0.1"; port = server.port
    hostKeyVerifier = trustAll
})
client.connect()                     // Success
client.authenticatePassword(...)     // Success

repeat(12) { attempt ->
    val session = client.openSession()
        ?: error("openSession returned null on attempt $attempt")   // fires at attempt 1
    session.requestExec("true")
    session.close()
}

Against a real OpenSSH server (OpenSSH 9.x on Ubuntu, from an Android client) it throws instead — and takes the whole connection down with it:

INFO  - Opening session channel (local=1)
INFO  - Channel open confirmed
INFO  - Channel opened: local=1, remote=1, remoteWindow=0
INFO  - Opening session channel (local=2)
INFO  - Channel open confirmed
java.lang.IllegalStateException: Remote channel 1 is already registered
ERROR - Failed to open session channel

…after which the transport drops and the client reconnects. So it isn't only the new channel that fails — the existing session dies too, which is what makes it hard to work around.

Reading of it (happy to be corrected): the second channel is confirmed by the server with the same remote channel number (remote=1) as the first, because the server freed that number when we closed channel 1. The client still has remote=1 in its registry, so it rejects its own channel. That would mean a channel is registered under the remote number but not deregistered on close. The MINA null and the OpenSSH exception look like the same root cause reached by two paths (MINA appears to hand out a fresh remote number more slowly, so it fails a little differently).

Impact: any long-lived connection that opens more than one session channel — which for us is the normal case: an interactive shell, then exec for a command, then another exec later. A single exec per connection is fine; the second is where it bites.

Repro is a plain JUnit test against MINA if that's useful — happy to push it somewhere or turn it into a PR against the test suite if you'd like it as a regression test.

For now Haven keeps the sshlib whole-connection engine behind an explicit opt-in flag, with this as a known limitation.

[kruton]

Last night I was converting the SFTP state machine to TLA+ supporting model which meant I added intermediate states like PendingOpen. I did see an issue like this, but I'll check tonight to see if it was fixed with the model checking changes.

[GlassOnTin]

Thanks — one thing worth separating out, since the SFTP PendingOpen work may not touch it: the leak I filed this for is in plain SessionChannel, independent of SFTP. unregisterForwardingChannel removes forwarding channels and the agent-channel open-failure path cleans up too, but SessionChannel.closeResources() never unregisters, so a session channel's entry in localRecipientByRemoteRecipient lives for the whole connection. When the server then reuses that remote number for the next channel — legal once both sides have sent CHANNEL_CLOSE (RFC 4254 §5.3) — bindRemoteRecipient rejects it, and on our end the whole connection dies.

The concrete repro is a second exec/shell session on one connection (e.g. tmux -A reattach, or SFTP-on-a-shared-connection after a shell session), not an SFTP-internal state transition — so I think it survives the state-machine refactor. #239 is the one-liner for it (connection.unregisterSessionChannel(localChannelNumber) at the end of closeResources()), CLEAN/mergeable, with the full trace in the PR body. If your model-checking pass already covers this exact path, happy to close it as redundant — just flagging so it doesn't fall between the two.

[kruton]

Could you try with v0.4.1 ?

[GlassOnTin]

Tried 0.4.1 — the deregistration part is fixed, thank you. But a second session still fails, for a new reason introduced by the same change, so I don't think this can close yet.

What 0.4.1 fixed

notifyChannelClosed() now unregisters the entry, so localRecipientByRemoteRecipient no longer leaks and bindRemoteRecipient no longer rejects a legitimately reused remote number. That was the root cause I filed this for, and #239 is redundant against it — I'll close that.

What still blocks a second session

The same method also does this:

if (autoDisconnectOnLastChannelClose && allClosed && (hasSession || wasSession)) {
    logger.info("All channels closed, sending disconnect")
    sendDisconnect()
}

Closing the only session empties the registry, so allClosed is vacuously true and wasSession is true — and the client disconnects the transport. The next openSession() then has nothing to open on.

Against MINA sshd 2.19.0 on 0.4.1, with a 400ms settle after each close:

PROBE connect: Success
PROBE auth:    Success
PROBE attempt 0 -> session(local=0)
PROBE   after close, openWorks=false
PROBE attempt 1 -> NULL

and the library's own log confirms the mechanism rather than me inferring it:

All channels closed, sending disconnect

So the failure mode has moved from "second channel is rejected" to "connection is already gone", but the observable for a caller is the same: one session channel per connection, ever.

Why this matters for a whole-connection client

Auto-disconnecting when the last channel closes is right for a one-shot client — it mirrors what ssh host command does. It's wrong for a client that owns the connection's lifetime and multiplexes over it: an interactive shell, then an exec for a command, then SFTP on the same transport, then another exec later. Each of those closes a channel, and the first one to leave the registry empty takes the connection with it.

autoDisconnectOnLastChannelClose looks like exactly the right escape hatch, but it's internal, so a consumer can't reach it.

Proposed fix

Expose it through SshClientConfig, defaulting to true so current behaviour is unchanged, and let a client that manages its own lifetime opt out. I'm happy to send that as a PR against main — small and additive — unless you'd rather shape the API differently, in which case tell me and I'll follow your preference.

The alternative reading is that the disconnect should only fire when the connection was opened for a single command, which is really a caller intent the library can't infer — hence a flag.

[GlassOnTin]

Correction to my previous comment, and good news: 0.4.1 fixes this completely. I was wrong that the escape hatch is unreachable — I read the internal var on SshConnection and didn't check SshClientConfig, where it is already public in the 0.4.1 release. My probe simply never set it. Apologies for the noise.

Verified against MINA sshd 2.19.0 on the released 0.4.1, twelve open/close cycles on one connection:

PROBE[autoDisconnectOnLastChannelClose=true]  FAILED at attempt 1 (openSession null)
PROBE[autoDisconnectOnLastChannelClose=false] opened 12/12 sessions OK

So both halves landed in 0.4.1: notifyChannelClosed() deregisters the entry, so a reused remote number is accepted, and SshClientConfig.autoDisconnectOnLastChannelClose = false lets a client that owns the connection's lifetime keep it up across channels. With the flag off there is no residual limit that I can find — the twelfth session opens exactly like the first.

The default of true is right, for what it's worth: a one-shot client should behave like ssh host command. It just needs to be off for a multiplexing one, which is what the flag now allows.

From my side this can close. #239 is redundant against your implementation and I've closed it. Thanks for turning this round quickly — Haven is moving its whole-connection engine to 0.4.1 with the flag off, and this was the last thing keeping that engine opt-in for us.