Refactor database error handling to prevent deletion on transient errors and improve corruption detection. Add tests to ensure valid databases are not deleted on non-corruption DatabaseErrors.

Author: Dylan Huang

eval_protocol/cli_commands/logs.py

@@ -62,7 +62,6 @@ def _is_database_corruption_error(error: Exception) -> tuple[bool, str]:
     corruption_indicators = [
         "file is not a database",
         "database disk image is malformed",
-        "database is locked",
         "unable to open database file",
     ]
 

eval_protocol/event_bus/sqlite_event_bus_database.py

@@ -66,19 +66,20 @@ def check_and_repair_database(db_path: str, auto_repair: bool = False) -> bool:
 
     except DatabaseError as e:
         error_str = str(e).lower()
-        if "file is not a database" in error_str or "database disk image is malformed" in error_str:
+        # Only treat specific SQLite corruption errors as corruption
+        corruption_indicators = [
+            "file is not a database",
+            "database disk image is malformed",
+            "file is encrypted or is not a database",
+        ]
+        if any(indicator in error_str for indicator in corruption_indicators):
             logger.warning(f"Database file is corrupted: {db_path}")
             if auto_repair:
                 _backup_and_remove_database(db_path)
                 return True
             raise DatabaseCorruptedError(db_path, e)
+        # For other DatabaseErrors (locks, busy, etc.), re-raise without deleting
         raise
-    except Exception as e:
-        logger.warning(f"Error checking database {db_path}: {e}")
-        if auto_repair:
-            _backup_and_remove_database(db_path)
-            return True
-        raise DatabaseCorruptedError(db_path, e)
 
 
 def _backup_and_remove_database(db_path: str) -> None:

tests/test_sqlite_hardening.py

@@ -194,6 +194,63 @@ def test_corrupted_file_backup_created(self):
             assert len(backup_files) == 1
             assert "corrupted" in backup_files[0]
 
+    def test_transient_errors_do_not_delete_database(self):
+        """Test that transient I/O errors (like PermissionError) don't trigger database deletion.
+
+        This is a regression test for a bug where the catch-all Exception handler
+        would delete valid databases on transient errors like PermissionError,
+        OSError, or temporary lock conflicts.
+        """
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = os.path.join(tmpdir, "valid.db")
+
+            # Create a valid database
+            conn = sqlite3.connect(db_path)
+            conn.execute("CREATE TABLE test (id INTEGER PRIMARY KEY, data TEXT)")
+            conn.execute("INSERT INTO test VALUES (1, 'important data')")
+            conn.commit()
+            conn.close()
+
+            # Verify the database is valid first
+            result = check_and_repair_database(db_path)
+            assert result is True
+            assert os.path.exists(db_path)
+
+            # The database should still exist and be valid
+            conn = sqlite3.connect(db_path)
+            cursor = conn.execute("SELECT data FROM test WHERE id=1")
+            row = cursor.fetchone()
+            conn.close()
+            assert row[0] == "important data"
+
+    def test_database_error_without_corruption_indicator_is_not_auto_repaired(self):
+        """Test that DatabaseError without corruption indicators is re-raised, not auto-repaired."""
+        from unittest.mock import patch, MagicMock
+        from peewee import DatabaseError
+
+        with tempfile.TemporaryDirectory() as tmpdir:
+            db_path = os.path.join(tmpdir, "locked.db")
+
+            # Create a valid database first
+            conn = sqlite3.connect(db_path)
+            conn.execute("CREATE TABLE test (id INTEGER)")
+            conn.close()
+
+            # Mock SqliteDatabase to raise a non-corruption DatabaseError (e.g., database locked)
+            with patch("eval_protocol.event_bus.sqlite_event_bus_database.SqliteDatabase") as mock_db_class:
+                mock_db = MagicMock()
+                mock_db_class.return_value = mock_db
+                mock_db.connect.side_effect = DatabaseError("database is locked")
+
+                # Should re-raise the error, not delete the database
+                with pytest.raises(DatabaseError) as exc_info:
+                    check_and_repair_database(db_path, auto_repair=True)
+
+                assert "locked" in str(exc_info.value)
+
+            # Database file should still exist (not deleted)
+            assert os.path.exists(db_path)
+
 
 class TestBackupAndRemoveDatabase:
     """Test the backup and remove database functionality."""