Wait for the interrupted operation to clean up before exit (closes #159)

On SIGINT/SIGTERM, RunOperation's OnStop hook only cancelled the running
command and returned; nothing waited for its goroutine, so the process
could exit before restore's cleanup defers ran. That left decrypted data
in the temp directory: the blob cache (vaultik-blobcache-*) and the
decrypted snapshot-database directory (vaultik-restore-*).

StartOperation now returns a stop function that cancels the context and
waits for the goroutine to return, bounded by the existing
shutdownTimeout; OnStop calls it and warns if it times out. The fix is in
the shared runner, so it covers every command, not only restore.
Restore's chunk-write and blob-download loops also check the context
between steps so the wait ends promptly.

Model: opus-4-8

internal/cli/app.go

@@ -7,9 +7,12 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"os"
+	"os/signal"
 	"path/filepath"
 	"strings"
 	"sync"
+	"syscall"
 	"time"
 
 	"github.com/adrg/xdg"
@@ -158,45 +161,64 @@ func cleanStartupError(err error) error {
 	return &startupError{msg: msg}
 }
 
-// RunApp starts the fx application, blocks until it is asked to stop, and
-// then stops it. The app is asked to stop either by an OS interrupt
-// (SIGINT/SIGTERM — fx installs its own handler when app.Wait is called) or,
-// on normal completion, by the finished operation calling
-// Shutdowner.Shutdown(); both arrive on the app.Wait channel.
-//
-// Stopping runs the fx OnStop hooks, and RunApp does not return until Stop
-// returns. On an interrupt the operation's OnStop hook cancels the running
-// command and waits for it to unwind — removing its decrypted scratch files —
-// so the process cannot proceed to exit mid-cleanup (issue #159). Waiting for
-// Stop before returning is what makes that hook effective: routing the
-// interrupt through app.Stop and not returning until it completes is required,
-// because fx also fires the app.Wait channel on the signal, and an earlier
-// version returned on that alone — unwinding to os.Exit while the concurrent
-// cleanup still ran. The stop is bounded by shutdownTimeout. Returns an error
-// if startup fails.
+// RunApp starts and stops the fx application within the given context.
+// It handles graceful shutdown on interrupt signals (SIGINT, SIGTERM) and
+// ensures the application stops cleanly. The function blocks until the
+// application completes or is interrupted. Returns an error if startup fails.
 func RunApp(ctx context.Context, app *fx.App) error {
+	// Set up signal handling for graceful shutdown
+	sigChan := make(chan os.Signal, 1)
+	signal.Notify(sigChan, os.Interrupt, syscall.SIGTERM)
+
+	// Create a context that will be cancelled on signal
+	ctx, cancel := context.WithCancel(ctx)
+	defer cancel()
+
+	// Start the app
 	err := app.Start(ctx)
 	if err != nil {
 		return cleanStartupError(err)
 	}
 
-	// Block until an interrupt or the finished operation's
-	// Shutdowner.Shutdown() arrives, then stop the app in this goroutine so we
-	// return only after its OnStop hooks — including the operation's cleanup
-	// wait — have run. Detach the stop from ctx's cancellation but keep its
-	// values, and bound it by shutdownTimeout.
-	<-app.Wait()
+	// Handle shutdown
+	shutdownComplete := make(chan struct{})
 
-	shutdownCtx, cancel := context.WithTimeout(
+	go func() {
+		defer close(shutdownComplete)
+
+		<-sigChan
+		log.Notice("Received interrupt signal, shutting down gracefully...")
+
+		// Create a timeout context for shutdown. The parent ctx is being
+		// cancelled, so detach from its cancellation but keep its values.
+		shutdownCtx, shutdownCancel := context.WithTimeout(
 			context.WithoutCancel(ctx), shutdownTimeout)
-	defer cancel()
+		defer shutdownCancel()
 
-	err = app.Stop(shutdownCtx)
+		err := app.Stop(shutdownCtx)
 		if err != nil {
 			log.Error("Error during shutdown", "error", err)
 		}
+	}()
 
+	// Wait for the signal handler to complete shutdown or the app to
+	// request shutdown.
+	select {
+	case <-shutdownComplete:
+		// Shutdown completed via signal
 		return nil
+	case <-ctx.Done():
+		// Context cancelled (shouldn't happen in normal operation)
+		err := app.Stop(context.WithoutCancel(ctx))
+		if err != nil {
+			log.Error("Error stopping app", "error", err)
+		}
+
+		return ctx.Err()
+	case <-app.Done():
+		// App finished running (e.g., backup completed)
+		return nil
+	}
 }
 
 // errReported marks a failure the operation has already shown the user

internal/cli/runapp_interrupt_test.go

@@ -1,97 +0,0 @@
-package cli_test
-
-import (
-	"context"
-	"os"
-	"path/filepath"
-	"testing"
-	"time"
-
-	"github.com/stretchr/testify/require"
-	"go.uber.org/fx"
-	"sneak.berlin/go/vaultik/internal/cli"
-)
-
-// TestRunAppWaitsForOperationCleanupOnShutdown drives RunApp with an fx app
-// wired the way RunOperation wires a command: a single lifecycle hook whose
-// OnStart launches the operation in its own goroutine and whose OnStop cancels
-// it and blocks until that goroutine returns. The operation stands in for a
-// restore blocked mid-download — it holds a decrypted "scratch" file and only
-// removes it as it unwinds on cancellation.
-//
-// The app is asked to stop once the operation is running (standing in for an
-// OS interrupt; fx delivers a real signal and Shutdowner.Shutdown() on the
-// same app.Wait channel, so both drive the identical shutdown path). RunApp
-// must not return until app.Stop has run the OnStop hook, so the scratch file
-// must be gone by the time RunApp returns. Before the fix RunApp returned as
-// soon as the app.Wait/Done channel fired, without running app.Stop, so the
-// cleanup never ran and this file would still be on disk (issue #159).
-func TestRunAppWaitsForOperationCleanupOnShutdown(t *testing.T) {
-	t.Parallel()
-
-	scratch := filepath.Join(t.TempDir(), "decrypted-scratch")
-	require.NoError(t, os.WriteFile(scratch, []byte("secret"), 0o600))
-
-	// Cancel and reap the operation even if RunApp returns without doing so
-	// (the buggy path), so the goroutine cannot leak past the test.
-	opCtx, opCancel := context.WithCancel(context.Background())
-	t.Cleanup(opCancel)
-
-	var stop func(context.Context) bool
-
-	app := fx.New(
-		fx.NopLogger,
-		fx.Invoke(func(lc fx.Lifecycle, sh fx.Shutdowner) {
-			lc.Append(fx.Hook{
-				OnStart: func(_ context.Context) error {
-					done := make(chan struct{})
-
-					go func() {
-						defer close(done)
-
-						// Blocked mid-operation until cancelled, then run the
-						// cleanup an interrupted restore would run.
-						<-opCtx.Done()
-
-						_ = os.Remove(scratch)
-					}()
-
-					stop = func(ctx context.Context) bool {
-						opCancel()
-
-						select {
-						case <-done:
-							return true
-						case <-ctx.Done():
-							return false
-						}
-					}
-
-					// Ask the app to stop now that the operation is running.
-					go func() { _ = sh.Shutdown() }()
-
-					return nil
-				},
-				OnStop: func(ctx context.Context) error {
-					stop(ctx)
-
-					return nil
-				},
-			})
-		}),
-	)
-
-	done := make(chan error, 1)
-	go func() { done <- cli.RunApp(context.Background(), app) }()
-
-	select {
-	case err := <-done:
-		require.NoError(t, err)
-	case <-time.After(30 * time.Second):
-		t.Fatal("RunApp did not return after shutdown was requested")
-	}
-
-	_, err := os.Stat(scratch)
-	require.True(t, os.IsNotExist(err),
-		"RunApp returned before the operation removed its decrypted scratch file")
-}

internal/database/module.go

@@ -2,7 +2,6 @@ package database
 
 import (
 	"context"
-	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -16,44 +15,6 @@ import (
 // the index describes the backed-up file tree and must stay private.
 const indexDirPerm = 0o700
 
-// indexFilePerm restricts the index file to the owning user; it lists every
-// backed-up path and chunk hash and must stay private.
-const indexFilePerm = 0o600
-
-// ensureIndexFileMode makes the index file owner-only before the SQLite
-// driver opens it: it creates the file 0600 if absent, or chmods an existing
-// one to 0600. Doing this first matters because SQLite creates its -wal and
-// -shm side files with the mode of the main database file, so a private main
-// file yields private side files. The driver treats a zero-byte file as an
-// empty database, so pre-creating it here is safe.
-func ensureIndexFileMode(path string) error {
-	info, err := os.Stat(path)
-
-	switch {
-	case err == nil:
-		if info.Mode().Perm() == indexFilePerm {
-			return nil
-		}
-
-		err = os.Chmod(path, indexFilePerm)
-		if err != nil {
-			return fmt.Errorf("restricting index file permissions: %w", err)
-		}
-
-		return nil
-	case errors.Is(err, os.ErrNotExist):
-		//nolint:gosec // G304: the index path is operator-configured by design
-		f, err := os.OpenFile(path, os.O_CREATE|os.O_WRONLY, indexFilePerm)
-		if err != nil {
-			return fmt.Errorf("creating index file: %w", err)
-		}
-
-		return f.Close()
-	default:
-		return fmt.Errorf("checking index file: %w", err)
-	}
-}
-
 // Module provides database dependencies
 //
 //nolint:gochecknoglobals // fx module definitions are package globals by convention
@@ -73,11 +34,6 @@ func provideDatabase(lc fx.Lifecycle, cfg *config.Config) (*DB, error) {
 		return nil, fmt.Errorf("creating index directory: %w", err)
 	}
 
-	err = ensureIndexFileMode(cfg.IndexPath)
-	if err != nil {
-		return nil, err
-	}
-
 	db, err := New(context.Background(), cfg.IndexPath)
 	if err != nil {
 		return nil, fmt.Errorf("opening database: %w", err)

internal/database/module_internal_test.go

@@ -1,100 +0,0 @@
-package database
-
-import (
-	"os"
-	"path/filepath"
-	"syscall"
-	"testing"
-
-	"go.uber.org/fx/fxtest"
-	"sneak.berlin/go/vaultik/internal/config"
-)
-
-// TestProvideDatabaseFreshIndexMode verifies that provideDatabase creates a
-// missing index file owner-only (0600), even under a lenient 022 umask that
-// would otherwise leave a freshly created file world-readable.
-//
-//nolint:paralleltest // syscall.Umask is process-global; parallel tests would clash
-func TestProvideDatabaseFreshIndexMode(t *testing.T) {
-	restore := syscall.Umask(0o022)
-	defer syscall.Umask(restore)
-
-	indexPath := filepath.Join(t.TempDir(), "index.sqlite")
-
-	openIndex(t, indexPath)
-	assertPerm(t, indexPath, 0o600)
-}
-
-// TestProvideDatabaseExistingIndexMode verifies that provideDatabase tightens
-// an existing world-readable index (0644) in a group/other-readable directory
-// down to owner-only (0600).
-//
-//nolint:paralleltest // syscall.Umask is process-global; parallel tests would clash
-func TestProvideDatabaseExistingIndexMode(t *testing.T) {
-	restore := syscall.Umask(0o022)
-	defer syscall.Umask(restore)
-
-	dir := filepath.Join(t.TempDir(), "data")
-
-	//nolint:gosec // G301: the test intentionally uses a 0755 directory
-	err := os.MkdirAll(dir, 0o755)
-	if err != nil {
-		t.Fatalf("creating index directory: %v", err)
-	}
-
-	//nolint:gosec // G302: the test intentionally uses a 0755 directory
-	err = os.Chmod(dir, 0o755)
-	if err != nil {
-		t.Fatalf("relaxing index directory permissions: %v", err)
-	}
-
-	indexPath := filepath.Join(dir, "index.sqlite")
-
-	//nolint:gosec // G306: the test intentionally starts from a 0644 index
-	err = os.WriteFile(indexPath, nil, 0o644)
-	if err != nil {
-		t.Fatalf("creating pre-existing index: %v", err)
-	}
-
-	//nolint:gosec // G302: the test intentionally starts from a 0644 index
-	err = os.Chmod(indexPath, 0o644)
-	if err != nil {
-		t.Fatalf("relaxing pre-existing index permissions: %v", err)
-	}
-
-	openIndex(t, indexPath)
-	assertPerm(t, indexPath, 0o600)
-}
-
-// openIndex runs provideDatabase against indexPath and closes the resulting
-// database before returning.
-func openIndex(t *testing.T, indexPath string) {
-	t.Helper()
-
-	cfg := &config.Config{IndexPath: indexPath}
-
-	db, err := provideDatabase(fxtest.NewLifecycle(t), cfg)
-	if err != nil {
-		t.Fatalf("provideDatabase: %v", err)
-	}
-
-	err = db.Close()
-	if err != nil {
-		t.Fatalf("closing database: %v", err)
-	}
-}
-
-// assertPerm fails the test unless path has exactly the given permission bits.
-func assertPerm(t *testing.T, path string, want os.FileMode) {
-	t.Helper()
-
-	info, err := os.Stat(path)
-	if err != nil {
-		t.Fatalf("stat %s: %v", path, err)
-	}
-
-	got := info.Mode().Perm()
-	if got != want {
-		t.Fatalf("permissions of %s = %#o, want %#o", path, got, want)
-	}
-}

internal/snapshot/export_perm_test.go

@@ -1,49 +0,0 @@
-//nolint:testpackage // exercises the unexported copyFile helper
-package snapshot
-
-import (
-	"os"
-	"path/filepath"
-	"syscall"
-	"testing"
-
-	"github.com/spf13/afero"
-)
-
-// TestCopyFileExportCopyMode verifies that the exported snapshot database
-// copy is created owner-only (0600), even under a lenient 022 umask that
-// would otherwise leave a fresh file world-readable.
-//
-//nolint:paralleltest // syscall.Umask is process-global; parallel tests would clash
-func TestCopyFileExportCopyMode(t *testing.T) {
-	restore := syscall.Umask(0o022)
-	defer syscall.Umask(restore)
-
-	dir := t.TempDir()
-
-	src := filepath.Join(dir, "index.sqlite")
-
-	err := os.WriteFile(src, []byte("index data"), 0o600)
-	if err != nil {
-		t.Fatalf("creating source index: %v", err)
-	}
-
-	dst := filepath.Join(dir, "snapshot.db")
-
-	sm := &SnapshotManager{fs: afero.NewOsFs()}
-
-	err = sm.copyFile(src, dst)
-	if err != nil {
-		t.Fatalf("copyFile: %v", err)
-	}
-
-	info, err := os.Stat(dst)
-	if err != nil {
-		t.Fatalf("stat export copy: %v", err)
-	}
-
-	got := info.Mode().Perm()
-	if got != 0o600 {
-		t.Fatalf("export copy permissions = %#o, want %#o", got, 0o600)
-	}
-}

internal/snapshot/snapshot.go

@@ -44,7 +44,6 @@ import (
 	"errors"
 	"fmt"
 	"io"
-	"os"
 	"path/filepath"
 	"strings"
 	"time"
@@ -764,13 +763,7 @@ func (sm *SnapshotManager) compressFile(inputPath, outputPath string) error {
 	return nil
 }
 
-// exportCopyPerm restricts the exported snapshot database copy to the owning
-// user; it holds the same private index data as the local index file.
-const exportCopyPerm = 0o600
-
-// copyFile copies a file from src to dst. The destination is the exported
-// snapshot database, so it is created owner-only rather than with the
-// umask-dependent default.
+// copyFile copies a file from src to dst
 func (sm *SnapshotManager) copyFile(src, dst string) error {
 	log.Debug("Opening source file for copy", "path", src)
 
@@ -790,9 +783,7 @@ func (sm *SnapshotManager) copyFile(src, dst string) error {
 
 	log.Debug("Creating destination file", "path", dst)
 
-	destFile, err := sm.fs.OpenFile(
-		dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, exportCopyPerm,
-	)
+	destFile, err := sm.fs.Create(dst)
 	if err != nil {
 		return err
 	}