Regression: SSHSession no longer sends exit-status/exit-signal when process ends

[twisted-trac]

	@cjwatson reported
Trac ID	trac#10313
Type	release blocker: regression
Created	2022-03-04 23:41:13Z

This is a regression caused by my fix for [[#10308](https://github.com//issues/10308)](#10308), which went slightly too far in a misguided attempt at changing what I thought were two similar pieces of code in the same way. I'm sorry about that. The fix I'll supply for this will add some improved commentary and more robust tests in the hope that future travellers won't get confused in the same way.

When I tried to upgrade https://git.launchpad.net/turnip (the hosting backend for git.launchpad.net) to a backport of #1696, I found that its SSH frontend tests failed with a series of extremely opaque errors which boiled down to git saying "fatal: the remote end hung up unexpectedly", and which on detailed comparison were because the "exit-status" channel request wasn't being sent after a "git upload-pack" subprocess exited.

This turns out to be because I drew an incorrect parallelism between SSHSession.eofReceived and SSHSession.closed when fixing #10308: while it is indeed necessary to lose the client transport's connection in SSHSession.closed even if there's a session adapter as well, it's not necessary to send a close message in SSHSession.eofReceived if there's a session adapter. Firstly, the session adapter will typically already send a close message via SSHChannel.loseConnection once it's finished cleaning up (and would presumably have done so prior to the fix for #10308). Secondly, and more critically, the SSH_MSG_CHANNEL_CLOSE message sent by SSHConnection.sendClose is a request to terminate the channel, and it's supposed to be the last thing that the party sending it sends on that channel; SSHConnection refuses to send any other messages after sending a close message. As a result, any asynchronous cleanup work done by SSHSession.eofReceived, even a Deferred that fires on the next iteration of the reactor, will fail to send cleanup messages such as "exit-status" or "exit-signal", which the peer may interpret as abrupt termination; that is indeed what git does.

We failed to notice this because the session tests use a StubConnection which behaves differently: it's happy to continue to "send" messages (i.e. record them in its various dicts of messages that would have been sent to a real peer by the code under test) even if the channel has been closed, and thus fails to be an accurate stub. Fixing the stub immediately exposes the failure in SessionInterfaceTests.test_requestExecWithData.

Searchable metadata

trac-id__10313 10313
type__release_blocker__regression release blocker: regression
reporter__cjwatson cjwatson
priority__high high
milestone__None None
branch__ 
branch_author__ 
status__closed closed
resolution__fixed fixed
component__conch conch
keywords__review review
time__1646437273538043 1646437273538043
changetime__1647277719796209 1647277719796209
version__None None
owner__None None

[twisted-trac]

	@cjwatson commented

#1701

[twisted-trac]

	@cjwatson set status to closed