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_event_service.py b/tests/agent_server/test_event_service.py index 3f53ad3331..1d4bf7294b 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,39 @@ 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 — fresh-conversation no-op branch. +# +# 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 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 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"