Repository navigation
fix(plugin): converge plugin toggles and reload on lost CLI replies, reap in-flight clients - #39
Conversation
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.
…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.
…e 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.
Investigation notes: where replies are droppedRig: Linux, Obsidian 1.13.7 under Xvfb, an isolated instance ( Socket and client: nothing dropped.
In the last run, one dispatch attempt's client hit its 2.5 s budget 150 ms before Obsidian replied, and a registry poll recovered the reply. CPU load alone did not reproduce a lost reply. Obsidian's command handler can stall forever. The main process awaits Why one lost reply cascaded. I read these from Obsidian's
After that, Orphans. vite-plus-test's quickaddNo consumer config change is needed. The patch reaches quickadd through Review findings not acted on
|
|
PASS Head: Independent live verification used Linux Obsidian 1.13.7 on display :200, the native CLI, and the supplied socket proxy. Both frozen installs and package builds succeeded. Identical consumer suites imported each worktree's built dist.
The proxy confirmed six swallowed CLI connections during each 40 s blackout and one during each orphan check. Shutdown used the rig PID files. No matching rig Obsidian process or obsidian-cli vault=vault client remains. Tracked files in both worktrees were unchanged. |
1 participant
Why
A lost Obsidian CLI reply could leave the plugin under test disabled for the rest of a run. That is the failure in chhoumann/quickadd#2217: 22 files failed with
Timed out waiting for plugin "quickadd" to be ready, andenabledPluginswas empty afterwards. A CLI client still waiting for a reply also outlived its Vitest worker, which produced the 20+ orphanedobsidian ... __obsidian-e2e:dispatchprocesses. The fix reads the plugin's real state instead of trusting replies, and it kills in-flight clients when the worker stops.What changed
plugin.enable()/plugin.disable()read the enabled flag back after each send. An unanswered send is resent once, unless dispatch reports it still running. A served reply whose flag did not change throws.plugin.reload()enables a disabled plugin instead of accepting Obsidian's exit-0not enablederror.createPluginHarnessno longer re-disables a plugin it found disabled. That snapshot is left over from an earlier failure, and re-applying it cascaded across files. Each restore re-enables the plugin even when the restore fails, and theafterEach/afterAllbudget is now 90 s.SIGTERMand then re-raises the signal. Vitest stops forks workers withSIGTERM, and the default action runs no timers.Scope
/usr/bin/open), and the reaper covers the forks pool only (worker threads get no signals).Tradeoffs
reload()reads the flag before it sends, which costs one extra CLI call per reload. The alternative parses Obsidian's error text to detect the same state.^0.12consumers get it without a dependency bump. The new throws happen only where 0.12.1 left a silently broken state.Blast Radius
The change touches every
enable/disable/reloadcall and everycreatePluginHarnesssuite. Toggles now cost one extra read.enable/disable/reloadcan throw where they used to resolve over a broken state.createPluginTestkeeps its documented restore-prior-state behavior.Verification
plugin:disablegives 65/65 passing, wheremainfails the next test. A test that ends with a dispatch in flight: its client closes about 1 s after connect, where onmainit was still alive 69 s after vitest exited.main.pnpm exec vp checkis clean, andpnpm exec vp testpasses 386 tests.