-
-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
c66fef6
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 |
|---|---|---|
|
|
@@ -163,11 +163,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 { | ||
|
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. The definition of what stranded means changes depending on the input, and the behaviour with the empty array makes little sense to me as it deletes all backup rows. Since the argument passed is always the same, maybe it would be better to bake the list in the mapper, so that the those functions keep a consistent meaning that cannot be changed from the outside. Another option would be to keep the parameter in the mapper, but add a repair service without parameter for those functions, so the logic of what stranded means stays in one place and does not require referencing the list from the service to use the mapper. |
||
| 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); | ||
| } | ||
|
miaulalala marked this conversation as resolved.
|
||
|
|
||
| /** | ||
| * 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, 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(); | ||
| } | ||
|
|
||
| return $updated; | ||
| } | ||
|
|
||
| /** | ||
| * @param list<int> $ids | ||
| * @return int Number of deleted rows | ||
| */ | ||
| public function deleteByIds(array $ids): int { | ||
| $deleted = 0; | ||
| 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))); | ||
| $deleted += $qb->executeStatement(); | ||
| } | ||
|
|
||
| return $deleted; | ||
| } | ||
|
|
||
| /** | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.