Fix racy tag count updates under concurrent tagging - #1329
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).
Tag.count was updated via a full-document read-modify-write (self.count += delta; self.save()), which loses updates when two callers tag/untag with the same tag concurrently: both read the same stale count, increment it locally, and save it back, so one increment is silently dropped instead of raising a conflict. Add Tag.increment_count(), which adjusts the count via a single self-contained AQL UPDATE (reusing the conflict-retry helper added in the link_to() atomicity fix), so the read and write happen in one server-side statement instead of racing across two round trips. It also republishes the same ObjectEvent that Tag.save() used to publish on every count change, since bypassing save() would otherwise silently drop that event. YetiTagModel.tag() now calls increment_count() instead of the racy count += 1 / count -= 1 + save() pattern. Verified via a concurrency regression test (20 objects tagging the same tag concurrently) that fails on the old code (undercounts) and passes with the fix. Note: Tag creation itself has a separate, pre-existing race -- concurrent Tag(name=...).save() calls for a brand new tag name can hit the unique index on tags.name with a write-write conflict rather than one of them finding the other's row. Out of scope here; left as a follow-up.
# Conflicts: # core/database_arango.py
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
1 participant
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.
Summary
Tag.countwas updated via a full-document read-modify-write(
self.count += delta; self.save()), which loses updates underconcurrent tagging: two callers tagging/untagging with the same tag at
the same time both read the same stale count, increment it locally,
and save it back — one increment is silently dropped instead of
raising a conflict.
This adds
Tag.increment_count(), which adjusts the count via asingle self-contained AQL
UPDATE, reusing the conflict-retry helper(
execute_aql_with_conflict_retry) introduced in #1328 for the sameclass of bug in
link_to()/link_to_acl(). The read and write happenin one server-side statement instead of racing across two client round
trips.
increment_count()also republishes the sameObjectEventthatTag.save()used to publish on every count change — bypassingsave()would otherwise silently drop that event, which the existingtest_publish_tag_eventregression test intests/core_tests/events.pycaught during verification.
YetiTagModel.tag()now callsincrement_count()instead of the racycount += 1/count -= 1+save()pattern, for both the tag-addand tag-removal (on
clear=True) paths.Depends on
Stacked on #1328 (
fix/link-to-atomic-upsert) — reusesexecute_aql_with_conflict_retry, which that PR introduces. Basebranch here is
fix/link-to-atomic-upsert, notmain; this should beretargeted to
mainonce #1328 merges (or merged after it).Out of scope: a separate tag-creation race
While verifying this fix, I found a different, pre-existing race in
tag creation (not count updates): concurrent
Tag(name=...).save()calls for a brand-new tag name can hit the unique index on
tags.namewith an ArangoDB write-write conflict (
[HTTP 409][ERR 1200]) ratherthan one caller finding the other's newly-created row. This is
timing-dependent and reproduces intermittently. It's a different
mechanism than the count race fixed here (it's about the initial
insert racing another initial insert, not a read-modify-write on an
existing document), so I've left it out of this PR as a follow-up.
Test plan
test_concurrent_tag_same_tag_count_is_atomictotests/schemas/tag.py: pre-creates the tag (to isolate from thecreation race above), then tags 20 different objects with it
concurrently via
ThreadPoolExecutor, asserting the final countis exactly 20.
6 != 20) andpasses with the fix.
tests/schemassuite: 190/190 pass.tests/apiv2suite: 198/200 pass (2 known pre-existingfailures in
tests/apiv2/tasks.py, unrelated to this change).tests/core_testssuite: 29/29 pass, includingtest_publish_tag_event, which caught the event-publishingregression during development (see above) and now passes.
ty check(core+yetictl and plugins jobs): 0 errors.ruff check/ruff format --check: clean.