From b78c719bd14c8ec9e76e2c4b5af65634b34fdb08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=90=BE=20smolpaws?= Date: Sat, 25 Jul 2026 20:48:41 +0200 Subject: [PATCH 1/2] fix(agent-server): base_state authoritative on resume for switched llm/condenser + acp_model (#4032) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A live switch persists only to base_state.json, never meta.json: switch_llm / switch_profile write llm + condenser; switch_acp_model writes acp_model — all onto ConversationState.agent -> base_state.json. But on resume EventService.start() rebuilt the agent purely from the meta.json snapshot (self.stored.agent), and ConversationState.create() then copied that over base_state.json (state.agent = agent). So the switch was silently reverted to the creation-time value (e.g. an LLM's timeout) on the next agent-server restart. Fix: add EventService._resume_agent_with_live_llm(), called from start(). On resume it keeps the creation-time stored agent (tools / agent_context / mcp_config, re-derived by the plugin merge on first run) but overrides the live-mutable fields from base_state.json — llm + condenser for regular agents, acp_model for ACP. Fresh conversations and cross-kind mismatches fall back to the stored agent unchanged. Because base_state is now authoritative for acp_model too, remove the write-side mirror in switch_acp_model that copied the switched model into meta.json. No mutation path mirrors into meta.json anymore. Supersedes the two prior bandaids for this bug: #4028 (mirror llm/condenser into meta on change) and #4219 (read-side authority for regular agents only, leaving ACP on the meta mirror — a split-brain across agent kinds). This unifies both agent kinds on one rule and should merge instead of them. Tests (red -> green): - test_switch_llm_survives_restart: end-to-end #4032 repro through a real service restart (failed assert 300 == 600 before the fix). - test_resume_agent_recovers_switched_{llm,acp_model}_from_base_state and test_resume_agent_fresh_conversation_uses_stored: cover the new method. - test_switch_acp_model_persists_to_meta rewritten to test_switch_acp_model_delegates_without_meta_mirror. Background: https://enyst.github.io/arch/meta-vs-base-state-duplication.html Fixes #4032 Co-authored-by: enyst Co-Authored-By: Claude Opus 4.8 (1M context) --- .../openhands/agent_server/event_service.py | 76 ++++++-- .../test_conversation_eviction.py | 4 +- .../agent_server/test_conversation_service.py | 48 ++++++ tests/agent_server/test_event_service.py | 163 ++++++++++++++++-- 4 files changed, 257 insertions(+), 34 deletions(-) diff --git a/openhands-agent-server/openhands/agent_server/event_service.py b/openhands-agent-server/openhands/agent_server/event_service.py index 7f84fd419d..eaf55c8467 100644 --- a/openhands-agent-server/openhands/agent_server/event_service.py +++ b/openhands-agent-server/openhands/agent_server/event_service.py @@ -177,6 +177,56 @@ async def save_meta(self): ) ) + def _resume_agent_with_live_llm(self) -> AgentBase: + """The agent to instantiate on start, with live-mutable config applied. + + Fresh conversation (no ``base_state.json`` yet): returns + ``self.stored.agent`` from the create request unchanged. + + Resume: ``base_state.json`` is authoritative for the fields a live + switch mutates and persists. ``switch_llm`` / ``switch_profile`` write + ``llm`` and ``condenser``; ``switch_acp_model`` writes ``acp_model`` — + all onto ``ConversationState.agent`` -> ``base_state.json`` only, never + ``meta.json``. So we keep the creation-time ``self.stored.agent`` (its + ``tools`` / ``agent_context`` / ``mcp_config`` are the un-merged config + the plugin merge expects, re-derived on first run) but override the + live-mutable fields from ``base_state.json``. + + Without this, rebuilding the resume agent purely from the ``meta.json`` + snapshot reinstates the stale creation-time value and then clobbers + ``base_state.json`` inside ``ConversationState.create()`` + (``state.agent = agent``), reverting e.g. a switched LLM's timeout — or + a switched ACP model — on the next restart (issue #4032). Scoping to the + switch-mutated fields is exactly what lets every switch path persist to + ``base_state.json`` alone, with no write-side mirror into ``meta.json``. + """ + stored_agent = self.stored.agent + + base_state_file = self.conversation_dir / BASE_STATE + if not base_state_file.exists(): + return stored_agent # fresh conversation, never persisted + + context = {"cipher": self.cipher} if self.cipher else None + persisted = ConversationState.model_validate_json( + base_state_file.read_text(), context=context + ).agent + + if isinstance(stored_agent, ACPAgent): + # ACP's live-switchable field is acp_model; start() re-validates the + # dumped agent, so model_post_init re-derives the sentinel llm.model + # from it. A mismatched persisted kind can't be merged in — fall back + # to the stored agent rather than mixing fields across types. + if not isinstance(persisted, ACPAgent): + return stored_agent + return stored_agent.model_copy(update={"acp_model": persisted.acp_model}) + + if isinstance(persisted, ACPAgent): + return stored_agent + + return stored_agent.model_copy( + update={"llm": persisted.llm, "condenser": persisted.condenser} + ) + def _without_stored_secret(self, secret_name: str) -> StoredConversation: secrets = dict(self.stored.secrets) secrets.pop(secret_name, None) @@ -971,9 +1021,14 @@ async def start(self): working_dir = Path(workspace.working_dir) working_dir.mkdir(parents=True, exist_ok=True) self._ensure_workspace_is_git_repo(working_dir) - agent_cls = type(self.stored.agent) + # base_state.json is authoritative for the live-mutable agent config + # (llm/condenser, or acp_model). meta.json's agent is only a + # creation-time snapshot; see _resume_agent_with_live_llm for why + # resuming purely from it reverts a switched config (issue #4032). + source_agent = self._resume_agent_with_live_llm() + agent_cls = type(source_agent) agent = agent_cls.model_validate( - self.stored.agent.model_dump(context={"expose_secrets": True}), + source_agent.model_dump(context={"expose_secrets": True}), ) # Create LocalConversation with plugins and hook_config. @@ -1617,13 +1672,12 @@ async def switch_acp_model(self, model: str) -> None: For a conversation that has already started, runs the (blocking) protocol-level ``session/set_model`` round-trip in a worker thread; for - one not yet run, the SDK defers the switch (persist-only). Either way it - mirrors the new model into ``meta.json`` so the switch survives an - agent-server restart: ``start()`` rebuilds the agent from - ``self.stored.agent`` and ``ConversationState.create()`` copies that over - the persisted base_state.json on resume. Only ``acp_model`` needs - updating — ``model_post_init`` re-derives the sentinel ``llm.model`` on - reload. + one not yet run, the SDK defers the switch (persist-only). Either way the + SDK persists the new ``acp_model`` onto ``ConversationState.agent`` -> + ``base_state.json``, and ``_resume_agent_with_live_llm`` reads it back on + resume — so the switch survives an agent-server restart with no mirror + into ``meta.json``. ``model_post_init`` re-derives the sentinel + ``llm.model`` from ``acp_model`` on reload. """ if self._conversation is None: # Match the inactive-service convention of the other event-service @@ -1633,10 +1687,6 @@ async def switch_acp_model(self, model: str) -> None: raise ValueError("inactive_service") loop = asyncio.get_running_loop() await loop.run_in_executor(None, self._conversation.switch_acp_model, model) - self.stored = self.stored.model_copy( - update={"agent": self.stored.agent.model_copy(update={"acp_model": model})} - ) - await self.save_meta() async def close(self): self._closing = True diff --git a/tests/agent_server/test_conversation_eviction.py b/tests/agent_server/test_conversation_eviction.py index d06637a5ac..b8a721cb0d 100644 --- a/tests/agent_server/test_conversation_eviction.py +++ b/tests/agent_server/test_conversation_eviction.py @@ -245,8 +245,8 @@ async def test_eviction_preserves_reassigned_stored_metadata(tmp_path): assert service._event_services is not None event_service = service._event_services[conversation_id] - # switch_acp_model / secret updates *replace* event_service.stored and - # persist it; the stale catalog object must not be used on rehydration. + # A title/secret update *replaces* event_service.stored and persists it; + # the stale catalog object must not be used on rehydration. event_service.stored = event_service.stored.model_copy( update={"title": "new-title"} ) diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index 204beb5b9b..4db6f38edf 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -3770,3 +3770,51 @@ async def refresh_then_replace(): ) assert [item.id for item in page.items] == [target] + + +@pytest.mark.asyncio +async def test_switch_llm_survives_restart(tmp_path): + """Issue #4032: a live ``switch_llm`` persists only to base_state.json. + + On resume the server rebuilds the agent from the (stale) meta.json snapshot, + so before the fix the switched LLM — and its ``timeout`` — reverted to the + creation-time value after an agent-server restart. base_state.json must win + for the live-mutable ``llm`` on resume. + """ + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + + request = StartConversationRequest( + agent=Agent( + llm=LLM(model="gpt-4o", usage_id="test-llm", timeout=300), + tools=[], + ), + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + confirmation_policy=NeverConfirm(), + ) + + async with ConversationService(conversations_dir=conversations_dir) as primary: + conversation_info, _ = await primary.start_conversation(request) + conversation_id = conversation_info.id + + # Switch the live LLM to timeout=600 (distinct usage_id so the registry + # installs it rather than reusing the first-write-wins cached entry). + # This writes ConversationState.agent -> base_state.json only. + event_service = await primary.get_event_service(conversation_id) + assert event_service is not None + conversation = event_service.get_conversation() + conversation.switch_llm( + LLM(model="gpt-4o", usage_id="test-llm-switched", timeout=600) + ) + assert conversation.state.agent.llm.timeout == 600 + + # Restart: a fresh service over the same directory hydrates from disk. + async with ConversationService(conversations_dir=conversations_dir) as restarted: + assert restarted._event_services is not None + assert conversation_id not in restarted._event_services + restarted_event_service = await restarted.get_event_service(conversation_id) + assert restarted_event_service is not None + restarted_conversation = restarted_event_service.get_conversation() + # The switch survives the restart instead of reverting to timeout=300. + assert restarted_conversation.state.agent.llm.timeout == 600 diff --git a/tests/agent_server/test_event_service.py b/tests/agent_server/test_event_service.py index 3f53ad3331..4a4333f686 100644 --- a/tests/agent_server/test_event_service.py +++ b/tests/agent_server/test_event_service.py @@ -1820,13 +1820,15 @@ async def test_save_meta_round_trips_agent_definition_mcp_secrets( assert env["TAVILY_API_KEY"].get_secret_value() == "${TAVILY_API_KEY}" @pytest.mark.asyncio - async def test_switch_acp_model_persists_to_meta(self, tmp_path): - """switch_acp_model mirrors the new model into meta.json. - - start() rebuilds the runtime agent from meta.json (self.stored.agent), - and ConversationState.create() copies that agent over the persisted - base_state.json on resume. So the switched model must also be written - to meta.json, otherwise a restart silently reverts to the old model. + async def test_switch_acp_model_delegates_without_meta_mirror(self, tmp_path): + """switch_acp_model delegates to the SDK and does NOT mirror into meta.json. + + The SDK's ``switch_acp_model`` persists the new ``acp_model`` onto + ``ConversationState.agent`` -> ``base_state.json``, and + ``_resume_agent_with_live_llm`` reads it back on resume. So the event + service no longer writes a duplicate copy into ``meta.json`` — that + two-sources-of-truth mirror became redundant once base_state.json was + made authoritative on resume (issue #4032). """ from openhands.sdk.agent import ACPAgent @@ -1842,24 +1844,20 @@ async def test_switch_acp_model_persists_to_meta(self, tmp_path): conv_dir = tmp_path / stored.id.hex conv_dir.mkdir(parents=True, exist_ok=True) - # Stand in for a live conversation; the protocol-level switch is - # covered elsewhere — here we only assert the meta.json mirroring. + # Stand in for a live conversation; the real base_state persistence is + # the SDK's job (covered by tests/sdk/.../test_switch_model.py and by + # test_resume_agent_recovers_switched_acp_model_from_base_state). service._conversation = MagicMock() await service.switch_acp_model("new-model") - # Live switch was delegated to the conversation... + # The live switch was delegated to the SDK conversation... service._conversation.switch_acp_model.assert_called_once_with("new-model") - # ...the in-memory stored agent was updated... + # ...and the event service did NOT mirror it into meta.json: `stored` is + # untouched and no meta.json was written by the switch. assert isinstance(service.stored.agent, ACPAgent) - assert service.stored.agent.acp_model == "new-model" - # ...and the new model was persisted to meta.json so it survives a - # restart. - loaded = StoredConversation.model_validate_json( - (conv_dir / "meta.json").read_text() - ) - assert isinstance(loaded.agent, ACPAgent) - assert loaded.agent.acp_model == "new-model" + assert service.stored.agent.acp_model == "old-model" + assert not (conv_dir / "meta.json").exists() @pytest.mark.asyncio async def test_switch_acp_model_inactive_service_raises_value_error(self, tmp_path): @@ -3423,3 +3421,130 @@ async def test_event_service_creates_lease_with_custom_ttl(tmp_path: Path) -> No assert service._lease is not None assert service._lease._ttl_seconds == 10.0 assert (tmp_path / stored.id.hex / LEASE_FILE_NAME).exists() + + +# --------------------------------------------------------------------------- +# _resume_agent_with_live_llm — base_state.json is authoritative on resume +# +# A live switch_llm / switch_acp_model persists only to base_state.json, never +# to meta.json. On resume EventService rebuilds the agent from the meta snapshot, +# so without reading base_state the switch is silently reverted after a restart +# (issue #4032). These cover the regular LLM/condenser path and the ACP +# acp_model path (which lets the meta write-mirror be removed from +# switch_acp_model), plus the fresh-conversation no-op. +# --------------------------------------------------------------------------- + + +def _persist_base_state_agent( + conversations_dir: Path, cid, agent: AgentBase, workspace_dir: Path +) -> None: + """Write conversations_dir//base_state.json carrying `agent`. + + ``EventService.conversation_dir`` is ``conversations_dir / id.hex``; that is + where ``_resume_agent_with_live_llm`` reads base_state.json. ``create()`` + writes ``base_state.json`` directly under its ``persistence_dir`` (unlike + ``LocalConversation``, which appends the id hex first), so point it straight + at the per-conversation dir. + """ + ConversationState.create( + id=cid, + agent=agent, + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + persistence_dir=str(conversations_dir / cid.hex), + ) + + +def test_resume_agent_recovers_switched_llm_from_base_state(tmp_path: Path) -> None: + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir(parents=True) + cid = uuid4() + + # meta.json snapshot: creation-time LLM (timeout=300). + stored = StoredConversation( + id=cid, + agent=Agent( + llm=LLM(model="gpt-4o", usage_id="test-llm", timeout=300), + tools=[], + ), + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + ) + # base_state.json: a live switch bumped the timeout to 600 (and switched + # to a distinct usage_id, as switch_llm does for a new registry entry). + _persist_base_state_agent( + conversations_dir, + cid, + Agent( + llm=LLM(model="gpt-4o", usage_id="test-llm-2", timeout=600), + tools=[], + ), + workspace_dir, + ) + + service = EventService(stored=stored, conversations_dir=conversations_dir) + resumed = service._resume_agent_with_live_llm() + + # base_state wins for the live-mutable llm; the rest still comes from meta. + assert resumed.llm.timeout == 600 + assert resumed.llm.usage_id == "test-llm-2" + + +def test_resume_agent_recovers_switched_acp_model_from_base_state( + tmp_path: Path, +) -> None: + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir(parents=True) + cid = uuid4() + + # meta.json snapshot: creation-time acp_model. + stored = StoredConversation( + id=cid, + agent=ACPAgent(acp_command=["echo", "test"], acp_model="model-a"), + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + ) + # base_state.json: a live switch_acp_model moved it to model-b. + _persist_base_state_agent( + conversations_dir, + cid, + ACPAgent(acp_command=["echo", "test"], acp_model="model-b"), + workspace_dir, + ) + + service = EventService(stored=stored, conversations_dir=conversations_dir) + resumed = service._resume_agent_with_live_llm() + + assert isinstance(resumed, ACPAgent) + # base_state wins for acp_model; model_post_init re-derives llm.model from it + # when start() re-validates the dumped agent. + assert resumed.acp_model == "model-b" + + +def test_resume_agent_fresh_conversation_uses_stored(tmp_path: Path) -> None: + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir(parents=True) + cid = uuid4() + + stored = StoredConversation( + id=cid, + agent=Agent( + llm=LLM(model="gpt-4o", usage_id="test-llm", timeout=300), + tools=[], + ), + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + confirmation_policy=NeverConfirm(), + initial_message=None, + metrics=None, + ) + # No base_state.json exists yet (never run): stored agent is used verbatim. + service = EventService(stored=stored, conversations_dir=conversations_dir) + resumed = service._resume_agent_with_live_llm() + + assert resumed is stored.agent From c2f1bad9bfe5de177b3b5e96f852ad6fc30abc62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=F0=9F=90=BE=20smolpaws?= Date: Sat, 25 Jul 2026 21:22:56 +0200 Subject: [PATCH 2/2] test: move switch-survives-restart spec to tests/cross/ per convention MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Conversation-resume behavior is cross-package (agent-server orchestrating the SDK's base_state.json persistence), so it belongs in tests/cross/ alongside test_conversation_lease_behavior.py — not in tests/agent_server/. - Add tests/cross/test_conversation_resume_behavior.py: the agent-server counterpart to test_conversation_restore_behavior.py (which documents the SDK LocalConversation contract, where the runtime-provided config wins). Here the caller replays a stale meta.json snapshot, so base_state.json must win for live-switched fields. Covers regular switch_llm AND switch_acp_model surviving a real ConversationService restart, end-to-end. - Drop the end-to-end test from test_conversation_service.py and the two white-box recovery tests from test_event_service.py (now covered behaviorally in cross/). Keep only the fresh-conversation no-op unit test, which the end-to-end spec does not reach, plus the mirror-deletion assertion. Co-authored-by: enyst Co-Authored-By: Claude Opus 4.8 (1M context) --- .../agent_server/test_conversation_service.py | 48 ------- tests/agent_server/test_event_service.py | 105 +-------------- .../test_conversation_resume_behavior.py | 125 ++++++++++++++++++ 3 files changed, 132 insertions(+), 146 deletions(-) create mode 100644 tests/cross/test_conversation_resume_behavior.py diff --git a/tests/agent_server/test_conversation_service.py b/tests/agent_server/test_conversation_service.py index 4db6f38edf..204beb5b9b 100644 --- a/tests/agent_server/test_conversation_service.py +++ b/tests/agent_server/test_conversation_service.py @@ -3770,51 +3770,3 @@ async def refresh_then_replace(): ) assert [item.id for item in page.items] == [target] - - -@pytest.mark.asyncio -async def test_switch_llm_survives_restart(tmp_path): - """Issue #4032: a live ``switch_llm`` persists only to base_state.json. - - On resume the server rebuilds the agent from the (stale) meta.json snapshot, - so before the fix the switched LLM — and its ``timeout`` — reverted to the - creation-time value after an agent-server restart. base_state.json must win - for the live-mutable ``llm`` on resume. - """ - conversations_dir = tmp_path / "conversations" - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir() - - request = StartConversationRequest( - agent=Agent( - llm=LLM(model="gpt-4o", usage_id="test-llm", timeout=300), - tools=[], - ), - workspace=LocalWorkspace(working_dir=str(workspace_dir)), - confirmation_policy=NeverConfirm(), - ) - - async with ConversationService(conversations_dir=conversations_dir) as primary: - conversation_info, _ = await primary.start_conversation(request) - conversation_id = conversation_info.id - - # Switch the live LLM to timeout=600 (distinct usage_id so the registry - # installs it rather than reusing the first-write-wins cached entry). - # This writes ConversationState.agent -> base_state.json only. - event_service = await primary.get_event_service(conversation_id) - assert event_service is not None - conversation = event_service.get_conversation() - conversation.switch_llm( - LLM(model="gpt-4o", usage_id="test-llm-switched", timeout=600) - ) - assert conversation.state.agent.llm.timeout == 600 - - # Restart: a fresh service over the same directory hydrates from disk. - async with ConversationService(conversations_dir=conversations_dir) as restarted: - assert restarted._event_services is not None - assert conversation_id not in restarted._event_services - restarted_event_service = await restarted.get_event_service(conversation_id) - assert restarted_event_service is not None - restarted_conversation = restarted_event_service.get_conversation() - # The switch survives the restart instead of reverting to timeout=300. - assert restarted_conversation.state.agent.llm.timeout == 600 diff --git a/tests/agent_server/test_event_service.py b/tests/agent_server/test_event_service.py index 4a4333f686..1d4bf7294b 100644 --- a/tests/agent_server/test_event_service.py +++ b/tests/agent_server/test_event_service.py @@ -3424,108 +3424,17 @@ async def test_event_service_creates_lease_with_custom_ttl(tmp_path: Path) -> No # --------------------------------------------------------------------------- -# _resume_agent_with_live_llm — base_state.json is authoritative on resume +# _resume_agent_with_live_llm — fresh-conversation no-op branch. # -# A live switch_llm / switch_acp_model persists only to base_state.json, never -# to meta.json. On resume EventService rebuilds the agent from the meta snapshot, -# so without reading base_state the switch is silently reverted after a restart -# (issue #4032). These cover the regular LLM/condenser path and the ACP -# acp_model path (which lets the meta write-mirror be removed from -# switch_acp_model), plus the fresh-conversation no-op. +# The behavioral spec (a live switch_llm / switch_acp_model surviving a restart +# because base_state.json is authoritative on resume, issue #4032) lives in +# tests/cross/test_conversation_resume_behavior.py, exercised end-to-end through +# ConversationService. Here we only pin the branch that spec does not reach: +# before the first run there is no base_state.json, so the stored (create-time) +# agent must be used verbatim. # --------------------------------------------------------------------------- -def _persist_base_state_agent( - conversations_dir: Path, cid, agent: AgentBase, workspace_dir: Path -) -> None: - """Write conversations_dir//base_state.json carrying `agent`. - - ``EventService.conversation_dir`` is ``conversations_dir / id.hex``; that is - where ``_resume_agent_with_live_llm`` reads base_state.json. ``create()`` - writes ``base_state.json`` directly under its ``persistence_dir`` (unlike - ``LocalConversation``, which appends the id hex first), so point it straight - at the per-conversation dir. - """ - ConversationState.create( - id=cid, - agent=agent, - workspace=LocalWorkspace(working_dir=str(workspace_dir)), - persistence_dir=str(conversations_dir / cid.hex), - ) - - -def test_resume_agent_recovers_switched_llm_from_base_state(tmp_path: Path) -> None: - conversations_dir = tmp_path / "conversations" - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir(parents=True) - cid = uuid4() - - # meta.json snapshot: creation-time LLM (timeout=300). - stored = StoredConversation( - id=cid, - agent=Agent( - llm=LLM(model="gpt-4o", usage_id="test-llm", timeout=300), - tools=[], - ), - workspace=LocalWorkspace(working_dir=str(workspace_dir)), - confirmation_policy=NeverConfirm(), - initial_message=None, - metrics=None, - ) - # base_state.json: a live switch bumped the timeout to 600 (and switched - # to a distinct usage_id, as switch_llm does for a new registry entry). - _persist_base_state_agent( - conversations_dir, - cid, - Agent( - llm=LLM(model="gpt-4o", usage_id="test-llm-2", timeout=600), - tools=[], - ), - workspace_dir, - ) - - service = EventService(stored=stored, conversations_dir=conversations_dir) - resumed = service._resume_agent_with_live_llm() - - # base_state wins for the live-mutable llm; the rest still comes from meta. - assert resumed.llm.timeout == 600 - assert resumed.llm.usage_id == "test-llm-2" - - -def test_resume_agent_recovers_switched_acp_model_from_base_state( - tmp_path: Path, -) -> None: - conversations_dir = tmp_path / "conversations" - workspace_dir = tmp_path / "workspace" - workspace_dir.mkdir(parents=True) - cid = uuid4() - - # meta.json snapshot: creation-time acp_model. - stored = StoredConversation( - id=cid, - agent=ACPAgent(acp_command=["echo", "test"], acp_model="model-a"), - workspace=LocalWorkspace(working_dir=str(workspace_dir)), - confirmation_policy=NeverConfirm(), - initial_message=None, - metrics=None, - ) - # base_state.json: a live switch_acp_model moved it to model-b. - _persist_base_state_agent( - conversations_dir, - cid, - ACPAgent(acp_command=["echo", "test"], acp_model="model-b"), - workspace_dir, - ) - - service = EventService(stored=stored, conversations_dir=conversations_dir) - resumed = service._resume_agent_with_live_llm() - - assert isinstance(resumed, ACPAgent) - # base_state wins for acp_model; model_post_init re-derives llm.model from it - # when start() re-validates the dumped agent. - assert resumed.acp_model == "model-b" - - def test_resume_agent_fresh_conversation_uses_stored(tmp_path: Path) -> None: conversations_dir = tmp_path / "conversations" workspace_dir = tmp_path / "workspace" diff --git a/tests/cross/test_conversation_resume_behavior.py b/tests/cross/test_conversation_resume_behavior.py new file mode 100644 index 0000000000..75417a9c10 --- /dev/null +++ b/tests/cross/test_conversation_resume_behavior.py @@ -0,0 +1,125 @@ +"""Integration-like tests for agent-server conversation resume semantics. + +These are the agent-server counterpart to ``test_conversation_restore_behavior`` +(which documents the *SDK* ``LocalConversation`` restore contract, where the +runtime-provided agent config wins). Here the caller is the agent server, which +rebuilds the runtime agent from its ``meta.json`` snapshot — a snapshot taken at +*creation* and never re-written when a live ``switch_llm`` / ``switch_profile`` / +``switch_acp_model`` mutates the agent. Those switches persist only to +``base_state.json``. So on an agent-server restart, ``base_state.json`` — not the +stale ``meta.json`` — must be authoritative for the live-mutable agent config, +or the switch silently reverts (issue #4032). + +Spec, exercised end-to-end through ``ConversationService`` (start -> switch -> +tear down -> restart over the same dir -> hydrate via the real lazy-resume path): + +- A live ``switch_llm`` (regular agent) survives a restart: ``llm`` (and its + ``timeout``) comes back switched, not reverted to the creation-time value. +- A ``switch_acp_model`` (ACP agent) survives a restart: ``acp_model`` comes back + switched, with no write-side mirror into ``meta.json``. +""" + +from pathlib import Path + +import pytest + +from openhands.agent_server.conversation_service import ConversationService +from openhands.agent_server.models import StartConversationRequest +from openhands.sdk import LLM, Agent +from openhands.sdk.agent import ACPAgent +from openhands.sdk.security.confirmation_policy import NeverConfirm +from openhands.sdk.workspace import LocalWorkspace + + +def _request(agent, workspace_dir: Path) -> StartConversationRequest: + return StartConversationRequest( + agent=agent, + workspace=LocalWorkspace(working_dir=str(workspace_dir)), + confirmation_policy=NeverConfirm(), + ) + + +@pytest.mark.asyncio +async def test_switched_llm_survives_agent_server_restart(tmp_path): + """Issue #4032: a live ``switch_llm`` persists only to base_state.json. + + Before the fix the server rebuilt the resume agent purely from the stale + ``meta.json`` snapshot, so the switched LLM — and its ``timeout`` — reverted + to the creation-time value after a restart. base_state.json must win for the + live-mutable ``llm`` on resume. + """ + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + + request = _request( + Agent(llm=LLM(model="gpt-4o", usage_id="test-llm", timeout=300), tools=[]), + workspace_dir, + ) + + async with ConversationService(conversations_dir=conversations_dir) as primary: + conversation_info, _ = await primary.start_conversation(request) + conversation_id = conversation_info.id + + # Switch the live LLM to timeout=600 (distinct usage_id so the registry + # installs it rather than reusing the first-write-wins cached entry). + # This writes ConversationState.agent -> base_state.json only. + event_service = await primary.get_event_service(conversation_id) + assert event_service is not None + conversation = event_service.get_conversation() + conversation.switch_llm( + LLM(model="gpt-4o", usage_id="test-llm-switched", timeout=600) + ) + assert conversation.state.agent.llm.timeout == 600 + + # Restart: a fresh service over the same directory hydrates from disk. + async with ConversationService(conversations_dir=conversations_dir) as restarted: + assert restarted._event_services is not None + assert conversation_id not in restarted._event_services + restarted_event_service = await restarted.get_event_service(conversation_id) + assert restarted_event_service is not None + restarted_conversation = restarted_event_service.get_conversation() + # The switch survives the restart instead of reverting to timeout=300. + assert restarted_conversation.state.agent.llm.timeout == 600 + + +@pytest.mark.asyncio +async def test_switched_acp_model_survives_agent_server_restart(tmp_path): + """A pre-session ``switch_acp_model`` persists only to base_state.json. + + ACP's live-switchable field is ``acp_model`` (``model_post_init`` re-derives + the sentinel ``llm.model`` from it). The switch defers before the first run + and persists to base_state.json; on resume the server must read it back from + there rather than from the stale ``meta.json`` snapshot — the same rule as + the regular LLM path, so no ``meta.json`` write-mirror is needed. + """ + conversations_dir = tmp_path / "conversations" + workspace_dir = tmp_path / "workspace" + workspace_dir.mkdir() + + request = _request( + ACPAgent(acp_command=["echo", "test"], acp_model="model-a"), + workspace_dir, + ) + + async with ConversationService(conversations_dir=conversations_dir) as primary: + conversation_info, _ = await primary.start_conversation(request) + conversation_id = conversation_info.id + + event_service = await primary.get_event_service(conversation_id) + assert event_service is not None + conversation = event_service.get_conversation() + # No live session yet: switch_acp_model defers and persists model-b to + # base_state.json (no protocol round-trip, no subprocess). + conversation.switch_acp_model("model-b") + acp_agent = conversation.state.agent + assert isinstance(acp_agent, ACPAgent) + assert acp_agent.acp_model == "model-b" + + async with ConversationService(conversations_dir=conversations_dir) as restarted: + restarted_event_service = await restarted.get_event_service(conversation_id) + assert restarted_event_service is not None + restarted_agent = restarted_event_service.get_conversation().state.agent + assert isinstance(restarted_agent, ACPAgent) + # The switch survives the restart instead of reverting to model-a. + assert restarted_agent.acp_model == "model-b"