From aff361e9a3a0052bc3b47519ef37ad9f760db530 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 1/4] fix(user_status): clear unreachable automated status when there is no backup revertUserStatus() bailed out as soon as no backup row was found, leaving the automated status on the live row. Nothing else ever removes it, so the user is stuck: setting themselves online manually is cleaned to offline 15 minutes later, and UserLiveStatusListener returns early for MESSAGE_CALENDAR_BUSY so no heartbeat can undo it. Delete the live row instead when its message id still matches the automation being reverted. A status the user has since changed themselves no longer matches and is left untouched. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch --- .../user_status/lib/Service/StatusService.php | 8 +- .../Service/StatusServiceIntegrationTest.php | 110 +++++++++++++++++- .../tests/Unit/Service/StatusServiceTest.php | 44 +++++++ 3 files changed, 159 insertions(+), 3 deletions(-) diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 6c1805963c881..4ac80965ad0be 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -530,7 +530,13 @@ public function revertUserStatus(string $userId, string $messageId, bool $revert /** @var UserStatus $userStatus */ $backupUserStatus = $this->mapper->findByUserId($userId, true); } catch (DoesNotExistException $ex) { - // No user status to revert, do nothing + // There is no backup to restore. The automated status still has to + // go, otherwise the user is stuck on it forever: UserLiveStatusListener + // refuses to overwrite an automated status, so no heartbeat can ever + // bring them back online. + if ($this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId)) { + $this->logger->debug('Cleared automated status "' . $messageId . '" for user ' . $userId . ': there was no backup to restore', ['app' => 'user_status']); + } return null; } diff --git a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php index a2acfe4458b7d..85aeeb97192e4 100644 --- a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php +++ b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php @@ -9,6 +9,8 @@ namespace OCA\UserStatus\Tests\Integration\Service; +use OCA\UserStatus\Db\UserStatus; +use OCA\UserStatus\Db\UserStatusMapper; use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\IDBConnection; @@ -22,17 +24,32 @@ class StatusServiceIntegrationTest extends TestCase { private StatusService $service; + private UserStatusMapper $mapper; + private IDBConnection $db; protected function setUp(): void { parent::setUp(); $this->service = Server::get(StatusService::class); + $this->mapper = Server::get(UserStatusMapper::class); - $db = Server::get(IDBConnection::class); - $qb = $db->getQueryBuilder(); + $this->db = Server::get(IDBConnection::class); + $qb = $this->db->getQueryBuilder(); $qb->delete('user_status')->executeStatement(); } + /** + * Reads a row without going through StatusService::processStatus(), which + * would rewrite a stale status before the assertion can see it. + */ + private function readRaw(string $userId): ?UserStatus { + try { + return $this->mapper->findByUserId($userId); + } catch (DoesNotExistException) { + return null; + } + } + public function testNoStatusYet(): void { $this->expectException(DoesNotExistException::class); @@ -191,4 +208,93 @@ public function testOtherAutomationsDoNotOverwriteEachOther(): void { $this->service->findByUserId('test123')->getMessageId(), ); } + + /* + * Orphaned automated statuses: a live row sits on an automated status but + * there is no backup row to revert into, so revertUserStatus() has nothing + * to restore. It must still clear the automated status, otherwise the user + * is stuck on it forever and the heartbeat can never bring them back + * online. + */ + + public function testRevertWithoutBackupClearsAutomatedStatus(): void { + // No backup taken, so nothing can ever be restored for this user. + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + false, + ); + self::assertSame( + IUserStatus::MESSAGE_CALENDAR_BUSY, + $this->readRaw('test123')?->getMessageId(), + ); + + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull($reverted, 'Nothing can be restored without a backup'); + self::assertNull( + $this->readRaw('test123'), + 'The unreachable automated status must be cleared, not left behind', + ); + } + + public function testRevertWithoutBackupKeepsStatusTheUserChangedThemselves(): void { + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + false, + ); + // The user replaces the automated message with their own. + $this->service->setCustomMessage('test123', '🍕', 'Lunch', null); + + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull($reverted); + $status = $this->readRaw('test123'); + self::assertNotNull($status, 'A status the user set themselves must not be deleted'); + self::assertSame('Lunch', $status->getCustomMessage()); + self::assertNull($status->getMessageId()); + } + + public function testRevertWithoutBackupKeepsOtherAutomatedStatus(): void { + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALL, + false, + ); + + // The meeting automation reverts, but the live status belongs to a call. + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull($reverted); + self::assertSame( + IUserStatus::MESSAGE_CALL, + $this->readRaw('test123')?->getMessageId(), + 'An unrelated automated status must be left alone', + ); + } + + public function testFreshUserAutomatedStatusIsClearedOnRevert(): void { + // A user who has never had a status row: there is nothing to back up, + // so the automated status is applied without a backup. + $applied = $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + self::assertNotNull($applied, 'A user without a previous status should still get the meeting status'); + self::assertNull($this->readRaw('_test123'), 'There was no status to back up'); + + $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull( + $this->readRaw('test123'), + 'The meeting status must be cleared when the meeting ends', + ); + } } diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index 69128033631e0..756e851f39c29 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -700,6 +700,50 @@ public function testBackup(): void { $this->assertTrue($this->service->backupCurrentStatus('john')); } + public function testBackupNothingToBackUp(): void { + // A user without a status row has nothing to move into a backup. That + // is not a conflict, so the automated status may still be applied. + $this->mapper->expects($this->once()) + ->method('createBackupStatus') + ->with('john') + ->willReturn(false); + + $this->assertTrue($this->service->backupCurrentStatus('john')); + } + + public function testRevertUserStatusWithoutBackupClearsAutomatedStatus(): void { + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john', true) + ->willThrowException(new DoesNotExistException('')); + + // There is nothing to restore, but the unreachable automated status + // must still be removed so the user is not stuck on it. + $this->mapper->expects($this->once()) + ->method('deleteCurrentStatusToRestoreBackup') + ->with('john', 'meeting') + ->willReturn(true); + + $this->assertNull($this->service->revertUserStatus('john', 'meeting')); + } + + public function testRevertUserStatusWithoutBackupAndNoMatchingStatus(): void { + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john', true) + ->willThrowException(new DoesNotExistException('')); + + // Nothing matched, so nothing was removed. Must not blow up. + $this->mapper->expects($this->once()) + ->method('deleteCurrentStatusToRestoreBackup') + ->with('john', 'meeting') + ->willReturn(false); + + $this->mapper->expects($this->never())->method('update'); + + $this->assertNull($this->service->revertUserStatus('john', 'meeting')); + } + public function testRevertMultipleUserStatus(): void { $john = new UserStatus(); $john->setId(1); From 6a857b51cbff769f74243906a1e952c0ecea5fc7 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 2/4] fix(user_status): refresh the status timestamp when restoring a backup A backup keeps the status_timestamp it had when the automation took over, so any automated status lasting longer than INVALIDATE_STATUS_THRESHOLD is restored already stale and rewritten to offline by the very next read. Stamp the restored status with the time of the revert, as the manual revert path already did. A user who really went away now stays online for up to INVALIDATE_STATUS_THRESHOLD instead, which is the better failure mode. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch --- .../user_status/lib/Service/StatusService.php | 15 +++-- .../Service/StatusServiceIntegrationTest.php | 51 +++++++++++++++++ .../tests/Unit/Service/StatusServiceTest.php | 56 +++++++++++++++++++ 3 files changed, 116 insertions(+), 6 deletions(-) diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 4ac80965ad0be..739ec7cc9c653 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -546,14 +546,17 @@ public function revertUserStatus(string $userId, string $messageId, bool $revert return null; } - if ($revertedManually) { - if ($backupUserStatus->getStatus() === IUserStatus::OFFLINE) { - // When the user reverts the status manually they are online - $backupUserStatus->setStatus(IUserStatus::ONLINE); - } - $backupUserStatus->setStatusTimestamp($this->timeFactory->getTime()); + if ($revertedManually && $backupUserStatus->getStatus() === IUserStatus::OFFLINE) { + // When the user reverts the status manually they are online + $backupUserStatus->setStatus(IUserStatus::ONLINE); } + // The restored status becomes the current one now. Keeping the timestamp + // from before the automation would make it instantly stale for anything + // longer than INVALIDATE_STATUS_THRESHOLD, so the next read would clean + // the user straight to offline. + $backupUserStatus->setStatusTimestamp($this->timeFactory->getTime()); + $backupUserStatus->setIsBackup(false); // Remove the underscore prefix added when creating the backup $backupUserStatus->setUserId(substr($backupUserStatus->getUserId(), 1)); diff --git a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php index 85aeeb97192e4..cd7fece50bf5d 100644 --- a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php +++ b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php @@ -50,6 +50,14 @@ private function readRaw(string $userId): ?UserStatus { } } + /** Simulates elapsed time by ageing the stored timestamps backwards. */ + private function age(string $userId, int $seconds): void { + $this->db->executeStatement( + 'UPDATE `*PREFIX*user_status` SET `status_timestamp` = `status_timestamp` - ? WHERE `user_id` IN (?, ?)', + [$seconds, $userId, '_' . $userId], + ); + } + public function testNoStatusYet(): void { $this->expectException(DoesNotExistException::class); @@ -297,4 +305,47 @@ public function testFreshUserAutomatedStatusIsClearedOnRevert(): void { 'The meeting status must be cleared when the meeting ends', ); } + + public function testRevertAfterLongMeetingRefreshesTimestamp(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + // A 90 minute meeting, well past INVALIDATE_STATUS_THRESHOLD. + $this->age('test123', 90 * 60); + + $before = time(); + $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNotNull($reverted); + self::assertGreaterThanOrEqual( + $before, + $this->readRaw('test123')?->getStatusTimestamp(), + 'A restored status must not carry the stale timestamp from before the meeting', + ); + } + + public function testRevertAfterLongMeetingDoesNotFallBackToOffline(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + $this->age('test123', 90 * 60); + + $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); + + // findByUserId() runs processStatus(), which cleans stale statuses. + self::assertSame( + IUserStatus::ONLINE, + $this->service->findByUserId('test123')->getStatus(), + 'The user was online before the meeting and must not be flipped to offline by reading the status', + ); + } } diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index 756e851f39c29..b42370991503c 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -744,6 +744,62 @@ public function testRevertUserStatusWithoutBackupAndNoMatchingStatus(): void { $this->assertNull($this->service->revertUserStatus('john', 'meeting')); } + public function testRevertUserStatusRefreshesTimestamp(): void { + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setStatus(IUserStatus::ONLINE); + $backup->setStatusTimestamp(1000); + $backup->setIsUserDefined(false); + $backup->setIsBackup(true); + + $this->mapper->expects($this->once()) + ->method('findByUserId') + ->with('john', true) + ->willReturn($backup); + $this->mapper->expects($this->once()) + ->method('deleteCurrentStatusToRestoreBackup') + ->with('john', 'meeting') + ->willReturn(true); + $this->timeFactory->method('getTime')->willReturn(9999); + + $this->mapper->expects($this->once()) + ->method('update') + ->willReturnArgument(0); + + $reverted = $this->service->revertUserStatus('john', 'meeting'); + + self::assertNotNull($reverted); + self::assertSame('john', $reverted->getUserId()); + self::assertFalse($reverted->getIsBackup()); + self::assertSame( + 9999, + $reverted->getStatusTimestamp(), + 'A restored status must not keep the timestamp from before the automated status', + ); + } + + public function testRevertUserStatusManuallyStillPromotesOfflineToOnline(): void { + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setStatus(IUserStatus::OFFLINE); + $backup->setStatusTimestamp(1000); + $backup->setIsUserDefined(false); + $backup->setIsBackup(true); + + $this->mapper->method('findByUserId')->with('john', true)->willReturn($backup); + $this->mapper->method('deleteCurrentStatusToRestoreBackup')->willReturn(true); + $this->timeFactory->method('getTime')->willReturn(9999); + $this->mapper->expects($this->once())->method('update')->willReturnArgument(0); + + $reverted = $this->service->revertUserStatus('john', 'meeting', true); + + self::assertNotNull($reverted); + self::assertSame(IUserStatus::ONLINE, $reverted->getStatus()); + self::assertSame(9999, $reverted->getStatusTimestamp()); + } + public function testRevertMultipleUserStatus(): void { $john = new UserStatus(); $john->setId(1); From a9a5eb3967cda2687d84c63b924f91be5951410b Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 3/4] fix(user_status): delete stranded backup statuses in the cleanup job A backup can only be restored by revertUserStatus(), which matches on the live row still carrying the automated message id. Once that no longer holds the backup is unreachable, and since 33.0.7 excluded backups from clearOlderThanClearAt() nothing removes it any more. createBackupStatus() then keeps hitting the unique constraint on user_id, so setUserStatus() silently aborts every later automated status change for that user. Delete unreachable backups from the existing cleanup job. The check is state based rather than age based on purpose: an out-of-office backup can legitimately be weeks old. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch --- .../ClearOldStatusesBackgroundJob.php | 1 + apps/user_status/lib/Db/UserStatusMapper.php | 74 +++++++++++ .../user_status/lib/Service/StatusService.php | 14 +++ .../Service/StatusServiceIntegrationTest.php | 100 +++++++++++++++ .../ClearOldStatusesBackgroundJobTest.php | 4 + .../tests/Unit/Db/UserStatusMapperTest.php | 118 ++++++++++++++++++ 6 files changed, 311 insertions(+) diff --git a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php index 2bce800c069ae..2fbb62c39063e 100644 --- a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php +++ b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php @@ -45,5 +45,6 @@ protected function run($argument) { $this->mapper->clearOlderThanClearAt($now); $this->mapper->clearStatusesOlderThan($now - StatusService::INVALIDATE_STATUS_THRESHOLD, $now); + $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); } } diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index ea3b76c8e564d..d213fa601a70e 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -163,6 +163,80 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa return $qb->executeStatement() > 0; } + /** + * Deletes backup rows that can never be restored, because the matching live + * status is gone or is no longer on one of the automated statuses that would + * revert into it. + * + * Such a row is not just clutter: while it exists, createBackupStatus() keeps + * hitting the unique constraint on user_id, which makes setUserStatus() + * silently abort every automated status change for that user. + * + * @param list $automatedMessageIds Message ids that own a backup + * @return int Number of deleted backup rows + */ + public function deleteStrandedBackups(array $automatedMessageIds): int { + $qb = $this->db->getQueryBuilder(); + $qb->select('id', 'user_id') + ->from($this->tableName) + ->where($qb->expr()->eq('is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL))); + + $result = $qb->executeQuery(); + /** @var array $backups live user id => backup row id */ + $backups = []; + while ($row = $result->fetch()) { + // Strip the underscore prefix that was added when creating the backup + $backups[substr((string)$row['user_id'], 1)] = (int)$row['id']; + } + $result->closeCursor(); + + if ($backups === []) { + return 0; + } + + $reachable = []; + if ($automatedMessageIds !== []) { + foreach (array_chunk(array_keys($backups), 1000) as $chunk) { + $qb = $this->db->getQueryBuilder(); + // Matching on the exact user id is enough to exclude backup rows, + // since those are always prefixed and user ids cannot start with + // an underscore. Not filtering on is_backup also means a row with + // a NULL is_backup errs towards keeping the backup. + $qb->select('user_id') + ->from($this->tableName) + ->where($qb->expr()->in('user_id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_STR_ARRAY))) + ->andWhere($qb->expr()->in('message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY))); + + $liveResult = $qb->executeQuery(); + while ($row = $liveResult->fetch()) { + $reachable[(string)$row['user_id']] = true; + } + $liveResult->closeCursor(); + } + } + + $stranded = []; + foreach ($backups as $userId => $id) { + if (!isset($reachable[$userId])) { + $stranded[] = $id; + } + } + + if ($stranded === []) { + return 0; + } + + $deleted = 0; + foreach (array_chunk($stranded, 1000) as $chunk) { + $qb = $this->db->getQueryBuilder(); + $qb->delete($this->tableName) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + $deleted += $qb->executeStatement(); + } + + return $deleted; + } + public function deleteByIds(array $ids): void { $qb = $this->db->getQueryBuilder(); $qb->delete($this->tableName) diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index 739ec7cc9c653..c08539c43c43c 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -59,6 +59,20 @@ class StatusService { IUserStatus::INVISIBLE, ]; + /** + * Message ids that are only ever set by an automation (calendar, call, + * availability, out-of-office). A status carrying one of these owns the + * backup of whatever the user had set before, and is expected to be + * reverted once the automation stops applying. + */ + public const AUTOMATED_MESSAGE_IDS = [ + IUserStatus::MESSAGE_CALENDAR_BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY_TENTATIVE, + IUserStatus::MESSAGE_CALL, + IUserStatus::MESSAGE_AVAILABILITY, + IUserStatus::MESSAGE_OUT_OF_OFFICE, + ]; + /** @var int */ public const INVALIDATE_STATUS_THRESHOLD = 15 /* minutes */ * 60 /* seconds */; diff --git a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php index cd7fece50bf5d..21e3f08862804 100644 --- a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php +++ b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php @@ -348,4 +348,104 @@ public function testRevertAfterLongMeetingDoesNotFallBackToOffline(): void { 'The user was online before the meeting and must not be flipped to offline by reading the status', ); } + + /* + * Stranded backups: a backup row exists but the live row is no longer on + * the automated status that would restore it, so revertUserStatus() can + * never match. Nothing else removes it, and while it exists + * backupCurrentStatus() keeps failing, which silently aborts every future + * automated status change for that user. + */ + + public function testStrandedBackupIsCleanedUp(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + // The user clears the status message, so the meeting revert can no + // longer find a matching row. + $this->service->clearMessage('test123'); + self::assertNotNull($this->readRaw('_test123'), 'Precondition: the backup is stranded'); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(1, $deleted); + self::assertNull($this->readRaw('_test123'), 'The stranded backup must be removed'); + self::assertNotNull($this->readRaw('test123'), 'The live status must be untouched'); + } + + public function testBackupOfAnOngoingMeetingSurvivesCleanup(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(0, $deleted); + self::assertNotNull( + $this->readRaw('_test123'), + 'The backup for a meeting that is still running must survive', + ); + } + + public function testLongOutOfOfficeBackupSurvivesCleanup(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::DND, + IUserStatus::MESSAGE_OUT_OF_OFFICE, + true, + ); + // Out of office can last for weeks; age well beyond any threshold. + $this->age('test123', 86400 * 30); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(0, $deleted); + self::assertNotNull( + $this->readRaw('_test123'), + 'A long running out-of-office backup must not be treated as stranded', + ); + } + + public function testAutomatedStatusWorksAgainAfterStrandedBackupCleanup(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + $this->service->clearMessage('test123'); + + // While the stranded backup exists, automated statuses are aborted. + self::assertNull( + $this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true), + 'Precondition: the stranded backup blocks automated statuses', + ); + + $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertNotNull( + $this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true), + 'Automated statuses must work again once the stranded backup is gone', + ); + } + + public function testCleanupLeavesUsersWithoutBackupsAlone(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setCustomMessage('test123', '🍕', 'Lunch', null); + + $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame(0, $deleted); + self::assertSame('Lunch', $this->readRaw('test123')?->getCustomMessage()); + } } diff --git a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php index 6d19d19e2702a..57308fdf6cb75 100644 --- a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php +++ b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php @@ -11,6 +11,7 @@ use OCA\UserStatus\BackgroundJob\ClearOldStatusesBackgroundJob; use OCA\UserStatus\Db\UserStatusMapper; +use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Utility\ITimeFactory; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -36,6 +37,9 @@ public function testRun(): void { $this->mapper->expects($this->once()) ->method('clearStatusesOlderThan') ->with(437, 1337); + $this->mapper->expects($this->once()) + ->method('deleteStrandedBackups') + ->with(StatusService::AUTOMATED_MESSAGE_IDS); $this->time->method('getTime') ->willReturn(1337); diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index a697571bd5709..75756e615d2f7 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -405,4 +405,122 @@ public function testRestoreBackupStatuses(): void { $this->assertEquals(true, $user3Status->getIsBackup()); $this->assertEquals('Vacationing', $user3Status->getCustomMessage()); } + + /** + * @param string[] $liveMessageIds keyed by user id; null means no live row + */ + private function insertBackupWithLiveStatus(string $userId, ?string $liveMessageId): void { + $backup = new UserStatus(); + $backup->setUserId('_' . $userId); + $backup->setStatus('online'); + $backup->setStatusTimestamp(5000); + $backup->setIsUserDefined(false); + $backup->setIsBackup(true); + $this->mapper->insert($backup); + + if ($liveMessageId === null) { + return; + } + + $live = new UserStatus(); + $live->setUserId($userId); + $live->setStatus('busy'); + $live->setStatusTimestamp(6000); + $live->setIsUserDefined(true); + $live->setIsBackup(false); + $live->setMessageId($liveMessageId === '' ? null : $liveMessageId); + $this->mapper->insert($live); + } + + public function testDeleteStrandedBackupsWithNoBackups(): void { + $this->insertSampleStatuses(); + + $this->assertSame(0, $this->mapper->deleteStrandedBackups(['meeting', 'call'])); + $this->assertCount(3, $this->mapper->findAll()); + } + + public function testDeleteStrandedBackupsKeepsBackupsOfAutomatedStatuses(): void { + $this->insertBackupWithLiveStatus('user1', 'meeting'); + $this->insertBackupWithLiveStatus('user2', 'call'); + $this->insertBackupWithLiveStatus('user3', 'availability'); + $this->insertBackupWithLiveStatus('user4', 'out-of-office'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call', 'availability', 'out-of-office']); + + $this->assertSame(0, $deleted); + foreach (['user1', 'user2', 'user3', 'user4'] as $userId) { + $this->assertEquals('_' . $userId, $this->mapper->findByUserId($userId, true)->getUserId()); + } + } + + public function testDeleteStrandedBackupsRemovesBackupWithoutLiveStatus(): void { + $this->insertBackupWithLiveStatus('user1', null); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + $this->expectException(DoesNotExistException::class); + $this->mapper->findByUserId('user1', true); + } + + public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusHasNoMessageId(): void { + $this->insertBackupWithLiveStatus('user1', ''); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + // The live status must survive. + $this->assertEquals('user1', $this->mapper->findByUserId('user1')->getUserId()); + } + + public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusIsUserDefinedMessage(): void { + $this->insertBackupWithLiveStatus('user1', 'vacationing'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + $this->assertEquals('vacationing', $this->mapper->findByUserId('user1')->getMessageId()); + } + + public function testDeleteStrandedBackupsOnlyRemovesTheStrandedOnes(): void { + $this->insertBackupWithLiveStatus('keepme', 'meeting'); + $this->insertBackupWithLiveStatus('stranded1', 'vacationing'); + $this->insertBackupWithLiveStatus('stranded2', null); + $this->insertBackupWithLiveStatus('keepme2', 'call'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(2, $deleted); + $this->assertEquals('_keepme', $this->mapper->findByUserId('keepme', true)->getUserId()); + $this->assertEquals('_keepme2', $this->mapper->findByUserId('keepme2', true)->getUserId()); + foreach (['stranded1', 'stranded2'] as $userId) { + try { + $this->mapper->findByUserId($userId, true); + $this->fail("Backup for $userId should have been deleted"); + } catch (DoesNotExistException) { + } + } + } + + public function testDeleteStrandedBackupsDoesNotConfuseUsersWithSimilarNames(): void { + // '_user1' as a backup of 'user1', plus a real user literally named + // 'user1x' whose backup must be judged on its own live row. + $this->insertBackupWithLiveStatus('user1', 'meeting'); + $this->insertBackupWithLiveStatus('user1x', 'vacationing'); + + $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); + + $this->assertSame(1, $deleted); + $this->assertEquals('_user1', $this->mapper->findByUserId('user1', true)->getUserId()); + $this->expectException(DoesNotExistException::class); + $this->mapper->findByUserId('user1x', true); + } + + public function testDeleteStrandedBackupsWithEmptyAutomatedListRemovesAll(): void { + $this->insertBackupWithLiveStatus('user1', 'meeting'); + $this->insertBackupWithLiveStatus('user2', 'call'); + + // Defensive: with nothing considered automated, every backup is stranded. + $this->assertSame(2, $this->mapper->deleteStrandedBackups([])); + } } From 82040697d23e689b7ce998fe02604fbf4c007087 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 4/4] feat(user_status): add occ user-status:repair for statuses left behind The preceding fixes stop new damage, but nothing repairs what is already in the database: reverts for call, availability and out-of-office are driven by automations that never fire again for a user who is already stuck. Add a command that repairs the three shapes, with --dry-run to see the scope first: - statuses whose is_backup is NULL, which every query comparing the column against false skips - live rows on an automated status with no backup to revert into - backup rows that can no longer be matched Orphaned rows are deleted rather than rewritten, matching what revertUserStatus() now does, and the next heartbeat recreates a normal status. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch --- apps/user_status/appinfo/info.xml | 3 + .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + apps/user_status/lib/Command/Repair.php | 129 ++++++++++++++ apps/user_status/lib/Db/UserStatusMapper.php | 165 +++++++++++++----- .../Service/StatusServiceIntegrationTest.php | 45 +++++ .../tests/Unit/Command/RepairTest.php | 85 +++++++++ .../tests/Unit/Db/UserStatusMapperTest.php | 94 ++++++++++ 8 files changed, 477 insertions(+), 46 deletions(-) create mode 100644 apps/user_status/lib/Command/Repair.php create mode 100644 apps/user_status/tests/Unit/Command/RepairTest.php diff --git a/apps/user_status/appinfo/info.xml b/apps/user_status/appinfo/info.xml index 7702671a50248..4a9d95be21a7e 100644 --- a/apps/user_status/appinfo/info.xml +++ b/apps/user_status/appinfo/info.xml @@ -29,6 +29,9 @@ OCA\UserStatus\BackgroundJob\ClearOldStatusesBackgroundJob + + OCA\UserStatus\Command\Repair + OCA\UserStatus\ContactsMenu\StatusProvider diff --git a/apps/user_status/composer/composer/autoload_classmap.php b/apps/user_status/composer/composer/autoload_classmap.php index b57df813bc9c4..4a1927147e7d3 100644 --- a/apps/user_status/composer/composer/autoload_classmap.php +++ b/apps/user_status/composer/composer/autoload_classmap.php @@ -10,6 +10,7 @@ 'OCA\\UserStatus\\AppInfo\\Application' => $baseDir . '/../lib/AppInfo/Application.php', 'OCA\\UserStatus\\BackgroundJob\\ClearOldStatusesBackgroundJob' => $baseDir . '/../lib/BackgroundJob/ClearOldStatusesBackgroundJob.php', 'OCA\\UserStatus\\Capabilities' => $baseDir . '/../lib/Capabilities.php', + 'OCA\\UserStatus\\Command\\Repair' => $baseDir . '/../lib/Command/Repair.php', 'OCA\\UserStatus\\Connector\\UserStatus' => $baseDir . '/../lib/Connector/UserStatus.php', 'OCA\\UserStatus\\Connector\\UserStatusProvider' => $baseDir . '/../lib/Connector/UserStatusProvider.php', 'OCA\\UserStatus\\ContactsMenu\\StatusProvider' => $baseDir . '/../lib/ContactsMenu/StatusProvider.php', diff --git a/apps/user_status/composer/composer/autoload_static.php b/apps/user_status/composer/composer/autoload_static.php index 17f45ab9bbf58..1d7a57bfe81d4 100644 --- a/apps/user_status/composer/composer/autoload_static.php +++ b/apps/user_status/composer/composer/autoload_static.php @@ -25,6 +25,7 @@ class ComposerStaticInitUserStatus 'OCA\\UserStatus\\AppInfo\\Application' => __DIR__ . '/..' . '/../lib/AppInfo/Application.php', 'OCA\\UserStatus\\BackgroundJob\\ClearOldStatusesBackgroundJob' => __DIR__ . '/..' . '/../lib/BackgroundJob/ClearOldStatusesBackgroundJob.php', 'OCA\\UserStatus\\Capabilities' => __DIR__ . '/..' . '/../lib/Capabilities.php', + 'OCA\\UserStatus\\Command\\Repair' => __DIR__ . '/..' . '/../lib/Command/Repair.php', 'OCA\\UserStatus\\Connector\\UserStatus' => __DIR__ . '/..' . '/../lib/Connector/UserStatus.php', 'OCA\\UserStatus\\Connector\\UserStatusProvider' => __DIR__ . '/..' . '/../lib/Connector/UserStatusProvider.php', 'OCA\\UserStatus\\ContactsMenu\\StatusProvider' => __DIR__ . '/..' . '/../lib/ContactsMenu/StatusProvider.php', diff --git a/apps/user_status/lib/Command/Repair.php b/apps/user_status/lib/Command/Repair.php new file mode 100644 index 0000000000000..796fd96bcd03a --- /dev/null +++ b/apps/user_status/lib/Command/Repair.php @@ -0,0 +1,129 @@ +setName('user-status:repair') + ->setDescription('Repair user statuses left behind by an interrupted automated status') + ->addOption('dry-run', null, InputOption::VALUE_NONE, 'Only report what would be repaired'); + } + + #[\Override] + public function execute(InputInterface $input, OutputInterface $output): int { + $dryRun = (bool)$input->getOption('dry-run'); + if ($dryRun) { + $output->writeln('Dry run, no changes will be written.'); + $output->writeln(''); + } + + $this->repairMissingBackupFlags($output, $dryRun); + $this->repairOrphanedStatuses($output, $dryRun); + $this->repairStrandedBackups($output, $dryRun); + + return self::SUCCESS; + } + + /** + * Rows written before is_backup had a default are invisible to every query + * comparing it against false, so other users see them as offline and the + * cleanup job skips them. + */ + private function repairMissingBackupFlags(OutputInterface $output, bool $dryRun): void { + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + if ($ids === []) { + $output->writeln('No statuses with a missing backup flag.'); + return; + } + + $count = count($ids); + if ($dryRun) { + $output->writeln("Would set the backup flag on $count status(es)."); + $this->listIds($output, $ids); + return; + } + + $fixed = $this->mapper->normalizeBackupFlagByIds($ids); + $output->writeln("Set the backup flag on $fixed status(es)."); + } + + /** + * A live status on an automated message id with no backup row can never be + * reverted by the automation that set it, and the heartbeat refuses to + * overwrite it, so the user is stuck. Removing the row lets the next + * heartbeat recreate a normal status. + */ + private function repairOrphanedStatuses(OutputInterface $output, bool $dryRun): void { + $ids = $this->mapper->findOrphanedAutomatedStatusIds(StatusService::AUTOMATED_MESSAGE_IDS); + if ($ids === []) { + $output->writeln('No users stuck on an automated status.'); + return; + } + + if ($dryRun) { + $output->writeln('Would clear ' . count($ids) . ' status(es) stuck on an automated status.'); + $this->listIds($output, $ids); + return; + } + + $deleted = $this->mapper->deleteByIds($ids); + $output->writeln("Cleared $deleted status(es) stuck on an automated status."); + } + + /** + * A backup that can no longer be matched blocks every future automated + * status change for that user, because createBackupStatus() keeps hitting + * the unique constraint on user_id. + */ + private function repairStrandedBackups(OutputInterface $output, bool $dryRun): void { + $ids = $this->mapper->findStrandedBackupIds(StatusService::AUTOMATED_MESSAGE_IDS); + if ($ids === []) { + $output->writeln('No stranded backup statuses.'); + return; + } + + if ($dryRun) { + $output->writeln('Would remove ' . count($ids) . ' stranded backup status(es).'); + $this->listIds($output, $ids); + return; + } + + $deleted = $this->mapper->deleteByIds($ids); + $output->writeln("Removed $deleted stranded backup status(es)."); + } + + /** + * The ids are what an administrator needs to look the rows up themselves, + * but there can be a lot of them, so only spell them out when asked. + * + * @param list $ids + */ + private function listIds(OutputInterface $output, array $ids): void { + if ($output->getVerbosity() >= OutputInterface::VERBOSITY_VERBOSE) { + $output->writeln(' ids: ' . implode(', ', $ids)); + } + } +} diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index d213fa601a70e..9ed29b39fe3e8 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -20,6 +20,12 @@ */ class UserStatusMapper extends QBMapper { + /** + * Oracle rejects an IN list with more than 1000 expressions, so anything + * built from an unbounded set of ids has to be split into chunks. + */ + private const MAX_IN_CHUNK = 1000; + /** * @param IDBConnection $db */ @@ -176,58 +182,132 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa * @return int Number of deleted backup rows */ public function deleteStrandedBackups(array $automatedMessageIds): int { + return $this->deleteByIds($this->findStrandedBackupIds($automatedMessageIds)); + } + + /** + * Ids of backup rows that can never be restored. See deleteStrandedBackups(). + * + * A backup is reachable exactly when the live row it belongs to still carries + * one of the automated message ids, because that is what revertUserStatus() + * matches on. The live row is the one whose user id is the backup's user id + * without the underscore prefix, so the two are matched with a self join. + * + * @param list $automatedMessageIds + * @return list + */ + public function findStrandedBackupIds(array $automatedMessageIds): array { $qb = $this->db->getQueryBuilder(); - $qb->select('id', 'user_id') - ->from($this->tableName) - ->where($qb->expr()->eq('is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL))); + $qb->select('b.id') + ->from($this->tableName, 'b') + ->where($qb->expr()->eq('b.is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL))); + + if ($automatedMessageIds === []) { + // No automated status can own a backup, so none of them is reachable. + return $this->fetchIds($qb); + } + + // Not filtering the live side on is_backup is deliberate: a row whose + // is_backup is NULL is still treated as a live row, so unexpected data + // errs towards keeping the backup. + $qb->leftJoin('b', $this->tableName, 'l', $qb->expr()->andX( + $qb->expr()->eq('l.user_id', $qb->func()->substring('b.user_id', $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT))), + $qb->expr()->in('l.message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY)), + )) + ->andWhere($qb->expr()->isNull('l.id')); + + return $this->fetchIds($qb); + } + + /** + * Ids of live rows that sit on an automated status with no backup row to + * revert into. Those can never be reverted by the automation that set them, + * so the user is stuck on that status until it is cleared. + * + * @param list $automatedMessageIds + * @return list + */ + public function findOrphanedAutomatedStatusIds(array $automatedMessageIds): array { + if ($automatedMessageIds === []) { + return []; + } + + $qb = $this->db->getQueryBuilder(); + // The backup of a live row carries the same user id with an underscore + // prefix, so the two are matched with a self join on the concatenation. + $qb->select('l.id') + ->from($this->tableName, 'l') + ->leftJoin('l', $this->tableName, 'b', $qb->expr()->eq( + 'b.user_id', + $qb->func()->concat($qb->createNamedParameter('_'), 'l.user_id'), + )) + ->where($qb->expr()->in('l.message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY))) + ->andWhere($qb->expr()->isNull('b.id')) + // Skip backup rows on the live side. Testing the prefix rather than + // is_backup keeps this correct for rows where is_backup is NULL, and + // a substring comparison avoids having to escape the underscore for + // a LIKE pattern. + ->andWhere($qb->expr()->neq( + $qb->func()->substring('l.user_id', $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT), $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT)), + $qb->createNamedParameter('_'), + )); + + return $this->fetchIds($qb); + } + /** + * @return list + */ + private function fetchIds(IQueryBuilder $qb): array { $result = $qb->executeQuery(); - /** @var array $backups live user id => backup row id */ - $backups = []; + $ids = []; while ($row = $result->fetch()) { - // Strip the underscore prefix that was added when creating the backup - $backups[substr((string)$row['user_id'], 1)] = (int)$row['id']; + $ids[] = (int)$row['id']; } $result->closeCursor(); - if ($backups === []) { - return 0; - } + return $ids; + } - $reachable = []; - if ($automatedMessageIds !== []) { - foreach (array_chunk(array_keys($backups), 1000) as $chunk) { - $qb = $this->db->getQueryBuilder(); - // Matching on the exact user id is enough to exclude backup rows, - // since those are always prefixed and user ids cannot start with - // an underscore. Not filtering on is_backup also means a row with - // a NULL is_backup errs towards keeping the backup. - $qb->select('user_id') - ->from($this->tableName) - ->where($qb->expr()->in('user_id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_STR_ARRAY))) - ->andWhere($qb->expr()->in('message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY))); - - $liveResult = $qb->executeQuery(); - while ($row = $liveResult->fetch()) { - $reachable[(string)$row['user_id']] = true; - } - $liveResult->closeCursor(); - } - } + /** + * Ids of rows where is_backup is NULL. Those predate the column default and + * are invisible to every query that compares is_backup against false. + * + * @return list + */ + public function findStatusesWithoutBackupFlagIds(): array { + $qb = $this->db->getQueryBuilder(); + $qb->select('id') + ->from($this->tableName) + ->where($qb->expr()->isNull('is_backup')); - $stranded = []; - foreach ($backups as $userId => $id) { - if (!isset($reachable[$userId])) { - $stranded[] = $id; - } - } + return $this->fetchIds($qb); + } - if ($stranded === []) { - return 0; + /** + * @param list $ids + * @return int Number of rows that were given an explicit is_backup value + */ + public function normalizeBackupFlagByIds(array $ids): int { + $updated = 0; + foreach (array_chunk($ids, self::MAX_IN_CHUNK) as $chunk) { + $qb = $this->db->getQueryBuilder(); + $qb->update($this->tableName) + ->set('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); + $updated += $qb->executeStatement(); } + return $updated; + } + + /** + * @param list $ids + * @return int Number of deleted rows + */ + public function deleteByIds(array $ids): int { $deleted = 0; - foreach (array_chunk($stranded, 1000) as $chunk) { + foreach (array_chunk($ids, self::MAX_IN_CHUNK) as $chunk) { $qb = $this->db->getQueryBuilder(); $qb->delete($this->tableName) ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); @@ -237,13 +317,6 @@ public function deleteStrandedBackups(array $automatedMessageIds): int { return $deleted; } - public function deleteByIds(array $ids): void { - $qb = $this->db->getQueryBuilder(); - $qb->delete($this->tableName) - ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); - $qb->executeStatement(); - } - /** * @param string $userId * @return bool diff --git a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php index 21e3f08862804..92f8fc5dc0eb9 100644 --- a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php +++ b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php @@ -448,4 +448,49 @@ public function testCleanupLeavesUsersWithoutBackupsAlone(): void { self::assertSame(0, $deleted); self::assertSame('Lunch', $this->readRaw('test123')?->getCustomMessage()); } + + /** + * The lookup matches a live row against its backup by concatenating the + * underscore prefix in SQL, so it has to be exercised on a real database + * rather than only through the mapper unit tests. + */ + public function testFindsOrphanedAutomatedStatusOnARealDatabase(): void { + // A user with no status row at all gets no backup, so the meeting + // status it is given can never be reverted. + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + self::assertNull($this->readRaw('_test123'), 'Precondition: there is no backup'); + + // A second user on the same automated status, but with a backup, must + // not be reported. + $this->service->setStatus('test456', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test456', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + $orphaned = $this->mapper->findOrphanedAutomatedStatusIds(StatusService::AUTOMATED_MESSAGE_IDS); + + self::assertSame([$this->readRaw('test123')?->getId()], $orphaned); + } + + public function testFindsStatusesWithoutBackupFlagOnARealDatabase(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->db->executeStatement( + 'UPDATE `*PREFIX*user_status` SET `is_backup` = NULL WHERE `user_id` = ?', + ['test123'], + ); + + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + + self::assertSame([$this->readRaw('test123')?->getId()], $ids); + self::assertSame(1, $this->mapper->normalizeBackupFlagByIds($ids)); + self::assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + } } diff --git a/apps/user_status/tests/Unit/Command/RepairTest.php b/apps/user_status/tests/Unit/Command/RepairTest.php new file mode 100644 index 0000000000000..e4a7eb1d8e2c2 --- /dev/null +++ b/apps/user_status/tests/Unit/Command/RepairTest.php @@ -0,0 +1,85 @@ +mapper = $this->createMock(UserStatusMapper::class); + $this->tester = new CommandTester(new Repair($this->mapper)); + } + + public function testRepairsEverything(): void { + $this->mapper->expects($this->once()) + ->method('findStatusesWithoutBackupFlagIds') + ->willReturn([1, 2]); + $this->mapper->expects($this->once()) + ->method('normalizeBackupFlagByIds') + ->with([1, 2]) + ->willReturn(2); + + $this->mapper->expects($this->once()) + ->method('findOrphanedAutomatedStatusIds') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn([7, 8, 9]); + $this->mapper->expects($this->once()) + ->method('findStrandedBackupIds') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn([11, 12, 13, 14]); + $this->mapper->expects($this->exactly(2)) + ->method('deleteByIds') + ->willReturnCallback(static fn (array $ids): int => count($ids)); + + self::assertSame(Command::SUCCESS, $this->tester->execute([])); + + $display = $this->tester->getDisplay(); + self::assertStringContainsString('2', $display); + self::assertStringContainsString('3', $display); + self::assertStringContainsString('4', $display); + } + + public function testDryRunChangesNothing(): void { + $this->mapper->method('findStatusesWithoutBackupFlagIds')->willReturn([1, 2]); + $this->mapper->method('findOrphanedAutomatedStatusIds')->willReturn([7, 8, 9]); + $this->mapper->method('findStrandedBackupIds')->willReturn([11, 12, 13, 14]); + + $this->mapper->expects($this->never())->method('normalizeBackupFlagByIds'); + $this->mapper->expects($this->never())->method('deleteByIds'); + $this->mapper->expects($this->never())->method('deleteStrandedBackups'); + + self::assertSame(Command::SUCCESS, $this->tester->execute(['--dry-run' => true])); + + self::assertStringContainsString('dry run', strtolower($this->tester->getDisplay())); + } + + public function testNothingToRepair(): void { + $this->mapper->method('findStatusesWithoutBackupFlagIds')->willReturn([]); + $this->mapper->method('findOrphanedAutomatedStatusIds')->willReturn([]); + $this->mapper->method('findStrandedBackupIds')->willReturn([]); + + // Nothing to normalise and nothing to delete. + $this->mapper->expects($this->never())->method('normalizeBackupFlagByIds'); + $this->mapper->expects($this->never())->method('deleteByIds'); + + self::assertSame(Command::SUCCESS, $this->tester->execute([])); + } +} diff --git a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php index 75756e615d2f7..4b03fa0ab9ba6 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -516,6 +516,100 @@ public function testDeleteStrandedBackupsDoesNotConfuseUsersWithSimilarNames(): $this->mapper->findByUserId('user1x', true); } + public function testFindStrandedBackupIds(): void { + $this->insertBackupWithLiveStatus('keepme', 'meeting'); + $this->insertBackupWithLiveStatus('stranded', 'vacationing'); + + $ids = $this->mapper->findStrandedBackupIds(['meeting', 'call']); + + $this->assertCount(1, $ids); + $this->assertSame( + $this->mapper->findByUserId('stranded', true)->getId(), + $ids[0], + ); + } + + public function testFindStrandedBackupIdsDoesNotDelete(): void { + $this->insertBackupWithLiveStatus('stranded', 'vacationing'); + + $this->mapper->findStrandedBackupIds(['meeting']); + + $this->assertEquals('_stranded', $this->mapper->findByUserId('stranded', true)->getUserId()); + } + + public function testFindOrphanedAutomatedStatusIds(): void { + // Live automated status with no backup to revert into: orphaned. + $orphan = new UserStatus(); + $orphan->setUserId('orphan'); + $orphan->setStatus('busy'); + $orphan->setStatusTimestamp(5000); + $orphan->setIsUserDefined(true); + $orphan->setIsBackup(false); + $orphan->setMessageId('meeting'); + $this->mapper->insert($orphan); + + // Same shape but with a backup: an ongoing meeting, must be left alone. + $this->insertBackupWithLiveStatus('inmeeting', 'meeting'); + + // A status the user set themselves: not automated, must be left alone. + $own = new UserStatus(); + $own->setUserId('ownstatus'); + $own->setStatus('dnd'); + $own->setStatusTimestamp(5000); + $own->setIsUserDefined(true); + $own->setIsBackup(false); + $own->setMessageId('vacationing'); + $this->mapper->insert($own); + + $ids = $this->mapper->findOrphanedAutomatedStatusIds(['meeting', 'call']); + + $this->assertCount(1, $ids); + $this->assertSame($this->mapper->findByUserId('orphan')->getId(), $ids[0]); + } + + public function testFindOrphanedAutomatedStatusIdsIgnoresBackupRows(): void { + // A backup row that happens to carry an automated message id must never + // be reported as an orphaned live status. + $backup = new UserStatus(); + $backup->setUserId('_someone'); + $backup->setStatus('busy'); + $backup->setStatusTimestamp(5000); + $backup->setIsUserDefined(true); + $backup->setIsBackup(true); + $backup->setMessageId('meeting'); + $this->mapper->insert($backup); + + $this->assertSame([], $this->mapper->findOrphanedAutomatedStatusIds(['meeting', 'call'])); + } + + public function testFindOrphanedAutomatedStatusIdsWithEmptyAutomatedList(): void { + $this->insertBackupWithLiveStatus('user1', 'meeting'); + + $this->assertSame([], $this->mapper->findOrphanedAutomatedStatusIds([])); + } + + public function testNormalizeBackupFlag(): void { + $this->insertSampleStatuses(); + self::$realDatabase->executeStatement( + 'UPDATE `*PREFIX*user_status` SET `is_backup` = NULL WHERE `user_id` = ?', + ['user1'], + ); + + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + $this->assertCount(1, $ids); + $this->assertSame(1, $this->mapper->normalizeBackupFlagByIds($ids)); + $this->assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + // The row is visible to findAll() again. + $this->assertCount(3, $this->mapper->findAll()); + } + + public function testNormalizeBackupFlagWithNothingToDo(): void { + $this->insertSampleStatuses(); + + $this->assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + $this->assertSame(0, $this->mapper->normalizeBackupFlagByIds([])); + } + public function testDeleteStrandedBackupsWithEmptyAutomatedListRemovesAll(): void { $this->insertBackupWithLiveStatus('user1', 'meeting'); $this->insertBackupWithLiveStatus('user2', 'call');