From 89aa11e405f84d1dc34a343e77a459cb2d4a60c2 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 2 Jun 2026 11:00:20 -0700 Subject: [PATCH 1/3] fix(assets): remove unused delete_content param from deleteAsset MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The delete_content query param on DELETE /api/assets/{id} was introduced in #12125 and had its default flipped to false in #12621. In practice no client sends it: the frontend issues a bare DELETE /assets/{id}, so every real caller already gets the default soft-delete (the reference is hidden, content preserved). The only thing that set delete_content=true was this repo's own test teardown. Remove the param from the route and the OpenAPI spec so the contract matches what clients actually use (and lines up with the cloud surface). The route now always soft-deletes. The underlying delete_asset_reference helper keeps its delete_content_if_orphan option, so orphan reclamation remains available internally for a future GC path — it's just no longer exposed on the public endpoint. Tests that used delete_content=true for hard cleanup now soft-delete; test_delete_upon_reference_count asserts content preservation instead of orphan removal. --- app/assets/api/routes.py | 10 +++------- tests-unit/assets_test/conftest.py | 4 ++-- tests-unit/assets_test/test_crud.py | 19 ++++++++++--------- tests-unit/assets_test/test_downloads.py | 2 +- tests-unit/assets_test/test_tags_api.py | 4 ++-- 5 files changed, 18 insertions(+), 21 deletions(-) diff --git a/app/assets/api/routes.py b/app/assets/api/routes.py index 544a614f2bd..7ef462f5cd0 100644 --- a/app/assets/api/routes.py +++ b/app/assets/api/routes.py @@ -533,18 +533,14 @@ async def update_asset_route(request: web.Request) -> web.Response: @_require_assets_feature_enabled async def delete_asset_route(request: web.Request) -> web.Response: reference_id = str(uuid.UUID(request.match_info["id"])) - delete_content_param = request.query.get("delete_content") - delete_content = ( - False - if delete_content_param is None - else delete_content_param.lower() not in {"0", "false", "no"} - ) try: + # Deleting an asset is a soft delete of the reference; the underlying + # content is preserved (it may be shared with other references). deleted = delete_asset_reference( reference_id=reference_id, owner_id=USER_MANAGER.get_request_user_id(request), - delete_content_if_orphan=delete_content, + delete_content_if_orphan=False, ) except Exception: logging.exception( diff --git a/tests-unit/assets_test/conftest.py b/tests-unit/assets_test/conftest.py index 9867b4e140a..6325b8e99da 100644 --- a/tests-unit/assets_test/conftest.py +++ b/tests-unit/assets_test/conftest.py @@ -212,7 +212,7 @@ def create(name: str, tags: list[str], meta: dict, data: bytes) -> dict: for aid in created: with contextlib.suppress(Exception): - http.delete(f"{api_base}/api/assets/{aid}?delete_content=true", timeout=30) + http.delete(f"{api_base}/api/assets/{aid}", timeout=30) @pytest.fixture @@ -260,4 +260,4 @@ def autoclean_unit_test_assets(http: requests.Session, api_base: str): break for aid in ids: with contextlib.suppress(Exception): - http.delete(f"{api_base}/api/assets/{aid}?delete_content=true", timeout=30) + http.delete(f"{api_base}/api/assets/{aid}", timeout=30) diff --git a/tests-unit/assets_test/test_crud.py b/tests-unit/assets_test/test_crud.py index fd2e9a0984c..b35768c79e2 100644 --- a/tests-unit/assets_test/test_crud.py +++ b/tests-unit/assets_test/test_crud.py @@ -45,8 +45,8 @@ def test_get_and_delete_asset(http: requests.Session, api_base: str, seeded_asse assert "user_metadata" in detail assert "filename" in detail["user_metadata"] - # DELETE (hard delete to also remove underlying asset and file) - rd = http.delete(f"{api_base}/api/assets/{aid}?delete_content=true", timeout=120) + # DELETE (soft delete; the reference is hidden, content is preserved) + rd = http.delete(f"{api_base}/api/assets/{aid}", timeout=120) assert rd.status_code == 204 # GET again -> 404 @@ -60,7 +60,7 @@ def test_soft_delete_hides_from_get(http: requests.Session, api_base: str, seede aid = seeded_asset["id"] asset_hash = seeded_asset["asset_hash"] - # Soft-delete (default, no delete_content param) + # Soft-delete (delete is always a soft delete) rd = http.delete(f"{api_base}/api/assets/{aid}", timeout=120) assert rd.status_code == 204 @@ -81,8 +81,7 @@ def test_soft_delete_hides_from_get(http: requests.Session, api_base: str, seede ids = [a["id"] for a in rl.json().get("assets", [])] assert aid not in ids - # Clean up: hard-delete the soft-deleted reference and orphaned asset - http.delete(f"{api_base}/api/assets/{aid}?delete_content=true", timeout=120) + # The reference is already soft-deleted; content is preserved by design. def test_delete_upon_reference_count( @@ -119,16 +118,18 @@ def test_delete_upon_reference_count( rh2 = http.head(f"{api_base}/api/assets/hash/{src_hash}", timeout=120) assert rh2.status_code == 200 # asset identity preserved (soft delete) - # Re-associate via from-hash, then hard-delete -> orphan content removed + # Re-associate via from-hash (reuses the preserved content), then + # soft-delete -> content is still preserved (delete is always soft). r3 = http.post(f"{api_base}/api/assets/from-hash", json=payload, timeout=120) assert r3.status_code == 201, r3.json() + assert r3.json()["created_new"] is False # content survived the soft deletes aid3 = r3.json()["id"] - rd3 = http.delete(f"{api_base}/api/assets/{aid3}?delete_content=true", timeout=120) + rd3 = http.delete(f"{api_base}/api/assets/{aid3}", timeout=120) assert rd3.status_code == 204 rh3 = http.head(f"{api_base}/api/assets/hash/{src_hash}", timeout=120) - assert rh3.status_code == 404 # orphan content removed + assert rh3.status_code == 200 # content preserved (soft delete) def test_update_asset_fields(http: requests.Session, api_base: str, seeded_asset: dict): @@ -249,7 +250,7 @@ def test_concurrent_delete_same_asset_info_single_204( # Hit the same endpoint N times in parallel. n_tests = 4 - url = f"{api_base}/api/assets/{aid}?delete_content=false" + url = f"{api_base}/api/assets/{aid}" def _do_delete(delete_url): with requests.Session() as s: diff --git a/tests-unit/assets_test/test_downloads.py b/tests-unit/assets_test/test_downloads.py index 672ba97284d..42c64a5fde7 100644 --- a/tests-unit/assets_test/test_downloads.py +++ b/tests-unit/assets_test/test_downloads.py @@ -117,7 +117,7 @@ def test_download_missing_file_returns_404( assert body["error"]["code"] == "FILE_NOT_FOUND" finally: # We created asset without the "unit-tests" tag(see `autoclean_unit_test_assets`), we need to clear it manually. - dr = http.delete(f"{api_base}/api/assets/{aid}?delete_content=true", timeout=120) + dr = http.delete(f"{api_base}/api/assets/{aid}", timeout=120) dr.content diff --git a/tests-unit/assets_test/test_tags_api.py b/tests-unit/assets_test/test_tags_api.py index 595bf29c66a..9729b7d03a6 100644 --- a/tests-unit/assets_test/test_tags_api.py +++ b/tests-unit/assets_test/test_tags_api.py @@ -69,8 +69,8 @@ def test_tags_empty_usage(http: requests.Session, api_base: str, asset_factory, used_names = [t["name"] for t in body2["tags"]] assert custom_tag in used_names - # Hard-delete the asset so the tag usage drops to zero - rd = http.delete(f"{api_base}/api/assets/{_asset['id']}?delete_content=true", timeout=120) + # Delete the asset reference so the tag usage drops to zero + rd = http.delete(f"{api_base}/api/assets/{_asset['id']}", timeout=120) assert rd.status_code == 204 # Now the custom tag must not be returned when include_zero=false From 795927b954c2ea7209e229408fc65ce79f4db5ae Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 2 Jun 2026 11:26:40 -0700 Subject: [PATCH 2/3] test/docs: address review on deleteAsset delete_content removal - Rename test_delete_upon_reference_count -> test_soft_delete_preserves_asset_identity_across_references; the old name implied last-ref cleanup, but it now verifies the opposite (soft delete preserves identity across references). - Strengthen the re-association assertion: also check asset_hash == src_hash so it proves content reuse rather than relying on the now-tautological created_new is False. - Document delete_asset_reference: the orphan-reclamation branch is intentionally internal-only; the public endpoint always soft-deletes. - Normalize the soft-delete comment phrasing. --- app/assets/services/asset_management.py | 10 ++++++++++ tests-unit/assets_test/test_crud.py | 16 +++++++++------- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/app/assets/services/asset_management.py b/app/assets/services/asset_management.py index 1072c95faae..d4e4fc61c4a 100644 --- a/app/assets/services/asset_management.py +++ b/app/assets/services/asset_management.py @@ -160,6 +160,16 @@ def delete_asset_reference( owner_id: str, delete_content_if_orphan: bool = True, ) -> bool: + """Delete an asset reference. + + With ``delete_content_if_orphan=False`` (a soft delete), the reference is + hidden and the underlying content is preserved. With ``True``, the content + is also removed once it becomes orphaned. + + Note: the public DELETE /api/assets/{id} endpoint always soft-deletes + (passes ``False``); the orphan-reclamation path is intentionally + internal-only, retained for a future GC/admin caller. + """ with create_session() as session: if not delete_content_if_orphan: # Soft delete: mark the reference as deleted but keep everything diff --git a/tests-unit/assets_test/test_crud.py b/tests-unit/assets_test/test_crud.py index b35768c79e2..36abb60eecb 100644 --- a/tests-unit/assets_test/test_crud.py +++ b/tests-unit/assets_test/test_crud.py @@ -45,7 +45,7 @@ def test_get_and_delete_asset(http: requests.Session, api_base: str, seeded_asse assert "user_metadata" in detail assert "filename" in detail["user_metadata"] - # DELETE (soft delete; the reference is hidden, content is preserved) + # Soft delete — the reference is hidden, content is preserved rd = http.delete(f"{api_base}/api/assets/{aid}", timeout=120) assert rd.status_code == 204 @@ -60,7 +60,7 @@ def test_soft_delete_hides_from_get(http: requests.Session, api_base: str, seede aid = seeded_asset["id"] asset_hash = seeded_asset["asset_hash"] - # Soft-delete (delete is always a soft delete) + # Soft delete — the reference is hidden, content is preserved rd = http.delete(f"{api_base}/api/assets/{aid}", timeout=120) assert rd.status_code == 204 @@ -81,10 +81,10 @@ def test_soft_delete_hides_from_get(http: requests.Session, api_base: str, seede ids = [a["id"] for a in rl.json().get("assets", [])] assert aid not in ids - # The reference is already soft-deleted; content is preserved by design. + # The reference is already soft-deleted; content is preserved. -def test_delete_upon_reference_count( +def test_soft_delete_preserves_asset_identity_across_references( http: requests.Session, api_base: str, seeded_asset: dict ): # Create a second reference to the same asset via from-hash @@ -118,11 +118,13 @@ def test_delete_upon_reference_count( rh2 = http.head(f"{api_base}/api/assets/hash/{src_hash}", timeout=120) assert rh2.status_code == 200 # asset identity preserved (soft delete) - # Re-associate via from-hash (reuses the preserved content), then - # soft-delete -> content is still preserved (delete is always soft). + # Re-associate via from-hash: it must reuse the same preserved content + # (created_new False AND the same hash), proving the soft deletes did not + # destroy the underlying asset. Then soft-delete again -> still preserved. r3 = http.post(f"{api_base}/api/assets/from-hash", json=payload, timeout=120) assert r3.status_code == 201, r3.json() - assert r3.json()["created_new"] is False # content survived the soft deletes + assert r3.json()["created_new"] is False + assert r3.json()["asset_hash"] == src_hash # reused the surviving content aid3 = r3.json()["id"] rd3 = http.delete(f"{api_base}/api/assets/{aid3}", timeout=120) From d600c76b46b307e318aa2e80f014139d381f8ba8 Mon Sep 17 00:00:00 2001 From: Matt Miller Date: Tue, 2 Jun 2026 11:52:55 -0700 Subject: [PATCH 3/3] test(assets): make seed content unique per test for isolation Removing the delete_content param means delete is always a soft delete, so content created by one test now survives into the next. The suite had been relying on hard-delete teardown for isolation, so shared fixed-content fixtures started colliding: seeded_asset (b"A"*4096) and make_asset_bytes (deterministic on name) produced the same hash every test, so the second seed deduped to the surviving asset and returned 200 instead of 201, cascading into ~14 failures/errors. Salt both fixtures with a per-test uuid so each test creates fresh content (created_new True, 201), while keeping content deterministic within a test (same name/size -> same bytes) and preserving exact byte length so size-based list/sort assertions are unaffected. --- tests-unit/assets_test/conftest.py | 17 +++++++++++++++-- 1 file changed, 15 insertions(+), 2 deletions(-) diff --git a/tests-unit/assets_test/conftest.py b/tests-unit/assets_test/conftest.py index 6325b8e99da..4aa20372f25 100644 --- a/tests-unit/assets_test/conftest.py +++ b/tests-unit/assets_test/conftest.py @@ -6,6 +6,7 @@ import sys import tempfile import time +import uuid from pathlib import Path from typing import Callable, Iterator, Optional @@ -188,9 +189,17 @@ def _post_multipart_asset( @pytest.fixture def make_asset_bytes() -> Callable[[str, int], bytes]: + # Salt content per test so it never collides with assets left over from + # earlier tests. Delete is now always a soft delete (content is preserved), + # so the suite can no longer rely on hard-deleting content for isolation. + # Deterministic within a test: the same (name, size) yields the same bytes. + salt = uuid.uuid4().bytes + def _make(name: str, size: int = 8192) -> bytes: seed = sum(ord(c) for c in name) % 251 - return bytes((i * 31 + seed) % 256 for i in range(size)) + body = bytearray((i * 31 + seed) % 256 for i in range(size)) + body[: len(salt)] = salt[:size] + return bytes(body) return _make @@ -227,7 +236,11 @@ def seeded_asset(request: pytest.FixtureRequest, http: requests.Session, api_bas if tags is None: tags = ["models", "checkpoints", "unit-tests", "alpha"] meta = {"purpose": "test", "epoch": 1, "flags": ["x", "y"], "nullable": None} - files = {"file": (name, b"A" * 4096, "application/octet-stream")} + # Unique content per test so the seed always creates a fresh asset (201). + # Delete is now always a soft delete, so content from a prior test survives + # and would otherwise dedup this upload into an existing asset (200). + content = uuid.uuid4().bytes + b"A" * (4096 - 16) + files = {"file": (name, content, "application/octet-stream")} form_data = { "tags": json.dumps(tags), "name": name,