From ef92de23af4dc87bdcae739c758245abe29ae36a Mon Sep 17 00:00:00 2001 From: Simon Pinfold Date: Wed, 26 Aug 2026 09:29:13 -0700 Subject: [PATCH] fix(assets): retire the live content row before re-registering a path in place (review2-3) --- app/assets/services/ingest.py | 25 ++++++ .../assets_test/services/test_upload_b.py | 90 +++++++++++++++++++ 2 files changed, 115 insertions(+) diff --git a/app/assets/services/ingest.py b/app/assets/services/ingest.py index 198f51d66..80405568d 100644 --- a/app/assets/services/ingest.py +++ b/app/assets/services/ingest.py @@ -314,6 +314,30 @@ def upload_from_temp_path( return _record_to_upload_result(session, record, created_new=True) +def _retire_stale_live_content( + session: Session, locator: str, new_hash: str | None +) -> None: + """Retire a live content row at ``locator`` whose bytes no longer match. + + ``create_content`` resolves a live-path uniqueness conflict + (``uq_asset_contents_path_live``) by returning the existing row, so + re-registering a path in place without first retiring the previous content + would hand the caller the OLD file's hash/size. When the on-disk bytes have + changed (hash differs) we mark the stale row missing so the fresh + ``create_content`` inserts a new live row; when the hash matches we leave the + row untouched and let ``create_content``'s dedup path reuse it (same-bytes + dedup is unchanged). + """ + existing = session.scalars( + select(AssetContent).where( + AssetContent.path == locator, + AssetContent.is_missing.is_(False), + ) + ).first() + if existing is not None and existing.hash != new_hash: + mark_content_missing(session, existing.id) + + def register_file_in_place( abs_path: str, name: str, @@ -367,6 +391,7 @@ def register_file_in_place( return _record_to_upload_result(session, record, created_new=True) with create_session() as session: + _retire_stale_live_content(session, locator, stored_hash) content = create_content( session, locator, stored_hash, size_bytes, mtime_ns ) diff --git a/tests-unit/assets_test/services/test_upload_b.py b/tests-unit/assets_test/services/test_upload_b.py index 52f36eb7a..4be2a1d4c 100644 --- a/tests-unit/assets_test/services/test_upload_b.py +++ b/tests-unit/assets_test/services/test_upload_b.py @@ -10,6 +10,7 @@ import app.assets.mode as mode_module import folder_paths from app.assets.database.models import Asset, AssetContent from app.assets.database.queries.records import create_content, mark_content_missing +from app.assets.helpers import to_stored_hash from app.assets.services.ingest import ( UploadUnstableError, register_file_in_place, @@ -189,6 +190,95 @@ def test_off_mode_register_file_in_place_different_bytes_two_paths( os.unlink(path) +def test_register_file_in_place_overwrite_returns_new_file_hash_and_size( + mock_create_session, hashing_on +): + """Given a path already registered, When it is re-registered in place with + new bytes, Then the result reports the NEW file's hash and size - not the + stale row that create_content's uniqueness-conflict path would return. + + The reported hash must equal the actual on-disk hash of the new file. + """ + output_dir = folder_paths.get_output_directory() + os.makedirs(output_dir, exist_ok=True) + path = os.path.join(output_dir, "overwrite_hash.png") + + v1 = b"overwrite-version-one" + v2 = b"v2" + try: + with open(path, "wb") as file: + file.write(v1) + r1 = register_file_in_place( + abs_path=path, name="overwrite_hash.png", tags=["output"] + ) + + with open(path, "wb") as file: + file.write(v2) + snapshot = snapshot_hash(path) + assert snapshot is not None + expected_new_hash = to_stored_hash(snapshot[0]) + + r2 = register_file_in_place( + abs_path=path, name="overwrite_hash.png", tags=["output"] + ) + + assert r2.asset.hash == expected_new_hash + assert r2.asset.hash != r1.asset.hash + assert r2.asset.size_bytes == len(v2) + finally: + if os.path.exists(path): + os.unlink(path) + + +def test_register_file_in_place_overwrite_marks_old_content_missing( + mock_create_session, hashing_on +): + """Given a path already registered, When it is re-registered in place with + new bytes, Then the previous content row is marked missing and exactly one + live row (carrying the new hash) remains at that path. + """ + output_dir = folder_paths.get_output_directory() + os.makedirs(output_dir, exist_ok=True) + path = os.path.join(output_dir, "overwrite_missing.png") + + v1 = b"overwrite-version-one" + v2 = b"v2" + try: + with open(path, "wb") as file: + file.write(v1) + r1 = register_file_in_place( + abs_path=path, name="overwrite_missing.png", tags=["output"] + ) + + with open(path, "wb") as file: + file.write(v2) + snapshot = snapshot_hash(path) + assert snapshot is not None + expected_new_hash = to_stored_hash(snapshot[0]) + + register_file_in_place( + abs_path=path, name="overwrite_missing.png", tags=["output"] + ) + + with mock_create_session() as session: + rows = list( + session.scalars( + select(AssetContent).where( + AssetContent.path == os.path.abspath(path) + ) + ) + ) + live = [row for row in rows if not row.is_missing] + missing = [row for row in rows if row.is_missing] + assert len(live) == 1 + assert live[0].hash == expected_new_hash + assert len(missing) == 1 + assert missing[0].hash == r1.asset.hash + finally: + if os.path.exists(path): + os.unlink(path) + + def test_upload_matching_missing_row_stores_bytes(mock_create_session, hashing_on): content_bytes = b"fresh-upload-bytes" temp = _write_temp(content_bytes)