Signals end every command, not only ssh to and ssh install (closes #48)

SIGINT, SIGTERM and SIGHUP were caught for the whole run, but only the
ssh and sftp children acted on them: the mnemonic prompt waited for
Enter, and an interrupted `age encrypt -o` put the encryption of the
cut-off input in place. Now they end the tool at once, except where a
command cleans up first: `ssh to` and `ssh install` while ssh or sftp
runs, and `age encrypt -o` and `age decrypt -o` while they write, where
a signal up to a tenth of a second after the input ends still removes
the unfinished file and exits 1. Those commands catch only the signals
the tool was not started ignoring, so a run under nohup survives a
hangup.

Model: opus-5-5

README.md

@@ -66,9 +66,8 @@ calls into `internal/`. The packages there are:
 - `internal/childmnemonic` derives a child mnemonic from the main one using
   BIP-85's own mnemonic application.
 - `internal/cli` builds the cobra command tree and runs it. Under it,
-  `cli/options` holds the flags every command shares, `cli/signals` catches
+  `cli/options` holds the flags every command shares, and `cli/ssh`, `cli/age`
-  SIGINT, SIGTERM and SIGHUP for the commands that clean up before they end, and
+  and `cli/mnemonic` are the command groups.
-  `cli/ssh`, `cli/age` and `cli/mnemonic` are the command groups.
 
 ### Adding a key type
 
@@ -291,12 +290,12 @@ passes through `ssh`'s own exit status.
 
 SIGINT, SIGTERM and SIGHUP end any command at once, at the mnemonic prompt too,
 with the status a shell gives a program killed by that signal (130 for SIGINT).
-While `age encrypt -o` or `age decrypt -o` is writing the file, the signal makes
+An interrupted `age encrypt -o` or `age decrypt -o` leaves no file. While it is
-it remove the unfinished file, leave a file already at the named path as it was,
+writing the file, the signal makes it remove the unfinished file, leave a file
-and exit with status 1. That holds for a signal that has reached `keyfunc` when
+already at the named path as it was, and exit with status 1. It puts the file in
-its input ends; a later one leaves the whole file in place. Ctrl-C on a pipeline
+place a tenth of a second after its input ends, and a signal in that time still
-ends the input at the same moment, and on Linux `keyfunc` sees the signal first,
+counts: Ctrl-C on a pipeline also ends the program feeding it, so the input can
-though no system promises that. While `ssh to` or `ssh install` has `ssh` or
+end just before the signal arrives. While `ssh to` or `ssh install` has `ssh` or
 `sftp` running, the signal ends that program instead, the tool removes its agent
 socket or working files, and it exits with status 1, or for `ssh to` with
 `ssh`'s own status if `ssh` reported one.

internal/cli/age/age.go

@@ -8,8 +8,8 @@ import (
 	"fmt"
 	"io"
 	"os"
-	"os/signal"
 	"path/filepath"
+	"time"
 
 	"github.com/spf13/cobra"
 	"sneak.berlin/go/keyfunc/internal/agekey"
@@ -24,6 +24,12 @@ var ErrInterrupted = errors.New(
 	"interrupted by a signal; the output file was left as it was",
 )
 
+// signalWait is how long after the work has ended a signal still keeps
+// the new file from being put in place. Ctrl-C on "producer | keyfunc
+// age encrypt -o file" ends the producer as well, and the end of the
+// input can reach the work a moment before the signal reaches the tool.
+const signalWait = 100 * time.Millisecond
+
 // Command returns the age command and everything under it.
 func Command() *cobra.Command {
 	group := &cobra.Command{
@@ -185,23 +191,13 @@ func input(cmd *cobra.Command, args []string) (io.Reader, func(), error) {
 // so a file that is already there survives a run that failed.
 //
 // Meanwhile SIGINT, SIGTERM and SIGHUP are caught, as signals.Context
-// does. One the tool has received by the time the work ends wins: the
+// does. One that comes while the work runs, or within signalWait after
-// new file is removed and ErrInterrupted returned, at once if the work
+// it has ended, wins: the new file is removed and ErrInterrupted
-// is still running, without waiting for it, since it may be blocked
+// returned at once, without waiting for the work, which may be blocked
 // reading its input.
 func output(
 	name string, src io.Reader, work func(io.Writer, io.Reader) error,
 ) error {
-	// received is registered before the context, so it gets every
-	// signal the context gets.
-	received := make(chan os.Signal, 1)
-	signals.Notify(received)
-
-	defer signal.Stop(received)
-
-	// The context goes on catching the signals until the file is in
-	// place or removed, so that a later one cannot end the tool with
-	// the new file left beside the named one.
 	interrupted, stop := signals.Context(context.Background())
 	defer stop()
 
@@ -219,11 +215,9 @@ func output(
 
 	select {
 	case failed := <-worked:
-		// Stop returns only once every signal the tool has received
+		select {
-		// has been handed over, so an empty received means none came.
+		case <-interrupted.Done():
-		signal.Stop(received)
+		case <-time.After(signalWait):
-
-		if len(received) == 0 {
 			return finish(file, name, failed)
 		}
 	case <-interrupted.Done():

internal/cli/age_test.go

@@ -1,11 +1,9 @@
 package cli_test
 
 import (
-	"errors"
 	"io"
 	"os"
 	"os/exec"
-	"os/signal"
 	"path/filepath"
 	"strings"
 	"syscall"
@@ -14,8 +12,6 @@ import (
 
 	"github.com/stretchr/testify/require"
 	"sneak.berlin/go/keyfunc/internal/agekey"
-	"sneak.berlin/go/keyfunc/internal/cli"
-	"sneak.berlin/go/keyfunc/internal/cli/age"
 	"sneak.berlin/go/keyfunc/internal/mnemonic"
 )
 
@@ -104,7 +100,7 @@ func TestASignalStopsAnEncryptionAndLeavesNoFile(t *testing.T) {
 	for _, ending := range []os.Signal{
 		syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP,
 	} {
-		interrupted(t, ending, "encrypt", "the start of the secret\n")
+		interrupted(t, ending, "encrypt", "the start of the secret\n", false)
 	}
 }
 
@@ -119,24 +115,11 @@ func TestASignalStopsADecryptionAndLeavesNoFile(t *testing.T) {
 	for _, ending := range []os.Signal{
 		syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP,
 	} {
-		interrupted(t, ending, "decrypt", cut)
+		interrupted(t, ending, "decrypt", cut, false)
 	}
 }
 
-func TestASignalReceivedAsTheInputEndsLeavesTheFileAsItWas(t *testing.T) {
+func TestASignalAsTheInputEndsLeavesNoFile(t *testing.T) {
-	t.Setenv(mnemonic.Variable, example())
-
-	sealed := run(t, "age", "encrypt", written(t, "notes.txt", "the secret\n"))
-
-	for _, ending := range []syscall.Signal{
-		syscall.SIGTERM, syscall.SIGINT, syscall.SIGHUP,
-	} {
-		receivedAtTheEnd(t, ending, "encrypt", "the secret\n")
-		receivedAtTheEnd(t, ending, "decrypt", sealed)
-	}
-}
-
-func TestASignalAsTheInputEndsLeavesNoUnfinishedFile(t *testing.T) {
 	t.Setenv(mnemonic.Variable, example())
 
 	sealed := run(t, "age", "encrypt", written(t, "notes.txt", "the secret\n"))
@@ -144,20 +127,10 @@ func TestASignalAsTheInputEndsLeavesNoUnfinishedFile(t *testing.T) {
 	// Ctrl-C on "producer | keyfunc age encrypt -o file" ends the
 	// producer too, so the input ends just as the signal comes, with
 	// enough of it in hand for a whole encryption or decryption. Which
-	// of the two the tool has first varies, so it is tried often, and
+	// of the two reaches the tool first varies, so it is tried often.
-	// a whole file in place is accepted as well as none.
 	for range 25 {
-		named := signalledAsTheInputEnds(t, "encrypt", "the start of the secret\n")
+		interrupted(t, syscall.SIGINT, "encrypt", "the start of the secret\n", true)
-		if named != "" {
+		interrupted(t, syscall.SIGINT, "decrypt", sealed, true)
-			require.Equal(t,
-				"the start of the secret\n", run(t, "age", "decrypt", named),
-			)
-		}
-
-		named = signalledAsTheInputEnds(t, "decrypt", sealed)
-		if named != "" {
-			require.Equal(t, "the secret\n", read(t, named))
-		}
 	}
 }
 
@@ -188,23 +161,32 @@ func TestAnEncryptionStartedUnderNohupSurvivesAHangup(t *testing.T) {
 
 // interrupted runs "age encrypt -o" or "age decrypt -o", as the
 // operation says, writing into a directory of its own, and once it has
-// begun writing sends it the signal and leaves the input open. The tool
+// begun writing sends it the signal, then ends the input if endInput
-// has to end with status 1 and leave the directory empty. A tool that
+// says so and otherwise leaves it open. The tool has to end with status
-// went on reading would not end until the input did; one that did not
+// 1 and leave the directory empty. A tool that went on reading would
-// remove the file it was writing would leave it there, with what it had
+// not end until the input did; one that did not remove the file it was
-// written so far.
+// writing would leave it there, with what it had written so far; one
-func interrupted(t *testing.T, ending os.Signal, operation, input string) {
+// that put that file in place because the input ended would leave the
+// named file.
+func interrupted(
+	t *testing.T, signal os.Signal, operation, input string, endInput bool,
+) {
 	t.Helper()
 
-	name := operation + " " + ending.String()
+	name := operation + " " + signal.String()
 	directory := t.TempDir()
 
-	command, _ := writing(
+	command, producer := writing(
 		t, directory, input,
 		os.Args[0], "age", operation, "-o", filepath.Join(directory, "notes"),
 	)
 
-	require.NoError(t, command.Process.Signal(ending))
+	require.NoError(t, command.Process.Signal(signal))
+
+	if endInput {
+		require.NoError(t, producer.Close())
+	}
+
 	waitForTool(t, name, command)
 
 	require.Equal(t, 1, command.ProcessState.ExitCode(), name)
@@ -214,108 +196,6 @@ func interrupted(t *testing.T, ending os.Signal, operation, input string) {
 	require.Empty(t, left, name)
 }
 
-// signalledAsTheInputEnds runs "age encrypt -o" or "age decrypt -o", as
-// the operation says, writing into a directory of its own, and once it
-// has begun writing sends it SIGINT and at once ends its input. Either
-// the tool ends with status 1 and leaves the directory empty, and ""
-// is returned, or it ends otherwise and leaves only the named file,
-// whose path is returned for the caller to check that it is whole.
-func signalledAsTheInputEnds(t *testing.T, operation, input string) string {
-	t.Helper()
-
-	directory := t.TempDir()
-	named := filepath.Join(directory, "notes")
-
-	command, producer := writing(
-		t, directory, input, os.Args[0], "age", operation, "-o", named,
-	)
-
-	require.NoError(t, command.Process.Signal(syscall.SIGINT))
-	require.NoError(t, producer.Close())
-	waitForTool(t, operation, command)
-
-	left, err := os.ReadDir(directory)
-	require.NoError(t, err)
-
-	if command.ProcessState.ExitCode() == failedStatus {
-		require.Empty(t, left, operation)
-
-		return ""
-	}
-
-	require.Len(t, left, 1, operation)
-
-	return named
-}
-
-// receivedAtTheEnd runs "age encrypt -o" or "age decrypt -o", as the
-// operation says, in this process, over a file that is already there,
-// with an input that at its end sends this process the signal and waits
-// until it has been received. The tool has to return ErrInterrupted and
-// leave that file as it was, with nothing beside it. A tool that went
-// by the end of the input alone would put its new file in place.
-func receivedAtTheEnd(
-	t *testing.T, ending syscall.Signal, operation, input string,
-) {
-	t.Helper()
-
-	name := operation + " " + ending.String()
-	existing := written(t, "notes", "what was already there\n")
-
-	// The test catches the signal as well, so that it does not end the
-	// test binary and so that the input can wait for it.
-	received := make(chan os.Signal, 1)
-	signal.Notify(received, ending)
-
-	defer signal.Stop(received)
-
-	root := cli.Root()
-	root.SetIn(&endingInASignal{
-		rest: strings.NewReader(input), ending: ending, received: received,
-	})
-	root.SetOut(io.Discard)
-	root.SetErr(io.Discard)
-	root.SetArgs([]string{"age", operation, "-o", existing})
-
-	err := root.ExecuteContext(t.Context())
-	require.ErrorIs(t, err, age.ErrInterrupted, name)
-
-	require.Equal(t, "what was already there\n", read(t, existing), name)
-
-	left, err := os.ReadDir(filepath.Dir(existing))
-	require.NoError(t, err)
-	require.Len(t, left, 1, name)
-}
-
-// endingInASignal is an input that, when it runs out, sends this
-// process its signal and waits for it on received before it reports its
-// end. It sends the signal only once: once nothing catches it, another
-// would end the test binary.
-type endingInASignal struct {
-	rest     io.Reader
-	ending   syscall.Signal
-	received chan os.Signal
-	sent     bool
-}
-
-func (input *endingInASignal) Read(buffer []byte) (int, error) {
-	n, err := input.rest.Read(buffer)
-	if !errors.Is(err, io.EOF) || input.sent {
-		return n, err
-	}
-
-	input.sent = true
-
-	err = syscall.Kill(os.Getpid(), input.ending)
-	if err != nil {
-		return n, err
-	}
-
-	<-input.received
-
-	return n, io.EOF
-}
-
 // writing starts argv, the tool told to write into directory, as a
 // subprocess reading the input from a pipe, and returns once the tool
 // has begun writing the file beside the one it was named. The pipe is

internal/cli/cli.go

@@ -5,7 +5,6 @@ import (
 	"errors"
 	"fmt"
 	"os"
-	"runtime"
 	"runtime/debug"
 
 	"github.com/spf13/cobra"
@@ -62,17 +61,6 @@ func Root() *cobra.Command {
 	return root
 }
 
-// init keeps the command on the main thread. Linux hands a signal sent
-// to the tool to that thread first, and a thread runs a pending signal
-// handler before its own code, so when "age encrypt -o" or "age
-// decrypt -o" checks for a signal as its input ends, one sent before
-// then, as by Ctrl-C on a pipeline, has been received.
-//
-//nolint:gochecknoinits // only an init can keep main on the main thread
-func init() {
-	runtime.LockOSThread()
-}
-
 // Main runs the tool and returns the status the process should exit
 // with. An error ends the tool with status 1, except when it carries a
 // status of its own, which "ssh to" uses to hand on the status ssh

internal/cli/signals/signals.go

@@ -16,38 +16,19 @@ import (
 // catching a signal turns an ignored one back on and would end a run
 // that was meant to survive it.
 func Context(parent context.Context) (context.Context, context.CancelFunc) {
-	endings := caught()
-
-	// Given no signals at all, NotifyContext would catch every one.
-	if len(endings) == 0 {
-		return context.WithCancel(parent)
-	}
-
-	return signal.NotifyContext(parent, endings...)
-}
-
-// Notify is signal.Notify for the signals Context catches: each one
-// that arrives is sent to c, until signal.Stop(c).
-func Notify(c chan<- os.Signal) {
-	endings := caught()
-
-	// Given no signals at all, Notify would catch every one.
-	if len(endings) > 0 {
-		signal.Notify(c, endings...)
-	}
-}
-
-// caught returns those of SIGINT, SIGTERM and SIGHUP that the tool was
-// not started with set to be ignored.
-func caught() []os.Signal {
 	endings := []os.Signal{syscall.SIGINT, syscall.SIGTERM, syscall.SIGHUP}
-	kept := make([]os.Signal, 0, len(endings))
+	caught := make([]os.Signal, 0, len(endings))
 
 	for _, ending := range endings {
 		if !signal.Ignored(ending) {
-			kept = append(kept, ending)
+			caught = append(caught, ending)
 		}
 	}
 
-	return kept
+	// Given no signals at all, NotifyContext would catch every one.
+	if len(caught) == 0 {
+		return context.WithCancel(parent)
+	}
+
+	return signal.NotifyContext(parent, caught...)
 }