fix(governance): remove the entity rows mined out of a dropped memory

[Eldad-Caura]

Closes audit finding H-02.

#808 named this case when it fixed the inline path: "entities mined out of
dropped content are the same leak in another table". It fixed that path by
ordering — _enrich_memory_background runs remediation first, and its early
return on a drop skips the entity extraction scheduled below it.

Both non-inline paths schedule extraction independently, at write time, as a
fire-and-forget task that races the verdict. ScheduleBackgroundTasks fires it
alongside the enrichment carrying run_governance_remediation=True in the fast
branch, and at write time in strong+deferred. process_entity_extraction never
re-checked the row.

And the schema's own expression of "these rows must not outlive the memory"
never fires: memory_entity_links.memory_id is ON DELETE CASCADE and
relations.evidence_memory_id is ON DELETE SET NULL, both on a HARD delete.
Governance soft-deletes — it sets deleted_at — so neither ever runs. The entity
row itself has no FK to the memory at all, so nothing would remove it even on a
hard delete.

Result: a tenant configured to drop had the memory removed and audited while the
names mined from it (person names, under a PII policy) stayed listable
tenant-wide through /entities and /graph, with nothing tying them to the drop.

Verified from the code rather than reproduced as one failing assertion, and the
distinction is worth being straight about: unlike the earlier findings in this
series there was no existing code path to make fail, because nothing could reach
these rows at all. What IS probe-confirmed is each guard added here — reverting
the candidate bounding fails the over-deletion test with 2 == 1, and neutering
the liveness check fails the dropped-row test.

Two halves, and they are not alternatives.

1. A purge on the drop path. New storage call, one transaction: delete the
   memory's entity links, then relations whose evidence IS this memory (one row
   carries one evidence id, so a relation attributed to dropped content has no
   other justification), then — from the entities this memory linked to and only
   those — the ones now left with no links and no relations. Both destructive
   dispositions cascade, not just the non-business one the finding described:
   they are separate branches reading separate configs, and a PII drop policy
   leaked identically.

   The candidate set is bounded on purpose. A first draft deleted every entity in
   the tenant with no links, which would sweep entities orphaned for unrelated
   reasons and race an entity a concurrent write had created but not yet linked.
   Under-deleting is recoverable; over-deleting another caller's rows is not. A
   test pins it: the unbounded version fails with 2 == 1.

2. A liveness re-check in the worker, immediately before persisting, reading the
   WRITER — the whole point is to observe a delete that just committed, and a
   replica under lag would report the row live exactly when the check most needed
   to fail.

Half 1 covers the common ordering: extraction is one LLM call while the verdict
needs enrichment plus an event round-trip, so extraction usually finishes first
and its rows are there to purge. Half 2 covers the tail where it does not — the
purge has already run by then and would miss what lands afterwards. Neither half
covers the other's case.

The purge is deliberately NOT gated on a marker, unlike H-10's child cascade: any
dropped memory may have been extracted from, no flag on the row says so, and the
purge is three targeted deletes keyed on memory_id.

The purge runs AFTER the soft-delete, and that ordering is pinned. Purging first
would destroy graph rows for a memory that is still live if the delete then
failed, and nothing would put them back.

Tests. Five in core-storage-api against real Postgres, because the query's
correctness is entirely about what it does and does not reach and a stub would
assert the code calls itself — including the over-deletion guard, the
still-asserted-by-another-memory case, and the tenant boundary. Three in
core-api for the wiring, one of them an over-refusal guard that flag and
keep_private purge nothing (those rows describe content that is still there and
still allowed). Two for the worker guard, asserting on the WRITES rather than
the early return so a refactor that keeps the check and persists anyway fails,
plus one that the check reads the writer.

Four existing entity-extraction test files gained a get_memory stub. That is the
honest cost of the worker now depending on a storage read it did not before, not
churn to hide a problem.

Overlaps #1292 (H-10), which is in review and adds its own cascade to the same
two drop branches. Whichever lands second needs a mechanical rebase; the two
mechanisms are independent — one covers rows derived into the memories table,
this one covers rows derived into the graph.

Verification

• Full root suite: 6077 passed, 5 skipped, 1 xfailed, 0 failed.
• core-storage-api/tests/: 343 passed, on its own scratch database.
• ruff check and ruff format --check run separately at CI's exact scopes — clean.
• mypy clean on both packages (core-api keeps its 2 pre-existing types-python-dateutil errors in an untouched file). The three new rowcount ignores match memory_soft_delete_by_ids' existing pattern.
• ratchet No new lines. · sentinel All 35 protected strings survive. · tenant-scope gate exit 0 · broker OpenAPI baseline current. All after git add.
• Checked for open PRs on this subsystem before starting; only fix(governance): cascade a drop or keep_private to rows derived before the verdict #1292, noted above.
• Branched from origin/main.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude please review at head 9550adfc. This deletes rows across three tables, so attack it rather than approve it.

What I most want challenged:

1. The orphan deletion. Entities are removed only when they were linked to THIS memory and are left with no links and no relations. I wrote an unbounded version first — every entity in the tenant with no links — and caught it myself; a test now pins the bounded behaviour. Please check the bounded version for the opposite failure: is there a shape where an entity that SHOULD be removed survives, e.g. one reachable only through a relation whose other endpoint is unrelated?

2. Relations. I delete relations whose evidence_memory_id is this memory, on the grounds that the column is singular so a relation has exactly one evidence row. Verify that — if a relation can be justified by more than one memory anywhere in the code, deleting on a single evidence match destroys a still-supported edge.

3. Concurrency. The purge is one transaction, but extraction for a DIFFERENT memory could be resolving the same entity concurrently. Can that interleave into deleting an entity another memory has just linked, or into a FK violation?

4. The two halves. I argue the worker liveness check and the purge cover different interleavings and neither subsumes the other. If the purge alone is sufficient, the check is a storage round-trip on every extraction that I should drop.

5. Four existing test files gained a get_memory stub. Confirm that is the honest consequence of the worker's new dependency and not a case of me changing tests to fit a change that should not have been made.

Note this overlaps #1292 (H-10), which adds a different cascade to the same two drop branches and is in review. Whichever lands second needs a mechanical rebase.

[Eldad-Caura]

Both correct. Fixed at 82d5dc43.

The High, and why my own reasoning was wrong

I wrote that failures propagate here "matching every other unapplied-policy path in this module". That is wrong about the caller, and you traced why: the other paths run under _enrich_memory_background, where a raise becomes a BackgroundTaskLog row. This one also runs under consumer.handle_memory_enriched, which has no guard, and the dispatcher nacks on a handler exception. So a raise redelivers the same event, re-runs the whole drop branch, and emits a second critical=True audit for a memory already dropped. Repeatedly.

I verified the invariant rather than taking it on trust — handle_memory_enriched calls remediation unguarded at consumer.py:107, and the dispatcher's ack/nack is at pubsub.py:1137.

The purge failure is now caught and logged at ERROR naming the memory. The trade is bounded and worth stating: the memory is already gone, so the content is not live — what remains is graph rows, and the log is enough to purge them by hand. Probe: letting it propagate again fails the new test with the raw RuntimeError.

The Low

Taken. The client reserves idempotent=True for endpoints that dedup replays storage-side; this one qualifies for a different reason worth writing down — a replay finds the rows already gone and deletes nothing more. That also means transient 5xx never reach the catch above.

One cost I recorded rather than hid: a lost response followed by a successful retry logs zero counts for a purge that did remove rows. A wrong number in an INFO line, against leaving forbidden content live.

This same defect is in #1292, and I am fixing it there next

#1292's cascade raises GovernanceCascadeError out of the same unguarded handler, so it has the identical nack-and-redeliver problem. Its own review round flagged the return-contract half of this and I answered that a duplicate parent audit was "the cost of not silently leaving forbidden content live" — your finding shows that is not a one-off cost but an unbounded redelivery loop. I was wrong there too, and for the same reason.

Flagging it here because the two PRs touch the same branches and the fix has to be consistent between them.

Verification

Suite 6092 passed / 0 failed on the rebased tree; storage suite 343; ruff, mypy, ratchet, sentinel, tenant-scope and the broker baseline all clean after git add.

[Eldad-Caura]

@claude re-review at head 507f40b4.

Your High is fixed, and the fix itself has not been reviewed.

Your last review ran against 9550adfc. Since then:

1. 82d5dc43 — the nack fix. You were right and my reasoning was wrong about the caller, not the mechanism. I wrote that purge failures propagate "matching every other unapplied-policy path in this module". The other paths run under _enrich_memory_background, where a raise becomes a BackgroundTaskLog row. This one also runs under consumer.handle_memory_enriched, which has no guard, and the dispatcher nacks on a handler exception — so a raise redelivered the event, re-ran the drop branch, and emitted a second critical=True audit for an already-dropped memory, repeatedly.

   The purge failure is now caught at the source in _purge_entity_artifacts and logged at ERROR naming the memory. Note this is deliberately not the shape I used for the same defect class in fix(governance): cascade a drop or keep_private to rows derived before the verdict #1292, where the catch sits in the consumer: there, _enrich_memory_background must still abort because it is about to create more derived rows. Here, a purge failure needs to abort nobody — the soft-delete has already committed, the content is gone, and what remains is graph rows plus a log line good enough to purge them by hand. A test pins it: letting the failure propagate again fails with the raw RuntimeError.

2. The branch was force-rebased onto current main (76def6a1). Patch-id unchanged (a1e2794051c7de7b3f0ab9f86f9a7499f9661098 before and after), so a pure rebase — but my local green predates the new base and CI on this head has not settled.

What I want attacked.

The catch site. Swallowing here is a real trade, not an obvious win: is there a caller or a failure mode where a silently-failed purge should have aborted something downstream? I convinced myself there is not because the purge runs last and nothing reads its result — falsify that if you can.

The over-deletion bound, again. It is the part I would least like to be wrong about, because under-deleting is recoverable and deleting another caller's rows is not. The candidate set is bounded to entities this memory linked to, then filtered to those with no remaining links and no remaining relations. My first draft deleted every unlinked entity in the tenant. Is the bounded version still reachable by a concurrent write — an entity created and linked between the candidate SELECT and the DELETE inside the same transaction?

One thing I will say plainly rather than let the test count imply otherwise: unlike the other findings in this series there was no pre-existing code path to make fail, because nothing could reach these rows at all. What is probe-confirmed is each guard I added — reverting the candidate bounding fails the over-deletion test with 2 == 1, and neutering the liveness check fails the dropped-row test.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude re-review at head 5abc12d2. Your High is correct and the fix is in.

You were right, and the tell was a comment I wrote three lines below the bug: "Tenant-scoped like everything else here." The entity delete was. The two statements above it were not. That is the same defect class this backlog keeps turning up — prose asserting a property the code does not have — and it has now landed in my own writing on both PRs in this pair.

What was wrong. memory_entity_links has no tenant_id column, so memory_id alone identifies a row without authorising anything. This file already knows that: _link_within_tenant sits in the method immediately above mine, _owned_link_endpoints guards the write side, and both exist because of GHSA-wgvw-28pq-jc36. I used neither. A caller naming a memory its tenant does not own deleted the owning tenant's link rows and received a count saying how many.

Reachability, stated honestly rather than talked up. The live caller passes tenant and memory from the same row, so this is not currently exploitable through governance. It is the invariant breaking ahead of anything exploiting it, on a storage endpoint whose entire job is to be called with caller-supplied ids. I would not have argued it down to Medium on that basis.

One deliberate divergence, which I want you to attack specifically. Both statements are now scoped through Memory — the memory end only, not _link_within_tenant. That helper requires both ends because a READ returning a straddling row hands back the other tenant's UUID. Deleting asks a different question: the row references a memory we own and are dropping, so a foreign entity on the far end is a reason to keep the entity (the tenant-scoped entity delete already does) and never a reason to keep a link pointing at dropped content. Requiring both ends would strand exactly the historical straddling rows the write path has refused to create since #1085/#1124 — the ones most likely to be real. If you think that reasoning is wrong, this is the place to say so.

Tests. The mismatched-pairing one is probe-confirmed: without the scoping it fails with links: 1 where 0 is required. The straddling-link one I will not oversell — it passes with or without the original bug. It exists to pin the choice above, and fails if someone later "fixes" this by reaching for _link_within_tenant.

You were also right that test_does_not_cross_tenants could not have caught this: it uses matched tenant/memory pairs throughout, so the unscoped delete only ever touched rows the caller did own.

Verified at this head: core-storage-api 353 passed, core-api 6092 passed, ruff check and format clean at CI's scopes, mypy clean on core-storage-api/src/, ratchet "No new lines.", sentinel "All 35 protected strings survive.", tenant-scope gate exit 0.

Note the branch was force-rebased three times today by its author while I worked; I confirmed by patch-id each time that content was unchanged (a1e2794051c7de7b3f0ab9f86f9a7499f9661098 throughout) before building on it.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude re-review at head 0bc8a5c6. Your Low is taken, but not as suggested — the suggested form deletes rows it shouldn't, and I have the failing test.

You were right that the three anti-join subqueries were unscoped and that correctness was never at risk from that. Narrowing them is worth doing on a path a drop-configured tenant runs constantly.

Where I diverged. The proposal scopes the two relation subqueries on Relation.tenant_id. That filter drops a historical straddling relation — one living in another tenant while naming an entity here — out of the "still referenced?" set. The entity is then deleted while something still points at it.

I wrote the test before believing myself, and it fails under the suggested version with entities: 1 where 0 is required.

What I did instead. All three subqueries are narrowed by the entity's tenant — joined to Entity, filtered on Entity.tenant_id — including the two relation ones. Same reduction in scan; cannot lose a reference. Every row capable of naming a candidate names an entity in this tenant, because that is the definition of a candidate. And erring wide is free here: a surplus reference only keeps an entity alive, which is the direction this file already learned to prefer in round 1, when my first draft over-deleted.

still_linked I took essentially as you wrote it — the Entity join was already the right shape there, and I applied that same shape to the relations rather than reaching for the column just because it exists.

Verified at this head: core-storage-api 354 passed (353 + the new straddling-relation test), ruff check and format clean, mypy clean on core-storage-api/src/, ratchet "No new lines.", sentinel "All 35 protected strings survive.", tenant-scope gate exit 0.

Worth noting for the record that both fixes on this PR — round 3's tenant scoping and round 4's narrowing — turned on the same distinction: which end of a link or relation you key on. Keyed on the memory, deleting is safe and reading leaks. Keyed on the entity, reading is safe and deleting over-reaches. The two halves of this function key on opposite ends deliberately, and each now says so where it does it.

🤖 Generated with Claude Code

[Eldad-Caura]

@erni-a please approve

[Eldad-Caura]

@claude re-review at head 5855c6de.

Your Medium was right, and it found the hole in my own reasoning rather than in my code.

Rebased onto main now that #1292 has merged — four files conflicted, all additive collisions between the two cascades. One resolution was a real decision rather than a mechanical merge, flagged below.

The Medium: the check ran too early to close the window it targeted

You're correct, and the tell was my own comment: "This narrows the window rather than closing it… the cascade in governance_remediation is what covers that ordering." The cascade covers extraction finishing before the verdict. It does nothing for the case you described — a drop landing during the writes, whose purge runs while these rows don't exist yet, after which the entities land and nothing ever revisits them, because the memory is gone and no later verdict names it.

So the fix had a blind spot shaped exactly like the leak it was closing.

Closed by re-checking after the writes and purging what was just written, which I picked over your option (a) because moving the check earlier still leaves a race — it just makes it smaller. The post-write version is an argument about what's observable rather than about timing:

• drop committed before our writes → its purge found nothing, our re-check sees the row deleted, we purge
• drop commits after our writes → its own purge sees our rows and takes them
• drop commits between → whichever purge runs later sees the rows; both key on the same memory_id

No ordering survives. I considered your option (c), enforcing it at the storage layer, and it is the better long-term shape — but it changes the contract of link and relation upserts for every caller, which is not something to slip into this PR.

Failures log rather than raise: this runs after the links are written, so raising would abort the subject write-back and cross-link discovery below over a cleanup concern.

The early check stays, downgraded in the comments to what it honestly is — an optimisation that avoids the work when the row is already gone.

Two tests. The drop-during-writes one is probe-confirmed: removing the post-write call fails it. The second is an over-refusal guard on the ordinary live-row path, and I want to be explicit about why it exists rather than folding it into the first — a post-write purge that fired on a live row would delete the graph rows of every successfully extracted memory in the install. The failure mode of this fix is considerably worse than the leak it closes, so it gets its own test.

The Low

Fixed — the route now returns 422 on a malformed memory_id, matching every sibling in that file. You were right that a 500 was the wrong thing for a caller that lets failures propagate out of a remediation to hear.

The merge decision worth reviewing

Both drop branches now run the entity purge and #1292's child cascade, and the purge goes first. _drop_children raises once any child fails, so ordering the cascade first would skip this parent's own graph rows on exactly the runs where something had already gone wrong. The purge can't raise, so it never blocks the cascade in return.

One thing I noticed during the merge and deliberately did not fix here. #1292's cascade soft-deletes child memories, and those children can have graph rows of their own — the cascade doesn't purge them. That is this same leak, one level down, created by the interaction of the two PRs rather than by either alone. It needs its own tests around the child path and I'd rather it be reviewed as its own change than appended to a PR already five rounds in. Filing it.

Verified at this head: core-api full suite green, core-storage-api 358 passed, ruff check and format clean at CI's scopes, mypy clean on core-storage-api/src/ and clean on core-api/src/ apart from the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py, ratchet "No new lines.", sentinel "All 35 protected strings survive.", tenant-scope gate exit 0.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude re-review at head 1e1e4213.

Your High is a defect in the previous round's own fix, and you found the thing that hid it.

_purge_written_artifacts_if_dropped detected the drop, purged, returned None — and then process_entity_extraction carried straight on: relation upserts carrying evidence_memory_id, subject write-back, contradiction detection, cross-link discovery. It cleaned the link table and immediately refilled the relation table. The leak moved rather than closed, and I shipped that as the fix for the leak.

The sharper half of your finding is the fixture, not the code: every test in that file leaves graph.relations empty, so the relation loop had nothing to iterate and none of them could have caught it. That's the second time on this PR that a green suite was green because the test data avoided the path.

Fixed. The helper returns bool; the caller returns on True. A test with a non-empty graph.relations pins it — restoring the fall-through fails it on upsert_relation having been awaited. A second test pins the other direction, that a live row still gets its relations and cross-links, because a short-circuit that fired unconditionally would silently stop writing them for every extracted memory in the install.

I also split the two failure modes while in there, since they're different states:

• liveness read fails → indeterminate → True. Carrying on would write graph rows for a row that may already be dropped; stopping costs relations for a row that's probably live, which the next content update rebuilds. Refuse to write what can't be justified.
• purge fails → True. A purge that didn't run doesn't make the memory live again, so continuing would be strictly worse than the failure.
• only an affirmatively live row returns False.

The Medium, with a correction to its premise

Taken — each cascaded child is now purged after its delete succeeds, never counted as a cascade failure (the helper logs and swallows, matching the parent).

But the reasoning in the finding is broader than what the code does, and I checked before writing it. Auto-chunk children go through sc.create_memories directly and get no extraction of their own. The parent is what gets extracted, over the full document — so the names mined from chunked content hang off the parent, and its purge already reached them. A child acquires graph rows only when something later rewrites its content, because update_memory re-extracts.

So this closes a narrow real case and keeps the invariant true of the cascade whatever populates children in future. It is not the broad "children leak PII entities" the wording implies, and I'd rather say so than let the fix look bigger than it is. I'd raised this myself last round in the broad form; verifying it is what corrected me.

Three tests: the cascade purges each child; a child whose delete failed is not purged (still live, so its graph rows describe content no policy removed); keep_private purges nothing. Probe-confirmed — removing the call fails the first with a dropped row kept its graph rows: {'m1'}.

Rebased onto the newer main under it; patch-id verified unchanged (2a7f20a6…) before building on it.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude re-review at head dfab6c5e.

Taken — and the suggested form of it would have left a live memory losing its relations. I have the failing test.

You're right that the invariant belongs where the deleting happens. Both callers check that the memory is dropped, so this wasn't reachable; but the method deletes across three tables and can't be undone, and "only purge what governance actually dropped" shouldn't live only in the callers' heads. A stale call, a reordering, or a future caller written from the method name alone would have wiped a live memory's entity graph.

Where I diverged. The proposal was to add deleted_at IS NOT NULL to the owned subquery. That subquery gates the link statements only — the relation delete keys on Relation.tenant_id and evidence_memory_id and never took it. So under that version, purging a live memory leaves its links and entities intact and deletes its relation: partial destruction, which is harder to diagnose than either clean outcome and just as unrecoverable.

I wrote the test before trusting that reading, and under your exact suggestion it fails with relations: 1 where 0 is required.

What I did instead: one guard, before anything is deleted — a row with this id, in this tenant, with deleted_at not null — and an early return with zero counts otherwise. That covers all three deletes rather than the two the subquery reached, and it reads as the authorisation check it is instead of a predicate threaded through three statements.

Two things about the tests worth flagging rather than burying.

The storage tests were creating live memories and purging them — a state no caller produces. They now soft-delete the target first via a _dropped_memory helper; the rows that must survive (the second asserting memory, the other tenant's) stay live deliberately.

And test_a_tenant_that_does_not_own_the_memory_deletes_nothing had to be updated too, for a reason worth naming: left with a live memory it would have kept passing for the wrong reason — the new liveness guard rather than the tenant check it exists to pin. Same failure mode as the empty graph.relations you caught last round, so I went looking for it this time.

Verified at this head: core-storage-api 359 passed, core-api full suite green, ruff check and format clean, mypy clean on core-storage-api/src/, ratchet "No new lines.", sentinel "All 35 protected strings survive.", tenant-scope gate exit 0.

🤖 Generated with Claude Code

[github-actions]

🤖 Review by Claude Code

Summary

This PR closes the H-02 gap where entities/links/relations mined from a memory could survive a governance soft-delete (since FK cascades only fire on hard delete). The storage-side purge (memory_purge_entity_artifacts) is well-guarded (tenant-scoped, requires the parent row to actually be soft-deleted, bounds the entity sweep to candidates linked from the target memory) and is backed by solid real-Postgres tests. The governance-side call sites (parent drop + child cascade) look correctly ordered. There is one real gap in the entity-extraction-worker fix: the post-write liveness re-check is placed too early, so it doesn't cover everything it claims to close.

Medium/Low Issues

Post-write liveness re-check doesn't cover relations/subject write-back/cross-link discovery

Severity: Medium
File: core-api/src/core_api/services/entity_extraction_worker.py:94-176 (helper) and core-api/src/core_api/services/entity_extraction_worker.py:506-522 (call site)
Problem: _purge_written_artifacts_if_dropped is invoked immediately after the entity-link bulk upsert, but the relation upserts (carrying evidence_memory_id), the subject write-back, and cross-link discovery all run after that check returns False, so a governance drop landing in that later window still leaves relation rows (and a subject pointer) for content the policy dropped — contradicting the docstring's claim that "there is no ordering left in which the rows survive."

🤖 Claude Code Prompt

In core-api/src/core_api/services/entity_extraction_worker.py, the call to
`_purge_written_artifacts_if_dropped(sc, memory_id, tenant_id)` is placed right
after `link_result = await sc.bulk_upsert_entity_links(...)` inside the
`if link_items:` block (around the diff hunk that starts
"@@ -402,6 +506,17 @@"). If that check finds the memory still live, execution
falls through to write relations (`upsert_relation` per `graph.relations`), the
subject write-back (`sc.set_subject_entity_if_null`), the audit log,
contradiction-detection triggering, and cross-link discovery — all of which
create additional graph rows for `memory_id` after the only liveness check in
this code path has already passed.

If governance's drop (and its own one-shot `purge_entity_artifacts` call in
governance_remediation.py) lands in the window between this check and the
relation/subject-write-back writes, those newly written rows are never
purged: governance's purge already ran and found nothing (the rows didn't
exist yet), and no further liveness check happens in
process_entity_extraction after this point.

Fix by moving the liveness re-check (or adding a second one) so it runs after
ALL graph-mutating writes for this memory (relations, subject write-back)
have completed — i.e., immediately before the audit-log/contradiction/cross-link
section, or by wrapping the whole "relations onward" section with its own
purge-and-stop check using `_purge_written_artifacts_if_dropped`. Update the
docstring's claim ("there is no ordering left in which the rows survive") to
match whichever window is actually closed, and add a test (mirroring
`test_a_drop_landing_during_the_writes_purges_what_was_just_written` in
tests/test_entity_linking_wiring.py) where the drop is detected only after
the relation write, to confirm the relation row also gets purged.

---

Reviewed by claude-sonnet-5 · cost $2.262992

[Eldad-Caura]

@claude re-review at head 2c07fbc5.

Right again, and it lands on the paragraph that existed to justify the mechanism.

Round 6 fixed the case where the check returns True — stop instead of purge-and-fall-through. It did nothing for the case where it returns False. The row is live at that moment, execution proceeds, and the subject write-back, the relation upserts carrying evidence_memory_id, and whatever cross-link discovery creates are all written after the only liveness check on that path. A drop landing across that stretch runs its own purge against rows that don't exist yet, and nothing revisits them.

And the docstring said "there is no ordering left in which the rows survive." True of rows written before the call; false of everything after it. Same overclaim as last round, one round later, in the paragraph whose whole job was to be precise about this. That's three times on this PR that my prose has asserted a property the code didn't have — it is reliably the weakest thing I produce here, and it is why I'd rather these rounds keep happening than stop early.

Fixed by calling it twice. The first call keeps its early-exit role: it saves the relation upserts and a cross-link discovery pass in the common case where the row was already gone. The second sits after every graph-mutating write, so the rows it can find are all of them — that's the one that actually closes the window. The docstring now says which call does what, rather than claiming the property for the mechanism as a whole.

I did not take the "move it later" half of your suggestion, because the early exit is worth keeping — cross-link discovery is a storage round-trip with a vector search, and skipping it for a row that's already gone is free.

One thing deliberately left uncovered, stated rather than assumed: contradiction detection. It's spawned via track_task so it can write after any check here, but it writes conflict rows rather than graph rows, and it re-checks deleted_at itself for exactly this race. Out of scope for H-02 rather than overlooked.

The test drives a drop that only becomes visible after the relation write — live at the pre-write check, live at the post-link check, dropped at the final one — and asserts both that the relation was written (that's the premise) and that the purge ran. Removing the final call fails it.

Verified at this head: core-api full suite green, core-storage-api 359 passed, ruff check and format clean, mypy clean apart from the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py, ratchet "No new lines.", sentinel "All 35 protected strings survive.", tenant-scope gate exit 0.

🤖 Generated with Claude Code

[github-actions]

🤖 Review by Claude Code

Summary

This PR closes a real gap (H-02): entity/relation/link rows mined out of a memory that governance drops mid-extraction previously survived forever, since soft-delete doesn't trigger the FK cascades. The storage-side purge (memory_purge_entity_artifacts) is carefully tenant-scoped, transactionally consistent, and well covered by negative tests (cross-tenant, straddling links/relations, live-row refusal). The worker-side race handling adds liveness re-checks around the persistence block. The design is heavily documented, and I did not find a data-correctness bug in the actual SQL purge logic. Two design trade-offs are worth surfacing though: one changes failure semantics for the common (non-drop) case, and one adds systematic extra load to the writer DB.

Medium/Low Issues

Transient liveness-check failures are treated as a confirmed drop, silently discarding audit/contradiction/cross-link work for a likely-still-live memory

Severity: Medium
File: core-api/src/core_api/services/entity_extraction_worker.py (added _purge_written_artifacts_if_dropped, except Exception: ... return True branch, and its two call sites)
Problem: When sc.get_memory(...) raises (e.g. a transient timeout, not an actual governance drop), the helper returns True ("treat as dropped, stop"), which makes both call sites (return mid-function, and the fall-through at the end) abort the rest of extraction — skipping the audit-log entry, contradiction-detection trigger, and cross-link discovery — for a memory that is almost certainly still live; nothing retries this later.

🤖 Claude Code Prompt

In core-api/src/core_api/services/entity_extraction_worker.py, the helper
_purge_written_artifacts_if_dropped() treats any exception from
sc.get_memory(...) as equivalent to a confirmed drop and returns True, causing
process_entity_extraction (both call sites — after the link upsert, and at the
end of the function) to stop early. This means a purely transient storage read
failure (unrelated to any governance decision) silently discards the audit log
entry (log_action), the contradiction-detection trigger, and cross-link
discovery for a memory whose entities/links were just successfully written and
which is in fact still live — with no retry mechanism to recover this later
(the task is fire-and-forget via track_task, non-fatal on exception).

Consider distinguishing "confirmed dropped" (live is not None and
deleted_at is set, or live is None i.e. 404) from "indeterminate" (the
get_memory call itself raised): for the indeterminate case, either retry once
with backoff before giving up, or continue with the downstream writes (audit
log / contradiction trigger / cross-link discovery) rather than discarding
them, since the entities/links have already been persisted and are cheap to
purge later if the drop is later confirmed. At minimum, emit a distinguishable
log/metric for the indeterminate outcome so its real-world frequency (and the
resulting silent loss of audit records) can be monitored — currently it logs
via logger.exception with the same downstream effect (early return) as a
confirmed drop, and no test in tests/test_entity_linking_wiring.py exercises
this exception branch.

---

Every entity-extraction run now makes 2–3 extra synchronous reads against the storage WRITER

Severity: Low
File: core-api/src/core_api/services/entity_extraction_worker.py (new sc.get_memory(..., read=False) calls: pre-persistence check, mid-check inside if link_items:, and the final check before the outer except)
Problem: read=False intentionally routes to the writer (needed to observe a just-committed soft-delete), but this adds up to three extra writer round-trips to a code path that runs for effectively every memory write with extractable entities, increasing primary-DB read load and per-task latency fleet-wide.

🤖 Claude Code Prompt

In core-api/src/core_api/services/entity_extraction_worker.py, the new
liveness checks (before persistence, after the link upsert, and at the end of
process_entity_extraction) all call sc.get_memory(..., read=False), which
routes to the storage WRITER rather than a read replica. Since this function
runs for essentially every memory that yields extracted entities, this adds
up to 3 extra writer-bound HTTP round trips per extraction task fleet-wide.
This is likely an acceptable, deliberate trade-off (a replica read could miss
a just-committed delete), but given it changes writer read volume
systematically, consider confirming this was load-tested / has monitoring for
writer read-QPS impact under bulk-import or high-throughput scenarios, and
whether the final post-cross-link-discovery check (which only exists to close
a narrow window after the mid-check) is worth its cost versus, e.g., relying
on the eventual reprocessing done by contradiction detection's own deleted_at
recheck for that specific narrow window.

---

Reviewed by claude-sonnet-5 · cost $1.8001799999999997

[Eldad-Caura]

Head 41a8f36a. Your Medium is fixed. Your Low is not, deliberately. And I have a correction to make about my own earlier comments on this PR.

The Medium

You were right, and the part I want to acknowledge specifically is the cost I had not weighed. My docstring reasoned about the indeterminate case only in terms of losing relations, which a later content update rebuilds. It also skipped the audit-log entry, the contradiction trigger and cross-link discovery, because the first call site returns before all three. An audit record lost to a read timeout is rebuilt by nothing, and the task is fire-and-forget.

The root cause was a boolean carrying a policy it couldn't express — "you must stop" forced the indeterminate case to impersonate one of the two real answers. The helper now returns live / dropped / unknown and the call sites decide, because they legitimately differ:

• the first site acts on dropped only. It exists to save the relation upserts and a cross-link pass, so it has no business destroying the audit work on a non-answer; a real drop is still caught by the final check, which is the guarantee.
• the final site doesn't branch at all — nothing follows it, so live and unknown are the same instruction and dropped has already purged.

A failed purge still reports dropped (the memory is gone either way); only a failed read is unknown, and it carries a structured liveness_check field so its real frequency is measurable rather than assumed. A test drives a transient read failure at the middle check and asserts the audit entry, the relations and cross-link discovery all still happen and nothing was purged — reverting unknown to dropped fails it.

The Low — not fixed, and why

Three writer-routed reads per extraction is real added load on a path that runs for essentially every memory with entities. The routing is deliberate (a replica can't see a just-committed delete), but you're right that nobody has load-tested the volume. That's an operational question — it wants a look at writer read-QPS under bulk import, not a code change decided inside this PR. It's recorded in the commit message as explicitly not done rather than left implied.

A correction about my own verification claims

Several of my earlier comments on this PR ended with a line like "ratchet 'No new lines.', sentinel 'All 35 protected strings survive.', tenant-scope gate exit 0." Those three gates were being run against the wrong working tree. My shell's directory resets between commands, and those scripts inspect the current directory rather than their own location — so they were measuring a different worktree, one sitting on the already-merged #1292 branch.

They reported green, and re-run correctly on this branch they are still green (ratchet 0, sentinel 0, tenant-scope gate 0), so nothing bad shipped. But for several rounds those lines were not evidence about this PR, and I stated them as if they were. The bug surfaced only because #1292 merged and that stale tree drifted from main, flipping a false pass into a false failure — which is exactly the way this class of mistake normally stays invisible.

Also rebased onto current main, which the branch had fallen behind.

Verified at this head, from the correct tree: core-api 6132 passed, core-storage-api 359 passed, ruff check and format clean, mypy clean on core-storage-api/src/ and clean on core-api/src/ apart from the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py, all three naming gates exit 0.

Not requesting another review round on this one — nine is enough, and the remaining item is an operational question rather than a code finding.

🤖 Generated with Claude Code

[Eldad-Caura]

@claude re-review at head 472f12cf.

This round reviews a change nothing has seen yet, and the finding is mine rather than a reviewer's. Round 9's fix left an exit uncovered.

What was wrong

The check at the end of the try block is the leak guarantee for the path that completes. It is unreachable on the path that does not. Anything between the link upsert and it that raises — a relation upsert, the subject write-back, the audit call — jumps straight to except Exception, and the entities and links already committed stay behind for a memory that may have been dropped. There is no finally; that call sits inside the try body like everything else.

Round 9 is what made this reachable rather than theoretical. Falling through on unknown was the right call — it stopped a transient read timeout destroying an audit record that nothing rebuilds — but it means the first call site now hands the responsibility forward to a later check, on a path where a later check may never run.

The comment I wrote there called the final check "the guarantee". It is the guarantee for one of the two ways out of the function. That is the third time on this PR that my prose has asserted a property the code did not have, and the second time the overclaim was in the paragraph written to justify the mechanism.

The fix

The except handler now carries the same check, guarded on whether anything was written at all.

Not a finally, and the three early returns are why. The no-entities exit happens before sc is bound, so an unguarded finally raises NameError out of a fire-and-forget task. The already-dropped exit would spend a writer read to learn what it just learned. The dropped exit would repeat a purge that had only just run. Those are the common paths, not the rare ones — and since the flag has to gate a finally anyway, all finally buys is one fewer call site.

The flag is set BEFORE the upsert await, not after, and that is load-bearing. "The call raised" is not "nothing was written": a timeout can land on a request storage already committed, and then the rows exist while the caller only ever saw an exception. Setting the flag afterwards would skip the check on exactly that case and leak the rows. Being wrong in the other direction costs one writer read on a call that never landed, on a path that is already failing.

Tests

Four, and I am explicit about which is which:

• The reproducer — a relation upsert raises after the links are committed, and the memory is dropped. Without the fix get_memory is awaited twice instead of three times and nothing is purged. Probe-confirmed.
• The ordering probe — moving wrote_graph_rows = True one line down, below the await, fails it. Probe-confirmed.
• Two guards on the fix, which pass either way and say so in their docstrings: a failure that wrote nothing must not pay for a writer read, and a failure before sc exists must not raise NameError out of the task.

Also in this push

Two comment corrections, both found by reading my own diff rather than by a reviewer:

• the helper docstring said this function is called twice; it is called three times now, and the docstring lists what each call site is for instead;
• a sentence weighing what an unguarded finally would cost said "two of those would spend a writer read" when one of the three would not get that far — it would raise NameError on the unbound sc.

And a commit-message repair unrelated to the code: the body had lost its opening line in some earlier amend, so it began mid-sentence on dropped content are the same leak in another table". Restored from the PR description, which still had it.

Still not done, deliberately

Your round-8 Low — three writer-routed reads per extraction task, fleet-wide, never load-tested — is unchanged, and this push makes it four on the failure path. The routing is deliberate (a replica cannot see a just-committed delete) but the volume is an operational question, not one to settle inside this PR.

Verification

ruff check and ruff format --check clean at CI's exact scopes (core-api/src/, tests/, run separately). mypy clean bar the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py. Legacy-name ratchet, do-not-touch sentinel and tenant-scope gate all exit 0, run from inside the worktree after git add. Broker OpenAPI baseline up to date (8 operations). Full core-api suite green.

[github-actions]

🤖 Review by Claude Code

Summary

This PR closes an entity/relation graph leak (H-02) where memories dropped by governance mid-extraction, or via cascade, left orphaned entity/link/relation rows behind. The core storage-layer purge (memory_purge_entity_artifacts), the new liveness re-checks in entity_extraction_worker.py, and the governance cascade wiring are all thoroughly reasoned and internally consistent, including deliberate handling of tenant scoping, straddling historical rows, and idempotency. I found one real inconsistency worth fixing before merge; everything else is either intentional/well-justified or out of scope (tests).

Missed opportunity to reuse the wrote_graph_rows guard on the success path

Severity: Medium
File: core-api/src/core_api/services/entity_extraction_worker.py (end-of-try call to _purge_written_artifacts_if_dropped, immediately before the except Exception: block)
Problem: The final, unconditional post-write liveness/purge check at the end of the successful path runs even when nothing was ever written (e.g. graph.entities was non-empty but every candidate was filtered out by _is_valid_entity/blocklist/dedup, leaving filtered empty and skipping the whole persistence branch), spending an extra writer-DB round trip for no reason — exactly the waste the wrote_graph_rows flag was introduced to avoid in the except handler.

🤖 Claude Code Prompt

In core-api/src/core_api/services/entity_extraction_worker.py, `process_entity_extraction`
sets `wrote_graph_rows = True` right before `sc.bulk_upsert_entities(...)` is awaited (inside
the `else` branch that only runs when `filtered` is non-empty), and the `except Exception:`
handler at the bottom correctly gates its call to `_purge_written_artifacts_if_dropped(sc,
memory_id, tenant_id)` on `if wrote_graph_rows:` specifically to avoid a wasted writer read
when nothing was ever written for this memory.

However, the unconditional call to `_purge_written_artifacts_if_dropped(sc, memory_id,
tenant_id)` at the very end of the `try` block (right before `except Exception:`, after the
cross-link-discovery step) has no such guard. It runs whenever `graph.entities` was non-empty
and the pre-write liveness check passed, even if `filtered` ended up empty after the
`_is_valid_entity`/blocklist/dedup filtering (i.e. `name_to_id == {}` and no entity/link rows
were ever written for this memory). In that case the call still performs a writer-side
`get_memory` read that can never find anything to purge.

Fix: gate this final call the same way the `except` handler does, e.g. change it to
`if wrote_graph_rows: await _purge_written_artifacts_if_dropped(sc, memory_id, tenant_id)`,
so the extra writer round-trip is only paid when something was actually written to the graph
for this memory. This keeps the cost proportional to what's at risk, consistent with the
rationale already documented for the `except`-handler guard, and does not change behavior
for any case where entities/links were actually persisted.

---

Reviewed by claude-sonnet-5 · cost $2.47907125

[Eldad-Caura]

@claude re-review at head bb7e3d84.

Taken — and the suggested form of it would have reopened H-02. I have the failing test.

You were right on the observation. The end-of-try check runs unconditionally, and there is a path that reaches it having written nothing: graph.entities non-empty, every name rejected by the blocklist or _is_valid_entity, so filtered is empty and the persistence block never runs. That read is waste and it should be gated.

Why the stated fix leaks

wrote_graph_rows, as the finding reads it, means "the persistence block ran". That is not the same as "this memory has graph rows".

Cross-link discovery is gated on auto_entity_linking_enabled alone — not on name_to_id, not on filtered — so it runs on exactly the path the finding describes. And storage-side it is not a read:

insert_link_returning = (
    pg_insert(MemoryEntityLink)
    .values(rows)
    .on_conflict_do_nothing(index_elements=["memory_id", "entity_id"])
    ...
)

memory_entity_links is the table the purge deletes from. So a memory can acquire graph rows without bulk_upsert_entities ever being called, and gating the final check on a flag that only tracks the persistence block skips the purge and strands the links discovery just created — for a memory governance dropped. That is this PR's own bug, arriving through the guard added to close it.

Probe-confirmed. Applying the suggestion verbatim:

FAILED tests/test_entity_linking_wiring.py::test_cross_link_discovery_alone_still_owes_the_memory_a_liveness_check

What I did instead

Both writers set the flag, each before its own await, and then the final check is gated. That is the efficiency you asked for — a run that wrote nothing and had linking disabled no longer pays for a writer read — without the leak. Behaviour is unchanged wherever anything was actually persisted.

The flag's declaration now states what it has to mean for the gate to be safe: "this memory MAY have graph rows", not "the persistence block ran". Gating on the narrower reading is the bug, so the name and the comment both have to carry the wider one.

The helper docstring is corrected in the same pass: it said the except-site check was the guarded one. Both trailing checks are guarded now.

A pattern worth naming

This is the third time on this PR that the observation was worth acting on while the remedy was wrong in the unrecoverable direction — after the relation anti-join scoped by Relation.tenant_id, and the deleted_at fix that gated only the owned subquery. Each was a narrowing that is locally consistent and drops a case the wider code still depends on. Each is now pinned by a test that fails under the suggested version, so the next person to reach for the "obvious" narrowing finds out immediately.

Verification

Full core-api suite 6137 passed, 0 failed. ruff check and ruff format --check clean at CI's exact scopes (core-api/src/, tests/, run separately). mypy clean bar the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py. Legacy-name ratchet, do-not-touch sentinel and tenant-scope gate all exit 0, run from inside the worktree after git add. Broker OpenAPI baseline up to date (8 operations).

Still unchanged and still deliberate: your round-8 Low on writer-routed read volume. That is an operational question for a load test, not something to settle in this PR.

[github-actions]

🤖 Review by Claude Code

Summary

This PR implements H-02: purging derived entity/relation/link graph rows when a memory is governance-dropped, both for the entity-extraction worker race window and for the governance remediation cascade. The core SQL purge logic (memory_purge_entity_artifacts) is carefully scoped by tenant and by a "candidates from links" set to avoid over-deletion, and the worker-side liveness-check placement (pre-write, post-link, end-of-try, except) is well reasoned and tested. One real defensive-coding gap stands out around the assumption that the storage response is always a well-formed dict.

Medium/Low Issues

Unguarded .get() on the purge response breaks the documented "never raises" contract

Severity: Medium
File: core-api/src/core_api/services/governance_remediation.py:407-414 (also core-api/src/core_api/services/entity_extraction_worker.py in _purge_written_artifacts_if_dropped, the block after counts = await sc.purge_entity_artifacts(...))
Problem: counts = await sc.purge_entity_artifacts(...) is wrapped in try/except Exception, but the subsequent counts.get("links")/"relations"/"entities" calls are outside that try, so any successful-but-malformed response (e.g. a non-dict body from a version-skewed or misbehaving storage instance) raises AttributeError uncaught.

🤖 Claude Code Prompt

In core-api/src/core_api/services/governance_remediation.py, function `_purge_entity_artifacts`
(around line 400-415), the success path does:

    if any(counts.get(k) for k in ("links", "relations", "entities")):
        logger.info(...)

This runs outside the `try/except Exception` that wraps the call to
`sc.purge_entity_artifacts(...)`. The function's own docstring states that
failures must be LOGGED, not RAISED, because this helper is called directly
from `remediate_after_enrichment`, which in turn is invoked by
`consumer.handle_memory_enriched` with no surrounding guard — and the Pub/Sub
dispatcher NACKs on any handler exception, causing redelivery and a duplicate
`critical=True` audit entry (the exact failure mode this docstring says must be
avoided).

If `sc.purge_entity_artifacts` returns a 200 response whose JSON body is not a
dict (e.g. `None`, a list, or an empty body from a misbehaving/mismatched
storage version), `resp.json()` succeeds (no exception raised inside `_post`),
so the `except Exception` clause never fires — but `counts.get(...)` then
raises `AttributeError`, which propagates uncaught out of
`_purge_entity_artifacts` and out of `remediate_after_enrichment`, straight
into the unguarded Pub/Sub consumer path this code was written specifically to
protect.

Fix: guard the counts access, e.g. only log if `isinstance(counts, dict)` and
treat any other shape the same way as a caught exception (log an error and
return without raising). The exact same pattern exists in
core-api/src/core_api/services/entity_extraction_worker.py inside
`_purge_written_artifacts_if_dropped`, in the block after
`counts = await sc.purge_entity_artifacts(tenant_id, str(memory_id))` — apply
the same defensive check there too (that call site is inside the worker's
top-level catch-all, so the practical risk is lower, but the same
type-safety gap exists).

---

Reviewed by claude-sonnet-5 · cost $1.7558937500000003

[Eldad-Caura]

@claude re-review at head 4eb5a9df.

Taken, in full — and your severity note on the worker site is understated rather than lower.

You're right that counts.get(...) sits outside the try, and right about why it matters. I verified the chain rather than taking it on the description: consumer.handle_memory_enriched catches GovernanceCascadeError and nothing else, so an AttributeError out of _purge_entity_artifacts reaches the dispatcher, nacks, redelivers, re-runs the whole drop branch and writes a second critical=True audit for an already-dropped memory. That is exactly the failure round 2 of this PR fixed, arriving back through a type nobody checked.

The tell you identified is in the client itself:

return await self._post(  # type: ignore[return-value]
    "/memories/purge-entity-artifacts",
    ...

_post is declared dict | list. The type: ignore is precisely where "this is a dict" was asserted without evidence, and nothing downstream re-checks it.

Where I differ

that call site is inside the worker's top-level catch-all, so the practical risk is lower

True of two of the three call sites. Not the third. The one added in round 10 is the catch-all — it lives in the except Exception: handler — and an exception raised inside an except block propagates out of the function rather than being caught by it. There it escapes process_entity_extraction entirely and surfaces as an unhandled exception on a fire-and-forget task. The worker test drives that site specifically, not one of the two protected ones.

What the fix says

Both sites now treat a non-object response as its own state instead of forcing it into one of the two answers they already had — the same mistake as the bool-with-a-policy from round 9, in a different place.

A 2xx means the call succeeded. A body we cannot read means we cannot say what it did. So neither "purged" nor "failed" is a claim worth making: governance logs at ERROR that the purge cannot be confirmed and the rows may still be listable, and the worker logs that the counts are unknown while still returning dropped — because the memory's state is not in question, only the report of what was cleaned.

A note on my own test

The governance test failed the first time I ran it, and the reason is worth recording. I wrote pii={"enabled": True, "disposition": "drop"} — but the PII branch keys on action; disposition is the non-business key. The wrong key lands silently on flag, which never purges, so the test would have exercised a path that never reaches the line under test and passed for no reason at all.

It now also asserts the pii_drop audit was emitted, so a config that doesn't reach the purge fails loudly instead of passing vacuously. That is the third instance on this PR of a test being green because the fixture avoided the path, after graph.relations = [] and the mismatched-tenant test.

Both new tests are probe-confirmed: removing the two guards fails exactly those two and nothing else.

Verification

Full core-api suite green. ruff check and ruff format --check clean at CI's exact scopes (core-api/src/, tests/, run separately). mypy clean bar the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py. Legacy-name ratchet, do-not-touch sentinel and tenant-scope gate all exit 0, run from inside the worktree after git add. Broker OpenAPI baseline up to date (8 operations).

Still unchanged and still deliberate: the round-8 Low on writer-routed read volume, which is a load-test question rather than a PR one.

[github-actions]

🤖 Review by Claude Code

Summary

This PR closes an entity/relation-graph data leak (H-02) where governance-dropped memories left their extracted PII/entity rows behind. The implementation is extensive and unusually well-reasoned (race windows, idempotency, tenant scoping, over-deletion guards are all carefully handled and tested). I found one real correctness gap in the purge's completeness guarantee; the rest is solid.

Medium/Low Issues

Purge misses entities that were created but never linked

Severity: Medium
File: core-storage-api/src/core_storage_api/services/postgres_service.py (new memory_purge_entity_artifacts, candidate-selection logic) and core-api/src/core_api/services/entity_extraction_worker.py (the if link_items: link_result = await sc.bulk_upsert_entity_links(...) call site)
Problem: memory_purge_entity_artifacts seeds its entity-candidate set exclusively from MemoryEntityLink rows for the memory, so if bulk_upsert_entities commits new entity rows but the subsequent bulk_upsert_entity_links call raises (no link row is ever written for those entities) and the memory is governance-dropped in that same window, the newly created entity names are permanently invisible to the purge — it runs, finds zero links/relations/entities, and returns a clean {"links": 0, "relations": 0, "entities": 0} with no diagnostic indicating anything was skipped. Unlike the "purge failed, loudly" path this PR carefully documents, this is a silent miss: the audit trail looks identical to "there was nothing to purge," so nobody is ever pointed at the orphaned rows.

🤖 Claude Code Prompt

In core-storage-api/src/core_storage_api/services/postgres_service.py, the
method `memory_purge_entity_artifacts` builds its `candidates` list for entity
deletion solely from `MemoryEntityLink.entity_id` rows matching `memory_id`
(see the `select(MemoryEntityLink.entity_id).where(MemoryEntityLink.memory_id
== memory_id)` query). Any entity row created via `bulk_upsert_entities` in
core-api/src/core_api/services/entity_extraction_worker.py's
`process_entity_extraction` (around the `if link_items:` block that calls
`sc.bulk_upsert_entity_links`) that never got a link row committed — e.g.
because `bulk_upsert_entity_links` itself raised after entity creation
succeeded — is invisible to this candidate set and will never be purged, even
when the memory it was extracted for is dropped and the purge runs and
reports success.

Fix by either:
1. Having `process_entity_extraction` track which entity_ids it successfully
   upserted (not just which got linked) and pass that list along so a purge
   triggered on failure can target them directly (e.g. an optional
   `entity_ids` parameter on `purge_entity_artifacts` / `memory_purge_
   entity_artifacts` used as an additional candidate source), or
2. Wrapping the `bulk_upsert_entity_links` call in its own try/except in
   entity_extraction_worker.py so that on failure it logs (with the specific
   entity_ids that were just upserted in step 2c) that those entities may be
   orphaned and unreachable by the standard purge, giving an operator a
   concrete list to clean up by hand instead of a clean-looking zero-count
   purge response.

Add a test (in the appropriate non-tests/ location per repo convention, or
extending the existing H-02 test suite) that upserts an entity via
`bulk_upsert_entities` without ever creating a `MemoryEntityLink` row for it,
then asserts that `memory_purge_entity_artifacts` either removes it or that
the gap is at least surfaced via logging/return value, rather than silently
reporting an all-zero success.

---

Reviewed by claude-sonnet-5 · cost $1.73800525

[Eldad-Caura]

@claude re-review at head 51efc4ff.

Taken — your second option, not your first, and the reason is the one this PR has already been burned by twice.

The gap is real and I'm not disputing it. The purge finds entities through the memory's links, so an entity row committed by bulk_upsert_entities whose link never landed is reachable by nothing: the purge runs, finds no links, deletes nothing, and returns {"links": 0, "relations": 0, "entities": 0}.

Your sharpest point isn't the gap, it's its shape. That response is indistinguishable from "there was nothing to purge". Everything else this PR leaves behind, it leaves behind loudly — a failed purge, a failed cascade, an unreadable response all name the memory in an ERROR. This one was the exception, and that's what made it worth changing.

Why not option 1

Passing the upserted ids to the purge as an extra candidate source re-introduces the exact race the candidate bounding exists to prevent. Round 1 went through this: the first draft deleted every unlinked entity in the tenant, and it was narrowed because that sweeps rows orphaned for unrelated reasons and races a concurrent writer that has created an entity but not yet linked it.

The narrower list doesn't escape that. bulk_upsert_entities upserts — an id it returns may be a row another writer created moments ago and is about to link. Deleting it because our link failed would be over-deletion caused by our own failure, and over-deletion is the direction that doesn't come back. A person handed a concrete list can check what a query cannot.

What went in

The link upsert gets its own try/except that logs the ids and re-raises. The re-raise is load-bearing: the except handler below still runs its liveness check and still purges everything that is reachable, so this adds a diagnostic and changes no behaviour.

Only entities this run created are named. bulk_upsert_entities reports action per row, and an entity that already existed is reachable through whatever linked it before — naming it would send an operator after rows that are nobody's orphans.

The gap itself stays, deliberately, and the code now says so at the place it lives rather than in a commit message nobody will find.

On the test you asked for

Add a test ... that upserts an entity via bulk_upsert_entities without ever creating a MemoryEntityLink row for it, then asserts that memory_purge_entity_artifacts either removes it or that the gap is at least surfaced

The "removes it" half is already pinned in the opposite direction by an existing storage test from round 1 — the unbounded candidate set fails it with 2 == 1. Adding a test that asserts the purge does reach unlinked entities would contradict it. So this is the "surfaced" half only: the new test asserts the created id appears in the log, and removing the handler fails it.

Verification

Full core-api suite 6140 passed, 0 failed. Probe-confirmed. ruff check and ruff format --check clean at CI's exact scopes (core-api/src/, tests/, run separately). mypy clean bar the two pre-existing types-python-dateutil errors in untouched common/enrichment/service.py. Legacy-name ratchet, do-not-touch sentinel and tenant-scope gate all exit 0, run from inside the worktree after git add. Broker OpenAPI baseline up to date (8 operations).

Still unchanged and still deliberate: the round-8 Low on writer-routed read volume — a load-test question rather than a PR one.

[github-actions]

🤖 Review by Claude Code

Claude Code Review ✅ No issues found.

---

Reviewed by claude-sonnet-5 · cost $1.7521535

[Eldad-Caura]

@erni-a please approve