fix: correctness defects from codebase audit (signal race, archive cleanup, rsync capture)

cmd/shuttle/main.go

@@ -158,24 +158,29 @@ func run() int {
 
 	rootCmd.AddCommand(runCmd, versionCmd, validateCmd, doctorCmd)
 
-	// Context wired to OS signals. The goroutine sets signaled before canceling
-	// so the exit-code check below can distinguish a signal from a normal error.
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
-	var signaled bool
-	sigCh := make(chan os.Signal, 1)
-	signal.Notify(sigCh, syscall.SIGINT, syscall.SIGTERM)
+	// Context canceled on SIGINT/SIGTERM. The parent is Background and stop
+	// runs only after the exit-code check below, so once ExecuteContext
+	// returns, ctx.Err() != nil can mean exactly one thing: a signal arrived.
+	ctx, stop := signal.NotifyContext(context.Background(), syscall.SIGINT, syscall.SIGTERM)
+	defer stop()
+
+	// Companion channel for UX only: print the interrupt notice at receipt
+	// time and restore the default disposition so a second signal terminates
+	// the process immediately instead of being swallowed. Registration order
+	// is load-bearing: NotifyContext above must register first so a signal
+	// landing between the two calls still cancels (at worst the notice line
+	// is skipped).
+	interruptCh := make(chan os.Signal, 1)
+	signal.Notify(interruptCh, syscall.SIGINT, syscall.SIGTERM)
 	go func() {
-		<-sigCh
-		signaled = true
+		<-interruptCh
+		signal.Reset(syscall.SIGINT, syscall.SIGTERM)
 		fmt.Fprintln(os.Stderr, "\nInterrupted. Shutting down...")
-		cancel()
 	}()
 
 	err := rootCmd.ExecuteContext(ctx)
 
-	if signaled {
+	if ctx.Err() != nil {
 		return exitSignal
 	}
 	if errors.Is(err, errPartialFailure) {

cmd/shuttle/main_test.go

@@ -1,13 +1,16 @@
 package main
 
 import (
+	"bufio"
 	"bytes"
 	"errors"
 	"fmt"
+	"io"
 	"os"
 	"os/exec"
 	"path/filepath"
 	"strings"
+	"syscall"
 	"testing"
 	"time"
 
@@ -29,7 +32,7 @@ func TestMain(m *testing.M) {
 
 	shuttleBin = filepath.Join(dir, "shuttle")
 	// Build from repo root; test files run from the package directory.
-	buildCmd := exec.Command("go", "build", "-o", shuttleBin, "./cmd/shuttle")
+	buildCmd := exec.Command("go", "build", "-race", "-o", shuttleBin, "./cmd/shuttle")
 	buildCmd.Dir = filepath.Join("..", "..")
 	if out, err := buildCmd.CombinedOutput(); err != nil {
 		fmt.Fprintf(os.Stderr, "building shuttle: %v\n%s\n", err, out)
@@ -877,3 +880,84 @@ func TestResolveRclonePassword_EnvPreset_ReturnsEmpty(t *testing.T) {
 		t.Errorf("RCLONE_CONFIG_PASS = %q, want unchanged 'already-set'", v)
 	}
 }
+
+// assertSignalExitsWithSignalCode verifies the public exit-code contract for
+// interrupted runs: the given signal during an active job yields exit 130 and
+// the interrupt notice on stderr. The job is throttled via --bwlimit so the
+// process is reliably still alive when the signal lands; the exit code is
+// 130 regardless of which pipeline stage the cancellation interrupts.
+func assertSignalExitsWithSignalCode(t *testing.T, sig syscall.Signal) {
+	t.Helper()
+	if _, err := exec.LookPath("rsync"); err != nil {
+		t.Skip("rsync not found on PATH")
+	}
+	src := t.TempDir()
+	payload := make([]byte, 1<<20) // 1 MiB at --bwlimit=100 (KB/s) ≈ 10s window
+	if err := os.WriteFile(filepath.Join(src, "big.bin"), payload, 0o644); err != nil {
+		t.Fatalf("writing payload: %v", err)
+	}
+	dst := t.TempDir()
+
+	env := writeConfig(t, fmt.Sprintf(`
+[[job]]
+name = "slow"
+engine = "rsync"
+sources = [%q]
+destination = %q
+extra_flags = ["--bwlimit=100"]
+`, filepath.Join(src, "big.bin"), dst))
+
+	cmd := exec.Command(shuttleBin, "run")
+	cmd.Env = env
+	stdout, err := cmd.StdoutPipe()
+	if err != nil {
+		t.Fatalf("stdout pipe: %v", err)
+	}
+	var stderrBuf bytes.Buffer
+	cmd.Stderr = &stderrBuf
+	if err := cmd.Start(); err != nil {
+		t.Fatalf("starting shuttle: %v", err)
+	}
+
+	// Signal only after the run has observably started so the process
+	// cannot have exited before the kill lands.
+	scanner := bufio.NewScanner(stdout)
+	started := false
+	for scanner.Scan() {
+		if strings.Contains(scanner.Text(), "Shuttle Started") {
+			started = true
+			break
+		}
+	}
+	if !started {
+		_ = cmd.Process.Kill()
+		t.Fatalf("never saw startup line; stderr: %s", stderrBuf.String())
+	}
+	if err := cmd.Process.Signal(sig); err != nil {
+		t.Fatalf("sending %v: %v", sig, err)
+	}
+	// Drain remaining stdout so the child never blocks on a full pipe.
+	go func() { _, _ = io.Copy(io.Discard, stdout) }()
+
+	err = cmd.Wait()
+	var exitErr *exec.ExitError
+	if !errors.As(err, &exitErr) {
+		t.Fatalf("Wait() = %v, want ExitError with code 130", err)
+	}
+	if code := exitErr.ExitCode(); code != 130 {
+		t.Errorf("exit code = %d, want 130; stderr: %s", code, stderrBuf.String())
+	}
+	if !strings.Contains(stderrBuf.String(), "Interrupted") {
+		t.Errorf("stderr = %q, want interrupt notice", stderrBuf.String())
+	}
+}
+
+func TestCLI_SIGINT_ExitsWithSignalCode(t *testing.T) {
+	assertSignalExitsWithSignalCode(t, syscall.SIGINT)
+}
+
+// SIGTERM is what cron and launchd send on shutdown, so the second registered
+// signal gets the same contract coverage as the interactive ctrl-C path.
+func TestCLI_SIGTERM_ExitsWithSignalCode(t *testing.T) {
+	assertSignalExitsWithSignalCode(t, syscall.SIGTERM)
+}

internal/engine/rclone.go

@@ -4,6 +4,7 @@ import (
 	"bufio"
 	"bytes"
 	"context"
+	"errors"
 	"fmt"
 	"io"
 	"os"
@@ -233,17 +234,73 @@ func selectMode(mode, destination, remoteName, backupPath, runTimestamp string,
 	return "sync", ""
 }
 
+// archiveDateLayout is the date prefix on archive directory names; the run
+// timestamp used for --backup-dir construction starts with this format.
+const archiveDateLayout = "2006-01-02"
+
+// rcloneExitDirNotFound is rclone's documented exit code for "directory not
+// found", the one lsd failure that means "no archives yet" rather than a
+// real problem. https://rclone.org/docs/#exit-code
+const rcloneExitDirNotFound = 3
+
+// archiveDirExpired classifies an archive directory name against a cutoff
+// date (archiveDateLayout format). recognized is false when the name does
+// not begin with a calendar-valid date, in which case expired is
+// meaningless. Pure so purge eligibility is testable without rclone; the
+// caller filters empty names before calling.
+func archiveDirExpired(dirName, cutoff string) (expired, recognized bool) {
+	if len(dirName) < len(archiveDateLayout) {
+		return false, false
+	}
+	datePrefix := dirName[:len(archiveDateLayout)]
+	if _, err := time.Parse(archiveDateLayout, datePrefix); err != nil {
+		return false, false
+	}
+	return datePrefix < cutoff, true
+}
+
+// isDirNotFound reports whether an rclone invocation failed only because the
+// target directory does not exist.
+func isDirNotFound(err error) bool {
+	var exitErr *exec.ExitError
+	return errors.As(err, &exitErr) && exitErr.ExitCode() == rcloneExitDirNotFound
+}
+
+// firstStderrLine returns the first stderr line captured by Output(), or ""
+// when stderr was empty (rclone routes error text to --log-file when set,
+// leaving stderr blank in normal runs).
+func firstStderrLine(err error) string {
+	var exitErr *exec.ExitError
+	if !errors.As(err, &exitErr) || len(exitErr.Stderr) == 0 {
+		return ""
+	}
+	return oneLine(string(exitErr.Stderr))
+}
+
+// purgeArchiveDir runs rclone purge for one expired archive directory,
+// mirroring the lsd call's log-file routing. The caller decides how to
+// handle a failure (warn and continue).
+func (e *RcloneExecutor) purgeArchiveDir(ctx context.Context, target string) error {
+	purgeArgs := []string{"purge", target}
+	if e.logFile != "" {
+		purgeArgs = append(purgeArgs, "--log-file", e.logFile, "--log-level", "INFO")
+	}
+	return e.rcloneCommand(ctx, purgeArgs...).Run()
+}
+
 // CleanupArchives purges archive subdirectories older than retentionDays
-// from the backup root on the given remote. It is non-fatal: individual purge
-// failures are logged as warnings and do not stop processing of remaining
-// directories. Skipped during dry-run, when backupPath is empty, or when
-// retentionDays is non-positive.
+// from the backup root on the given remote. Individual purge failures are
+// logged as warnings and do not stop processing of remaining directories,
+// but a failure to list the backup root at all is returned as an error so
+// the caller can surface it; a missing backup root (rclone exit 3) is the
+// expected first-run state and stays a non-error. Skipped during dry-run,
+// when backupPath is empty, or when retentionDays is non-positive.
 func (e *RcloneExecutor) CleanupArchives(ctx context.Context, remoteName, backupPath string, retentionDays int, dryRun bool) error {
 	if backupPath == "" || retentionDays <= 0 || dryRun {
 		return nil
 	}
 
-	cutoff := time.Now().AddDate(0, 0, -retentionDays).Format("2006-01-02")
+	cutoff := time.Now().AddDate(0, 0, -retentionDays).Format(archiveDateLayout)
 	archiveRoot := fmt.Sprintf("%s:%s", remoteName, strings.TrimRight(backupPath, "/"))
 
 	lsdArgs := []string{"lsd", archiveRoot + "/"}
@@ -252,8 +309,14 @@ func (e *RcloneExecutor) CleanupArchives(ctx context.Context, remoteName, backup
 	}
 	output, err := e.rcloneCommand(ctx, lsdArgs...).Output()
 	if err != nil {
-		e.logger.Info(fmt.Sprintf("no archive directory on %s (nothing to clean)", remoteName))
-		return nil
+		if isDirNotFound(err) {
+			e.logger.Info(fmt.Sprintf("no archive directory on %s (nothing to clean)", remoteName))
+			return nil
+		}
+		if detail := firstStderrLine(err); detail != "" {
+			return fmt.Errorf("listing archive root %s: %w (%s)", archiveRoot, err, detail)
+		}
+		return fmt.Errorf("listing archive root %s: %w", archiveRoot, err)
 	}
 
 	purged := 0
@@ -264,30 +327,25 @@ func (e *RcloneExecutor) CleanupArchives(ctx context.Context, remoteName, backup
 			continue
 		}
 		dirName := fields[len(fields)-1]
-		// Archive directory names are prefixed with a YYYY-MM-DD date stamp.
-		// Skip entries that are too short to contain a full date prefix.
-		if len(dirName) < 10 {
+		expired, recognized := archiveDirExpired(dirName, cutoff)
+		if !recognized {
+			e.logger.Warn(fmt.Sprintf("archive cleanup: skipping unrecognized directory %s on %s", dirName, remoteName))
 			continue
 		}
-		dirDate := dirName[:10]
-		// Validate that the prefix looks like YYYY-MM-DD before comparing.
-		if dirDate[4] != '-' || dirDate[7] != '-' {
+		if !expired {
 			continue
 		}
-		if dirDate < cutoff {
-			target := archiveRoot + "/" + dirName
-			e.logger.Info(fmt.Sprintf("purging expired archive: %s (%s < %s)", target, dirDate, cutoff))
-			purgeArgs := []string{"purge", target}
-			if e.logFile != "" {
-				purgeArgs = append(purgeArgs, "--log-file", e.logFile, "--log-level", "INFO")
-			}
-			if purgeErr := e.rcloneCommand(ctx, purgeArgs...).Run(); purgeErr != nil {
-				e.logger.Warn(fmt.Sprintf("failed to purge %s: %v", target, purgeErr))
-			} else {
-				purged++
-			}
+		target := archiveRoot + "/" + dirName
+		e.logger.Info(fmt.Sprintf("purging expired archive: %s (%s < %s)", target, dirName[:len(archiveDateLayout)], cutoff))
+		if purgeErr := e.purgeArchiveDir(ctx, target); purgeErr != nil {
+			e.logger.Warn(fmt.Sprintf("failed to purge %s: %v", target, purgeErr))
+		} else {
+			purged++
 		}
 	}
+	if scanErr := scanner.Err(); scanErr != nil {
+		return fmt.Errorf("scanning archive listing for %s: %w", archiveRoot, scanErr)
+	}
 
 	if purged > 0 {
 		e.logger.Info(fmt.Sprintf("archive cleanup: purged %d expired director(ies) from %s", purged, remoteName))