Mirrors/super-productivity
1
0
Fork 0
mirror of https://github.com/johannesjo/super-productivity.git synced 2026-07-17 16:37:43 +00:00
Code Issues Projects Releases Wiki Activity

fix(sync): serialize remote archive side effects behind the archive mutex

Browse source 
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.
This commit is contained in:
Johannes Millan 2026-07-11 20:21:00 +02:00
parent f572b6d077
commit 867d84a3d1
2 changed files with 61 additions and 58 deletions
Download patch file Download diff file Expand all files Collapse all files

53
src/app/features/archive/task-archive.service.ts
View file

@ -109,13 +109,17 @@ export class TaskArchiveService {
constructor() {}
private _runTaskArchiveMutation(
mutation: () => Promise<void>,
isIgnoreDBLock: boolean = false,
): Promise<void> {
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<void>): Promise<void> {
return this._lockService.request(LOCK_NAMES.TASK_ARCHIVE, mutation);
}
async loadYoung(): Promise<TaskArchive> {
@ -246,10 +250,7 @@ export class TaskArchiveService {
taskIdsToDelete: string[],
options?: { isIgnoreDBLock?: boolean },
): Promise<void> {
return this._runTaskArchiveMutation(
() => this._deleteTasks(taskIdsToDelete),
options?.isIgnoreDBLock,
);
return this._runTaskArchiveMutation(() => this._deleteTasks(taskIdsToDelete));
}
private async _deleteTasks(taskIdsToDelete: string[]): Promise<void> {
@ -296,9 +297,8 @@ export class TaskArchiveService {
changedFields: Partial<Task>,
options?: { isSkipDispatch?: boolean; isIgnoreDBLock?: boolean },
): Promise<void> {
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<Task>[],
options?: { isSkipDispatch?: boolean; isIgnoreDBLock?: boolean },
): Promise<void> {
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<void> {
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<void> {
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<void> {
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<void> {
return this._runTaskArchiveMutation(
() => this._unlinkIssueProviderFromArchiveTasks(issueProviderId),
options?.isIgnoreDBLock,
return this._runTaskArchiveMutation(() =>
this._unlinkIssueProviderFromArchiveTasks(issueProviderId),
);
}

66
src/app/op-log/apply/archive-operation-handler.service.ts
View file

@ -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<Persistent
// ═══════════════════════════════════════════════════════════════════════════
private _injector = inject(Injector);
private _archiveDbAdapter = inject(ArchiveDbAdapter);
private _lockService = inject(LockService);
private _getArchiveService = lazyInject(this._injector, ArchiveService);
private _getTaskArchiveService = lazyInject(this._injector, TaskArchiveService);
private _getTimeTrackingService = lazyInject(this._injector, TimeTrackingService);
@ -139,9 +142,10 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
/**
* Process an action and handle any archive-related side effects.
*
* This method handles both local and remote operations. For remote operations
* (action.meta.isRemote === true), it uses isIgnoreDBLock: true because sync
* processing has the database locked.
* This method handles both local and remote operations. Remote operations
* run while sync holds the OPERATION_LOG lock; archive mutations additionally
* serialize behind the separate TASK_ARCHIVE mutex (the legacy
* isIgnoreDBLock option no longer bypasses it).
*
* @param action The action that was dispatched
* @returns Promise that resolves when archive operations are complete
@ -217,7 +221,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
* Removes a restored task from archive storage.
*
* @localBehavior Executes normally (acquires DB lock)
* @remoteBehavior Executes with isIgnoreDBLock (sync has DB locked)
* @remoteBehavior Executes under the TASK_ARCHIVE mutex
*/
private async _handleRestoreTask(action: PersistentAction): Promise<void> {
const task = (action as ReturnType<typeof TaskSharedActions.restoreTask>).task;
@ -313,27 +317,33 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
const timestamp = (action as ReturnType<typeof flushYoungToOld>).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<Persistent
* Removes all archived tasks for a deleted project.
*
* @localBehavior Executes (cleans up archive for deleted project)
* @remoteBehavior Executes with isIgnoreDBLock (sync has DB locked)
* @remoteBehavior Executes under the TASK_ARCHIVE mutex
*/
private async _handleDeleteProject(action: PersistentAction): Promise<void> {
const projectId = (action as ReturnType<typeof TaskSharedActions.deleteProject>)
@ -380,7 +390,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
* Removes tag references from archived tasks and deletes orphaned tasks.
*
* @localBehavior Executes (cleans up archive for deleted tags)
* @remoteBehavior Executes with isIgnoreDBLock (sync has DB locked)
* @remoteBehavior Executes under the TASK_ARCHIVE mutex
*/
private async _handleDeleteTags(action: PersistentAction): Promise<void> {
const tagIdsToRemove =
@ -403,7 +413,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
* Removes repeatCfgId from archived tasks.
*
* @localBehavior Executes (cleans up archive for deleted repeat config)
* @remoteBehavior Executes with isIgnoreDBLock (sync has DB locked)
* @remoteBehavior Executes under the TASK_ARCHIVE mutex
*/
private async _handleDeleteTaskRepeatCfg(action: PersistentAction): Promise<void> {
const repeatCfgId = (
@ -420,7 +430,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
* Unlinks issue data from archived tasks for a deleted issue provider.
*
* @localBehavior Executes (cleans up archive for deleted provider)
* @remoteBehavior Executes with isIgnoreDBLock (sync has DB locked)
* @remoteBehavior Executes under the TASK_ARCHIVE mutex
*/
private async _handleDeleteIssueProvider(action: PersistentAction): Promise<void> {
const issueProviderId = (
@ -437,7 +447,7 @@ export class ArchiveOperationHandler implements ArchiveSideEffectPort<Persistent
* Unlinks issue data from archived tasks for multiple deleted issue providers.
*
* @localBehavior Executes (cleans up archive for deleted providers)
* @remoteBehavior Executes with isIgnoreDBLock (sync has DB locked)
* @remoteBehavior Executes under the TASK_ARCHIVE mutex
*/
private async _handleDeleteIssueProviders(action: PersistentAction): Promise<void> {
const ids = (action as ReturnType<typeof TaskSharedActions.deleteIssueProviders>).ids;