Repository navigation
Conversation
…ation A server reads every meta.json once at startup. When it later loads a conversation (takeover from a dead owner, or a claim after the owner released it on shared storage) it built the EventService from that startup copy and saved it straight back, reverting the title, tags, stored secrets and updated_at the previous owner wrote in between. A resume now re-reads meta.json in EventService.start() right after the lease claim and before any write, so the catalog record, save_meta(), the autotitle check and the returned ConversationInfo all use what is on disk. The ChatGPT auth fallback a resume request supplies is passed as resume_secrets and layered on top, instead of being patched into the catalog record. New conversations and forks are unchanged. The F31.takeover-keeps-metadata map bullet loses its known-bug markers. Fixes #5629 Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014MKndVj7XWvRQmvW9w2bDg
1 of 11 tasks
Contributor
REST API breakage checks (OpenAPI) — ✅ PASSEDResult: ✅ PASSED |
Contributor
Coverage Report •
|
|||||||||||||||||||||||||
enyst
marked this pull request as ready for review
October 13, 2026 22:58
all-hands-bot
approved these changes
Oct 13, 2026
all-hands-bot
left a comment
all-hands-bot
left a comment
Collaborator
There was a problem hiding this comment.
This review was posted by an AI agent (OpenHands).
Verdict: correct, well-scoped fix
The change addresses a real shared-storage bug: a second agent server builds an EventService from the catalog copy of meta.json it read at its own startup and, on first load, saves that stale copy back. Adding reload_meta (set for every load that is not a new conversation or a fork) so start() re-reads meta.json after the lease claim and before any write is the minimal, targeted correction. The reload happens under the lease, so newer metadata from the previous owner is preserved rather than reverted, and a subsequent lease loss still fails closed through guarded_write.
What I verified against the current head
- Reload placement and failure path.
event_service.start()re-reads only whenmeta.jsonexists; a missing file keeps the catalog copy, and an unreadable file raises out ofload_meta()before any write, so the existing startup-failure cleanup closes the service and leaves the file intact.test_load_with_unreadable_meta_fails_without_overwriting_itexercises exactly that. - Downstream consumers use the re-read copy.
_start_event_servicenow branches autotitle onevent_service.stored(post-reload) and passes the same loaded copy into_maybe_subscribe_telemetry, and the catalog record / returnedConversationInfoare taken fromevent_service.stored. So title, tags,updated_at, and secrets written by the owner survive in bothmeta.jsonandGET. resume_secretscarries the ChatGPT fallback. The previous in-place mutation ofexisting_record.stored(and itsfinallyrestore) is replaced by layeringresume_secretson top of the re-read secrets insidestart(). This preserves the original effective behavior: on the resume path_scrub_persisted_credentialspops binding-managed secret names from the merged map regardless of source, so the fallback is accepted when unscrubbed and dropped when a credential binding owns it.test_load_layers_resume_secrets_over_reread_metacovers the layering.- Scope of
reload_meta. It isnot is_new_conversation, so create and fork paths (is_new_conversation=True) keep the caller-suppliedstoredand never re-read; only genuine loads re-read. No schema, REST, lease, or Docker-runtime change.
Checks
- Focused tests pass on the head:
tests/agent_server/test_conversation_service.py(134 passed, including the 3 new tests) andtests/agent_server/test_event_service.py(118 passed). pre-commit(ruff format/lint, pycodestyle, pyright, import-dependency and registration checks) passes on the three changed Python files.- Current-head CI is green on the reported check runs (
agent-server-tests,sdk-tests,coverage-report, and the REST/OpenAPI breakage check reported PASSED). - The PR supplies the requested live before/after evidence on two real
python -m openhands.agent_serverprocesses sharing oneOH_CONVERSATIONS_PATH, plus theF31.takeover-keeps-metadatamapxfail->xpass->passtransition, for both graceful-stop and SIGKILL takeover. The reproduction is local processes rather than a Kubernetes/NFS deployment, and the PR states that limitation; the shared-directory and lease mechanism is the same one the change relies on.
No material correctness, security, compatibility, or acceptance-criterion defect found. The linked issue (#5629) is open with ready-for-dev.
✅ APPROVED
enyst
commented
Oct 25, 2026
enyst
left a comment
enyst
left a comment
Member
Author
There was a problem hiding this comment.
To note, I'm not sure this should happen: have two servers running on the same conversations directory - or rather, if it does happen, it's not supported. It's possible that the agent saw the leasing mechanism and thought it's a supported use case; or that it has become a supported use case with docker runtimes; or that I'm wrong and misremember when I think the leasing addresses the same agent-server bad restart.
I'd prefer we don't merge this until we look at bit into it. The PR is not big, but the code design around meta.json is not always great... smells of unnecessary complexity.
…eps-metadata Co-authored-by: openhands <openhands@all-hands.dev>
Contributor
|
📁 PR Artifacts Notice This PR contains a |
enyst
marked this pull request as draft
October 26, 2026 12:56
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
HUMAN:
This PR proposes a fix for a bug uncovered during the first full agentic run of
verify-agent-server.AGENT:
Why
A second server could take over a conversation using metadata cached at its own startup, then overwrite newer title, tags, secrets and timestamps.
Summary
Reload metadata when resuming an existing conversation. Pass the ChatGPT fallback credential separately so it is applied after the reload rather than lost with the stale catalog snapshot.
Source: event_service.py · conversation_service.py.
Issue Number
Fixes #5629
How to Test
Previously recorded verification
Unit tests (new, in
tests/agent_server/test_conversation_service.py; the first two fail onmain):test_second_service_load_keeps_metadata_written_after_its_startup: a secondConversationServicestarted before the owner renames and retags; after the owner releases, the second one's load keeps title, tags andupdated_atinmeta.jsonandget_conversation.test_load_with_unreadable_meta_fails_without_overwriting_it: onmainthe load overwrites a corruptmeta.json; now it raises and leaves the file.test_load_layers_resume_secrets_over_reread_meta.uv run pytest tests/agent_server/test_conversation_service.py tests/agent_server/telemetry399 passed;tests/agent_serverotherwise green here excepttest_canvas_extension_backend.py::test_exited_leader_descendant_does_not_block_recovery[status], which fails the same way onmainin this sandbox.pre-commitpasses on the changed files.Live before/after, the issue's Steps to Reproduce on two real
python -m openhands.agent_serverprocesses sharing oneOH_CONVERSATIONS_PATH(script run against amaincheckout and this branch):Map proof (verify-agent-server, real processes via
launch --new --share-conversations-fromandrestart --hard):main(a430c77):control-agent-server map run --file F31 --only F31.takeover-keeps-metadata --fresh --repeat 2 --checkout <main checkout>reportsxfailin both attempts.--checkout:xpassin both attempts.control-agent-server map checkok, andmap run --file F31 --only F31.takeover-keeps-metadata --freshreportspass.The reproduction is local processes on Linux, not a Kubernetes/NFS deployment; the shared directory and lease file are the same mechanism.
Video/Screenshots
n/a
Type
Notes
A missing metadata file keeps the catalog fallback; an unreadable file fails startup and is left untouched. No metadata schema, REST shape, lease protocol or Docker refresh change. Creation-fixed runtime and credential-binding fields still come from the catalog. Independent comparison: #5745.
Visual explanation by Codex using HumanLayer show-me.
🐳 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-jdkpython-node-runtimepython-node-runtimepython-node-runtimegolang: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:0ccf60b-pythonRun
All tags pushed for this build
About Multi-Architecture Support
0ccf60b-python) is a multi-arch manifest supporting both amd64 and arm640ccf60b-python-amd64) are also available if neededJev-Fast-Audit
⚡ Jev fast audit · estimates · 0.43s · commit 86ddad7
Strongest signal: No primary concern selected.
Evidence: No primary concern to locate.
Coverage: complete supplied coverage; 14/14 hunks, 4/4 files.
All estimates and evidence