From b5389a62b5baf715d5411076a377848197b87868 Mon Sep 17 00:00:00 2001 From: clawbot <35+clawbot@noreply.example.org> Date: Wed, 7 Oct 2026 21:59:27 +0200 Subject: [PATCH] Exit 130 and say so when a command is interrupted (closes #267) Ctrl-C or SIGTERM during snapshot create, restore or verify exited 0 with no error line (1, also silent, under snapshot verify --json), so an unfinished --cron backup looked like a success. RunOperation now records whether op returned while the Vaultik context was still live; a run where it had not by the time RunWithApp returned is interrupted, whatever op returned. It used to look for context.Canceled in op's error, which verify --json does not return. Entry prints "interrupted before the command finished" on stderr for it and returns 130, under --cron and --json too. SIGTERM also gives 130, as the issue asks, not 143. The test does not cover an op still running when the 30s shutdown timeout ends. Model: opus-5-5 --- TODO.md | 9 ++ internal/cli/app.go | 53 +++++-- internal/cli/entry.go | 19 ++- internal/cli/entry_interrupt_test.go | 203 +++++++++++++++++++++++++++ 4 files changed, 266 insertions(+), 18 deletions(-) create mode 100644 internal/cli/entry_interrupt_test.go diff --git a/TODO.md b/TODO.md index 076aae6..4320581 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,15 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-07: Made an interrupted command exit 130 and say so + ([issue #267](https://git.eeqj.de/sneak/vaultik/issues/267)). Ctrl-C + or SIGTERM during `snapshot create`, `snapshot restore` or `snapshot + verify` exited 0 with no error line (`snapshot verify --json` exited + 1, also without one), so a `--cron` run that never finished looked + like a success. A command stopped by either signal now exits 130 and + prints `interrupted before the command finished` on stderr, under + `--cron` and `--json` too. + - 2026-10-07: Corrected documentation, help text and comments that were false about the code ([issue #233](https://git.eeqj.de/sneak/vaultik/issues/233)). A blob diff --git a/internal/cli/app.go b/internal/cli/app.go index 76ba625..889327f 100644 --- a/internal/cli/app.go +++ b/internal/cli/app.go @@ -206,6 +206,10 @@ func RunApp(ctx context.Context, app *fx.App) error { // RunOperation through cobra to Entry. var errReported = errors.New("operation failed") +// errInterrupted marks an operation that SIGINT or SIGTERM stopped +// before it finished. Entry shows it and returns exitCodeInterrupted. +var errInterrupted = errors.New("interrupted before the command finished") + // RunOperation runs op against the Vaultik instance inside the fx app // and turns a failure into a returned error rather than an os.Exit from // within the goroutine. An os.Exit there skipped main's deferred @@ -220,17 +224,21 @@ var errReported = errors.New("operation failed") // interrupt OnStop cancels op and waits for the goroutine to return, so // op's cleanup (removing decrypted scratch files) runs before the // process exits; the wait is bounded by shutdownTimeout. report is -// called with a non-canceled failure so the caller can show it to the -// user before it becomes errReported. A context cancellation is the -// interrupt path, not a failure: it is neither reported nor counted as -// one. +// called with a failure so the caller can show it to the user before +// it becomes errReported. +// +// The run counts as interrupted unless op returned, without an +// interrupt having cancelled it, before RunWithApp returned. An +// interrupted op is not reported, whatever it returned; RunOperation +// returns errInterrupted instead. func RunOperation( ctx context.Context, opts AppOptions, op func(v *vaultik.Vaultik) error, report func(err error), ) error { var ( - mu sync.Mutex - failed bool + mu sync.Mutex + finished bool // op returned before any interrupt cancelled it + failed bool // op finished with an error ) opts.Invokes = append(opts.Invokes, @@ -241,11 +249,21 @@ func RunOperation( OnStart: func(_ context.Context) error { stop = v.StartOperation(func() { err := op(v) - if err != nil && !errors.Is(err, context.Canceled) { - report(err) + + // Only stop, called from OnStop below, cancels the + // Vaultik context, so a live context means no + // interrupt cancelled op. Check the context, not + // err: an interrupted op need not return + // context.Canceled (`snapshot verify --json` + // returns a verification failure). + if v.Context().Err() == nil { + if err != nil { + report(err) + } mu.Lock() - failed = true + finished = true + failed = err != nil mu.Unlock() } @@ -278,16 +296,23 @@ func RunOperation( return err } - // The goroutine sets failed before triggering the shutdown that lets - // RunWithApp return, so the write is in place by the time we read it. + // RunWithApp returns only after the app was asked to stop, either by + // an interrupt or by the goroutine's Shutdown call. When op finished + // without being cancelled, the goroutine set finished before that + // call. So if finished is unset here, an interrupt stopped the app, + // and op either returned after it was cancelled or is still running + // because the shutdown timed out. mu.Lock() defer mu.Unlock() - if failed { + switch { + case !finished: + return errInterrupted + case failed: return errReported + default: + return nil } - - return nil } // runVaultikApp runs the standard single-operation command lifecycle diff --git a/internal/cli/entry.go b/internal/cli/entry.go index 5736c40..e9d3a6c 100644 --- a/internal/cli/entry.go +++ b/internal/cli/entry.go @@ -15,6 +15,11 @@ import ( // the startup banner. const shortCommitLen = 12 +// exitCodeInterrupted is the exit status of a command that SIGINT or +// SIGTERM stopped. It is 128 plus SIGINT's number, 2, which is what a +// shell reports for a command stopped by Ctrl-C. +const exitCodeInterrupted = 130 + // Entry is the main entry point for the CLI application. // It prints the startup banner to stderr (unless a banner-suppressing // flag is present in os.Args — see bannerSuppressedInArgs), executes the @@ -23,9 +28,10 @@ const shortCommitLen = 12 // The banner goes to stderr because stdout carries only the output the // user asked for, such as a completion script or a `config get` value. // -// It returns the process exit code (0 on success, 1 on error) rather -// than calling os.Exit, so that main's deferred profile writers run -// before the process ends. See run in cmd/vaultik/main.go. +// It returns the process exit code (0 on success, 130 when interrupted, +// 1 on any other error) rather than calling os.Exit, so that main's +// deferred profile writers run before the process ends. See run in +// cmd/vaultik/main.go. func Entry() int { emitStartupBanner(os.Args[1:], os.Stderr) @@ -39,11 +45,16 @@ func Entry() int { // document instead); errReported says so. Printing it again // here would double the error line. // Every other error — bad arguments, a config that would not - // load — reaches Entry unreported, so it is shown here. + // load, an interrupt — reaches Entry unreported, so it is shown + // here. if !errors.Is(err, errReported) { ReportErrorf("%s", err.Error()) } + if errors.Is(err, errInterrupted) { + return exitCodeInterrupted + } + return 1 } diff --git a/internal/cli/entry_interrupt_test.go b/internal/cli/entry_interrupt_test.go new file mode 100644 index 0000000..b11c24b --- /dev/null +++ b/internal/cli/entry_interrupt_test.go @@ -0,0 +1,203 @@ +package cli //nolint:testpackage // shares runEntry and the argument constants + +import ( + "fmt" + "net/http" + "net/http/httptest" + "os" + "os/signal" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/adrg/xdg" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// stalledStoreConfig is hermeticConfig with an s3:// destination store +// in place of the file:// one. The server behind it accepts any +// credentials. +const stalledStoreConfig = `age_recipients: + - age1278m9q7dp3chsh2dcy82qk27v047zywyvtxwnj4cvt0z65jw6a7q5dqhfj +snapshots: + test: + paths: + - %s +storage_url: s3://bucket?endpoint=%s&ssl=false +s3: + access_key_id: key + secret_access_key: secret +index_path: %s +hostname: test-host +` + +// interruptRepeat is how often interruptOnFirstRequest sends SIGINT. +const interruptRepeat = 50 * time.Millisecond + +// TestEntryInterruptedRun sends SIGINT to the test process while a +// command waits on the destination store, and checks that Entry returns +// 130 and prints one line on stderr saying the run was interrupted. The +// store is a local HTTP server that holds every request open, so the +// command is always mid-operation when the signal arrives. The two +// cases cover --cron and --json, which silence other output. +// +// Not parallel: it signals the process and replaces os.Args, os.Stdout, +// os.Stderr and the xdg globals. +// +//nolint:paralleltest // signals the process and replaces process globals +func TestEntryInterruptedRun(t *testing.T) { + for _, testCase := range []struct { + name string + args []string + }{ + { + name: "snapshot create --cron", + args: []string{cmdSnapshot, cmdCreate, "--cron"}, + }, + { + name: "snapshot verify --json", + args: []string{cmdSnapshot, cmdVerify, someSnapshotID, flagJSON}, + }, + } { + t.Run(testCase.name, func(t *testing.T) { + endpoint, requestArrived := startStalledStore(t) + configPath := writeStalledStoreConfig(t, endpoint) + interruptOnFirstRequest(t, requestArrived) + + code, _, stderr := runEntry(t, + append([]string{flagConfig, configPath}, testCase.args...)...) + + assert.Equal(t, 130, code) + assert.Equal(t, 1, + strings.Count(stderr, errInterrupted.Error()), stderr) + }) + } +} + +// interruptOnFirstRequest sends SIGINT to the test process every +// interruptRepeat, from the first request to the destination store until +// the test ends. One signal is not enough: the command can reach the +// store before fx has started catching signals. The test catches SIGINT +// too, so that a signal fx is not catching does not kill the test +// binary. +func interruptOnFirstRequest(t *testing.T, requestArrived <-chan struct{}) { + t.Helper() + + self, err := os.FindProcess(os.Getpid()) + require.NoError(t, err) + + caught := make(chan os.Signal, 1) + signal.Notify(caught, os.Interrupt) + + testEnded := make(chan struct{}) + senderDone := make(chan struct{}) + + // Stop catching SIGINT only after the sender has returned. The sender + // waits for each SIGINT it sends to arrive on caught; one still on + // its way after signal.Stop would kill the test binary. + t.Cleanup(func() { + close(testEnded) + <-senderDone + signal.Stop(caught) + }) + + go func() { + defer close(senderDone) + + select { + case <-requestArrived: + case <-testEnded: + return + } + + ticker := time.NewTicker(interruptRepeat) + defer ticker.Stop() + + for { + // Empty caught, so that the receive below waits for this + // SIGINT rather than an earlier one. + select { + case <-caught: + default: + } + + sendErr := self.Signal(os.Interrupt) + if sendErr != nil { + t.Errorf("sending SIGINT: %v", sendErr) + + return + } + + <-caught + + select { + case <-testEnded: + return + case <-ticker.C: + } + } + }() +} + +// startStalledStore starts an HTTP server that never answers: each +// request is held until the client gives up on it or the test ends. +// It returns the server's host:port and a channel that receives a value +// when the first request arrives. +func startStalledStore(t *testing.T) (string, <-chan struct{}) { + t.Helper() + + requestArrived := make(chan struct{}, 1) + release := make(chan struct{}) + + server := httptest.NewServer(http.HandlerFunc( + func(_ http.ResponseWriter, r *http.Request) { + select { + case requestArrived <- struct{}{}: + default: + } + + select { + case <-r.Context().Done(): + case <-release: + } + })) + + // Cleanups run last-registered first, so release lets any held + // request return before Close waits for it. + t.Cleanup(server.Close) + t.Cleanup(func() { close(release) }) + + return server.Listener.Addr().String(), requestArrived +} + +// writeStalledStoreConfig writes a config whose destination store is the +// server at endpoint and whose snapshot source holds one small file, so +// that `snapshot create` has a blob to upload. Returns the config path. +func writeStalledStoreConfig(t *testing.T, endpoint string) string { + t.Helper() + + dir := t.TempDir() + configPath := filepath.Join(dir, "config.yml") + sourceDir := filepath.Join(dir, "source") + + require.NoError(t, os.Mkdir(sourceDir, 0o750)) + require.NoError(t, os.WriteFile(filepath.Join(sourceDir, "file.txt"), + []byte("contents"), 0o600)) + + contents := fmt.Sprintf(stalledStoreConfig, + sourceDir, endpoint, filepath.Join(dir, "index.sqlite")) + + require.NoError(t, + os.WriteFile(configPath, []byte(contents), configFileMode)) + + // The PID lock lives under xdg.DataHome, which xdg resolves at + // package init; point it at the temp dir so the test neither + // touches nor collides with the real one. + t.Setenv("XDG_DATA_HOME", filepath.Join(dir, "data")) + xdg.Reload() + t.Cleanup(xdg.Reload) + + return configPath +}