Skip to content

fix(store): workspace-level guard for N-hop parent cycles (BUG-2074)#871

Merged
xarmian merged 1 commit into
mainfrom
fix/parent-cycle-nhop
Jul 8, 2026
Merged

fix(store): workspace-level guard for N-hop parent cycles (BUG-2074)#871
xarmian merged 1 commit into
mainfrom
fix/parent-cycle-nhop

Conversation

@xarmian

@xarmian xarmian commented Jul 8, 2026

Copy link
Copy Markdown
Collaborator

Summary

BUG-2073 (PR #870) closed the 1-hop A↔B parent-cycle race by folding the child key into a sorted per-endpoint lock batch. But that batch only covers the two endpoints of the edge being written, so an N-hop cycle closed via an edge on an item that neither endpoint locks still slipped past the per-endpoint checkParentCycleQ walk under concurrency (Postgres-only; SQLite serializes all writers via BEGIN IMMEDIATE).

Concrete reproduction: with A→B and C→D already committed, two concurrent adds SetParentLink(B,C) and SetParentLink(D,A) lock the disjoint sets {B,C} and {D,A}, so both cycle walks pass on stale snapshots and both inserts commit — forming A→B→C→D→A.

Fix — workspace-level cycle guard

  • acquireWorkspaceParentLinkLock — a Postgres advisory xact lock on a distinct namespace (pad:parent-link-cycle: || workspaceID) that serializes all parent-edge-adding transactions per workspace. With no two adds running concurrently, checkParentCycleQ always walks a consistent, non-racing ancestor snapshot and catches arbitrary N-hop cycles.
  • Acquired outermost (before the seq lock and the per-item parent-children batch) in the edge-adding paths — SetParentLink and UpdateItemWithParentLink (only when it actually adds a parent). Global order is cycle → seq → parent-children in every transaction that takes them, so no AB/BA inversion can form. Deadlock-free.
  • CreateItemLink was a second, unguarded parent-edge adder: link_type="parent" appended a raw row with no cycle check and no workspace lock — it could form cycles even single-threaded and give a child multiple parent rows (the schema only uniques (source_id,target_id,link_type)), which the single-chain cycle walk can't see. It now routes parent links through SetParentLink, inheriting the full guarded protocol (single-parent DELETE-then-INSERT, workspace cycle lock, cycle check, retry). Non-parent link types keep the append-only insert — none are followed by the cycle walk.
  • Scope is edge-adders only. Removals and plain field/status updates can't create a cycle and don't take the lock, so the common UpdateItem path stays un-serialized. BUG-2073's per-endpoint locks remain (they still bound the open-children re-read invariant); the workspace lock is the outer guard.
  • Rejected alternative (locking the full ancestor chain per write): complex, deadlock-prone under concurrent reparents, hard to keep in canonical sorted order.

Tests (Postgres-gated where concurrent)

  • TestSetParentLink_ConcurrentNHopNoCycle + TestCreateItemLink_ConcurrentNHopNoCycle — build the disjoint-lock A→B→C→D→A quad via parallel goroutines and assert no cycle forms. Both verified to reproduce the bug with the guard disabled (6/64 and 14/64 quads cycle) and pass with it.
  • TestCreateItemLink_ParentSingleParentAndCycle (dialect-agnostic) — asserts CreateItemLink(parent) now enforces single-parent (latest wins) and rejects a direct cycle.

Gates

~/go/bin/golangci-lint run (0 issues), go test ./..., and the full Postgres suite (make test-pg equivalent) all green. Two rounds of Codex review addressed (CreateItemLink hole → cycle check + delegation); final review CLEAN.

https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS

BUG-2073 (PR #870) closed the 1-hop A<->B parent-cycle race by folding
the child key into a sorted per-endpoint lock batch and re-reading under
lock. But that batch only covers the two endpoints of the edge being
written, so a cycle closed via an edge on an item that NEITHER endpoint
locks still slips through the per-endpoint checkParentCycleQ ancestor
walk. Concrete N-hop reproduction (Postgres): with A->B and C->D already
committed, two concurrent adds SetParentLink(B,C) and SetParentLink(D,A)
lock the DISJOINT sets {B,C} and {D,A}, so both cycle walks pass on stale
snapshots and both inserts commit, forming A->B->C->D->A.

Fix (workspace-level cycle guard):

- New acquireWorkspaceParentLinkLock — a Postgres advisory xact lock keyed
  on a DISTINCT namespace ('pad:parent-link-cycle:' || workspaceID) that
  serializes ALL parent-edge-ADDING transactions in a workspace. With no
  two adds running concurrently, checkParentCycleQ always walks a
  consistent, non-racing ancestor snapshot and catches arbitrary N-hop
  cycles.
- Acquired OUTERMOST (before the seq lock and the per-item parent-children
  batch) in the parent-edge-adding paths: setParentLinkOnce (SetParentLink)
  and updateItemWithParentLinkOnce (UpdateItemWithParentLink, only when it
  actually adds a parent). Global lock order is therefore
  cycle -> seq -> parent-children in every transaction that takes them, so
  no AB/BA inversion can form.
- CreateItemLink was the SECOND parent-edge adder and a hole: with
  link_type="parent" it appended a raw parent row with NO cycle check and
  NO workspace lock — it could form cycles even single-threaded, could give
  a child multiple parent rows (the schema only uniques
  (source_id,target_id,link_type)), and checkParentCycleQ follows only ONE
  arbitrary parent per source so the extra row could hide an N-hop cycle.
  Now CreateItemLink routes link_type="parent" through SetParentLink, which
  gives it the full guarded protocol: single-parent DELETE-then-INSERT, the
  workspace cycle lock + checkParentCycleQ under lock, and the
  errParentSetChanged retry wrapper. Non-parent link types (blocks /
  supersedes / implements / related) keep the append-only INSERT — none are
  followed by the cycle walk, so none can form a cycle.
- Scope is edge-ADDERS only: edge removals (clear/detach) and plain field
  updates / status flips can't create a cycle, so they don't take the lock
  — the common UpdateItem path stays un-serialized. The BUG-2073
  per-endpoint locks remain (they still bound the re-read-under-lock
  open-children invariant); the workspace lock is the outer guard that
  closes the N-hop gap.
- Rejected alternative (locking the full ancestor chain per write):
  complex, deadlock-prone under concurrent reparents, hard to keep in
  canonical sorted order.

Postgres-only for the concurrency races (SQLite serializes all writers via
BEGIN IMMEDIATE). Tests:
- TestSetParentLink_ConcurrentNHopNoCycle and
  TestCreateItemLink_ConcurrentNHopNoCycle build the disjoint-lock
  A->B->C->D->A quad via parallel goroutines and assert no cycle forms;
  both verified to reproduce the bug with the guard disabled (6/64 and
  14/64 quads cycle) and pass with it.
- TestCreateItemLink_ParentSingleParentAndCycle (dialect-agnostic) asserts
  CreateItemLink(parent) now enforces single-parent (latest wins) and
  rejects a direct cycle.
golangci-lint, go test ./..., and the full Postgres suite all green.

Claude-Session: https://claude.ai/code/session_019knGmnHcx5rrgWXQ8V8DZS
@xarmian xarmian merged commit e071b62 into main Jul 8, 2026
3 of 4 checks passed
@xarmian xarmian deleted the fix/parent-cycle-nhop branch July 8, 2026 18:46
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.

1 participant