Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 18 additions & 0 deletions app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt
Original file line number Diff line number Diff line change
Expand Up @@ -206,6 +206,24 @@ abstract class InternalDatabase : RoomDatabase() {
},
).build(),
)

fun newInternalDatabaseInstance(context: Context, dbName: String = DB_NAME): InternalDatabase =
Room
.databaseBuilder(context, InternalDatabase::class.java, dbName)
.openHelperFactory(BackupBeforeMigrationFactory(context, dbName))
.addMigrations(
MIGRATION_1_2,
MIGRATION_21_24,
MIGRATION_22_24,
MIGRATION_24_25,
).setJournalMode(RoomDatabase.JournalMode.WRITE_AHEAD_LOGGING)
.setTransactionExecutor(
java.util.concurrent.Executors
.newFixedThreadPool(4),
).setQueryExecutor(
java.util.concurrent.Executors
.newFixedThreadPool(4),
).build()
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@ import dagger.hilt.android.lifecycle.HiltViewModel
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.runBlocking
import timber.log.Timber
import java.io.File
import java.io.FileInputStream
import java.io.FileOutputStream
import java.util.zip.ZipEntry
Expand Down Expand Up @@ -99,11 +100,31 @@ class BackupRestoreViewModel @Inject constructor(
}

fun restore(context: Context, uri: Uri, clearAuthData: Boolean = false) {
var migrationSucceeded: Boolean? = null
runCatching {
Timber.tag("RESTORE").i("Starting restore from URI: $uri, clearAuthData: $clearAuthData")

// Backup current DB before restore
val currentDbPath = database.openHelper.writableDatabase.path
val backupDbFile = if (currentDbPath != null) {
val backupFile = File("${currentDbPath}_restore_backup_${System.currentTimeMillis()}")
try {
File(currentDbPath).copyTo(backupFile, overwrite = true)
val walFile = File("${currentDbPath}-wal")
if (walFile.exists()) walFile.copyTo(File("${backupFile.absolutePath}-wal"), overwrite = true)
val shmFile = File("${currentDbPath}-shm")
if (shmFile.exists()) shmFile.copyTo(File("${backupFile.absolutePath}-shm"), overwrite = true)
Timber.tag("RESTORE").i("Created DB backup at ${backupFile.absolutePath}")
backupFile
} catch (e: Exception) {
Timber.tag("RESTORE").e(e, "Failed to create DB backup")
null
}
} else null
Comment on lines +107 to +123
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Take the rollback snapshot after the database is quiesced.

This copy happens while the current Room instance is still open and before the checkpoint/close at Lines 146-147. In WAL mode that means the backup used for rollback is not guaranteed to be a self-consistent snapshot, so a failed migration can still roll back to missing or inconsistent data. Move the checkpoint/close ahead of this copy step, or use SQLite’s backup API for the pre-restore snapshot.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 107 - 123, The current rollback snapshot copy uses
currentDbPath/backupFile while the Room instance is still active; quiesce the DB
before taking the backup by performing the WAL checkpoint and closing the Room
database (i.e., invoke the same checkpoint/close sequence you already have
later, then compute currentDbPath and perform the File(...) copy to backupFile),
or alternatively replace the file-copy logic for creating backupFile with a call
to SQLite’s online backup API to produce a consistent snapshot; update
BackupRestoreViewModel so the code that uses currentDbPath and backupFile runs
only after the checkpoint/close (or after a successful SQLite backup call).


context.applicationContext.contentResolver.openInputStream(uri)?.use { raw ->
raw.zipInputStream().use { inputStream ->
var entry = tryOrNull { inputStream.nextEntry } // prevent ZipException
var entry = tryOrNull { inputStream.nextEntry }
var foundAny = false
while (entry != null) {
Timber.tag("RESTORE").i("Found zip entry: ${entry.name}")
Expand All @@ -119,21 +140,64 @@ class BackupRestoreViewModel @Inject constructor(
InternalDatabase.DB_NAME -> {
Timber.tag("RESTORE").i("Restoring DB (entry = ${entry.name})")
foundAny = true
// capture path before closing DB to avoid reopening race
val dbPath = database.openHelper.writableDatabase.path
runBlocking(Dispatchers.IO) { database.checkpoint() }
database.close()
Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath")
FileOutputStream(dbPath).use { outputStream ->
inputStream.copyTo(outputStream)
if (currentDbPath == null) {
Timber.tag("RESTORE").e("Database path is null, cannot restore")
} else {
runBlocking(Dispatchers.IO) { database.checkpoint() }
database.close()
Timber.tag("RESTORE").i("Overwriting DB at path: $currentDbPath")
FileOutputStream(currentDbPath).use { outputStream ->
inputStream.copyTo(outputStream)
}
Timber.tag("RESTORE").i("DB overwrite complete, triggering migrations")
try {
val migratedDb = InternalDatabase.newInternalDatabaseInstance(context, InternalDatabase.DB_NAME)
migratedDb.openHelper.writableDatabase
migratedDb.close()
Comment on lines +146 to +156
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🌐 Web query:

When restoring an SQLite database that uses WAL mode by replacing only the main .dbfile, should the existing-waland-shm sidecar files be removed before reopening the database?

💡 Result:

Yes—if you are restoring only by replacing the main “.db” file while leaving any existing “-wal” and “-shm” sidecar files in place, you should remove the old sidecars first (or otherwise ensure they do not exist) before reopening the database. Rationale: In WAL mode, SQLite uses the “-wal” file plus a shared-memory “-shm” wal-index/coordination file. If the existing sidecars do not match the restored “.db” state, the next opener will run WAL recovery to rebuild the wal-index from the WAL; if the WAL/index state is inconsistent with what you restored, you risk incorrect recovery or even failure. SQLite’s WAL design also notes that the “shm” content is not required for crash recovery and is normally truncated/unlinked when the database becomes quiescent, and that cleanup unlinks both “-wal” and “-shm” after a proper close. What “safe restore” usually looks like: 1) Restore/replace the main “.db”. 2) Delete the matching “-wal” and “-shm” files for that database. 3) Open the database normally; SQLite will initialize/recover the WAL-index as needed. Caveat: If some process still has the database open, deleting “-wal/-shm” can make the database become corrupted later; make sure the database is not in use during restore.

Citations:


🏁 Script executed:

cat -n app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt | sed -n '120,145p'

Repository: MetrolistGroup/Metrolist

Length of output: 2346


🏁 Script executed:

# Check if there's any cleanup of WAL/SHM files in this file
rg -A 5 -B 5 "delete|removeFile|wal|shm" app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt

Repository: MetrolistGroup/Metrolist

Length of output: 559


🏁 Script executed:

# Check InternalDatabase for WAL mode configuration
find . -name "InternalDatabase.kt" -type f | head -1 | xargs cat -n

Repository: MetrolistGroup/Metrolist

Length of output: 50


🏁 Script executed:

# Search for InternalDatabase more broadly
find . -name "*InternalDatabase*" -type f

Repository: MetrolistGroup/Metrolist

Length of output: 50


🏁 Script executed:

# Search for WAL configuration in the codebase
rg -i "wal|WAL" --type kt -l

Repository: MetrolistGroup/Metrolist

Length of output: 95


🏁 Script executed:

# Search for InternalDatabase class definition
rg "class InternalDatabase" --type-list | head -20

Repository: MetrolistGroup/Metrolist

Length of output: 458


🏁 Script executed:

# Use correct syntax for rg
rg "class InternalDatabase" -l

Repository: MetrolistGroup/Metrolist

Length of output: 128


🏁 Script executed:

# Broader search for database initialization or WAL
rg "InternalDatabase|\.database|RoomDatabase" | head -30

Repository: MetrolistGroup/Metrolist

Length of output: 3836


🏁 Script executed:

cat -n app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt | sed -n '1,100p'

Repository: MetrolistGroup/Metrolist

Length of output: 4193


🏁 Script executed:

cat -n app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt | sed -n '100,200p'

Repository: MetrolistGroup/Metrolist

Length of output: 5576


🏁 Script executed:

cat -n app/src/main/kotlin/com/metrolist/music/db/MusicDatabase.kt | sed -n '200,280p'

Repository: MetrolistGroup/Metrolist

Length of output: 4126


Remove the old WAL/SHM sidecars before overwriting the restored database file.

backup() checkpoints and archives only the main database file, but the restore overwrites only dbPath without removing stale song.db-wal and song.db-shm files from the current install. Since the database uses WAL mode, SQLite will attempt to recover from the old WAL on the next open, which can cause integrity failures or incorrect recovery against the restored base file. The codebase already deletes these files in testMigration() (lines 254–255); apply the same pattern here before the overwrite.

Suggested change
                                 runBlocking(Dispatchers.IO) { database.checkpoint() }
                                 database.close()
+                                java.io.File("$dbPath-wal").delete()
+                                java.io.File("$dbPath-shm").delete()
                                 Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath")
                                 FileOutputStream(dbPath).use { outputStream ->
                                     inputStream.copyTo(outputStream)
                                 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
runBlocking(Dispatchers.IO) { database.checkpoint() }
database.close()
Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath")
FileOutputStream(dbPath).use { outputStream ->
inputStream.copyTo(outputStream)
}
Timber.tag("RESTORE").i("DB overwrite complete, triggering migrations")
try {
val migratedDb = InternalDatabase.newInternalDatabaseInstance(context, InternalDatabase.DB_NAME)
migratedDb.openHelper.writableDatabase
migratedDb.close()
runBlocking(Dispatchers.IO) { database.checkpoint() }
database.close()
java.io.File("$dbPath-wal").delete()
java.io.File("$dbPath-shm").delete()
Timber.tag("RESTORE").i("Overwriting DB at path: $dbPath")
FileOutputStream(dbPath).use { outputStream ->
inputStream.copyTo(outputStream)
}
Timber.tag("RESTORE").i("DB overwrite complete, triggering migrations")
try {
val migratedDb = InternalDatabase.newInternalDatabaseInstance(context, InternalDatabase.DB_NAME)
migratedDb.openHelper.writableDatabase
migratedDb.close()
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 126 - 136, In BackupRestoreViewModel (the restore flow around the
code using dbPath and InternalDatabase.newInternalDatabaseInstance), delete any
existing WAL/SHM sidecar files for the database (dbPath + "-wal" and dbPath +
"-shm") before opening FileOutputStream(dbPath) to overwrite the main DB file;
follow the same pattern used in testMigration() (remove song.db-wal and
song.db-shm) so SQLite won't attempt WAL recovery against the old sidecars when
InternalDatabase (openHelper/writableDatabase) is opened later.

Timber.tag("RESTORE").i("Migrations completed successfully")
// Delete backup on success
backupDbFile?.delete()
val walBackup = File("${backupDbFile?.absolutePath}-wal")
if (walBackup.exists()) walBackup.delete()
val shmBackup = File("${backupDbFile?.absolutePath}-shm")
if (shmBackup.exists()) shmBackup.delete()
Comment on lines +159 to +163
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟡 Minor | ⚡ Quick win

Null-safe path construction for backup cleanup.

If backupDbFile is null, "${backupDbFile?.absolutePath}-wal" evaluates to "null-wal", creating File objects with unintended paths. While the exists() checks prevent actual harm, this is misleading.

Suggested fix
                                        // Delete backup on success
-                                       backupDbFile?.delete()
-                                       val walBackup = File("${backupDbFile?.absolutePath}-wal")
-                                       if (walBackup.exists()) walBackup.delete()
-                                       val shmBackup = File("${backupDbFile?.absolutePath}-shm")
-                                       if (shmBackup.exists()) shmBackup.delete()
+                                       backupDbFile?.let { backup ->
+                                           backup.delete()
+                                           File("${backup.absolutePath}-wal").delete()
+                                           File("${backup.absolutePath}-shm").delete()
+                                       }
                                        migrationSucceeded = true
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
backupDbFile?.delete()
val walBackup = File("${backupDbFile?.absolutePath}-wal")
if (walBackup.exists()) walBackup.delete()
val shmBackup = File("${backupDbFile?.absolutePath}-shm")
if (shmBackup.exists()) shmBackup.delete()
backupDbFile?.let { backup ->
backup.delete()
File("${backup.absolutePath}-wal").delete()
File("${backup.absolutePath}-shm").delete()
}
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 159 - 163, The code constructs wal/shm backup paths using string
interpolation on backupDbFile which can be null, producing
"null-wal"/"null-shm"; fix by guarding null before building those File
objects—e.g., only create and delete walBackup and shmBackup inside a null-check
or backupDbFile?.let { ... } block that uses the non-null reference
(backupDbFile or the lambda param) to build "${it.absolutePath}-wal" and
"${it.absolutePath}-shm" and then call exists() and delete() so no File is
created with a "null" path.

migrationSucceeded = true
} catch (e: Exception) {
Timber.tag("RESTORE").e(e, "Migration failed, restoring backup")
var backupRestored = false
try {
backupDbFile?.let { backup ->
File(currentDbPath).delete()
backup.copyTo(File(currentDbPath), overwrite = true)
backup.delete()
val walBackup = File("${backup.absolutePath}-wal")
if (walBackup.exists()) {
walBackup.copyTo(File("${currentDbPath}-wal"), overwrite = true)
walBackup.delete()
}
val shmBackup = File("${backup.absolutePath}-shm")
if (shmBackup.exists()) {
shmBackup.copyTo(File("${currentDbPath}-shm"), overwrite = true)
shmBackup.delete()
Comment on lines +169 to +181
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major | ⚡ Quick win

Clear the failed-migration sidecars before copying the backup back.

If the failed open created song.db-wal / song.db-shm and the saved backup did not include matching sidecars, those files survive this rollback and will be reopened against the restored base DB. Delete the current sidecars unconditionally before restoring the backup files.

Suggested change
                                             backupDbFile?.let { backup ->
                                                 File(currentDbPath).delete()
+                                                File("${currentDbPath}-wal").delete()
+                                                File("${currentDbPath}-shm").delete()
                                                 backup.copyTo(File(currentDbPath), overwrite = true)
                                                 backup.delete()
                                                 val walBackup = File("${backup.absolutePath}-wal")
                                                 if (walBackup.exists()) {
                                                     walBackup.copyTo(File("${currentDbPath}-wal"), overwrite = true)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@app/src/main/kotlin/com/metrolist/music/viewmodels/BackupRestoreViewModel.kt`
around lines 169 - 181, The rollback restores the main DB file but only
conditionally deletes/overwrites -wal/-shm sidecars, leaving stale sidecars from
a failed migration; update the restore logic in BackupRestoreViewModel (around
backupDbFile and currentDbPath usage) to unconditionally delete any existing
currentDbPath sidecars (File("${currentDbPath}-wal") and
File("${currentDbPath}-shm")) before copying the backup and its sidecars, then
proceed to copy backup, copy its -wal/-shm if present, and delete the backups as
currently done.

}
backupRestored = true
}
} catch (restoreEx: Exception) {
Timber.tag("RESTORE").e(restoreEx, "Failed to restore backup after migration failure")
}
if (!backupRestored) {
throw e
}
Timber.tag("RESTORE").i("Backup restored, migration not possible")
migrationSucceeded = false
}
}
Timber.tag("RESTORE").i("DB overwrite complete")
}
else -> {
Timber.tag("RESTORE").i("Skipping unexpected entry: ${entry.name}")
}
}
entry = tryOrNull { inputStream.nextEntry } // prevent ZipException
entry = tryOrNull { inputStream.nextEntry }
}
if (!foundAny) {
Timber.tag("RESTORE").w("No expected entries found in archive")
Expand All @@ -155,13 +219,18 @@ class BackupRestoreViewModel @Inject constructor(
}
}

context.stopService(Intent(context, MusicService::class.java))
context.filesDir.resolve(PERSISTENT_QUEUE_FILE).delete()
val intent = context.packageManager.getLaunchIntentForPackage(context.packageName)?.apply {
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
// Only restart if migration succeeded or no DB restore was attempted
if (migrationSucceeded != false) {
context.stopService(Intent(context, MusicService::class.java))
context.filesDir.resolve(PERSISTENT_QUEUE_FILE).delete()
val intent = context.packageManager.getLaunchIntentForPackage(context.packageName)?.apply {
addFlags(Intent.FLAG_ACTIVITY_NEW_TASK or Intent.FLAG_ACTIVITY_CLEAR_TASK)
}
context.startActivity(intent)
Runtime.getRuntime().exit(0)
} else {
Toast.makeText(context, R.string.restore_database_incompatible, Toast.LENGTH_LONG).show()
}
Comment thread
coderabbitai[bot] marked this conversation as resolved.
context.startActivity(intent)
Runtime.getRuntime().exit(0)
}.onFailure {
reportException(it)
Timber.tag("RESTORE").e(it, "Restore failed")
Expand Down
1 change: 1 addition & 0 deletions app/src/main/res/values/metrolist_strings.xml
Original file line number Diff line number Diff line change
Expand Up @@ -896,6 +896,7 @@
<string name="restore_confirm_message">This will restore your app data from the selected backup</string>
<string name="restore_account_warning">You will need to log in again after restoring. The following account will be signed out:</string>
<string name="restore">Restore</string>
<string name="restore_database_incompatible">Cannot restore backup: database version incompatible. Your current data has been preserved.</string>
<string name="checking_previous_account">Checking for previous account…</string>
<string name="no_account_found">No account found</string>

Expand Down
Loading