From 5de54147e17bcb57a18eef6cb12c5a522c23e967 Mon Sep 17 00:00:00 2001 From: Patrick Buckley Date: Wed, 5 Aug 2026 21:37:11 -0700 Subject: [PATCH] fix(832): a prep fault walks the fallbacks it can no longer speak for MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Making prepare_wire lane-variant invalidated the premise behind the walk-abort on WirePreparationError: with the fold posture following each lane's capabilities, a preparation fault on one lane no longer implies every lane fails, so aborting the walk skipped healthy fallbacks and the dedicated fatal message was wrong on both of its claims. Preparation faults now keep their no-health rule on every lane but continue the walk — the primary's fault enters it and a fallback's fault yields to the next alias — and the fatal message drops the no-fallback claim. Riding cleanup: the self-surfacing exception pair gets one spelling for the re-issue mask (_SELF_SURFACING_ERRORS; the walk arms stay per-class because auth aborts where prep continues); the tag-scan gate gains a capabilities-shaped form (caps_scan_inline_reasoning) that the lane form delegates to and the title peel now uses, retiring the third spelling; the three streaming provider fakes build on one provider_shell; a comment in session_ui_base names the module function that replaced the deleted session delegate; close_run spells its carry cut as removesuffix; and the prepare_wire docstring paragraph is re-flowed. The walk-continues and per-lane no-health pins are mutation-probed. --- tests/_session_helpers.py | 28 ++++++++----- tests/test_midstream_retry.py | 68 ++++++++++++++++++++++++++++--- turnstone/core/model_turn.py | 29 +++++++------ turnstone/core/session.py | 43 +++++++++++-------- turnstone/core/session_ui_base.py | 2 +- turnstone/core/streaming_text.py | 2 +- 6 files changed, 123 insertions(+), 49 deletions(-) diff --git a/tests/_session_helpers.py b/tests/_session_helpers.py index d9e224a2..93d93497 100644 --- a/tests/_session_helpers.py +++ b/tests/_session_helpers.py @@ -382,9 +382,7 @@ def seam_provider(utterance: str, *, provider_name: str = "openai-compatible") - later session in the test run (the SSE-recovery e2e servers resolve the same instance). """ - provider = MagicMock() - provider.provider_name = provider_name - provider.get_capabilities.return_value = ModelCapabilities() + provider = provider_shell(provider_name) provider.create_streaming = MagicMock(return_value=think_tag_stream(utterance)) return provider @@ -520,6 +518,20 @@ class ArmedHandle: self.closed = True +def provider_shell( + name: str = "openai-compatible", + retryable: frozenset[str] = frozenset({"IncompleteStreamError"}), +) -> MagicMock: + """The armed-provider fake skeleton every streaming fake builds on: + provider_name / capabilities / retryable set. ONE spelling, so a new + attribute the seam starts probing lands in every fake at once.""" + provider = MagicMock() + provider.provider_name = name + provider.get_capabilities.return_value = ModelCapabilities() + provider.retryable_error_names = retryable + return provider + + def arm_session( session: Any, *streams: Any, @@ -543,10 +555,7 @@ def arm_session( before the main loop ran. """ session._title_generated = True - provider = MagicMock() - provider.provider_name = name - provider.get_capabilities.return_value = ModelCapabilities() - provider.retryable_error_names = retryable + provider = provider_shell(name, retryable) # One handle PER CREATE (the real adapters' rule): `handles` records # them all, `_armed_handle` is the latest. provider._armed_handle = None @@ -580,10 +589,7 @@ def scripted_provider(chunks: list[StreamChunk]) -> MagicMock: handle is appended per call, matching the one-handle-per-create behavior of every real adapter. """ - provider = MagicMock() - provider.provider_name = "openai-compatible" - provider.get_capabilities.return_value = ModelCapabilities() - provider.retryable_error_names = frozenset({"IncompleteStreamError"}) + provider = provider_shell() def _create(**kwargs: Any): ref = kwargs.get("cancel_ref") diff --git a/tests/test_midstream_retry.py b/tests/test_midstream_retry.py index d6326d0a..22d86dfa 100644 --- a/tests/test_midstream_retry.py +++ b/tests/test_midstream_retry.py @@ -810,10 +810,11 @@ class TestRecreateWindowClassification: assert ui.of("content").count(flushed) == 1 def test_wire_preparation_failure_never_touches_backend_health(self, tmp_db): - """A deterministic lowering failure is a session-data fault: no - dispatch, no health record, no fallback walk — the typed - ``WirePreparationError`` rides to the fatal formatter's dedicated - branch.""" + """A lowering failure is a session-data fault: no dispatch and no + health record on ANY lane — but it DOES walk the fallbacks, since + prepare is lane-variant and another lane's posture may serve the + turn. When none does, the typed ``WirePreparationError`` rides to + the fatal formatter's dedicated branch.""" ui = RecordingUI() session = _make_session(ui) provider = arm_session(session, _good_stream("unreached")) @@ -823,7 +824,7 @@ class TestRecreateWindowClassification: session._registry = registry with ( patch.object(session, "_get_health_tracker", return_value=tracker), - patch.object(session, "_try_fallback_lane") as fb_spy, + patch.object(session, "_try_fallback_lane", return_value=None) as fb_spy, patch.object( session, "_prepare_wire_messages", side_effect=ValueError("malformed turn 7") ), @@ -832,12 +833,67 @@ class TestRecreateWindowClassification: session.send("test") tracker.record_failure.assert_not_called() - fb_spy.assert_not_called() + fb_spy.assert_called_once() provider.create_streaming.assert_not_called() errors = ui.of("error") assert errors and "stored history" in errors[-1] assert "malformed turn 7" in errors[-1] + def test_fallback_prep_fault_continues_walk(self, tmp_db): + """A prep fault on one lane must not abort the walk: the next + alias may still serve the turn (prepare is lane-variant).""" + from tests._session_helpers import make_result + + session = _make_session(RecordingUI()) + registry = MagicMock() + registry.fallback = ["a", "b"] + session._registry = registry + served = make_result(content="ok") + tracker = MagicMock() + with ( + patch.object(session, "_get_health_tracker", return_value=tracker), + patch.object( + session, + "_model_turn_with_retry", + side_effect=WirePreparationError("primary prep fault"), + ), + patch.object(session, "_try_fallback_lane", side_effect=[None, served]) as fb, + ): + from turnstone.core.session import _StreamTurnConsumer + + consumer = _StreamTurnConsumer(session, 0) + result = session._model_turn_with_fallback(consumer, lambda w, lane: w, 0) + + assert result is served + assert fb.call_count == 2 + tracker.record_failure.assert_not_called() + + def test_fallback_prep_fault_records_no_health(self, tmp_db): + ui = RecordingUI() + session = _make_session(ui) + registry = MagicMock() + registry.resolve_binding.return_value = (MagicMock(), "m", None, MagicMock(), None) + fb_tracker = MagicMock() + session._registry = registry + session._health_registry = MagicMock() + session._health_registry.get_tracker_for_alias.return_value = fb_tracker + with ( + patch.object(session, "_build_main_lane", return_value=MagicMock()), + patch.object( + session, + "_model_turn_with_retry", + side_effect=WirePreparationError("fold blew up"), + ), + ): + from turnstone.core.session import _StreamTurnConsumer + + consumer = _StreamTurnConsumer(session, 0) + out = session._try_fallback_lane("fb", consumer, lambda w, lane: w, 0) + + assert out is None + fb_tracker.record_failure.assert_not_called() + assert any("Fallback fb also failed: WirePreparationError" in i for i in ui.of("info")) + class TestDebugDumpLatch: """The debug request dump prints once per ``_stream_response`` diff --git a/turnstone/core/model_turn.py b/turnstone/core/model_turn.py index 2139f396..a2d63be8 100644 --- a/turnstone/core/model_turn.py +++ b/turnstone/core/model_turn.py @@ -492,19 +492,22 @@ def lane_thinking_suppressed(lane: ModelLane) -> bool: ) -def lane_scans_inline_reasoning(lane: ModelLane | None) -> bool: +def caps_scan_inline_reasoning(caps: ModelCapabilities | None) -> bool: """THE inline tag-scan gate — the single spelling of the #978 rule. - Scan ```` tags out of the content stream unless the lane's + Scan ```` tags out of the content stream unless the capabilities declare that the server segregates reasoning itself - (``server_parses_reasoning``); no capabilities, or no lane yet, keeps - the passthrough-server default: scan. Shared by the drain seam and - the interactive display splitter so the two readings of one stream - cannot disagree. + (``server_parses_reasoning``); no declaration keeps the + passthrough-server default: scan. Shared by the drain seam, the + interactive display splitter, and the title peel, so no two + consumers can read one stream differently. """ - return ( - lane is None or lane.capabilities is None or not lane.capabilities.server_parses_reasoning - ) + return caps is None or not caps.server_parses_reasoning + + +def lane_scans_inline_reasoning(lane: ModelLane | None) -> bool: + """:func:`caps_scan_inline_reasoning` over a lane (no lane yet = scan).""" + return caps_scan_inline_reasoning(lane.capabilities if lane is not None else None) def lane_without_thinking(lane: ModelLane) -> ModelLane: @@ -962,10 +965,10 @@ def model_turn( reads the error at all, which is what keeps a Stop mid-summary off the red-error path. - *prepare_wire* is the caller's OWN deterministic lowering (called - with the serving lane, so per-lane capability posture is available), - composed - after the seam passes and before the Phase-5 attach: the main loop's + *prepare_wire* is the caller's OWN deterministic lowering, called + with the serving lane so per-lane capability posture is available, + composed after the seam passes and before the Phase-5 attach: the + main loop's system-message prepend, sender labels, capability-sensitive system-turn fold, empty-user drop, and orphan repair live here, each a ``lowering.py``-composed pass. It must be pure lowering — no diff --git a/turnstone/core/session.py b/turnstone/core/session.py index aa90fde1..4e0edeff 100644 --- a/turnstone/core/session.py +++ b/turnstone/core/session.py @@ -143,6 +143,7 @@ from turnstone.core.model_turn import ( ModelLane, ModelTurnResult, WirePreparationError, + caps_scan_inline_reasoning, create_provider, finalize_provider_blocks, folds_trailing_info, @@ -1817,6 +1818,16 @@ class BackendAuthUnavailableError(RuntimeError): """A fail-closed dynamic model credential could not be resolved.""" +# Errors that carry their own remediation and must surface AS THEMSELVES: +# the re-issue ladder never masks them behind an earlier stream death. +# (Walk policy stays per-class — an auth refusal aborts the walk, a +# wire-preparation fault continues it — only the mask reads this set.) +_SELF_SURFACING_ERRORS: tuple[type[Exception], ...] = ( + BackendAuthUnavailableError, + WirePreparationError, +) + + def _mint_refusal_cause( prefix: str, alias: str, @@ -4258,7 +4269,7 @@ class ChatSession: # (``server_parses_reasoning``): there a close tag in content # IS quoted prose, and cutting would eat a title that mentions # it. See ``_TITLE_*``. - if not self._get_capabilities().server_parses_reasoning: + if caps_scan_inline_reasoning(self._get_capabilities()): _cut = max( (raw.rfind(_t) + len(_t) for _t in ThinkTagSplitter.CLOSE_TAGS if _t in raw), default=0, @@ -5753,9 +5764,8 @@ class ChatSession: return ( f"Preparing the request from this conversation's history failed: " f"{detail}. This is a fault in the session's stored history, not " - f"in the {model_label} backend (no fallback was tried and backend " - f"health is unaffected). /compact may clear a malformed turn; " - f"please report this." + f"in the {model_label} backend (backend health is unaffected). " + f"/compact may clear a malformed turn; please report this." ) # Context overflow — matched by text, because it arrives as BadRequestError @@ -6257,18 +6267,16 @@ class ChatSession: # Explicit fail-closed policy: never reinterpret an authentication # refusal as backend health and never route it to a static fallback. raise - except WirePreparationError: - # No health record and no fallback walk: every lane would - # re-run the same deterministic passes, and N recorded - # failures would paint a cluster-wide outage over one - # session's malformed history. - raise except Exception as primary_err: if consumer.attempt_armed: # Mid-stream death: the re-issue ladder owns it (UI finalize, # backoff, discard, full re-create) — not the fallback walk. raise - if tracker: + # A wire-preparation fault is the session's data, never backend + # health — but it DOES enter the walk: prepare runs per lane + # (fold posture follows lane.capabilities), so another lane's + # posture may serve a turn the primary's could not. + if tracker and not isinstance(primary_err, WirePreparationError): tracker.record_failure() if not self._registry or not self._registry.fallback: raise @@ -6335,15 +6343,16 @@ class ChatSession: return self._model_turn_with_retry( fb_lane, fb_tracker, consumer, prepare_wire, my_generation ) - except (BackendAuthUnavailableError, WirePreparationError): - # An auth refusal is fail-closed policy and a wire-preparation - # failure is the caller's data bug: neither is this fallback's - # health signal. + except BackendAuthUnavailableError: + # Fail-closed policy — never another lane's business. raise except Exception as fb_err: if consumer.attempt_armed: raise - if fb_tracker: + # This lane's wire-preparation fault is not its health signal, + # and it must not abort the walk: prepare is lane-variant, so + # the next alias may still serve the turn. + if fb_tracker and not isinstance(fb_err, WirePreparationError): fb_tracker.record_failure() # Class name only in the UI line: a ConnectError's text can # carry a credential-bearing base_url, and this string lands in @@ -8354,7 +8363,7 @@ class ChatSession: # base_url verbatim. if ( last_stream_death is None - or isinstance(e, (BackendAuthUnavailableError, WirePreparationError)) + or isinstance(e, _SELF_SURFACING_ERRORS) or _is_ctx_overflow(e) ): raise diff --git a/turnstone/core/session_ui_base.py b/turnstone/core/session_ui_base.py index 494c42bd..b6b9c066 100644 --- a/turnstone/core/session_ui_base.py +++ b/turnstone/core/session_ui_base.py @@ -2122,7 +2122,7 @@ class SessionUIBase: # sibling or the ``__budget_override__`` pseudo-tool makes # candidates < pending, AND # - call_ids must be unique — some local models emit duplicate - # non-empty tool-call ids (``_ensure_tool_call_ids`` only fills + # non-empty tool-call ids (``model_turn.ensure_tool_call_ids`` only fills # MISSING ones), which collapse in the ``needed`` set and would let # one verdict clear two distinct calls (their args differ). # Either mismatch → hold the whole batch for a human. Checked diff --git a/turnstone/core/streaming_text.py b/turnstone/core/streaming_text.py index d0c61351..6365931d 100644 --- a/turnstone/core/streaming_text.py +++ b/turnstone/core/streaming_text.py @@ -93,7 +93,7 @@ class ThinkTagSplitter: if not self.pending: return "" tail = partial_tag_tail(self.pending) - closeable = self.pending[: len(self.pending) - len(tail)] if tail else self.pending + closeable = self.pending.removesuffix(tail) if closeable: self._emit(closeable, self.in_think) self.pending = ""