From 31a00dc325a8b541d8bd8eb03dbb993a1dc11ac4 Mon Sep 17 00:00:00 2001 From: sneak Date: Tue, 13 Oct 2026 01:45:00 +0000 Subject: [PATCH] Exit as interrupted when Ctrl-C lands during a local index write (closes #301) Once the run's context is cancelled, database/sql rolls an open transaction back itself, so the rollback in WithTx returned sql.ErrTxDone. WithTx treated any rollback error as fatal and called os.Exit(1): an interrupted backup printed "FATAL: failed to rollback transaction", exited 1 instead of 130 and skipped its deferred cleanup. WithTx now ignores sql.ErrTxDone from the rollback and returns the body's error. A rollback that really fails is returned together with the body's error instead of ending the process. On a panic the rollback error is dropped and the panic carries on. Model: opus-5-5 --- TODO.md | 10 +++++ internal/database/repositories.go | 21 ++++++---- internal/database/repositories_test.go | 58 ++++++++++++++++++++++++++ 3 files changed, 80 insertions(+), 9 deletions(-) diff --git a/TODO.md b/TODO.md index 018c80c..bdb660c 100644 --- a/TODO.md +++ b/TODO.md @@ -22,6 +22,16 @@ the tag exists and is exercised; what is left is merging `next` to # Completed Steps +- 2026-10-13: Made Ctrl-C or SIGTERM during a local index write end the + command as interrupted + ([issue #301](https://git.eeqj.de/sneak/vaultik/issues/301)). Once the + run is cancelled, `database/sql` rolls an open transaction back itself, + so the rollback in `WithTx` failed with `sql.ErrTxDone` and the process + exited 1 with `FATAL: failed to rollback transaction`, skipping the + interrupted exit and message. That result is no longer an error, and a + rollback that does fail is returned with the transaction's error + instead of ending the process. + - 2026-10-12: Rejected a retention duration longer than about 292 years ([issue #289](https://git.eeqj.de/sneak/vaultik/issues/289)). The number given to `--older-than` or `--keep-newer-than` was multiplied diff --git a/internal/database/repositories.go b/internal/database/repositories.go index 64a3f66..4db15d1 100644 --- a/internal/database/repositories.go +++ b/internal/database/repositories.go @@ -3,6 +3,7 @@ package database import ( "context" "database/sql" + "errors" "fmt" ) @@ -60,22 +61,24 @@ func (r *Repositories) WithTx(ctx context.Context, fn TxFunc) error { defer func() { if p := recover(); p != nil { - rollbackErr := tx.Rollback() - if rollbackErr != nil { - Fatalf("failed to rollback transaction: %v", rollbackErr) - } + // The panic is the failure to report, so a rollback error + // is dropped. + _ = tx.Rollback() panic(p) - } else if err != nil { - rollbackErr := tx.Rollback() - if rollbackErr != nil { - Fatalf("failed to rollback transaction: %v", rollbackErr) - } } }() err = fn(ctx, tx) if err != nil { + // Once ctx is cancelled, database/sql rolls the transaction + // back itself and Rollback returns sql.ErrTxDone. + rollbackErr := tx.Rollback() + if rollbackErr != nil && !errors.Is(rollbackErr, sql.ErrTxDone) { + return fmt.Errorf("%w; rolling back transaction: %w", + err, rollbackErr) + } + return err } diff --git a/internal/database/repositories_test.go b/internal/database/repositories_test.go index f4e2199..7950c40 100644 --- a/internal/database/repositories_test.go +++ b/internal/database/repositories_test.go @@ -235,6 +235,64 @@ func TestRepositoriesTransactionRollback(t *testing.T) { } } +// When the context is cancelled during the body, database/sql rolls the +// transaction back itself, so WithTx's own rollback finds it already +// done. WithTx must still return the body's error. +func TestRepositoriesTransactionContextCancelled(t *testing.T) { + t.Parallel() + + db, cleanup := setupTestDB(t) + defer cleanup() + + ctx, cancel := context.WithCancel(context.Background()) + defer cancel() + + repos := database.NewRepositories(db) + + err := repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { + file := &database.File{ + Path: "/test/cancelled_file.txt", + MTime: time.Now().Truncate(time.Second), + Size: 1024, + Mode: 0644, + UID: 1000, + GID: 1000, + } + + err := repos.Files.Create(ctx, tx, file) + if err != nil { + return err + } + + cancel() + + // database/sql rolls back from a goroutine of its own; wait + // until it has, so the test does not depend on that race. + for { + _, err = tx.ExecContext(context.Background(), "SELECT 1") + if errors.Is(err, sql.ErrTxDone) { + return ctx.Err() + } + + time.Sleep(time.Millisecond) + } + }) + if !errors.Is(err, context.Canceled) { + t.Fatalf("expected context.Canceled, got: %v", err) + } + + file, err := repos.Files.GetByPath( + context.Background(), "/test/cancelled_file.txt", + ) + if err != nil { + t.Fatalf("error checking for file: %v", err) + } + + if file != nil { + t.Error("file should not exist after the cancelled transaction") + } +} + func TestRepositoriesReadTransaction(t *testing.T) { t.Parallel()