fix(agent-server): persist LLM/profile switch to meta.json so it survives restart - #4028
fix(agent-server): persist LLM/profile switch to meta.json so it survives restart#4028VascoSch92 wants to merge 11 commits into
Conversation
…ives restart switch_llm / switch_profile updated only the live ConversationState (base_state.json). On resume, EventService.start() rebuilds the agent from meta.json and ConversationState.create() overwrites base_state.json with it, so the switch — including the LLM timeout — reverted to the conversation's creation-time config on every agent-server restart. Re-switching the profile fixed it live but the fix was lost on the next restart. Mirror the switched agent into meta.json (+ save_meta), matching the existing switch_acp_model precedent, by routing both endpoints through new EventService.switch_llm / switch_profile methods.
Extract the mirror-into-meta.json logic into a single EventService helper, _sync_agent_to_meta(), used by switch_llm, switch_profile, and a new post-run hook. The post-run hook covers the third switch path — the agent's built-in switch_llm tool (SwitchLLMExecutor -> conversation.switch_profile), which bypasses the HTTP endpoints — so tool-initiated switches also survive a restart. An identity check against a start()-established baseline keeps the hook a no-op for ordinary runs.
…erged Addresses review feedback (trim comments) and a correctness gap: mirroring the whole live agent baked plugin-merged agent_context/mcp_config into meta.json, which would double-merge plugins on resume (_ensure_plugins_loaded merges once per process on top of the stored agent). Instead, surgically update the stored agent's llm/condenser only — the fields a switch changes — mirroring the switch_acp_model precedent. Detection is keyed on the LLM object identity (a swap replaces it; plugin loading replaces the agent but keeps the LLM), so ordinary and plugin-loading runs stay no-ops. Add a regression test that a plugin-style agent swap is not persisted.
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Are we sure this is the best code design? The tool firing is an event we could capture with a hook or maybe a new listener, rather than attempting to always save 🤔
Could we analyze the options a bit here?
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
Issue #4032 notes:
The agent should continue using the LLM profile timeout value configured before the restart (1234) or the one of the active profile.
I see from this PR that what happened is that the entire profile switch may have not been persisted as expanded LLM. That makes me wonder, could we maybe save the profile, rather than the expanded LLM? And load it on restore.
Alternatively, since that is quite involving, for the bug fix maybe we could read the active profile at restore time?
Please consider it just quick thoughts, open for discussion.
@enyst I did consider it. Two reasons I went with persisting the expanded LLM instead:
Make it sense, or? |
…stener Replace the post-run _sync_llm_to_meta hook with a listener on ConversationStateUpdateEvent(key="agent"), flushed at run teardown, so the in-run switch_llm tool's swap is persisted to meta.json as an event instead of polled after every run. The /switch_llm and /switch_profile endpoints keep their synchronous persist; the LLM-identity guard keeps plugin merges and ordinary runs no-ops.
|
Makes sense. It's a fix for the current expanded LLM config. Thanks! It also makes sense to deprecate the redundancy, but the name "expanded" sounds more like an expanded per-conversation config override. In this case, the live in-memory config (of a session that has never been restarted) should be:
And the configs should always be written to the location where the agent can load them again after a restart. |
|
and architectural changes are better handled in a separate PR |
|
I cherry-picked the PR and on my env the issue is still present, ie. agent-server resets the LLM timeout once OH reconnects. |
|
Just to note, for investigation of the best solution here:
Doesn’t the endpoint change the “active” profile? If not, I think maybe it should. The human-triggered switch does… 🤔 On a side note, we already have two persistence formats, that is why I think it’s worth thinking how to reduce them to one: profiles, as much as possible. Maybe the agent is confused here, though maybe it’s me. 😅 |
I tested again with latest version of OH:
So the PR solves the issue. |
|
@OpenHands describe what does the PR do, if the user has a conversation which is started, works, then later restored, but they never used any of switch llm profile during this conversation. reply directly on gh. |
|
I'm on it! enyst can track my progress at all-hands.dev |
|
If the conversation never switches LLM/profile, this PR does not change its behavior. At conversation creation, the expanded LLM config is stored in Therefore:
So this PR specifically makes an actual live LLM/profile switch durable; it does not change profile resolution for conversations that never switched. This reply was generated by an AI agent (OpenHands) on behalf of the user. |
|
Replied directly on GitHub: #4028 (comment) The response clarifies that the PR is a no-op for conversations that never switch LLM/profile: restoration continues using the creation-time expanded LLM snapshot from |
|
FYI, I depend on this PR (or a similar solution). |
|
@OpenHands solve the conflicts |
|
I'm on it! VascoSch92 can track my progress at all-hands.dev |
|
@enyst what do you think? |
Resolve conflict in tests/agent_server/test_conversation_service.py: keep both test_switch_llm_survives_restart (this PR) and test_startup_and_search_do_not_hydrate_idle_conversation (main). Update the restart assertion to explicitly hydrate via get_event_service now that persisted conversations are loaded lazily (OpenHands#4100). Co-authored-by: openhands <openhands@all-hands.dev>
|
Resolved the merge conflicts with The only conflict was in
One semantic adjustment was needed: main's #4100 ("Lazily hydrate persisted conversations") means idle conversations are no longer eagerly loaded on restart, so the restart assertion now explicitly hydrates via Verification:
The PR is now mergeable. This comment was created by an AI agent (OpenHands) on behalf of the user. |
Preserve both ACP credential binding activation and the LLM persistence baseline during EventService startup. Co-authored-by: openhands <openhands@all-hands.dev>
|
@enyst :-) |
Adds the QA harness used to check whether the LLM timeout really is lost across an agent-server restart, and under which conditions. - timeout_restart_evidence.py: end-to-end reproduction against a real agent-server process (SIGKILLed and restarted on the same persistence dirs), a real mock LLM and a real agent run. Reads the timeout three ways: cold, warm, and behaviourally by stalling the LLM and measuring when the agent gives up. - no_switch_repro.py: the three no-switch paths (plain agent, agent profile launch, profile edited after creation), in-process and offline. - mock_llm.py: OpenAI-compatible mock with a switchable stall mode. - RESULTS.md: the recorded runs. Findings: the revert reproduces only when a switch leaves the live LLM diverged from meta.json, which is what this PR fixes; without a switch the creation-time timeout already survives, including at the commit the issue was filed against. Also documents that a cold read right after a restart is served from base_state.json and still reports the pre-restart value, so manual verification has to touch the conversation first.
|
📁 PR Artifacts Notice This PR contains a |
…penHands#4032) Answers the question: profile timeout=600, made active, NEW conversation started from it, never switched — is it 600 or 300 after an agent-server restart? Independent of the existing harness: dumps both parsed paths and raw byte tokens from meta.json and base_state.json, then restarts the service over the same dir and reads the live agent timeout. Result on this branch AND on main: 600 -> 600. The active profile's LLM is copied into the agent at creation and written to both files; with no switch the two never diverge, so the meta.json rebuild reinstates the same 600. The 300 default only surfaces when a switch isn't mirrored into meta.json. Co-authored-by: smolpaws <engel@enyst.org>
enyst
left a comment
enyst
left a comment
There was a problem hiding this comment.
👋 I'm an AI agent (Opus 5) reviewing this PR on behalf of Engel Nyst (@enyst).
🔴 Needs improvement - the synchronization marker is committed before the durable state it is meant to represent.
[CRITICAL ISSUES]
- [
openhands-agent-server/openhands/agent_server/event_service.py, Line 1676] Persistence Correctness:_synced_llmadvances beforesave_meta()succeeds. The callback deliberately catches persistence errors, so one transient failed write leavesmeta.jsonstale while the in-memory marker says this exact LLM is already synchronized; every later agent update then returns at the identity guard and never retries. Update the marker only after a successful save. Keep the candidateStoredConversationlocal until persistence succeeds as well, so the marker, in-memory metadata, and disk commit advance together.
[TESTING GAPS]
- [
tests/agent_server/test_event_service.py] Missing Failure-Retry Proof: add a test where the firstsave_meta()fails and a later matching agent-state update retries and persists the switched LLM. The current exception tests do not prove recovery after a transient disk failure.
KEY INSIGHT:
A synchronization watermark must represent committed durable state, not an attempted write.
[RISK ASSESSMENT]
- [Overall PR]
⚠️ Risk Assessment: 🟡 MEDIUM
The change is localized, but it modifies restart persistence for live LLM/profile switching; a failed write currently defeats the PR's core guarantee until another distinct switch occurs.
VERDICT:
❌ Needs rework: make the durable write succeed before advancing the synchronization marker.
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.
Review Summary
This PR correctly identifies the root cause (resume rebuilds the agent from meta.json and ConversationState.create() overwrites the live base_state.json agent) and mirrors the three switch paths through a single _sync_llm_to_meta() helper, matching the existing switch_acp_model precedent. The plugin-merge guard (LLM identity check) is the right call — mirroring the whole live agent would bake in merged agent_context/mcp_config and double-merge on resume. The endpoint tests were correctly migrated from mocking conversation.switch_* to event_service.switch_*. Overall the fix is sound and the e2e resume-from-disk test is valuable.
Material findings
1. Synchronization watermark advances before save_meta() commits (correctness).
In _sync_llm_to_meta(), self._synced_llm = conversation.agent.llm (line 1676) and self.stored = ... (line 1675) are set before await self.save_meta() (line 1677). If save_meta() raises, the in-memory marker and self.stored already reflect the new LLM, but meta.json on disk does not. The _on_conversation_event listener swallows that exception (except Exception: logger.exception(...)), so the failed write is not retried: every subsequent agent-change event hits the identity guard and returns early, so the switched LLM never reaches meta.json until a different LLM is installed. This silently defeats the PR's core guarantee on a transient disk error. Move both the self.stored update and the _synced_llm advance to after a successful save_meta(), keeping the candidate StoredConversation local until persistence succeeds. The endpoint paths (switch_llm/switch_profile) propagate the exception (good), but the in-run tool path through the listener does not retry.
2. .pr/ dev artifacts are committed and this is a fork PR (cannot be auto-removed).
Per the repo's PR_ARTIFACTS policy, .pr/ is PR-only reviewer context that the pr-artifacts.yml workflow auto-removes on approval — but that auto-cleanup cannot push to forks. Since the head branch lives on VascoSch92/software-agent-sdk, these 5 files (RESULTS.md, engel_active_profile_question.py, mock_llm.py, no_switch_repro.py, timeout_restart_evidence.py, ~1100 lines) must be manually removed before merge or they will land on main. None of them are referenced by the actual fix or its tests; they are repro/evidence scripts. Note some contain placeholder API keys (api_key="sk-x", "api_key": "sk-mock"); these are test-only mock values, not real secrets, but they should still not ship in the tree.
Non-blocking observations (no action required for merge)
- The post-run
wait_for_pendingflush (lines 1198–1204) is a belt-and-suspenders backstop for the in-run tool path. The_meta_sync_listeneris the second callback in the composed chain, so its coroutine is already scheduled by the time the firstwait_for_pending(for_callback_wrapper) runs; the second flush mainly guards against the schedule-but-not-complete window. Reasonable as-is. _meta_sync_listeneris not explicitly drained/closed inclose(), but it follows the same lifecycle as_callback_wrapper(which is also not explicitly closed there), so this is consistent with the existing pattern._sync_llm_to_metamirrorsllmandcondenseronly. For anACPAgent,switch_acp_modelhandlesacp_model/agent_stateseparately via its own dedicated path, so the two paths don't conflict. If a future agent type stored switch-relevant state in other top-level fields, this would need revisiting, but that's not the case today.
Risk assessment: MEDIUM
The change is localized and the design (identity-guarded partial mirror) is correct for the plugin case. The main correctness gap is the watermark-before-commit ordering, which only manifests on a failed save_meta() (transient disk/permission error) but is easy to fix. The committed .pr/ artifacts are a merge-hygiene blocker for a fork PR.
This review was created by an AI agent (OpenHands) on behalf of the repository reviewer.
| } | ||
| ) | ||
| self.stored = self.stored.model_copy(update={"agent": agent}) | ||
| self._synced_llm = conversation.agent.llm |
There was a problem hiding this comment.
Watermark advances before the durable commit. self.stored (line 1675) and self._synced_llm (here, line 1676) are set before await self.save_meta() (line 1677). If save_meta() raises, the listener swallows the exception (except Exception: logger.exception(...) in _on_conversation_event), so the failed write is never retried — every later agent-change event returns at the identity guard and the switched LLM never reaches meta.json. Move both mutations to after a successful save_meta(), keeping the candidate StoredConversation local until persistence succeeds. A test where the first save_meta() fails and a later matching update retries would lock this in.
7 participants
HUMAN:
Fix bug with restart.
AGENT:
Why
When the agent-server restarts (e.g. the container is stopped and the conversation is reopened), a conversation's LLM config — including
timeout— silently reverts to the values captured at conversation creation. Switching the LLM profile appears to fix it, but the fix is lost again on the next restart.Root cause: switching the LLM updates only the live
ConversationState(persisted tobase_state.json). On resume,EventService.start()rebuilds the agent frommeta.json(self.stored.agent) andConversationState.create()overwrites the just-loadedbase_state.jsonagent with it (state.agent = agent). Because the switch was never mirrored intometa.json, the creation-time LLM wins on every restart.There are three ways the live LLM changes, and all three were affected: the
/switch_llmendpoint (app-servers, #3017), the/switch_profileendpoint (agent-canvas), and the agent's own built-inswitch_llmtool (SwitchLLMExecutor→conversation.switch_profile, which bypasses the HTTP layer entirely). The ACP model-switch path (switch_acp_model) already handles exactly this by writing the change back tometa.json; the LLM/profile paths were missing it.Summary
EventService._sync_llm_to_meta()helper that writes a swapped LLM back intometa.json(save_meta), matching the existingswitch_acp_modelprecedent./switch_llmand/switch_profileendpoints through newEventServicemethods that call the helper, and call it from the post-run hook so the agent's in-runswitch_llmtool is covered too — all three switch paths now survive a restart.llm/condenserare updated (not the whole live agent), keyed on LLM object identity. This keeps the stored agent plugin-unmerged —_ensure_plugins_loadedmerges pluginagent_context/mcp_configinto the live agent on first run, and persisting that would double-merge on resume — and makes the post-run hook a no-op for ordinary/plugin-loading runs.Issue Number
Fix #4032
Addresses the report: "agent-server sets wrong LLM timeouts when it starts; switching the LLM profile sets the correct values." Reproduction below.
How to Test
Automated (real resume from disk / real agent run, not just unit mocks):
test_switch_llm_survives_restart(endpoint path) starts a realConversationService, creates a conversation whose LLM hastimeout=300, switches to an LLM withtimeout=600, tears the service down, then starts a freshConversationServiceon the same directory (a genuine resume from disk) and asserts the resumed conversation still reportstimeout=600.test_in_run_switch_llm_persists_to_meta(tool path) starts a realEventService, swaps the live LLM directly on the conversation (simulating the agent'sswitch_llmtool, bypassing the endpoints), drives a real run to completion with a scriptedTestLLM, and asserts the post-run hook wrotetimeout=600intometa.json.test_sync_llm_to_meta_ignores_plugin_mergeguards the plugin case: a plugin-style agent swap (new agent object, same LLM) must not be persisted, so the stored agent stays unmerged. (Verified it fails if the whole live agent is mirrored instead.)Manual reproduction (matches the report):
timeout.cat /workspace/conversations/<SESSION>/*.json | jq '.. | .timeout? // empty'— before this change the timeout reverts to the creation-time value; after it, the switched value persists.Video/Screenshots
End-to-end evidence that the fix is exercised (not unit-mocked): each regression test fails when its persistence hook is removed (pre-fix behavior) and passes with the fix.
Endpoint path — pre-fix (mirror step reverted):
Tool path — pre-fix (post-run hook removed):
Both pass with the fix. Full suites for the touched files pass locally (
test_event_service.py,test_conversation_service.py,test_conversation_router.py,test_goal_loop.py— 306 total).ruff check,ruff format, andpyrightare clean (all pre-commit hooks pass).Type
Notes
llmandcondenser(the fields aswitch_llmchanges: condenser is disabled for subscription LLMs / rebuilt for the new model), so the condenser LLM'stimeoutis fixed on resume too — not just the primary LLM.save_metaserializes with the cipher context, so at-rest secret encryption is unchanged.meta.json.switch_acp_model) — untouched.404unknown conversation,404unknown profile,400corrupted profile).