fix(core): enforce materialization lock order - #1193
Conversation
Signed-off-by: phernandez <paul@basicmachines.co>
There was a problem hiding this comment.
💡 Codex Review
When this new Entity lock has to wait behind an Entity-first index/reconcile transaction, RepositoryNoteMaterializationPublisher has already computed publish_plan from the pre-wait NoteContent.file_path, and the guarded update only checks db_version. A same-content file move/reconcile can update Entity.file_path and realign NoteContent.file_path without changing db_version; after the wait, this assignment records the checksum/version of the file written to the old prepared path under the new Entity path and the publisher still updates Entity metadata as written. Recompute or recheck the current NoteContent.file_path after acquiring the Entity lock, or take the lock before planning the publish.
ℹ️ 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".
Signed-off-by: phernandez <paul@basicmachines.co>
|
Addressed the current-head Codex path-race finding in 6cb9f0a. The publisher now acquires the project-scoped Entity row lock before reading NoteContent or planning publication, so a same-version move cannot change the destination while the publisher waits. The real Postgres regression now covers both a stable-path reconciliation (publish succeeds) and a moved-path reconciliation (old-path write is reported stale/orphaned and does not advance file state). Verification: 44 focused tests plus 2 SQLite skips, 23 real-Postgres repository/lock-order tests, 69 broader stale/CAS/checksum/redelivery tests, and just fast-check passed. |
Summary
Fixes the Entity/NoteContent lock-order inversion reported from hosted note
materialization. Version-guarded NoteContent state updates now acquire the
owning Entity row lock before updating NoteContent, matching the existing
Entity-first indexing and reconciliation order.
Fixes #1187.
Problem
The materialization publisher updated NoteContent first and then flushed file
metadata to Entity in the same transaction. Entity-first reconciliation could
hold the Entity row while waiting to update NoteContent. Under concurrency,
those two transactions formed the cycle:
Postgres could select either transaction as a deadlock victim. Retrying at the
Cloud job boundary reduced impact but did not remove the Core lock inversion.
Solution
NoteContentRepository._load_entity_identity()to request aSELECT ... FOR UPDATElock.NoteContent or planning publication, so a same-version move cannot change the
destination while publication waits.
update.
db_versionupdate, stale-job skip, checksumpropagation, and Entity file-metadata flush unchanged.
tenant-neutral Core row-lock invariant.
The ordering is now consistently:
Regression coverage
The new real-Postgres integration test orchestrates two transactions:
transaction owns Entity;
then commits;
stale and orphaned, without deadlock or incorrect file-state advancement.
Before this fix, the publisher reaches the instrumented NoteContent update while
the Entity lock is held, exposing the inversion. The test also verifies the
stable-path file version, checksum, sync status, Entity mtime, and Entity size.
The moved-path case proves the old write is not recorded against the new path.
Verification
BASIC_MEMORY_ENV=test LOGFIRE_IGNORE_NO_CONFIG=1 uv run pytest -q --no-cov tests/repository/test_note_content_repository.py tests/indexing/test_note_materialization_runner.py tests/cloud/test_note_content_materialization.py tests/runtime/test_note_content_store_materialization.py tests/runtime/test_note_materialization_request_matching.pyBASIC_MEMORY_ENV=test BASIC_MEMORY_TEST_POSTGRES=1 LOGFIRE_IGNORE_NO_CONFIG=1 uv run pytest -p pytest_mock --no-cov -q tests/repository/test_note_content_repository.py test-int/test_note_materialization_lock_order.pyjust fast-checkgit diff --cached --checkRisk
Version-guarded NoteContent updates now hold the Entity row lock until their
transaction completes. That adds intentional serialization for updates to the
same note, but it removes the conflicting lock order. Updates for different
entities remain independent.