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

On SIGINT/SIGTERM the process could exit before the interrupted command's
cleanup defers ran, leaving decrypted data in the temp directory: the blob
cache (vaultik-blobcache-*) and the decrypted snapshot database
(vaultik-restore-*).

RunApp now mirrors fx's run sequence: start, block on app.Wait(), then
app.Stop(), returning only after Stop completes. fx delivers both an OS
interrupt and the finished operation's Shutdowner.Shutdown() on that one
channel. Stop runs the OnStop hooks; the operation's hook cancels the command
and waits for its goroutine to return (bounded by shutdownTimeout) before
exit. The old code returned as soon as app.Done fired, without Stop, so on a
real interrupt it unwound to os.Exit while cleanup still ran and the wait
never blocked exit.

Restore's loops check the context between chunks and blobs so the wait ends
promptly. A cli test drives RunApp through the OnStop hook and asserts the
scratch file is gone before RunApp returns.

Model: opus-4-8

internal/cli/app.go

@@ -7,12 +7,9 @@ import (
 	"context"
 	"errors"
 	"fmt"
-	"os"
-	"os/signal"
 	"path/filepath"
 	"strings"
 	"sync"
-	"syscall"
 	"time"
 
 	"github.com/adrg/xdg"
@@ -161,64 +158,45 @@ func cleanStartupError(err error) error {
 	return &startupError{msg: msg}
 }
 
-// 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.
+// 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.
 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)
 	}
 
-	// Handle shutdown
-	shutdownComplete := make(chan struct{})
+	// 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()
 
-	go func() {
-		defer close(shutdownComplete)
+	shutdownCtx, cancel := context.WithTimeout(
+		context.WithoutCancel(ctx), shutdownTimeout)
+	defer cancel()
 
-		<-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 shutdownCancel()
-
-		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
+	err = app.Stop(shutdownCtx)
+	if err != nil {
+		log.Error("Error during shutdown", "error", err)
 	}
+
+	return nil
 }
 
 // errReported marks a failure the operation has already shown the user

internal/cli/runapp_interrupt_test.go

@@ -0,0 +1,97 @@
+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,6 +2,7 @@ package database
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"os"
 	"path/filepath"
@@ -15,6 +16,44 @@ 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
@@ -34,6 +73,11 @@ 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

@@ -0,0 +1,100 @@
+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

@@ -0,0 +1,49 @@
+//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,6 +44,7 @@ import (
 	"errors"
 	"fmt"
 	"io"
+	"os"
 	"path/filepath"
 	"strings"
 	"time"
@@ -763,7 +764,13 @@ func (sm *SnapshotManager) compressFile(inputPath, outputPath string) error {
 	return nil
 }
 
-// copyFile copies a file from src to dst
+// 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.
 func (sm *SnapshotManager) copyFile(src, dst string) error {
 	log.Debug("Opening source file for copy", "path", src)
 
@@ -783,7 +790,9 @@ func (sm *SnapshotManager) copyFile(src, dst string) error {
 
 	log.Debug("Creating destination file", "path", dst)
 
-	destFile, err := sm.fs.Create(dst)
+	destFile, err := sm.fs.OpenFile(
+		dst, os.O_WRONLY|os.O_CREATE|os.O_TRUNC, exportCopyPerm,
+	)
 	if err != nil {
 		return err
 	}