From 298c5781846e451d548b0597ae716e146f25a32c Mon Sep 17 00:00:00 2001 From: Cursor Agent Date: Fri, 23 Oct 2026 11:13:27 +0000 Subject: [PATCH 1/3] fix(suppliers): do not cascade facility rows that arrive during removal Facility deletion counted lots and CTE events, then deleted the facility in a later statement. Those child rows are ON DELETE CASCADE, so a lot or event that committed in between was destroyed with the facility. Lock the facility on PostgreSQL and re-check linked rows in the DELETE itself so that statement removes nothing when a link exists. Co-authored-by: christophersellers --- .../admin/app/supplier_facilities_routes.py | 165 ++++++++++++------ .../test_supplier_facilities_routes_1342.py | 96 ++++++++++ 2 files changed, 204 insertions(+), 57 deletions(-) diff --git a/services/admin/app/supplier_facilities_routes.py b/services/admin/app/supplier_facilities_routes.py index 4783dcc9f..250f7f5c3 100644 --- a/services/admin/app/supplier_facilities_routes.py +++ b/services/admin/app/supplier_facilities_routes.py @@ -7,11 +7,11 @@ from __future__ import annotations import uuid as uuid_module -from typing import Any +from typing import Any, NoReturn from fastapi import APIRouter, Depends, HTTPException, Query from shared.pagination import PaginationParams -from sqlalchemy import delete, func, select +from sqlalchemy import delete, exists, func, select from sqlalchemy.orm import Session from .database import get_session @@ -134,6 +134,73 @@ async def create_supplier_facility( ) +_LINKED_RECORD_MODELS = ( + SupplierTraceabilityLotModel, + SupplierCTEEventModel, + SupplierFunnelEventModel, +) + + +def _linked_record_counts( + db: Session, + tenant_id: uuid_module.UUID, + facility_id: uuid_module.UUID, +) -> tuple[int, int, int]: + counts: list[int] = [] + for model in _LINKED_RECORD_MODELS: + counts.append( + db.execute( + select(func.count()) + .select_from(model) + .where(model.tenant_id == tenant_id, model.facility_id == facility_id) + ).scalar() + or 0 + ) + return counts[0], counts[1], counts[2] + + +def _no_linked_records(tenant_id: uuid_module.UUID, facility_id: uuid_module.UUID) -> tuple[Any, ...]: + """WHERE clauses that match only a facility with no linked rows.""" + return tuple( + ~exists( + select(model.id).where(model.tenant_id == tenant_id, model.facility_id == facility_id) + ) + for model in _LINKED_RECORD_MODELS + ) + + +def _load_facility_for_removal( + db: Session, + facility_id: uuid_module.UUID, + tenant_id: uuid_module.UUID, +) -> SupplierFacilityModel | None: + stmt = select(SupplierFacilityModel).where( + SupplierFacilityModel.id == facility_id, + SupplierFacilityModel.tenant_id == tenant_id, + ) + bind = db.get_bind() + if bind is not None and bind.dialect.name == "postgresql": + stmt = stmt.with_for_update() + return db.execute(stmt).scalar_one_or_none() + + +def _reject_linked_facility( + facility_name: str, + lots_count: int, + cte_events_count: int, + funnel_events_count: int, +) -> NoReturn: + raise HTTPException( + status_code=409, + detail=( + f"Cannot remove facility '{facility_name}' with linked records: " + f"{lots_count} lots/TLCs, {cte_events_count} CTE events, " + f"{funnel_events_count} other records. " + f"Delete only when zero linked rows." + ), + ) + + @router.get("/facilities/{facility_id}/removal-check", response_model=FacilityRemovalCheckResponse) async def check_supplier_facility_removal( facility_id: str, @@ -154,26 +221,9 @@ async def check_supplier_facility_removal( if facility is None or facility.tenant_id != tenant_id: raise HTTPException(status_code=404, detail="Facility not found") - lots_count = db.execute( - select(func.count()).select_from(SupplierTraceabilityLotModel).where( - SupplierTraceabilityLotModel.tenant_id == tenant_id, - SupplierTraceabilityLotModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - cte_events_count = db.execute( - select(func.count()).select_from(SupplierCTEEventModel).where( - SupplierCTEEventModel.tenant_id == tenant_id, - SupplierCTEEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - funnel_events_count = db.execute( - select(func.count()).select_from(SupplierFunnelEventModel).where( - SupplierFunnelEventModel.tenant_id == tenant_id, - SupplierFunnelEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 + lots_count, cte_events_count, funnel_events_count = _linked_record_counts( + db, tenant_id, facility_uuid + ) scoping_config_count = db.execute( select(func.count()).select_from(SupplierFacilityFTLCategoryModel).where( @@ -225,56 +275,57 @@ async def delete_supplier_facility( except ValueError as exc: raise HTTPException(status_code=400, detail="Invalid facility id") from exc - facility = db.get(SupplierFacilityModel, facility_uuid) - if facility is None or facility.tenant_id != tenant_id: + # Lock the facility on PostgreSQL before counting. A lot or CTE insert + # takes a key share lock on this row, so it cannot commit until this + # transaction ends. Lots and CTE events are ON DELETE CASCADE: a count + # in an earlier statement would miss a row that commits before the + # DELETE, and the cascade would destroy it. + facility = _load_facility_for_removal(db, facility_uuid, tenant_id) + if facility is None: raise HTTPException(status_code=404, detail="Facility not found") - lots_count = db.execute( - select(func.count()).select_from(SupplierTraceabilityLotModel).where( - SupplierTraceabilityLotModel.tenant_id == tenant_id, - SupplierTraceabilityLotModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - cte_events_count = db.execute( - select(func.count()).select_from(SupplierCTEEventModel).where( - SupplierCTEEventModel.tenant_id == tenant_id, - SupplierCTEEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - funnel_events_count = db.execute( - select(func.count()).select_from(SupplierFunnelEventModel).where( - SupplierFunnelEventModel.tenant_id == tenant_id, - SupplierFunnelEventModel.facility_id == facility_uuid, - ) - ).scalar() or 0 - - if lots_count > 0 or cte_events_count > 0 or funnel_events_count > 0: - raise HTTPException( - status_code=409, - detail=( - f"Cannot remove facility '{facility.name}' with linked records: " - f"{lots_count} lots/TLCs, {cte_events_count} CTE events, " - f"{funnel_events_count} other records. " - f"Delete only when zero linked rows." - ), + lots_count, cte_events_count, funnel_events_count = _linked_record_counts( + db, tenant_id, facility_uuid + ) + if lots_count or cte_events_count or funnel_events_count: + _reject_linked_facility(facility.name, lots_count, cte_events_count, funnel_events_count) + + facility_name = facility.name + # The DELETE re-checks linked rows in the same statement. A row that + # arrived after the count above makes this match nothing, so the + # cascade never runs. + result = db.execute( + delete(SupplierFacilityModel).where( + SupplierFacilityModel.id == facility_uuid, + SupplierFacilityModel.tenant_id == tenant_id, + *_no_linked_records(tenant_id, facility_uuid), + ), + execution_options={"synchronize_session": "fetch"}, + ) + if result.rowcount != 1: + db.rollback() + if _load_facility_for_removal(db, facility_uuid, tenant_id) is None: + raise HTTPException(status_code=404, detail="Facility not found") + lots_count, cte_events_count, funnel_events_count = _linked_record_counts( + db, tenant_id, facility_uuid ) + _reject_linked_facility(facility_name, lots_count, cte_events_count, funnel_events_count) + # FTL scoping is configuration, not traceability evidence. Postgres + # already cascades it with the facility; this covers databases that + # are not enforcing that foreign key. db.execute( delete(SupplierFacilityFTLCategoryModel).where( SupplierFacilityFTLCategoryModel.tenant_id == tenant_id, SupplierFacilityFTLCategoryModel.facility_id == facility_uuid, ) ) - - db.delete(facility) db.commit() return { "status": "deleted", "id": str(facility_uuid), - "facility_name": facility.name, + "facility_name": facility_name, } diff --git a/services/admin/tests/test_supplier_facilities_routes_1342.py b/services/admin/tests/test_supplier_facilities_routes_1342.py index 114ed8081..0337636dd 100644 --- a/services/admin/tests/test_supplier_facilities_routes_1342.py +++ b/services/admin/tests/test_supplier_facilities_routes_1342.py @@ -26,6 +26,7 @@ from __future__ import annotations +from datetime import datetime, timezone from unittest.mock import MagicMock, patch from uuid import UUID @@ -596,3 +597,98 @@ def _deny_owner(): resp = member_client.delete("/v1/supplier/facilities/00000000-0000-0000-0000-000000000000") assert resp.status_code == 403 + def test_delete_empty_facility(self, client: TestClient, db_session: Session): + facility = _make_facility(db_session) + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 200 + assert resp.json()["status"] == "deleted" + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is None + + def test_linked_lot_returns_409(self, client: TestClient, db_session: Session): + facility = _make_facility(db_session) + db_session.add( + SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=facility.id, + tlc_code="TLC-KEEP", + product_description="Spinach", + status="active", + ) + ) + db_session.commit() + + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is not None + + def test_stale_empty_count_does_not_cascade_lot(self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch): + """A lot committed after the pre-check must not be deleted with the facility. + + Lots use ON DELETE CASCADE. The pre-check and the DELETE used to be + separate statements, so a lot that landed in between was cascaded away. + """ + import app.supplier_facilities_routes as fac_routes + + facility = _make_facility(db_session) + lot = SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=facility.id, + tlc_code="TLC-RACE", + product_description="Spinach", + status="active", + ) + db_session.add(lot) + db_session.commit() + monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) + + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is not None + assert db_session.get(SupplierTraceabilityLotModel, lot.id) is not None + + def test_stale_empty_count_does_not_cascade_cte_event( + self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch + ): + """An event filed on this facility must survive even when its lot lives elsewhere.""" + import app.supplier_facilities_routes as fac_routes + + target = _make_facility(db_session, "Target") + other = _make_facility(db_session, "Other") + lot = SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=other.id, + tlc_code="TLC-OTHER", + product_description="Spinach", + status="active", + ) + db_session.add(lot) + db_session.commit() + event = SupplierCTEEventModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=target.id, + lot_id=lot.id, + cte_type="shipping", + event_time=datetime.now(timezone.utc), + kde_data={}, + payload_sha256="abc", + merkle_hash="def", + sequence_number=1, + obligation_ids=[], + ) + db_session.add(event) + db_session.commit() + monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) + + resp = client.delete(f"/v1/supplier/facilities/{target.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, target.id) is not None + assert db_session.get(SupplierCTEEventModel, event.id) is not None + From aa0e0688b7f972da5f0c9e00a27b24348ebe83e1 Mon Sep 17 00:00:00 2001 From: Christopher Sellers <1@christophersellers.com> Date: Sat, 24 Oct 2026 00:56:48 -0700 Subject: [PATCH 2/3] test(admin): drop delete-facility tests that used invented records Removes four tests (TLC-KEEP/RACE/OTHER fixtures) per real-data-only rule. Route fix unchanged. Signed-off-by: Christopher Sellers <1@christophersellers.com> --- .../test_supplier_facilities_routes_1342.py | 101 +----------------- 1 file changed, 1 insertion(+), 100 deletions(-) diff --git a/services/admin/tests/test_supplier_facilities_routes_1342.py b/services/admin/tests/test_supplier_facilities_routes_1342.py index 0337636dd..847c7bc6c 100644 --- a/services/admin/tests/test_supplier_facilities_routes_1342.py +++ b/services/admin/tests/test_supplier_facilities_routes_1342.py @@ -26,7 +26,6 @@ from __future__ import annotations -from datetime import datetime, timezone from unittest.mock import MagicMock, patch from uuid import UUID @@ -78,9 +77,7 @@ def db_session() -> Session: SM = sessionmaker(bind=engine, autoflush=False, autocommit=False, expire_on_commit=False, future=True) session = SM() session.add(TenantModel(id=TEST_TENANT_ID, name="T", slug="t", status="active", settings={})) - session.add( - UserModel(id=TEST_USER_ID, email="u@example.com", status="active", is_sysadmin=False) - ) + session.add(UserModel(id=TEST_USER_ID, email="u@example.com", status="active", is_sysadmin=False)) session.commit() try: @@ -596,99 +593,3 @@ def _deny_owner(): with TestClient(app) as member_client: resp = member_client.delete("/v1/supplier/facilities/00000000-0000-0000-0000-000000000000") assert resp.status_code == 403 - - def test_delete_empty_facility(self, client: TestClient, db_session: Session): - facility = _make_facility(db_session) - resp = client.delete(f"/v1/supplier/facilities/{facility.id}") - assert resp.status_code == 200 - assert resp.json()["status"] == "deleted" - db_session.expire_all() - assert db_session.get(SupplierFacilityModel, facility.id) is None - - def test_linked_lot_returns_409(self, client: TestClient, db_session: Session): - facility = _make_facility(db_session) - db_session.add( - SupplierTraceabilityLotModel( - tenant_id=TEST_TENANT_ID, - supplier_user_id=TEST_USER_ID, - facility_id=facility.id, - tlc_code="TLC-KEEP", - product_description="Spinach", - status="active", - ) - ) - db_session.commit() - - resp = client.delete(f"/v1/supplier/facilities/{facility.id}") - assert resp.status_code == 409 - db_session.expire_all() - assert db_session.get(SupplierFacilityModel, facility.id) is not None - - def test_stale_empty_count_does_not_cascade_lot(self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch): - """A lot committed after the pre-check must not be deleted with the facility. - - Lots use ON DELETE CASCADE. The pre-check and the DELETE used to be - separate statements, so a lot that landed in between was cascaded away. - """ - import app.supplier_facilities_routes as fac_routes - - facility = _make_facility(db_session) - lot = SupplierTraceabilityLotModel( - tenant_id=TEST_TENANT_ID, - supplier_user_id=TEST_USER_ID, - facility_id=facility.id, - tlc_code="TLC-RACE", - product_description="Spinach", - status="active", - ) - db_session.add(lot) - db_session.commit() - monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) - - resp = client.delete(f"/v1/supplier/facilities/{facility.id}") - assert resp.status_code == 409 - db_session.expire_all() - assert db_session.get(SupplierFacilityModel, facility.id) is not None - assert db_session.get(SupplierTraceabilityLotModel, lot.id) is not None - - def test_stale_empty_count_does_not_cascade_cte_event( - self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch - ): - """An event filed on this facility must survive even when its lot lives elsewhere.""" - import app.supplier_facilities_routes as fac_routes - - target = _make_facility(db_session, "Target") - other = _make_facility(db_session, "Other") - lot = SupplierTraceabilityLotModel( - tenant_id=TEST_TENANT_ID, - supplier_user_id=TEST_USER_ID, - facility_id=other.id, - tlc_code="TLC-OTHER", - product_description="Spinach", - status="active", - ) - db_session.add(lot) - db_session.commit() - event = SupplierCTEEventModel( - tenant_id=TEST_TENANT_ID, - supplier_user_id=TEST_USER_ID, - facility_id=target.id, - lot_id=lot.id, - cte_type="shipping", - event_time=datetime.now(timezone.utc), - kde_data={}, - payload_sha256="abc", - merkle_hash="def", - sequence_number=1, - obligation_ids=[], - ) - db_session.add(event) - db_session.commit() - monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) - - resp = client.delete(f"/v1/supplier/facilities/{target.id}") - assert resp.status_code == 409 - db_session.expire_all() - assert db_session.get(SupplierFacilityModel, target.id) is not None - assert db_session.get(SupplierCTEEventModel, event.id) is not None - From ef99d3f13d63f57de19526b477883172140f442e Mon Sep 17 00:00:00 2001 From: Christopher <131207452+PetrefiedThunder@users.noreply.github.com> Date: Sun, 25 Oct 2026 00:05:39 -0700 Subject: [PATCH 3/3] test(admin): restore delete and race tests for facility removal without invented values Restores the four TestDeleteSupplierFacility tests removed in aa0e0688. Lot/event rows now carry empty placeholder strings (no fabricated TLCs, products or hashes); only row existence and FK linkage matter. --- .../test_supplier_facilities_routes_1342.py | 103 +++++++++++++++++- 1 file changed, 101 insertions(+), 2 deletions(-) diff --git a/services/admin/tests/test_supplier_facilities_routes_1342.py b/services/admin/tests/test_supplier_facilities_routes_1342.py index 847c7bc6c..f43d76b3d 100644 --- a/services/admin/tests/test_supplier_facilities_routes_1342.py +++ b/services/admin/tests/test_supplier_facilities_routes_1342.py @@ -26,6 +26,7 @@ from __future__ import annotations +from datetime import datetime, timezone from unittest.mock import MagicMock, patch from uuid import UUID @@ -77,7 +78,9 @@ def db_session() -> Session: SM = sessionmaker(bind=engine, autoflush=False, autocommit=False, expire_on_commit=False, future=True) session = SM() session.add(TenantModel(id=TEST_TENANT_ID, name="T", slug="t", status="active", settings={})) - session.add(UserModel(id=TEST_USER_ID, email="u@example.com", status="active", is_sysadmin=False)) + session.add( + UserModel(id=TEST_USER_ID, email="u@example.com", status="active", is_sysadmin=False) + ) session.commit() try: @@ -517,7 +520,7 @@ def test_lots_with_facility_filter(self, client: TestClient, db_session: Session supplier_user_id=TEST_USER_ID, facility_id=f.id, tlc_code="TLC-FILTER-001", - product_description="Spinach", + product_description="", status="active", ) db_session.add(lot) @@ -593,3 +596,99 @@ def _deny_owner(): with TestClient(app) as member_client: resp = member_client.delete("/v1/supplier/facilities/00000000-0000-0000-0000-000000000000") assert resp.status_code == 403 + + def test_delete_empty_facility(self, client: TestClient, db_session: Session): + facility = _make_facility(db_session) + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 200 + assert resp.json()["status"] == "deleted" + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is None + + def test_linked_lot_returns_409(self, client: TestClient, db_session: Session): + facility = _make_facility(db_session) + db_session.add( + SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=facility.id, + tlc_code="", + product_description="", + status="active", + ) + ) + db_session.commit() + + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is not None + + def test_stale_empty_count_does_not_cascade_lot(self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch): + """A lot committed after the pre-check must not be deleted with the facility. + + Lots use ON DELETE CASCADE. The pre-check and the DELETE used to be + separate statements, so a lot that landed in between was cascaded away. + """ + import app.supplier_facilities_routes as fac_routes + + facility = _make_facility(db_session) + lot = SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=facility.id, + tlc_code="", + product_description="", + status="active", + ) + db_session.add(lot) + db_session.commit() + monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) + + resp = client.delete(f"/v1/supplier/facilities/{facility.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, facility.id) is not None + assert db_session.get(SupplierTraceabilityLotModel, lot.id) is not None + + def test_stale_empty_count_does_not_cascade_cte_event( + self, client: TestClient, db_session: Session, monkeypatch: pytest.MonkeyPatch + ): + """An event filed on this facility must survive even when its lot lives elsewhere.""" + import app.supplier_facilities_routes as fac_routes + + target = _make_facility(db_session, "Target") + other = _make_facility(db_session, "Other") + lot = SupplierTraceabilityLotModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=other.id, + tlc_code="", + product_description="", + status="active", + ) + db_session.add(lot) + db_session.commit() + event = SupplierCTEEventModel( + tenant_id=TEST_TENANT_ID, + supplier_user_id=TEST_USER_ID, + facility_id=target.id, + lot_id=lot.id, + cte_type="shipping", + event_time=datetime.now(timezone.utc), + kde_data={}, + payload_sha256="", + merkle_hash="", + sequence_number=1, + obligation_ids=[], + ) + db_session.add(event) + db_session.commit() + monkeypatch.setattr(fac_routes, "_linked_record_counts", lambda *_args, **_kwargs: (0, 0, 0)) + + resp = client.delete(f"/v1/supplier/facilities/{target.id}") + assert resp.status_code == 409 + db_session.expire_all() + assert db_session.get(SupplierFacilityModel, target.id) is not None + assert db_session.get(SupplierCTEEventModel, event.id) is not None +