Simplify conflict scheduling and lifecycle subscriptions

package-lock.json

@@ -32,7 +32,7 @@
                 "markdown-it": "^14.2.0",
                 "minimatch": "^10.2.5",
                 "obsidian": "^1.13.1",
-                "octagonal-wheels": "^0.1.53",
+                "octagonal-wheels": "^0.1.54",
                 "qrcode-generator": "^1.4.4",
                 "xxhash-wasm-102": "npm:xxhash-wasm@^1.0.2"
             },
@@ -9682,9 +9682,9 @@
             "license": "MIT"
         },
         "node_modules/octagonal-wheels": {
-            "version": "0.1.53",
+            "version": "0.1.54",
-            "resolved": "https://registry.npmjs.org/octagonal-wheels/-/octagonal-wheels-0.1.53.tgz",
+            "resolved": "https://registry.npmjs.org/octagonal-wheels/-/octagonal-wheels-0.1.54.tgz",
-            "integrity": "sha512-4NJsb96Sk6rJXhrTyjAY5GRIoWMFJsFp56b5ba9fV8/87ys2HCjMo0hior4F8k4ma72pLTJT7i6DvuihHxSsMA==",
+            "integrity": "sha512-Je3ancYhjKX7UY2K19T/qTjG8C9nK8YVrACr5naIf78mN4bbjQkYyWmlj+ooifV/moWVsQrp4fEWz/7mv6It3A==",
             "license": "MIT",
             "dependencies": {
                 "idb": "^8.0.3"

package.json

@@ -186,7 +186,7 @@
         "markdown-it": "^14.2.0",
         "minimatch": "^10.2.5",
         "obsidian": "^1.13.1",
-        "octagonal-wheels": "^0.1.53",
+        "octagonal-wheels": "^0.1.54",
         "qrcode-generator": "^1.4.4",
         "xxhash-wasm-102": "npm:xxhash-wasm@^1.0.2"
     },

src/modules/features/InteractiveConflictResolving/ConflictResolveModal.ts

@@ -34,8 +34,7 @@ export class ConflictResolveModal extends Modal {
     readOnly: boolean = false;
     localName: string = "Base";
     remoteName: string = "Conflicted";
-    offConflictCancelled?: ReturnType<typeof eventHub.onEvent>;
+    private eventSubscriptions?: AbortController;
-    offPluginUnloaded?: ReturnType<typeof eventHub.onceEvent>;
     currentDiffIndex = -1;
     diffView!: HTMLDivElement;
     diffNavIndicator!: HTMLSpanElement;
@@ -112,23 +111,31 @@ export class ConflictResolveModal extends Modal {
 
     override onOpen() {
         const { contentEl } = this;
-        this.offConflictCancelled?.();
+        this.eventSubscriptions?.abort();
-        this.offConflictCancelled = undefined;
+        const eventSubscriptions = new AbortController();
-        this.offPluginUnloaded?.();
+        this.eventSubscriptions = eventSubscriptions;
-        this.offPluginUnloaded = eventHub.onceEvent(EVENT_PLUGIN_UNLOADED, () => {
+        eventHub.onceEvent(
-            this.sendResponse(CANCELLED);
+            EVENT_PLUGIN_UNLOADED,
-        });
+            () => {
+                this.sendResponse(CANCELLED);
+            },
+            { signal: eventSubscriptions.signal }
+        );
         if (!this.readOnly) {
             // Cancel an older dialogue for this path before subscribing this
             // instance. Emitting after subscription would close the replacement
             // itself; the instance-owned result promise then completes the older
             // caller even when it only begins waiting after this event.
             eventHub.emitEvent(EVENT_CONFLICT_CANCELLED, this.filename);
-            this.offConflictCancelled = eventHub.onEvent(EVENT_CONFLICT_CANCELLED, (path) => {
+            eventHub.onEvent(
-                if (path === this.filename) {
+                EVENT_CONFLICT_CANCELLED,
-                    this.sendResponse(CANCELLED);
+                (path) => {
-                }
+                    if (path === this.filename) {
-            });
+                        this.sendResponse(CANCELLED);
+                    }
+                },
+                { signal: eventSubscriptions.signal }
+            );
         }
         this.titleEl.setText(this.title);
         contentEl.empty();
@@ -219,10 +226,8 @@ export class ConflictResolveModal extends Modal {
     override onClose() {
         const { contentEl } = this;
         contentEl.empty();
-        this.offConflictCancelled?.();
+        this.eventSubscriptions?.abort();
-        this.offConflictCancelled = undefined;
+        this.eventSubscriptions = undefined;
-        this.offPluginUnloaded?.();
-        this.offPluginUnloaded = undefined;
         if (this.consumed) {
             return;
         }

src/serviceFeatures/conflictResolution/checker.ts

@@ -28,12 +28,9 @@ export interface ConflictCheckingHandlers {
     readonly ensureAllProcessed: () => Promise<boolean>;
 }
 
-/**
+/** Create conflict-checking handlers while retaining scheduling state privately. */
- * Create conflict-checking handlers and retain both queue processors privately.
- * The returned operations do not expose queue state to a host or consumer.
- */
 export function createConflictCheckingHandlers(dependencies: ConflictCheckingDependencies): ConflictCheckingHandlers {
-    const conflictResolveQueue = new QueueProcessor<FilePathWithPrefix, void>(
+    const conflictQueue = new QueueProcessor<FilePathWithPrefix, void>(
         async (filenames: FilePathWithPrefix[]) => {
             const filename = filenames[0];
             return await dependencies.conflict.resolve(filename);
@@ -47,28 +44,13 @@ export function createConflictCheckingHandlers(dependencies: ConflictCheckingDep
             concurrentLimit: 10,
             delay: 0,
             keepResultUntilDownstreamConnected: false,
+            totalRemainingReactiveSource: dependencies.conflictProcessQueueCount,
         }
     ).replaceEnqueueProcessor((queue, newEntity) => {
         const newQueue = [...queue].filter((entry) => entry != newEntity);
         return [...newQueue, newEntity];
     });
 
-    const conflictCheckQueue = new QueueProcessor<FilePathWithPrefix, FilePathWithPrefix>(
-        (files: FilePathWithPrefix[]) => {
-            const filename = files[0];
-            return Promise.resolve([filename]);
-        },
-        {
-            suspended: false,
-            batchSize: 1,
-            concurrentLimit: 10,
-            delay: 0,
-            keepResultUntilDownstreamConnected: true,
-            pipeTo: conflictResolveQueue,
-            totalRemainingReactiveSource: dependencies.conflictProcessQueueCount,
-        }
-    );
-
     const queueCheckForIfOpen = async (file: FilePathWithPrefix): Promise<void> => {
         const path = file;
         if (dependencies.currentSettings().checkConflictOnlyOnOpen) {
@@ -90,11 +72,11 @@ export function createConflictCheckingHandlers(dependencies: ConflictCheckingDep
             // The conflict should be resolved by the newer entry.
             await dependencies.conflict.resolveByNewest(file);
         } else {
-            conflictCheckQueue.enqueue(file);
+            conflictQueue.enqueue(file);
         }
     };
 
-    const ensureAllProcessed = (): Promise<boolean> => conflictResolveQueue.waitForAllProcessed();
+    const ensureAllProcessed = (): Promise<boolean> => conflictQueue.waitForAllProcessed();
 
     return {
         queueCheckForIfOpen,

src/serviceFeatures/conflictResolution/conflictResolution.unit.spec.ts

@@ -472,6 +472,126 @@ describe("conflict resolution serviceFeature", () => {
         );
     });
 
+    it("limits concurrent conflict checks and reports queued and active work", async () => {
+        const paths = Array.from({ length: 11 }, (_, index) => `concurrent-${index}.md` as FilePathWithPrefix);
+        const harness = createHarness();
+        const finishByPath = new Map<FilePathWithPrefix, () => void>();
+        harness.tryAutoMerge.mockImplementation(
+            async (path: FilePathWithPrefix) =>
+                await new Promise<{ ok: typeof NOT_CONFLICTED }>((resolve) => {
+                    finishByPath.set(path, () => resolve({ ok: NOT_CONFLICTED }));
+                })
+        );
+
+        await Promise.all(paths.map(async (path) => await harness.conflict.queueCheckFor(path)));
+        await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledTimes(10));
+
+        expect(harness.conflict.conflictProcessQueueCount.value).toBe(11);
+        let allProcessed = false;
+        const completion = harness.conflict.ensureAllProcessed().then((result) => {
+            allProcessed = true;
+            return result;
+        });
+        await Promise.resolve();
+        expect(allProcessed).toBe(false);
+
+        const finishFirst = finishByPath.get(paths[0]);
+        finishByPath.delete(paths[0]);
+        finishFirst?.();
+        await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledTimes(11));
+        for (const finish of finishByPath.values()) finish();
+
+        await expect(completion).resolves.toBe(true);
+        expect(harness.conflict.conflictProcessQueueCount.value).toBe(0);
+    });
+
+    it("replaces an older same-path check while every resolver slot is occupied", async () => {
+        const occupiedPaths = Array.from({ length: 10 }, (_, index) => `occupied-${index}.md` as FilePathWithPrefix);
+        const repeatedPath = "waiting-replacement.md" as FilePathWithPrefix;
+        const harness = createHarness();
+        const finishers: Array<{ path: FilePathWithPrefix; finish: () => void }> = [];
+        harness.tryAutoMerge.mockImplementation(
+            async (path: FilePathWithPrefix) =>
+                await new Promise<{ ok: typeof NOT_CONFLICTED }>((resolve) => {
+                    finishers.push({ path, finish: () => resolve({ ok: NOT_CONFLICTED }) });
+                })
+        );
+
+        await Promise.all(occupiedPaths.map(async (path) => await harness.conflict.queueCheckFor(path)));
+        await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledTimes(10));
+
+        await harness.conflict.queueCheckFor(repeatedPath);
+        await harness.conflict.queueCheckFor(repeatedPath);
+        for (const { finish } of finishers.filter(({ path }) => path !== repeatedPath)) finish();
+
+        await vi.waitFor(() => expect(harness.conflict.conflictProcessQueueCount.value).toBe(1));
+        expect(harness.tryAutoMerge.mock.calls.filter(([path]) => path === repeatedPath)).toHaveLength(1);
+
+        finishers.find(({ path }) => path === repeatedPath)?.finish();
+        await expect(harness.conflict.ensureAllProcessed()).resolves.toBe(true);
+    });
+
+    it("waits for a conflict check requeued by an automatic merge", async () => {
+        const path = "requeued-automatic-merge.md" as FilePathWithPrefix;
+        const harness = createHarness({ settings: { syncAfterMerge: false } });
+        let finishFirst!: (result: { ok: typeof AUTO_MERGED }) => void;
+        harness.tryAutoMerge
+            .mockImplementationOnce(
+                async () =>
+                    await new Promise<{ ok: typeof AUTO_MERGED }>((resolve) => {
+                        finishFirst = resolve;
+                    })
+            )
+            .mockResolvedValueOnce({ ok: NOT_CONFLICTED });
+
+        await harness.conflict.queueCheckFor(path);
+        await vi.waitFor(() => expect(harness.tryAutoMerge).toHaveBeenCalledOnce());
+        const completion = harness.conflict.ensureAllProcessed();
+
+        finishFirst({ ok: AUTO_MERGED });
+
+        await expect(completion).resolves.toBe(true);
+        expect(harness.tryAutoMerge).toHaveBeenCalledTimes(2);
+        expect(harness.conflict.conflictProcessQueueCount.value).toBe(0);
+    });
+
+    it("drains repeated manual resolutions for a file with more than two conflicting versions", async () => {
+        const path = "requeued-manual-merge.md" as FilePathWithPrefix;
+        const harness = createHarness({ settings: { syncAfterMerge: false } });
+        const firstPair = {
+            leftRev: "3-current",
+            rightRev: "2-second",
+            leftLeaf: leaf("3-current", "Current\n", 3),
+            rightLeaf: leaf("2-second", "Second\n", 2),
+        };
+        const remainingPair = {
+            leftRev: "4-merged",
+            rightRev: "2-third",
+            leftLeaf: leaf("4-merged", "Merged\n", 4),
+            rightLeaf: leaf("2-third", "Third\n", 2),
+        };
+        harness.tryAutoMerge
+            .mockResolvedValueOnce(firstPair)
+            .mockResolvedValueOnce(remainingPair)
+            .mockResolvedValueOnce({ ok: NOT_CONFLICTED });
+        const resolvePair = vi.fn(async (filename: FilePathWithPrefix) => {
+            await harness.conflict.queueCheckFor(filename);
+            return false;
+        });
+        const unregister = harness.conflict.resolveByUserInteraction.addHandler(resolvePair);
+
+        try {
+            await harness.conflict.queueCheckFor(path);
+
+            await expect(harness.conflict.ensureAllProcessed()).resolves.toBe(true);
+            expect(harness.tryAutoMerge).toHaveBeenCalledTimes(3);
+            expect(resolvePair).toHaveBeenCalledTimes(2);
+            expect(harness.conflict.conflictProcessQueueCount.value).toBe(0);
+        } finally {
+            unregister();
+        }
+    });
+
     it("honours optional conflict handlers before entering the check queue", async () => {
         const path = "optional.md" as FilePathWithPrefix;
         const harness = createHarness();

src/serviceFeatures/interactiveConflictResolution/index.ts

@@ -61,22 +61,23 @@ export function useInteractiveConflictResolutionFeature(
     });
     services.appLifecycle.getUnresolvedMessages.addHandler(operations.getActiveConflictMessages);
     services.conflict.resolveByUserInteraction.addHandler(operations.resolveByUserInteraction);
-    const offConflictCancelled = services.context.events.onEvent(EVENT_CONFLICT_CANCELLED, (filename) => {
+    const eventSubscriptions = new AbortController();
-        operations.invalidateWaitingResolution(filename);
-        fireAndForget(() => operations.refreshConflictState(filename));
-    });
-    let featureDisposed = false;
     const dispose = () => {
-        if (featureDisposed) return;
-        featureDisposed = true;
         // Stop the refresh listener before cancellation so that unloading does
         // not start a database read which can race with database disposal.
-        offConflictCancelled();
+        eventSubscriptions.abort();
         operations.dispose();
     };
-    const offPluginUnloaded = services.context.events.onceEvent(EVENT_PLUGIN_UNLOADED, dispose);
+    services.context.events.onEvent(
+        EVENT_CONFLICT_CANCELLED,
+        (filename) => {
+            operations.invalidateWaitingResolution(filename);
+            fireAndForget(() => operations.refreshConflictState(filename));
+        },
+        { signal: eventSubscriptions.signal }
+    );
+    services.context.events.onceEvent(EVENT_PLUGIN_UNLOADED, dispose, { signal: eventSubscriptions.signal });
     services.appLifecycle.onUnload.addHandler(() => {
-        offPluginUnloaded();
         dispose();
         return Promise.resolve(true);
     });

test/e2e-obsidian/runner/ui.ts

@@ -287,19 +287,28 @@ export async function captureObsidianElement(
     await mkdir(dirname(screenshotPath), { recursive: true });
 
     await withObsidianPage(port, async (page) => {
-        try {
+        for (let attempt = 0; attempt < 3; attempt++) {
-            const element = await resolveElement(page);
+            try {
-            await element.waitFor({ state: "visible", timeout: timeoutMs });
+                const element = await resolveElement(page);
-            await element.screenshot({
+                await element.waitFor({ state: "visible", timeout: timeoutMs });
-                path: screenshotPath,
+                await element.screenshot({
-                animations: "disabled",
+                    path: screenshotPath,
-                style: ".notice-container { visibility: hidden !important; }",
+                    animations: "disabled",
-            });
+                    style: ".notice-container { visibility: hidden !important; }",
-        } catch (error) {
+                });
-            const failurePath = screenshotPath.replace(/\.png$/u, ".failure.png");
+                return;
-            await page.screenshot({ path: failurePath, fullPage: true });
+            } catch (error) {
-            console.error(`UI element failure screenshot: ${failurePath}`);
+                const detachedDuringCapture =
-            throw error;
+                    error instanceof Error && error.message.includes("not attached to the DOM");
+                if (detachedDuringCapture && attempt < 2) {
+                    await page.waitForTimeout(50);
+                    continue;
+                }
+                const failurePath = screenshotPath.replace(/\.png$/u, ".failure.png");
+                await page.screenshot({ path: failurePath, fullPage: true });
+                console.error(`UI element failure screenshot: ${failurePath}`);
+                throw error;
+            }
         }
     });
 

test/e2e-obsidian/scripts/conflict-dialog-policy.ts

@@ -423,7 +423,17 @@ async function main(): Promise<void> {
                 state: "visible",
                 timeout: uiTimeoutMs,
             });
-            const actionButtonBounds = await modal.locator(".conflict-action-button").evaluateAll((buttons) =>
+        });
+        const firstDialogueScreenshot = await captureObsidianElement(
+            session.remoteDebuggingPort,
+            "conflict-dialog-three-versions.png",
+            (page) => conflictDialogue(page).locator(".modal").first()
+        );
+        await withObsidianPage(session.remoteDebuggingPort, async (page) => {
+            const modal = conflictDialogue(page);
+            const actionButtons = modal.locator(".conflict-action-button");
+            await actionButtons.nth(3).waitFor({ state: "visible", timeout: uiTimeoutMs });
+            const actionButtonBounds = await actionButtons.evaluateAll((buttons) =>
                 buttons.map((button) => {
                     const bounds = button.getBoundingClientRect();
                     return { top: bounds.top, bottom: bounds.bottom };
@@ -435,16 +445,20 @@ async function main(): Promise<void> {
                     (bounds, index) => index > 0 && bounds.top < actionButtonBounds[index - 1].bottom
                 )
             ) {
+                const buttonDetails = await modal.locator("button").evaluateAll((buttons) =>
+                    buttons.map((button) => ({
+                        text: button.textContent,
+                        className: button.className,
+                    }))
+                );
                 throw new Error(
-                    `Conflict action buttons are not stacked vertically: ${JSON.stringify(actionButtonBounds)}`
+                    `Conflict action buttons are not stacked vertically: ${JSON.stringify({
+                        actionButtonBounds,
+                        buttonDetails,
+                    })}`
                 );
             }
         });
-        const firstDialogueScreenshot = await captureObsidianElement(
-            session.remoteDebuggingPort,
-            "conflict-dialog-three-versions.png",
-            (page) => conflictDialogue(page).locator(".modal").first()
-        );
         await withObsidianPage(session.remoteDebuggingPort, async (page) => {
             const modal = conflictDialogue(page);
             await modal.getByRole("button", { name: "Concat both", exact: true }).click({ timeout: uiTimeoutMs });