Make remote and log resource ownership explicit

src/modules/features/ModuleLog.ts

@@ -1,4 +1,10 @@
-import { computed, reactive, reactiveSource, type ReactiveValue } from "octagonal-wheels/dataobject/reactive";
+import {
+    computed,
+    reactive,
+    reactiveSource,
+    type ReactiveSource,
+    type ReactiveValue,
+} from "octagonal-wheels/dataobject/reactive";
 import {
     LOG_LEVEL_DEBUG,
     LOG_LEVEL_INFO,
@@ -7,6 +13,7 @@ import {
     type LOG_LEVEL,
 } from "@vrtmrz/livesync-commonlib/compat/common/types";
 import { scheduleTask } from "octagonal-wheels/concurrency/task";
+import { createOwnedResource, type OwnedResource } from "octagonal-wheels/resource";
 import { fireAndForget, isDirty, throttle } from "@vrtmrz/livesync-commonlib/compat/common/utils";
 import {
     chunkFetchCounts,
@@ -46,7 +53,11 @@ import {
     isValidFilenameInDarwin,
     isValidFilenameInWidows,
 } from "@vrtmrz/livesync-commonlib/compat/string_and_binary/path";
-import { MARK_LOG_NETWORK_ERROR, MARK_LOG_SEPARATOR } from "@vrtmrz/livesync-commonlib/compat/services/lib/logUtils";
+import {
+    MARK_LOG_NETWORK_ERROR,
+    MARK_LOG_SEPARATOR,
+    type LogFunction,
+} from "@vrtmrz/livesync-commonlib/compat/services/lib/logUtils";
 import { NetworkWarningStyles } from "@vrtmrz/livesync-commonlib/compat/common/models/setting.const";
 import { compatGlobal } from "@vrtmrz/livesync-commonlib/compat/common/coreEnvFunctions";
 import { generateReport } from "@/common/reportTool.ts";
@@ -57,20 +68,22 @@ import {
 
 // This module cannot be a core module because it depends on the Obsidian UI.
 
-// DI the log again.
-const recentLogEntries = reactiveSource<LogEntry[]>([]);
-const globalLogFunction = (message: unknown, level?: number, key?: string) => {
-    const messageX =
-        message instanceof Error
-            ? new LiveSyncError("[Error Logged]: " + message.message, { cause: message })
-            : typeof message === "string"
-              ? message
-              : JSON.stringify(message);
-    const entry = { message: messageX, level, key } as LogEntry;
-    recentLogEntries.value = [...recentLogEntries.value, entry];
-};
+function createBufferedLog(entries: ReactiveSource<LogEntry[]>) {
+    return (message: unknown, level?: number, key?: string) => {
+        const messageX =
+            message instanceof Error
+                ? new LiveSyncError("[Error Logged]: " + message.message, { cause: message })
+                : typeof message === "string"
+                  ? message
+                  : JSON.stringify(message);
+        const entry = { message: messageX, level, key } as LogEntry;
+        entries.value = [...entries.value, entry];
+    };
+}
 
-setGlobalLogFunction(globalLogFunction);
+// Legacy Logger calls use this shared buffer; service logs use their owner's buffer.
+const recentGlobalLogEntries = reactiveSource<LogEntry[]>([]);
+setGlobalLogFunction(createBufferedLog(recentGlobalLogEntries));
 // Keep the recent logs in memory for display, but also keep a longer history in logForDump for when the user wants to see more logs.
 // logForDump is not reactive and is only used for dumping logs when requested, while recentLogs is reactive and is used for displaying logs in the UI.
 const logForDump = [] as string[];
@@ -113,6 +126,16 @@ function redactLog(log: string) {
 const showDebugLog = false;
 export const MARK_DONE = "\u{2009}\u{2009}";
 export class ModuleLog extends AbstractObsidianModule {
+    /** The host releases this connection after the complete asynchronous unload. */
+    logResource!: OwnedResource<LogFunction>;
+    private readonly recentLogEntries = reactiveSource<LogEntry[]>([]);
+    private readonly displayLogEntries = () => {
+        if (this.recentLogEntries.value.length === 0) return;
+        const entries = [...this.recentLogEntries.value];
+        this.recentLogEntries.value = [];
+        entries.forEach((entry) => this.__addLog(entry.message, entry.level, entry.key));
+    };
+
     statusBar?: HTMLElement;
 
     statusDiv?: HTMLElement;
@@ -460,12 +483,7 @@ ${stringifyYaml(info)}
         return Promise.resolve(true);
     }
     private _everyOnloadAfterLoadSettings(): Promise<boolean> {
-        recentLogEntries.onChanged((entries) => {
-            if (entries.value.length === 0) return;
-            const newEntries = [...entries.value];
-            recentLogEntries.value = [];
-            newEntries.forEach((e) => this.__addLog(e.message, e.level, e.key));
-        });
+        this.recentLogEntries.onChanged(this.displayLogEntries);
         eventHub.onEvent(EVENT_FILE_RENAMED, (data) => {
             void this.setFileStatus();
         });
@@ -597,7 +615,22 @@ ${stringifyYaml(info)}
         }
     }
     override onBindFunction(core: LiveSyncCore, services: typeof core.services): void {
-        services.API.addLog.setHandler(globalLogFunction);
+        const log = createBufferedLog(this.recentLogEntries);
+        const unregisterLog = services.API.addLog.setHandler(log);
+        const collectGlobalLogs = () => {
+            const entries = recentGlobalLogEntries.value;
+            if (entries.length === 0) return;
+            recentGlobalLogEntries.value = [];
+            this.recentLogEntries.value = [...this.recentLogEntries.value, ...entries];
+        };
+        this.logResource = createOwnedResource(log, () => {
+            unregisterLog();
+            recentGlobalLogEntries.offChanged(collectGlobalLogs);
+            this.recentLogEntries.offChanged(this.displayLogEntries);
+            this.recentLogEntries.value = [];
+        });
+        recentGlobalLogEntries.onChanged(collectGlobalLogs);
+        collectGlobalLogs();
         services.appLifecycle.onInitialise.addHandler(this._everyOnloadStart.bind(this));
         services.appLifecycle.onSettingLoaded.addHandler(this._everyOnloadAfterLoadSettings.bind(this));
         services.appLifecycle.onLoaded.addHandler(this._everyOnload.bind(this));

src/modules/features/ModuleLog.unit.spec.ts

@@ -0,0 +1,228 @@
+import { afterEach, beforeEach, describe, expect, it, vi } from "vitest";
+
+vi.mock("@/deps.ts", () => ({
+    addIcon: vi.fn(),
+    debounce: (callback: () => void) => callback,
+    normalizePath: (path: string) => path,
+    Notice: class {},
+    stringifyYaml: vi.fn(),
+}));
+vi.mock("./Log/LogPaneView.ts", () => ({ LogPaneView: class {}, VIEW_TYPE_LOG: "log" }));
+vi.mock("@/common/reportTool.ts", () => ({ generateReport: vi.fn() }));
+vi.mock("@/common/utils.ts", () => ({ isValidPath: () => true }));
+vi.mock("@/common/translation", () => ({ $msg: (key: string) => key }));
+vi.mock("@/common/events.ts", () => ({
+    EVENT_FILE_RENAMED: "renamed",
+    EVENT_LAYOUT_READY: "ready",
+    EVENT_LEAF_ACTIVE_CHANGED: "active",
+    EVENT_ON_UNRESOLVED_ERROR: "error",
+    eventHub: { onEvent: vi.fn() },
+}));
+vi.mock("@vrtmrz/livesync-commonlib/compat/replication/trystero/P2PLogCollector", () => ({
+    P2PLogCollector: class {},
+}));
+
+async function createHarness() {
+    const { ModuleLog } = await import("./ModuleLog.ts");
+    const { bindableFunction, allFunction } =
+        await import("@vrtmrz/livesync-commonlib/compat/services/lib/HandlerUtils");
+    const services = {
+        API: {
+            addLog: bindableFunction<(message: unknown, level?: number, key?: string) => void>("log"),
+            addCommand: vi.fn(),
+            registerWindow: vi.fn(),
+            addRibbonIcon: vi.fn(),
+            registerProtocolHandler: vi.fn(),
+        },
+        context: { events: {}, notices: { hide: vi.fn() } },
+        appLifecycle: {
+            onInitialise: allFunction<() => Promise<boolean>>(),
+            onSettingLoaded: allFunction<() => Promise<boolean>>(),
+            onLoaded: allFunction<() => Promise<boolean>>(),
+            onBeforeUnload: allFunction<() => Promise<boolean>>(),
+        },
+    };
+    const core = { services, settings: {} };
+    const module = new ModuleLog({ app: {} } as never, core as never);
+    const sink = vi.spyOn(module, "__addLog").mockImplementation(() => undefined);
+    vi.spyOn(module, "observeForLogs").mockImplementation(() => undefined);
+    vi.spyOn(module, "adjustStatusDivPosition").mockImplementation(() => undefined);
+    module.onBindFunction(core as never, services as never);
+    const { Logger } = await import("octagonal-wheels/common/logger");
+    return { module, services, sink, Logger };
+}
+
+describe("ModuleLog service connection", () => {
+    beforeEach(() => {
+        vi.resetModules();
+        vi.stubGlobal("document", { querySelectorAll: () => [] });
+    });
+    afterEach(() => {
+        vi.unstubAllGlobals();
+    });
+
+    it("buffers startup logs and preserves global and service call order", async () => {
+        const { services, sink, Logger } = await createHarness();
+        Logger("before-global");
+        services.API.addLog("before-service", 32, "startup");
+        Logger("between-global");
+        expect(sink).not.toHaveBeenCalled();
+        await services.appLifecycle.onSettingLoaded();
+        const messages = sink.mock.calls.map(([message]) => message);
+        expect(messages.filter((message) => typeof message === "string" && /^(before|between)-/.test(message))).toEqual(
+            ["before-global", "before-service", "between-global"]
+        );
+        expect(sink).toHaveBeenCalledWith("before-service", 32, "startup");
+    });
+
+    it("delivers service logs only to their own module", async () => {
+        const first = await createHarness();
+        const second = await createHarness();
+        await first.services.appLifecycle.onSettingLoaded();
+        await second.services.appLifecycle.onSettingLoaded();
+        first.sink.mockClear();
+        second.sink.mockClear();
+
+        first.services.API.addLog("first", 32, "first-key");
+        second.services.API.addLog("second", 64, "second-key");
+        expect(first.sink.mock.calls).toEqual([["first", 32, "first-key"]]);
+        expect(second.sink.mock.calls).toEqual([["second", 64, "second-key"]]);
+    });
+
+    it("preserves message conversion, levels, keys, and error causes", async () => {
+        const { services, sink } = await createHarness();
+        await services.appLifecycle.onSettingLoaded();
+        sink.mockClear();
+        const error = new Error("failed");
+        services.API.addLog(error, 128, "failure");
+        services.API.addLog({ result: "value" }, 32, "object");
+        services.API.addLog("plain");
+        const converted = sink.mock.calls[0][0] as Error & { cause: unknown };
+        expect(converted.message).toBe("[Error Logged]: failed");
+        expect(converted.cause).toBe(error);
+        expect(sink.mock.calls[0].slice(1)).toEqual([128, "failure"]);
+        expect(sink.mock.calls.slice(1)).toEqual([
+            ['{"result":"value"}', 32, "object"],
+            ["plain", undefined, undefined],
+        ]);
+    });
+
+    it("detaches both service registration and legacy subscription on disposal", async () => {
+        const first = await createHarness();
+        const second = await createHarness();
+        await first.services.appLifecycle.onSettingLoaded();
+        await second.services.appLifecycle.onSettingLoaded();
+        first.sink.mockClear();
+        second.sink.mockClear();
+
+        await first.module.logResource.dispose();
+        await first.module.logResource.dispose();
+        expect(() => first.services.API.addLog("retired")).toThrow("not assigned");
+        second.services.API.addLog("active", 32, "service");
+        first.Logger("legacy-after-retirement", 32, "legacy");
+        expect(first.sink).not.toHaveBeenCalled();
+        expect(second.sink.mock.calls).toEqual([
+            ["active", 32, "service"],
+            ["legacy-after-retirement", 32, "legacy"],
+        ]);
+        await second.module.logResource.dispose();
+    });
+
+    it("leaves a replacement service handler registered when the old owner is disposed", async () => {
+        const { services, module, sink, Logger } = await createHarness();
+        await services.appLifecycle.onSettingLoaded();
+        const replacement = vi.fn();
+        services.API.addLog.setHandler(replacement, true);
+        sink.mockClear();
+
+        await module.logResource.dispose();
+        await module.logResource.dispose();
+        services.API.addLog("replacement", 32, "key");
+        Logger("detached-legacy");
+        expect(replacement).toHaveBeenCalledExactlyOnceWith("replacement", 32, "key");
+        expect(sink).not.toHaveBeenCalled();
+    });
+
+    it("releases a connection which never reached the settings-loaded phase", async () => {
+        const { services, module, sink, Logger } = await createHarness();
+        services.API.addLog("pending");
+        await module.logResource.dispose();
+        Logger("after-disposal");
+        expect(sink).not.toHaveBeenCalled();
+        expect(() => services.API.addLog("retired")).toThrow("not assigned");
+
+        const replacement = await createHarness();
+        await replacement.services.appLifecycle.onSettingLoaded();
+        const messages = replacement.sink.mock.calls.map(([message]) => message);
+        expect(messages).toContain("after-disposal");
+        expect(messages).not.toContain("pending");
+        await replacement.module.logResource.dispose();
+    });
+
+    it.each(["complete", "fail"] as const)(
+        "keeps logging through asynchronous database shutdown and releases after it: %s",
+        async (outcome) => {
+            const { services, module, sink, Logger } = await createHarness();
+            await services.appLifecycle.onSettingLoaded();
+            sink.mockClear();
+            const { ControlService } = await import("@vrtmrz/livesync-commonlib/compat/services/base/ControlService");
+            const { ServiceContext } = await import("@vrtmrz/livesync-commonlib/context");
+            const { withOwnedResource } = await import("octagonal-wheels/resource");
+            let finish!: () => void;
+            let fail!: (reason: Error) => void;
+            let started!: () => void;
+            const closing = new Promise<void>((resolve) => {
+                started = resolve;
+            });
+            const closed = new Promise<void>((resolve, reject) => {
+                finish = resolve;
+                fail = reject;
+            });
+            const control = new ControlService(new ServiceContext({ translate: (key) => key }), {
+                appLifecycleService: {
+                    ...services.appLifecycle,
+                    onAppUnload: vi.fn(async () => []),
+                    onUnload: vi.fn(async () => {
+                        services.API.addLog("lifecycle-unload");
+                        return true;
+                    }),
+                },
+                APIService: services.API,
+                databaseService: {
+                    localDatabaseDirect: {
+                        onunload: vi.fn(() => services.API.addLog("database-unload")),
+                        close: vi.fn(() => {
+                            started();
+                            return closed;
+                        }),
+                    },
+                },
+            } as never);
+
+            const unloading = withOwnedResource(module.logResource, () => control.onUnload());
+            await closing;
+            services.API.addLog("during-close");
+            expect(sink.mock.calls.map(([message]) => message)).toEqual([
+                "lifecycle-unload",
+                "database-unload",
+                "during-close",
+            ]);
+            if (outcome === "fail") {
+                const failure = new Error("database close failed");
+                const rejected = expect(unloading).rejects.toBe(failure);
+                fail(failure);
+                await rejected;
+            } else {
+                finish();
+                await unloading;
+                expect(sink.mock.calls[sink.mock.calls.length - 1][0]).toContain(
+                    "moduleLiveSyncMain.logUnloadingPlugin"
+                );
+            }
+            expect(() => services.API.addLog("retired")).toThrow("not assigned");
+            sink.mockClear();
+            Logger("after-unload");
+            expect(sink).not.toHaveBeenCalled();
+        }
+    );
+});