diff --git a/src/did/did_matlab_python_bridge.yaml b/src/did/did_matlab_python_bridge.yaml index d7d29d2..16bda24 100644 --- a/src/did/did_matlab_python_bridge.yaml +++ b/src/did/did_matlab_python_bridge.yaml @@ -40,7 +40,7 @@ classes: - name: database type: class matlab_path: "+did/database.m" - matlab_last_sync_hash: "8bea92c" + matlab_last_sync_hash: "bb39399" status: ported python_path: "did/database.py" python_class: "Database" @@ -933,6 +933,13 @@ classes: connection is managed differently (see the sqlitedb open_db/close_db note). Synchronized 2026-03-15; validation 2026-08-28. + DID-matlab#227 (bb39399, 2026-10-24) changed add_docs(..., 'Validate', false) + to set journal_mode=MEMORY instead of OFF, since each document is now + one transaction and ROLLBACK is undefined with the journal OFF. + Nothing to port: Python's add_docs has no such fast path and never + changes journal_mode. The rest of that commit is in sqlitedb.m; see + the sqlitedb entry. + # ----------------------------------------------------------------------- # did.document # ----------------------------------------------------------------------- diff --git a/src/did/did_matlab_python_bridge_implementations.yaml b/src/did/did_matlab_python_bridge_implementations.yaml index eec508d..5c613c6 100644 --- a/src/did/did_matlab_python_bridge_implementations.yaml +++ b/src/did/did_matlab_python_bridge_implementations.yaml @@ -29,7 +29,7 @@ classes: - name: sqlitedb type: class matlab_path: "+did/+implementations/sqlitedb.m" - matlab_last_sync_hash: "da83fa3" + matlab_last_sync_hash: "bb39399" status: ported python_path: "did/implementations/sqlitedb.py" python_class: "SQLiteDB" @@ -194,6 +194,27 @@ classes: 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. + + DID-matlab#227 (bb39399, 2026-10-24) brought MATLAB to the same design: each + write operation is one transaction (beginWrite/commitWrite, BEGIN + IMMEDIATE), journaled and replayed after a reopen (recoverTransaction, + replayJournal), with the same pre-commit replaced-file check + (fileWasReplaced, unix dev:inode via Java). Synced here: two things it + does that Python did not. (1) delete_original sources are deleted only + once the operation commits (_delete_after_commit, flushed by + _journal_committed), so a rollback no longer leaves the only copy of a + file unreferenced in FileDir; reclaimed cached copies already waited + for the commit. A source two files share is deleted once, and a + missing one is not a warning, as MATLAB suppresses + MATLAB:DELETE:FileNotFound. (2) _recover_transaction does not reopen a + database file that no longer exists -- that created a new, empty + database there -- and raises the original error with a + FileNotFoundError as its cause (MATLAB: DID:SQLITEDB:DatabaseFileGone). + Remaining differences: MATLAB begins with BEGIN IMMEDIATE, Python's + sqlite3 with a deferred BEGIN at the first write; MATLAB checks + changes() after each write where Python compares cursor.rowcount; and + MATLAB restores fields_cache in abandonWrite where Python restores + _fields_cache in each operation's except path. Same effect each time. 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 8e17c80..f4f4101 100644 --- a/src/did/implementations/sqlitedb.py +++ b/src/did/implementations/sqlitedb.py @@ -125,10 +125,14 @@ def wrapper(self, *args, **kwargs): if self._journal is not None: return method(self, *args, **kwargs) self._journal = [] + self._pending_deletes = [] try: return method(self, *args, **kwargs) finally: + # Whatever was not committed keeps its files (see + # _delete_after_commit). self._journal = None + self._pending_deletes = None return wrapper @@ -227,6 +231,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._pending_deletes = None # originals to delete once it commits self._db_identity = None # (st_dev, st_ino) of the file opened self._open_db() @@ -563,6 +568,38 @@ def _journal_committed(self): for entry in self._journal: entry["proxy"]._entry = None self._journal.clear() + pending, self._pending_deletes = self._pending_deletes or [], [] + for path, what in pending: + self._delete_original(path, what) + + def _delete_after_commit(self, path, what): + """Delete an ingested original once the write operation commits. + + Inside a write operation the delete is queued and done by + _journal_committed, so a rollback leaves the only copy of the file + where it was. Outside one it happens at once, as before. Mirrors + DID-matlab's sqlitedb.deleteAfterCommit. + """ + if self._pending_deletes is None: + self._delete_original(path, what) + else: + self._pending_deletes.append((path, what)) + + @staticmethod + def _delete_original(path, what): + try: + os.remove(path) + except FileNotFoundError: + # Already gone (two files can share one original): the point is + # that it must not be there afterwards. MATLAB's commitWrite + # suppresses MATLAB:DELETE:FileNotFound for the same case. + pass + except OSError as error: + warnings.warn( + f'Ingested {what} but could not delete the original "{path}": ' + f"{error}", + stacklevel=3, + ) def _record(self, proxy, method, sql, params): entry = { @@ -691,6 +728,18 @@ def _recover_transaction(self, err, pending=None, commit=False): with contextlib.suppress(sqlite3.Error): self.dbid.rollback() time.sleep(backoff) + if not os.path.exists(self.connection): + # Reopening would create a new, empty database at the path + # and replay into it. The database is gone; say so. Mirrors + # DID-matlab's DID:SQLITEDB:DatabaseFileGone. + print( + f"DID:SQLITEDB the database file {self.connection} no " + "longer exists; not reopening it, which would create a " + "new, empty database there. Nothing from this operation " + "was written.", + file=sys.stderr, + ) + raise err from FileNotFoundError(self.connection) # Suspended while reopening: _open_db commits on its own. self._journal = None try: @@ -1310,14 +1359,7 @@ def _ingest_location(self, filename, entry, custom_file_handler, document_id="") # Only now, with a copy safely in place. MATLAB deletes the original # solely on the success branch, for the same reason. if entry.get("delete_original") and not is_remote: - try: - os.remove(source_path) - except OSError as error: - warnings.warn( - f'Ingested "{filename}" but could not delete the original ' - f'"{source_path}": {error}', - stacklevel=2, - ) + self._delete_after_commit(source_path, f'"{filename}"') return dest_path def _populate_files(self, cursor, doc_idx, document_obj, custom_file_handler=None): @@ -2253,14 +2295,7 @@ def _ingest_series_members(self, document_obj, custom_file_handler): continue if member.get("delete_original") and not is_remote: - try: - os.remove(source_path) - except OSError as error: - warnings.warn( - f"Ingested series member but could not delete " - f'the original "{source_path}": {error}', - stacklevel=2, - ) + self._delete_after_commit(source_path, "series member") def _open_series_member(self, series_locations, custom_file_handler): """Open a file series member: local first, then fetch through handler. diff --git a/tests/test_open_doc_locations.py b/tests/test_open_doc_locations.py index 34905f1..6e87792 100644 --- a/tests/test_open_doc_locations.py +++ b/tests/test_open_doc_locations.py @@ -17,7 +17,6 @@ import os import tempfile import unittest -import warnings from unittest import mock from did.database import FileAccessError @@ -349,20 +348,15 @@ def _matlab_shaped_doc(self, content=b"ingested payload"): This used to be hand-built. Since DID-python#45 add_docs produces the shape itself: a local location marked `ingest` is copied to /files/ and the original is deleted, as MATLAB's - do_add_doc does. Both files here point at the SAME original, so the - first is ingested and the second -- reached after the original is - gone -- ends with no ingested copy and no original, which is the case - test_a_file_with_no_ingested_copy_and_no_original_is_an_error needs. + do_add_doc does. Both files here point at the SAME original. The + original is deleted only once the document commits, so both are + ingested, each under its own uid. """ original = self._local_file("original.bin", content) doc = Document("demoFile", **{"demoFile.value": 1}) doc.add_file("filename1.ext", original) doc.add_file("filename2.ext", original) - with warnings.catch_warnings(): - # filename2's ingest cannot succeed -- filename1's delete_original - # already removed the source. MATLAB warns there too. - warnings.simplefilter("ignore") - self.db.add_docs([doc]) + self.db.add_docs([doc]) # The files table should carry a row per file, and filename1's copy # should be sitting under its uid. @@ -400,7 +394,18 @@ def test_ingested_copy_is_found_after_the_original_is_deleted(self): def test_a_file_with_no_ingested_copy_and_no_original_is_an_error(self): doc, _ = self._matlab_shaped_doc() - # filename2.ext shares the deleted original but was never ingested. + # filename2.ext shares the deleted original; take its ingested copy + # away too, so there is neither. + row = ( + self.db.dbid.cursor() + .execute( + "SELECT f.cached_location FROM docs d, files f " + "WHERE d.doc_idx = f.doc_idx AND d.doc_id = ? AND f.filename = ?", + (doc.id(), "filename2.ext"), + ) + .fetchone() + ) + os.remove(row["cached_location"]) with self.assertRaises(FileAccessError) as caught: self.db.open_doc(doc.id(), "filename2.ext") self.assertEqual(caught.exception.identifier, "DID:SQLITEDB:open") diff --git a/tests/test_sqlitedb_transient_errors.py b/tests/test_sqlitedb_transient_errors.py index 5b74579..048e604 100644 --- a/tests/test_sqlitedb_transient_errors.py +++ b/tests/test_sqlitedb_transient_errors.py @@ -573,11 +573,12 @@ def test_a_deleted_file_is_an_error_not_a_silent_loss( db.add_branch("main") self._swap_during_add(monkeypatch, os.remove) - with pytest.raises(sqlite3.OperationalError, match="was replaced"): + with pytest.raises(sqlite3.OperationalError, match="was replaced") as caught: 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) + assert isinstance(caught.value.__cause__, FileNotFoundError) + # Not reopened: that would have created a new, empty database at the + # path and replayed into it. + assert not os.path.exists(db.connection) def test_a_false_alarm_costs_one_replay_onto_the_same_file( self, db, sleeps, monkeypatch @@ -619,3 +620,51 @@ def test_the_identity_follows_a_reopen(self, db, sleeps): db.add_branch("b2", parent_branch_id="main") # recovered by reopen assert db._db_identity == db._file_identity() assert not db._file_was_replaced() + + +class TestOriginalsAreDeletedAfterCommit: + """delete_original waits for the commit, so a rollback keeps the file.""" + + def _file_doc(self, db): + source = os.path.join(os.path.dirname(db.connection), "source.bin") + with open(source, "wb") as handle: + handle.write(b"payload") + doc = Document("demoFile", **{"demoFile.value": 1}) + doc.add_file("filename1.ext", "placeholder", ingest=0, delete_original=0) + doc.add_file("filename2.ext", "placeholder", ingest=0, delete_original=0) + _, info, _ = doc.is_in_file_list("filename1.ext") + info["locations"] = [ + { + "location": source, + "location_type": "file", + "uid": "u-local", + "ingest": 1, + "delete_original": 1, + "parameters": "", + } + ] + return doc, source + + def test_a_rolled_back_add_keeps_the_original(self, db, monkeypatch): + db.add_branch("main") + doc, source = self._file_doc(db) + + def refuse(self): + raise sqlite3.OperationalError("disk I/O error") + + monkeypatch.setattr(SQLiteDB, "_commit", refuse) + with pytest.raises(sqlite3.OperationalError, match="disk I/O"): + db.add_docs([doc], branch_id="main", validate=False) + assert os.path.exists(source), "a rollback must not delete the original" + assert doc.id() not in _doc_ids_on_disk(db.connection) + + monkeypatch.undo() + db.add_docs([doc], branch_id="main", validate=False) + assert not os.path.exists(source), "deleted once the add commits" + assert doc.id() in _doc_ids_on_disk(db.connection) + + def test_nothing_is_left_queued_after_an_operation(self, db): + db.add_branch("main") + doc, _ = self._file_doc(db) + db.add_docs([doc], branch_id="main", validate=False) + assert db._pending_deletes is None