fix(chat): request-local LLM overrides to close the A/B concurrency race (closes #86)#124
Merged
Merged
Conversation
added 16 commits
July 18, 2026 06:24
Commit the OpenSpec change artifacts (proposal, design, spec delta, tasks) for the request-local LLM override fix and repoint the tracked tasks.md symlink at this change, mirroring how prior change branches activate their task list. No code change; the implementation tasks (section 1 onward) follow in subsequent commits. Ralph-Task: spec/proposal phase for fix-issue-86-request-local-llm-overrides
Let a caller stream from a supplied pipeline instead of the shared self.pipeline via an optional pipeline= keyword, without mutating shared state. supports_stream() now evaluates the pipeline actually in use. This is the orchestrator seam the request-local LLM override (issue #86) will drive; the non-override path is unchanged. Ralph-Task: 1.2 Add the optional `pipeline=None` keyword to `archi.stream()` in `src/archi/archi.py`, defaulting to `self.pipeline`; make `supports_stream()` evaluate the pipeline actually in use, and keep `_prepare_call_kwargs` + `_ensure_pipeline_output` on the path. Confirm 1.1 passes.
Extend the optional pipeline= seam introduced on stream() to invoke() and astream(), with supports_astream() now evaluating the pipeline actually in use. A caller may serve one request from its own pipeline view without mutating the shared self.pipeline. Ralph-Task: 1.3 Extend the same optional-pipeline support to archi.invoke() and archi.astream() (with matching supports_astream() handling), with a test per method.
Add `_build_request_local_pipeline(pipeline, override_llm)` to the chat app: a `copy.copy` view of the shared pipeline bound to a per-request override LLM, with every per-run attribute (`agent`, `_active_tools`, `_active_middleware`, `_active_memory`, `_static_tools`, `_vector_tools`, `_vector_retrievers`) reset and `refresh_agent(force=True)` re-run so tools rebuild bound to the view. This performs zero writes to the shared pipeline instance (design D1) and rebinds the self-bound static-tool callbacks to the view so an overridden request records documents into its own run memory, not the shared one (design D1a) — the seam the override wiring (issue #86) will drive. Tests exercise the real BaseReActAgent refresh/tools/RunMemory paths: the identity guard for the zero-writes invariant (2.1), the per-run-state reset (2.2), and the static-tool document-isolation regression (2.3). Ralph-Task: 2.4 Implement `_build_request_local_pipeline(pipeline, override_llm)` in `src/interfaces/chat_app/app.py` per design D1 + D1a — `copy.copy`, reset the full attribute list from 2.2, then `refresh_agent(force=True)` on the copy so tools rebuild bound to the view. Include the comment stating the real invariant: *any attribute that is per-run state, or that closes over a bound method of the pipeline, must be rebuilt on the view, never shared.* Confirm 2.1-2.3 pass.
Add a deterministic concurrency test for the shared pipeline's lazy MCP memoization (issue #86, design D6): two threads racing refresh_agent(force=True) with _mcp_tools is None must yield exactly one _build_mcp_tools() call and one MCP client. The build blocks on a release event that only fires once both threads have provably entered it (unguarded) or a bounded wait elapses (guarded), so the outcome is timing-independent. RED phase: the D6 lock is not implemented, so refresh_agent races and builds twice. Marked xfail(strict=True); the GREEN phase (task 2.6) adds the instance lock and removes the marker. Ralph-Task: 2.5 Write a failing test that two threads concurrently building a view from a pipeline with `_mcp_tools is None` trigger exactly one `_build_mcp_tools()` call and construct exactly one MCP client.
Guard the lazy _mcp_tools memoization in refresh_agent with a per-instance threading.Lock created in __init__ (issue #86, design D6). Concurrent request-local view builds — the A/B comparison pair this change enables — could both observe _mcp_tools is None, both enter _build_mcp_tools, and construct then leak two MCP clients on the shared AsyncLoopThread. Double-checked locking now yields exactly one _build_mcp_tools() call and one client per pipeline instance. Filling _mcp_tools is D1's single permitted write to shared state: a pure, idempotent memoization that leaves agent / agent_llm / _active_* untouched. Removes the strict xfail marker from the task 2.5 concurrency test, which now passes. Ralph-Task: 2.6 Implement design D6: add an instance-level `threading.Lock` in `BaseReActAgent.__init__` and guard the lazy `_mcp_tools is None` check-and-build in `refresh_agent` with it.
Add a deterministic concurrency test at the ChatWrapper.stream() level (issue #86, the fix's core acceptance criterion): two overlapping overridden turns must each observe their own override LLM for the whole turn, and a request started after both complete must observe the configured default LLM and its extra_kwargs, with no residue on the shared pipeline. A threading.Barrier rendezvouses the two turns so both have installed their override before either observes the LLM in use (no sleep-based timing). The test bypasses ChatWrapper.__init__ and stubs the collaborators the override path does not touch, so it stays a unit test while exercising the real override block, the real self.archi.stream(...) call site, and the real finally cleanup. RED phase: stream() still swaps the shared pipeline's agent_llm, so request A observes B's LLM mid-turn. Marked xfail(strict=True); the GREEN phase (task 3.3) builds a request-local view and passes pipeline=view to archi.stream, then removes the marker. Ralph-Task: 3.1 Write the failing concurrency regression test (the issue's core acceptance criterion): two overlapping overridden ChatWrapper.stream() turns each observe their own override LLM for the whole turn, and a third request started after both complete observes the configured default LLM and its extra_kwargs.
Add a second deterministic concurrency test at the ChatWrapper.stream() level (issue #86): the two overlapping overridden turns must not be serialized. Request A yields its first output and then parks; request B is only started once A is suspended, so B's override applies strictly after A's and B must still make progress while A's turn is open. A timeline records that B produces its first output before A's turn completes, proving genuine overlap (a lock spanning the whole turn would deadlock and time out here). The same run observes the LLM in use at every step, so it also proves each turn keeps its own override LLM across the overlap. Refactor _make_wrapper to accept an optional archi_factory so the test can supply its asymmetric interleaving without disturbing the barrier-driven 3.1 test. RED phase: against the swap/restore implementation request A observes B's LLM on its second step (B mutated the shared pipeline and has not yet restored it), so the own-LLM assertion fails. Marked xfail(strict=True); the GREEN phase (task 3.3) builds a request-local view and passes pipeline=view to archi.stream, then removes the marker. Ralph-Task: 3.2 Write a failing test that the A/B pair is not serialized: the second overridden request produces its first output before the first request's turn completes.
Rewrite the override block in ChatWrapper.stream() to build a request-local pipeline view via _build_request_local_pipeline instead of swapping the shared pipeline's agent_llm, and pass it through archi.stream(..., pipeline=view). The shared pipeline is never mutated, so overlapping overridden requests each keep their own LLM for the whole turn and run in parallel. Preserves the D5 error contract: ValueError from override construction → HTTP 400 + return; any other failure, including a view-build failure, → warning event + fall back to the shared pipeline. Drops the LLM swap/restore from the finally (nothing shared is mutated now); the reported-model restore stays until task 3.5. Removes the strict xfail markers from the two concurrency regression tests, which now pass. Ralph-Task: 3.3 Rewrite the override block in `ChatWrapper.stream()` (~app.py:2003-2031) to build a request-local view instead of calling `_swap_pipeline_llm`, and pass it to `self.archi.stream(..., pipeline=view)` at the call site (~app.py:2045). Preserve the existing error contract per design D5: `ValueError` → HTTP 400 + return; any other exception (including a view-build failure) → warning event + fall back to the shared pipeline. Confirm 3.1 and 3.2 pass.
Add a ChatWrapper.stream()-level test for design D3's persistence half (issue #86): an overridden turn must persist the OVERRIDE's provider/model into the conversation row's model_used, while the shared self.current_model_used attribute is never mutated mid-turn. Asserting only "the shared field is untouched" would pass while shipping a silent mislabelling regression (every overridden row persisted with the default model), so the test asserts the persisted value by replicating insert_conversation's documented default (model_used if supplied, else self.current_model_used). The test bypasses __init__ (object.__new__) and stubs the collaborators the override path does not exercise, running the real override block and the real self._finalize_result(...) call site as a unit test. RED phase: against the swap/restore implementation stream() sets self.current_model_used to the override before finalization and restores it in finally, so the shared attribute reads the override when the row is persisted — the shared-field-unchanged assertion fails. Marked xfail(strict=True); the GREEN phase (task 3.5) threads the override's model through _finalize_result / insert_conversation as a parameter and stops writing the shared attribute, then removes the marker. Ralph-Task: 3.4 Write a failing test for the persistence half of design D3: an overridden turn persists the override's provider/model into the conversation row's model_used, while self.current_model_used is unchanged.
Make the reported model request-local (issue #86, design D3). Add an optional model_used parameter to insert_conversation and _finalize_result, defaulting to self.current_model_used so the non-override and non-streaming __call__ paths are byte-for-byte unchanged. The override stream path now tracks the reported model in a local (reported_model) and threads the override's provider/model through finalization instead of mutating the shared self.current_model_used mid-turn, so overridden A/B rows persist the override while concurrent requests never bleed one another's label. Also cover insert_conversation / _finalize_result and the design D5 override error contract (ValueError -> 400, generic error and view-build failure -> warning + fall back to default) with real-code tests. Ralph-Task: 3.5 Make the reported model request-local (design D3): add an optional model_used parameter to insert_conversation and _finalize_result, defaulting to self.current_model_used so the non-override and non-streaming __call__ paths are unchanged; use it at the three read sites and at the response payload; pass the override's provider/model from the request-local stream path and stop writing the shared attribute. Confirm 3.4 passes.
The request-local view (issue #86) never mutates the shared pipeline's agent_llm and the reported model is threaded through finalization as a request-local value (design D3), so the override_applied / override_original_model locals and their finally restore block are dead weight. Remove them, keeping the cursor/connection cleanup, and add a regression test that the shared pipeline's agent_llm is the identical object before and after an overridden turn that raises mid-stream. Ralph-Task: 4.1 Delete the override_applied / override_original_llm / override_original_model locals and their restore block from the finally, keeping the cursor/connection cleanup intact. Add a test that the shared pipeline's agent_llm is unchanged across an overridden turn that raises mid-stream.
Remove `_swap_pipeline_llm` / `_restore_pipeline_llm` and their unit tests now that `_build_request_local_pipeline` fully replaces the shared-pipeline mutation path (issue #86); no callers remain, and the tree grep is clean (only the OpenSpec change docs still name the removed helpers, describing the change). Keep the `_build_provider_config_from_payload` / `extra_kwargs` coverage and add direct coverage for the override-gating helper `_is_provider_enabled_in_config`. Ralph-Task: 4.2 Delete `_swap_pipeline_llm` and `_restore_pipeline_llm` from `src/interfaces/chat_app/app.py` (~163-192) once they have no callers.
`bash scripts/gate.sh` passes on the committed branch: black + isort clean, 891 tests pass, and diff coverage vs origin/dev is 86% (>= 80%). Marks task 5.1. Ralph-Task: 5.1 Run `bash scripts/gate.sh` and get it green — black 24.10.0 + isort 6.0.1, full pytest, and `diff-cover --fail-under=80` against `origin/dev`.
Filed #123 for the two out-of-scope hazards recorded in the change design's Open Questions: concurrent default requests still share _active_memory on the shared pipeline, with the structural fix being to move the active RunMemory into a contextvars.ContextVar. References this change and issue #86. Ralph-Task: 5.2 File a follow-up issue for the adjacent, out-of-scope hazards named in design Open Questions
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7ba73c6e19
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| # in `reported_model` and is threaded through finalization, so the shared | ||
| # `self.current_model_used` is never mutated mid-turn (design D3). | ||
| request_pipeline = None | ||
| reported_model = self.current_model_used |
There was a problem hiding this comment.
When a streaming request passes a different config_name without a provider/model override, this snapshot still contains the model from the previously active config because update_config() runs later and updates self.current_model_used on line 2016. The new reported_model is then threaded into _finalize_result() and the final event, so the conversation row and response metadata are mislabelled with the old model whenever users switch configs (and also on override fallback). Move this assignment until after update_config() succeeds, before applying any request-local override.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in bc6be64. Moved the reported_model = self.current_model_used snapshot to after update_config() (it was captured before, so a config switch without an override persisted the previous config's model). The override path still overwrites it with provider/model. Test: test_config_switch_without_override_reports_new_config_model.
| view._vector_tools = None | ||
| if hasattr(view, "_vector_retrievers"): | ||
| view._vector_retrievers = None | ||
| view.refresh_agent(force=True) |
There was a problem hiding this comment.
When the source pipeline selects mcp but _mcp_tools is still None (for example a deferred agent initialization or cache reset), two concurrent overrides each copy that None into separate views and then this refresh populates self._mcp_tools on each view, not on the shared source. The new lock serializes the calls but the second view still sees its own None after waiting, so it builds another MCP client and discards/leaks the loser; the memoization needs to happen on the source before/while copying, or the view needs to reuse a source-populated list under the same lock.
Useful? React with 👍 / 👎.
Member
Author
There was a problem hiding this comment.
Fixed in bc6be64 — this was design D6 not fully landing. _build_request_local_pipeline now memoizes MCP tools on the source (under the source's _mcp_lock) before copy.copy, so concurrent views share one initialize_mcp_client() and inherit the populated list instead of each building/leaking their own. _build_mcp_tools binds the MCP client (not the per-instance memory callbacks), so the shared list is safe — as D6 assumed. Test: test_mcp_tools_memoized_on_source_and_shared_by_views.
added 2 commits
July 18, 2026 05:27
…rce memoization Finding A (regression): reported_model was snapshotted before update_config(), so a request switching config_name without an override persisted the PREVIOUS config's model. Move the snapshot after update_config() (the override path still overwrites it). Test: test_config_switch_without_override_reports_new_config_model. Finding B (design D6 gap): _build_request_local_pipeline let refresh_agent build MCP tools on each view, so concurrent overrides each built/leaked their own client. Memoize on the SOURCE before copying (the one permitted shared write, per D6); views inherit the list. Test: test_mcp_tools_memoized_on_source_and_shared_by_views.
…ocal-llm-overrides # Conflicts: # tasks.md
Member
Author
|
Resolved the merge conflict with
No code changed beyond the merge; the two Codex findings remain fixed in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #86.
The race
A per-request provider/model override in the chat app was applied by mutating the process-wide shared pipeline (
ChatWrapperis a singleton), and the override was held for the entire streamed turn. Two overlapping requests therefore raced: request B captured request A's override as its "original" LLM, and B'sfinallyrestored A's override onto the shared pipeline — pinning every later user to A's model and itsextra_kwargs(e.g.enable_thinking).This is reachable from the A/B comparison UI, which deliberately starts two streaming requests in parallel. PR #85 made the swap atomic and restored it on completion, closing permanent poisoning from a single request — but it could not close the concurrent case, because the window is the whole turn, not the swap. A mutex is not an acceptable fix: serializing the two requests defeats the parallel comparison the A/B path exists for. The same shared-state hazard also bled the retrieved-document sink and the reported model name, which are instance state on the same singleton.
The fix: a request-local pipeline view
stream()/invoke()/astream()now accept an optional caller-supplied pipeline, defaulting toself.pipeline— so the chat app can drive a request-local pipeline through the existing vectorstore /PipelineOutputplumbing rather than around it.supports_stream()/supports_astream()evaluate the pipeline actually in use._build_request_local_pipeline()builds a per-request view viacopy.copy, resets all per-run state (agent,_active_tools,_active_middleware,_active_memory,_static_tools,_vector_tools/_vector_retrievers), and callsrefresh_agent(force=True)so tools rebuild bound to the view. This is the D1a fix: the cached static tools close overself._store_documents, so sharing them would route an overridden request's retrieved documents into the shared pipeline's run memory — the same bleed, moved from the LLM to the sources. The shared pipeline'sagent_llm/agentremain the identical objects (asserted withis).insert_conversation/_finalize_resultgain an optionalmodel_useddefaulting toself.current_model_used), so an overridden turn records the model that actually answered inmodel_usedwithout writing the shared field that later default requests read back.threading.LockinBaseReActAgentguards the lazy_mcp_tools is Nonecheck-and-build, so two threads building a view concurrently trigger exactly one_build_mcp_tools()/ one MCP client._swap_pipeline_llm/_restore_pipeline_llmand theoverride_applied/override_original_llm/override_original_modelbookkeeping and itsfinallyrestore) is removed now that the request-local path replaces it.ValueError→ HTTP 400; any other exception, including a view-build failure → warning event + fall back to the shared pipeline) are all preserved.Proof
The core acceptance test drives the interleaving deterministically (threads + a barrier, no
sleep): two overlapping overriddenChatWrapper.stream()turns each observe their own override LLM for the whole turn, and a third request started after both complete observes the configured default LLM and itsextra_kwargs. A companion test proves the A/B pair is not serialized — the second overridden request emits its first output before the first turn completes. Additional tests cover the zero-writes-to-shared identity invariant, the static-tool document-isolation bug, the single-MCP-build guard, and request-local persistence ofmodel_used.Follow-up #123 tracks the adjacent, out-of-scope hazard (concurrent default requests still share
_active_memory; structural fix is acontextvars.ContextVarfor the activeRunMemory).🤖 Generated with Claude Code