diff --git a/CHANGELOG.md b/CHANGELOG.md index 653f8b91..1d511a93 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -3,6 +3,12 @@ ## [Unreleased] +## [v0.51.534] — 2026-06-20 — Release SS (clarify prompt no longer bricks the session on expiry) + +### Fixed + +- **An expired clarify prompt no longer locks up the session (#4504).** When a clarify prompt's countdown hit zero, the agent cleared server state but the browser kept the clarify card docked and the composer locked, and the only escape — submitting — was rejected with `409 {stale: true}`, leaving the session stuck until a reload. The client now treats a 409 on submit as terminal: it re-enables the composer, hands the typed draft back to it, and dismisses the stale card. The dismissal is guarded by the same `clarify_id` check the success path uses, so a late 409 for an expired prompt can't tear down a newer prompt that already rendered (#2639 preserved); and the composer's loading state is cleared before the draft is stashed so the typed answer is never silently dropped. `clear_pending` also emits an SSE notify for any future stream subscriber. Thanks @Sanjays2402. + ## [v0.51.533] — 2026-06-20 — Release SR (restart the gateway from the agent-health alert) ### Added diff --git a/api/clarify.py b/api/clarify.py index f43d40dd..b42b0f13 100644 --- a/api/clarify.py +++ b/api/clarify.py @@ -61,9 +61,21 @@ def unregister_gateway_notify(session_key: str) -> None: def clear_pending(session_key: str) -> int: - """Clear any pending clarify prompts for the session without removing the callback.""" + """Clear any pending clarify prompts for the session without removing the callback. + + Emits an SSE notify (``pending=None, total=0``) so any browser subscribed to + the clarify stream takes down its visible card and unlocks the composer. + Without this notify, the silent-timeout path in + ``streaming.py::_clarify_callback_impl`` cleared server state but left the + browser with a stuck card and a locked composer that 409'd on any submit + (#4504). + """ with _lock: entries = _clear_queue_locked(session_key) + # Notify from inside _lock for ordering consistency with submit_pending + # and resolve_clarify*, which also publish under the same lock. + if entries: + _clarify_sse_notify(session_key, None, 0) if entries: publish_session_list_changed("attention_cleared") for entry in entries: diff --git a/static/messages.js b/static/messages.js index c5538a37..2369344e 100644 --- a/static/messages.js +++ b/static/messages.js @@ -5213,15 +5213,55 @@ async function respondClarify(response) { if (typeof setStatus === "function") setStatus(errMsg); } } catch(e) { - // Stale (409) or network error — keep the card and draft visible so the user can retry. + // The server returns 409 with ``stale: true`` for both genuinely-expired + // prompts and wrong-session/next-prompt-loaded races. In both cases the + // server-side ``_pending`` entry for *this* clarify_id is gone, so + // retrying it can never succeed — the prior keep-card-and-draft + // behavior left the user with a permanently 409-ing card and a locked + // composer, with no affordance to dismiss it (#4504). Treat 409 as + // terminal here, but only when the visible card still matches what we + // just submitted: mirroring the success path's ``_clarifyId === clarifyId`` + // guard (codex review P1, #2639) — if a parallel poll already rendered + // the *next* queued prompt B while A's response was in flight, we must + // not tear B down on A's late 409. The SSE/poll path will re-render the + // next prompt's card from scratch via ``showClarifyCard`` either way. + if (e && e.status === 409) { + if (_clarifyId === clarifyId) { + // Same card still showing — dismiss it and rescue the typed draft. + // Order matters: ``_stashClarifyDraft`` (called from + // ``hideClarifyCard``) bails when ``#clarifySubmit`` still carries + // the ``loading`` class set above. Clear loading first, otherwise + // the typed answer is silently dropped (reviewer P1). + _clarifySetControlsDisabled(false, false); + _clarifySessionId = null; + _clarifyId = null; + _clearClarifyPendingForSession(sid); + hideClarifyCard(true, "expired"); + const errMsg = (e.message || "Clarification prompt expired or not found."); + if (typeof setStatus === "function") setStatus("Clarify: " + errMsg); + // ``_stashClarifyDraft('expired')`` already surfaces the actionable + // "Clarification timed out. Your draft was kept in the composer." + // toast when there is a draft to rescue, so we don't double-toast. + return; + } + // A newer prompt is showing (race between user click and SSE/poll). + // Don't dismiss it on this late 409 — just re-enable controls and + // surface the error. The user's draft for the now-stale prompt is + // dropped intentionally; the next prompt has its own input cycle. + _clarifySetControlsDisabled(false, false); + if (typeof setStatus === "function") { + setStatus("Clarify: previous prompt expired — a newer one is showing."); + } + return; + } + // Network / other transient errors — keep the card and draft visible so + // the user can retry once connectivity returns. _clarifySetControlsDisabled(false, false); if (input) { input.value = draft; input.focus(); } - const errMsg = (e && e.status === 409) - ? (e.message || "Clarification prompt expired or not found.") - : ((e && e.message) || "Failed to deliver clarification response."); + const errMsg = (e && e.message) || "Failed to deliver clarification response."; if (typeof setStatus === "function") setStatus("Clarify: " + errMsg); if (typeof showToast === "function") showToast(errMsg, 5000); } diff --git a/tests/test_4504_clarify_stuck_on_expiry.py b/tests/test_4504_clarify_stuck_on_expiry.py new file mode 100644 index 00000000..6f552514 --- /dev/null +++ b/tests/test_4504_clarify_stuck_on_expiry.py @@ -0,0 +1,261 @@ +"""Regression coverage for #4504 — clarify card / composer stuck after expiry. + +Two compounding gaps the user experienced: + + 1. Server-side ``clear_pending(sid)`` ran on silent timeout but emitted no + SSE notify, so the browser never knew the prompt was gone (the visible + card stayed up and the composer stayed locked). + + 2. The client's ``respondClarify`` catch-block treated every 409 (including + ``stale: true``) as retryable, leaving the card + draft visible and the + controls re-enabled — but every retry returned 409, so the session was + permanently stuck. The user had zero affordance to dismiss the card. + +This file pins: + + - ``clear_pending`` notifies SSE subscribers (head=None, total=0) so the + silent-timeout path takes the card down via the existing pending=null + branch in ``_handleClarifyEvent`` if/when an SSE consumer is re-attached. + (As of v0.51.340 the WebUI clarify transport is HTTP-poll only — the SSE + notify is still ordering-correct and drives the sessions-list attention + badge via the pre-existing ``publish_session_list_changed`` call.) + - The client's ``respondClarify`` catch-block, on ``e.status === 409``: + * matches the success path's ``_clarifyId === clarifyId`` guard so a + late 409 for prompt A does not dismiss a rendered prompt B + (#2639-style regression), + * clears the loading/disabled state *before* ``hideClarifyCard`` so + ``_stashClarifyDraft`` does not bail on the loading class (reviewer + P1 from the first review pass — see PR #4524), + * routes the same-id case through ``hideClarifyCard(true, 'expired')`` + so the draft is rescued into the now-unlocked composer. + +The tests intentionally mirror the static-analysis + unit pattern already in +``test_clarify_sse.py`` so they ride the existing clarify suite layout. +""" + +from __future__ import annotations + +import os +import queue + +import pytest + + +_CLARIFY = os.path.join(os.path.dirname(__file__), "..", "api", "clarify.py") +_MESSAGES = os.path.join(os.path.dirname(__file__), "..", "static", "messages.js") + + +def _read(path: str) -> str: + with open(path) as f: + return f.read() + + +# ═════════════════════════════════════════════════════════════════════════════ +# 1. Server-side fix — clear_pending must emit an SSE notify so the silent +# timeout path actually wakes the browser. (Phase A in the issue.) +# ══════════════════════════════════════════════════════════════════════════════ +@pytest.fixture() +def clarify_mod(): + from api import clarify + return clarify + + +@pytest.fixture(autouse=True) +def _cleanup_subscribers(clarify_mod): + yield + clarify_mod._clarify_sse_subscribers.clear() + clarify_mod._gateway_queues.clear() + clarify_mod._pending.clear() + + +class TestClearPendingNotifiesSSE: + """clear_pending must push (head=None, total=0) so any SSE subscriber + (or future SSE reattachment) gets a take-down event for the card.""" + + def test_clear_pending_pushes_none_head_to_subscriber(self, clarify_mod): + sid = "sess-4504-a" + # Pre-load a pending clarify entry the way submit_pending does. + entry = clarify_mod.submit_pending(sid, {"question": "y/n?"}) + assert entry is not None + # Subscribe AFTER the submit so we don't have to drain its notify. + sub = clarify_mod.sse_subscribe(sid) + # Now expire it. + cleared = clarify_mod.clear_pending(sid) + assert cleared == 1 + # We should receive a clear push (head=None, total=0). + msg = sub.get(timeout=1.0) + assert msg == {"pending": None, "pending_count": 0}, ( + "clear_pending must emit a head=None / total=0 SSE notify so the " + "silent-timeout path tells any SSE subscriber to take the card " + "down (#4504)." + ) + + def test_clear_pending_no_op_does_not_notify(self, clarify_mod): + sid = "sess-4504-b" + sub = clarify_mod.sse_subscribe(sid) + # No pending entry → no clear → no spurious notify. + cleared = clarify_mod.clear_pending(sid) + assert cleared == 0 + with pytest.raises(queue.Empty): + sub.get(timeout=0.1) + + def test_clear_pending_unblocks_caller_event(self, clarify_mod): + """The existing event.set() on the cleared entry stays in place.""" + sid = "sess-4504-c" + entry = clarify_mod.submit_pending(sid, {"question": "ok?"}) + assert not entry.event.is_set() + clarify_mod.clear_pending(sid) + assert entry.event.is_set(), ( + "Clearing must still unblock the agent-side wait() so the " + "_clarify_callback_impl timeout branch returns its fallback string." + ) + + +class TestClarifyClearPendingSourceMarkers: + """Static-analysis pin for the Phase A fix (#4504).""" + + def test_clear_pending_calls_notify(self): + src = _read(_CLARIFY) + # The fix adds _clarify_sse_notify(session_key, None, 0) inside + # clear_pending's _lock block. + assert "_clarify_sse_notify(session_key, None, 0)" in src, ( + "clear_pending must invoke _clarify_sse_notify with None head so " + "the silent-timeout path notifies SSE subscribers (#4504)." + ) + + +# ══════════════════════════════════════════════════════════════════════════════ +# 2. Client-side fix — respondClarify catch must treat 409 as terminal +# *only* for the visible card, and rescue the typed draft into the +# composer in the same-id case. (Phase B in the issue, plus reviewer P1s.) +# ══════════════════════════════════════════════════════════════════════════════ +class TestRespondClarify409Terminal: + """The 409 branch must guard on id, clear loading first, and dismiss only + the visible card — never a newer prompt B that rendered while A's + response was in flight.""" + + @pytest.fixture(autouse=True) + def _load_js(self): + self.js = _read(_MESSAGES) + + def _respond_clarify_body(self) -> str: + start = self.js.index("async function respondClarify(") + # The function ends at the next top-level "function " or "var ". + end_fn = self.js.index("\nfunction ", start + 1) + end_var = self.js.index("\nvar ", start + 1) + end = min(end_fn, end_var) + return self.js[start:end] + + def _catch_409_branch(self, body: str) -> str: + """Return the source of the 409 branch in respondClarify's catch.""" + idx = body.index("e.status === 409") + # Pull a generous window covering both the same-id and different-id + # paths inside the 409 branch, before the network-error fallback. + return body[idx : idx + 1500] + + def test_409_routes_to_hide_clarify_card_expired(self): + body = self._respond_clarify_body() + assert "e.status === 409" in body, ( + "respondClarify catch should branch on e.status === 409 to handle " + "the stale/expired case distinctly from network errors (#4504)." + ) + assert 'hideClarifyCard(true, "expired")' in body, ( + "On 409 (same-id) the card must be dismissed via " + "hideClarifyCard(true, 'expired') so _stashClarifyDraft('expired') " + "routes the draft into the now-unlocked composer (#4504)." + ) + + def test_409_clears_loading_before_hide_so_draft_is_rescued(self): + """Reviewer P1: _stashClarifyDraft bails when #clarifySubmit still + carries the ``loading`` class set by _clarifySetControlsDisabled(true, + true) at the top of respondClarify. The 409 same-id branch must + call ``_clarifySetControlsDisabled(false, false)`` *before* + ``hideClarifyCard(true, "expired")`` — otherwise the typed answer is + silently dropped and no toast fires.""" + body = self._respond_clarify_body() + branch = self._catch_409_branch(body) + # Both calls must be present. + assert "_clarifySetControlsDisabled(false, false)" in branch, ( + "409 same-id branch must clear the loading/disabled state so the " + "_stashClarifyDraft loading-class guard does not bail (reviewer P1)." + ) + assert 'hideClarifyCard(true, "expired")' in branch + # And the clear MUST precede the hide. + clear_idx = branch.index("_clarifySetControlsDisabled(false, false)") + hide_idx = branch.index('hideClarifyCard(true, "expired")') + assert clear_idx < hide_idx, ( + "_clarifySetControlsDisabled(false, false) must run before " + "hideClarifyCard(true, 'expired') in the 409 same-id branch — " + "otherwise _stashClarifyDraft bails on the loading class and the " + "user's typed draft is silently dropped (PR #4524 reviewer P1)." + ) + + def test_409_is_guarded_by_clarify_id_match(self): + """Reviewer P1: the success path is gated by ``_clarifyId === clarifyId`` + so a parallel poll that already rendered the next queued prompt B + isn't clobbered (#2639). The 409 branch must follow the same + contract — otherwise A's late 409 will dismiss B.""" + body = self._respond_clarify_body() + branch = self._catch_409_branch(body) + assert "_clarifyId === clarifyId" in branch, ( + "409 branch must mirror the success path's _clarifyId === " + "clarifyId guard so a late 409 for prompt A does not dismiss a " + "newer prompt B that rendered while A's response was in flight " + "(#2639 regression flagged in PR #4524 review)." + ) + + def test_409_same_id_branch_clears_session_cache(self): + """In the same-id case the stale per-session pending cache must be + cleared so the SSE/poll path can't re-render the just-dismissed card. + Mirrors the success path's _clearClarifyPendingForSession(sid) call.""" + body = self._respond_clarify_body() + branch = self._catch_409_branch(body) + assert "_clearClarifyPendingForSession(sid)" in branch, ( + "409 same-id branch must call _clearClarifyPendingForSession(sid) " + "so the cached pending entry for this session cannot re-render " + "the card we just dismissed (mirrors success-path contract)." + ) + + def test_409_different_id_branch_does_not_dismiss(self): + """When _clarifyId != clarifyId at the time the 409 returns, a newer + prompt B is showing. The branch must re-enable controls and return + without touching the visible card (no hideClarifyCard, no + _clearClarifyPendingForSession).""" + body = self._respond_clarify_body() + branch = self._catch_409_branch(body) + # The 409 branch should dismiss the card in exactly one arm — the + # same-id arm — so the different-id arm leaves the visible newer + # prompt alone (#2639-style regression P1). + assert branch.count('hideClarifyCard(true, "expired")') == 1, ( + "409 branch must dismiss the card in exactly one arm (the " + "same-id arm); the different-id arm must leave the visible " + "newer prompt alone (PR #4524 reviewer P1)." + ) + # Both arms (same-id and different-id) must call + # _clarifySetControlsDisabled(false, false) so the user can keep + # interacting with whichever prompt is now visible. + assert branch.count("_clarifySetControlsDisabled(false, false)") >= 2, ( + "Both 409 arms (same-id and different-id) must call " + "_clarifySetControlsDisabled(false, false) so the user can keep " + "interacting with whichever prompt is now visible." + ) + + def test_409_early_returns_before_network_fallback(self): + body = self._respond_clarify_body() + branch = self._catch_409_branch(body) + # The 409 branch must early-return so the network-error fallback + # below it does not double-touch state. + assert "return;" in branch, ( + "The 409 branch must early-return so the network-error fallback " + "does not re-enable the controls of a card we just dismissed." + ) + + def test_non_409_still_keeps_card_visible(self): + """Network/transient errors keep the existing retry-friendly behavior.""" + body = self._respond_clarify_body() + # The non-409 branch still calls _clarifySetControlsDisabled(false, false) + # and re-focuses the input — that path must remain reachable. + assert "_clarifySetControlsDisabled(false, false)" in body, ( + "Non-409 transient errors must still re-enable the controls so " + "the user can retry once connectivity returns." + )