Skip to content

fix(kb): serialize knowledge tree writes - #3358

Open
Sanderhoff-alt wants to merge 1 commit into
vectorize-io:mainfrom
Sanderhoff-alt:fix/kb-serialize-tree-writes
Open

fix(kb): serialize knowledge tree writes#3358
Sanderhoff-alt wants to merge 1 commit into
vectorize-io:mainfrom
Sanderhoff-alt:fix/kb-serialize-tree-writes

Conversation

@Sanderhoff-alt

@Sanderhoff-alt Sanderhoff-alt commented Aug 10, 2026

Copy link
Copy Markdown
Contributor

Summary

Serialize knowledge-base structural writes at the bank level so create, delete, and move operations cannot validate or rewrite the hierarchy from incompatible snapshots. This prevents concurrent opposite moves from both passing cycle detection and committing a parent loop.

Problem

Knowledge-base hierarchy checks previously ran without a shared serialization point. Two transactions moving A under B and B under A could both read the same pre-commit tree, independently pass the existing cycle guard, and then commit a cycle. Create and delete also change the same hierarchy, so serializing move alone would leave the structural-write invariant incomplete.

Design

Add _kp_lock_bank() and acquire a row lock on the owning bank before structural hierarchy reads or writes. Folder creation, page-node insertion, move, and delete all use the same lock, so structural writers for one bank execute against one committed tree state. Writers for different banks remain independent.

The lock uses FOR NO KEY UPDATE rather than FOR UPDATE. It still conflicts with another structural writer taking the same lock, but it does not conflict with the FOR KEY SHARE locks PostgreSQL takes for inserts into tables that reference the bank. This preserves writer serialization without unnecessarily blocking unrelated bank-scoped inserts or introducing the foreign-key lock interaction caused by the stronger lock. The Oracle adapter already rewrites this form to supported FOR UPDATE syntax.

Move takes the bank lock before validating the destination parent and reading the parent map. Delete takes the same lock before reading and removing a subtree. Create takes it before validating the parent and inserting the new hierarchy row. Non-structural operations such as rename do not take this lock.

Concurrency behavior

Step First move: A under B Second move: B under A
Acquire bank lock Proceeds Waits on the same bank row
Read hierarchy Sees both nodes at their original parents Runs after the first transaction commits
Validate cycle Passes Sees A.parent_id = B and rejects the move
Write Commits A.parent_id = B Does not commit a cycle

Test coverage

Add a deterministic regression test that pauses the first request after it acquires the real bank-row lock, verifies the second request reaches the same lock and cannot finish early, then confirms the first move succeeds and the second move is rejected. The test uses events rather than sleeps, making the required interleaving explicit and repeatable.

Validation

  • ./scripts/hooks/lint.sh
  • uv run pytest tests/test_knowledge_base.py -q -n 0 (40 passed)

Scope and tradeoffs

The serialization scope is one bank row, so structural writes within the same bank are intentionally serialized. This is coarser than locking individual paths, but it keeps a uniform lock order across create, delete, and move, makes the tree invariant straightforward to audit, and avoids locking every knowledge node.

This change is intentionally limited to preventing concurrent structural writes from creating a parent loop. It does not add defensive traversal for trees already corrupted before the fix, change page-creation failure compensation, or alter existing exception semantics.

@Sanderhoff-alt
Sanderhoff-alt force-pushed the fix/kb-serialize-tree-writes branch 4 times, most recently from c30334f to 65582f0 Compare August 10, 2026 16:05
Knowledge-tree create, delete, and move operations previously read and
updated the hierarchy without a common lock. Concurrent opposite moves
could both pass cycle detection against the same snapshot and commit a
parent loop.

Lock the bank row with FOR NO KEY UPDATE before structural reads and
writes. This serializes tree writers for one bank without conflicting
with the FOR KEY SHARE locks taken by unrelated foreign-key inserts.
The second opposite move now observes the first committed parent link
and is rejected by the existing cycle guard.

Add a deterministic concurrency test that holds the first bank lock,
verifies the second move cannot finish early, and checks that only the
first move succeeds.
@Sanderhoff-alt
Sanderhoff-alt force-pushed the fix/kb-serialize-tree-writes branch from 65582f0 to 716d10c Compare August 10, 2026 16:15
@handnewb

Copy link
Copy Markdown
Contributor

👋 Friendly suggestion from a contributor review: PRs #3358 (serialize writes), #3369 (atomic creation), and #3370 (compensation on failure) all address knowledge-base transaction safety. Consider consolidating these three into a single PR — it would reduce review overhead, prevent merge conflicts between them, and make the KB transaction story coherent. Each PR is valuable individually, but together they form a stronger atomicity guarantee.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants