docs(core): set review expectations for eventually consistent state - #1230
Conversation
Signed-off-by: phernandez <paul@basicmachines.co>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a0c92f0d21
ℹ️ 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".
| - adding `SELECT ... FOR UPDATE`, lock ordering, or wider/shared transactions — this class | ||
| of "fix" caused the production deadlock clusters (#1213, #1224) and the silent observation | ||
| duplication (#1214); |
There was a problem hiding this comment.
When reviewing a new derived-state transaction that locks Entity or Relation before NoteContent, this blanket ban prevents flagging a real lock cycle. current_relation_generation_statement() explicitly defines NoteContent-before-Entity/Relation as the authoritative invariant (src/basic_memory/repository/relation_repository.py:67-81), and directory deletion follows it before projection cleanup (src/basic_memory/indexing/directory_delete_runner.py:228-235). Exempt wider/shared locking if desired, but retain review of inconsistent lock ordering; otherwise this guidance can reintroduce the production deadlocks it aims to avoid.
AGENTS.md reference: AGENTS.md:L165-L166
Useful? React with 👍 / 👎.
| - **Derived state** — entity file metadata, observation/relation graph rows, search index | ||
| rows, materialized files. This is **eventually consistent by design**. It converges |
There was a problem hiding this comment.
When DB-first materialization writes accepted Markdown to durable storage, these “materialized files” are the same Markdown files classified as canonical immediately above, not disposable projection rows. docs/DOMAIN_MODEL.md:106-116 and docs/DOMAIN_MODEL.md:140-148 define materialization as persisting the accepted Markdown bytes into their portable file representation. Calling the files derived can cause reviewers to ignore checksum, write, or deletion corruption under the prohibition below; classify only materialization status and metadata as derived while keeping the Markdown bytes canonical.
AGENTS.md reference: AGENTS.md:L115-L117
Useful? React with 👍 / 👎.
| operations. Correctness findings here are welcome. | ||
| - **Derived state** — entity file metadata, observation/relation graph rows, search index | ||
| rows, materialized files. This is **eventually consistent by design**. It converges | ||
| through the next write, the next index pass, `reindex`, `doctor`, or the scheduled orphan |
There was a problem hiding this comment.
When derived state is stale in an existing project, basic-memory doctor cannot make it converge: run_doctor() creates a uniquely named temporary project, exercises that project, and deletes it (src/basic_memory/cli/commands/doctor.py:94-193) without opening or reindexing the affected project. Because the later rule says to withhold findings when a listed mechanism repairs the drift, naming doctor here can incorrectly suppress a non-converging bug; remove it or replace it with an actual repair operation.
Useful? React with 👍 / 👎.
Signed-off-by: phernandez <paul@basicmachines.co>
|
All three findings accepted and applied — these are exactly the finding shape the section asks for (precise wrong-state claims, no serialization proposals): (1) the lock bullet now bans adding serialization while explicitly keeping violations of the canonical NoteContent-first ordering reviewable; (2) canonical state is now the note's markdown bytes wherever they live — materialization writes canonical content into portable form, so content corruption/wrongful deletion is always a real finding — and only materialization status/lineage timing is derived, per docs/DOMAIN_MODEL.md; (3) doctor removed from the repair mechanisms (it diagnoses in a temporary project, it does not repair). |
1 participant
Adds a "Consistency Model — Review Expectations" section to AGENTS.md so reviewers — the
Codex PR bot in particular — stop proposing lock/transaction machinery for derived-state
paths.
Context: every Codex review on #1227 and #1228 has flagged eventually-consistent projection
behavior as a bug and proposed SELECT-time locks, wider transactions, or compensating retry
markers. That class of "fix" is what produced the production deadlock clusters (#1213,
#1224) and the silent observation duplication (#1214). The new section draws the canonical
vs derived state line, lists the existing convergence mechanisms (next write, index pass,
reindex, doctor, cloud orphan sweeper), and defines the only shape of derived-state race
that warrants a finding: one that converges to a wrong state no existing mechanism repairs.
🤖 Generated with Claude Code
https://claude.ai/code/session_01CPdSXDbYyhyZ1TwgFnpEv8