Exit 130 and say so when a command is interrupted (closes #267)
check / check (push) Waiting to run
check / check (push) Waiting to run
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
This commit was merged in pull request #273.
This commit is contained in:
@@ -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
|
||||
|
||||
+39
-14
@@ -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
|
||||
|
||||
+15
-4
@@ -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
|
||||
}
|
||||
|
||||
|
||||
@@ -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
|
||||
}
|
||||
Reference in New Issue
Block a user