ssh install ends on a signal while sftp's ssh is still connecting (closes #57)

A signal during ssh install now sends sftp a SIGTERM instead of killing it, as ssh to already does for ssh, so sftp stops the ssh it started. The sftp command also has a WaitDelay of a quarter second, so a child that still holds sftp's output cannot keep the tool waiting on a host that does not answer: it ends with status 1 and removes its working directory. The test's sftp stand-in now leaves such a child.

Judgement call: exec.ErrWaitDelay counts as success, so a run that used ControlPersist does not report a failure for a key it added.

Model: opus-5-5
Co-authored-by: clawbot <sneak+clawbot@sneak.cloud>

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 {

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)