fix(user_status): fix the status lifecycle - #63151
Open
miaulalala wants to merge 4 commits into
Open
Conversation
miaulalala
requested review from
a team,
Antreesy and
nickvergessen
as code owners
August 11, 2026 08:39
miaulalala
requested review from
Altahrim,
icewind1991 and
salmart-dev
and removed request for
a team
August 11, 2026 08:39
miaulalala
changed the title
Contributor
Author
|
/backport to stable34 |
Contributor
Author
|
/backport to stable33 |
Contributor
Author
|
/backport to stable32 |
miaulalala
added
3. to review
come-nc
reviewed
Aug 11, 2026
come-nc
left a comment
come-nc
left a comment
Contributor
There was a problem hiding this comment.
The commit descriptions are really verbose, that’s not helping much.
Comment on lines
+201
to
+232
| 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))); | ||
|
|
||
| $result = $qb->executeQuery(); | ||
| /** @var array<string, int> $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 []; | ||
| } | ||
|
|
||
| $reachable = $automatedMessageIds === [] | ||
| ? [] | ||
| : $this->findUsersOnAutomatedStatus(array_keys($backups), $automatedMessageIds); | ||
|
|
||
| $stranded = []; | ||
| foreach ($backups as $userId => $id) { | ||
| if (!isset($reachable[$userId])) { | ||
| $stranded[] = $id; | ||
| } | ||
| } | ||
|
|
||
| return $stranded; | ||
| } |
Contributor
There was a problem hiding this comment.
All in all this reads really convoluted to me, I tried to understand what it does but I’m still a bit lost.
It gets statuses where is_backup is true, and in this case user_id also start with a an underscore, which we remove and then search for statuses matching those user ids and the id given as parameter?
It feels expensive to compute a list of all backed up users only to try a few ids.
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.
Agreed, changed to self join. Added some tests to make sure it also works on Oracle, let's see what CI says.
… backup revertUserStatus() bailed out as soon as no backup row was found, leaving the automated status on the live row. Nothing else ever removes it, so the user is stuck: setting themselves online manually is cleaned to offline 15 minutes later, and UserLiveStatusListener returns early for MESSAGE_CALENDAR_BUSY so no heartbeat can undo it. Delete the live row instead when its message id still matches the automation being reverted. A status the user has since changed themselves no longer matches and is left untouched. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch <anna@nextcloud.com>
A backup keeps the status_timestamp it had when the automation took over, so any automated status lasting longer than INVALIDATE_STATUS_THRESHOLD is restored already stale and rewritten to offline by the very next read. Stamp the restored status with the time of the revert, as the manual revert path already did. A user who really went away now stays online for up to INVALIDATE_STATUS_THRESHOLD instead, which is the better failure mode. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch <anna@nextcloud.com>
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>
The preceding fixes stop new damage, but nothing repairs what is already in the database: reverts for call, availability and out-of-office are driven by automations that never fire again for a user who is already stuck. Add a command that repairs the three shapes, with --dry-run to see the scope first: - statuses whose is_backup is NULL, which every query comparing the column against false skips - live rows on an automated status with no backup to revert into - backup rows that can no longer be matched Orphaned rows are deleted rather than rewritten, matching what revertUserStatus() now does, and the next heartbeat recreates a normal status. AI-Assisted-By: Claude Opus 5 Signed-off-by: Anna Larch <anna@nextcloud.com>
miaulalala
force-pushed
the
fix/noid/user-status-orphan-lifecycle
branch
from
August 11, 2026 15:03
5e610de to
8204069
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #63150
Summary
ff07d7717f48713f7ccfd2119d4a66c1cd53720afix(user_status): clear unreachable automated status when there is no backup-revertUserStatus()deletes the live row instead of bailing out, when its message id still matches the automation being reverted. A status the user has since changed themselves is left untouched.b6bd023365446ea2abd2088c55f93dd6a2e5fa4dfix(user_status): refresh the status timestamp when restoring a backup- covers item 4.34107e90ee5e868b7088974b02a951b754ccb247fix(user_status): delete stranded backup statuses in the cleanup job- state-based, not age-based (an out-of-office backup can legitimately be weeks old). Three simple statements rather than a self-referencing DELETE, for cross-database portability. A live row withis_backupNULL is treated as reachable, so unexpected data errs towards keeping the backup.5e610de57a9a29c12ea24b5875b34923763aa730feat(user_status): add occ user-status:repair for statuses left behind- repairs the three existing shapes (NULLis_backup, live automated row with no backup, unmatchable backup row). Supports--dry-run. Orphaned rows are deleted rather than rewritten, matching whatrevertUserStatus()now does; the next heartbeat recreates a normal status.Checklist
3. to review, feature component)stable32)AI (if applicable)