-
-
Notifications
You must be signed in to change notification settings - Fork 5.1k
fix(user_status): fix the status lifecycle #63151
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
aff361e
6a857b5
a9a5eb3
8204069
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,129 @@ | ||
| <?php | ||
|
|
||
| declare(strict_types=1); | ||
|
|
||
| /** | ||
| * SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors | ||
| * SPDX-License-Identifier: AGPL-3.0-or-later | ||
| */ | ||
|
|
||
| namespace OCA\UserStatus\Command; | ||
|
|
||
| use OCA\UserStatus\Db\UserStatusMapper; | ||
| use OCA\UserStatus\Service\StatusService; | ||
| use Symfony\Component\Console\Command\Command; | ||
| use Symfony\Component\Console\Input\InputInterface; | ||
| use Symfony\Component\Console\Input\InputOption; | ||
| use Symfony\Component\Console\Output\OutputInterface; | ||
|
|
||
| class Repair extends Command { | ||
|
|
||
| public function __construct( | ||
| private UserStatusMapper $mapper, | ||
| ) { | ||
| parent::__construct(); | ||
| } | ||
|
|
||
| #[\Override] | ||
| protected function configure(): void { | ||
| $this | ||
| ->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('<comment>Dry run, no changes will be written.</comment>'); | ||
| $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 <info>$count</info> status(es)."); | ||
| $this->listIds($output, $ids); | ||
| return; | ||
| } | ||
|
|
||
| $fixed = $this->mapper->normalizeBackupFlagByIds($ids); | ||
| $output->writeln("Set the backup flag on <info>$fixed</info> 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 <info>' . count($ids) . '</info> status(es) stuck on an automated status.'); | ||
| $this->listIds($output, $ids); | ||
| return; | ||
| } | ||
|
|
||
| $deleted = $this->mapper->deleteByIds($ids); | ||
| $output->writeln("Cleared <info>$deleted</info> 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 <info>' . count($ids) . '</info> stranded backup status(es).'); | ||
| $this->listIds($output, $ids); | ||
| return; | ||
| } | ||
|
|
||
| $deleted = $this->mapper->deleteByIds($ids); | ||
| $output->writeln("Removed <info>$deleted</info> 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<int> $ids | ||
| */ | ||
| private function listIds(OutputInterface $output, array $ids): void { | ||
| if ($output->getVerbosity() >= OutputInterface::VERBOSITY_VERBOSE) { | ||
| $output->writeln(' ids: ' . implode(', ', $ids)); | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
| */ | ||
|
|
@@ -163,11 +169,152 @@ public function deleteCurrentStatusToRestoreBackup(string $userId, string $messa | |
| return $qb->executeStatement() > 0; | ||
| } | ||
|
|
||
| public function deleteByIds(array $ids): void { | ||
| /** | ||
| * 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<string> $automatedMessageIds Message ids that own a backup | ||
| * @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<string> $automatedMessageIds | ||
| * @return list<int> | ||
| */ | ||
| public function findStrandedBackupIds(array $automatedMessageIds): array { | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->delete($this->tableName) | ||
| ->where($qb->expr()->in('id', $qb->createNamedParameter($ids, IQueryBuilder::PARAM_INT_ARRAY))); | ||
| $qb->executeStatement(); | ||
| $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); | ||
| } | ||
|
Comment on lines
+199
to
+220
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. All in all this reads really convoluted to me, I tried to understand what it does but I’m still a bit lost. Is it not feasible in one request with a join? Or would that be a bad idea for some reason?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Agreed, changed to self join. Added some tests to make sure it also works on Oracle, let's see what CI says. |
||
|
|
||
| /** | ||
| * 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<string> $automatedMessageIds | ||
| * @return list<int> | ||
| */ | ||
| 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<int> | ||
| */ | ||
| private function fetchIds(IQueryBuilder $qb): array { | ||
| $result = $qb->executeQuery(); | ||
| $ids = []; | ||
| while ($row = $result->fetch()) { | ||
| $ids[] = (int)$row['id']; | ||
| } | ||
| $result->closeCursor(); | ||
|
|
||
| return $ids; | ||
| } | ||
|
|
||
| /** | ||
| * 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<int> | ||
| */ | ||
| public function findStatusesWithoutBackupFlagIds(): array { | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->select('id') | ||
| ->from($this->tableName) | ||
| ->where($qb->expr()->isNull('is_backup')); | ||
|
|
||
| return $this->fetchIds($qb); | ||
| } | ||
|
|
||
| /** | ||
| * @param list<int> $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<int> $ids | ||
| * @return int Number of deleted rows | ||
| */ | ||
| public function deleteByIds(array $ids): int { | ||
| $deleted = 0; | ||
| 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))); | ||
| $deleted += $qb->executeStatement(); | ||
| } | ||
|
|
||
| return $deleted; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.