diff --git a/internal/cli/ssh/install.go b/internal/cli/ssh/install.go index 263ed45..1e89e2a 100644 --- a/internal/cli/ssh/install.go +++ b/internal/cli/ssh/install.go @@ -11,6 +11,8 @@ import ( "path/filepath" "slices" "strings" + "syscall" + "time" "github.com/spf13/cobra" "sneak.berlin/go/keyfunc/internal/cli/signals" @@ -34,6 +36,13 @@ const ( localMode = 0o600 ) +// waitDelay is the WaitDelay sftp runs with: from a signal, or from sftp +// ending, how long the tool waits for sftp to end and its output to +// close before it kills sftp and stops reading. That is ample for sftp +// to stop the ssh it started, and short enough that a signal still ends +// the tool within a second. +const waitDelay = 250 * time.Millisecond + // ErrCannotEnter is the refusal of a host whose .ssh is there but // cannot be entered, so that nothing in it can be read or written. var ErrCannotEnter = errors.New( @@ -187,8 +196,24 @@ func session( command.Stdout = &said command.Stderr = &said + // A cancelled context means a signal arrived. Send sftp a SIGTERM + // rather than the default kill, so it stops the ssh it started + // before it goes. Anything sftp started that still holds its output + // keeps the tool waiting no longer than waitDelay. + command.Cancel = func() error { + return command.Process.Signal(syscall.SIGTERM) + } + command.WaitDelay = waitDelay + err := command.Run() + // sftp ended well and only something it started, such as the + // master ssh leaves running for ControlPersist under -v, still held + // its output: the session worked. + if errors.Is(err, exec.ErrWaitDelay) { + err = nil + } + _, _ = cmd.ErrOrStderr().Write(said.Bytes()) if err != nil { diff --git a/internal/cli/ssh_test.go b/internal/cli/ssh_test.go index 4776237..5342323 100644 --- a/internal/cli/ssh_test.go +++ b/internal/cli/ssh_test.go @@ -209,14 +209,16 @@ fi sleep 5 ` -// stalled is a stand-in for the system sftp that notes it has started -// and then blocks, so a test can signal the tool while sftp is running -// and watch it remove its working directory. The exec keeps the shell -// from leaving a sleep behind that holds the output the tool reads sftp -// through. +// stalled is a stand-in for the system sftp that starts a child, notes +// it has started, and then blocks, so a test can signal the tool while +// sftp is running. The child holds the output the tool reads sftp +// through, as the ssh that sftp starts does, and is started before the +// note so that it is there when the signal ends the shell and still +// holds that output afterwards. const stalled = ` +sleep 5 & touch "$KEYFUNC_TEST_STARTED" -exec sleep 5 +wait ` // pretended is where a stand-in writes down what it was asked to do. @@ -495,6 +497,22 @@ func TestWhatComesAfterTheDashesIsGivenToSFTP(t *testing.T) { ) } +func TestAProcessSFTPLeavesBehindDoesNotFailTheRun(t *testing.T) { + t.Setenv(mnemonic.Variable, example()) + + pretend := pretendHost(t) + + // A session that works ends by leaving a child behind that holds + // sftp's output, as the master ssh leaves running for ControlPersist + // does under -v. + standIn(t, "sftp", installer+"sleep 5 &\n") + + require.Equal(t, "added\n", install(t, host)) + require.Equal(t, keyLine, + read(t, filepath.Join(pretend.home, keptUnder, keptIn)), + ) +} + func TestSSHIsPointedAtTheAgentAndItsStatusIsHandedOn(t *testing.T) { t.Setenv(mnemonic.Variable, example()) @@ -634,8 +652,8 @@ func TestASignalTakesTheInstallWorkingDirectoryDown(t *testing.T) { // signalEndsTheInstall runs "ssh install" as a subprocess against a // stand-in sftp that blocks, with a temporary directory of the test's // own, waits until sftp is running, sends the tool the signal, and -// requires the working directory the tool made there to be gone once -// the tool has ended. +// requires the tool to end within a second with status 1 and the +// working directory it made there to be gone. func signalEndsTheInstall(t *testing.T, name string, signal os.Signal) { t.Helper() @@ -663,9 +681,15 @@ func signalEndsTheInstall(t *testing.T, name string, signal os.Signal) { require.NoError(t, err) require.Len(t, working, 1, name) + sent := time.Now() + require.NoError(t, command.Process.Signal(signal)) waitForTool(t, name, command) + // The child sftp started would hold sftp's output for seconds yet. + require.Less(t, time.Since(sent), time.Second, name) + require.Equal(t, failedStatus, command.ProcessState.ExitCode(), name) + left, err := os.ReadDir(temporary) require.NoError(t, err) require.Empty(t, left, name)