From 867d84a3d1c931fb9df0b6fe0fb2a16499a8399a Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Sat, 11 Jul 2026 20:21:00 +0200 Subject: [PATCH] fix(sync): serialize remote archive side effects behind the archive mutex MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit isIgnoreDBLock historically meant "sync already holds the op-log DB lock", but _runTaskArchiveMutation also treated it as permission to skip the new TASK_ARCHIVE mutex — so every remote archive side effect except moveToArchive ran unserialized against locked local mutations, and a concurrent read-modify-write could silently drop one side's archive write (the exact race the mutex was added to close). TASK_ARCHIVE is deliberately separate from OPERATION_LOG, so acquiring it while sync holds the op-log lock is safe — the remote moveToArchive path has always done exactly that. The mutex is now unconditional, and the remote flushYoungToOld handler wraps its two-archive read-modify- write in the same lock instead of writing through the adapter bare. The isIgnoreDBLock option is retained as inert API surface; removing the threading is tracked as a follow-up. --- .../features/archive/task-archive.service.ts | 53 +++++++-------- .../archive-operation-handler.service.ts | 66 +++++++++++-------- 2 files changed, 61 insertions(+), 58 deletions(-) diff --git a/src/app/features/archive/task-archive.service.ts b/src/app/features/archive/task-archive.service.ts index 1ae352eca3..6d2cad4dbc 100644 --- a/src/app/features/archive/task-archive.service.ts +++ b/src/app/features/archive/task-archive.service.ts @@ -109,13 +109,17 @@ export class TaskArchiveService { constructor() {} - private _runTaskArchiveMutation( - mutation: () => Promise, - isIgnoreDBLock: boolean = false, - ): Promise { - return isIgnoreDBLock - ? mutation() - : this._lockService.request(LOCK_NAMES.TASK_ARCHIVE, mutation); + /** + * Every archive mutation is serialized behind the cross-tab TASK_ARCHIVE + * lock — including remote sync side effects: TASK_ARCHIVE is deliberately a + * separate lock from OPERATION_LOG, so acquiring it while sync holds the + * op-log lock is safe (and the remote moveToArchive path already does). + * The legacy `isIgnoreDBLock` option no longer bypasses this mutex; a + * bypassed remote mutation could interleave with a locked local one and + * silently drop one side's archive write. + */ + private _runTaskArchiveMutation(mutation: () => Promise): Promise { + return this._lockService.request(LOCK_NAMES.TASK_ARCHIVE, mutation); } async loadYoung(): Promise { @@ -246,10 +250,7 @@ export class TaskArchiveService { taskIdsToDelete: string[], options?: { isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._deleteTasks(taskIdsToDelete), - options?.isIgnoreDBLock, - ); + return this._runTaskArchiveMutation(() => this._deleteTasks(taskIdsToDelete)); } private async _deleteTasks(taskIdsToDelete: string[]): Promise { @@ -296,9 +297,8 @@ export class TaskArchiveService { changedFields: Partial, options?: { isSkipDispatch?: boolean; isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._updateTask(id, changedFields, options), - options?.isIgnoreDBLock, + return this._runTaskArchiveMutation(() => + this._updateTask(id, changedFields, options), ); } @@ -345,10 +345,7 @@ export class TaskArchiveService { updates: Update[], options?: { isSkipDispatch?: boolean; isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._updateTasks(updates, options), - options?.isIgnoreDBLock, - ); + return this._runTaskArchiveMutation(() => this._updateTasks(updates, options)); } private async _updateTasks( @@ -408,9 +405,8 @@ export class TaskArchiveService { projectIdToDelete: string, options?: { isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._removeAllArchiveTasksForProject(projectIdToDelete), - options?.isIgnoreDBLock, + return this._runTaskArchiveMutation(() => + this._removeAllArchiveTasksForProject(projectIdToDelete), ); } @@ -434,9 +430,8 @@ export class TaskArchiveService { tagIdsToRemove: string[], options?: { isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._removeTagsFromAllTasks(tagIdsToRemove), - options?.isIgnoreDBLock, + return this._runTaskArchiveMutation(() => + this._removeTagsFromAllTasks(tagIdsToRemove), ); } @@ -472,9 +467,8 @@ export class TaskArchiveService { repeatConfigId: string, options?: { isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._removeRepeatCfgFromArchiveTasks(repeatConfigId), - options?.isIgnoreDBLock, + return this._runTaskArchiveMutation(() => + this._removeRepeatCfgFromArchiveTasks(repeatConfigId), ); } @@ -508,9 +502,8 @@ export class TaskArchiveService { issueProviderId: string, options?: { isIgnoreDBLock?: boolean }, ): Promise { - return this._runTaskArchiveMutation( - () => this._unlinkIssueProviderFromArchiveTasks(issueProviderId), - options?.isIgnoreDBLock, + return this._runTaskArchiveMutation(() => + this._unlinkIssueProviderFromArchiveTasks(issueProviderId), ); } diff --git a/src/app/op-log/apply/archive-operation-handler.service.ts b/src/app/op-log/apply/archive-operation-handler.service.ts index b7c98ac393..2ff5d14421 100644 --- a/src/app/op-log/apply/archive-operation-handler.service.ts +++ b/src/app/op-log/apply/archive-operation-handler.service.ts @@ -22,6 +22,8 @@ import { loadAllData } from '../../root-store/meta/load-all-data.action'; import { ArchiveModel } from '../../features/archive/archive.model'; import { ArchiveDbAdapter } from '../../core/persistence/archive-db-adapter.service'; import { OpType } from '../core/operation.types'; +import { LockService } from '../sync/lock.service'; +import { LOCK_NAMES } from '../core/operation-log.const'; import { confirmDialog } from '../../util/native-dialogs'; /** @@ -104,7 +106,7 @@ export const isArchiveAffectingAction = (action: Action): action is PersistentAc * * ## Important Notes * - * - For remote operations, uses `isIgnoreDBLock: true` because sync processing has the DB locked + * - Remote archive mutations serialize behind the TASK_ARCHIVE mutex (separate from the OPERATION_LOG lock sync holds) * - All operations are idempotent - safe to run multiple times * - Use `isArchiveAffectingAction()` helper to check if an action needs archive handling */ @@ -128,6 +130,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const task = (action as ReturnType).task; @@ -313,27 +317,33 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort).timestamp; - // Load current state using ArchiveDbAdapter - // Default to empty archives if they don't exist (first-time usage) - const currentArchiveYoung = - (await this._archiveDbAdapter.loadArchiveYoung()) ?? createEmptyArchiveModel(); - const currentArchiveOld = - (await this._archiveDbAdapter.loadArchiveOld()) ?? createEmptyArchiveModel(); + // Serialize against local archive mutations: this is a read-modify-write + // over BOTH archives, so an unlocked run could silently drop a concurrent + // locked local write. TASK_ARCHIVE is separate from OPERATION_LOG, so + // acquiring it while sync holds the op-log lock is safe. + await this._lockService.request(LOCK_NAMES.TASK_ARCHIVE, async () => { + // Load current state using ArchiveDbAdapter + // Default to empty archives if they don't exist (first-time usage) + const currentArchiveYoung = + (await this._archiveDbAdapter.loadArchiveYoung()) ?? createEmptyArchiveModel(); + const currentArchiveOld = + (await this._archiveDbAdapter.loadArchiveOld()) ?? createEmptyArchiveModel(); - const newSorted = sortTimeTrackingAndTasksFromArchiveYoungToOld({ - archiveYoung: currentArchiveYoung, - archiveOld: currentArchiveOld, - threshold: ARCHIVE_TASK_YOUNG_TO_OLD_THRESHOLD, - now: timestamp, + const newSorted = sortTimeTrackingAndTasksFromArchiveYoungToOld({ + archiveYoung: currentArchiveYoung, + archiveOld: currentArchiveOld, + threshold: ARCHIVE_TASK_YOUNG_TO_OLD_THRESHOLD, + now: timestamp, + }); + + // Atomic write: both archives written in a single IndexedDB transaction. + // If either write fails, the entire transaction is rolled back automatically. + await this._archiveDbAdapter.saveArchivesAtomic( + { ...newSorted.archiveYoung, lastTimeTrackingFlush: timestamp }, + { ...newSorted.archiveOld, lastTimeTrackingFlush: timestamp }, + ); }); - // Atomic write: both archives written in a single IndexedDB transaction. - // If either write fails, the entire transaction is rolled back automatically. - await this._archiveDbAdapter.saveArchivesAtomic( - { ...newSorted.archiveYoung, lastTimeTrackingFlush: timestamp }, - { ...newSorted.archiveOld, lastTimeTrackingFlush: timestamp }, - ); - OpLog.log( '______________________\nFLUSHED ALL FROM ARCHIVE YOUNG TO OLD (via remote op handler)\n_______________________', ); @@ -363,7 +373,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const projectId = (action as ReturnType) @@ -380,7 +390,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const tagIdsToRemove = @@ -403,7 +413,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const repeatCfgId = ( @@ -420,7 +430,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const issueProviderId = ( @@ -437,7 +447,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort { const ids = (action as ReturnType).ids;