test(agent-server): cover conversation reads not serializing behind an unrelated start - #4685
Conversation
…n unrelated start #4570 replaced the process-wide `_lifecycle_lock` with per-conversation lifecycle locks, but the tests it added exercise `_conversation_lifecycle` directly. Nothing drives the public API, so the guarantee callers actually depend on — a conversation-scoped read completing while another conversation is mid-start — is unguarded. Add that end-to-end regression test: wedge one conversation inside `_start_event_service` and assert `get_event_service` for a different, already-live conversation still resolves. The test times out against 611629e (the commit before #4570) and passes on main.
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
|
✅ Review complete. This review was performed through OpenHands Cloud Automation. You can log in and view the conversation here. |
Coverage Report •
|
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
🟢 Good taste — Clean, well-targeted regression test that closes the acceptance-criterion gap identified in #4675.
The test drives the public API (start_conversation + get_event_service) rather than poking the lock helper internals directly, so it will catch a regression regardless of how the locking is implemented — exactly what a regression test should do. CI is green on all relevant suites (agent-server-tests, sdk-tests, windows-tests, agent-server-stress-tests).
One very minor note worth considering (not blocking):
await starting in the finally block has no timeout. After release_start.set() the blocked coroutine should unblock quickly, but if start_conversation were to hang for an unrelated reason after the lock release, the test would run forever. Wrapping it in asyncio.wait_for(starting, timeout=5) would bound the worst case. Entirely optional — the current form is readable and consistent with surrounding tests.
No production code changes. No new dependencies. Draft → open when you're happy with it.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟢 LOW
Pure test addition, zero production code changes. The only risk is a flaky test, and the 5-secondasyncio.wait_forguards on both the wedge-entry and the read make that unlikely.
VERDICT:
✅ Worth merging — Core logic is sound, single optional improvement noted.
KEY INSIGHT:
The test correctly targets the public API layer rather than the lock implementation detail, so it remains valid even if the locking strategy changes again.
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.
This review was generated by an AI agent (OpenHands) on behalf of the user through OpenHands Automation. View conversation
…ion test Addresses review feedback: after release_start.set() the wedged start should unblock immediately, but an unrelated hang would otherwise run the test forever. Bound it like the other two waits in the same test.
3 participants
HUMAN:
Just adding a regression test.
AGENT:
Why
#4675 reports that
ConversationService._lifecycle_lockis a single process-wideasyncio.Lockheld on the read path of every conversation-scoped request, so anyrequest for conversation A queues behind a start/fork/delete/eviction for an
unrelated conversation B.
That code fix already landed in #4570 (
replace global _lifecycle_lock with per-conversation locks, merged 2026-08-23) — four days before #4675 was filed.The issue quotes
conversation_service.py:647and a_get_or_load_event_servicethat opens with
async with self._lifecycle_lock:; that is the pre-#4570 file.On
mainall nine lifecycle sites already go through_conversation_lifecycle(per-conversation) or
_exclusive_lifecycle(registry-wide sweeps only).What is not covered is the issue's acceptance criterion. The tests #4570 added
(
test_conversation_lifecycle_serializes_only_matching_ids,test_prepare_for_sandbox_pause_blocks_new_hydration) exercise the_conversation_lifecyclehelper and internals directly. Nothing drives the publicAPI, so the property callers actually depend on — a conversation-scoped read
completes while another conversation is mid-start — can regress without any test
going red. This PR closes that gap rather than re-fixing solved code.
Summary
test_conversation_read_completes_while_another_conversation_startstotests/agent_server/test_conversation_service.py: wedge conversation B inside_start_event_service, then assertget_event_service()for an already-liveconversation A still resolves.
start_conversation+get_event_service) instead of thelock helper, so it fails if the read path is ever put back behind a process-wide
lock — regardless of how the locking is implemented internally.
Issue Number
Fixes #4675
How to Test
The test is only meaningful if it fails without the fix, so it was verified in both
directions using two worktrees of this repo.
1 · It passes on
main(fix present):2 · It reproduces the bug on
611629e69— the commit immediately before #4570,i.e. the exact code #4675 quotes. The same test body was dropped into a worktree at
that commit:
The
TimeoutErroris exactly the reported behaviour:get_event_service(A)neverreturns while B sits inside
_start_event_serviceholding the process-wide lock.3 · Lint/type:
Video/Screenshots
Not applicable — no user-facing surface. The pass/fail transcripts above are the
evidence; the failing run against
611629e69is what a screenshot would show.Type
Test-only change. The bug fix itself shipped in #4570.
Notes
fixed by fix(agent-server): replace global _lifecycle_lock with per-conversation locks #4570, this test still stands on its own as the missing guard.
(file preview panel stops working during streaming). This PR does not diagnose
it either — if #16331 is still live on a build that contains fix(agent-server): replace global _lifecycle_lock with per-conversation locks #4570, the
process-wide lock is not its mechanism and that investigation should continue
separately.
main: registry-wide sweeps(
_evict_idle_conversations,prepare_for_sandbox_pause,__aexit__) take_exclusive_lifecycle(), which blocks new per-conversation work while it drains.That matches [Bug]: process-wide _lifecycle_lock serializes every conversation-scoped request behind start/fork/delete/eviction #4675's own proposed fix ("keep a process-wide lock only for
operations that genuinely mutate the service registry") and is not covered here.
🐳 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:9523b2e-pythonRun
All tags pushed for this build
About Multi-Architecture Support
9523b2e-python) is a multi-arch manifest supporting both amd64 and arm649523b2e-python-amd64) are also available if needed