Skip to content

fix: close ssh sessions gracefully #10732

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Nov 17, 2023

Conversation

spikecurtis
Copy link
Contributor

Re-enables TestSSH/RemoteForward_Unix_Signal and addresses the underlying race: we were not closing the remote forward on context expiry, only the session and connection.

However, there is still a more fundamental issue in that we don't have the ability to ensure that TCP sessions are properly terminated before tearing down the Tailnet conn. This is due to the assumption in the sockets API, that the underlying IP interface is long
lived compared with the TCP socket, and thus closing a socket returns immediately and does not wait for the TCP termination handshake --- that is handled async in the tcpip stack. However, this assumption does not hold for us and tailnet, since on shutdown,
we also tear down the tailnet connection, and this can race with the TCP termination.

Closing the remote forward explicitly should prevent forward state from accumulating, since the Close() function waits for a reply from the remote SSH server.

I've also attempted to workaround the TCP/tailnet issue for --stdio by using CloseWrite() instead of Close(). By closing the write side of the connection, half-close the TCP connection, and the server detects this and closes the other direction, which then
triggers our read loop to exit only after the server has had a chance to process the close.

TODO in a stacked PR is to implement this logic for vscodessh as well.

Copy link
Contributor Author

Current dependencies on/for this PR:

This stack of pull requests is managed by Graphite.

@spikecurtis spikecurtis force-pushed the spike/close-ssh-sessions-gracefully branch from ad2ffdd to 70ec85c Compare November 17, 2023 07:06
@spikecurtis spikecurtis requested a review from johnstcn November 17, 2023 07:06
@spikecurtis spikecurtis force-pushed the spike/close-ssh-sessions-gracefully branch from 70ec85c to 6bcee71 Compare November 17, 2023 07:09
@spikecurtis spikecurtis merged commit 3dd35e0 into main Nov 17, 2023
@spikecurtis spikecurtis deleted the spike/close-ssh-sessions-gracefully branch November 17, 2023 08:43
Copy link
Contributor Author

Merge activity

@github-actions github-actions bot locked and limited conversation to collaborators Nov 17, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants