fix: make link_to()/link_to_acl() atomic under concurrent callers - #1328
Merged
Conversation
Both methods do check-then-act: query whether an edge already exists between (source, target[, type]), then either update it or create a new one. Under concurrent calls for the same pair (e.g. two feed workers linking the same observable/entity, or rbac.set_acls() called concurrently), two callers can both see "no edge yet" and each create one -- producing duplicate edges instead of one edge with an accurate count, plus the count updates on top of that race independently (lost updates on the read-modify-write .count += 1). Reproduced empirically before this fix: 20 concurrent link_to() calls for the same pair produced 3 duplicate edges whose counts summed to less than 20 (a second, compounding race on top of the duplicate-edge one). Fix: replace the check-then-act with a single atomic ArangoDB AQL UPSERT per call (match on _from/_to[/type], INSERT if absent, UPDATE count/description/ modified if present). ArangoDB evaluates a single-document UPSERT atomically server-side -- there's no client-side read to go stale -- but two genuinely concurrent writers to the *same* document can still get ArangoDB's own write-write conflict rejection (error 1200) rather than a silent lost update; added a small retry helper for that case (verified: without retry, 50 concurrent increments on one counter reproducibly hit this; with retry, they don't). Both methods keep their exact external contract: same return type/shape (Relationship/RoleRelationship, loaded via .load() as before), same LinkEvent semantics (EventType.new vs .update, derived from whether the UPSERT's OLD pseudo-variable is null -- verified this is exactly null on insert, the previous document on update), same collection-name event-skip guard. The pre-existing "new" branch already used col.link() through the async-execution context; the replacement uses a direct sync AQL call (matching what the existing existence-check query in the same method already did) -- this doesn't touch or resolve the separate, currently-paused question of de-asyncing the rest of the connector. Added concurrency regression tests (tests/schemas/graph.py, tests/schemas/rbac.py): 20 concurrent callers linking the same pair now collapse to exactly one edge with the correct count/role. Verified both fail against the pre-fix code (2 edges each) and pass against the fix. Found and deliberately left out of scope: deleting a RoleRelationship object always fails to publish its deletion event (core/events/message.py's YetiObjectTypes discriminated union has no "acl" branch for it, only "relationship" for the plain Relationship) -- pre-existing, unrelated to this change (link_to_acl() itself never published events even before this fix), silently swallowed by delete()'s broad except. Worth a follow-up. Verified: both ty jobs 0 errors; ruff check + format clean; tests/schemas 189/189 (full suite, incl. the 2 new tests), tests/core_tests 29/29, tests/apiv2 198/200 (2 pre-existing tasks.py failures, confirmed unrelated across every PR in this batch). From the backend architecture review, item #7 (transactions/racy denormalized counters) -- the two spots it named (tag() and link_to()) as having user-visible atomicity gaps. tag()'s count race is a separate, smaller follow-up (core/schemas/model.py, not the connector).
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Both
link_to()andlink_to_acl()do check-then-act: query whether an edge already exists between(source, target[, type]), then either update it or create a new one. Under concurrent calls for the same pair — two feed workers linking the same observable/entity, orrbac.set_acls()called concurrently — two callers can both see "no edge yet" and each create one, producing duplicate edges instead of one edge with an accurate count. The count update on top of that races independently too (classic lost-update on.count += 1).Reproduced empirically before this fix: 20 concurrent
link_to()calls for the same pair produced 3 duplicate edges whose counts summed to less than 20 — two compounding races, not one.Fix
Replace the check-then-act with a single atomic ArangoDB AQL UPSERT per call (match on
_from/_to[/type],INSERTif absent,UPDATEcount/description/modified if present). ArangoDB evaluates a single-document UPSERT atomically server-side — there's no client-side read to go stale — but two genuinely concurrent writers to the same document can still hit ArangoDB's own write-write-conflict rejection (error 1200) rather than a silent lost update. Added a small retry helper for that case — verified: without it, 50 concurrent increments on one counter reproducibly hit this conflict; with it, they don't and land exactly correct.Both methods keep their exact external contract: same return type/shape, same
LinkEventsemantics (EventType.newvs.update, derived from whether the UPSERT'sOLDpseudo-variable is null — verified this is exactly null on insert, the previous document on update), same collection-name event-skip guard.The replacement uses a direct sync AQL call (matching what the existing existence-check query in the same method already did) rather than the async-execution context the old "create" branch used — this doesn't touch or resolve the separate, currently-paused question of de-asyncing the rest of the connector (see
plans/backend-architecture-review.md).Tests
Added concurrency regression tests (
tests/schemas/graph.py,tests/schemas/rbac.py): 20 concurrent callers linking the same pair collapse to exactly one edge with the correct count/role. Verified both fail against the pre-fix code (2 duplicate edges each) and pass against the fix.Found and deliberately left out of scope
Deleting a
RoleRelationshipobject always fails to publish its deletion event —core/events/message.py'sYetiObjectTypesdiscriminated union has no"acl"branch for it (only"relationship"for the plainRelationship). Pre-existing, unrelated to this change (link_to_acl()itself never published events even before this fix), silently swallowed bydelete()'s broadexcept. Worth a follow-up.Verification
ruff check+ruff format --check: cleantests/schemas189/189 (full suite, incl. the 2 new tests),tests/core_tests29/29,tests/apiv2198/200 (2 pre-existingtasks.pyfailures, confirmed unrelated across every PR in this batch)From the backend architecture review, item #7 (transactions/racy denormalized counters) — the two spots it named as having user-visible atomicity gaps.
tag()'s count race (same item, different file) is a separate, smaller follow-up.