fix(v2): make ModeRegistry.get_handlers() thread-safe with double-checked locking - #2425
fix(v2): make ModeRegistry.get_handlers() thread-safe with double-checked locking#2425nexiouscaliver wants to merge 2 commits into
Conversation
…cked locking Concurrent first-access to a lazy-loaded mode handler caused a race condition where the winning thread popped the lazy loader entry from _lazy_loaders while other threads were still in flight. Those losing threads found the key absent from both _handlers and _lazy_loaders and raised KeyError permanently — affecting ~49/50 threads in the first concurrent batch per (provider, mode). Root cause: the check → pop → import → set sequence in get_handlers() operated on shared dicts with no synchronization. Fix: added threading.Lock to ModeRegistry with double-checked locking pattern: - Fast path: unsynchronized _handlers dict read (safe under GIL for reads) - Slow path: _lock protects the entire pop → loader() → set sequence - register() and register_lazy() also synchronized to prevent concurrent mutation during lazy resolution Includes regression tests that reproduce the original 49/50 failure rate and verify the fix achieves 0/50. Closes 567-labs#2422
|
Hey @jxnl @ivanleomk — this fixes the thread-safety race condition in TL;DR: The |
- Remove unused 'sys' import - Prefix unused function args with underscore + noqa: ARG001 - Apply ruff format All 5 tests pass.
|
Friendly nudge! Just pushed a cleanup commit fixing ruff lint/format in the test file. CI workflows need maintainer approval to run on fork PRs — could someone approve? 5/5 tests pass, ruff clean. @jxnl @ivanleomk |
|
Superseded by #2495, which uses a lazy-load-specific lock and avoids coupling registration to loader resolution. |
Summary
Fixes the thread-safety race condition in
ModeRegistry.get_handlers()reported in #2422.Concurrent first-access to a lazy-loaded
(provider, mode)pair caused 49/50 threads to fail withKeyError/RegistryErrorbecause the winning thread popped the lazy loader entry from_lazy_loadersbefore the others could use it. Losing threads found the key absent from both_handlersand_lazy_loaderswith no retry path.Root Cause
The check → pop → import → set sequence in
get_handlers()operated on shared module-level dicts with no synchronization:Fix
Added
threading.LocktoModeRegistrywith double-checked locking:_handlersdict read — safe under the GIL for lookups, and once a handler is cached it is never removed_lockprotects the entire pop →loader()→ set sequence so concurrent callers serialize on the same in-flight loadregister()andregister_lazy()since they mutate the same shared dictsVerification
New regression test in
tests/core/test_registry_thread_safety.py:test_concurrent_first_access_no_failures: 50 threads viaThreadPoolExecutor(max_workers=10)hit the same lazy-loaded mode simultaneously. Before fix: 49/50 fail. After fix: 0/50 fail.test_loader_called_exactly_once: Confirms the lock serializes lazy loading — loader is invoked exactly once even under heavy concurrency.test_double_checked_locking_resolves_quickly: Verifies fast path after first resolution.test_unregistered_mode_still_raises: EnsuresKeyErroris still raised for genuinely unregistered modes.test_concurrent_different_modes: Concurrent access to different keys does not interfere.Files Changed
instructor/v2/core/registry.py— addedthreading.Lock, double-checked locking inget_handlers(), synchronizedregister()andregister_lazy()tests/core/test_registry_thread_safety.py— new regression testsCHANGELOG.md— entry under[Unreleased]Closes #2422