fix(agent-server): base_state authoritative on resume for switched llm/condenser + acp_model (#4032)

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

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"}
         )

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

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"