stage #4524: clarify 409-terminal (id-guarded, loading-cleared) + SSE-notify on expiry (#4504) + v0.51.534 CHANGELOG

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

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:

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);
   }

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."
+        )