Skip to content

Remove SELECT FOR UPDATE from note materialization publish; rely on the db_version CAS with guarded writes #1224

Closed

Description

@phernandez

Problem

publish_written_file_state in src/basic_memory/indexing/note_materialization_runner.py acquires FOR UPDATE row locks on NoteContent and Entity and holds them across the whole plan-compute-write span. In Basic Memory Cloud this anchors real Postgres deadlocks: a three-transaction wait cycle on the entity row lock, recurring on the pgq worker (basicmachines-co/basic-memory-cloud#1727, Logfire #2384, 4 occurrences 2026-08-07 → 2026-08-10).

Why the locks are removable

The path already has the mechanisms that make pessimistic locking redundant:

  • Every NoteContent write goes through apply_note_content_update_plan(..., expected_db_version=...) — a compare-and-swap with an explicit stale-skip path. The code comments state this exists precisely because concurrent publishers are expected.
  • with_for_update is a documented no-op on SQLite (note_content_repository.py), so core semantics must already be correct without row locks; the CAS is load-bearing by design.
  • Hosted runtimes can serialize same-entity materialization externally via NoteMaterializationSessionLock (Cloud passes a pg_advisory_xact_lock(project_id, entity_id) taken first in the transaction, which is deadlock-free by construction). The default Noop lock is exactly the concurrent case the CAS guards.

Fix

In publish_written_file_state:

  • Read NoteContent and Entity plainly — drop both with_for_update() calls.
  • NoteContent writes: unchanged; the CAS row lock is taken only at UPDATE time.
  • Entity write: replace the blind ORM attribute writes (entity.mtime, entity.size) with one guarded single-statement UPDATE (WHERE id AND project_id; rowcount 0 → the existing "entity disappeared" result). It already runs only after a successful CAS in the same transaction, so the canonical NoteContent-before-Entity order is preserved with a millisecond lock window.
  • Same treatment for NoteFileVacateRepository.clear_vacate_path, which takes with_for_update on Entity inside this transaction.

Out of scope: the canonical lock-order protocol in relation_repository.py (current_relation_generation_statement and friends) stays as is — this change only stops the materialization publisher from holding SELECT-time locks that other protocols (including FK KEY SHARE from relation writes) can deadlock against.

Acceptance criteria

  • publish_written_file_state and the vacate-marker path hold no SELECT-time row locks; all writes are guarded single-statement updates with rowcount checks.
  • Synthetic regression test: concurrent materialization publish vs newer accepted write — the newer db_version/file_version is never reverted, and the stale publisher reports stale rather than erroring.
  • Existing SQLite behavior unchanged (locks were no-ops there already).
  • Cloud symptom tracked in basicmachines-co/basic-memory-cloud#1727 resolves (no Logfire #2384 recurrence).

Activity

  1. claude commented on Aug 10, 2026

    @claude
    Contributor

    Claude finished @phernandez's task in 43s —— View job


    Triage

    Type: Bug (verified against code — publish_written_file_state in src/basic_memory/indexing/note_materialization_runner.py:440 and :448 currently calls .with_for_update() on the NoteContent and Entity queries, matching the report. The expected_db_version CAS path is present at lines 475–509.)

    Component: Database/sync (core indexing/materialization path in src/basic_memory/indexing/). Symptom manifests specifically under Postgres in Cloud (SQLite with_for_update is a documented no-op), but the fix lives in shared core code rather than Cloud-only code, so no cloud label applied.

    Priority: High — this anchors real, recurring Postgres deadlocks on the pgq worker in production (linked: basicmachines-co/basic-memory-cloud#1727, Logfire #2384, 4 occurrences 2026-08-07 → 2026-08-10). Not critical/data-loss since the CAS path already prevents stale writes; the bug is availability/throughput (deadlock aborts), not correctness.

    Complexity: Medium — the fix is scoped to one function plus NoteFileVacateRepository.clear_vacate_path (replacing two SELECT ... FOR UPDATE reads with a plain read + guarded single-statement UPDATE ... WHERE id AND project_id with rowcount check), but it touches transaction/locking semantics shared between SQLite and Postgres runtimes, so it needs careful regression coverage for the concurrent-write race described in the acceptance criteria (newer db_version never reverted, stale publisher returns stale not an error).

    No duplicate issues found in the repo.

    Labels applied: bug (type), component left as none per the reasoning above — Cloud engineers may want to cross-reference basicmachines-co/basic-memory-cloud#1727 for the observed incident.

  2. added this to the v0.23 milestone on Aug 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    bugSomething isn't working

    Projects

    No projects

      Milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions