fix(core): remove entity read locks from materialization publish

[phernandez]

Closes #1224. Cloud symptom: basicmachines-co/basic-memory-cloud#1727 (Logfire #2384).

What changed

publish_written_file_state no longer takes any SELECT-time row locks. The existing
expected_db_version CAS is the only write guard, and the transaction's first row lock is
the CAS UPDATE itself — preserving the canonical NoteContent-first order, so the publisher
can no longer anchor a lock cycle.

• Dropped both with_for_update() calls on the NoteContent and Entity planning reads.
• Also removed the Entity lock inside the CAS path (_load_entity_identity(lock_for_update=True)
  in update_state_fields). This lock isn't in the issue text, but dropping only the
  publisher's read locks would have left the publisher acquiring Entity → NoteContent while
  accepted mutations acquire NoteContent → Entity — recreating the deadlock with inverted
  order. The dead lock_for_update parameter is deleted.
• Entity mtime/size writes are now one guarded single-statement UPDATE via the existing
  Repository.update_fields (WHERE id AND project_id, rowcount-checked). rowcount 0 maps
  to the existing "entity disappeared" missing result and skips the vacate-marker clear. No
  staleness guard is needed: a successful CAS holds the note_content row lock until commit,
  and every producer of newer entity file metadata crosses that row first.
• CAS-loss orphan re-check (slightly beyond the issue text — flagging for review): with
  the old locked reads, a concurrent move was visible at plan time and the written file at
  the vacated path got orphan cleanup. With plain reads that move surfaces as a CAS loss, so
  the loss branch now re-reads Entity (populate_existing=True to bypass the identity map);
  if the entity is gone or its path moved, the stale result carries
  written_file_orphaned=True, restoring exact cleanup parity.

Issue-text corrections

• clear_vacate_path is untouched: it is already a plain project-scoped DELETE with no
  Entity lock. The Entity with_for_update in that file is lock_recoverable_vacate, which
  belongs to move-recovery and never runs in the publish transaction.

Verification

• tests/indexing/test_note_materialization_runner.py + repo/reconciler suites: 75 passed
  (4 new unit tests: entity missing pre-CAS, entity missing post-CAS, CAS loss same-path not
  orphaned, CAS loss orphans vacated path).
• test-int/test_note_materialization_lock_order.py on real Postgres (testcontainers):
  3 passed — the FOR UPDATE deadlock probe is kept as a regression tripwire, the moved-path
  case now models a real move (db_version advances) and asserts the newer path is never
  reverted, and the new test_publish_cas_loss_never_reverts_newer_accepted_write pins the
  acceptance criterion (stale publisher reports stale, newer write survives untouched).
• just typecheck, just lint: clean.

Note for release: verify basic-memory-cloud's pgq worker doesn't rely on the publisher
holding Entity row locks (it injects the advisory session_lock, which is unchanged).

🤖 Generated with Claude Code

https://claude.ai/code/session_01CPdSXDbYyhyZ1TwgFnpEv8

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: bfc450ad6a

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Claim NoteContent before copying Entity identity

When a project-index move overlaps this CAS, the unlocked identity read can capture the old Entity.file_path, the move can then commit both paths without advancing NoteContent.db_version (project_index_maintenance.py builds only path assignments), and this CAS still succeeds and writes the old path back into NoteContent. That leaves Entity and NoteContent disagreeing and can make a materialization publish or watcher recreate the vacated file; acquire the NoteContent claim before reading identity or add an identity/path condition that detects this race.

Useful? React with 👍 / 👎.

[chatgpt-codex-connector]

Recheck CAS loss for already-stale publications

With these planning reads now unlocked, if the request is already stale at the snapshot and an accepted move advances the row again before publication, the stale_db_version branch ignores the failed apply_note_content_update_plan result and returns its original non-orphaned result. run_note_materialization therefore does not enqueue cleanup even though the entity now owns a different path, leaving the just-written old-path file available for duplicate re-indexing; handle CAS loss here with the same current-Entity recheck used by the current branch.

Useful? React with 👍 / 👎.

[phernandez]

Reviewed both findings. Declining both — rationale for the record:

P1 (stale identity copy under a concurrent project-index move): the premise is correct —
project_index_maintenance.py repoints Entity.file_path and NoteContent.file_path in
bulk without advancing db_version, so an overlapping move can make the unlocked identity
read stale while the CAS still passes, transiently writing the old path back into
note_content.file_path. But the pre-PR code did not prevent this race — it deadlocked on
it: the mover claims NoteContent first (lock_note_content_before_entity_mutation) while
the old publisher held the Entity row lock, which is exactly the #1224 cycle this PR
removes. Post-PR the overlap resolves without deadlock, and the divergence is a transient
denormalized-copy drift: materialization planning already prefers entity.file_path, and
the next accepted write or index pass refreshes the copy. We explicitly prefer that
eventual consistency over reintroducing SELECT-time locks. If telemetry ever shows this
mattering, the lock-free fix is copying the identity fields via a correlated scalar
subquery inside the single CAS UPDATE — noted for the future, not needed now.

P2 (re-check CAS loss in the already-stale branch): that branch's written file is
already superseded at plan time; if its guarded stale-write additionally loses the race,
the vacated-path marker and the next index pass reconcile the leftover file. Derived files
are eventually consistent by design here — a second re-read on an already-rare branch is
complexity without a failure mode that doesn't self-heal.

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 899b62af47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Mark deletion-race writes as orphaned

When a project-index deletion was planned from a missing-file scan, it can delete both rows after this unlocked NoteContent query but before the following Entity query; the publisher then retains the stale note snapshot, returns the entity is None result with written_file_orphaned=False, and run_note_materialization skips cleanup of the file it just wrote. Because files are authoritative to indexing, the next scan can recreate the deleted note; mark this missing-Entity outcome as orphaned so the checksum-guarded cleanup is enqueued.

AGENTS.md reference: AGENTS.md:L251-L251

Useful? React with 👍 / 👎.

[phernandez]

Declining this finding as well — same doctrine, with the reasoning on record:

The race is real but the window is two adjacent SELECTs wide: a deletion must commit in the
microseconds between the NoteContent read and the Entity read while a materialization for
that same entity is mid-flight. And the outcome is not corruption — the written file is the
materialization of a real accepted generation. Under this system's files-are-source-of-truth
model, a file on disk is a note; the next scan indexing it is the system converging on
"the accepted write happened," which is a coherent last-writer-wins resolution of a
genuinely ambiguous race (the file was deleted while a legitimate write for the same note
was in flight). Marking the result orphaned would resolve the same ambiguity in the other
direction — deleting an accepted write's output on the strength of a row-visibility race.

We are deliberately not adding per-race special cases to this path. Derived state here is
eventually consistent; every projection converges via the next write or index pass. If
telemetry ever shows deleted notes resurrecting through this window in practice, the fix is
one line (written_file_orphaned=True on the missing-entity results, cleanup stays
checksum-guarded) — recorded here for that day.