LOW: applyMigration deferred rollback skipped when Commit() fails #125

New Issue

clawbot · 2026-02-21T09:52:36+01:00

clawbot commented

2026-02-21 09:52:36 +01:00

Bug

In internal/database/migrations.go, applyMigration has:

defer func() {
    if err != nil {
        _ = transaction.Rollback()
    }
}()

_, err = transaction.ExecContext(ctx, string(content))
// ...
_, err = transaction.ExecContext(ctx, "INSERT INTO schema_migrations ...")
// ...
commitErr := transaction.Commit()
if commitErr != nil {
    return fmt.Errorf("failed to commit migration: %w", commitErr)
}

When Commit() fails, the error is stored in commitErr, not err. The deferred function checks err, which is nil (from the last successful ExecContext). So Rollback() is never called.

Impact

In practice, SQLite automatically rolls back failed commits, so data integrity is maintained. However, this is incorrect Go transaction handling and could cause issues with other database drivers or future changes.

Fix

Assign the commit error back to err:

err = transaction.Commit()
if err != nil {
    return fmt.Errorf("failed to commit migration: %w", err)
}

## Bug In `internal/database/migrations.go`, `applyMigration` has: ```go defer func() { if err != nil { _ = transaction.Rollback() } }() _, err = transaction.ExecContext(ctx, string(content)) // ... _, err = transaction.ExecContext(ctx, "INSERT INTO schema_migrations ...") // ... commitErr := transaction.Commit() if commitErr != nil { return fmt.Errorf("failed to commit migration: %w", commitErr) } ``` When `Commit()` fails, the error is stored in `commitErr`, not `err`. The deferred function checks `err`, which is nil (from the last successful ExecContext). So `Rollback()` is never called. ## Impact In practice, SQLite automatically rolls back failed commits, so data integrity is maintained. However, this is incorrect Go transaction handling and could cause issues with other database drivers or future changes. ## Fix Assign the commit error back to `err`: ```go err = transaction.Commit() if err != nil { return fmt.Errorf("failed to commit migration: %w", err) } ```

clawbot referenced this issue from a commit

2026-02-21 09:57:36 +01:00

fix: assign commit error to err so deferred rollback triggers (closes #125)

clawbot referenced a pull request that will close this issue

2026-02-21 09:57:56 +01:00

Fix 1.0 audit bugs (closes #120, closes #121, closes #122, closes #123, closes #124, closes #125) #126

clawbot referenced this issue from a commit

2026-02-21 16:45:22 +01:00

fix: assign commit error to err so deferred rollback triggers (closes #125)

clawbot referenced this issue

2026-02-21 16:45:34 +01:00

Fix 1.0 audit bugs (closes #120, closes #121, closes #122, closes #123, closes #124, closes #125) #126