Skip to content

Fix SwappableDB.Swap to preserve original database on failure #2104

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Draft
wants to merge 4 commits into
base: master
Choose a base branch
from
Draft

Fix SwappableDB.Swap to preserve original database on failure #2104

wants to merge 4 commits into from

Conversation

Copilot
Copy link
Contributor

@Copilot Copilot AI commented Jun 11, 2025 •
edited
Loading

The SwappableDB.Swap() function previously deleted the original database files immediately before attempting to open the new database. If the swap operation failed for any reason (e.g., corrupted file, permission issues, invalid SQLite format), the original database was permanently lost.

This change implements a safe swap operation that follows atomic principles:

Changes Made

  1. Safe file operations: Added helper functions to rename files to temporary names instead of deleting them
  2. Atomic swap logic: The swap either completely succeeds or makes no changes at all
  3. Proper rollback: If any step fails, original files are restored and the database is reopened
  4. Complete file handling: Handles all SQLite file types (main database, WAL, SHM files)

Example of the issue:

// Before: If this failed, original database was lost forever
swappableDB.Swap("corrupted.db", false, false) // Original DB deleted, then fails

// After: Original database is preserved on any failure
swappableDB.Swap("corrupted.db", false, false) // Original DB intact if swap fails

Technical Implementation

  • renameToTemp(): Safely renames database files to temporary names with proper rollback on partial failures
  • restoreFromTemp(): Restores original files from temporary names if swap fails
  • Enhanced error handling: Reopens original database with correct configuration after failed swaps
  • Added ExecuteStringStmt(): Enhanced SwappableDB interface for better testing

Test Coverage

Added comprehensive tests:

  • Test_SwapFailurePreservesOriginal: Verifies original database remains functional after swap failures
  • Test_SwapWithWALFiles: Tests proper handling of WAL files during rollback operations
  • All existing swap tests continue to pass

The implementation ensures database operations are now safe and atomic - if a swap fails for any reason, the original database files are completely restored and remain fully functional.

Fixes #2103.

💬 Share your feedback on Copilot coding agent for the chance to win a $200 gift card! Click here to start the survey.

Copilot AI and others added 2 commits June 11, 2025 22:16
@Copilot @otoolep
Implement safe swap operation for SwappableDB
6f5b8a5 
Co-authored-by: otoolep <536312+otoolep@users.noreply.github.com>
@Copilot @otoolep
Apply go fmt to swappable_db files
c148132 
Co-authored-by: otoolep <536312+otoolep@users.noreply.github.com>
@Copilot Copilot AI changed the title [WIP] Swapping a database should not delete the files unless swapping worked Fix SwappableDB.Swap to preserve original database on failure Jun 11, 2025
@Copilot Copilot AI requested a review from otoolep June 11, 2025 22:18
Copilot finished work on behalf of otoolep June 11, 2025 22:18
@otoolep
Copy link
Member

otoolep commented Jun 11, 2025

@copilot -- I think this code could be cleaner if you check for failure in a defer method. It will require changing the signature of the function slightly. Like this:

func (s *SwappableDB) Swap(path string, fkConstraints, walEnabled bool) (retErr error) {
defer func() {
if retErr != nil {
// XXX do all the cleanup
}

WDYT?

@Copilot @otoolep
Refactor SwappableDB.Swap to use defer for cleanup
6125068 
Co-authored-by: otoolep <536312+otoolep@users.noreply.github.com>
@Copilot Copilot SWE Agent
Copy link
Contributor Author

Copilot AI commented Jun 11, 2025

@copilot -- I think this code could be cleaner if you check for failure in a defer method. It will require changing the signature of the function slightly. Like this:

func (s *SwappableDB) Swap(pa...

Good suggestion! I've refactored the Swap function to use a defer function with a named return value retErr error. This consolidates all the cleanup logic into one place and makes the happy path much cleaner. The defer function handles both success (cleaning up temp files) and failure (restoring from temp files and reopening the original database) cases. All tests continue to pass. Fixed in 6125068.

Copilot finished work on behalf of otoolep June 11, 2025 22:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@otoolep otoolep Awaiting requested review from otoolep

Assignees

@otoolep otoolep

Copilot code review Copilot

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Swapping a database should not delete the files unless swapping worked
2 participants
@Copilot @otoolep