fix(plugin): converge plugin toggles and reload on lost CLI replies, reap in-flight clients

.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 "<id>" 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.

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)`.
 

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<ChildProcess>();
+
+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);
+}

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<boolean>;
@@ -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,

src/fixtures/test-context.ts

@@ -12,14 +12,15 @@ import type { CreateObsidianTestOptions, PluginSessionOptions, TestContext } fro
 interface CreateInternalTestContextOptions extends CreateObsidianTestOptions {
   beforeSandbox?: (obsidian: ObsidianClient) => Promise<void> | void;
   createVault?: (obsidian: ObsidianClient) => Promise<VaultApi> | 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;

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<boolean | null> {
+    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<void> {
+    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);