Skip to content

fix(user_status): make setUserStatus() idempotent for concurrent duplicate requests #59913

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
miaulalala wants to merge 1 commit into
base: master
Choose a base branch
from
+45 −2
Open

fix(user_status): make setUserStatus() idempotent for concurrent duplicate requests #59913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion apps/user_status/lib/Service/StatusService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,7 +285,18 @@ public function setUserStatus(string $userId,

if ($createBackup) {
if ($this->backupCurrentStatus($userId) === false) {
return null; // Already a status set automatically => abort.
// A backup already exists, meaning another automated status is active.
// 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);

@artonge artonge May 12, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't we already have the user status as $userStatus?

@miaulalala miaulalala Jun 24, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$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.

@nickvergessen nickvergessen Jun 26, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

if ($currentStatus->getMessageId() === $messageId) {
return $currentStatus;
}
} catch (DoesNotExistException) {
// No active status row exists, fall through to abort
}
return null; // A different automated status is active => abort.
}

// If we just created the backup
Expand Down
34 changes: 33 additions & 1 deletion apps/user_status/tests/Unit/Service/StatusServiceTest.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -803,7 +803,7 @@ public function testSetUserStatus(string $messageId, string $oldMessageId, bool
$previous->setUserId('john');
$previous->setIsBackup(false);

$this->mapper->expects($this->once())
$this->mapper->expects($this->exactly($expectedUpdateShortcut ? 1 : 2))
->method('findByUserId')
->with('john')
->willReturn($previous);
Expand All @@ -826,4 +826,36 @@ public function testSetUserStatus(string $messageId, string $oldMessageId, bool

$this->service->setUserStatus('john', IUserStatus::DND, $messageId, true);
}

public function testSetUserStatusIdempotentOnConcurrentDuplicateRequest(): void {
$existing = new UserStatus();
$existing->setId(1);
$existing->setStatus(IUserStatus::DND);
$existing->setStatusTimestamp(1337);
$existing->setIsUserDefined(true);
$existing->setMessageId(IUserStatus::MESSAGE_CALL);
$existing->setUserId('john');
$existing->setIsBackup(false);

$this->mapper->expects($this->exactly(2))
->method('findByUserId')
->with('john')
->willReturn($existing);

/** @var MockObject&Exception $exception */
$exception = $this->createMock(Exception::class);
$exception->method('getReason')->willReturn(Exception::REASON_UNIQUE_CONSTRAINT_VIOLATION);
$this->mapper->expects($this->once())
->method('createBackupStatus')
->willThrowException($exception);

$this->predefinedStatusService->expects($this->once())
->method('isValidId')
->with(IUserStatus::MESSAGE_CALL)
->willReturn(true);

// Active status already has MESSAGE_CALL — concurrent duplicate request should succeed
$result = $this->service->setUserStatus('john', IUserStatus::DND, IUserStatus::MESSAGE_CALL, true);
$this->assertSame($existing, $result);
}
}
Loading