From ecea6b38448913ad4c807382001913ef9cc0c960 Mon Sep 17 00:00:00 2001 From: ciregenz Date: Thu, 27 Aug 2026 11:59:53 -0700 Subject: [PATCH] [eric] agents: preflight bounces the router but never declares a credential dead, because it has not dispatched (ENG-414) --- .../apps/agents/manager/run/lane_preflight.py | 39 ++++-- backend/tests/test_lane_preflight.py | 26 ++-- ...est_lane_preflight_never_declares_death.py | 118 ++++++++++++++++++ 3 files changed, 163 insertions(+), 20 deletions(-) create mode 100644 backend/tests/test_lane_preflight_never_declares_death.py diff --git a/backend/apps/agents/manager/run/lane_preflight.py b/backend/apps/agents/manager/run/lane_preflight.py index 78a2faff..ddeb6a6e 100644 --- a/backend/apps/agents/manager/run/lane_preflight.py +++ b/backend/apps/agents/manager/run/lane_preflight.py @@ -13,9 +13,17 @@ So this looks first, and it tries to fix it before it complains: `unavailable` stamp and modelLock cooldowns), then DISPATCH ANYWAY and let the turn itself be the verdict. If it goes through, the user never learns anything happened. - sticky-dead, again -> one accurate sentence, immediately. No invented rotation window, no wait - the user has already tried by hand, no promise of a self-heal that cannot - happen. + sticky-dead, bounce + already throttled -> DISPATCH ANYWAY too, and flag the session so that if the turn really does + 401, handle_run_error can say the accurate sentence with no rotation story. + +This file NEVER tells the user a credential is dead, because it has not dispatched and therefore +cannot know. It used to, off the bounce cooldown, and that cooldown is a GLOBAL router-restart +throttle: the branch that meant "permanently dead" actually meant "another chat restarted the +router in the last five minutes". It killed a live build on a working credential while telling the +user "waiting will not clear this one", which was backwards, since waiting out the throttle is +exactly what cleared it (ENG-414). The death verdict lives in ONE place now, downstream of a real +failed dispatch. The bounce is NOT allowed to declare success on its own, and that mistake is worth recording: the first version re-read the health flag afterwards and called a cleared stamp a recovery. But a fresh @@ -108,16 +116,15 @@ async def preflight_lane(resolved_model: str, return None dead = await dead_connection(provider) - if dead is None: - return None - - # Whatever happens next, an auth failure on THIS turn is a dead credential, not a rotation - # window: the router had already given up before we sent anything. + # Written on EVERY pass, both directions. It used to be latched True and never cleared, so one + # blip made every later auth error in that session claim a permanently dead credential. if session is not None: try: - session.lane_credential_dead = True + session.lane_credential_dead = dead is not None except Exception: pass + if dead is None: + return None now = time.time() if now - LAST_BOUNCE.get(provider, 0.0) >= BOUNCE_COOLDOWN_S: @@ -141,8 +148,14 @@ async def preflight_lane(resolved_model: str, # Deliberately no post-bounce health re-read: see the module docstring. Dispatch is the test. return None - return RECONNECT_COPY.get( - provider, - "This model's sign-in expired and could not be renewed. Reconnect it in Settings, then " - "Models. Waiting will not clear this one.", + # The bounce was throttled, and that says NOTHING about this credential: LAST_BOUNCE is global, + # so the timer belongs to whichever OTHER chat restarted the router last. Carding here declared + # a working lane dead and killed a live build (ENG-414), and it broke this file's own rule that + # only a real dispatch can decide. So dispatch. If the credential really is gone, the turn 401s + # and handle_run_error shows the accurate card immediately off `lane_credential_dead`, which is + # the same sentence this used to return, minus the guessing. + logger.info( + f"lane preflight: {provider} looks dead but the router bounce is throttled; dispatching " + "anyway and letting the turn decide" ) + return None diff --git a/backend/tests/test_lane_preflight.py b/backend/tests/test_lane_preflight.py index 31ae23a7..c87f9b6f 100644 --- a/backend/tests/test_lane_preflight.py +++ b/backend/tests/test_lane_preflight.py @@ -78,18 +78,30 @@ def test_a_cleared_stamp_is_never_mistaken_for_a_working_credential(monkeypatch) ) -def test_a_lane_still_dead_on_the_next_ask_gets_one_accurate_sentence(monkeypatch): - """Second encounter inside the cooldown: we already spent a bounce and a turn, so stop - pretending and say the true thing.""" +def test_a_lane_still_dead_inside_the_cooldown_DISPATCHES(monkeypatch): + """CORRECTED 2026-08-27 (ENG-414). This used to assert the opposite, and the assumption it + encoded is the bug: "second encounter inside the cooldown" was read as "we already spent a + bounce and a turn on THIS session". `LAST_BOUNCE` is module-global, so in production it meant + "some other chat bounced recently" and it hard-stopped a live build on a working credential. + + Preflight has not dispatched, so it cannot know. It dispatches and flags the session; the + accurate sentence now comes from handle_run_error after a real 401. The cooldown still holds.""" st = p_providers(monkeypatch, P_DEAD, bounce_result=P_DEAD) assert asyncio.run(lp.preflight_lane("cx/gpt-5.6")) is None - msg = asyncio.run(lp.preflight_lane("cx/gpt-5.6")) - assert msg and "ChatGPT" in msg and "Reconnect" in msg - assert "rotated" not in msg.lower(), "never claim a rotation that did not happen" - assert "no action needed" not in msg.lower(), "there IS action needed; saying otherwise is the bug" + assert asyncio.run(lp.preflight_lane("cx/gpt-5.6")) is None, \ + "a throttled bounce is not evidence about the credential" assert st["bounced"] == 1, "the cooldown holds; one restart, not one per ask" +def test_the_downstream_card_still_refuses_the_rotation_story(): + """What the old assertion above was really protecting: when the card DOES fire, it must not + invent a rotation window or claim no action is needed. That copy moved, it did not soften.""" + for msg in lp.RECONNECT_COPY.values(): + assert "rotated" not in msg.lower(), "never claim a rotation that did not happen" + assert "no action needed" not in msg.lower(), "there IS action needed; saying otherwise is the bug" + assert "reconnect" in msg.lower(), msg + + def test_the_bounce_is_rate_limited(monkeypatch): """A bounce restarts a process every other session shares, so it is once per lane per window, never once per turn.""" st = p_providers(monkeypatch, P_DEAD, bounce_result=P_DEAD) diff --git a/backend/tests/test_lane_preflight_never_declares_death.py b/backend/tests/test_lane_preflight_never_declares_death.py new file mode 100644 index 00000000..cd9c20fe --- /dev/null +++ b/backend/tests/test_lane_preflight_never_declares_death.py @@ -0,0 +1,118 @@ +"""Preflight may bounce the router; it may never say a credential is dead. It has not dispatched. + +ENG-414, production 1.7.9, hit twice in one session and the second one killed a live app build. +`LAST_BOUNCE` is a MODULE-LEVEL dict keyed by provider, and `BOUNCE_COOLDOWN_S` is 300, so the +branch that meant "this credential is permanently dead" actually meant "some OTHER chat restarted +the router in the last five minutes". The user was told "Waiting will not clear this one" and then +proved it wrong by typing "continue" a minute later and watching the run finish. +""" + +import time + +import pytest + +from backend.apps.agents.core.models import AgentSession +from backend.apps.agents.manager.run import lane_preflight as p_pf + +SRC = "backend/apps/agents/manager/run/lane_preflight.py" + + +def p_session() -> AgentSession: + return AgentSession(id="s1", name="c", title="c", model="cc/claude-opus-5") + + +@pytest.fixture(autouse=True) +def p_clean(): + p_pf.LAST_BOUNCE.clear() + yield + p_pf.LAST_BOUNCE.clear() + + +@pytest.mark.asyncio +async def test_a_throttled_bounce_dispatches_instead_of_carding(monkeypatch): + """The exact shape that killed the build: another chat bounced 10s ago, this lane reads dead.""" + monkeypatch.setattr(p_pf, "dead_connection", + lambda provider: p_async({"testStatus": "unavailable", "errorCode": 401})) + p_pf.LAST_BOUNCE["claude"] = time.time() - 10 # another chat, well inside the 300s window + s = p_session() + assert await p_pf.preflight_lane("cc/claude-opus-5", s) is None, \ + "a throttled bounce says nothing about the credential; the turn must still be spent" + + +@pytest.mark.asyncio +async def test_the_session_is_flagged_so_a_REAL_401_can_still_be_honest(monkeypatch): + """Dispatching anyway must not lose the accuracy the old card had.""" + monkeypatch.setattr(p_pf, "dead_connection", + lambda provider: p_async({"testStatus": "unavailable", "errorCode": 401})) + p_pf.LAST_BOUNCE["claude"] = time.time() - 10 + s = p_session() + await p_pf.preflight_lane("cc/claude-opus-5", s) + assert s.lane_credential_dead is True, "handle_run_error keys its accurate card on this" + + +@pytest.mark.asyncio +async def test_a_healthy_lane_CLEARS_the_flag_instead_of_latching_it(monkeypatch): + """It was set True and never reset anywhere, so one blip made every later auth error in that + session claim a permanently dead credential.""" + s = p_session() + s.lane_credential_dead = True + monkeypatch.setattr(p_pf, "dead_connection", lambda provider: p_async(None)) + assert await p_pf.preflight_lane("cc/claude-opus-5", s) is None + assert s.lane_credential_dead is False, "the flag is a live fact, not a latch" + + +@pytest.mark.asyncio +async def test_the_death_verdict_exists_in_exactly_one_place(monkeypatch): + """Two code paths for one verdict is how they disagreed. Only the one downstream of a real + failed dispatch may say it.""" + src = open(SRC).read() + i = src.index("def preflight_lane") + body = src[i:] + assert "RECONNECT_COPY.get(" not in body, "preflight cannot know; it has not dispatched" + handler = open("backend/apps/agents/manager/run/handle_run_error.py").read() + assert "RECONNECT_COPY.get(" in handler, "the accurate card must survive downstream" + assert 'getattr(session, "lane_credential_dead", False)' in handler, \ + "and it must still be gated on the router having given up BEFORE the turn" + + +@pytest.mark.asyncio +async def test_the_bounce_itself_is_still_throttled(monkeypatch): + """A bounce restarts a process every session shares. Dispatching anyway must NOT turn into + bouncing on every turn.""" + p_calls = [] + + async def p_fake_bounce(provider): + p_calls.append(provider) + return True + + monkeypatch.setattr(p_pf, "dead_connection", + lambda provider: p_async({"testStatus": "unavailable", "errorCode": 401})) + import backend.apps.nine_router.bounce_after_connect as p_b + monkeypatch.setattr(p_b, "bounce_router_after_connect", p_fake_bounce) + s = p_session() + await p_pf.preflight_lane("cc/claude-opus-5", s) # first: allowed to bounce + await p_pf.preflight_lane("cc/claude-opus-5", s) # second: throttled + await p_pf.preflight_lane("cc/claude-opus-5", s) # third: throttled + assert len(p_calls) == 1, f"one bounce per {p_pf.BOUNCE_COOLDOWN_S}s window, got {len(p_calls)}" + + +@pytest.mark.asyncio +async def test_a_router_that_does_not_come_back_still_stops_the_turn(monkeypatch): + """The one thing preflight CAN know without dispatching: it just restarted the router and the + router is not listening. Dispatching into that is a guaranteed connection error the user would + read as the model failing.""" + monkeypatch.setattr(p_pf, "dead_connection", + lambda provider: p_async({"testStatus": "unavailable", "errorCode": 401})) + import backend.apps.nine_router.bounce_after_connect as p_b + + async def p_dead_bounce(provider): + return False + + monkeypatch.setattr(p_b, "bounce_router_after_connect", p_dead_bounce) + msg = await p_pf.preflight_lane("cc/claude-opus-5", p_session()) + assert msg and "restarting" in msg + assert "Waiting will not clear" not in msg, "a restarting router is not a dead credential" + + +async def p_async(value): + return value