Gate-found (Opus SHIP-WITH-FIXES; Codex SAFE — split resolved by reading the contract: no dropped-turn, but a real transient start-race flicker). #4424 removed the _sendInProgress guards entirely; that re-opened the #2689 start-race SYMPTOM for the one session mid-send (a /api/sessions poll landing during the /api/chat/start round-trip would blank the just-sent bubble + queue a spurious force-reload). Re-add the guard SCOPED to sid===_sendInProgressSid only, in both _reconcileActiveSessionIdleStateFromList and _purgeStaleInflightEntries — a long-hung session has _sendInProgress===false so it still gets unstuck (#4354), only the in-flight start window is protected. Also fixes Opus's test-masking finding: test_inflight_send_start_race now strips JS comments before asserting + requires a real , so a comment can't green-light the #2689 guard, and adds coverage for the reconcile path. Co-authored-by: bergeouss <bergeouss@users.noreply.github.com>
This commit is contained in:
parent
0a1e5d97a4
commit
f57c0330ec
2 changed files with 47 additions and 3 deletions
|
|
@ -381,9 +381,16 @@ function _isServerIdleSessionRow(s) {
|
|||
|
||||
function _reconcileActiveSessionIdleStateFromList(serverRows) {
|
||||
if (!S || !S.session || !S.session.session_id) return false;
|
||||
// #4354: Removed _sendInProgress guard — server is_streaming=false is authoritative.
|
||||
if (!Array.isArray(serverRows)) return false;
|
||||
const sid=S.session.session_id;
|
||||
// #4354: clear a stuck indicator when the server reports idle — server
|
||||
// is_streaming/active_stream_id is authoritative. BUT skip the ONE session
|
||||
// that is actively mid-send (#2689 start-race): during the /api/chat/start
|
||||
// round-trip the server row is still idle while the client owns the optimistic
|
||||
// turn, so reconciling it here would blank the just-sent bubble + queue a
|
||||
// spurious force-reload. A long-hung session has _sendInProgress===false, so
|
||||
// it still gets unstuck — only the in-flight start window is protected.
|
||||
if (typeof _sendInProgress !== 'undefined' && _sendInProgress && sid === _sendInProgressSid) return false;
|
||||
const serverRow=serverRows.find(s=>s&&s.session_id===sid);
|
||||
if (!serverRow) return false;
|
||||
if (!_isServerIdleSessionRow(serverRow)) return false;
|
||||
|
|
@ -435,7 +442,13 @@ function _purgeStaleInflightEntries() {
|
|||
}
|
||||
}
|
||||
for (const sid of Object.keys(INFLIGHT)) {
|
||||
// #4354: Removed _sendInProgressSid skip — purge stale INFLIGHT even during hung sends.
|
||||
// #4354: purge stale INFLIGHT even for a hung/idle session, BUT skip the one
|
||||
// session actively mid-send (#2689 start-race) — during /api/chat/start the
|
||||
// server row is briefly idle while the client owns the optimistic INFLIGHT
|
||||
// entry; purging it here would drop the in-flight turn's local state.
|
||||
if (typeof _sendInProgress !== 'undefined' && _sendInProgress && sid === _sendInProgressSid) {
|
||||
continue;
|
||||
}
|
||||
if (!sessionsById.has(sid)) {
|
||||
// Session is absent from _allSessions — it was deleted / archived /
|
||||
// filtered and can never stream again, so drop the entry.
|
||||
|
|
|
|||
|
|
@ -39,9 +39,19 @@ def test_send_preserves_optimistic_messages_across_chat_start_await():
|
|||
)
|
||||
|
||||
|
||||
def _strip_js_comments(src: str) -> str:
|
||||
"""Remove // line comments and /* */ block comments so source-grep assertions
|
||||
match real statements, not comment text (a comment must not satisfy a guard
|
||||
regression check). Good enough for these structural checks — not a full JS parser."""
|
||||
import re
|
||||
src = re.sub(r"/\*.*?\*/", "", src, flags=re.DOTALL)
|
||||
src = re.sub(r"(?m)//.*$", "", src)
|
||||
return src
|
||||
|
||||
|
||||
def test_stale_inflight_purge_preserves_current_send_before_stream_id_exists():
|
||||
"""Sidebar cleanup must not delete the active send before /api/chat/start responds."""
|
||||
body = _function_body(SESSIONS_JS, "_purgeStaleInflightEntries")
|
||||
body = _strip_js_comments(_function_body(SESSIONS_JS, "_purgeStaleInflightEntries"))
|
||||
|
||||
assert "_sendInProgress" in body and "_sendInProgressSid" in body, (
|
||||
"_purgeStaleInflightEntries() should skip the current send while start is in progress"
|
||||
|
|
@ -49,6 +59,27 @@ def test_stale_inflight_purge_preserves_current_send_before_stream_id_exists():
|
|||
skip_idx = body.index("_sendInProgress")
|
||||
delete_idx = body.index("delete INFLIGHT[sid];")
|
||||
assert skip_idx < delete_idx, "the current-send skip must run before any purge deletion"
|
||||
# The skip must be a real guarded `continue`, not just a token in a comment (#4354/#2689).
|
||||
assert "continue;" in body[skip_idx:delete_idx], (
|
||||
"the current-send skip must be an actual `continue` before the purge deletion"
|
||||
)
|
||||
|
||||
|
||||
def test_idle_reconcile_preserves_current_send_before_stream_id_exists():
|
||||
"""The list-poll idle reconciler must not clear the session that is mid-send.
|
||||
|
||||
#4354 removed the _sendInProgress guard here to unstick a hung indicator;
|
||||
#2689's start-race protection must still cover the ONE session actively
|
||||
mid-send (server row is briefly idle during /api/chat/start). Verified
|
||||
against comment-stripped source so a comment can't satisfy the check.
|
||||
"""
|
||||
body = _strip_js_comments(_function_body(SESSIONS_JS, "_reconcileActiveSessionIdleStateFromList"))
|
||||
assert "_sendInProgress" in body and "_sendInProgressSid" in body, (
|
||||
"_reconcileActiveSessionIdleStateFromList() must skip the active mid-send session"
|
||||
)
|
||||
guard_idx = body.index("_sendInProgress")
|
||||
clear_idx = body.index("S.busy=false")
|
||||
assert guard_idx < clear_idx, "the mid-send skip must run before the idle clear"
|
||||
|
||||
|
||||
def test_send_clears_stale_busy_state_before_queue_branch():
|
||||
|
|
|
|||
Loading…
Reference in a new issue