Skip to content

Commit a9a5eb3

Browse files
miaulalalamiaulalala
committed
fix(user_status): delete stranded backup statuses in the cleanup job
A backup can only be restored by revertUserStatus(), which matches on the live row still carrying the automated message id. Once that no longer holds the backup is unreachable, and since 33.0.7 excluded backups from clearOlderThanClearAt() nothing removes it any more. createBackupStatus() then keeps hitting the unique constraint on user_id, so setUserStatus() silently aborts every later automated status change for that user. Delete unreachable backups from the existing cleanup job. The check is state based rather than age based on purpose: an out-of-office backup can legitimately be weeks old. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch <anna@nextcloud.com>
1 parent 6a857b5 commit a9a5eb3

6 files changed

Lines changed: 311 additions & 0 deletions

File tree

‎apps/user_status/lib/BackgroundJob/ClearOldStatusesBackgroundJob.php‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,5 +45,6 @@ protected function run($argument) {
4545

4646
$this->mapper->clearOlderThanClearAt($now);
4747
$this->mapper->clearStatusesOlderThan($now - StatusService::INVALIDATE_STATUS_THRESHOLD, $now);
48+
$this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS);
4849
}
4950
}

‎apps/user_status/lib/Db/UserStatusMapper.php‎

Lines changed: 74 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -163,6 +163,80 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa
163163
return $qb->executeStatement() > 0;
164164
}
165165

166+
/**
167+
* Deletes backup rows that can never be restored, because the matching live
168+
* status is gone or is no longer on one of the automated statuses that would
169+
* revert into it.
170+
*
171+
* Such a row is not just clutter: while it exists, createBackupStatus() keeps
172+
* hitting the unique constraint on user_id, which makes setUserStatus()
173+
* silently abort every automated status change for that user.
174+
*
175+
* @param list<string> $automatedMessageIds Message ids that own a backup
176+
* @return int Number of deleted backup rows
177+
*/
178+
public function deleteStrandedBackups(array $automatedMessageIds): int {
179+
$qb = $this->db->getQueryBuilder();
180+
$qb->select('id', 'user_id')
181+
->from($this->tableName)
182+
->where($qb->expr()->eq('is_backup', $qb->createNamedParameter(true, IQueryBuilder::PARAM_BOOL)));
183+
184+
$result = $qb->executeQuery();
185+
/** @var array<string, int> $backups live user id => backup row id */
186+
$backups = [];
187+
while ($row = $result->fetch()) {
188+
// Strip the underscore prefix that was added when creating the backup
189+
$backups[substr((string)$row['user_id'], 1)] = (int)$row['id'];
190+
}
191+
$result->closeCursor();
192+
193+
if ($backups === []) {
194+
return 0;
195+
}
196+
197+
$reachable = [];
198+
if ($automatedMessageIds !== []) {
199+
foreach (array_chunk(array_keys($backups), 1000) as $chunk) {
200+
$qb = $this->db->getQueryBuilder();
201+
// Matching on the exact user id is enough to exclude backup rows,
202+
// since those are always prefixed and user ids cannot start with
203+
// an underscore. Not filtering on is_backup also means a row with
204+
// a NULL is_backup errs towards keeping the backup.
205+
$qb->select('user_id')
206+
->from($this->tableName)
207+
->where($qb->expr()->in('user_id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_STR_ARRAY)))
208+
->andWhere($qb->expr()->in('message_id', $qb->createNamedParameter($automatedMessageIds, IQueryBuilder::PARAM_STR_ARRAY)));
209+
210+
$liveResult = $qb->executeQuery();
211+
while ($row = $liveResult->fetch()) {
212+
$reachable[(string)$row['user_id']] = true;
213+
}
214+
$liveResult->closeCursor();
215+
}
216+
}
217+
218+
$stranded = [];
219+
foreach ($backups as $userId => $id) {
220+
if (!isset($reachable[$userId])) {
221+
$stranded[] = $id;
222+
}
223+
}
224+
225+
if ($stranded === []) {
226+
return 0;
227+
}
228+
229+
$deleted = 0;
230+
foreach (array_chunk($stranded, 1000) as $chunk) {
231+
$qb = $this->db->getQueryBuilder();
232+
$qb->delete($this->tableName)
233+
->where($qb->expr()->in('id', $qb->createNamedParameter($chunk, IQueryBuilder::PARAM_INT_ARRAY)));
234+
$deleted += $qb->executeStatement();
235+
}
236+
237+
return $deleted;
238+
}
239+
166240
public function deleteByIds(array $ids): void {
167241
$qb = $this->db->getQueryBuilder();
168242
$qb->delete($this->tableName)

‎apps/user_status/lib/Service/StatusService.php‎

Lines changed: 14 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,20 @@ class StatusService {
5959
IUserStatus::INVISIBLE,
6060
];
6161

62+
/**
63+
* Message ids that are only ever set by an automation (calendar, call,
64+
* availability, out-of-office). A status carrying one of these owns the
65+
* backup of whatever the user had set before, and is expected to be
66+
* reverted once the automation stops applying.
67+
*/
68+
public const AUTOMATED_MESSAGE_IDS = [
69+
IUserStatus::MESSAGE_CALENDAR_BUSY,
70+
IUserStatus::MESSAGE_CALENDAR_BUSY_TENTATIVE,
71+
IUserStatus::MESSAGE_CALL,
72+
IUserStatus::MESSAGE_AVAILABILITY,
73+
IUserStatus::MESSAGE_OUT_OF_OFFICE,
74+
];
75+
6276
/** @var int */
6377
public const INVALIDATE_STATUS_THRESHOLD = 15 /* minutes */ * 60 /* seconds */;
6478

‎apps/user_status/tests/Integration/Service/StatusServiceIntegrationTest.php‎

Lines changed: 100 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -348,4 +348,104 @@ public function testRevertAfterLongMeetingDoesNotFallBackToOffline(): void {
348348
'The user was online before the meeting and must not be flipped to offline by reading the status',
349349
);
350350
}
351+
352+
/*
353+
* Stranded backups: a backup row exists but the live row is no longer on
354+
* the automated status that would restore it, so revertUserStatus() can
355+
* never match. Nothing else removes it, and while it exists
356+
* backupCurrentStatus() keeps failing, which silently aborts every future
357+
* automated status change for that user.
358+
*/
359+
360+
public function testStrandedBackupIsCleanedUp(): void {
361+
$this->service->setStatus('test123', IUserStatus::ONLINE, null, false);
362+
$this->service->setUserStatus(
363+
'test123',
364+
IUserStatus::BUSY,
365+
IUserStatus::MESSAGE_CALENDAR_BUSY,
366+
true,
367+
);
368+
// The user clears the status message, so the meeting revert can no
369+
// longer find a matching row.
370+
$this->service->clearMessage('test123');
371+
self::assertNotNull($this->readRaw('_test123'), 'Precondition: the backup is stranded');
372+
373+
$deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS);
374+
375+
self::assertSame(1, $deleted);
376+
self::assertNull($this->readRaw('_test123'), 'The stranded backup must be removed');
377+
self::assertNotNull($this->readRaw('test123'), 'The live status must be untouched');
378+
}
379+
380+
public function testBackupOfAnOngoingMeetingSurvivesCleanup(): void {
381+
$this->service->setStatus('test123', IUserStatus::ONLINE, null, false);
382+
$this->service->setUserStatus(
383+
'test123',
384+
IUserStatus::BUSY,
385+
IUserStatus::MESSAGE_CALENDAR_BUSY,
386+
true,
387+
);
388+
389+
$deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS);
390+
391+
self::assertSame(0, $deleted);
392+
self::assertNotNull(
393+
$this->readRaw('_test123'),
394+
'The backup for a meeting that is still running must survive',
395+
);
396+
}
397+
398+
public function testLongOutOfOfficeBackupSurvivesCleanup(): void {
399+
$this->service->setStatus('test123', IUserStatus::ONLINE, null, false);
400+
$this->service->setUserStatus(
401+
'test123',
402+
IUserStatus::DND,
403+
IUserStatus::MESSAGE_OUT_OF_OFFICE,
404+
true,
405+
);
406+
// Out of office can last for weeks; age well beyond any threshold.
407+
$this->age('test123', 86400 * 30);
408+
409+
$deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS);
410+
411+
self::assertSame(0, $deleted);
412+
self::assertNotNull(
413+
$this->readRaw('_test123'),
414+
'A long running out-of-office backup must not be treated as stranded',
415+
);
416+
}
417+
418+
public function testAutomatedStatusWorksAgainAfterStrandedBackupCleanup(): void {
419+
$this->service->setStatus('test123', IUserStatus::ONLINE, null, false);
420+
$this->service->setUserStatus(
421+
'test123',
422+
IUserStatus::BUSY,
423+
IUserStatus::MESSAGE_CALENDAR_BUSY,
424+
true,
425+
);
426+
$this->service->clearMessage('test123');
427+
428+
// While the stranded backup exists, automated statuses are aborted.
429+
self::assertNull(
430+
$this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true),
431+
'Precondition: the stranded backup blocks automated statuses',
432+
);
433+
434+
$this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS);
435+
436+
self::assertNotNull(
437+
$this->service->setUserStatus('test123', IUserStatus::BUSY, IUserStatus::MESSAGE_CALL, true),
438+
'Automated statuses must work again once the stranded backup is gone',
439+
);
440+
}
441+
442+
public function testCleanupLeavesUsersWithoutBackupsAlone(): void {
443+
$this->service->setStatus('test123', IUserStatus::ONLINE, null, false);
444+
$this->service->setCustomMessage('test123', '🍕', 'Lunch', null);
445+
446+
$deleted = $this->mapper->deleteStrandedBackups(StatusService::AUTOMATED_MESSAGE_IDS);
447+
448+
self::assertSame(0, $deleted);
449+
self::assertSame('Lunch', $this->readRaw('test123')?->getCustomMessage());
450+
}
351451
}

‎apps/user_status/tests/Unit/BackgroundJob/ClearOldStatusesBackgroundJobTest.php‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111

1212
use OCA\UserStatus\BackgroundJob\ClearOldStatusesBackgroundJob;
1313
use OCA\UserStatus\Db\UserStatusMapper;
14+
use OCA\UserStatus\Service\StatusService;
1415
use OCP\AppFramework\Utility\ITimeFactory;
1516
use PHPUnit\Framework\MockObject\MockObject;
1617
use Test\TestCase;
@@ -36,6 +37,9 @@ public function testRun(): void {
3637
$this->mapper->expects($this->once())
3738
->method('clearStatusesOlderThan')
3839
->with(437, 1337);
40+
$this->mapper->expects($this->once())
41+
->method('deleteStrandedBackups')
42+
->with(StatusService::AUTOMATED_MESSAGE_IDS);
3943

4044
$this->time->method('getTime')
4145
->willReturn(1337);

‎apps/user_status/tests/Unit/Db/UserStatusMapperTest.php‎

Lines changed: 118 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -405,4 +405,122 @@ public function testRestoreBackupStatuses(): void {
405405
$this->assertEquals(true, $user3Status->getIsBackup());
406406
$this->assertEquals('Vacationing', $user3Status->getCustomMessage());
407407
}
408+
409+
/**
410+
* @param string[] $liveMessageIds keyed by user id; null means no live row
411+
*/
412+
private function insertBackupWithLiveStatus(string $userId, ?string $liveMessageId): void {
413+
$backup = new UserStatus();
414+
$backup->setUserId('_' . $userId);
415+
$backup->setStatus('online');
416+
$backup->setStatusTimestamp(5000);
417+
$backup->setIsUserDefined(false);
418+
$backup->setIsBackup(true);
419+
$this->mapper->insert($backup);
420+
421+
if ($liveMessageId === null) {
422+
return;
423+
}
424+
425+
$live = new UserStatus();
426+
$live->setUserId($userId);
427+
$live->setStatus('busy');
428+
$live->setStatusTimestamp(6000);
429+
$live->setIsUserDefined(true);
430+
$live->setIsBackup(false);
431+
$live->setMessageId($liveMessageId === '' ? null : $liveMessageId);
432+
$this->mapper->insert($live);
433+
}
434+
435+
public function testDeleteStrandedBackupsWithNoBackups(): void {
436+
$this->insertSampleStatuses();
437+
438+
$this->assertSame(0, $this->mapper->deleteStrandedBackups(['meeting', 'call']));
439+
$this->assertCount(3, $this->mapper->findAll());
440+
}
441+
442+
public function testDeleteStrandedBackupsKeepsBackupsOfAutomatedStatuses(): void {
443+
$this->insertBackupWithLiveStatus('user1', 'meeting');
444+
$this->insertBackupWithLiveStatus('user2', 'call');
445+
$this->insertBackupWithLiveStatus('user3', 'availability');
446+
$this->insertBackupWithLiveStatus('user4', 'out-of-office');
447+
448+
$deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call', 'availability', 'out-of-office']);
449+
450+
$this->assertSame(0, $deleted);
451+
foreach (['user1', 'user2', 'user3', 'user4'] as $userId) {
452+
$this->assertEquals('_' . $userId, $this->mapper->findByUserId($userId, true)->getUserId());
453+
}
454+
}
455+
456+
public function testDeleteStrandedBackupsRemovesBackupWithoutLiveStatus(): void {
457+
$this->insertBackupWithLiveStatus('user1', null);
458+
459+
$deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']);
460+
461+
$this->assertSame(1, $deleted);
462+
$this->expectException(DoesNotExistException::class);
463+
$this->mapper->findByUserId('user1', true);
464+
}
465+
466+
public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusHasNoMessageId(): void {
467+
$this->insertBackupWithLiveStatus('user1', '');
468+
469+
$deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']);
470+
471+
$this->assertSame(1, $deleted);
472+
// The live status must survive.
473+
$this->assertEquals('user1', $this->mapper->findByUserId('user1')->getUserId());
474+
}
475+
476+
public function testDeleteStrandedBackupsRemovesBackupWhenLiveStatusIsUserDefinedMessage(): void {
477+
$this->insertBackupWithLiveStatus('user1', 'vacationing');
478+
479+
$deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']);
480+
481+
$this->assertSame(1, $deleted);
482+
$this->assertEquals('vacationing', $this->mapper->findByUserId('user1')->getMessageId());
483+
}
484+
485+
public function testDeleteStrandedBackupsOnlyRemovesTheStrandedOnes(): void {
486+
$this->insertBackupWithLiveStatus('keepme', 'meeting');
487+
$this->insertBackupWithLiveStatus('stranded1', 'vacationing');
488+
$this->insertBackupWithLiveStatus('stranded2', null);
489+
$this->insertBackupWithLiveStatus('keepme2', 'call');
490+
491+
$deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']);
492+
493+
$this->assertSame(2, $deleted);
494+
$this->assertEquals('_keepme', $this->mapper->findByUserId('keepme', true)->getUserId());
495+
$this->assertEquals('_keepme2', $this->mapper->findByUserId('keepme2', true)->getUserId());
496+
foreach (['stranded1', 'stranded2'] as $userId) {
497+
try {
498+
$this->mapper->findByUserId($userId, true);
499+
$this->fail("Backup for $userId should have been deleted");
500+
} catch (DoesNotExistException) {
501+
}
502+
}
503+
}
504+
505+
public function testDeleteStrandedBackupsDoesNotConfuseUsersWithSimilarNames(): void {
506+
// '_user1' as a backup of 'user1', plus a real user literally named
507+
// 'user1x' whose backup must be judged on its own live row.
508+
$this->insertBackupWithLiveStatus('user1', 'meeting');
509+
$this->insertBackupWithLiveStatus('user1x', 'vacationing');
510+
511+
$deleted = $this->mapper->deleteStrandedBackups(['meeting', 'call']);
512+
513+
$this->assertSame(1, $deleted);
514+
$this->assertEquals('_user1', $this->mapper->findByUserId('user1', true)->getUserId());
515+
$this->expectException(DoesNotExistException::class);
516+
$this->mapper->findByUserId('user1x', true);
517+
}
518+
519+
public function testDeleteStrandedBackupsWithEmptyAutomatedListRemovesAll(): void {
520+
$this->insertBackupWithLiveStatus('user1', 'meeting');
521+
$this->insertBackupWithLiveStatus('user2', 'call');
522+
523+
// Defensive: with nothing considered automated, every backup is stranded.
524+
$this->assertSame(2, $this->mapper->deleteStrandedBackups([]));
525+
}
408526
}

0 commit comments

Comments
 (0)