test(agent-server): lifecycle lock deadlocks on thread-pool exhaustion - #4513
test(agent-server): lifecycle lock deadlocks on thread-pool exhaustion#4513neubig wants to merge 12 commits into
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
|
🚦 CI is currently failing on this PR's latest commit. Please fix the failing checks before OpenHands reviews it - this is re-checked automatically once you push a new commit. (A maintainer can also request This is an automated check - no AI was used to generate this comment. |
Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
The fix adds a fast-path cache check in _get_or_load_event_service before acquiring _lifecycle_lock, so already-cached conversations can be retrieved even when the lock is held by a slow asyncio.to_thread(_prepare_persisted_runtime) call for a different conversation.
Analysis
The fix is sound. The fast path:
event_services = self._event_services
if event_services is not None:
cached = event_services.get(conversation_id)
if cached is not None and cached.is_open():
cached.touch()
return cachedis safe because it contains no await points, making it atomic with respect to the asyncio event loop. No other coroutine (including the lock holder) can interleave between the dict read, the is_open() check, and touch(). The inline comment accurately documents this reasoning.
I verified all mutation sites of _event_services:
= {}in__aenter__— before the service is active, so no concurrent readers.= None/= {...}in__aexit__— under_lifecycle_lock.pop()indelete_conversationand_evict_idle_conversations— under_lifecycle_lock.[stored.id] = event_servicein_start_event_service— all callers (_get_or_load_event_service_locked,_start_conversation, fork) acquire_lifecycle_lockfirst.
The claim that _event_services is only mutated under the lock (with the __aenter__ exception) holds.
Non-blocking observation
The fix addresses the most impactful scenario (cached conversations blocked by a slow uncached load), but the root cause — asyncio.to_thread(_prepare_persisted_runtime) inside the lock in _get_or_load_event_service_locked — still means that loading an uncached conversation while the thread pool is exhausted will block all other uncached loads behind the lock. This is a known scope limitation acknowledged in the PR description and is acceptable for this fix. A future improvement could move the to_thread call outside the lock or use per-conversation locking, but that's a larger change.
Test
The test is well-structured: it reproduces the deadlock by exhausting a 1-worker thread pool, evicting one cached conversation to force a to_thread load, and asserting that a different cached conversation still loads within a timeout. The cleanup in the finally block is thorough.
Risk Assessment
Low risk. The change adds a read-only fast path that is a strict optimization — the locked path is unchanged and remains the fallback. No new mutations, no new await points, no behavioral change for uncached conversations.
Verdict
No material issues found. The fix is correct, minimal, and well-documented.
|
@/tmp/pr-evidence-revised-4513.md |
|
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
This review was created by an AI agent (OpenHands) on behalf of the repository maintainers.
Summary
This PR fixes a real production deadlock: _get_or_load_event_service holds _lifecycle_lock across asyncio.to_thread(_prepare_persisted_runtime), so thread-pool exhaustion wedges all event-service lookups — including cached conversations that need no thread work.
Fix Assessment: Sound
The unlocked fast-path cache check before _lifecycle_lock acquisition is safe in this codebase:
- Single-threaded event loop: CPython dict reads are atomic with respect to the event loop. No concurrent coroutine can mutate
_event_servicesduring a synchronous.get()—asyncio.to_threadoffloads_prepare_persisted_runtimebut does not touch_event_servicesdirectly. - Established pattern: Unlocked reads of
_event_servicesalready exist elsewhere (_renew_all_leases_loop,get_conversation,_start_conversation), so this is consistent. touch()/is_open()are safe outside the lock: both are simple synchronous attribute reads/writes with no I/O or lock dependencies.- Stale-snapshot fallback is correct: if the entry is evicted between the unlocked check and lock acquisition, the locked path re-checks and handles the miss.
Findings
.pr/repro-async-executor-close-hang.py — accidental commit from unrelated work
This script should be removed before merge. It has several problems:
- References a different issue/PR: The docstring cites PR #4548 / issue #4546, not #4513 / #4514 that this PR claims to fix. This looks like it was carried over from a separate debugging session.
- Hardcoded developer paths:
sys.path.insert(0, "/home/gneubig/work/software-agent-sdk/openhands-sdk")(line 47) — crashes or does nothing on any other machine. - Reads a local credential file at import time:
open("/home/gneubig/.openhands/agent-canvas/api-key.txt").read().strip()(line 268) is evaluated as a default argument value at module load. This will raiseFileNotFoundErroron any machine that lacks this exact path, making the script unrunnable. It also references a local credential file.
While .pr/ is auto-removed on PR approval, this artifact adds noise and appears unrelated to the actual fix. Recommend removing it.
Test: acceptable but brittle
The test directly manipulates private internals (service._event_services.pop(conv_a, None)) and calls dunder lifecycle methods (service.__aenter__() / service.__aexit__()) instead of using async with. This is understandable for controlling concurrency state in a deadlock repro, but makes the test fragile to internal refactors.
Risk Assessment: LOW
The core fix is correct, minimal, and consistent with existing patterns. The only actionable issue is removing the accidental repro script.
The repro script (.pr/repro-async-executor-close-hang.py) was accidentally committed from a separate debugging session (references PR #4548/issue #4546, not this PR's issue #4514). It fails pre-commit (import ordering, ARG001 unused arg) and has hardcoded developer paths and credential references. Deleting it resolves both the CI lint failures and the 3 review threads. Co-authored-by: openhands <openhands@all-hands.dev>
|
Re-requesting review after fix: removed unrelated repro script (commit 0608752). Pre-commit failures and all 3 review threads addressed. |
…reads Co-authored-by: openhands <openhands@all-hands.dev>
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Minimal, correct fix for a real production deadlock. The double-checked locking pattern (fast-path cache read before lock acquisition) is the right tool here, and the test exercises the real code path without mocks.
Two small observations inline; neither is blocking.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The fast-path read is atomic in asyncio (noawaitpoints between the dict snapshot andreturn cached), so no coroutine can interleave. All mutations to_event_serviceshappen under the lock, confirmed at call sites. The worst-case fast-path miss is a benign fallthrough to the locked path. The test exercises real code; timing is inherently best-effort for asyncio concurrency tests but adequate here.
VERDICT:
✅ Worth merging: The fix is minimal, mechanically sound, and properly regression-tested against the real deadlock scenario.
KEY INSIGHT:
The fast path is fully atomic (no await between snapshot and return), so the lock invariant it relies on only needs to hold for the locked slow path — which it does.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Improve this review? If any feedback above seems incorrect or irrelevant to this repository, you can teach the reviewer to do better:
- Add a
.agents/skills/custom-codereview-guide.mdfile to your branch (or edit it if one already exists) with the/codereviewtrigger and the context the reviewer is missing. See the customization docs for the required frontmatter format.- Re-request a review - the reviewer reads guidelines from the PR branch, so your changes take effect immediately.
- When your PR is merged, the guideline file goes through normal code review by repository maintainers.
Resolve with AI? Install the iterate skill in your agent and run
/iterateto automatically drive this PR through CI, review, and QA until it's merge-ready.Was this review helpful? React with 👍 or 👎 to give feedback.
…anup in finally) - Reword fast-path comment per bot suggestion: replace stale-snapshot language with accurate description of the fast-path miss scenario. - Declare stuck_task before try block, init to None, cancel in finally to prevent pending task warning if TimeoutError jumps to finally. Co-authored-by: openhands <openhands@all-hands.dev>
|
Re-requesting review after addressing both bot suggestions (commit 37099a1): (1) reworded fast-path comment, (2) added stuck_task cleanup in finally block. Both threads resolved. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Use Task[EventService | None] instead of Task[None] to match the return type of get_event_service. Add assert for type narrowing in the happy-path cleanup block. Co-authored-by: openhands <openhands@all-hands.dev>
|
Updates pushed at d1b25f5 — fixed pyright type errors in test file (stuck_task type annotation and type narrowing). Please re-review. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
Taste Rating: 🟢 Good taste
The fix is minimal and correct. The fast-path cache check before acquiring lifecycle_lock is the right approach: it eliminates the deadlock without changing the locked slow path's semantics. The inline comment correctly explains why the unlocked dict lookup is safe — insert/delete happen only under the lock, and a fast-path miss falls through to the locked path which re-checks.
The test exercises the real code path end-to-end (no mocks): it creates a real ConversationService, saturates the default executor pool with a 1-worker ThreadPoolExecutor, evicts one conversation from the cache, kicks off a background load that hangs inside asyncio.to_thread(_prepare_persisted_runtime), and then verifies that loading a different cached conversation completes immediately. That is exactly the production scenario the bug report describes.
Both follow-up commits address the previous review feedback cleanly: the stray repro script is gone, stuck_task is cleaned up in finally, the comment wording is tightened, and the stuck_task type annotation is now Task[EventService | None] to match get_event_service's return type. Nice iteration loop.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The unlocked fast-path reads _event_services (a dict) without holding lifecycle_lock, but the invariant — that _event_services is only mutated under the lock — is preserved by the existing code (all insert/delete sites go through the locked path). is_open() and touch() on the cached service are simple atomic operations. A fast-path miss correctly falls through to the locked path which re-checks the cache.
VERDICT
✅ Worth merging: Core logic is sound, the test is pragmatic and exercises real code paths, and all previous review feedback has been addressed.
KEY INSIGHT
Adding a lock-free read path before acquiring a hot lock is the correct pattern for eliminating convoy effects — the key invariant is that the unlocked state must be a self-consistent snapshot that the locked path will re-validate.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
|
Requesting changes: this is a mitigation, not a fundamental fix. The cache fast-path only helps the read path for cached conversations — The root cause is a single global lock for all conversation lifecycle operations. See the ready-for-dev issue #4569 for the fundamental solution (per-conversation locks). This PR can likely be closed as superseded once #4569 is implemented — the underlying scenario (a stuck operation on conversation A blocking conversation B) can no longer occur. |
|
Closing since it's not a fundamental fix. |
3 participants
HUMAN:
Demonstrates a production deadlock where the conversation lifecycle lock blocks all event loading when the thread pool is exhausted. Fix included in second commit.
AGENT:
Why
_get_or_load_event_serviceacquireslifecycle_lockand then callsasyncio.to_thread(_prepare_persisted_runtime)inside the lock. If the default thread pool is exhausted (all workers stuck on slow I/O), theto_threadcall queues indefinitely while still holding the lock. Every subsequentget_event_servicecall — including the WebSocket event-stream path and the REST/events/searchendpoint — blocks waiting for the lock, making the entire agent-server appear wedged even though simple endpoints (/ready,/api/settings) still respond.This caused a production incident where conversations opened but events never loaded after the server ran long enough for a blocking operation to tie up all thread-pool workers.
Summary
test_thread_pool_exhaustion_does_not_block_cached_conversation) that reproduces the deadlock by exhausting a 1-worker thread pool and asserting that a cached conversation can still be loaded.to_threadcall for a different conversation.REST API contract changes
Compared with base OpenAPI
b56221283f74for public/api/**paths.Issue Number
Fixes #4514.
How to Test
Before the fix (first commit only): fails with
TimeoutError— the cachedget_event_servicecall hangs because the lifecycle lock is held by the stuckto_threadcall.After the fix (both commits): passes — the cached conversation loads instantly because the cache is checked before acquiring the lock.
All existing tests pass:
uv run pytest tests/agent_server/test_conversation_service.py tests/agent_server/test_event_service.py -x --timeout=120 # 217 passedVideo/Screenshots
Test output before fix (first commit):
Test output after fix (both commits):
Type
🐳 Agent Server images for this PR — GHCR package, pull/run commands, and all pushed tags (click to expand)
• GHCR package: https://github.com/OpenHands/agent-sdk/pkgs/container/agent-server
Variants & Base Images
eclipse-temurin:17-jdknikolaik/python-nodejs:python3.13-nodejs22-slimgolang:1.21-bookwormPull (multi-arch manifest)
# Each variant is a multi-arch manifest supporting both amd64 and arm64 docker pull ghcr.io/openhands/agent-server:d1b25f5-pythonRun
All tags pushed for this build
About Multi-Architecture Support
d1b25f5-python) is a multi-arch manifest supporting both amd64 and arm64d1b25f5-python-amd64) are also available if needed