Repository navigation
perf(agent-server): index conversation execution status for search/count - #4201
Conversation
Status-filtered `search_conversations` and `count_conversations` called `get_state()` once per catalog entry, validating a full `ConversationState` (agent, LLM config, workspace, secret registry, stats) just to read one enum field -- and paid a separate thread hop for each one. Every filtered list or count call was therefore O(total conversations) in full state loads. Cache `execution_status` on `_ConversationRecord` alongside a (mtime_ns, size) signature of `base_state.json`, and refresh the whole catalog in one batched worker-thread pass. Live conversations answer from memory; persisted ones cost a `stat()` each and are only re-parsed -- for the `execution_status` scalar alone -- when the file actually changed. Full state is now loaded only for the conversations a page returns. Out-of-process status writes are still observed, since the signature check runs against the file on every query. At N=2000 conversations: filtered count 234ms -> 3ms, filtered page(20) 244ms -> 10ms. Benchmark and results in .pr/. Closes #3142
|
✅ PR Artifacts Cleaned Up The |
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
Address review: replace the try/except blocks in _state_signature and _read_execution_status_sync with contextlib.suppress. Condense docstrings and inline comments across the change.
Self-review turned up two defects in the previous commits. _load_catalog_sync had silently become lenient: routing the startup parse through _read_execution_status_sync meant a corrupt or unknown-status base_state.json left the conversation in the catalog as IDLE instead of dropping it and logging error_loading_conversation_catalog. Restore the strict inline parse and keep the lenient helper for the refresh path, where "keep the cached status, unverified" is the wanted behaviour. _refresh_execution_statuses could also stamp a stale disk status onto a conversation that went live during its worker-thread hop, since the record is rebuilt by _start_event_service while the read is in flight. Skip any conversation with an open event service when applying results.
Review catch: guarding the refresh's apply loop was not enough. Filtered search snapshotted _conversation_records before awaiting the refresh, so a conversation that went live during the hop had its record replaced while the filter still tested the superseded object -- search(RUNNING) returned nothing while the catalog record said RUNNING. Refresh first, then snapshot and filter the current catalog, then sort. Filtering before sorting is also cheaper and yields the same order, since the sort is stable and both orderings start from catalog order. Adds a deterministic regression test (no sleeps) that replaces a record during the refresh and asserts the filter still sees it.
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Status-filtered conversation count/search now behave correctly and are much faster under real persisted-conversation load.
Does this PR achieve its stated goal?
Yes. The PR set out to remove the expensive full-state scan from status-filtered conversation search/count while preserving correctness. I exercised the real ConversationService path before/after and the agent-server HTTP endpoints on the PR: at 1000 persisted conversations, filtered count dropped from 292.8 ms to 3.5 ms and filtered page search dropped from 322.7 ms to 18.5 ms, while an out-of-process base_state.json status change was picked up on the next query.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully. |
| CI Status | 🟡 Most checks are green; PR is still BLOCKED only because QA / PR artifacts / image build checks were still in progress when checked. |
| Functional Verification | ✅ Real service calls and HTTP requests confirmed the performance improvement and status correctness. |
Functional Verification
Test 1: Reproduce baseline status-filtered scan cost, then verify PR improvement
Step 1 — Reproduce / establish baseline without the fix:
Checked out origin/main at c248ec0a and ran a temporary harness that creates one real conversation with ConversationService.start_conversation(), clones its persisted on-disk layout to 100 and 1000 conversations, then calls count_conversations(execution_status=IDLE) and search_conversations(limit=20, execution_status=IDLE) with real persisted data.
BASE_COMMIT=c248ec0a
{
"sizes": [
[100, {"count_all": 100.0, "count_filtered_ms": 33.9300540000238, "page_filtered_ms": 42.19226299997558, "page_items": 20.0}],
[1000, {"count_all": 1000.0, "count_filtered_ms": 292.7906220000125, "page_filtered_ms": 322.6791700000149, "page_items": 20.0}]
]
}
This confirms the reported problem: status-filtered count/search become hundreds of milliseconds at 1000 persisted conversations.
Step 2 — Apply the PR's changes:
Checked out PR commit 19114eb1.
Step 3 — Re-run with the fix in place:
PR_COMMIT=19114eb1
{
"sizes": [
[100, {"count_all": 100.0, "count_filtered_ms": 0.47147200001518286, "page_filtered_ms": 12.877546999987999, "page_items": 20.0}],
[1000, {"count_all": 1000.0, "count_filtered_ms": 3.475533999989011, "page_filtered_ms": 18.53672499998993, "page_items": 20.0}]
]
}
This shows the fix works in the changed path: at 1000 conversations, filtered count improved by about 84x and filtered page search by about 17x in this environment.
Test 2: Verify status correctness after out-of-process persisted writes
The same harness changed one conversation's base_state.json from idle to finished while the service was running, then queried again.
Base/main mutation result:
{"before_finished": 0, "after_finished": 1, "after_idle": 999, "finished_page_ids": ["36f2b514-6307-4348-81db-a9b5661a218f"]}
PR mutation result:
{"before_finished": 0, "after_finished": 1, "after_idle": 999, "finished_page_ids": ["8b7c8ad9-7944-4684-a879-fde869923408"]}
This confirms the PR preserves the correctness property described in the PR: out-of-process status writes are observed on the next filtered count/search.
Test 3: Exercise the real agent-server HTTP API on the PR
Seeded 1000 persisted conversations, started the local agent server with OH_CONVERSATIONS_PATH=/tmp/qa-http-conversations, and called the OpenAPI-documented status filter parameter (status).
COUNT status=idle
1000 time=0.005852 http=200
COUNT status=running
0 time=0.005272 http=200
SEARCH limit=20&status=idle items=20 next_page_id=46ccefd232454e608085e971476899ea
SEARCH limit=20&status=running items=0 next_page_id=None
Then mutated one persisted base_state.json to finished without restarting the server:
COUNT status=finished
1 time=0.005800 http=200
COUNT status=idle
999 time=0.005093 http=200
FINISHED_SEARCH items=1 ids=['d43f20a5-0b11-4eb5-a0cd-4f9df13d9a27']
This verifies the user-facing HTTP endpoints return the expected filtered counts/pages and pick up persisted status changes.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
3 participants
HUMAN:
Addressing issue #3142
AGENT:
Why
Fixes #3142. Status-filtered conversation search and count were O(total conversations) in full state loads, not just in iteration.
_search_conversationsand_count_conversationsboth called_execution_status()once per catalog entry. For any conversation without a liveEventService, that ranConversationState.model_validate_json()overbase_state.json— validating the agent, LLM config, workspace, secret registry and stats blob — purely to reach one enum field, and paid a separateasyncio.to_threadhop per conversation on top.The unfiltered paths were already fixed by #4100 (the catalog is metadata-only), so this was the last remaining full scan: with 2000 conversations, one
GET /conversations?execution_status=...call blocked for ~244 ms.Summary
execution_statuson_ConversationRecordnext to a(st_mtime_ns, st_size)signature ofbase_state.json, and refresh the whole catalog in one batched worker-thread pass instead of one thread hop per conversation.stat()each and are only re-parsed — for theexecution_statusscalar alone, never a fullConversationState— when the file actually changed. Full state is loaded only for the conversations a page returns.TestConversationSearchScaling(5 tests) pinning both the scaling property and the correctness properties it must not trade away.Out-of-process status writes are still observed: the signature is re-checked against the file on every query, so another server instance finishing a conversation is picked up on the next call. The signature is captured before the content read, so a write racing the read leaves a signature that no longer matches and forces a re-read rather than caching a torn value.
Issue Number
Closes #3142
How to Test
1. Run the benchmark (builds a real conversation, clones its on-disk layout N times, times the read APIs behind
GET /conversationsandGET /conversations/count):2. Run the regression tests:
To confirm they actually catch the regression, check out
conversation_service.pyfrommainand re-run —test_filtered_search_loads_state_only_for_the_pageandtest_filtered_count_loads_no_stateboth fail.3. Full agent-server suite:
Video/Screenshots
End-to-end benchmark output, median of 5 calls after one warmup, macOS 15 (arm64, APFS SSD), Python 3.13.11. Full write-up in
.pr/RESULTS.md.Before —
main@c248ec0ae:After — this branch:
count(status)page(20, status)Test evidence. On
main, the two scaling tests fail:On this branch all four pass, and the full agent-server suite is green:
pre-commit runon the changed files passes (ruff format, ruff lint, pycodestyle, pyright, import rules, tool registration).Type
Notes
Why
page(20, status)doesn't go fully flat, and shouldn't. It still loads full state for the 20 conversations it actually returns — that's the ~5 ms floor visible in the unchangedpage(20)column. The remaining slope oncount(status)is thestat()pass that keeps the index honest against out-of-process writers, ~1.5 µs per conversation. Both are deliberate: dropping them would mean serving statuses this process cannot vouch for.Micro-optimisations worth flagging, since they look odd out of context — both are on a path that runs once per conversation per filtered query:
_state_signaturetakes astrand callsos.statdirectly rather thanPath.stat(). Measured over 1000 conversations:Pathconstruction +Path.stat()= 4.62 ms, cachedstr+os.stat= 1.26 ms. The path is memoised on the record via_base_state_path._refresh_persisted_statuses_syncis a@staticmethodtaking plain tuples rather than readingself._conversation_records, so the worker thread never touches the catalog the event loop may be mutating.Follow-up not taken here.
_search_conversationsstill sorts the whole catalog and linear-scans forpage_idon every call. Both are pure in-memory work (~0.5 ms at N=2000) and were not the bottleneck, so I left them alone to keep the diff reviewable. A genuinely sorted index would be the next step if conversation counts grow another order of magnitude.Review fixes since the first push (
0dbc78fa4,19114eb12):base_state.jsonland in the catalog as IDLE instead of being dropped and logged. Verified againstmain: catalog size 0 and twoerror_loading_conversation_catalogentries on both._refresh_execution_statusesno longer stamps a stale disk status onto a conversation that went live during its worker-thread hop, and filtered search now refreshes before it snapshots the catalog. Without the reorder,search(RUNNING)returned[]while the catalog record saidRUNNING; there is now a deterministic (sleep-free) regression test for it.Behaviour was checked with a differential harness: 37 conversations across 5 statuses, fixed UUIDs and timestamps, every sort order × 4 filters walked to the last page (16 walks, 88 pages, 240 items), plus on-disk mutation and a live in-memory status flip. Output is byte-identical to
main.Related. There is an existing draft, #3905, opened by an OpenHands agent against the same issue on branch
openhands/conversation-search-index. It predates #4100 and targets the state of the code before the catalog existed. This PR is an independent fix against currentmain; whichever lands, the other should be closed.Agent Server images for this PR
• 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:e17c390-pythonRun
All tags pushed for this build
About Multi-Architecture Support
e17c390-python) is a multi-arch manifest supporting both amd64 and arm64e17c390-python-amd64) are also available if needed