fix(plugins): clear a plugin's hook error when its worker generation starts - #1024
Merged
Conversation
…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.
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
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.
The problem
The last hook-handler error a sandboxed worker reported is surfaced on
GET /api/plugins/{id}/healthas operator context. The field's own comment scoped it to one worker generation:
It was cleared in exactly one place —
disablePlugin. But a worker crash goes throughonWorkerExit, and a failed enable goes through the catch that terminates the host. Neither runs adisable. Both drop the host without clearing the record.
So the next enable installs a new host,
checkPluginHealthfinds the record again, and reports the deadgeneration'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 (
checkPluginHealthreturns early otherwise), which is whythis 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
onWorkerExitalone would have missed the enable-failurepath, 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:
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:
onWorkerExitalreadyperforms five — log-drop flush, search-provider unregister,
setPluginStatus(ERROR),hookManager.unregisterPlugin, andsandboxHosts.delete.lastSandboxHookErrorwas the only stepgenuinely missing, so this PR fixes that alone rather than restructuring the release paths.
Noted but deliberately untouched: the lead comment on
onWorkerExitstill 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 --noEmitover the full project including specs, build, 4012 unittests, 137 e2e tests, and the dashboard build all pass on Node 22.