diff --git a/hindsight-api-slim/hindsight_api/engine/memory_engine.py b/hindsight-api-slim/hindsight_api/engine/memory_engine.py index 6212877f3..1c56449f4 100644 --- a/hindsight-api-slim/hindsight_api/engine/memory_engine.py +++ b/hindsight-api-slim/hindsight_api/engine/memory_engine.py @@ -11755,6 +11755,8 @@ async def _submit_async_operation( **task_payload, } + from hindsight_api.extensions.operation_validator import OperationValidationError + async with acquire_with_retry(backend) as conn: async with conn.transaction(): if dedupe_by_bank: @@ -11776,10 +11778,20 @@ async def _submit_async_operation( # serialize) but not with FOR KEY SHARE (so those inserts proceed). # On Oracle this rewrites to FOR UPDATE, which there does not block # indexed-FK child inserts. - await conn.execute( + # + # Use fetchval so we can also verify the bank actually exists. + # Without this check, callers that race against bank deletion + # or that derive bank IDs before creating the bank reach the + # INSERT below and get an asyncpg.ForeignKeyViolationError, which + # surfaces as an opaque 500 from the API. A clean + # OperationValidationError(404) is the right shape — the FastAPI + # handler already converts it via its existing except clause. + bank_exists = await conn.fetchval( f"SELECT 1 FROM {fq_table('banks')} WHERE bank_id = $1 FOR NO KEY UPDATE", bank_id, ) + if bank_exists is None: + raise OperationValidationError(f"Bank '{bank_id}' not found", status_code=404) # Only check 'pending', not 'processing': a processing task uses a # watermark from when it started, so memories added after that need # a fresh run regardless. @@ -11809,6 +11821,16 @@ async def _submit_async_operation( "operation_id": str(row["operation_id"]), "deduplicated": True, } + else: + # Scoped/non-dedupe submits skip the lock + dedup above. + # Still verify the bank exists so an FK violation can't + # escape as a 500. + bank_exists = await conn.fetchval( + f"SELECT 1 FROM {fq_table('banks')} WHERE bank_id = $1", + bank_id, + ) + if bank_exists is None: + raise OperationValidationError(f"Bank '{bank_id}' not found", status_code=404) await conn.execute( f""" diff --git a/hindsight-api-slim/tests/test_async_submit_bank_not_found.py b/hindsight-api-slim/tests/test_async_submit_bank_not_found.py new file mode 100644 index 000000000..4213e3d9f --- /dev/null +++ b/hindsight-api-slim/tests/test_async_submit_bank_not_found.py @@ -0,0 +1,90 @@ +"""Regression test: submitting an async op for a bank that doesn't exist must +raise a clean validation error, not a raw asyncpg `ForeignKeyViolationError`. + +`_submit_async_operation` inserts into `async_operations`, which has an FK to +`banks.bank_id`. If a caller submits for a missing bank (typo, race against a +deletion, integration that derives bank IDs before the bank is created), the +INSERT raises `asyncpg.exceptions.ForeignKeyViolationError`. The FastAPI +endpoint's broad `except Exception` then surfaces it as a 500 — but this is +a client error, not a server error, and should be a 404. + +This test exercises the call directly via `MemoryEngine.submit_async_*` so +the failure mode is observable without spinning up the HTTP layer. +""" + +import uuid + +import pytest + +from hindsight_api.extensions.operation_validator import OperationValidationError + +pytestmark = pytest.mark.xdist_group("async_submit_bank_not_found_tests") + + +@pytest.fixture +def no_inline_execution(memory): + """Prevent SyncTaskBackend from running the submitted op inline so we + only test the submit-path failure, not downstream execution.""" + + async def _noop(_payload): + return None + + original = memory._task_backend.submit_task + memory._task_backend.submit_task = _noop + yield + memory._task_backend.submit_task = original + + +@pytest.mark.asyncio +async def test_consolidation_submit_on_missing_bank_raises_validation_error( + memory, request_context, no_inline_execution +): + """A `/consolidate` submit against a bank that doesn't exist must raise + OperationValidationError(404), not a raw asyncpg FK violation that bubbles + out as a 500 from the API.""" + missing_bank = f"does-not-exist-{uuid.uuid4().hex[:8]}" + + with pytest.raises(OperationValidationError) as exc_info: + await memory.submit_async_consolidation( + bank_id=missing_bank, + request_context=request_context, + ) + + assert exc_info.value.status_code == 404 + assert missing_bank in exc_info.value.reason + + +@pytest.mark.asyncio +async def test_scoped_consolidation_submit_on_missing_bank_raises_validation_error( + memory, request_context, no_inline_execution +): + """Scoped consolidates (with `observation_scopes`) take the + `dedupe_by_bank=False` branch, which historically skipped the bank lock + entirely and went straight to the FK-violating INSERT. Same 404 contract.""" + missing_bank = f"does-not-exist-{uuid.uuid4().hex[:8]}" + + with pytest.raises(OperationValidationError) as exc_info: + await memory.submit_async_consolidation( + bank_id=missing_bank, + request_context=request_context, + observation_scopes=[{"tag": "anything"}], + ) + + assert exc_info.value.status_code == 404 + assert missing_bank in exc_info.value.reason + + +@pytest.mark.asyncio +async def test_graph_maintenance_on_missing_bank_short_circuits(memory, request_context, no_inline_execution): + """`submit_async_graph_maintenance` has its own short-circuit that checks + the per-bank queue before calling `_submit_async_operation`. A missing + bank means an empty queue, so it returns `no_work=True` without reaching + the FK-violating INSERT. This test pins that behaviour.""" + missing_bank = f"does-not-exist-{uuid.uuid4().hex[:8]}" + + result = await memory.submit_async_graph_maintenance( + bank_id=missing_bank, + request_context=request_context, + ) + + assert result == {"operation_id": None, "no_work": True}