From a0e07cf89d9dbcc268032287b51f3d939ff8895b Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 9 Oct 2026 15:04:54 +0200 Subject: [PATCH 1/3] test: reproduce lost-reply plugin cascade and orphaned CLI clients Regression tests that fail on main: - plugin.reload() accepts Obsidian's exit-0 'not enabled' reply and never re-enables a plugin an earlier failure left disabled. - plugin.disable()/enable() fail on a lost reply even when Obsidian applied the toggle, and never resend a toggle whose request was lost. - createPluginHarness re-disables a plugin it found disabled at teardown, and a failed data restore skips the re-enable. - A CLI client in flight when Vitest stops the worker outlives it. --- tests/core/plugin.test.ts | 53 +++++++++++++ tests/core/transport.test.ts | 61 +++++++++++++- tests/fixtures/plugin-harness.test.ts | 69 +++++++++++++++- tests/helpers/fake-plugin-app.ts | 96 +++++++++++++++++++++++ tests/helpers/orphan-client-child.test.ts | 30 +++++++ 5 files changed, 306 insertions(+), 3 deletions(-) create mode 100644 tests/helpers/fake-plugin-app.ts create mode 100644 tests/helpers/orphan-client-child.test.ts diff --git a/tests/core/plugin.test.ts b/tests/core/plugin.test.ts index a1c480c..a6ef1b2 100644 --- a/tests/core/plugin.test.ts +++ b/tests/core/plugin.test.ts @@ -9,6 +9,7 @@ import { } from "../helpers/create-temp-dir"; import { createStubObsidianClient } from "../helpers/stub-obsidian-client"; import { createExecResult, frameEvalPayload } from "../helpers/create-exec-result"; +import { createFakePluginApp } from "../helpers/fake-plugin-app"; import type { CommandTransport } from "../../src/core/types"; const tempDirectories: string[] = []; @@ -395,6 +396,58 @@ describe("plugin data ergonomics", () => { }); }); +describe("plugin lifecycle under lost CLI replies", () => { + function createLifecycleFixture(enabled: boolean) { + const app = createFakePluginApp({ enabled, pluginId: "quickadd", vaultRoot: "/tmp/vault" }); + const client = createObsidianClient({ + intervalMs: 5, + timeoutMs: 500, + transport: app.transport, + vault: "dev", + }); + + return { app, plugin: client.plugin("quickadd") }; + } + + test("reload enables a plugin that an earlier failure left disabled", async () => { + const { app, plugin } = createLifecycleFixture(false); + + await plugin.reload({ waitUntilReady: true }); + + expect(app.isEnabled()).toBe(true); + }); + + test("disable succeeds when Obsidian disabled the plugin but the reply was lost", async () => { + const { app, plugin } = createLifecycleFixture(true); + app.failNext("plugin:disable", "lose-reply"); + + await plugin.disable(); + + expect(app.isEnabled()).toBe(false); + }); + + test("enable resends when the first request never reached Obsidian", async () => { + const { app, plugin } = createLifecycleFixture(false); + app.failNext("plugin:enable", "lose-request"); + + await plugin.enable(); + + expect(app.isEnabled()).toBe(true); + }); + + test("enable succeeds when Obsidian enabled the plugin but the reply was lost", async () => { + const { app, plugin } = createLifecycleFixture(false); + app.failNext("plugin:enable", "lose-reply"); + + await plugin.enable(); + + expect(app.isEnabled()).toBe(true); + expect(app.calls.filter((call) => call.startsWith("plugin:enable"))).toEqual([ + "plugin:enable -> Enabled: quickadd", + ]); + }); +}); + async function createVaultRoot(): Promise { return createTrackedTempDir(tempDirectories, "obsidian-e2e-plugin-"); } diff --git a/tests/core/transport.test.ts b/tests/core/transport.test.ts index e267a41..18dab70 100644 --- a/tests/core/transport.test.ts +++ b/tests/core/transport.test.ts @@ -1,7 +1,66 @@ -import { describe, expect, it } from "vite-plus/test"; +import { spawnSync } from "node:child_process"; +import { readFileSync } from "node:fs"; +import path from "node:path"; +import process from "node:process"; + +import { afterEach, describe, expect, it } from "vite-plus/test"; import { ObsidianCommandError, ObsidianCommandTimeoutError } from "../../src/core/errors"; import { executeCommand } from "../../src/core/transport"; +import { waitForValue } from "../../src/core/wait"; +import { cleanupTempDirectories, createTempDir } from "../helpers/create-temp-dir"; + +const tempDirectories: string[] = []; + +afterEach(async () => { + await cleanupTempDirectories(tempDirectories); +}); + +function isAlive(pid: number): boolean { + try { + process.kill(pid, 0); + return true; + } catch { + return false; + } +} + +describe("executeCommand client ownership", () => { + it("kills an in-flight client when Vitest stops the worker", { timeout: 60_000 }, async () => { + const root = await createTempDir(tempDirectories, "obsidian-e2e-orphan-"); + const pidPath = path.join(root, "client.pid"); + + const child = spawnSync( + process.execPath, + [ + path.resolve("node_modules/vite-plus/bin/vp"), + "test", + path.resolve("tests/helpers/orphan-client-child.test.ts"), + ], + { + encoding: "utf8", + env: { + ...process.env, + OBSIDIAN_E2E_ORPHAN_CHILD: "1", + OBSIDIAN_E2E_ORPHAN_PID_PATH: pidPath, + }, + }, + ); + expect(child.status, child.stdout + child.stderr).toBe(0); + + const clientPid = Number(readFileSync(pidPath, "utf8")); + + try { + await expect( + waitForValue(() => !isAlive(clientPid), { intervalMs: 50, timeoutMs: 5_000 }), + ).resolves.toBe(true); + } finally { + if (isAlive(clientPid)) { + process.kill(clientPid, "SIGKILL"); + } + } + }); +}); describe("executeCommand", () => { it("captures stdout from a completed command", async () => { diff --git a/tests/fixtures/plugin-harness.test.ts b/tests/fixtures/plugin-harness.test.ts index e91523a..9fb2b10 100644 --- a/tests/fixtures/plugin-harness.test.ts +++ b/tests/fixtures/plugin-harness.test.ts @@ -9,6 +9,7 @@ import { createPluginHarnessSession } from "../../src/fixtures/plugin-harness"; import type { CreatePluginHarnessOptions } from "../../src/fixtures/plugin-harness"; import { createExecResult, frameEvalPayload } from "../helpers/create-exec-result"; import { cleanupTempDirectories, createTempDir } from "../helpers/create-temp-dir"; +import { ObsidianCommandTimeoutError } from "../../src/core/errors"; import type { CommandTransport } from "../../src/core/types"; const PLUGIN_ID = "harness-plugin"; @@ -23,6 +24,8 @@ afterEach(async () => { interface HarnessFixture { dataPath: string; events: string[]; + isEnabled(): boolean; + loseNextDisableReply(): void; lockRoot: string; setEnabled(value: boolean): void; setThrowOnDisable(value: boolean): void; @@ -41,6 +44,7 @@ async function createHarnessFixture(): Promise { const events: string[] = []; let enabled = true; let throwOnDisable = false; + let loseNextDisableReply = false; const transport: CommandTransport = async (request) => { if (request.argv[0] === "--help") { @@ -84,12 +88,23 @@ async function createHarnessFixture(): Promise { enabled = false; events.push("plugin:disable"); + + if (loseNextDisableReply) { + loseNextDisableReply = false; + throw new ObsidianCommandTimeoutError(request.bin, request.argv, 0); + } + return createExecResult(request.bin, request.argv, ""); } if (command === "plugin:reload") { events.push("plugin:reload"); - return createExecResult(request.bin, request.argv, ""); + // Obsidian answers a reload of a disabled plugin with an error on exit code 0. + return createExecResult( + request.bin, + request.argv, + enabled ? "" : `Error: Plugin "${PLUGIN_ID}" is not enabled.\n`, + ); } if (command === "eval") { @@ -110,7 +125,7 @@ async function createHarnessFixture(): Promise { return createExecResult( request.bin, request.argv, - `${frameEvalPayload(code, JSON.stringify({ ok: true, value: true }))}\n`, + `${frameEvalPayload(code, JSON.stringify({ ok: true, value: enabled }))}\n`, ); } @@ -130,6 +145,10 @@ async function createHarnessFixture(): Promise { return { dataPath, events, + isEnabled: () => enabled, + loseNextDisableReply() { + loseNextDisableReply = true; + }, lockRoot, setEnabled(value: boolean) { enabled = value; @@ -348,3 +367,49 @@ describe("createPluginHarnessSession", () => { await expect(fs.readdir(fixture.lockRoot)).resolves.toHaveLength(0); }); }); + +describe("createPluginHarnessSession under lost CLI replies", () => { + it("leaves a plugin it found disabled enabled after teardown", async () => { + const fixture = await createHarnessFixture(); + fixture.setEnabled(false); + const session = createPluginHarnessSession(baseOptions(fixture), "found-disabled"); + + await session.setup(); + await session.teardown(); + + expect(fixture.isEnabled()).toBe(true); + }); + + it("restores data and re-enables the plugin when the disable reply is lost", async () => { + const fixture = await createHarnessFixture(); + const session = createPluginHarnessSession(baseOptions(fixture), "lost-disable-reply"); + + await session.setup(); + await session.getContext().plugin.data().write({ changed: true }); + fixture.loseNextDisableReply(); + + await session.restoreData(); + + expect(fixture.isEnabled()).toBe(true); + await expect(fs.readFile(fixture.dataPath, "utf8")).resolves.not.toContain('"changed"'); + await session.teardown(); + }); + + it("re-enables the plugin when restoring its data fails", async () => { + const fixture = await createHarnessFixture(); + const session = createPluginHarnessSession(baseOptions(fixture), "restore-fails"); + + await session.setup(); + await session.getContext().plugin.data().write({ changed: true }); + // A file where the plugin folder was makes the data.json rollback fail. + const pluginDir = path.dirname(fixture.dataPath); + await fs.rm(pluginDir, { force: true, recursive: true }); + await fs.writeFile(pluginDir, "", "utf8"); + + await expect(session.restoreData()).rejects.toThrow(); + + expect(fixture.isEnabled()).toBe(true); + await fs.rm(pluginDir, { force: true }); + await session.teardown().catch(() => {}); + }); +}); diff --git a/tests/helpers/fake-plugin-app.ts b/tests/helpers/fake-plugin-app.ts new file mode 100644 index 0000000..04d1e8f --- /dev/null +++ b/tests/helpers/fake-plugin-app.ts @@ -0,0 +1,96 @@ +import { ObsidianCommandTimeoutError } from "../../src/core/errors"; +import type { CommandTransport } from "../../src/core/types"; +import { createExecResult, frameEvalPayload } from "./create-exec-result"; + +/** + * `lose-request`: the command never reaches Obsidian. `lose-reply`: Obsidian + * runs it, then the reply is lost. Both surface as a transport timeout. + */ +export type CommandFault = "lose-reply" | "lose-request"; + +export interface FakePluginApp { + readonly calls: string[]; + failNext(command: string, ...faults: CommandFault[]): void; + isEnabled(): boolean; + setEnabled(value: boolean): void; + transport: CommandTransport; +} + +/** + * A transport faithful to Obsidian 1.13's community plugin CLI handlers: a + * toggle that is already in effect, or a reload of a disabled plugin, is + * answered with an `Error: ...` reply on a zero exit code. + */ +export function createFakePluginApp(options: { + enabled?: boolean; + pluginId: string; + vaultRoot: string; +}): FakePluginApp { + const { pluginId, vaultRoot } = options; + let enabled = options.enabled ?? true; + const calls: string[] = []; + const faults = new Map(); + + function run(command: string, args: Record): string { + switch (command) { + case "vault": + return vaultRoot; + case "plugin": + return `id\t${pluginId}\nenabled\t${enabled}`; + case "plugin:enable": + if (enabled) return `Error: Plugin "${pluginId}" is already enabled.`; + enabled = true; + return `Enabled: ${pluginId}`; + case "plugin:disable": + if (!enabled) return `Error: Plugin "${pluginId}" is already disabled.`; + enabled = false; + return `Disabled: ${pluginId}`; + case "plugin:reload": + return enabled ? `Reloaded: ${pluginId}` : `Error: Plugin "${pluginId}" is not enabled.`; + case "eval": + // Every framed eval the plugin handle sends is a live-state probe. + return frameEvalPayload(args.code ?? "", JSON.stringify({ ok: true, value: enabled })); + default: + throw new Error(`Unhandled fake command: ${command}`); + } + } + + const transport: CommandTransport = async (request) => { + const [, command = "", ...rest] = request.argv; + const args = Object.fromEntries( + rest + .filter((entry) => entry.includes("=")) + .map((entry) => { + const [key, ...value] = entry.split("="); + return [key, value.join("=")]; + }), + ); + const fault = faults.get(command)?.shift(); + const lost = () => new ObsidianCommandTimeoutError(request.bin, request.argv, 0); + + if (fault === "lose-request") { + throw lost(); + } + + const reply = run(command, args); + calls.push(`${command} -> ${reply.split("\n")[0]}`); + + if (fault === "lose-reply") { + throw lost(); + } + + return createExecResult(request.bin, request.argv, `${reply}\n`); + }; + + return { + calls, + failNext(command, ...nextFaults) { + faults.set(command, [...(faults.get(command) ?? []), ...nextFaults]); + }, + isEnabled: () => enabled, + setEnabled(value) { + enabled = value; + }, + transport, + }; +} diff --git a/tests/helpers/orphan-client-child.test.ts b/tests/helpers/orphan-client-child.test.ts new file mode 100644 index 0000000..177a563 --- /dev/null +++ b/tests/helpers/orphan-client-child.test.ts @@ -0,0 +1,30 @@ +import { existsSync } from "node:fs"; +import process from "node:process"; + +import { test } from "vite-plus/test"; + +import { executeCommand } from "../../src/core/transport"; +import { sleep } from "../../src/core/wait"; + +// Run by tests/core/transport.test.ts in a child `vp test`. The test ends while +// a CLI client is still waiting for its reply, so Vitest stops the worker with +// the call in flight; the parent then checks the client did not outlive it. +test.runIf(process.env.OBSIDIAN_E2E_ORPHAN_CHILD === "1")( + "ends while a CLI client is in flight", + async () => { + const pidPath = process.env.OBSIDIAN_E2E_ORPHAN_PID_PATH!; + + void executeCommand({ + argv: [ + "-e", + `require("node:fs").writeFileSync(${JSON.stringify(pidPath)}, String(process.pid)); setInterval(() => {}, 1000);`, + ], + bin: process.execPath, + timeoutMs: 120_000, + }).catch(() => {}); + + while (!existsSync(pidPath)) { + await sleep(20); + } + }, +); From e68581f9cda6f76d77bd8ac30491956d861c22b5 Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 9 Oct 2026 15:39:39 +0200 Subject: [PATCH 2/3] fix(plugin): converge plugin toggles and reload on lost CLI replies, reap in-flight clients A lost or failed Obsidian CLI reply could leave the plugin under test disabled for the rest of a run, and a CLI client still waiting for a reply outlived the Vitest worker that spawned it. - plugin.enable()/disable() read Obsidian's enabled flag back instead of trusting the reply. An unanswered send is resent once unless dispatch reports it still running; a served reply whose flag did not change throws ObsidianCommandError. Toggles always send with allowNonZeroExit: false so a resolved exec means a served reply. - plugin.reload() reads the flag first: a disabled plugin is enabled (Obsidian answers plugin:reload on a disabled plugin with an exit-0 "not enabled" error and does nothing). Any other served Error: reply throws. The caller's exec options reach every call reload makes. - createPluginHarness no longer re-disables a plugin it found disabled (that snapshot is the product of an earlier failure and cascaded across files). The per-test restore always re-enables, reporting both errors when restore and re-enable fail, and afterEach/afterAll use teardownTimeoutMs (default 30 s -> 90 s). - The transport tracks live clients and kills them on SIGTERM (Vitest stops forks workers with SIGTERM, whose default action runs no timers), then re-raises when it is the only listener. A signal-killed client reports 128 + signal instead of resolving as exit 0. - The dist-binding test gets a 60 s timeout: it rebuilds dist after any fixture edit, which takes longer than the default 5 s under load. --- .changeset/converge-plugin-toggles.md | 24 ++++++ README.md | 16 ++++ src/core/transport.ts | 51 ++++++++++++- src/fixtures/plugin-harness.ts | 39 ++++++++-- src/fixtures/test-context.ts | 7 +- src/plugin/plugin.ts | 89 ++++++++++++++++++++--- tests/core/client.test.ts | 26 ++++++- tests/core/plugin.test.ts | 67 ++++++++++++++++- tests/core/transport.test.ts | 10 +++ tests/fixtures/plugin-harness.test.ts | 88 ++++++++++++++++++---- tests/helpers/fake-plugin-app.ts | 49 ++++++++++--- tests/helpers/orphan-client-child.test.ts | 3 - 12 files changed, 409 insertions(+), 60 deletions(-) create mode 100644 .changeset/converge-plugin-toggles.md diff --git a/.changeset/converge-plugin-toggles.md b/.changeset/converge-plugin-toggles.md new file mode 100644 index 0000000..f39e31f --- /dev/null +++ b/.changeset/converge-plugin-toggles.md @@ -0,0 +1,24 @@ +--- +"obsidian-e2e": patch +--- + +A lost Obsidian CLI reply no longer leaves the plugin under test disabled for +the rest of the run, and no CLI client outlives its test worker. + +- `plugin.enable()` and `plugin.disable()` read the plugin's enabled flag back + instead of trusting the reply. A toggle whose reply was lost resolves once the + flag matches, and a toggle whose request was lost is sent once more. When + Obsidian answers but the flag does not change, they now throw + `ObsidianCommandError` instead of resolving silently. +- `plugin.reload()` enables a disabled plugin. Obsidian answers a reload of a + disabled plugin with `Error: Plugin "" is not enabled.` on exit code 0, + which used to pass as success. Any other `Error:` reply to a reload (for + example `Failed to reload`) now throws `ObsidianCommandError`. +- `createPluginHarness()` leaves the plugin enabled after the suite, even when + it found the plugin disabled, and every data restore re-enables the plugin + and waits until it is ready, even when the restore fails. `afterEach` now uses + `teardownTimeoutMs`, whose default rises from 30 to 90 seconds. +- A CLI client still waiting for its reply is killed when Vitest stops the test + worker with SIGTERM. It used to keep running after the worker exited. +- A CLI client killed by a signal now fails with exit code `128 + signal` + (`ObsidianCommandError`) instead of resolving as a successful empty reply. diff --git a/README.md b/README.md index 0f1cab2..76a0c0c 100644 --- a/README.md +++ b/README.md @@ -715,6 +715,14 @@ only then the vault-lock release - so a waiting run never acquires the vault mid-cleanup. A failed marker clear fails the suite instead of being swallowed, since it means Obsidian retained stale ownership metadata. +The harness leaves the plugin enabled after the suite, even if it found the +plugin disabled. A disabled plugin under test is left over from an earlier +failure, and disabling it again would fail every later suite in the run. Each +data restore re-enables the plugin and waits until it is ready, even when +`beforeDataRestore` or the restore itself fails. `teardownTimeoutMs` (default +90 seconds) bounds both `afterEach` and `afterAll`, so a restore that has to +retry a lost CLI reply finishes before the next test starts. + `resolveObsidianEnvOptions()` maps the canonical `OBSIDIAN_E2E_VAULT`, `OBSIDIAN_E2E_VAULT_PATH`, and `OBSIDIAN_E2E_OBSIDIAN_HOME` env (with an optional `legacyPrefix` fallback such as `PODNOTES_E2E_VAULT`) into spreadable client @@ -1036,6 +1044,14 @@ test("waits for generated content and plugin state", async ({ obsidian, sandbox, }); ``` +`plugin.enable()` and `plugin.disable()` read the plugin's enabled flag back +from Obsidian instead of trusting the CLI reply. If the reply is lost but the +toggle took effect, they resolve. If the request never reached Obsidian, they +send it once more. If Obsidian answers but the flag did not change (for example +`Failed to enable`), they throw `ObsidianCommandError`. `plugin.reload()` on a +disabled plugin enables it, which loads the plugin's `main.js` from disk. Any +other `Error:` reply to a reload throws `ObsidianCommandError`. + If you just need time to pass without inventing a fake polling condition, use `await obsidian.sleep(ms)`. diff --git a/src/core/transport.ts b/src/core/transport.ts index 53cc888..bebb6d7 100644 --- a/src/core/transport.ts +++ b/src/core/transport.ts @@ -1,10 +1,50 @@ -import { spawn } from "node:child_process"; +import { spawn, type ChildProcess } from "node:child_process"; +import { constants } from "node:os"; import { ObsidianCommandError, ObsidianCommandTimeoutError } from "./errors"; import type { CommandTransport, ExecuteRequest, ExecResult } from "./types"; const DEFAULT_TIMEOUT_MS = 30_000; +/** + * Vitest stops a forks worker with SIGTERM, whose default action skips timers + * and `exit` handlers, so a client still in flight would outlive the worker. + */ +const liveClients = new Set(); + +function killLiveClientsOnSigterm(): void { + for (const child of liveClients) { + child.kill("SIGTERM"); + } + + liveClients.clear(); + process.off("SIGTERM", killLiveClientsOnSigterm); + + // With no other listener left, re-raising runs the default action (terminate) + // exactly as if this listener had never been attached. + if (process.listenerCount("SIGTERM") === 0) { + process.kill(process.pid, "SIGTERM"); + } +} + +function trackClient(child: ChildProcess): void { + if (liveClients.size === 0) { + process.prependListener("SIGTERM", killLiveClientsOnSigterm); + } + + liveClients.add(child); + + const untrack = () => { + liveClients.delete(child); + + if (liveClients.size === 0) { + process.off("SIGTERM", killLiveClientsOnSigterm); + } + }; + + child.once("exit", untrack).once("error", untrack); +} + export const executeCommand: CommandTransport = async ({ allowNonZeroExit = false, argv, @@ -18,6 +58,7 @@ export const executeCommand: CommandTransport = async ({ env, stdio: ["ignore", "pipe", "pipe"], }); + trackClient(child); const stdoutChunks: Buffer[] = []; const stderrChunks: Buffer[] = []; @@ -43,9 +84,9 @@ export const executeCommand: CommandTransport = async ({ reject(error); }); - child.on("close", (code) => { + child.on("close", (code, signal) => { clearTimeout(timer); - resolve(code ?? 0); + resolve(shellExitCode(code, signal)); }); }); @@ -66,3 +107,7 @@ export const executeCommand: CommandTransport = async ({ return result; }; + +function shellExitCode(code: number | null, signal: NodeJS.Signals | null): number { + return signal ? 128 + constants.signals[signal] : (code ?? 0); +} diff --git a/src/fixtures/plugin-harness.ts b/src/fixtures/plugin-harness.ts index 922a34b..5a0b8a7 100644 --- a/src/fixtures/plugin-harness.ts +++ b/src/fixtures/plugin-harness.ts @@ -24,7 +24,7 @@ import { createInternalTestContext } from "./test-context"; import type { CreateObsidianTestOptions, TestContext } from "./types"; const DEFAULT_SETUP_TIMEOUT_MS = 90_000; -const DEFAULT_TEARDOWN_TIMEOUT_MS = 30_000; +const DEFAULT_TEARDOWN_TIMEOUT_MS = 90_000; const DEFAULT_RELOAD_TIMEOUT_MS = 30_000; const DEFAULT_READY_INTERVAL_MS = 200; const DEFAULT_LOCK_TIMEOUT_MS = 60_000; @@ -62,7 +62,7 @@ export interface CreatePluginHarnessOptions extends CreateObsidianTestOptions { symlinkArtifacts?: string[]; /** Root the symlinked artifacts must point at. Defaults to `process.cwd()`. */ symlinkRepoRoot?: string; - /** `afterAll` timeout in ms. Defaults to 30000. */ + /** `afterEach` and `afterAll` timeout in ms. Defaults to 90000. */ teardownTimeoutMs?: number; /** Extra readiness predicate beyond plugin-loaded and the ready command. */ waitUntilReady?: (obsidian: ObsidianClient) => boolean | Promise; @@ -102,7 +102,7 @@ export function createPluginHarness( beforeEach(() => session.resetDiagnostics()); - afterEach(() => session.restoreData()); + afterEach(() => session.restoreData(), teardownTimeoutMs); afterAll(() => session.teardown(), teardownTimeoutMs); @@ -167,11 +167,33 @@ export function createPluginHarnessSession( return; } - await beforeDataRestore?.(context.obsidian); - await plugin.disable({ filter: pluginFilter }); - await plugin.restoreData(); - await plugin.enable({ filter: pluginFilter }); - await plugin.waitUntilReady(readyWaitOptions); + const errors: unknown[] = []; + + try { + await beforeDataRestore?.(context.obsidian); + await plugin.disable({ filter: pluginFilter }); + await plugin.restoreData(); + } catch (error) { + errors.push(error); + } + + try { + await plugin.enable({ filter: pluginFilter }); + await plugin.waitUntilReady(readyWaitOptions); + } catch (error) { + errors.push(error); + } + + if (errors.length === 1) { + throw errors[0]; + } + + if (errors.length > 1) { + throw new AggregateError( + errors, + `Plugin "${pluginId}" data restore and re-enable both failed.`, + ); + } } return { @@ -197,6 +219,7 @@ export function createPluginHarnessSession( context = await createInternalTestContext({ ...testOptions, beforeSandbox: needsPreflight ? runPreflight : undefined, + keepPluginsEnabled: true, sharedVaultLock: testOptions.sharedVaultLock ?? { onBusy: "wait", timeoutMs: DEFAULT_LOCK_TIMEOUT_MS, diff --git a/src/fixtures/test-context.ts b/src/fixtures/test-context.ts index 659f69d..bf8a070 100644 --- a/src/fixtures/test-context.ts +++ b/src/fixtures/test-context.ts @@ -12,14 +12,15 @@ import type { CreateObsidianTestOptions, PluginSessionOptions, TestContext } fro interface CreateInternalTestContextOptions extends CreateObsidianTestOptions { beforeSandbox?: (obsidian: ObsidianClient) => Promise | void; createVault?: (obsidian: ObsidianClient) => Promise | VaultApi; + keepPluginsEnabled?: boolean; testName?: string; vaultLock?: VaultRunLock | null; } interface TrackedPluginSession { + disableOnCleanup: boolean; filter?: PluginSessionOptions["filter"]; plugin: PluginHandle; - wasEnabled: boolean; } export async function createTestContext( @@ -94,7 +95,7 @@ export async function createInternalTestContext( await getClientInternals(obsidian).restoreAll(); } finally { for (const session of [...trackedPlugins.values()].reverse()) { - if (!session.wasEnabled) { + if (session.disableOnCleanup) { await recordCleanupError(cleanupErrors, async () => session.plugin.disable({ filter: session.filter }), ); @@ -154,9 +155,9 @@ export async function createInternalTestContext( } trackedPlugins.set(id, { + disableOnCleanup: !wasEnabled && !options.keepPluginsEnabled, filter: sessionOptions.filter, plugin, - wasEnabled, }); return plugin; diff --git a/src/plugin/plugin.ts b/src/plugin/plugin.ts index 3b19b13..fec9f7b 100644 --- a/src/plugin/plugin.ts +++ b/src/plugin/plugin.ts @@ -1,7 +1,10 @@ import path from "node:path"; +import { ObsidianCommandDispatchError, ObsidianCommandError } from "../core/errors"; import { getClientInternals } from "../core/internals"; import type { + ExecOptions, + ExecResult, JsonFile, JsonFileUpdater, ObsidianClient, @@ -17,6 +20,8 @@ import type { import { runEvalJson } from "../dev/eval-json"; import { createJsonFile } from "../vault/json-file"; +const MAX_TOGGLE_SENDS = 2; + export function createPluginHandle(client: ObsidianClient, id: string): PluginHandle { async function resolveDataPath() { const vaultPath = await client.vaultPath(); @@ -31,6 +36,62 @@ export function createPluginHandle(client: ObsidianClient, id: string): PluginHa } } + async function readEnabledFlag(execOptions: ExecOptions = {}): Promise { + const output = await client.execText( + "plugin", + { id }, + { ...execOptions, allowNonZeroExit: true }, + ); + const match = /^enabled\s+(true|false)\s*$/m.exec(output); + return match ? match[1] === "true" : null; + } + + async function setEnabled( + enabled: boolean, + options: PluginToggleOptions, + execOptions: ExecOptions = {}, + ): Promise { + const command = enabled ? "plugin:enable" : "plugin:disable"; + + for (let send = 1; ; send += 1) { + let served: ExecResult | undefined; + let sendError: unknown; + + try { + // A nonzero exit means no reply was served, so it must take the resend path. + served = await client.exec( + command, + { filter: options.filter, id }, + { ...execOptions, allowNonZeroExit: false }, + ); + } catch (error) { + sendError = error; + } + + const flag = await readEnabledFlag(execOptions); + + if (flag === enabled) { + return; + } + + if (served) { + const state = flag === null ? "reported no enabled flag" : `its enabled flag is ${flag}`; + throw new ObsidianCommandError( + `Obsidian answered ${command} for plugin "${id}" with "${served.stdout.trim()}", but ${state}.`, + served, + ); + } + + // A second enable during Obsidian's `loadPlugin` read of main.js would load the plugin twice. + const stillRunning = + sendError instanceof ObsidianCommandDispatchError && sendError.reason === "still-pending"; + + if (send === MAX_TOGGLE_SENDS || stillRunning) { + throw sendError; + } + } + } + function withDefaultReadyReloadOptions(options: PluginReloadOptions = {}): PluginReloadOptions { return { ...options, @@ -63,26 +124,32 @@ export function createPluginHandle(client: ObsidianClient, id: string): PluginHa return resolveDataPath(); }, async disable(options: PluginToggleOptions = {}) { - await client.exec("plugin:disable", { - filter: options.filter, - id, - }); + await setEnabled(false, options); }, async enable(options: PluginToggleOptions = {}) { - await client.exec("plugin:enable", { - filter: options.filter, - id, - }); + await setEnabled(true, options); }, id, async isEnabled() { - const output = await client.execText("plugin", { id }, { allowNonZeroExit: true }); - return /enabled\s+true/i.test(output); + return (await readEnabledFlag()) === true; }, async reload(options: PluginReloadOptions = {}) { const { readyOptions, waitUntilReady, ...execOptions } = options; - await client.exec("plugin:reload", { id }, execOptions); + // Obsidian refuses to reload a disabled plugin (an exit-0 "Error:" reply). + // Enabling loads its main.js from disk, which is the reload asked for. + if ((await readEnabledFlag(execOptions)) === false) { + await setEnabled(true, {}, execOptions); + } else { + const result = await client.exec("plugin:reload", { id }, execOptions); + + if (result.stdout.startsWith("Error:")) { + throw new ObsidianCommandError( + `Obsidian answered plugin:reload for plugin "${id}" with "${result.stdout.trim()}".`, + result, + ); + } + } if (waitUntilReady) { await this.waitUntilReady(readyOptions); diff --git a/tests/core/client.test.ts b/tests/core/client.test.ts index 8dedc62..99cfcda 100644 --- a/tests/core/client.test.ts +++ b/tests/core/client.test.ts @@ -174,6 +174,16 @@ describe("createObsidianClient", () => { test("applies default exec options to plugin and command handles", async () => { const transport = vi.fn().mockImplementation(async (request) => { + if (request.argv[1] === "plugin") { + return { + argv: request.argv, + command: request.bin, + exitCode: 0, + stderr: "", + stdout: "enabled\ttrue\n", + }; + } + if (request.argv[1] === "plugin:reload" || request.argv[1] === "command") { return { argv: request.argv, @@ -205,8 +215,17 @@ describe("createObsidianClient", () => { }); await client.command("workspace:save").run(); - expect(transport).toHaveBeenNthCalledWith( - 1, + expect(transport).toHaveBeenCalledWith( + expect.objectContaining({ + argv: ["vault=dev", "plugin", "id=quickadd"], + cwd: "/tmp/default", + env: { + BASE: "1", + EXTRA: "2", + }, + }), + ); + expect(transport).toHaveBeenCalledWith( expect.objectContaining({ argv: ["vault=dev", "plugin:reload", "id=quickadd"], cwd: "/tmp/default", @@ -216,8 +235,7 @@ describe("createObsidianClient", () => { }, }), ); - expect(transport).toHaveBeenNthCalledWith( - 2, + expect(transport).toHaveBeenCalledWith( expect.objectContaining({ argv: ["vault=dev", "command", "id=workspace:save"], cwd: "/tmp/default", diff --git a/tests/core/plugin.test.ts b/tests/core/plugin.test.ts index a6ef1b2..7485c77 100644 --- a/tests/core/plugin.test.ts +++ b/tests/core/plugin.test.ts @@ -2,6 +2,7 @@ import { promises as fs } from "node:fs"; import path from "node:path"; import { afterEach, describe, expect, test, vi } from "vite-plus/test"; import { createObsidianClient } from "../../src/core/client"; +import { ObsidianCommandError, ObsidianCommandTimeoutError } from "../../src/core/errors"; import { sleep, waitForValue } from "../../src/core/wait"; import { cleanupTempDirectories, @@ -10,7 +11,7 @@ import { import { createStubObsidianClient } from "../helpers/stub-obsidian-client"; import { createExecResult, frameEvalPayload } from "../helpers/create-exec-result"; import { createFakePluginApp } from "../helpers/fake-plugin-app"; -import type { CommandTransport } from "../../src/core/types"; +import type { CommandTransport, ExecOptions } from "../../src/core/types"; const tempDirectories: string[] = []; @@ -84,6 +85,10 @@ describe("plugin readiness helpers", () => { ); } + if (command === "plugin") { + return createExecResult(request.bin, request.argv, "enabled\ttrue\n"); + } + if (command === "plugin:reload") { return createExecResult(request.bin, request.argv, ""); } @@ -397,9 +402,10 @@ describe("plugin data ergonomics", () => { }); describe("plugin lifecycle under lost CLI replies", () => { - function createLifecycleFixture(enabled: boolean) { + function createLifecycleFixture(enabled: boolean, defaultExecOptions?: ExecOptions) { const app = createFakePluginApp({ enabled, pluginId: "quickadd", vaultRoot: "/tmp/vault" }); const client = createObsidianClient({ + defaultExecOptions, intervalMs: 5, timeoutMs: 500, transport: app.transport, @@ -446,6 +452,63 @@ describe("plugin lifecycle under lost CLI replies", () => { "plugin:enable -> Enabled: quickadd", ]); }); + + test("enable does not resend while Obsidian reports the first one still running", async () => { + const { app, plugin } = createLifecycleFixture(false); + app.failNext("plugin:enable", "still-pending"); + + await expect(plugin.enable()).rejects.toThrow("still running"); + + expect(app.calls.filter((call) => call.startsWith("plugin:enable"))).toEqual([ + "plugin:enable -> (still running)", + ]); + }); + + test("enable throws when Obsidian refuses it, without resending", async () => { + const { app, plugin } = createLifecycleFixture(false); + app.failNext("plugin:enable", "refuse"); + + await expect(plugin.enable()).rejects.toBeInstanceOf(ObsidianCommandError); + + expect(app.calls.filter((call) => call.startsWith("plugin:enable"))).toEqual([ + "plugin:enable -> Error: Failed to enable: quickadd", + ]); + }); + + test("enable resends after an unanswered send even when nonzero exits are allowed", async () => { + const { app, plugin } = createLifecycleFixture(false, { allowNonZeroExit: true }); + app.failNext("plugin:enable", "connect-fail"); + + await plugin.enable(); + + expect(app.isEnabled()).toBe(true); + expect(app.calls.filter((call) => call.startsWith("plugin:enable"))).toEqual([ + "plugin:enable -> (exit 1)", + "plugin:enable -> Enabled: quickadd", + ]); + }); + + test("enable gives up after a resend also goes unanswered", async () => { + const { app, plugin } = createLifecycleFixture(false); + app.failNext("plugin:enable", "lose-request", "lose-request"); + + await expect(plugin.enable()).rejects.toBeInstanceOf(ObsidianCommandTimeoutError); + + expect(app.isEnabled()).toBe(false); + expect(app.calls.filter((call) => call.startsWith("plugin:enable"))).toHaveLength(2); + }); + + test("reload throws when Obsidian fails to reload an enabled plugin", async () => { + const { app, plugin } = createLifecycleFixture(true); + app.failNext("plugin:reload", "refuse"); + + const error = await plugin.reload().catch((thrown: unknown) => thrown); + + expect(error).toBeInstanceOf(ObsidianCommandError); + expect((error as Error).message).toBe( + 'Obsidian answered plugin:reload for plugin "quickadd" with "Error: Failed to reload: quickadd".', + ); + }); }); async function createVaultRoot(): Promise { diff --git a/tests/core/transport.test.ts b/tests/core/transport.test.ts index 18dab70..ef502a0 100644 --- a/tests/core/transport.test.ts +++ b/tests/core/transport.test.ts @@ -84,6 +84,16 @@ describe("executeCommand", () => { expect((error as ObsidianCommandError).result.stderr).toBe("broken"); }); + it("rejects a client killed by a signal instead of resolving it as success", async () => { + const error = await executeCommand({ + argv: ["-e", "process.kill(process.pid, 'SIGTERM')"], + bin: process.execPath, + }).catch((thrown: unknown) => thrown); + + expect(error).toBeInstanceOf(ObsidianCommandError); + expect((error as ObsidianCommandError).result.exitCode).toBe(143); + }); + // Real child process and wall clock on purpose: this exercises the actual // spawn/kill timeout path; fake timers cannot advance a separate process. it("kills an overrunning command and rejects with ObsidianCommandTimeoutError", async () => { diff --git a/tests/fixtures/plugin-harness.test.ts b/tests/fixtures/plugin-harness.test.ts index 9fb2b10..d6506cc 100644 --- a/tests/fixtures/plugin-harness.test.ts +++ b/tests/fixtures/plugin-harness.test.ts @@ -9,7 +9,7 @@ import { createPluginHarnessSession } from "../../src/fixtures/plugin-harness"; import type { CreatePluginHarnessOptions } from "../../src/fixtures/plugin-harness"; import { createExecResult, frameEvalPayload } from "../helpers/create-exec-result"; import { cleanupTempDirectories, createTempDir } from "../helpers/create-temp-dir"; -import { ObsidianCommandTimeoutError } from "../../src/core/errors"; +import { ObsidianCommandError, ObsidianCommandTimeoutError } from "../../src/core/errors"; import type { CommandTransport } from "../../src/core/types"; const PLUGIN_ID = "harness-plugin"; @@ -27,8 +27,9 @@ interface HarnessFixture { isEnabled(): boolean; loseNextDisableReply(): void; lockRoot: string; + refuseEnable(): void; setEnabled(value: boolean): void; - setThrowOnDisable(value: boolean): void; + setThrowOnMarkerClear(value: boolean): void; transport: CommandTransport; vaultRoot: string; } @@ -43,8 +44,9 @@ async function createHarnessFixture(): Promise { const events: string[] = []; let enabled = true; - let throwOnDisable = false; + let throwOnMarkerClear = false; let loseNextDisableReply = false; + let refuseEnable = false; const transport: CommandTransport = async (request) => { if (request.argv[0] === "--help") { @@ -76,16 +78,20 @@ async function createHarnessFixture(): Promise { } if (command === "plugin:enable") { + if (refuseEnable) { + return createExecResult( + request.bin, + request.argv, + `Error: Failed to enable: ${PLUGIN_ID}\n`, + ); + } + enabled = true; events.push("plugin:enable"); return createExecResult(request.bin, request.argv, ""); } if (command === "plugin:disable") { - if (throwOnDisable) { - throw new Error("plugin:disable failed"); - } - enabled = false; events.push("plugin:disable"); @@ -111,6 +117,10 @@ async function createHarnessFixture(): Promise { const code = args.code ?? ""; if (code.includes("delete window.__obsidianE2ELock")) { + if (throwOnMarkerClear) { + throw new Error("marker clear failed"); + } + events.push("eval:clearMarker"); return createExecResult(request.bin, request.argv, "cleared\n"); } @@ -150,11 +160,14 @@ async function createHarnessFixture(): Promise { loseNextDisableReply = true; }, lockRoot, + refuseEnable() { + refuseEnable = true; + }, setEnabled(value: boolean) { enabled = value; }, - setThrowOnDisable(value: boolean) { - throwOnDisable = value; + setThrowOnMarkerClear(value: boolean) { + throwOnMarkerClear = value; }, transport, vaultRoot, @@ -232,7 +245,7 @@ describe("createPluginHarness runner binding", () => { } }); - it('externalizes the runner as "vitest" in the built dist', () => { + it('externalizes the runner as "vitest" in the built dist', { timeout: 60_000 }, () => { // The source guard above is not enough on its own: inside this package // "vitest" is aliased to vite-plus-test, so if the vitest peer dep were dropped // vp pack would bundle that runner inline (emitting a local `function beforeAll`) @@ -315,8 +328,7 @@ describe("createPluginHarnessSession", () => { it("aggregates multiple teardown errors and still releases the lock", async () => { const fixture = await createHarnessFixture(); - fixture.setEnabled(false); - fixture.setThrowOnDisable(true); + fixture.setThrowOnMarkerClear(true); const session = createPluginHarnessSession( { @@ -395,18 +407,66 @@ describe("createPluginHarnessSession under lost CLI replies", () => { await session.teardown(); }); + it("reports a restore hook that rejects without a reason", async () => { + const fixture = await createHarnessFixture(); + const session = createPluginHarnessSession( + { + ...baseOptions(fixture), + beforeDataRestore: () => Promise.reject(), + }, + "restore-rejects-undefined", + ); + + await session.setup(); + + try { + const outcome = await session.restoreData().then( + () => "resolved", + () => "rejected", + ); + + expect(outcome).toBe("rejected"); + expect(fixture.isEnabled()).toBe(true); + } finally { + await session.teardown().catch(() => {}); + } + }); + + it("reports both failures when the restore and the re-enable fail", async () => { + const fixture = await createHarnessFixture(); + const session = createPluginHarnessSession(baseOptions(fixture), "restore-and-enable-fail"); + + await session.setup(); + await session.getContext().plugin.data().write({ changed: true }); + const pluginDir = path.dirname(fixture.dataPath); + await fs.rm(pluginDir, { force: true, recursive: true }); + await fs.writeFile(pluginDir, "", "utf8"); + fixture.refuseEnable(); + + const error = await session.restoreData().catch((thrown: unknown) => thrown); + + expect(error).toBeInstanceOf(AggregateError); + const [restoreError, enableError] = (error as AggregateError).errors; + expect(restoreError).toMatchObject({ code: "ENOTDIR" }); + expect(enableError).toBeInstanceOf(ObsidianCommandError); + await fs.rm(pluginDir, { force: true }); + await session.teardown().catch(() => {}); + }); + it("re-enables the plugin when restoring its data fails", async () => { const fixture = await createHarnessFixture(); const session = createPluginHarnessSession(baseOptions(fixture), "restore-fails"); await session.setup(); await session.getContext().plugin.data().write({ changed: true }); - // A file where the plugin folder was makes the data.json rollback fail. const pluginDir = path.dirname(fixture.dataPath); await fs.rm(pluginDir, { force: true, recursive: true }); await fs.writeFile(pluginDir, "", "utf8"); - await expect(session.restoreData()).rejects.toThrow(); + await expect(session.restoreData()).rejects.toMatchObject({ + code: "ENOTDIR", + path: fixture.dataPath, + }); expect(fixture.isEnabled()).toBe(true); await fs.rm(pluginDir, { force: true }); diff --git a/tests/helpers/fake-plugin-app.ts b/tests/helpers/fake-plugin-app.ts index 04d1e8f..6a27089 100644 --- a/tests/helpers/fake-plugin-app.ts +++ b/tests/helpers/fake-plugin-app.ts @@ -1,18 +1,22 @@ -import { ObsidianCommandTimeoutError } from "../../src/core/errors"; +import { + ObsidianCommandDispatchError, + ObsidianCommandError, + ObsidianCommandTimeoutError, +} from "../../src/core/errors"; import type { CommandTransport } from "../../src/core/types"; import { createExecResult, frameEvalPayload } from "./create-exec-result"; -/** - * `lose-request`: the command never reaches Obsidian. `lose-reply`: Obsidian - * runs it, then the reply is lost. Both surface as a transport timeout. - */ -export type CommandFault = "lose-reply" | "lose-request"; +export type CommandFault = + | "connect-fail" + | "lose-reply" + | "lose-request" + | "refuse" + | "still-pending"; export interface FakePluginApp { readonly calls: string[]; failNext(command: string, ...faults: CommandFault[]): void; isEnabled(): boolean; - setEnabled(value: boolean): void; transport: CommandTransport; } @@ -48,7 +52,6 @@ export function createFakePluginApp(options: { case "plugin:reload": return enabled ? `Reloaded: ${pluginId}` : `Error: Plugin "${pluginId}" is not enabled.`; case "eval": - // Every framed eval the plugin handle sends is a live-state probe. return frameEvalPayload(args.code ?? "", JSON.stringify({ ok: true, value: enabled })); default: throw new Error(`Unhandled fake command: ${command}`); @@ -68,11 +71,36 @@ export function createFakePluginApp(options: { const fault = faults.get(command)?.shift(); const lost = () => new ObsidianCommandTimeoutError(request.bin, request.argv, 0); + if (fault === "connect-fail") { + calls.push(`${command} -> (exit 1)`); + const result = { ...createExecResult(request.bin, request.argv, ""), exitCode: 1 }; + + if (!request.allowNonZeroExit) { + throw new ObsidianCommandError("Obsidian command failed with exit code 1", result); + } + + return result; + } + if (fault === "lose-request") { + calls.push(`${command} -> (request lost)`); throw lost(); } - const reply = run(command, args); + if (fault === "still-pending") { + calls.push(`${command} -> (still running)`); + throw new ObsidianCommandDispatchError( + "still running", + "still-pending", + "nonce", + request.argv, + ); + } + + const reply = + fault === "refuse" + ? `Error: Failed to ${command.replace("plugin:", "")}: ${pluginId}` + : run(command, args); calls.push(`${command} -> ${reply.split("\n")[0]}`); if (fault === "lose-reply") { @@ -88,9 +116,6 @@ export function createFakePluginApp(options: { faults.set(command, [...(faults.get(command) ?? []), ...nextFaults]); }, isEnabled: () => enabled, - setEnabled(value) { - enabled = value; - }, transport, }; } diff --git a/tests/helpers/orphan-client-child.test.ts b/tests/helpers/orphan-client-child.test.ts index 177a563..80cc721 100644 --- a/tests/helpers/orphan-client-child.test.ts +++ b/tests/helpers/orphan-client-child.test.ts @@ -6,9 +6,6 @@ import { test } from "vite-plus/test"; import { executeCommand } from "../../src/core/transport"; import { sleep } from "../../src/core/wait"; -// Run by tests/core/transport.test.ts in a child `vp test`. The test ends while -// a CLI client is still waiting for its reply, so Vitest stops the worker with -// the call in flight; the parent then checks the client did not outlive it. test.runIf(process.env.OBSIDIAN_E2E_ORPHAN_CHILD === "1")( "ends while a CLI client is in flight", async () => { From 80f7650161c1f4558a363f520045800382f7d9ee Mon Sep 17 00:00:00 2001 From: Christian Bager Bach Houmann Date: Fri, 9 Oct 2026 17:31:32 +0200 Subject: [PATCH 3/3] test: wait up to 20 s for a child vp test to start in the lock fixture tests Spawning a child `vp test` takes longer than 2 s under load (load average 40-60 on the 12-core dev host), so both cross-process lock tests failed on main. The wait only bounds a failure; a healthy child still signals at once. --- tests/fixtures/shared-vault-lock-fixture.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/tests/fixtures/shared-vault-lock-fixture.test.ts b/tests/fixtures/shared-vault-lock-fixture.test.ts index 081c988..62911ec 100644 --- a/tests/fixtures/shared-vault-lock-fixture.test.ts +++ b/tests/fixtures/shared-vault-lock-fixture.test.ts @@ -254,7 +254,7 @@ async function spawnFixtureChild({ async release() { await writeFile(releaseFile, "release\n", "utf8"); }, - async waitForJson(fileName, timeoutMs = 2_000) { + async waitForJson(fileName, timeoutMs = 20_000) { const targetPath = path.join(signalDir, fileName); const startedAt = Date.now();