diff --git a/src/did/did_matlab_python_bridge_implementations.yaml b/src/did/did_matlab_python_bridge_implementations.yaml index e4daa18..d3c84e7 100644 --- a/src/did/did_matlab_python_bridge_implementations.yaml +++ b/src/did/did_matlab_python_bridge_implementations.yaml @@ -175,9 +175,25 @@ classes: nothing is written, and the original error is raised; it is not retried. Two things found while porting: SQLite reports DBMOVED at a transaction's FIRST WRITE, not at COMMIT, so that is where recovery - usually happens; and a file swapped out after that first write is not - detected at all -- COMMIT "succeeds" into the unlinked file -- which no - retry in either language can see. + usually happens; and SQLite does not detect a file swapped out after + that first write at all -- COMMIT "succeeds" into the unlinked file and + the write is lost without an error. + + PYTHON-ONLY ADDITION (2026-10-24), closing that second gap. _open_db + records the (st_dev, st_ino) of the file it opened, and _commit, inside + a journaled write operation, checks that the path still names that + file before committing (_file_was_replaced; a deleted file counts as + replaced). If it does not, the handle is treated as poisoned and the + transaction recovered as above -- reopened, replayed against the file + now at the path, verified, committed -- instead of being committed to + a file nobody can reach. Only a window of microseconds between the + check and the COMMIT remains. It is the comparison SQLite's unix VFS + makes for DBMOVED, at the one moment SQLite does not; one stat per + commit (~1 us locally, against ~1.8 ms for the commit). A volume with + unstable inode numbers can trip it falsely, which costs one replay + onto the same file, since nothing has been committed yet. MATLAB has + the same gap, narrower because each mksqlite statement commits on its + own; this check is not in DID-matlab. do_run_sql_query, outside a write operation, keeps its own retry: only when no transaction is open, reopening first for READONLY; inside an unjournaled transaction the error is raised at once. diff --git a/src/did/implementations/sqlitedb.py b/src/did/implementations/sqlitedb.py index 0356aee..0122b31 100644 --- a/src/did/implementations/sqlitedb.py +++ b/src/did/implementations/sqlitedb.py @@ -224,6 +224,7 @@ def __init__(self, filename): self.dbid = None self._fields_cache = {} # (class, field_name) -> field_idx self._journal = None # statements of the current write operation + self._db_identity = None # (st_dev, st_ino) of the file opened self._open_db() # Create FileDir now, as DID-MATLAB's sqlitedb constructor does: @@ -254,6 +255,9 @@ def _open_db(self): if is_new: self._create_db_tables() + # Which file the connection holds, so a later COMMIT can tell whether + # the path still names it -- see _file_was_replaced. + self._db_identity = self._file_identity() # Always ensure the search-critical indexes exist — including on # databases created before this fix (idempotent). self._ensure_indexes() @@ -459,6 +463,11 @@ def _commit(self): what is re-run is the whole transaction, replayed and verified by :meth:`_recover_transaction`, and then committed. """ + if self._journal is not None and self._file_was_replaced(): + # Not on this handle: it would commit to the replaced file. + self._recover_transaction(self._replaced_file_error(), commit=True) + self._journal_committed() + return try: self.dbid.commit() except sqlite3.Error as err: @@ -494,6 +503,41 @@ def _commit(self): self._report_sql_error(last_err) raise last_err + def _file_identity(self): + """``(st_dev, st_ino)`` of the file at the path, or None if absent.""" + try: + info = os.stat(self.connection) + except OSError: + return None + return (info.st_dev, info.st_ino) + + def _file_was_replaced(self): + """Does the path no longer name the file this connection opened? + + SQLite checks this itself (SQLITE_READONLY_DBMOVED) only when a write + transaction starts. A file replaced after that -- by a sync client, a + restore, a copy-then-rename -- is not noticed, and COMMIT succeeds + into the old file, which no longer has a name: the write is lost + without an error. Checked just before COMMIT, it is caught while + nothing has been committed. It is the same comparison SQLite's unix + VFS makes (the path's inode against the open file's), costs one + stat, and a deleted file counts as replaced. A volume that reports + unstable inode numbers can trip it falsely; that costs one replay + onto the same file, since nothing was committed yet. + """ + if self._db_identity is None: + return False + return self._file_identity() != self._db_identity + + def _replaced_file_error(self): + # Worded as SQLite words DBMOVED, so it is classified the same way. + return sqlite3.OperationalError( + "attempt to write a readonly database (the database file at " + f"{self.connection!r} was replaced while it was open, so this " + "connection would have committed to a file that no longer has a " + "name)" + ) + def _rollback(self): """Roll back, if there is still a connection to roll back. @@ -653,6 +697,9 @@ def _recover_transaction(self, err, pending=None, commit=False): try: cursors = self._replay(journal) if commit: + if self._file_was_replaced(): + # Replaced again while we replayed: go round. + raise self._replaced_file_error() self.dbid.commit() except _ReplayDiverged as diverged: with contextlib.suppress(sqlite3.Error): diff --git a/tests/test_sqlitedb_transient_errors.py b/tests/test_sqlitedb_transient_errors.py index 6cbae46..5b74579 100644 --- a/tests/test_sqlitedb_transient_errors.py +++ b/tests/test_sqlitedb_transient_errors.py @@ -521,3 +521,101 @@ def capture(): with pytest.raises(sqlitedb_module._ReplayDiverged, match="failed on replay"): db._replay(journal) db.dbid.rollback() + + +class TestFileReplacedAfterTheFirstWrite: + """The swap SQLite does not see: after the transaction's first write. + + SQLite's DBMOVED check runs only when a write transaction starts. A + file replaced later is not noticed, and COMMIT succeeds into the old, + now nameless file -- the write is lost with no error. SQLiteDB checks + that the path still names its file just before COMMIT. + """ + + def _swap_during_add(self, monkeypatch, action): + # _populate_files runs after the docs and doc_data INSERTs, so the + # write transaction is already open when the file goes. + original = SQLiteDB._populate_files + + def populate_then_swap(self, *args, **kwargs): + action(self.connection) + return original(self, *args, **kwargs) + + monkeypatch.setattr(SQLiteDB, "_populate_files", populate_then_swap) + + def test_sqlite_alone_loses_the_write(self, tmp_path): + # The failure, shown on a bare connection: no error, no data. + path = str(tmp_path / "bare.sqlite") + conn = sqlite3.connect(path) + conn.execute("CREATE TABLE t (a)") + conn.commit() + conn.execute("INSERT INTO t VALUES (1)") + _move_file_under_handle(path) + conn.commit() # succeeds + conn.close() + with sqlite3.connect(path) as check: + assert check.execute("SELECT * FROM t").fetchall() == [] + + def test_a_replaced_file_is_caught_and_the_write_lands( + self, db, sleeps, monkeypatch + ): + db.add_branch("main") + self._swap_during_add(monkeypatch, _move_file_under_handle) + + db.add_docs([_doc("5" * 32)], branch_id="main", validate=False) + + assert sleeps == [0.1] + assert "5" * 32 in _doc_ids_on_disk(db.connection) + + def test_a_deleted_file_is_an_error_not_a_silent_loss( + self, db, sleeps, monkeypatch + ): + db.add_branch("main") + self._swap_during_add(monkeypatch, os.remove) + + with pytest.raises(sqlite3.OperationalError, match="was replaced"): + db.add_docs([_doc("6" * 32)], branch_id="main", validate=False) + # Reopening made a new, empty database at the path; the replay read + # no branch there and was refused rather than written. + assert "6" * 32 not in _doc_ids_on_disk(db.connection) + + def test_a_false_alarm_costs_one_replay_onto_the_same_file( + self, db, sleeps, monkeypatch + ): + # A volume with unstable inode numbers: the path seems to have moved + # once, but it is the same file. Nothing was committed before the + # check, so the replay lands the document exactly once. + db.add_branch("main") + real = SQLiteDB._file_identity + calls = {"n": 0} + + def flaky(self): + calls["n"] += 1 + if calls["n"] == 1: + return ("elsewhere", 0) + return real(self) + + monkeypatch.setattr(SQLiteDB, "_file_identity", flaky) + + db.add_docs([_doc("4" * 32)], branch_id="main", validate=False) + + assert sleeps == [0.1] + with sqlite3.connect(db.connection) as check: + count = check.execute( + "SELECT COUNT(*) FROM docs WHERE doc_id = ?", ("4" * 32,) + ).fetchone()[0] + assert count == 1 + + def test_an_unchanged_file_is_not_replayed(self, db, sleeps): + db.add_branch("main") + handle = db.dbid + db.add_docs([_doc("3" * 32)], branch_id="main", validate=False) + assert sleeps == [] + assert db.dbid is handle + + def test_the_identity_follows_a_reopen(self, db, sleeps): + db.add_branch("main") + _move_file_under_handle(db.connection) + db.add_branch("b2", parent_branch_id="main") # recovered by reopen + assert db._db_identity == db._file_identity() + assert not db._file_was_replaced()