From 6a0075c76cb0198ce539765354f1a1c1ee3f7245 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 alone. A rollback that really fails is returned together with the body's error instead of ending the process; on a panic it is dropped and the panic carries on. The cancel test checks that the returned error carries no sql.ErrTxDone; the other tests make the rollback really fail by running ROLLBACK inside the body. Model: opus-5-5 --- internal/database/repositories.go | 21 +++-- internal/database/repositories_test.go | 120 +++++++++++++++++++++++++ 2 files changed, 132 insertions(+), 9 deletions(-) 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..06e588e 100644 --- a/internal/database/repositories_test.go +++ b/internal/database/repositories_test.go @@ -4,6 +4,7 @@ import ( "context" "database/sql" "errors" + "strings" "testing" "time" @@ -235,6 +236,125 @@ 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 return the body's error alone, without that rollback. +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.WithoutCancel(ctx), "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) + } + + if errors.Is(err, sql.ErrTxDone) { + t.Fatalf("the already-done rollback was reported: %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") + } +} + +// A body that ends the transaction itself with ROLLBACK makes WithTx's +// own rollback fail. WithTx must return that failure together with the +// body's error. +func TestRepositoriesTransactionRollbackFails(t *testing.T) { + t.Parallel() + + db, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + repos := database.NewRepositories(db) + + err := repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, "ROLLBACK") + if err != nil { + return err + } + + return errIntentionalRollback + }) + if !errors.Is(err, errIntentionalRollback) { + t.Fatalf("expected the body's error, got: %v", err) + } + + if !strings.Contains(err.Error(), "rolling back transaction") { + t.Errorf("expected the failed rollback in the error, got: %v", err) + } +} + +// When the body ends the transaction itself with ROLLBACK and then +// panics, WithTx's own rollback fails; the panic must still reach the +// caller. +func TestRepositoriesTransactionPanicRollbackFails(t *testing.T) { + t.Parallel() + + db, cleanup := setupTestDB(t) + defer cleanup() + + ctx := context.Background() + repos := database.NewRepositories(db) + + defer func() { + if recover() == nil { + t.Error("expected the body's panic to reach the caller") + } + }() + + _ = repos.WithTx(ctx, func(ctx context.Context, tx *sql.Tx) error { + _, err := tx.ExecContext(ctx, "ROLLBACK") + if err != nil { + return err + } + + panic(errIntentionalRollback) + }) +} + func TestRepositoriesReadTransaction(t *testing.T) { t.Parallel() -- 2.54.0