Repository navigation
Conversation
4e14543 to
39bf9cb
Compare
kvnloo
left a comment
There was a problem hiding this comment.
There is still an uncovered path from the issue: this only bounds the handoff when a background discovery thread already exists.
With this head:
def discover_plugins(force=False):
if not _join_background_discovery():
...
return
get_plugin_manager().discover_and_load(force=force)If no background thread was started, _join_background_discovery() returns True immediately and the caller still enters discover_and_load() synchronously, with no wall-clock bound on the discovery body.
That matters for the reported ACP path. Current model_tools.py still imports and calls discover_plugins() as a module-level side effect; its comment notes ACP session init runs the import under asyncio.to_thread, which protects the event loop but does not make session/new complete. I do not see ACP pre-arming start_background_plugin_discovery() the way hermes_cli.main::_prepare_agent_startup does for ordinary CLI launches.
So the regression added here proves:
synchronous caller does not re-enter the manager lock after an already-running background scan exceeds the join bound.
It does not yet prove the issue's stronger invariant:
a first discovery triggered by ACP / another direct caller cannot remain blocked indefinitely inside a plugin's discovery/register path.
Suggested additional regression before claiming #108139 fixed:
- reset manager +
_background_discovery_thread=None; - make the actual discovery body stall;
- call
discover_plugins()through the direct/first-call shape; - assert the public call returns under the intended bound and the stalled work remains isolated/backgrounded (or explicitly narrow the PR/issue claim to background-handoff only).
If the intended architecture is instead “every agent-capable surface must call start_background_plugin_discovery() before any possible discover_plugins()”, enforce and test that invariant for ACP/gateway/cron rather than relying on the CLI launcher's ordering.
|
Thanks @KoNit-K — closing as superseded by #118915 (merge SHA Main already carried the 30 s bound on the join onto an in-flight background discovery ( |
What does this PR do?
Related Issue
Fixes #108139
Type of Change
Changes Made
CONTRIBUTING.md— production or test change in this diffAGENTS.md— production or test change in this diffHow to Test
scripts/run_tests.shon the files in Changes Made — focused verificationscripts/run_tests.sh tests/hermes_cli/test_plugins.py -k test_discover_plugins_returns_when_background_discovery_exceeds_bound -v - failed as expected because discover_plugins() re-entered the locked discovery path - GREEN: HERMES_PYTHON=/Users/blockkonit./Dev/hermes/hermes-agent/.venv/bin/python scripts/run_tests.sh tests/hermes_cli/test_plugins.py -v - 78 passed, 0 failed - git diff --check - clean - Developer self-review: P0=0 - Independent review not triggered: 2 files, 91 diff lines, no auth/crypto/state.db changes - Residual behavior is intentionally fail-open: after the timeout, a caller may temporarily observe partial plugin state while the daemon discovery thread completes. A force reload requested during that interval must be retried after completion.Evidence
scripts/run_tests.shon the files in Changes Made — focused verification — 78 passedChecklist
Code
Documentation & Housekeeping
cli-config.yaml.example: N/ACONTRIBUTING.mdorAGENTS.md: N/A