From 923053b02738247af59a7192d252c493e70dadfa Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 1/6] 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. Assisted-by: ClaudeCode: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 20a61dcbc3d7bbfe797e6da4a908aa23f58658ba Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 2/6] 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. Assisted-by: ClaudeCode: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 46b3ec5acbfb76010f859f9877481ea5bc000d5e Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 3/6] 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. Assisted-by: ClaudeCode: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 9fd27e5019b32371e6aeb2f54ead3a263edf3e61 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 11 Aug 2026 17:02:21 +0200 Subject: [PATCH 4/6] 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. Assisted-by: ClaudeCode: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'); From 3780937c17523e1ebea2d7d6550ca4a74a96c58b Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Tue, 1 Sep 2026 17:41:26 +0200 Subject: [PATCH 5/6] fix(user_status): use IQueryBuilder::MAX_IN_PARAMETERS for IN-list chunking Replaces the app-local MAX_IN_CHUNK constant with the public constant, as requested in review. Both hold 1000, so behaviour is unchanged. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Anna Larch --- apps/user_status/lib/Db/UserStatusMapper.php | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 9ed29b39fe3e8..2754421924d75 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -20,12 +20,6 @@ */ 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 */ @@ -290,7 +284,7 @@ public function findStatusesWithoutBackupFlagIds(): array { */ public function normalizeBackupFlagByIds(array $ids): int { $updated = 0; - foreach (array_chunk($ids, self::MAX_IN_CHUNK) as $chunk) { + foreach (array_chunk($ids, IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) { $qb = $this->db->getQueryBuilder(); $qb->update($this->tableName) ->set('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)) @@ -307,7 +301,7 @@ public function normalizeBackupFlagByIds(array $ids): int { */ public function deleteByIds(array $ids): int { $deleted = 0; - foreach (array_chunk($ids, self::MAX_IN_CHUNK) as $chunk) { + foreach (array_chunk($ids, IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) { $qb = $this->db->getQueryBuilder(); $qb->delete($this->tableName) ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); From c7643e40dc1993a6b19af0afc3d342fbe3e8c7a5 Mon Sep 17 00:00:00 2001 From: Anna Larch Date: Thu, 17 Sep 2026 14:03:07 +0200 Subject: [PATCH 6/6] fix(user_status): share one definition of a stranded status Stranded detection moves into StatusRepairService so the cleanup job, the repair command and the mapper share one rule. Bulk restores now refresh the status timestamp, and normalising is_backup no longer turns a backup into a live row. Assisted-by: ClaudeCode:claude-opus-5 Signed-off-by: Anna Larch --- .../composer/composer/autoload_classmap.php | 1 + .../composer/composer/autoload_static.php | 1 + .../ClearOldStatusesBackgroundJob.php | 5 +- apps/user_status/lib/Command/Repair.php | 41 ++--- apps/user_status/lib/Db/UserStatusMapper.php | 90 +++++----- .../lib/Service/StatusRepairService.php | 67 ++++++++ .../user_status/lib/Service/StatusService.php | 127 +++++++------- .../Service/StatusServiceIntegrationTest.php | 138 +++++++++++----- .../ClearOldStatusesBackgroundJobTest.php | 11 +- .../tests/Unit/Command/RepairTest.php | 43 +++-- .../tests/Unit/Db/UserStatusMapperTest.php | 43 +++-- .../Unit/Service/StatusRepairServiceTest.php | 52 ++++++ .../tests/Unit/Service/StatusServiceTest.php | 156 +++++++++++++++++- 13 files changed, 548 insertions(+), 227 deletions(-) create mode 100644 apps/user_status/lib/Service/StatusRepairService.php create mode 100644 apps/user_status/tests/Unit/Service/StatusRepairServiceTest.php diff --git a/apps/user_status/composer/composer/autoload_classmap.php b/apps/user_status/composer/composer/autoload_classmap.php index 4a1927147e7d3..5641b449531f2 100644 --- a/apps/user_status/composer/composer/autoload_classmap.php +++ b/apps/user_status/composer/composer/autoload_classmap.php @@ -38,5 +38,6 @@ 'OCA\\UserStatus\\ResponseDefinitions' => $baseDir . '/../lib/ResponseDefinitions.php', 'OCA\\UserStatus\\Service\\JSDataService' => $baseDir . '/../lib/Service/JSDataService.php', 'OCA\\UserStatus\\Service\\PredefinedStatusService' => $baseDir . '/../lib/Service/PredefinedStatusService.php', + 'OCA\\UserStatus\\Service\\StatusRepairService' => $baseDir . '/../lib/Service/StatusRepairService.php', 'OCA\\UserStatus\\Service\\StatusService' => $baseDir . '/../lib/Service/StatusService.php', ); diff --git a/apps/user_status/composer/composer/autoload_static.php b/apps/user_status/composer/composer/autoload_static.php index 1d7a57bfe81d4..fceb7d2c0269a 100644 --- a/apps/user_status/composer/composer/autoload_static.php +++ b/apps/user_status/composer/composer/autoload_static.php @@ -53,6 +53,7 @@ class ComposerStaticInitUserStatus 'OCA\\UserStatus\\ResponseDefinitions' => __DIR__ . '/..' . '/../lib/ResponseDefinitions.php', 'OCA\\UserStatus\\Service\\JSDataService' => __DIR__ . '/..' . '/../lib/Service/JSDataService.php', 'OCA\\UserStatus\\Service\\PredefinedStatusService' => __DIR__ . '/..' . '/../lib/Service/PredefinedStatusService.php', + 'OCA\\UserStatus\\Service\\StatusRepairService' => __DIR__ . '/..' . '/../lib/Service/StatusRepairService.php', 'OCA\\UserStatus\\Service\\StatusService' => __DIR__ . '/..' . '/../lib/Service/StatusService.php', ); diff --git a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php index 2fbb62c39063e..e9a34035613cc 100644 --- a/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php +++ b/apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php @@ -10,6 +10,7 @@ namespace OCA\UserStatus\BackgroundJob; use OCA\UserStatus\Db\UserStatusMapper; +use OCA\UserStatus\Service\StatusRepairService; use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Utility\ITimeFactory; use OCP\BackgroundJob\TimedJob; @@ -26,10 +27,12 @@ class ClearOldStatusesBackgroundJob extends TimedJob { * * @param ITimeFactory $time * @param UserStatusMapper $mapper + * @param StatusRepairService $repairService */ public function __construct( ITimeFactory $time, private UserStatusMapper $mapper, + private StatusRepairService $repairService, ) { parent::__construct($time); @@ -45,6 +48,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); + $this->repairService->deleteStrandedBackups(); } } diff --git a/apps/user_status/lib/Command/Repair.php b/apps/user_status/lib/Command/Repair.php index 796fd96bcd03a..3b10e83b1cf0a 100644 --- a/apps/user_status/lib/Command/Repair.php +++ b/apps/user_status/lib/Command/Repair.php @@ -9,8 +9,7 @@ namespace OCA\UserStatus\Command; -use OCA\UserStatus\Db\UserStatusMapper; -use OCA\UserStatus\Service\StatusService; +use OCA\UserStatus\Service\StatusRepairService; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Input\InputInterface; use Symfony\Component\Console\Input\InputOption; @@ -19,7 +18,7 @@ class Repair extends Command { public function __construct( - private UserStatusMapper $mapper, + private StatusRepairService $repairService, ) { parent::__construct(); } @@ -47,13 +46,9 @@ public function execute(InputInterface $input, OutputInterface $output): int { 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. - */ + /** The flag comes from the user id, so a pre-default backup stays a backup. */ private function repairMissingBackupFlags(OutputInterface $output, bool $dryRun): void { - $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + $ids = $this->repairService->findStatusesWithoutBackupFlagIds(); if ($ids === []) { $output->writeln('No statuses with a missing backup flag.'); return; @@ -61,23 +56,17 @@ private function repairMissingBackupFlags(OutputInterface $output, bool $dryRun) $count = count($ids); if ($dryRun) { - $output->writeln("Would set the backup flag on $count status(es)."); + $output->writeln("Would give $count status(es) an explicit backup flag."); $this->listIds($output, $ids); return; } - $fixed = $this->mapper->normalizeBackupFlagByIds($ids); - $output->writeln("Set the backup flag on $fixed status(es)."); + $fixed = $this->repairService->normalizeBackupFlagByIds($ids); + $output->writeln("Gave $fixed status(es) an explicit backup flag."); } - /** - * 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); + $ids = $this->repairService->findOrphanedAutomatedStatusIds(); if ($ids === []) { $output->writeln('No users stuck on an automated status.'); return; @@ -89,17 +78,12 @@ private function repairOrphanedStatuses(OutputInterface $output, bool $dryRun): return; } - $deleted = $this->mapper->deleteByIds($ids); + $deleted = $this->repairService->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); + $ids = $this->repairService->findStrandedBackupIds(); if ($ids === []) { $output->writeln('No stranded backup statuses.'); return; @@ -111,14 +95,11 @@ private function repairStrandedBackups(OutputInterface $output, bool $dryRun): v return; } - $deleted = $this->mapper->deleteByIds($ids); + $deleted = $this->repairService->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 { diff --git a/apps/user_status/lib/Db/UserStatusMapper.php b/apps/user_status/lib/Db/UserStatusMapper.php index 2754421924d75..71d2631bffe37 100644 --- a/apps/user_status/lib/Db/UserStatusMapper.php +++ b/apps/user_status/lib/Db/UserStatusMapper.php @@ -164,15 +164,7 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa } /** - * 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 + * @param list $automatedMessageIds * @return int Number of deleted backup rows */ public function deleteStrandedBackups(array $automatedMessageIds): int { @@ -180,30 +172,20 @@ public function deleteStrandedBackups(array $automatedMessageIds): int { } /** - * 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 { + if ($automatedMessageIds === []) { + return []; + } + $qb = $this->db->getQueryBuilder(); $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. + // A NULL is_backup counts as live: odd data keeps 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)), @@ -214,10 +196,6 @@ public function findStrandedBackupIds(array $automatedMessageIds): array { } /** - * 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 */ @@ -227,8 +205,6 @@ public function findOrphanedAutomatedStatusIds(array $automatedMessageIds): arra } $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( @@ -237,10 +213,6 @@ public function findOrphanedAutomatedStatusIds(array $automatedMessageIds): arra )) ->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('_'), @@ -264,9 +236,6 @@ private function fetchIds(IQueryBuilder $qb): array { } /** - * 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 { @@ -279,17 +248,31 @@ public function findStatusesWithoutBackupFlagIds(): array { } /** + * Takes is_backup from the user id prefix: false for everything would make a + * pre-default backup an unrestorable live row called "_alice". + * * @param list $ids - * @return int Number of rows that were given an explicit is_backup value + * @return int Number of rows given an explicit is_backup value */ public function normalizeBackupFlagByIds(array $ids): int { $updated = 0; foreach (array_chunk($ids, IQueryBuilder::MAX_IN_PARAMETERS) 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(); + foreach ([true, false] as $isBackup) { + $qb = $this->db->getQueryBuilder(); + $firstCharacter = $qb->func()->substring( + 'user_id', + $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT), + $qb->createNamedParameter(1, IQueryBuilder::PARAM_INT), + ); + $underscore = $qb->createNamedParameter('_'); + $qb->update($this->tableName) + ->set('is_backup', $qb->createNamedParameter($isBackup, IQueryBuilder::PARAM_BOOL)) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))) + ->andWhere($isBackup + ? $qb->expr()->eq($firstCharacter, $underscore) + : $qb->expr()->neq($firstCharacter, $underscore)); + $updated += $qb->executeStatement(); + } } return $updated; @@ -328,13 +311,20 @@ public function createBackupStatus(string $userId): bool { return $qb->executeStatement() > 0; } - public function restoreBackupStatuses(array $ids): void { - $qb = $this->db->getQueryBuilder(); - $qb->update($this->tableName) - ->set('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)) - ->set('user_id', $qb->func()->substring('user_id', $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT))) - ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); + /** + * @param list $ids + * @param int $statusTimestamp The backed up one would already be stale. + */ + public function restoreBackupStatuses(array $ids, int $statusTimestamp): void { + foreach (array_chunk($ids, IQueryBuilder::MAX_IN_PARAMETERS) as $chunk) { + $qb = $this->db->getQueryBuilder(); + $qb->update($this->tableName) + ->set('is_backup', $qb->createNamedParameter(false, IQueryBuilder::PARAM_BOOL)) + ->set('status_timestamp', $qb->createNamedParameter($statusTimestamp, IQueryBuilder::PARAM_INT)) + ->set('user_id', $qb->func()->substring('user_id', $qb->createNamedParameter(2, IQueryBuilder::PARAM_INT))) + ->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY))); - $qb->executeStatement(); + $qb->executeStatement(); + } } } diff --git a/apps/user_status/lib/Service/StatusRepairService.php b/apps/user_status/lib/Service/StatusRepairService.php new file mode 100644 index 0000000000000..51f7e8a11150c --- /dev/null +++ b/apps/user_status/lib/Service/StatusRepairService.php @@ -0,0 +1,67 @@ + + */ + public function findStrandedBackupIds(): array { + return $this->mapper->findStrandedBackupIds(StatusService::AUTOMATED_MESSAGE_IDS); + } + + /** + * @return int Number of deleted backup rows + */ + public function deleteStrandedBackups(): int { + return $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + } + + /** + * @return list + */ + public function findOrphanedAutomatedStatusIds(): array { + return $this->mapper->findOrphanedAutomatedStatusIds(StatusService::AUTOMATED_MESSAGE_IDS); + } + + /** + * @return list + */ + public function findStatusesWithoutBackupFlagIds(): array { + return $this->mapper->findStatusesWithoutBackupFlagIds(); + } + + /** + * @param list $ids + * @return int Number of rows given an explicit is_backup value + */ + public function normalizeBackupFlagByIds(array $ids): int { + return $this->mapper->normalizeBackupFlagByIds($ids); + } + + /** + * @param list $ids + * @return int Number of deleted rows + */ + public function deleteByIds(array $ids): int { + return $this->mapper->deleteByIds($ids); + } +} diff --git a/apps/user_status/lib/Service/StatusService.php b/apps/user_status/lib/Service/StatusService.php index c08539c43c43c..bbe28a478538e 100644 --- a/apps/user_status/lib/Service/StatusService.php +++ b/apps/user_status/lib/Service/StatusService.php @@ -17,9 +17,11 @@ use OCA\UserStatus\Exception\InvalidStatusTypeException; use OCA\UserStatus\Exception\StatusMessageTooLongException; use OCP\AppFramework\Db\DoesNotExistException; +use OCP\AppFramework\Db\TTransactional; use OCP\AppFramework\Utility\ITimeFactory; use OCP\DB\Exception; use OCP\IConfig; +use OCP\IDBConnection; use OCP\IEmojiHelper; use OCP\IUserManager; use OCP\UserStatus\IUserStatus; @@ -32,6 +34,8 @@ * @package OCA\UserStatus\Service */ class StatusService { + use TTransactional; + private bool $shareeEnumeration; private bool $shareeEnumerationInGroupOnly; private bool $shareeEnumerationPhone; @@ -60,10 +64,7 @@ class StatusService { ]; /** - * 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. + * Message ids only ever set by an automation, expected to be reverted. */ public const AUTOMATED_MESSAGE_IDS = [ IUserStatus::MESSAGE_CALENDAR_BUSY, @@ -87,6 +88,7 @@ public function __construct( private IConfig $config, private IUserManager $userManager, private LoggerInterface $logger, + private IDBConnection $connection, ) { $this->shareeEnumeration = $this->config->getAppValue('core', 'shareapi_allow_share_dialog_user_enumeration', 'yes') === 'yes'; $this->shareeEnumerationInGroupOnly = $this->shareeEnumeration && $this->config->getAppValue('core', 'shareapi_restrict_user_enumeration_to_group', 'no') === 'yes'; @@ -539,76 +541,83 @@ public function backupCurrentStatus(string $userId): bool { } } + /** + * Looking up the backup, dropping the automated status and promoting the backup + * back to the live row is one unit: in between, the user has no live row and the + * backup looks stranded to the cleanup job, which would delete it. + */ public function revertUserStatus(string $userId, string $messageId, bool $revertedManually = false): ?UserStatus { - try { - /** @var UserStatus $userStatus */ - $backupUserStatus = $this->mapper->findByUserId($userId, true); - } catch (DoesNotExistException $ex) { - // 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 $this->atomic(function () use ($userId, $messageId, $revertedManually): ?UserStatus { + try { + /** @var UserStatus $userStatus */ + $backupUserStatus = $this->mapper->findByUserId($userId, true); + } catch (DoesNotExistException $ex) { + // No backup, but the status must go or the user stays stuck on it. + 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; } - return null; - } - $deleted = $this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId); - if (!$deleted) { - $this->logger->debug('Status revert skipped for user ' . $userId . ': current status does not match messageId "' . $messageId . '" (user may have changed status manually)', ['app' => 'user_status']); - return null; - } + $deleted = $this->mapper->deleteCurrentStatusToRestoreBackup($userId, $messageId); + if (!$deleted) { + $this->logger->debug('Status revert skipped for user ' . $userId . ': current status does not match messageId "' . $messageId . '" (user may have changed status manually)', ['app' => 'user_status']); + return null; + } - if ($revertedManually && $backupUserStatus->getStatus() === IUserStatus::OFFLINE) { - // When the user reverts the status manually they are online - $backupUserStatus->setStatus(IUserStatus::ONLINE); - } + 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()); + // Stale after a long meeting otherwise, which reads as offline. + $backupUserStatus->setStatusTimestamp($this->timeFactory->getTime()); - $backupUserStatus->setIsBackup(false); - // Remove the underscore prefix added when creating the backup - $backupUserStatus->setUserId(substr($backupUserStatus->getUserId(), 1)); - $this->mapper->update($backupUserStatus); + $backupUserStatus->setIsBackup(false); + // Remove the underscore prefix added when creating the backup + $backupUserStatus->setUserId(substr($backupUserStatus->getUserId(), 1)); + $this->mapper->update($backupUserStatus); - return $backupUserStatus; + return $backupUserStatus; + }, $this->connection); } + /** + * Same unit as revertUserStatus(), for the bulk path: the ids are read, the + * automated statuses deleted and the backups restored in one transaction, so + * the cleanup job never sees a backup without its live row. + */ public function revertMultipleUserStatus(array $userIds, string $messageId): void { - // Get all user statuses and the backups - $findById = $userIds; - foreach ($userIds as $userId) { - $findById[] = '_' . $userId; - } - $userStatuses = $this->mapper->findByUserIds($findById); - - $backups = $restoreIds = $statuesToDelete = []; - foreach ($userStatuses as $userStatus) { - if (!$userStatus->getIsBackup() - && $userStatus->getMessageId() === $messageId) { - $statuesToDelete[$userStatus->getUserId()] = $userStatus->getId(); - } elseif ($userStatus->getIsBackup()) { - $backups[$userStatus->getUserId()] = $userStatus->getId(); + $this->atomic(function () use ($userIds, $messageId): void { + // Get all user statuses and the backups + $findById = $userIds; + foreach ($userIds as $userId) { + $findById[] = '_' . $userId; + } + $userStatuses = $this->mapper->findByUserIds($findById); + + $backups = $restoreIds = $statuesToDelete = []; + foreach ($userStatuses as $userStatus) { + if (!$userStatus->getIsBackup() + && $userStatus->getMessageId() === $messageId) { + $statuesToDelete[$userStatus->getUserId()] = $userStatus->getId(); + } elseif ($userStatus->getIsBackup()) { + $backups[$userStatus->getUserId()] = $userStatus->getId(); + } } - } - // For users with both (normal and backup) delete the status when matching - foreach ($statuesToDelete as $userId => $statusId) { - $backupUserId = '_' . $userId; - if (isset($backups[$backupUserId])) { - $restoreIds[] = $backups[$backupUserId]; + // For users with both (normal and backup) delete the status when matching + foreach ($statuesToDelete as $userId => $statusId) { + $backupUserId = '_' . $userId; + if (isset($backups[$backupUserId])) { + $restoreIds[] = $backups[$backupUserId]; + } } - } - $this->mapper->deleteByIds(array_values($statuesToDelete)); + $this->mapper->deleteByIds(array_values($statuesToDelete)); - // For users that matched restore the previous status - $this->mapper->restoreBackupStatuses($restoreIds); + $this->mapper->restoreBackupStatuses($restoreIds, $this->timeFactory->getTime()); + }, $this->connection); } protected function insertWithoutThrowingUniqueConstrain(UserStatus $userStatus): UserStatus { diff --git a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php index 92f8fc5dc0eb9..126813f47e3cf 100644 --- a/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php +++ b/apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php @@ -11,6 +11,7 @@ use OCA\UserStatus\Db\UserStatus; use OCA\UserStatus\Db\UserStatusMapper; +use OCA\UserStatus\Service\StatusRepairService; use OCA\UserStatus\Service\StatusService; use OCP\AppFramework\Db\DoesNotExistException; use OCP\IDBConnection; @@ -24,6 +25,7 @@ class StatusServiceIntegrationTest extends TestCase { private StatusService $service; + private StatusRepairService $repairService; private UserStatusMapper $mapper; private IDBConnection $db; @@ -31,6 +33,7 @@ protected function setUp(): void { parent::setUp(); $this->service = Server::get(StatusService::class); + $this->repairService = Server::get(StatusRepairService::class); $this->mapper = Server::get(UserStatusMapper::class); $this->db = Server::get(IDBConnection::class); @@ -38,10 +41,7 @@ protected function setUp(): void { $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. - */ + /** Reads a row without processStatus() rewriting a stale status first. */ private function readRaw(string $userId): ?UserStatus { try { return $this->mapper->findByUserId($userId); @@ -217,16 +217,9 @@ public function testOtherAutomationsDoNotOverwriteEachOther(): void { ); } - /* - * 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. - */ + /* An automated status with no backup must still be cleared. */ public function testRevertWithoutBackupClearsAutomatedStatus(): void { - // No backup taken, so nothing can ever be restored for this user. $this->service->setUserStatus( 'test123', IUserStatus::BUSY, @@ -274,7 +267,6 @@ public function testRevertWithoutBackupKeepsOtherAutomatedStatus(): void { 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); @@ -286,8 +278,6 @@ public function testRevertWithoutBackupKeepsOtherAutomatedStatus(): void { } 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, @@ -315,8 +305,7 @@ public function testRevertAfterLongMeetingRefreshesTimestamp(): void { true, ); - // A 90 minute meeting, well past INVALIDATE_STATUS_THRESHOLD. - $this->age('test123', 90 * 60); + $this->age('test123', StatusService::INVALIDATE_STATUS_THRESHOLD * 2); $before = time(); $reverted = $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); @@ -337,7 +326,7 @@ public function testRevertAfterLongMeetingDoesNotFallBackToOffline(): void { IUserStatus::MESSAGE_CALENDAR_BUSY, true, ); - $this->age('test123', 90 * 60); + $this->age('test123', StatusService::INVALIDATE_STATUS_THRESHOLD * 2); $this->service->revertUserStatus('test123', IUserStatus::MESSAGE_CALENDAR_BUSY); @@ -349,13 +338,84 @@ public function testRevertAfterLongMeetingDoesNotFallBackToOffline(): void { ); } - /* - * 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 testRevertMultipleAfterLongMeetingRefreshesTimestamp(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + $this->age('test123', StatusService::INVALIDATE_STATUS_THRESHOLD * 2); + + $before = time(); + $this->service->revertMultipleUserStatus(['test123'], IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertGreaterThanOrEqual( + $before, + $this->readRaw('test123')?->getStatusTimestamp(), + 'A bulk-restored status must not carry the stale timestamp from before the meeting', + ); + } + + public function testRevertMultipleAfterLongMeetingDoesNotFallBackToOffline(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + $this->age('test123', StatusService::INVALIDATE_STATUS_THRESHOLD * 2); + + $this->service->revertMultipleUserStatus(['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 after bulk status update followed by reading the status', + ); + } + + public function testRevertMultipleWithoutBackupClearsAutomatedStatus(): void { + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + + $this->service->revertMultipleUserStatus(['test123'], IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertNull( + $this->readRaw('test123'), + 'A bulk revert must clear an automated status that has no backup, or the user stays stuck on it', + ); + } + + public function testRevertMultipleKeepsStatusTheUserChangedThemselves(): void { + $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); + $this->service->setUserStatus( + 'test123', + IUserStatus::BUSY, + IUserStatus::MESSAGE_CALENDAR_BUSY, + true, + ); + // The user replaces the automated message with their own. + $this->service->setCustomMessage('test123', '🍕', 'Lunch', null); + + $this->service->revertMultipleUserStatus(['test123'], IUserStatus::MESSAGE_CALENDAR_BUSY); + + self::assertSame( + 'Lunch', + $this->readRaw('test123')?->getCustomMessage(), + 'A status the user set themselves must survive the bulk revert', + ); + } + + /* A backup whose live row moved off the automated status is stranded. */ public function testStrandedBackupIsCleanedUp(): void { $this->service->setStatus('test123', IUserStatus::ONLINE, null, false); @@ -365,12 +425,10 @@ public function testStrandedBackupIsCleanedUp(): void { 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); + $deleted = $this->repairService->deleteStrandedBackups(); self::assertSame(1, $deleted); self::assertNull($this->readRaw('_test123'), 'The stranded backup must be removed'); @@ -386,7 +444,7 @@ public function testBackupOfAnOngoingMeetingSurvivesCleanup(): void { true, ); - $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + $deleted = $this->repairService->deleteStrandedBackups(); self::assertSame(0, $deleted); self::assertNotNull( @@ -403,10 +461,9 @@ public function testLongOutOfOfficeBackupSurvivesCleanup(): void { IUserStatus::MESSAGE_OUT_OF_OFFICE, true, ); - // Out of office can last for weeks; age well beyond any threshold. - $this->age('test123', 86400 * 30); + $this->age('test123', StatusService::INVALIDATE_STATUS_THRESHOLD * 100); - $deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS); + $deleted = $this->repairService->deleteStrandedBackups(); self::assertSame(0, $deleted); self::assertNotNull( @@ -425,13 +482,12 @@ public function testAutomatedStatusWorksAgainAfterStrandedBackupCleanup(): void ); $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); + $this->repairService->deleteStrandedBackups(); self::assertNotNull( $this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true), @@ -443,20 +499,14 @@ 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); + $deleted = $this->repairService->deleteStrandedBackups(); 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. - */ + /** The prefix concatenation is SQL, so it needs a real database. */ 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, @@ -465,8 +515,6 @@ public function testFindsOrphanedAutomatedStatusOnARealDatabase(): void { ); 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', @@ -475,7 +523,7 @@ public function testFindsOrphanedAutomatedStatusOnARealDatabase(): void { true, ); - $orphaned = $this->mapper->findOrphanedAutomatedStatusIds(StatusService::AUTOMATED_MESSAGE_IDS); + $orphaned = $this->repairService->findOrphanedAutomatedStatusIds(); self::assertSame([$this->readRaw('test123')?->getId()], $orphaned); } diff --git a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php index 57308fdf6cb75..d668a53b7ab1b 100644 --- a/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php +++ b/apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php @@ -11,7 +11,7 @@ use OCA\UserStatus\BackgroundJob\ClearOldStatusesBackgroundJob; use OCA\UserStatus\Db\UserStatusMapper; -use OCA\UserStatus\Service\StatusService; +use OCA\UserStatus\Service\StatusRepairService; use OCP\AppFramework\Utility\ITimeFactory; use PHPUnit\Framework\MockObject\MockObject; use Test\TestCase; @@ -19,6 +19,7 @@ class ClearOldStatusesBackgroundJobTest extends TestCase { private ITimeFactory&MockObject $time; private UserStatusMapper&MockObject $mapper; + private StatusRepairService&MockObject $repairService; private ClearOldStatusesBackgroundJob $job; protected function setUp(): void { @@ -26,8 +27,9 @@ protected function setUp(): void { $this->time = $this->createMock(ITimeFactory::class); $this->mapper = $this->createMock(UserStatusMapper::class); + $this->repairService = $this->createMock(StatusRepairService::class); - $this->job = new ClearOldStatusesBackgroundJob($this->time, $this->mapper); + $this->job = new ClearOldStatusesBackgroundJob($this->time, $this->mapper, $this->repairService); } public function testRun(): void { @@ -37,9 +39,8 @@ 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->repairService->expects($this->once()) + ->method('deleteStrandedBackups'); $this->time->method('getTime') ->willReturn(1337); diff --git a/apps/user_status/tests/Unit/Command/RepairTest.php b/apps/user_status/tests/Unit/Command/RepairTest.php index e4a7eb1d8e2c2..86e6d39988a46 100644 --- a/apps/user_status/tests/Unit/Command/RepairTest.php +++ b/apps/user_status/tests/Unit/Command/RepairTest.php @@ -10,42 +10,39 @@ namespace OCA\UserStatus\Tests\Command; use OCA\UserStatus\Command\Repair; -use OCA\UserStatus\Db\UserStatusMapper; -use OCA\UserStatus\Service\StatusService; +use OCA\UserStatus\Service\StatusRepairService; use PHPUnit\Framework\MockObject\MockObject; use Symfony\Component\Console\Command\Command; use Symfony\Component\Console\Tester\CommandTester; use Test\TestCase; class RepairTest extends TestCase { - private UserStatusMapper&MockObject $mapper; + private StatusRepairService&MockObject $repairService; private CommandTester $tester; protected function setUp(): void { parent::setUp(); - $this->mapper = $this->createMock(UserStatusMapper::class); - $this->tester = new CommandTester(new Repair($this->mapper)); + $this->repairService = $this->createMock(StatusRepairService::class); + $this->tester = new CommandTester(new Repair($this->repairService)); } public function testRepairsEverything(): void { - $this->mapper->expects($this->once()) + $this->repairService->expects($this->once()) ->method('findStatusesWithoutBackupFlagIds') ->willReturn([1, 2]); - $this->mapper->expects($this->once()) + $this->repairService->expects($this->once()) ->method('normalizeBackupFlagByIds') ->with([1, 2]) ->willReturn(2); - $this->mapper->expects($this->once()) + $this->repairService->expects($this->once()) ->method('findOrphanedAutomatedStatusIds') - ->with(StatusService::AUTOMATED_MESSAGE_IDS) ->willReturn([7, 8, 9]); - $this->mapper->expects($this->once()) + $this->repairService->expects($this->once()) ->method('findStrandedBackupIds') - ->with(StatusService::AUTOMATED_MESSAGE_IDS) ->willReturn([11, 12, 13, 14]); - $this->mapper->expects($this->exactly(2)) + $this->repairService->expects($this->exactly(2)) ->method('deleteByIds') ->willReturnCallback(static fn (array $ids): int => count($ids)); @@ -58,13 +55,13 @@ public function testRepairsEverything(): void { } 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->repairService->method('findStatusesWithoutBackupFlagIds')->willReturn([1, 2]); + $this->repairService->method('findOrphanedAutomatedStatusIds')->willReturn([7, 8, 9]); + $this->repairService->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'); + $this->repairService->expects($this->never())->method('normalizeBackupFlagByIds'); + $this->repairService->expects($this->never())->method('deleteByIds'); + $this->repairService->expects($this->never())->method('deleteStrandedBackups'); self::assertSame(Command::SUCCESS, $this->tester->execute(['--dry-run' => true])); @@ -72,13 +69,13 @@ public function testDryRunChangesNothing(): void { } public function testNothingToRepair(): void { - $this->mapper->method('findStatusesWithoutBackupFlagIds')->willReturn([]); - $this->mapper->method('findOrphanedAutomatedStatusIds')->willReturn([]); - $this->mapper->method('findStrandedBackupIds')->willReturn([]); + $this->repairService->method('findStatusesWithoutBackupFlagIds')->willReturn([]); + $this->repairService->method('findOrphanedAutomatedStatusIds')->willReturn([]); + $this->repairService->method('findStrandedBackupIds')->willReturn([]); // Nothing to normalise and nothing to delete. - $this->mapper->expects($this->never())->method('normalizeBackupFlagByIds'); - $this->mapper->expects($this->never())->method('deleteByIds'); + $this->repairService->expects($this->never())->method('normalizeBackupFlagByIds'); + $this->repairService->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 4b03fa0ab9ba6..c3c93b449c369 100644 --- a/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php +++ b/apps/user_status/tests/Unit/Db/UserStatusMapperTest.php @@ -388,12 +388,13 @@ public function testRestoreBackupStatuses(): void { $userStatus3->setClearAt(50000); $this->mapper->insert($userStatus3); - $this->mapper->restoreBackupStatuses([$userStatus1->getId(), $userStatus2->getId()]); + $this->mapper->restoreBackupStatuses([$userStatus1->getId(), $userStatus2->getId()], 123456); $user1Status = $this->mapper->findByUserId('user1', false); $this->assertEquals('user1', $user1Status->getUserId()); $this->assertEquals(false, $user1Status->getIsBackup()); $this->assertEquals('Releasing', $user1Status->getCustomMessage()); + $this->assertSame(123456, $user1Status->getStatusTimestamp(), 'A restored status becomes current, so it carries the restore timestamp'); $user2Status = $this->mapper->findByUserId('user2', false); $this->assertEquals('user2', $user2Status->getUserId()); @@ -469,7 +470,6 @@ public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusHasNoMessage $deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']); $this->assertSame(1, $deleted); - // The live status must survive. $this->assertEquals('user1', $this->mapper->findByUserId('user1')->getUserId()); } @@ -503,8 +503,7 @@ public function testDeleteStrandedBackupsOnlyRemovesTheStrandedOnes(): void { } 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. + // 'user1x' is a real user, judged on its own live row, not user1's. $this->insertBackupWithLiveStatus('user1', 'meeting'); $this->insertBackupWithLiveStatus('user1x', 'vacationing'); @@ -538,7 +537,6 @@ public function testFindStrandedBackupIdsDoesNotDelete(): void { } public function testFindOrphanedAutomatedStatusIds(): void { - // Live automated status with no backup to revert into: orphaned. $orphan = new UserStatus(); $orphan->setUserId('orphan'); $orphan->setStatus('busy'); @@ -548,10 +546,8 @@ public function testFindOrphanedAutomatedStatusIds(): void { $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'); @@ -568,8 +564,7 @@ public function testFindOrphanedAutomatedStatusIds(): void { } public function testFindOrphanedAutomatedStatusIdsIgnoresBackupRows(): void { - // A backup row that happens to carry an automated message id must never - // be reported as an orphaned live status. + // A backup carrying an automated id is not an orphaned live status. $backup = new UserStatus(); $backup->setUserId('_someone'); $backup->setStatus('busy'); @@ -599,7 +594,6 @@ public function testNormalizeBackupFlag(): void { $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()); } @@ -610,11 +604,34 @@ public function testNormalizeBackupFlagWithNothingToDo(): void { $this->assertSame(0, $this->mapper->normalizeBackupFlagByIds([])); } - public function testDeleteStrandedBackupsWithEmptyAutomatedListRemovesAll(): void { + public function testDeleteStrandedBackupsWithEmptyAutomatedListDoesNothing(): void { $this->insertBackupWithLiveStatus('user1', 'meeting'); $this->insertBackupWithLiveStatus('user2', 'call'); - // Defensive: with nothing considered automated, every backup is stranded. - $this->assertSame(2, $this->mapper->deleteStrandedBackups([])); + $this->assertSame(0, $this->mapper->deleteStrandedBackups([])); + $this->assertNotNull($this->mapper->findByUserId('user1', true)); + $this->assertNotNull($this->mapper->findByUserId('user2', true)); + } + + public function testNormalizeBackupFlagKeepsBackupRowsAsBackups(): void { + self::$realDatabase->executeStatement( + 'INSERT INTO `*PREFIX*user_status` (`user_id`, `status`, `status_timestamp`, `is_user_defined`, `is_backup`) VALUES (?, ?, ?, ?, ?)', + ['user1', 'online', 5000, 0, null], + ); + self::$realDatabase->executeStatement( + 'INSERT INTO `*PREFIX*user_status` (`user_id`, `status`, `status_timestamp`, `is_user_defined`, `is_backup`) VALUES (?, ?, ?, ?, ?)', + ['_user1', 'away', 4000, 0, null], + ); + + $ids = $this->mapper->findStatusesWithoutBackupFlagIds(); + $this->assertCount(2, $ids); + $this->assertSame(2, $this->mapper->normalizeBackupFlagByIds($ids)); + + $this->assertSame([], $this->mapper->findStatusesWithoutBackupFlagIds()); + $this->assertFalse($this->mapper->findByUserId('user1', false)->getIsBackup()); + $this->assertTrue( + $this->mapper->findByUserId('user1', true)->getIsBackup(), + 'A backup row that predates the column default must stay a backup, or no revert can ever restore it', + ); } } diff --git a/apps/user_status/tests/Unit/Service/StatusRepairServiceTest.php b/apps/user_status/tests/Unit/Service/StatusRepairServiceTest.php new file mode 100644 index 0000000000000..b614edd54e6ea --- /dev/null +++ b/apps/user_status/tests/Unit/Service/StatusRepairServiceTest.php @@ -0,0 +1,52 @@ +mapper = $this->createMock(UserStatusMapper::class); + $this->service = new StatusRepairService($this->mapper); + } + + /** The reason this class exists. */ + public function testStrandedIsAlwaysDecidedAgainstTheAutomatedMessageIds(): void { + $this->mapper->expects($this->once()) + ->method('findStrandedBackupIds') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn([1, 2]); + $this->mapper->expects($this->once()) + ->method('deleteStrandedBackups') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn(2); + $this->mapper->expects($this->once()) + ->method('findOrphanedAutomatedStatusIds') + ->with(StatusService::AUTOMATED_MESSAGE_IDS) + ->willReturn([3]); + + self::assertSame([1, 2], $this->service->findStrandedBackupIds()); + self::assertSame(2, $this->service->deleteStrandedBackups()); + self::assertSame([3], $this->service->findOrphanedAutomatedStatusIds()); + } + + public function testAutomatedMessageIdsAreNeverEmpty(): void { + self::assertNotEmpty(StatusService::AUTOMATED_MESSAGE_IDS); + } +} diff --git a/apps/user_status/tests/Unit/Service/StatusServiceTest.php b/apps/user_status/tests/Unit/Service/StatusServiceTest.php index b42370991503c..0e33083a1049a 100644 --- a/apps/user_status/tests/Unit/Service/StatusServiceTest.php +++ b/apps/user_status/tests/Unit/Service/StatusServiceTest.php @@ -22,6 +22,7 @@ use OCP\AppFramework\Utility\ITimeFactory; use OCP\DB\Exception; use OCP\IConfig; +use OCP\IDBConnection; use OCP\IEmojiHelper; use OCP\IUserManager; use OCP\UserStatus\IUserStatus; @@ -37,6 +38,7 @@ class StatusServiceTest extends TestCase { private IConfig&MockObject $config; private IUserManager&MockObject $userManager; private LoggerInterface&MockObject $logger; + private IDBConnection&MockObject $connection; private StatusService $service; @@ -50,6 +52,7 @@ protected function setUp(): void { $this->userManager = $this->createMock(IUserManager::class); $this->config = $this->createMock(IConfig::class); $this->logger = $this->createMock(LoggerInterface::class); + $this->connection = $this->createMock(IDBConnection::class); $this->config->method('getAppValue') ->willReturnMap([ @@ -64,6 +67,7 @@ protected function setUp(): void { $this->config, $this->userManager, $this->logger, + $this->connection, ); } @@ -121,6 +125,7 @@ public function testFindAllRecentStatusChangesNoEnumeration(): void { $this->config, $this->userManager, $this->logger, + $this->connection, ); $this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50)); @@ -141,6 +146,7 @@ public function testFindAllRecentStatusChangesNoEnumeration(): void { $this->config, $this->userManager, $this->logger, + $this->connection, ); $this->assertEquals([], $this->service->findAllRecentStatusChanges(20, 50)); @@ -861,13 +867,161 @@ public function testRevertMultipleUserStatus(): void { ->method('deleteByIds') ->with([1, 3, 5]); + $this->timeFactory->method('getTime') + ->willReturn(1337); + $this->mapper->expects($this->once()) ->method('restoreBackupStatuses') - ->with([2]); + ->with([2], 1337); $this->service->revertMultipleUserStatus(['john', 'nobackup', 'backuponly', 'nobackupanddnd'], 'call'); } + /** + * The delete and the restore must not be observable separately: in between, the + * backup has no live row and the cleanup job would treat it as stranded. + */ + public function testRevertUserStatusRunsInOneTransaction(): void { + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setStatus(IUserStatus::ONLINE); + $backup->setStatusTimestamp(1000); + $backup->setIsBackup(true); + + $calls = []; + $this->connection->expects($this->once()) + ->method('beginTransaction') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'begin'; + }); + $this->connection->expects($this->once()) + ->method('commit') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'commit'; + }); + $this->connection->expects($this->never())->method('rollBack'); + + $this->mapper->method('findByUserId') + ->with('john', true) + ->willReturnCallback(function () use (&$calls, $backup): UserStatus { + $calls[] = 'find'; + return $backup; + }); + $this->mapper->method('deleteCurrentStatusToRestoreBackup') + ->willReturnCallback(function () use (&$calls): bool { + $calls[] = 'delete'; + return true; + }); + $this->mapper->method('update') + ->willReturnCallback(function (UserStatus $status) use (&$calls): UserStatus { + $calls[] = 'restore'; + return $status; + }); + $this->timeFactory->method('getTime')->willReturn(9999); + + $this->service->revertUserStatus('john', 'meeting'); + + self::assertSame(['begin', 'find', 'delete', 'restore', 'commit'], $calls); + } + + public function testRevertUserStatusRollsBackWhenTheRestoreFails(): void { + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setStatus(IUserStatus::ONLINE); + $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->method('update')->willThrowException(new \RuntimeException('nope')); + + $this->connection->expects($this->once())->method('beginTransaction'); + $this->connection->expects($this->never())->method('commit'); + // Without the rollback the automated status would stay deleted while the + // backup keeps its backup flag, which strands the backup for good. + $this->connection->expects($this->once())->method('rollBack'); + + $this->expectException(\RuntimeException::class); + + $this->service->revertUserStatus('john', 'meeting'); + } + + public function testRevertMultipleUserStatusRunsInOneTransaction(): void { + $live = new UserStatus(); + $live->setId(1); + $live->setUserId('john'); + $live->setMessageId('call'); + $live->setIsBackup(false); + + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setMessageId('hello'); + $backup->setIsBackup(true); + + $calls = []; + $this->connection->expects($this->once()) + ->method('beginTransaction') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'begin'; + }); + $this->connection->expects($this->once()) + ->method('commit') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'commit'; + }); + $this->connection->expects($this->never())->method('rollBack'); + + $this->mapper->method('findByUserIds') + ->willReturnCallback(function () use (&$calls, $live, $backup): array { + $calls[] = 'find'; + return [$live, $backup]; + }); + $this->mapper->method('deleteByIds') + ->willReturnCallback(function (array $ids) use (&$calls): int { + $calls[] = 'delete'; + return count($ids); + }); + $this->mapper->method('restoreBackupStatuses') + ->willReturnCallback(function () use (&$calls): void { + $calls[] = 'restore'; + }); + $this->timeFactory->method('getTime')->willReturn(1337); + + $this->service->revertMultipleUserStatus(['john'], 'call'); + + self::assertSame(['begin', 'find', 'delete', 'restore', 'commit'], $calls); + } + + public function testRevertMultipleUserStatusRollsBackWhenTheRestoreFails(): void { + $live = new UserStatus(); + $live->setId(1); + $live->setUserId('john'); + $live->setMessageId('call'); + $live->setIsBackup(false); + + $backup = new UserStatus(); + $backup->setId(2); + $backup->setUserId('_john'); + $backup->setMessageId('hello'); + $backup->setIsBackup(true); + + $this->mapper->method('findByUserIds')->willReturn([$live, $backup]); + $this->mapper->method('deleteByIds')->willReturn(1); + $this->timeFactory->method('getTime')->willReturn(1337); + $this->mapper->method('restoreBackupStatuses')->willThrowException(new \RuntimeException('nope')); + + $this->connection->expects($this->once())->method('beginTransaction'); + $this->connection->expects($this->never())->method('commit'); + $this->connection->expects($this->once())->method('rollBack'); + + $this->expectException(\RuntimeException::class); + + $this->service->revertMultipleUserStatus(['john'], 'call'); + } + public static function dataSetUserStatus(): array { return [ [IUserStatus::MESSAGE_CALENDAR_BUSY, '', false],