fix(tui_gateway): reconcile config.set races with deferred agent build - #63998
fix(tui_gateway): reconcile config.set races with deferred agent build#63998counterposition wants to merge 1 commit into
Conversation
_make_agent can block for seconds (MCP discovery, prompt/skill build). A `config.set reasoning` arriving in that window only updates session["create_reasoning_override"] — session["agent"] is still None, so the live-apply branch is skipped — and the build then installs an agent constructed from the values snapshotted at build start and emits session.info with the old effort. The user's pick is silently lost for the life of the session: nothing reconciles the built agent against the session dict afterwards. The same window loses a model override pinned mid-build by the agent-None paths of `/model X --provider Y` and `/moa` (nothing ever adopts it later — _sync_agent_model_with_config skips sessions that carry a model_override), and the /new rebuild (_reset_session_agent) has the identical shape between snapshotting reset_kw and installing the new agent. Fix: install freshly built agents through _install_agent_reconciled(), which compares the session dict's current overrides (reasoning / service tier / model) against what the build actually used and applies any drift to the agent before publishing it — reasoning/tier exactly as the live config.set path would, the model via the same in-place switch_model the live /model path uses (which rolls back on failure, so a failed reconcile keeps the built model). The reconcile-and- install runs under a per-session agent_config_lock, and config.set reasoning's write-then-check now holds the same lock, so a concurrent mutation either observes the installed agent (live-apply path) or is adopted at install — never dropped. Comparing against the build-time snapshot rather than the agent's resolved attributes avoids redundant re-switches on deferred resumes, where the stored provider string can legitimately differ from the agent's resolved one. Tests thread the real deferred build through an event-controlled barrier and mutate the session mid-build, asserting the installed agent and the emitted session.info reflect the newer value; plus a no-redundant-switch guard for unchanged overrides and the /new rebuild variant. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real deferred-construction race. Current main snapshots the reasoning override before _make_agent() (tui_gateway/server.py:1383-1389) and installs afterward (tui_gateway/server.py:1393-1395), while config.set reasoning only updates the session dict when no agent exists (tui_gateway/server.py:10643-10650).
Problems
- The proposed reconcile helper logs a failed
switch_model()but retains the newly writtenmodel_override. That differs from the live path, which raises before committing the override (tui_gateway/server.py:2995-3006); a later/newor resume can therefore resurrect the failed target. - The new lock only covers reasoning. Explicit-provider
config.set modelcan bypass the initialization wait (tui_gateway/server.py:10324-10342) and writesmodel_overridein_apply_model_switch()(tui_gateway/server.py:3029-3036); pre-agent/moawrites it directly (tui_gateway/server.py:12116-12122). Those paths remain able to race the proposed reconcile read/install sequence.
Suggested changes
- Synchronize all build-relevant override writers with installation, and restore/clear a failed reconciled model override.
- Add barrier tests through the actual explicit-provider model and
/moaroutes, plus a failed-switch regression test.
Automated hermes-sweeper review.
| logger.warning( | ||
| "mid-build model switch reconcile failed; keeping %s", | ||
| getattr(agent, "model", ""), | ||
| exc_info=True, |
There was a problem hiding this comment.
Catching this exception leaves session["model_override"] at the new target even though the agent rolled back. _apply_model_switch() deliberately raises before committing that override on a live failure (tui_gateway/server.py:2995-3006); restore or clear the pending override here so /new and resume do not later select the failed model.
Summary
Fixes a race in the TUI gateway's deferred agent construction that silently loses a user's
config.setpick for the life of the session.The race:
_start_agent_buildsnapshotssession["create_reasoning_override"]into the build kwargs, then calls_make_agent, which can block for seconds (MCP discovery, prompt/skill build).config.set reasoningarrives during that window, it updatessession["create_reasoning_override"]— but becausesession["agent"]is stillNone, the live-apply branch is skipped: no agent is updated and nosession.infois emitted.session.infowith the old effort. The session dict retains the new override, but nothing ever reconciles the built agent against it — the user's setting is silently lost until the session is rebuilt.The same window affects two siblings:
Nonepaths of/model X --provider Yand/moa(both writesession["model_override"]directly). Nothing adopts it later —_sync_agent_model_with_configdeliberately skips sessions carrying amodel_override— so the turn silently runs on the model snapshotted at build start./newrebuild (_reset_session_agent) has the identical shape between snapshottingreset_kwand installing the new agent.The eager build paths (eager resume,
session.branch) are immune: they only register the session after the build completes, so no client can issueconfig.setagainst them mid-build.The fix
Freshly built agents are now installed through
_install_agent_reconciled(), which compares the session dict's current overrides (reasoning / service tier / model) against what the build actually used and applies any drift to the agent before publishing it:config.setpath does;agent.switch_model()the live/modelpath uses (it rolls back atomically on failure, so a failed reconcile keeps the built model — matching the live path's failed-switch-is-a-no-op contract).The reconcile-and-install runs under a per-session
agent_config_lock, andconfig.set reasoning's write-then-check now holds the same lock. That makes the fix airtight rather than just narrowing the window: a concurrent mutation either observes the installed agent (live-apply path) or is adopted at install — there is no interleaving where it's dropped.Comparing against the build-time snapshot rather than the agent's resolved attributes avoids redundant re-switches on deferred resumes, where the stored provider string (e.g.
custom:<name>) can legitimately differ from the agent's resolved one.Tests
Four new tests in
tests/test_tui_gateway_server.pythread the real deferred build through an event-controlled barrier (a fake_make_agentblocks on anEvent) and mutate the session mid-build:test_deferred_build_adopts_reasoning_set_mid_build— issues a realconfig.set reasoningmid-build and asserts both the installed agent and thesession.infoemitted at build completion carry the newer value.test_deferred_build_adopts_model_pin_set_mid_build— pinssession["model_override"]mid-build and asserts the built agent is switched in place to the pinned identity.test_deferred_build_does_not_reswitch_unchanged_model_override— regression guard: an override that did not change mid-build (every normal create / deferred resume) must not trigger a redundantswitch_modelon install.test_reset_session_agent_adopts_reasoning_set_mid_rebuild— the/newrebuild variant.🤖 Generated with Claude Code