fix: generation-versioned relation persistence - #1220
Conversation
43bfb5b to
a4cc135
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43bfb5b0c6
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9d7897ba7f
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1fe16ef022
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c21102d570
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2f72e9594
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 48398846d1
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 66b74dc590
ℹ️ 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".
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7dc9a23922
ℹ️ 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".
|
Windows CI triage for job 93312513075: Group A — inherited from main, not branch-owned. The failed-job log contains 12 (not 11) Group B — branch-owned, but not a path-separator identity bug. All three actual assertions fail because The fix is at that producing boundary: after normalization/formatting, return content and checksum from one binary read of the bytes that actually reached disk. The regression simulates Windows CRLF persistence and asserts both returned content bytes and checksum match disk. This preserves the strict snapshot/claim fence; it does not weaken equality or normalize the tests. The three failed end-to-end paths are in the pending verification set and will rerun in Windows CI after the push. |
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
Signed-off-by: phernandez <paul@basicmachines.co>
7dc9a23 to
6d11ed3
Compare
|
Exhaustive relation-generation race audit at I swept the codebase for both recurring bug classes: (a) any reachable transaction that can take an Entity or Relation lock before NoteContent when both domains are touched, using Lock-order paths checked:
Snapshot/generation paths checked:
Windows CI triage is recorded separately in the earlier comment. Group A reproduced identically on main and is fixed by merged PR #1221, now included by this rebase. Group B's three assertions all showed empty outgoing relations; the cause was the LF in-memory/CRLF persisted-byte mismatch above, not separator-sensitive relation identity or permalink construction. Verification on the rebased head:
|
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
When a note is moved after this unlocked snapshot is read but before delete_directory_entities() acquires its NoteContent locks, the move can commit the entity's new path and the delete then still removes that entity by the stale snapshotted ID. The active /delete-directory flow consequently deletes DB state for a note that no longer belongs to the requested directory and queues cleanup using its old path; depending on materialization timing, this can also lose the moved note. After locking NoteContent, re-query and lock the entities using the directory predicate, then delete only those that still match.
ℹ️ 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 Codex review 4892873946 in The directory snapshot remains intentionally unlocked, and this adds no new lock construct. After the existing sorted NoteContent source fence, the store revalidates the snapshotted IDs against the current project and directory prefix without locking. The final Entity mutation repeats all three predicates in-statement:
It uses Regression coverage runs on both backends and proves the stale snapshotted ID becomes a no-op after move-out, preserving both Entity and NoteContent at the destination. The SQL-shape test separately asserts that the final DELETE itself contains the ID, project, and directory predicates. Verification:
GitHub emitted 4892873946 as a review summary rather than an inline thread, so there is no review-thread node to resolve. The live PR currently has zero unresolved review threads; the two prior pending threads are resolved. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6987500df2
ℹ️ 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".
Route project-index relation writes through the generation-guarded repository, preserve repair work when a search refresh loses its generation, and reject directory targets with uncaptured incoming relations without adding locks.\n\nThe Windows CRLF failures were test I/O normalization, not production permalink rewriting: the rewrite preserved the fixture's CRLF bytes while text-mode reads normalized them to LF. Compare the affected assertions with persisted bytes. Signed-off-by: phernandez <paul@basicmachines.co>
|
Windows CI follow-up for job 93320769061, fixed in 659842f:\n\nThe two CRLF failures were test-I/O normalization, not production permalink-rewrite churn. On Windows the fixture Path.write_text call writes CRLF and the indexing path reads and preserves those bytes. The assertions then called file_service.read_file_content, whose text-mode universal-newline read normalized the persisted CRLF to LF. The affected assertions now compare indexed markdown with persisted bytes, matching the contract they intend to verify.\n\nLocal verification: both affected tests pass; just fast-check, focused SQLite and PostgreSQL suites, just fast-test, and just doctor are green. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 659842f06c
ℹ️ 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".
Why
Concurrent indexing currently replaces each source entity's outgoing relations with a
delete-then-insert transaction. Mutual links can make those transactions acquire source entity,
relation, foreign-key, and uniqueness locks in opposite orders, producing the production
PostgreSQL deadlocks captured in #1213. Retrying identical work does not change that lock graph.
Closes #1213
What Changed
relation.generationcolumn, backfill migration, and source/generation index.followed by a separate stale-generation cleanup statement.
relation publication after the entity/content transaction commits.
to_id = NULL) and made the shared resolver exclusively owngeneration-fenced
to_idbackfill.EntityService paths.
exemption.
coverage, retry-loop regressions, and the existing 10,000-target resolver budget test on the
generation-aware setup.
Implementation Details
Ordered generations and source fences
The generation token is the existing ordered
note_content.db_version, not a content checksum.The publisher needs a monotonic ordering to reject stale work; a checksum can establish identity
but cannot say which write is newer.
Every relation upsert chunk and cleanup repeats a source-level
note_content.db_version = generationpredicate. The per-row conflict guard alone is not enough:without the source fence, an older publisher could still insert a relation key that is absent from
the newer generation.
Name-identity upserts use the existing unique constraint on
(from_id, to_name, relation_type)as theON CONFLICTarbiter. Upserts accept only an equal ornewer row generation, reset
to_idtoNULL, and run in deterministic bounded chunks. Cleanup isa separate transaction and deletes only older generations while the same source-generation fence
still holds.
SQLAlchemy renders the source-row
FOR UPDATEfence on PostgreSQL and omits it on SQLite. SQLite'ssingle-writer serialization preserves the fence's meaning; both older/newer race directions run
against both backends.
Publication and resolution boundaries
Entity/content reconciliation now claims the accepted
db_versionbefore publication. Acceptedcreate, update, edit, and move flows commit their entity/content/search/observation state first,
then publish relations in sorted bounded transactions. A
deferredreconciliation means the filelineage is older than accepted content, so both retry loops now finish successfully without
publishing instead of retrying identical bytes five times.
Relations remain unresolved during persistence. Resolver plans carry the observed source
generation, preserve customer-authored
to_name, and update onlyto_idunder the sourcegeneration predicate. The real mutual-link test exposed an explicit target-row lock inversion;
resolution now relies on the target external-ID predicate and the foreign-key key-share lock
acquired by the guarded mutation instead of taking the conflicting explicit target lock.
Legacy caller disposition
replace_accepted_outgoing_relations: no production caller after the accepted-write inversion;removed.
update_entity_relations: reachable only through undeployedupsert_entity_from_markdown;removed, with the supported EntityService write surface moved to generation publication.
delete_outgoing_relations_from_entity: no source caller; removed.add_all_ignore_duplicates: no source caller, only legacy tests/scaling setup; removed. It alsosilently discarded
Relation.generation, so it always wrote generation 0.There is no single-writer exemption. A generation-0 row written by a legacy path would be deleted
as superseded by a later generation cleanup on the same database, so leaving any mixed writer was
a correctness hole.
Testing
Automated
just fast-check: passed (Ruff formatting/lint and type checking).just test: returned 0 under the repository gate. SQLite unit and integration completed; theSQLite integration result was 466 passed, 9 skipped, and 29 deselected. PostgreSQL unit
completed. PostgreSQL integration reached 30% with no assertion failure before the recipe's
intentional 600-second timeout sent signal 9;
test-int-postgresexplicitly accepts exit 137because of the linked FastMCP issue, so this is not represented as all 474 cases passing.
case skipped.
batch publication and local retry flows.
33 passed. The real-PostgreSQL 10,000-target resolver test stayed within its unchanged 30-query
budget.
just doctor: passed on the final head, including migrations and the file-to-database indexingloop.
Risks / Follow-ups
indexing worker before enabling generation publishers. Full generation guarantees begin only
after no legacy writer remains.
services in-process.
issue-owned PostgreSQL concurrency, repository, resolver, and scaling paths were run directly
and passed as described above.
Reviewer Guide
The commit boundaries are deliberate. In particular, review the generation-claim inversion in
8d1db15cand the accepted-content-first publication change in47b1cd52independently from theschema/repository foundation and resolver changes.