Lazily hydrate persisted conversations#4100
Conversation
Python API breakage checks — ✅ PASSEDResult: ✅ PASSED |
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Coverage Report •
|
||||||||||||||||||||
all-hands-bot
left a comment
all-hands-bot
left a comment
There was a problem hiding this comment.
✅ QA Report: PASS
Lazy persisted-conversation loading works as described in a real backend workflow: idle conversations stay unloaded after restart while search/count still work, runtime access hydrates on demand, concurrent access reuses one runtime, and duplicate fork IDs are rejected without overwriting persisted metadata.
Does this PR achieve its stated goal?
Yes. I created persisted conversations, restarted the conversation service, and compared main versus this PR using the same workflow. On main, restart eagerly loaded all 3 persisted conversations into live runtimes; on this PR, restart loaded 0 live runtimes but kept 3 catalog entries, search/count returned all 3 conversations without hydration, and get_event_service() hydrated only the requested conversation. Concurrent hydration returned the same service object for all callers, and a fork using an existing unloaded conversation ID failed with ValueError while preserving the target metadata.
| Phase | Result |
|---|---|
| Environment Setup | ✅ make build completed successfully; no tests/linters run locally. |
| CI Status | ⏳ 19 successful, 9 pending, 1 skipped, 0 failing when checked. |
| Functional Verification | ✅ Backend persisted-conversation workflow passed before/after verification. |
Functional Verification
Test 1: Startup no longer eagerly hydrates idle persisted conversations
Step 1 — Establish baseline on main:
Ran rm -rf /tmp/qa-lazy-data-main && cd /tmp/qa-sdk-main && OPENHANDS_SUPPRESS_BANNER=1 uv run python /tmp/qa_lazy_conversation_loading.py /tmp/qa-lazy-data-main.
Relevant output:
{
"count": 3,
"initial_live_runtimes_after_restart": 3,
"live_runtimes_after_search_and_count": 3,
"single_hydration_returned_service": true,
"live_runtimes_after_single_hydration": 3
}This confirms the old behavior: simply restarting with 3 idle persisted conversations immediately created 3 live runtimes before any runtime operation.
Step 2 — Apply the PR's changes:
Checked out PR branch codex/simplify-lazy-conversation-loading at bf0c3d3f70a39c1fe2c15ec2ecd5852260b9ce48.
Step 3 — Re-run with the fix in place:
Ran rm -rf /tmp/qa-lazy-data-pr && uv run python /tmp/qa_lazy_conversation_loading.py /tmp/qa-lazy-data-pr.
Relevant output:
{
"catalog_entries_after_restart": 3,
"count": 3,
"initial_live_runtimes_after_restart": 0,
"live_runtimes_after_search_and_count": 0,
"single_hydration_returned_service": true,
"live_runtimes_after_single_hydration": 1
}This shows the PR behavior: restart builds a lightweight catalog, search/count still find all conversations, and only the requested conversation hydrates when runtime access is needed.
Test 2: Concurrent hydration and duplicate fork protection
Step 1 — Establish baseline on main:
The same baseline run returned:
{
"concurrent_hydration_returned_services": [true, true, true, true, true],
"unique_hydrated_service_objects": 1,
"live_runtimes_after_concurrent_hydration": 3,
"fork_duplicate_result": "ValueError: Conversation with id c3802f2a-8831-4cef-a4cc-710b769f2105 already exists",
"fork_target_meta_unchanged": true
}Because main had already loaded every conversation at startup, concurrent access reused the existing runtime and duplicate fork IDs were rejected against loaded state.
Step 2 — Re-run with the PR in place:
The PR run returned:
{
"concurrent_hydration_returned_services": [true, true, true, true, true],
"unique_hydrated_service_objects": 1,
"live_runtimes_after_concurrent_hydration": 2,
"fork_duplicate_result": "ValueError: Conversation with id b91c5ed0-be2d-4e8a-b693-0b5fa01f83c6 already exists",
"fork_target_meta_unchanged": true
}This confirms concurrent requests did not create duplicate runtimes under lazy hydration, and duplicate fork protection also works for catalog-only persisted conversations without clobbering the existing meta.json.
Issues Found
None.
This review was created by an AI agent (OpenHands) on behalf of the user.
|
✅ PR Artifacts Cleaned Up The |
Co-authored-by: openhands <openhands@all-hands.dev>
Co-authored-by: openhands <openhands@all-hands.dev>
|
@OpenHands Check this PR, see its temporary |
|
I'm on it! enyst can track my progress at all-hands.dev |
There was a problem hiding this comment.
This review was generated by an OpenHands AI agent on behalf of @enyst.
🔴 Taste Rating: Needs improvement
The catalog/runtime split is a good simplification and the live evidence supports the startup-memory win, but lifecycle synchronization is incomplete. I left three important inline findings, each reproduced deterministically against the current head:
- status-filtered search/count can disagree with fresh persisted state in shared-storage deployments;
- delete can report success while a waiting request resurrects the runtime and catalog entry;
- shutdown can finish while in-flight hydration publishes an open, unclosed runtime afterward.
Focused regression suites still pass (90 passed), so the existing tests do not cover these interleavings. The required Validate PR description check is also failing: the human author needs to restore the HUMAN: / AGENT: template structure and provide the human-written note; an AI agent must not fill that field.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🔴 HIGH
This changes core persisted-runtime ownership and teardown behavior. The performance approach is sound, but the confirmed races can leak runtimes or make a deleted conversation reappear. Recommendation: do not auto-merge; have a human maintainer validate the lifecycle-locking fix and shared-storage semantics.
VERDICT:
❌ Needs rework: make hydration, removal, and shutdown share one lifecycle synchronization boundary, and refresh unloaded status from base_state.json when filtering.
KEY INSIGHT:
The hydration lock must protect runtime publication against every competing lifecycle transition, not only duplicate hydration.
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.- Re-request a review; the reviewer reads guidelines from the PR branch.
- When merged, the guideline goes through normal maintainer review.
Resolve with AI? Install the iterate skill and run
/iterateto drive the PR through CI, review, and QA.Was this review helpful? React with 👍 or 👎.
|
@OpenHands I’m curious about some parts of the code in this PR: is it all necessary, is it introducing redundancies, does it follow the patterns in the codebase, or, can we think about achieving its goal simpler? Examples to look into:
Do not modify the PR. Just answer me here on github. Intro yourself first so that readers know who is speaking (you are OpenHands-GPT-5.6) |
|
I'm on it! enyst can track my progress at all-hands.dev |
Co-authored-by: openhands <openhands@all-hands.dev>
|
I’m OpenHands-GPT-5.6, commenting through the Short version: the lazy catalog/runtime split is the right design, but the implementation carries compatibility code for tests that mutate private state. I would remove that compatibility rather than preserve it in production. 1. “Direct embedders and existing test fixtures” — ❌ Test-only compatibilityIn this repository, I searched non-test callers. The server constructs I also replaced
That is strong evidence that reconciliation repairs an invalid fixture rather than a real lifecycle gap. Since The missing-record fallback in 2. Is
|
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Thank you for this! We definitely shouldn’t load entire conversations just for a few details like knowing how many they are or their titles. It looks like this gives the largest gain in performance.
My agent verified a few potential nits, and it seems that 2 of them are happening (which come down to the same root cause I think). I think maybe they are opportunities for simpler code: #4100 (comment)
|
@OpenHands pick the latest nits from here #4100 (comment) and open a new PR from main (we merged this one) as a follow up to address the simplification suggested there |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
Opened follow-up PR #4122 from current It removes read-time catalog reconciliation/repair, makes test fixtures establish the catalog/runtime invariant explicitly, and deletes unloaded conversations without hydrating event history while respecting leases. Focused conversation/lease tests pass (98), and adjacent router/event/plugin/restore regressions pass (262). This comment was created by an AI agent (OpenHands) on behalf of the requester. |
|
Opened draft follow-up PR: #4122 Implemented the requested simplification:
Validation:
Commit: I also replied on merged PR #4100 with the follow-up link. The socket getter test failure noted in the PR reproduces on clean merged main and is unrelated to these changes. |
4 participants
HUMAN:
I (@neubig) have tested this and it is working quite well, much faster conversation load times and lower memory usage.
AGENT:
Why
Persisted conversation startup was hydrating every event history, which raised memory use and delayed readiness for large local corpora.
How to Test
Run the focused conversation-service and lease tests from the Validation section below. The Live acceptance testing section records a real Agent Server run against 120 persisted conversations and 55200 production-format event files.
Summary
EventServiceand event history only when an operation needs a live runtimeThis is a smaller replacement for #3895.
Part of #3140
Root cause
The agent server currently reconstructs every persisted conversation and its event history during startup. On the local corpus used to reproduce the watchdog failures, 136 conversations containing 55,312 event files raise the server to roughly 3 GiB RSS before any active work. A long-running npm test or coverage process can then exhaust the host's 7.8 GiB memory limit, causing an exit-137/OOM kill. The watchdog health failure is a downstream symptom of that memory pressure and process death.
With the catalog-only startup path, the same corpus uses roughly 355 MiB RSS before runtime hydration.
Impact
Idle historical conversations no longer consume event-history memory. Search and count remain available from the catalog, while runtime operations transparently hydrate the requested conversation. A single hydration lock keeps check-and-load atomic and bounds transient memory pressure.
Validation
uv run pytest -q tests/agent_server/test_conversation_service.py tests/cross/test_conversation_lease_behavior.py -x— 90 passedAgent 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:808bcbd-pythonRun
All tags pushed for this build
About Multi-Architecture Support
808bcbd-python) is a multi-arch manifest supporting both amd64 and arm64808bcbd-python-amd64) are also available if neededCloses #3140
Live evidence
On 2026-07-14 UTC I launched real
python -m openhands.agent_serverprocesses against a deterministic production-format corpus: 120 persisted conversations, 460 events each, 55,200 event JSON files total, corpus SHA-256c6a5867f0bb11bf9d64cd28ab296a657b8ae22bcd2035aeb6ccbb4b8099e64c4. The benchmark used fetchedorigin/main56ac31719f91059dc9b319dbc6bb79f17ef60cd7as the before ref and current PR head808bcbd5254ee9c7b65fae40147aaa8e8a66b2bbas the after ref. The harness used real HTTP endpoints plus productionowner_lease.jsonfiles and resume logs; no test-only endpoint or monkeypatch was used.Additional PR checks: duplicate fork to an existing unloaded conversation ID returned HTTP 409 with the target
meta.jsonunchanged. Startup with one persistedrunningrecord hydrated only that record, retained all 120 catalog records, and reported one recoverederrorrecord.