Add force-uninstall escape hatch and run deactivate before uninstall#9
Merged
Conversation
A throwing (or unloadable) uninstall hook could permanently block plugin removal: the DELETE handler returned 400 before deleting anything, with no recovery path short of DB surgery. Server: - DELETE /admin/api/cms/plugins/:id?force=true skips lifecycle hooks and proceeds with full teardown. Capability + step-up gating unchanged (plugins.install + step-up); the audit event records forced: true. - Normal uninstall now runs deactivate (when active) before uninstall, matching the documented "(if active) deactivate -> uninstall" contract. - Hook failures return a 400 that names the failed hook and points at the force-remove escape hatch. - All removal variants (normal, forced, corrupt manifest) converge on one teardown (removePluginCompletely): worker unload, canvas module pack deactivation, DB row delete, crash counter + crash events + schedule run history sweep (the latter two have no FK and used to outlive the row), and removal of the whole uploads/plugins/<id>/ tree - including stale version dirs left behind by interrupted upgrades (removeAllPluginAssets replaces the per-version removePluginAssets, assertPathWithin retained). Admin UI: - A hook-failed uninstall surfaces as a role="alert" with a "Remove anyway" action; forcing is gated by its own confirmation dialog warning that the plugin's cleanup code is skipped and external resources may remain. - PluginRemoveDialog gains a force variant; pendingRemove carries the mode. Docs/tests: - plugin-system.md documents force-uninstall; the lifecycle contract line now matches the code. - Server tests: deactivate-before-uninstall ordering, hook-failure 400 with escape-hatch message, force teardown (row + crash events + stale version dirs), force removal with a missing entry file. - UI test: failed uninstall -> Remove anyway -> confirm -> DELETE ?force=true. - plugin-boot-resilience architecture gate updated for the unified teardown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
DavidBabinec
force-pushed
the
feat/force-uninstall
branch
from
9bbf632 to
0092b6c
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
A throwing (or unloadable)
uninstallhook permanently blocked plugin removal:server/handlers/cms/plugins/state.tsreturned 400 before deleting the DB row, assets, or worker, with no recovery path short of DB surgery. A missing/corrupt entry file failed the same way. Related hygiene gaps:clearPluginCrashesclaimed to run "on uninstall" but never did,plugin_crash_events/plugin_schedule_runsrows outlived uninstall (no FK), and interrupted upgrades left stale version dirs underuploads/plugins/<id>/forever.What changed
Server (
server/handlers/cms/plugins/state.ts,shared.ts,index.ts)DELETE /admin/api/cms/plugins/:id?force=trueskips lifecycle hooks and proceeds with full teardown. Capability + step-up gating identical to normal uninstall (plugins.install+ step-up); audit event carriesforced: true.deactivate(when active) beforeuninstall— code now matches the documented "(if active) deactivate → uninstall" contract.error.removePluginCompletely): worker unload, canvas module pack deactivation, DB row delete (records/schedules cascade), crash counter + crash events + schedule run history sweep, and removal of the wholeuploads/plugins/<id>/tree including stale version dirs (removeAllPluginAssets,assertPathWithinguard retained). The old duplicate corrupt-manifest delete path is gone.Admin UI (
src/admin/pages/plugins/)role="alert"with the server message and a "Remove anyway" action; forcing is gated by its own confirmation dialog (shared<Dialog>primitive, no native dialogs) warning that the plugin's cleanup code is skipped and external resources (webhooks, third-party registrations) may remain.PluginRemoveDialoggains aforcevariant;removeCmsPlugin(pluginId, force)insrc/core/persistence/cmsPlugins.ts.Docs —
docs/features/plugin-system.mdgains a "Force-uninstall" section; the lifecycle contract line is now true.Tests
src/__tests__/server/cmsPlugins.test.ts: deactivate runs before uninstall (ordered markers); hook failure → 400 with escape-hatch message and plugin still installed;?force=trueremoves row + crash events + entire asset tree (stale0.9.0dir included) despite a throwing hook; force removal works when the entry file is missing from disk.src/__tests__/plugins/pluginsAdmin.test.tsx: failed uninstall → "Remove anyway" → confirm dialog →DELETE ?force=true.src/__tests__/architecture/plugin-boot-resilience.test.ts: gate updated for the unified teardown (drifted rule fixed in the same change per CLAUDE.md).Verification
bun test— 4912 pass / 0 failbun run build— clean (tsc -b && vite build)bun run lint— clean🤖 Generated with Claude Code