Release SshCommand's session subscriptions and channel on completion (#1204)

[harsh-official]

Relates to #1204 (the "keep the connection alive and run RunCommand/CreateCommand without disposing" scenario from the later comments; it does not touch the ShellStream scenario of the original report).

Root cause

SshCommand subscribes to Session.Disconnected and Session.ErrorOccured in its constructor, and since #1596 it also creates its ChannelSession there. Both are released only in SshCommand.Dispose(). So a command that has run to completion but was never disposed, e.g.

while (true)
{
    var cmd = client.RunCommand("dir"); // not disposed
}

stays reachable from the session for the session's whole lifetime. Its channel stays reachable too, and the channel is itself subscribed to ~12 session events and holds 4 ManualResetEvents. With a long-lived SshClient, memory and handle counts grow without bound, and every incoming channel message is dispatched to a growing list of dead channels.

Why not a finalizer

As discussed in the issue, a finalizer isn't the answer. SshCommand owns no unmanaged resources. And the thing keeping the command alive is the session's reference to it through the event handlers, so the command never becomes unreachable and a finalizer would never run. Even if it did run, it couldn't safely touch the Session or the channel, since finalization order is non-deterministic. The fix has to be that a completed command no longer needs to be referenced by the session.

The change

• The constructor no longer creates the channel or subscribes to the session. ExecuteAsync does both.
• The session subscriptions are removed as soon as the execution completes, on every completion path: channel closed, cancellation, timeout, session error or disconnect, Dispose.
• The channel is disposed (which unsubscribes it from the session and frees its wait handles) as soon as it has been closed at both ends and no thread is using it in a request. At that point Channel.Dispose() neither sends a message nor blocks.
• If Open() or SendExecRequest() throws, the channel and subscriptions are released and the task is faulted. Previously the instance was left "in progress", so it could never be executed again.
• Cleanup (session unsubscription, channel release, output stream disposal) happens before the task completes. A caller that re-executes the same instance as soon as the previous execution completes therefore can't have its new subscriptions or output streams torn down by the previous execution's cleanup.

Disposing a completed command remains correct and is still recommended. It just stops being the only way to release the session's references.

The #1596 race is preserved

#1596 moved channel creation to the constructor and stopped disposing the channel on completion because SSH_MSG_CHANNEL_CLOSE can be processed on the message loop thread before the executing thread has finished waiting in SendExecRequest(). Disposing the channel at that point disposes _channelRequestResponse underneath the waiting thread.

This PR keeps that fixed with a per-channel use count, guarded by a lock:

• ExecuteAsync sets the use count to 1 before Open()/SendExecRequest() and decrements it afterwards. CancelAsync does the same around SendSignalRequest().
• Channel_Closed only marks the channel as closed. Release is attempted on completion and on every decrement, and happens only when the channel is closed and the use count is zero. If the close is processed while SendExecRequest() is still waiting, the dispose is deferred to the executing thread once SendExecRequest() returns.
• A channel that is still open when the execution completes (cancellation without a server response, session error/disconnect) is also released on completion. But its Dispose() sends SSH_MSG_CHANNEL_CLOSE and waits up to ChannelCloseTimeout for the server, so it's disposed on a thread-pool thread (ThreadAbstraction.ExecuteThread) rather than on the thread completing the execution, which may be the message loop. Our handlers are detached from the channel first, so the command is not kept alive while that happens.

_channelLock is never held while calling into the channel, so it can't deadlock against Channel._messagingLock. Disposing the channel from within its own Closed event (message loop thread) is safe: Close() re-enters _messagingLock and is a no-op, since IsOpen is already false.

Tests

New SshCommandTest_NotDisposed:

• ConstructorShouldNotCreateChannelNorSubscribeToSession
• CompletedCommandShouldReleaseChannelWithoutDispose
• ChannelClosedBeforeSendExecRequestReturnsShouldNotDisposeChannelUnderneathIt (the Don't dispose channel when completing SshCommand #1596 race)
• CompletedCommandShouldNotBeKeptAliveBySession (WeakReference + GC; it clears Moq's recorded invocations, which would otherwise root the command through the delegates passed to add_/remove_)
• FailureToStartShouldReleaseChannelAndAllowReexecution
• CancelledCommandWhoseChannelIsNotClosedShouldReleaseChannel
• SessionErrorShouldReleaseChannel
• CancelledCommandWhoseChannelIsNotClosedShouldNotBeKeptAliveBySession

All eight fail against develop. CompletedCommandShouldNotBeKeptAliveBySession fails for the intended reason ("still referenced by the session"). Locally on macOS (arm64), net8.0/net9.0/net10.0:

• all SshCommand* unit tests pass in Debug and Release;
• the new tests passed 30/30 consecutive runs per target framework in Debug;
• the full unit test suite shows no new failures compared to develop (the same 53 pre-existing ForwardedPort test failures on this machine, on both branches);
• dotnet build Renci.SshNet.slnx with CI=true (warnings as errors) is clean.

net462 was built but not run, and the integration tests were not run locally (no Docker on this machine). I'm relying on CI for those.

Flaky integration tests (separate commit)

The first CI run failed on two integration tests that don't depend on this change. Both have also failed intermittently on unrelated branches (bcl-x25519dh, feature/configurable-socket-buffer-size, feature/connection-handler). The second commit fixes them. It's independent of the #1204 change, so I'm happy to drop it or move it to its own PR.

• Ssh_RemotePortForwarding: s.Expect("DEF") only matches the terminal's echo of the line typed into telnet, not its delivery through the forwarded port. The shell (and with it telnet) was closed immediately, and the listeners were only waited on afterwards, for 1s. The test now waits (up to 10s) for each listener to receive its bytes before closing the shell that drives its telnet session.
• Common_DetectSessionKilledOnServer: Session.RaiseError raises ErrorOccurred on the message listener thread before it calls Disconnect(), so IsConnected can still be true right after the event is signaled. The test now waits (up to 5s) for IsConnected to become false, rather than asserting it immediately.

AI disclosure

Per CONTRIBUTING.md: the root-cause analysis, design, implementation, tests and this description were written by Claude (Claude Code) under my direction. I'm relying on the local testing described above and on CI.

🤖 Generated with Claude Code