fix(user_status): make setUserStatus() idempotent for concurrent duplicate requests#59913
fix(user_status): make setUserStatus() idempotent for concurrent duplicate requests#59913miaulalala wants to merge 1 commit into
Conversation
miaulalala
requested review from
ArtificialOwl,
artonge,
come-nc and
provokateurin
and removed request for
a team
miaulalala
force-pushed
the
fix/noid/user-status-phase2b-idempotent-set-status
branch
from
6597362 to
b3313cd
Compare
miaulalala
force-pushed
the
fix/noid/user-status-phase2b-idempotent-set-status
branch
2 times, most recently
from
b110271 to
4fc7729
Compare
| // If the active status already carries the same messageId, this is a | ||
| // duplicate/concurrent request for the same event — treat it as success. | ||
| try { | ||
| $currentStatus = $this->mapper->findByUserId($userId); |
There was a problem hiding this comment.
Don't we already have the user status as $userStatus?
There was a problem hiding this comment.
$userStatus was read at the top of the function, before backupCurrentStatus() ran.
In the concurrent race case, another request has already renamed the original status row to a backup (_alice), and inserted a new active row with the automated messageId.
Our in-memory $userStatus object reflects the DB state from before those writes, so it's stale. The fresh read after backupCurrentStatus() returns false is the only way to reliably see what's currently active — which is what we need to compare against $messageId for the idempotency check.
There was a problem hiding this comment.
I'm with artonge here and can not follow this.
If we assume a race condition after both requests have read $userStatus = $this->mapper->findByUserId($userId); in line 245. there is also no guarantee that the other process already finished it's insert yet?
The code from the read to the code point here is 4 boolean comparisons with 1 boolean assignment and 1 debug log in the winning/loosing case and then the one UPDATE query.
…icate requests When two automated events of the same type fire simultaneously (e.g. a calendar webhook is delivered twice, or two Talk clients both trigger a call status at the same time), both requests race through setUserStatus(). The first request succeeds: it creates a backup of the user's previous status and sets the new automated status. The second request finds the backup slot already occupied (unique constraint on user_id), so backupCurrentStatus() returns false. Previously this caused setUserStatus() to return null, which the caller interpreted as a failure even though the desired status was already correctly set by the first request. Fix: after backupCurrentStatus() returns false, re-read the current active status row. If it already carries the same messageId that we were trying to set, the operation is effectively complete — return the existing status instead of null. If it carries a different messageId (a higher-priority automated status is active), we still return null to abort as before. This also covers the meeting-with-call scenario: if a calendar BUSY status is active and a Talk call starts, the call correctly overrides it via the priority shortcut path. If the calendar fires a duplicate webhook while the call is already active, the re-read finds MESSAGE_CALL ≠ MESSAGE_CALENDAR_BUSY and correctly aborts rather than overriding the ongoing call. AI-Assisted-By: Claude Sonnet 4.6 <noreply@anthropic.com> Signed-off-by: Anna Larch <anna@nextcloud.com>
miaulalala
force-pushed
the
fix/noid/user-status-phase2b-idempotent-set-status
branch
from
4fc7729 to
428fe31
Compare
|
/backport to stable34 |
|
/backport to stable33 |
|
/backport to stable32 |
nickvergessen
left a comment
nickvergessen
left a comment
There was a problem hiding this comment.
I'd really like to review this as maintainer give me some more time 🙈
7 participants
Summary
When two automated events of the same type fire simultaneously (e.g. a calendar webhook is delivered twice, or two Talk clients both trigger a call status at the same time), both requests race through setUserStatus().
The first request succeeds: it creates a backup of the user's previous status and sets the new automated status. The second request finds the backup slot already occupied (unique constraint on user_id), so backupCurrentStatus() returns false. Previously this caused setUserStatus() to return null, which the caller interpreted as a failure even though the desired status was already correctly set by the first request.
Fix: after backupCurrentStatus() returns false, re-read the current active status row. If it already carries the same messageId that we were trying to set, the operation is effectively complete — return the existing status instead of null. If it carries a different messageId (a higher-priority automated status is active), we still return null to abort as before.
This also covers the meeting-with-call scenario: if a calendar BUSY status is active and a Talk call starts, the call correctly overrides it via the priority shortcut path. If the calendar fires a duplicate webhook while the call is already active, the re-read finds MESSAGE_CALL ≠ MESSAGE_CALENDAR_BUSY and correctly aborts rather than overriding the ongoing call.
AI-Assisted-By: Claude Sonnet 4.6 noreply@anthropic.com
Checklist
3. to review, feature component)stable32)AI (if applicable)