[Bug]: setObjectIdsForTag() dispatches removed-object events twice, doubling cost for large removed-sets

[deividmodesto]

⚠️ This issue respects the following points: ⚠️

• This is a bug, not a question or a configuration/webserver/proxy issue.
• This issue is not already reported on Github OR Nextcloud Community Forum (I've searched it).
• Nextcloud Server is up to date. See Maintenance and Release Schedule for supported versions.
• I agree to follow Nextcloud's Code of Conduct.

Bug description

Description

OC\SystemTag\SystemTagObjectMapper::setObjectIdsForTag() dispatches the legacy
MapperEvent::EVENT_UNASSIGN event for every object in $removedObjectIds twice:
once right after the DELETE commits, and again at the very end of the method.

This is a straightforward code duplication bug — both loops iterate the exact same
$removedObjectIds array and dispatch the exact same event, with no behavior
difference between them.

Steps to reproduce

Steps to reproduce

1. Inspect lib/private/SystemTag/SystemTagObjectMapper.php
2. grep -n "removedObjectIds as" SystemTagObjectMapper.php returns two matches
   inside setObjectIdsForTag():
   • one right after the DELETE transaction commits
   • one at the very end of the method, under the comment
     "// Dispatch unassign events for removed object ids"

Expected behavior

Impact

Each dispatch(MapperEvent::EVENT_UNASSIGN, ...) call triggers real listener work
(e.g. apps/systemtags/lib/Activity/Listener.php writes an Activity feed entry per
call). For a tag with a large removed-set (e.g. thousands of objects — which happens
whenever a client submits a "set object ids for tag" PROPPATCH with a smaller/stale
list against a tag that currently has many more tagged objects, a completely normal
occurrence via the systemtags WebDAV collection endpoint), this doubles the method's
execution time.

Measured directly on a production instance (Nextcloud 34.0.1, MariaDB 10.6) using a
disposable test tag populated with 11,932 real object mappings, then submitting a
tiny 3-item object list (simulating a stale client view):

• With both loops present: ~37.7s total, ~18.3s of which was the removed-event
  dispatch loop alone (measured twice, matching the duplication)
• After removing the duplicate loop: ~18.8s total

Because the DELETE already commits early in the method (before either event-dispatch
loop runs), if the request is interrupted during this now-halved-but-still-long
window (execution timeout, worker recycling, etc.), the tag ends up with zero
mappings and no reinsert ever attempted — i.e. this contributes directly to
data-loss-shaped failures for tags with many objects, not just a performance
inconvenience.

We've also observed evidence suggesting a related concurrency race: because the
DELETE and the subsequent INSERT happen in two separate transactions with a real
gap between them (now ~18s even after removing the duplicate dispatch), a second
concurrent caller's initial getObjectIdsForTags() read can land in that gap and
compute its diff against a transiently near-empty table, compounding the problem
under concurrent usage of the same tag.

Suggested fix

Remove the second (redundant) loop at the end of setObjectIdsForTag(). Ideally,
also consider replacing the per-object dispatch() calls entirely with the already-
present batched dispatchTyped(new TagUnassignedEvent(...)) call, since that already
carries the full object list in one event and listeners could be updated to use it
instead of the legacy per-object event — this would remove the O(n) per-object
listener cost entirely rather than just halving it. Separately, wrapping the DELETE
and INSERT in a single transaction (with appropriate row locking) would close the
concurrency race described above.

Environment

• Nextcloud 34.0.1.2
• PHP 8.3
• MariaDB 10.6.22

[deividmodesto]

Thanks for the quick fix — can confirm the duplicate-loop removal is correct and
matches a patch we'd already applied locally (measured 37.7s → 18.8s on our
production instance, exactly as expected).

However, I want to flag that this fix alone did not fully resolve the underlying
data-loss problem for us. A few days after applying it, we hit the same
catastrophic wipe again (tag with ~12,000 objects collapsed to a handful), and this
time we captured evidence pointing to a genuine concurrency race, not just
single-request slowness.

We log every DELETE against oc_systemtag_object_mapping via a DB trigger (for
exactly this kind of forensics). Looking at the burst sizes leading up to the crash,
they climbed normally by ~1 each time as different users tagged individual files
(12005, 12006, 12007, ... 12010) — but one burst was anomalously only 9,000,
sandwiched between a 12,010 burst and a 12,011 burst moments later. I.e. the live
row count transiently dropped by ~3,000 mid-sequence and bounced back, before the
final collapse to near-zero shortly after.

Our read on this: because setObjectIdsForTag() still does its DELETE and INSERT
in two separate transactions with a real (if now shorter) gap between them, a
second concurrent caller's initial getObjectIdsForTags() read can land in that
gap — reading a transiently near-empty table — and compute its diff against that
stale/incomplete view. That caller's own reinsert then only contains what it
believes should be there, and if that reinsert is the one that lands last, it
overwrites everyone else's concurrent additions.

This means the risk scales with concurrent usage of the same tag, not just its
size — halving the per-call cost (this fix) narrows the window but doesn't close
it, and we'd expect it to resurface as usage grows even with this fix in place,
which matches what we observed.

Wrapping the DELETE+INSERT in a single transaction (with appropriate row locking,
e.g. SELECT ... FOR UPDATE on the relevant rows before diffing) would close the
race properly rather than just narrowing the window. Happy to share more detail on
our reproduction/monitoring setup if useful.