Skip to content

fix(plugins): clear a plugin's hook error when its worker generation starts - #1024

Merged
rmyndharis merged 1 commit into
mainfrom
fix/plugin-stale-hook-error
Jul 31, 2026
Merged

fix(plugins): clear a plugin's hook error when its worker generation starts#1024
rmyndharis merged 1 commit into
mainfrom
fix/plugin-stale-hook-error

Conversation

@rmyndharis

Copy link
Copy Markdown
Owner

The problem

The last hook-handler error a sandboxed worker reported is surfaced on GET /api/plugins/{id}/health
as operator context. The field's own comment scoped it to one worker generation:

Cleared on disable (fresh enable = fresh slate).

It was cleared in exactly one place — disablePlugin. But a worker crash goes through
onWorkerExit, and a failed enable goes through the catch that terminates the host. Neither runs a
disable. Both drop the host without clearing the record.

So the next enable installs a new host, checkPluginHealth finds the record again, and reports the dead
generation's error as current. An operator restarting a plugin to clear a fault saw the same fault
attributed to the healthy worker that replaced it.

The record is only read while a host exists (checkPluginHealth returns early otherwise), which is why
this is invisible while the plugin is down and only appears once it is brought back up.

The fix

One line, at the start of enableSandboxed.

The invariant is about a generation beginning, so it belongs where generations begin rather than
repeated on each way one can end. Patching onWorkerExit alone would have missed the enable-failure
path, which returns early on intentional=true.

Test

Drives the real enable path with a stubbed worker host: records an error through the hook shim, crashes
the worker (onWorkerExit(1, false)), re-enables, and asserts health carries nothing from before.

It fails without the fix with exactly the stale note:

Expected substring: not "last hook error"
Received string:        "last hook error in 'message:sent' at …: boom"

The sibling test for the disable path already existed and is unchanged, so both ways a generation ends
are now covered.

Scope correction

An earlier plan described this as a six-step release set assembled across three sites with "no single
site performing all six". Checked at source, that is no longer accurate: onWorkerExit already
performs five — log-drop flush, search-provider unregister, setPluginStatus(ERROR),
hookManager.unregisterPlugin, and sandboxHosts.delete. lastSandboxHookError was the only step
genuinely missing, so this PR fixes that alone rather than restructuring the release paths.

Noted but deliberately untouched: the lead comment on onWorkerExit still says broader crash cleanup
"is a pre-existing gap ... out of scope here", while the code below it already writes status and
unregisters hooks. That comment predates this change.

Verification

Backend lint, prettier --check, tsc --noEmit over the full project including specs, build, 4012 unit
tests, 137 e2e tests, and the dashboard build all pass on Node 22.

…starts

The last hook-handler error a sandboxed worker reported is surfaced on
GET /api/plugins/{id}/health as operator context, and the field's own comment
scoped it to one worker generation: "a fresh enable starts from a clean slate".

It was cleared in exactly one place — disablePlugin. A worker crash goes through
onWorkerExit, and a failed enable through the catch that terminates the host;
neither runs a disable. Both drop the host without clearing the record, so the
next enable installs a new host and checkPluginHealth reads the dead
generation's error and reports it as current. An operator restarting a plugin to
clear a fault saw the same fault attributed to the healthy worker that replaced
it.

Clearing it at the start of enableSandboxed fixes both paths at once, and any
future one: the invariant is about a generation BEGINNING, so it belongs where
generations begin rather than repeated on each way they can end. Patching
onWorkerExit alone would have left the enable-failure path, which returns early
on intentional=true.

The regression test drives the real enable path with a stubbed worker host,
records an error through the hook shim, crashes the worker, re-enables, and
asserts health carries nothing from before. It fails without the fix with
exactly the stale note in the message.

Noted but deliberately not touched: the lead comment on onWorkerExit still says
broader crash cleanup "is a pre-existing gap ... out of scope here", while the
code below it already writes status and unregisters hooks. That comment predates
this change.
@rmyndharis
rmyndharis merged commit 694502a into main Jul 31, 2026
15 checks passed
@rmyndharis
rmyndharis deleted the fix/plugin-stale-hook-error branch July 31, 2026 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant