Repository navigation
Plugin loader: a hung import/register() is skipped after a short deadline instead of hanging startup (#108139) - #118915
Merged
Merged
Conversation
૮ >ﻌ< ა ci reviewran on 0cdbd29 — fix(plugins): per-plugin load deadline so a hung register() debug infoCI timingsCI timings · View report · View jobWall time 7m17s vs 5m59s (+21.7%). 6 job(s) slower, 6 faster, 1 unchanged.
|
… hangs startup A plugin whose import or register() never returns (an infinite loop, a blocking network call) held PluginManager.discover_and_load() forever, and with it every synchronous caller: `hermes chat`, gateway startup, ACP session/new (#108139). Each plugin's import + register() now runs under `plugins.load_timeout_seconds` (default 10, 0 disables, max 600) on a daemon worker. On overrun the plugin is recorded as failed with "load timed out after Ns" (same channel as every other load failure: startup WARNING, `/plugins`, `list_plugins()`), its pre-hang registrations are disposed, and discovery continues with the next plugin. The abandoned worker's later `ctx.register_*`/`subscribe`/`on_unload` calls are refused with a WARNING (the context is marked abandoned), so a late registration can never land in a registry the failure path already swept. Abandoned loaders are capped per process (8); past the cap further loads are refused with a named reason rather than run inline, which would recreate the hang (#98382 shape). Because the worker cannot own the caller's RLocks: the deferred-platform eager fallback now runs outside the replacement transaction, discovery re-entered from a loader worker returns on the already-set discovered flag instead of blocking on the sweep's lock, and such a worker never joins the background discovery thread that is waiting on it.
teknium1
force-pushed
the
register-deadline
branch
from
September 22, 2026 08:03
3b4302a to
0cdbd29
Compare
arkheioncorp
left a comment
There was a problem hiding this comment.
Review: Plugin loader deadline (#118915)
Verdict: APPROVE (with one observation)
This addresses #108139 — a hung register() no longer hangs startup. The implementation is robust.
Strengths
run_with_load_deadline: runs import+register on a daemon thread viacontextvars.copy_context().run(preserves the Hermes-home override ContextVar). On timeout, the worker is abandoned,ctx._abandon_load()is set, andPluginLoadTimeoutpropagates to the calling thread. Correct._ignore_after_abandoned_load: decorator that wraps everyregister_*,subscribe, andon_unloadmethod onPluginContext. Late registrations from the abandoned worker are logged and dropped. This is the critical safety net._reserve_abandoned_loader_slot: caps abandoned loaders at 8. When the cap is reached, further loads are refused (not run inline) — this is the right call, since at the cap the process already has several hung loaders and an inline load would recreate the original hang.in_plugin_load_worker(): used indiscover_and_loadto skip re-entrant discovery blocking, and in_join_background_discoveryto avoid joining the worker from itself. Correct.load_timeout_seconds: 0disables the deadline and runs register() inline — tested and documented.- Deferred platform fallback: if
_lease_deferred_platformfails,_load_pluginis called as an eager fallback. The eager load runs on a deadline worker, whose registrations need the coordinator lock — the comment explains this correctly.
Observation
_register_deferred_platformlost its@_serialized_replacementdecorator in the refactor;_lease_deferred_platformnow carries it. The fallback path calls_load_pluginoutside the serialized section. If two threads concurrently try to load the same deferred platform and both leases fail,_load_plugincould run twice. Verify that_load_plugin/_load_plugin_scopedhas its own synchronization (likely the_discovery_lockor an internal lock) that makes this safe. If not, the eager fallback could race.
Tests are thorough: timed-out plugin skips, late registration blocked, zero timeout runs inline, other plugins still load. No security concerns. Approved with the above observation noted.
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.
A plugin whose import or
register()never returns no longer hangs Hermes startup: each plugin's load runs under a short deadline, the offender alone is skipped with a named reason, and the rest keep loading.Fixes #108139 — thanks @Graemezee1 for the faulthandler-backed report. Supersedes #108144 by @KoNit-K: that PR bounded the join onto an in-flight background discovery (main already carries a 30 s bound there), while the hang lives in the load itself and also hits the first synchronous discovery, as @kvnloo's review noted; this PR bounds it at the source.
Changes
plugins.load_timeout_seconds(config.yaml, default10,0disables, max600) — deadline on one plugin's import +register()(hermes_cli/plugins_loader.py::run_with_load_deadline). The load runs on a daemon worker that inherits the caller's context (the Hermes-home ContextVar); on overrun the worker is abandoned and the plugin is recorded as failed withload timed out after 10s (import + register() never returned)— the sameLoadedPlugin.errorchannel every other load failure uses (startup WARNING,/plugins,list_plugins()), and discovery continues with the next plugin. Pre-hang registrations are disposed through the existing failure path (Plugin loader: version gate reads running code, SystemExit isolated, uv quarantine from any cwd, impostor dirs refused, range pins (#72052 #104404 #101962 #112096 #108371 #71650 #86992 #98407) #118841's SystemExit/Exception isolation is unchanged;KeyboardInterruptstill propagates).PluginContextis marked abandoned on timeout; everyctx.register_*/subscribe/on_unloadfrom the still-running worker is refused with a WARNING (called register_hook() after its load timed out; ignored), so nothing lands in a registry the failure path already swept.not loaded: 8 abandoned plugin loader thread(s) are still running … restart Hermes to retryrather than run inline (inline would recreate the hang).discover_and_load()re-entered from a loader worker (a plugin importingmodel_toolsdoes that) returns on the already-set_discoveredflag instead of blocking on the sweep's lock;_join_background_discoveryis a no-op from a loader worker (its parent is the thread it would join).hermes_cli/config_defaults.py,hermes_cli/web_server_config.py), docs inwebsite/docs/user-guide/features/plugins.md.Root cause in one sentence:
PluginManager._load_plugin_scopedcalled the plugin'sregister()synchronously on the discovering thread with no deadline, so one blocking plugin helddiscover_and_load()— and every synchronous caller (hermes chat, gateway boot, ACPsession/new) — forever.Validation
Live repro (fake home,
hangplugwithwhile True: passinregister()+ healthyokplug, both enabled):hermes chat -q hiorigin/maind855200timeout 40(exit 124), no outputWARNING … Failed to load plugin 'hangplug': load timed out after 10s (import + register() never returned)exactly 10.0 s after the load began,okplugloads,Plugin discovery complete: 60 found, 54 enabled, chat runs to completion (exit 0,Messages: 2)Cap E2E: 10 hanging plugins + 1 healthy at
load_timeout_seconds: 0.2→ discovery finishes in 1.7 s, 8 loaders abandoned, plugins 9–11 refused with the cap reason,list_plugins()reports every reason, 8 liveplugin-load:*threads (never more).Tests (
tests/hermes_cli/test_plugin_manifest_v2.py::TestLoadIsolation):test_register_overrunning_load_timeout_skips_only_that_plugin— red on base (hangs past the runner timeout), green here;test_load_timeout_zero_runs_register_inline—0keeps the inline path.scripts/run_tests.sh tests/hermes_cli tests/plugins: 15323 passed; 4test_dashboard_auth_gatereds are host-environmental (BACKEND_PORT_IN_USE port=9119, the live serve) and the onetest_relay_shared_metrics/test_cmd_updatered each pass in isolation (host load 27–48). ruff,check_no_tmp_literals,check-windows-footguns,check_compat_pointers,git diff --checkclean.Known limit (inherent to abandoning a thread): a plugin that hangs in a pure-Python busy loop keeps competing for the GIL after it is skipped, so the rest of startup runs slower than usual; a hang in blocking I/O (the reported MCP case) releases the GIL and costs nothing after the skip.
Infographic