fix(agent-server): replace global _lifecycle_lock with per-conversation locks - #4570
Conversation
…on locks ConversationService guarded all conversation lifecycle operations with a single global asyncio.Lock (_lifecycle_lock). Two of those operations do slow/blocking work under the lock: - _get_or_load_event_service_locked: asyncio.to_thread(_prepare_persisted_runtime) - delete_conversation: await event_service.close() (can hang — see #4546) A stuck or slow operation on conversation A therefore blocked every other conversation — the entire server wedged while /health kept answering. Replace the global lock with per-conversation locks (_conversation_locks dict + _catalog_lock for dict mutation only). Each method acquires only its conversation's lock, so a stuck close() on conversation A blocks only conversation A; conversation B's create/search/open proceeds unimpeded. The three operations that genuinely touch ALL conversations keep the global lock: prepare_for_sandbox_pause, _evict_idle_conversations, __aexit__. This is the fundamental fix for issue #4569. PRs #4513 and #4548 become mitigations that can be closed as superseded once this lands.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
close() passed no exception to the portal context manager, so anyio took its graceful path -- portal.stop(cancel_remaining=False) -- and waited for in-flight tasks to finish on their own. It then joined the portal thread with no timeout. Either half can block the caller indefinitely. That matters because LocalConversation.close() releases tool executors in an unbounded loop, so one stuck portal task wedges conversation shutdown and every later operation that needs the conversation lock. Cancel remaining tasks on shutdown, and bound the wait for the portal thread. The portal thread is a daemon, so abandoning it with a warning is safe when it is stuck on work that ignores cancellation. Closes #4546 Co-authored-by: openhands <openhands@all-hands.dev>
|
👋 This PR needs a couple of things fixed before OpenHands can review it:
Push an update once this is addressed and this check re-runs automatically. This is an automated check - no AI was used to generate this comment. |
Stress test evidence — completely isolated agent-serverRan the repro against a completely isolated agent-server (separate ports :9000/:19000/:19001, separate settings dir, separate API key, separate automation DB) with the per-conversation lock fix applied (branch Phase 1 —
|
| Case | Without fix | With fix |
|---|---|---|
| A (cancellable task) | HANGS > 60s | 0.03s ✓ |
| B (uncancellable task) | hangs forever | 2.02s (timeout=2.0) ✓ |
| C (idempotent close) | n/a | 0.00s ✓ |
Phase 2 — HTTP concurrent read load (10 workers × 3 rounds)
round 1/3: 10/10 cycles ok in 0.16s
round 2/3: 10/10 cycles ok in 0.13s
round 3/3: 10/10 cycles ok in 0.13s
90 requests: 0 failures, p50=26ms, p99=96ms
✓ PASS — all requests succeeded, backend stayed responsive
How this eliminates the root cause
With per-conversation locks, a stuck close() on conversation A blocks only conversation A — conversation B's create/search/open proceeds unimpeded. The global _lifecycle_lock is retained only for operations that genuinely touch all conversations (prepare_for_sandbox_pause, _evict_idle_conversations, __aexit__).
Repro script: .pr/repro-async-executor-close-hang.py
Coverage Report •
|
||||||||||||||||||||||||||||||
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
🤖 OpenHands is reviewing this PR. Head commit: This comment was posted by an AI agent (OpenHands). |
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the shutdown review direction in c0e35e9 and updated the HUMAN test note. Please re-review this current SHA. This message was created by an AI agent (OpenHands) on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. This comment was updated by an AI agent (OpenHands) on behalf of the user. |
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🔴 Taste Rating: Needs improvement
The direction is right — replacing one global lock with per-conversation serialization addresses a real responsiveness problem. But the current split leaves global lifecycle operations and the new per-conversation paths uncoordinated, and the lock registry can grow without bound from arbitrary UUID lookups.
[CRITICAL ISSUES]
- Global lifecycle operations (
prepare_for_sandbox_pause, idle eviction, and service shutdown) can now race with per-conversation start/load/delete paths because those paths no longer acquire the global lifecycle gate. See inline comment.
[IMPROVEMENT OPPORTUNITIES]
- The per-conversation lock registry should not permanently allocate entries for every UUID lookup. See inline comment.
[TESTING GAPS]
- Please add regression coverage that exercises a global lifecycle operation racing with
start_conversation/lazy load, and a miss/delete path that proves the lock registry does not grow for random nonexistent IDs.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
This touches agent-server lifecycle synchronization. The intended hot-path responsiveness win is valuable and the current tests are passing, but the change affects shutdown/pause/eviction semantics and can leave orphaned live services or unbounded in-memory lock state under realistic concurrent access.
VERDICT:
❌ Needs rework: The per-conversation lock design needs a global lifecycle gate or equivalent coordination before this is safe to merge.
KEY INSIGHT:
Per-conversation locks solve head-of-line blocking only if all-conversation operations still exclude the per-conversation lifecycle paths they are trying to quiesce.
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 (e.g., "Security concerns about X do not apply here because Y"). 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.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed both lifecycle review findings in a5a0f09 with shared/exclusive gating, weak lock retention, miss fast paths, and regression tests. Ready for another look. This comment was generated by OpenHands on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
🤖 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.
🟡 Acceptable — correct implementation of a classic readers-writer lock pattern, with one minor housekeeping note.
Summary
This PR replaces a single global _lifecycle_lock with:
- A per-conversation
asyncio.Lock(stored in aWeakValueDictionary) for normal per-conversation ops - A readers-writer style coordination via
asyncio.Conditionfor exclusive ops (prepare_for_sandbox_pause,_evict_idle_conversations,__aexit__)
The logic is correct. The WeakValueDictionary approach is elegant — locks for conversations that exit scope get reclaimed without any explicit cleanup. Pre-checks before lock acquisition skip the machinery entirely for non-existent conversations. atomic_write_text is a genuine improvement over the raw open(..., 'w') it replaces.
[IMPROVEMENT OPPORTUNITIES]
See inline comments.
[TESTING GAPS]
The key correctness property of this change — that two different conversations can now proceed concurrently through their lifecycle — is tested implicitly by the stress test referenced in the PR description, but there is no focused unit test for it. A test that verifies two per-conversation tasks make independent progress while a third task for the same conversation blocks would make the invariant explicit and catch regressions if the locking logic is accidentally collapsed back. Not a blocker given the stress coverage, but worth tracking.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
Custom asyncio readers-writer lock replacing a singleasyncio.Lock. The implementation is correct and well-tested, but the increased complexity means future maintainers need to understand the interplay between_lifecycle_lock,_lifecycle_condition,_active_lifecycle_operations,_exclusive_lifecycle_pending, and per-conversation locks when modifying this area. Any change to the exclusion logic warrants a focused stress-test run.
VERDICT:
✅ Worth merging — real performance fix, implementation is sound, tests cover the critical scenarios.
KEY INSIGHT:
The WeakValueDictionary for per-conversation locks is the right data structure here: it gives automatic cleanup without needing a separate eviction pass, and the GC-based lifecycle matches the conversation lifecycle.
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 (e.g., "Security concerns about X do not apply here because Y"). 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.
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 replaces the single global _lifecycle_lock with a per-conversation locking scheme while retaining global coordination for all-conversation operations (sandbox pause, idle eviction, shutdown). I reviewed the locking design, the atomic local persistence change, and the test updates.
Analysis
Locking coordination is sound. The _conversation_lifecycle / _exclusive_lifecycle split uses an asyncio.Condition (_lifecycle_condition) with an _exclusive_lifecycle_pending flag and _active_lifecycle_operations counter. Per-conversation operations increment the counter (blocking exclusive operations from starting), and exclusive operations set the pending flag (blocking new per-conversation operations from starting) and wait for the counter to drain to zero. I verified there is no lock-ordering deadlock between _lifecycle_lock (now used only to serialize exclusive operations against each other) and the condition's internal lock, since per-conversation paths never acquire _lifecycle_lock.
WeakValueDictionary prevents unbounded lock growth. Per-conversation locks are stored in a WeakValueDictionary, so entries are GC'd when no caller holds a reference. The _catalog_lock_sync (threading.Lock) correctly serializes the get-or-create access. Early-return checks in _get_or_load_event_service and delete_conversation prevent creating locks for non-existent conversation IDs, and the new test_missing_conversations_do_not_accumulate_locks test validates this.
Early-return checks are TOCTOU-safe in practice. The unlocked membership check before acquiring the per-conversation lock introduces a benign race window: a conversation could be created or deleted between the check and lock acquisition. This is equivalent to the call arriving slightly earlier or later -- _get_or_load_event_service_locked re-checks inside the lock, so no stale state is observed. The previous global lock eliminated this window, but the tradeoff (avoiding lock allocation for misses) is reasonable and the behavior is equivalent.
Atomic text writes are a good improvement. Using atomic_write_text (temp file + os.replace) prevents concurrent state reads from observing partially written JSON. The 0o600 permissions are more restrictive than the previous default-umask writes, which is appropriate for conversation state files. The bytes branch remains non-atomic, which is consistent with the PR's stated scope (text persistence).
Tests adequately cover the new behavior. test_prepare_for_sandbox_pause_blocks_new_hydration verifies that an exclusive operation blocks per-conversation hydration, and test_waiting_hydration_cannot_restore_deleted_conversation was correctly updated to use the per-conversation lock instead of the global lock.
Risk Assessment
LOW -- The change touches agent-server lifecycle synchronization, which is inherently sensitive, but the condition-variable coordination is correct, the test coverage addresses the key race scenarios, and the previous review's concerns (global/per-conversation coordination, unbounded lock growth, missing test coverage) have all been addressed in this revision. No material issues found.
Verdict
No material findings. The PR is safe to merge.
Co-authored-by: openhands <openhands@all-hands.dev>
|
Addressed the final housekeeping suggestion in ed82a88 and resolved both follow-up threads. Ready for a final look. This comment was generated by OpenHands on behalf of the user. |
|
✅ 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.
🟡 Acceptable — the refactor is sound and meaningfully improves concurrency, but a couple of small things are worth tightening.
What this PR does well:
- The split is the right factoring: per-conversation operations (start/delete/get/fork/credential) get their own lock; only all-conversation operations (
prepare_for_sandbox_pause,_evict_idle_conversations,__aexit__) still take the global_exclusive_lifecyclegate. This is exactly the surgery the original cross-talk symptom called for. - The early-exit before
_conversation_lifecycleis acquired closes the lock-leak that the previous round flagged — no_conversation_locksentries are created for non-existent conversations, and deletes don't leave stale entries. - Switching
LocalFileStore.writetoatomic_write_textis a real correctness fix on its own: it stops readers from observing a partially-writtenmeta.jsonafter a crash mid-write, and theos.fchmod/os.replacesequence is the canonical pattern. - The new
test_prepare_for_sandbox_pause_blocks_new_hydrationtest covers the most important new invariant — exclusive ops must block new hydration until the existing services are drained.
Suggestions (none blocking):
_catalog_lock_sync: threading.Lockis suspicious in a single-event-loop asyncio service. IfEventServiceconsumers ever touch_conversation_locksfrom another thread, thisthreading.Lockwon't help anyway because theasyncio.Lockinstances it protects are bound to one loop. Either remove it (the dict ops are already serialized by the loop) or add a one-line comment documenting the threading assumption._lifecycle_lockis now strictly the mutex that serializes_exclusive_lifecyclecallers. Renaming it to_exclusive_lifecycle_lockwould make its single remaining purpose obvious at every call site.- No deterministic unit test that proves "per-conversation op on A doesn't block per-conversation op on B" — the stress test is timing-based and lives behind a
-m stressmarker. A 20-line async test that holds_conversation_lifecycle(A)and asserts a sibling_conversation_lifecycle(B)task completes during the hold would catch a regression of the core feature in milliseconds rather than seconds.
Verdict: Approve. The refactor solves a real problem, the prior round's blocking issues are all addressed, and the remaining items are style/clarity, not correctness.
Co-authored-by: openhands <openhands@all-hands.dev>
|
Final refinement is in f287969: redundant sync locking removed and focused cross-conversation concurrency coverage added. All existing threads are resolved and CI is green. Ready for final review. This comment was generated by OpenHands on behalf of the user. |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
|
🤖 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.
🟢 Good taste — all prior blocking concerns have been addressed; this is ready to merge.
What changed since the last review
The head commit (f287969) closes the three remaining suggestions from the previous round:
- Removed
_catalog_lock_sync: The synchronous that guarded in an asyncio-only context is gone. The get-or-create in is already serialised by the event loop, so no supplemental lock was needed. - Added the deterministic cross-conversation test: now directly proves the headline invariant — holding does not block , while a second waits. This is exactly the regression-catching test the PR needed.
- Retained existing naming: / names were kept with a justified rationale — the context managers and already make the two-regime model explicit at every call site.
Overall design check (fresh read on f287969)
The readers-writer pattern is correct:
- ****: increments while holding , then acquires the per-conversation weak-ref lock. The decrement in notifies only when the counter reaches zero, which is the exact condition the exclusive waiter needs — no spurious wakes, no missed wakes.
- ****: holds (serialises concurrent exclusive ops), sets to stop new shared ops from entering, drains in-flight shared ops via , then yields with the flag still set so nothing new slips in between drain and body execution. The block resets the flag and notifies, which is correct under exceptions and cancellation alike.
- ** for per-conversation locks**: correct choice — locks are kept alive by the statement for as long as they are in use, and collected automatically when no one holds them. The early-exit for unknown conversation IDs before ensures no lock is ever allocated for a miss.
- ** in **: the temp-in-same-dir + pattern is the canonical POSIX atomic write; using / before replace is correct. No regression relative to the old call for the service's intended JSON use case.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
The change narrows lock scope (correctness improvement) and adds atomic writes (correctness improvement). The concurrency invariants are well-tested with deterministic async unit tests. No public API changes. Risk of regression is low.
VERDICT:
✅ Worth merging — correct implementation, all prior issues resolved, real tests in place.
KEY INSIGHT:
The + early-exit combination elegantly solves both the lock-leak and bounded-memory requirements without any explicit cleanup path.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
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 replaces the single global _lifecycle_lock with a per-conversation locking scheme (_conversation_lifecycle / _exclusive_lifecycle) while retaining a global exclusive path for operations that span all conversations (sandbox pause, shutdown, eviction). It also makes LocalFileStore.write atomic for text content.
Risk Assessment: Low
The locking design is sound and well-tested. I traced the full lock-ordering and deadlock analysis:
- No deadlock risk.
_conversation_lifecycleonly touches_lifecycle_condition(and the per-conversation lock); it never acquires_lifecycle_lock._exclusive_lifecycleacquires_lifecycle_lock->_lifecycle_conditionin that order, then releases the condition before yielding. There is no lock-ordering cycle. - No exclusive starvation. Once
_exclusive_lifecycle_pendingis set, new_conversation_lifecyclecallers block on the condition before incrementing_active_lifecycle_operations, so the exclusive waiter is guaranteed to drain. - No conversation-lifecycle starvation. The exclusive path holds
_lifecycle_lockonly for its own duration; once it clears the pending flag and notifies, queued conversation lifecycles proceed. - Counter correctness.
_active_lifecycle_operationsis incremented before acquiring the per-conversation lock and decremented in afinallyafter releasing it, so the exclusive path correctly waits for in-flight operations (including those blocked on a per-conversation lock) to fully complete. Cancellation duringwait_forcannot leak the counter because the increment hasn't happened yet. - WeakValueDictionary is safe. Per-conversation locks are kept alive by strong references while held (the
async withstatement holds the lock object). Unreferenced locks are GC'd, preventing accumulation for transient/missing conversations -- verified bytest_missing_conversations_do_not_accumulate_locks. - atomic_write_text is a strict improvement. The old
open(path, "w")could leave partially written JSON on crash;os.replace-based atomic writes prevent partial reads. The 0o600 mode is consistent with the existing usage inacp_file_credentials.py.
Pre-existing (not a regression)
The unlocked pre-checks in _get_or_load_event_service and delete_conversation (checking _event_services / _conversation_records before acquiring any lock) introduce a benign TOCTOU window: a conversation could be created between the check and lock acquisition. In single-threaded asyncio this is harmless -- the worst case is a slightly stale "not found" response, and the creator has its own lock. This pattern existed before the PR.
fork_conversation releases the source's per-conversation lock before calling source_conversation.fork() (line 1930), leaving a window where the source could be deleted. This race also existed with the old global lock and is not a regression.
No material findings
No bugs, security issues, or design flaws found. The tests are focused and verify the key invariants: per-conversation serialization, exclusive blocking of hydration, non-accumulation of locks for missing conversations, and the deletion-under-hydration regression.
4 participants
HUMAN:
I tested focused conversation tests, SDK utility tests, pre-commit, and the concurrent stress regression.
AGENT:
This description was updated by an AI agent (OpenHands) on behalf of the user.
Why
Conversation lifecycle operations previously shared one global lock, so slow persistence or shutdown work for one conversation could block unrelated conversations.
Summary
Issue Number
#4569
How to Test
uv run pytest -q tests/agent_server/test_conversation_service.py::test_waiting_hydration_cannot_restore_deleted_conversation tests/agent_server/test_conversation_service.py::TestConversationTreeForkAndNavigate tests/agent_server/test_docker_build.py::test_build_with_telemetry_returns_parsed_buildkit_fields --no-covuv run pytest -q -m stress tests/agent_server/stress/test_concurrent_conversations.py::test_concurrent_conversations_isolated_and_fast --no-covuv run pre-commit run --files openhands-agent-server/openhands/agent_server/conversation_service.py openhands-sdk/openhands/sdk/io/local.py tests/agent_server/test_conversation_service.pyAll commands passed locally. The stress test completed successfully with 1 passed and 355 warnings.
Video/Screenshots
Not applicable: this is a backend concurrency and persistence fix.
Design Doc
Not applicable.
Type
Notes
The changes are limited to lifecycle locking, atomic local text writes, and the regression test synchronization.
🐳 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:f287969-pythonRun
All tags pushed for this build
About Multi-Architecture Support
f287969-python) is a multi-arch manifest supporting both amd64 and arm64f287969-python-amd64) are also available if needed