Skip to content

fix(plugins): serialize plugin loader import critical-section (partial-import race) - #412

Merged
Kyzcreig merged 1 commit into
mainfrom
fix/plugin-loader-import-race
Jul 22, 2026
Merged

Kyzcreig merged 1 commit into
mainfrom
fix/plugin-loader-import-race

Conversation

@Kyzcreig

Copy link
Copy Markdown
Collaborator

Fixes an intermittent, silent partial-import race in the three plugin loaders (context_engine, memory, cron_providers).

Symptom

Intermittent WARNING run_agent: Context engine "lcm" not found — falling back to built-in compressor (and the identical mem0 provider signature), always preceded by delegate_tool concurrency warnings. Live evidence: on a bpx session the built-in compressor (not LCM) emitted the summary that fired the CLI reseed; lcm_status showed compression_count: 0 because LCM was never the active engine that turn.

Root cause

Each loader does sys.modules[module_name] = mod before spec.loader.exec_module(mod). Child agents run concurrently in a shared-process DaemonThreadPoolExecutor (delegate_task, max_concurrent_children) and share one sys.modules. A second concurrent caller sees the module in sys.modules (the if module_name in sys.modules short-circuit), grabs the half-initialized shell — register() / the engine class not defined yet — finds no engine, and falls back. The memory loaders getattr(cached, "__file__", None) guard does NOT help: module_from_spec sets __file__ before exec_module runs.

Fix

Wrap each loaders import critical-section in a module-level threading.RLock. A concurrent caller now either waits for the full load or observes a fully-initialized module — never a half-exec shell. Minimal diff: inner body renamed to *_locked, thin locking wrapper keeps the public name. RLock (not Lock) tolerates any reentrant load during exec.

Test (behavior contract, not snapshot)

tests/context_engine/test_loader_import_race.py reproduces the race deterministically: a coordination Event is set from inside the plugins module-top-level (i.e. mid-exec_module, after sys.modules is populated), so the racing thread is guaranteed to enter the vulnerable window. Validated fail-before/pass-after — with the lock bypassed the test FAILS with the exact production symptom (loaded but no engine instance found, B gets None); with the lock active it passes. 430/430 green across the three plugin-loader suites (context_engine, memory-provider, cron scheduler-provider).

…e partial-import race

Context-engine, memory-provider, and cron-scheduler-provider loaders each set
sys.modules[name]=mod BEFORE spec.loader.exec_module(mod) runs. Child agents run
concurrently in a shared-process ThreadPoolExecutor (delegate_task,
max_concurrent_children) sharing one sys.modules, so a second caller could
observe the module registered but not yet executed, grab the half-initialized
shell (no register()/engine class defined yet), and silently fall back to the
built-in compressor / no memory provider / built-in scheduler.

Observed live as intermittent "Context engine lcm loaded but no engine instance
found -> falling back to built-in compressor" (+ identical mem0 signature),
always preceded by delegate_tool concurrency warnings.

Fix: wrap each loaders import critical-section in a module-level RLock. A
concurrent caller now either waits for the full load or sees a fully-initialized
module; never a half-exec shell. Behavior-contract test reproduces the race
deterministically (coordination Event set from inside exec_module) and gates it:
fails with lock bypassed (production symptom), passes with lock active.
@Kyzcreig
Kyzcreig merged commit 3174e25 into main Jul 22, 2026
36 checks passed
@Kyzcreig
Kyzcreig deleted the fix/plugin-loader-import-race branch July 22, 2026 04:41
@Kyzcreig
Kyzcreig restored the fix/plugin-loader-import-race branch September 21, 2026 10:32
Kyzcreig added a commit that referenced this pull request Sep 25, 2026
…25 rows (t_caabb1fa)

49/84 rows. LCM rows graded against stephenschoettler/hermes-lcm @ 8d1b1e6
(v1.0.0-rc.1) per D1a and reconciled with the lcm-upstream-divergence-ledger
(DD-1 -> #168 SUPERSEDED, DD-2 -> #107 SUPERSEDED, DC-1/DC-2/DC-3 KEEP).
Three generic fixes upstream still lacks by read: #412, #902, dc4245a -> UPSTREAM.
Route disclosure corrected in plugins.md header (judgment on claude-fable-5-1).
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