From 67c9277ed47a9ec8984e6db126bb9e8a64aaa163 Mon Sep 17 00:00:00 2001 From: nesquena-hermes Date: Sat, 11 Jul 2026 01:39:19 +0000 Subject: [PATCH] fix(send): clear composer immediately after capture to prevent stalled-text race (salvage of #4750) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Salvaged from #4750 by @harryazj — pulling ONLY the atomic composer capture-and-clear fix from the larger PR. Prevents a re-entrant/interrupt-mode send from re-reading stale textarea DOM and double-sending. Co-authored-by: harryazj --- static/messages.js | 19 +- tests/test_composer_capture_clear_race.py | 188 ++++++++++++++++++ ...issue5472_preserve_draft_on_failed_send.py | 45 ++++- 3 files changed, 239 insertions(+), 13 deletions(-) create mode 100644 tests/test_composer_capture_clear_race.py diff --git a/static/messages.js b/static/messages.js index f3a6a0c2b..3f7fe9060 100644 --- a/static/messages.js +++ b/static/messages.js @@ -1552,6 +1552,20 @@ async function send(){ const activeSid=S.session.session_id; _sendInProgressSid=activeSid; + // Salvage of #4750 (@harryazj): capture the composer text and clear the + // textarea NOW — immediately after capture and BEFORE the uploadPendingFiles() + // / forced-skill-directive awaits below. send() re-reads the LIVE composer when + // it is re-entered while a send is in flight (the _sendInProgress guard at the + // top of this function reads _composerTextWithPendingSelections()). If we + // cleared only after the async work — as the pre-fix code did, down at the + // _clearComposerDraft site — a re-entrant/interrupt-mode send during the upload + // window would read the still-populated DOM and double-submit the same message. + // _submittedDraftTextForClear is the sole authority for the send-time draft + // signature from here down; no code path below re-reads $('msg').value on the + // happy path. + const _submittedDraftTextForClear=$('msg').value||''; + $('msg').value='';autoResize(); + setComposerStatus(S.pendingFiles&&S.pendingFiles.length?'Uploading…':''); let uploaded=[]; try{uploaded=await uploadPendingFiles();} @@ -1589,9 +1603,10 @@ async function send(){ } } if(!msgText){setComposerStatus('Nothing to send');return;} - const _submittedDraftTextForClear=$('msg').value||''; + // Composer textarea + _submittedDraftTextForClear were already captured and + // cleared immediately after capture (above, salvage of #4750) to close the + // re-entrant double-send race. Only the files snapshot is finalized here. const _submittedDraftFilesForClear=Array.isArray(_failedSendFilesSnapshot)?[..._failedSendFilesSnapshot]:[]; - $('msg').value='';autoResize(); // Clear persisted composer draft since message was sent. Capture the promise // so the #5472 failed-send restore can chain its re-persist AFTER this clear // resolves — otherwise the two same-origin POSTs (clear text:'' then restore diff --git a/tests/test_composer_capture_clear_race.py b/tests/test_composer_capture_clear_race.py new file mode 100644 index 000000000..0c1815327 --- /dev/null +++ b/tests/test_composer_capture_clear_race.py @@ -0,0 +1,188 @@ +"""Regression coverage for the composer capture-and-clear race (salvage of #4750). + +Bug (salvaged from #4750, credit @harryazj): ``send()`` in ``static/messages.js`` +captured the composer text early but only wiped the textarea +(``$('msg').value=''``) LATE — after ``await uploadPendingFiles()`` and the +forced-skill-directive await. ``send()`` is re-entrant: while a send is in +flight, a second invocation (e.g. an interrupt-mode / ``busy_input_mode`` drain, +or a fast second Enter) hits the ``if (_sendInProgress)`` guard at the top and +re-reads the LIVE composer via ``_composerTextWithPendingSelections()``. Because +the textarea still held the original text during the async upload window, the +re-entrant guard read the stale DOM and QUEUED the same message again -> +double-submit. + +Fix: capture the composer value and clear the textarea IMMEDIATELY after capture, +BEFORE any await. Once cleared, a re-entrant read sees an empty composer and the +guard's ``if(_text && _targetSid)`` short-circuits -> no duplicate queue. + +This module verifies BOTH: + 1. (static) the capture+wipe is ordered before the upload await in send(), and + 2. (behavioral, via node's ``vm``) the REAL re-entrancy guard block extracted + from send() does NOT re-queue when the composer was already cleared, and + WOULD have re-queued had the composer still held the stale text. +""" +import json +import shutil +import subprocess +import textwrap +from pathlib import Path + +import pytest + +ROOT = Path(__file__).resolve().parents[1] +MESSAGES_JS = ROOT.joinpath("static", "messages.js").read_text(encoding="utf-8") + + +# --------------------------------------------------------------------------- +# Static ordering assertions +# --------------------------------------------------------------------------- + +def _function_body(src: str, name: str) -> str: + marker = f"function {name}" + start = src.index(marker) + brace = src.index("{", start) + depth = 1 + i = brace + 1 + while depth and i < len(src): + if src[i] == "{": + depth += 1 + elif src[i] == "}": + depth -= 1 + i += 1 + return src[brace + 1 : i - 1] + + +def test_composer_captured_and_cleared_before_upload_await(): + """The fix: capture + textarea wipe must run before uploadPendingFiles().""" + body = _function_body(MESSAGES_JS, "send") + + capture_idx = body.index("const _submittedDraftTextForClear=$('msg').value||'';") + # The wipe sits immediately after the capture. + wipe_rel = body.index("$('msg').value='';autoResize();", capture_idx) + assert wipe_rel - capture_idx < 120, "the textarea wipe must sit immediately after the capture" + + upload_idx = body.index("uploaded=await uploadPendingFiles();") + directive_await_idx = body.index("const _directivePayload = await _pending.promise;") + + assert capture_idx < upload_idx, ( + "composer capture+clear must happen BEFORE the uploadPendingFiles() await " + "(salvage of #4750 — closes the re-entrant double-send race)" + ) + assert capture_idx < directive_await_idx, ( + "composer capture+clear must happen BEFORE the forced-skill-directive await too" + ) + + +def test_reentrancy_guard_reads_live_composer(): + """Sanity: the in-flight guard reads the LIVE composer (why the clear must be early).""" + body = _function_body(MESSAGES_JS, "send") + guard_idx = body.index("if (_sendInProgress) {") + guard_block = body[guard_idx : body.index("_sendInProgress = true;", guard_idx)] + assert "_composerTextWithPendingSelections().trim()" in guard_block, ( + "the re-entrant guard reads the live composer, so the composer must be " + "cleared before the first await to avoid a stale double-send" + ) + assert "queueSessionMessage(" in guard_block, ( + "the re-entrant guard queues the message it reads from the live composer" + ) + + +# --------------------------------------------------------------------------- +# Behavioral test — run the REAL re-entrancy guard against a cleared composer +# --------------------------------------------------------------------------- + +def _extract_reentrancy_guard() -> str: + """Slice the real `if (_sendInProgress) { ... }` guard block out of send().""" + body = _function_body(MESSAGES_JS, "send") + start = body.index("if (_sendInProgress) {") + # Balance braces from the guard's opening brace to its close. + brace = body.index("{", start) + depth = 1 + i = brace + 1 + while depth and i < len(body): + if body[i] == "{": + depth += 1 + elif body[i] == "}": + depth -= 1 + i += 1 + return body[start:i] + + +def _run_reentrant_guard_in_node(composer_value: str): + """Execute the REAL re-entrancy guard with a given live-composer value. + + Returns the list of queueSessionMessage calls the guard made. A non-empty + list means the guard re-queued (double-submit); an empty list means it + short-circuited on an empty composer (the fix's post-clear state). + """ + node = shutil.which("node") + if not node: # pragma: no cover + pytest.skip("node not available") + + helper = _function_body(MESSAGES_JS, "_composerTextWithPendingSelections") + guard = _extract_reentrancy_guard() + + harness = textwrap.dedent( + """ + const queued = []; + const state = { input: { value: %(composer_value)s } }; + const $ = (id) => (id === 'msg' ? state.input : null); + global.document = { getElementById: (id) => $(id) }; + // No pending inline selections in this scenario. + const _pendingSelections = []; + function _formatSelectedTextReplyQuote(t){ return t; } + // Real helper the guard uses to read the live composer. + function _composerTextWithPendingSelections(){%(helper)s} + + // Minimal in-flight state: a send is already running for sid-1. + let _sendInProgress = true; + let _sendInProgressSid = 'sid-1'; + const S = { session: { session_id: 'sid-1' }, pendingFiles: [], activeProfile: 'default' }; + + // Stubs the guard branch touches. + function _chatPayloadModelState(){ return { model: 'm', model_provider: 'p' }; } + function queueSessionMessage(sid, payload){ queued.push({ sid, payload }); } + function _clearComposerAfterQueuedSelectionSend(){ state.input.value = ''; } + function _clearComposerDraft(){} + function updateQueueBadge(){} + function renderTray(){} + function showToast(){} + + // Run the REAL guard block verbatim. + (function () { + %(guard)s + })(); + + console.log(JSON.stringify({ queued, composerAfter: state.input.value })); + """ + ) % { + "composer_value": json.dumps(composer_value), + "helper": helper, + "guard": guard, + } + + proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30) + assert proc.returncode == 0, f"node harness failed: {proc.stderr}" + return json.loads(proc.stdout.strip()) + + +def test_reentrant_send_does_not_requeue_after_composer_cleared(): + """With the fix, the composer is EMPTY when the re-entrant guard runs, so it + must NOT queue a duplicate of the stale text.""" + out = _run_reentrant_guard_in_node("") + assert out["queued"] == [], ( + "a re-entrant send must not re-queue anything once the composer has been " + "cleared by the in-flight send (salvage of #4750)" + ) + + +def test_reentrant_send_would_double_submit_if_composer_not_cleared(): + """Sanity / bug demonstration: had the composer NOT been cleared before the + async window (the pre-fix behaviour), the re-entrant guard reads the stale + DOM text and queues a DUPLICATE — exactly the double-send this fix prevents.""" + out = _run_reentrant_guard_in_node("hello world") + assert len(out["queued"]) == 1, ( + "the stale-composer scenario must reproduce the double-submit the fix removes" + ) + assert out["queued"][0]["payload"]["text"] == "hello world" + assert out["queued"][0]["sid"] == "sid-1" diff --git a/tests/test_issue5472_preserve_draft_on_failed_send.py b/tests/test_issue5472_preserve_draft_on_failed_send.py index 42571116f..76518477a 100644 --- a/tests/test_issue5472_preserve_draft_on_failed_send.py +++ b/tests/test_issue5472_preserve_draft_on_failed_send.py @@ -93,23 +93,46 @@ def test_error_branch_restores_original_snapshot_not_mutated_payload(): def test_send_still_clears_composer_on_the_happy_path(): - # Anchor to the MAIN-path clear specifically (the send-time composer wipe + - # persisted-draft clear), not a bare `$('msg').value=''` string that also - # appears at ~13 other sites (slash returns, bundle-error paths). This - # assertion must actually guard the main send path's clear. (Opus #5484 NIT.) + # Anchor to the MAIN-path persisted-draft clear specifically (not a bare + # `$('msg').value=''` string that also appears at ~13 other sites — slash + # returns, bundle-error paths). This assertion must actually guard the main + # send path's clear. main_clear = ( "if (activeSid && typeof _clearComposerDraft === 'function') " "_composerDraftClearPromise=_clearComposerDraft(activeSid,_submittedDraftTextForClear,_submittedDraftFilesForClear);" ) assert main_clear in MESSAGES_JS, "main send path must clear the persisted draft at send time" - # The composer textarea wipe must sit immediately above that clear. - clear_idx = MESSAGES_JS.find(main_clear) - window_before = MESSAGES_JS[clear_idx - 700:clear_idx] - assert "const _submittedDraftTextForClear=$('msg').value||'';" in window_before - assert "const _submittedDraftFilesForClear=Array.isArray(_failedSendFilesSnapshot)?[..._failedSendFilesSnapshot]:[];" in window_before - assert "$('msg').value='';autoResize();" in window_before, ( - "the main-path composer wipe must precede the persisted-draft clear" + + # Salvage of #4750: the composer textarea capture + wipe was moved UP to run + # immediately after capture (right after `_sendInProgressSid=activeSid;`) and + # BEFORE `uploadPendingFiles()` / the forced-skill-directive await — so a + # re-entrant/interrupt-mode send during the async window can't re-read the + # still-populated DOM and double-submit. Verify that ordering here. + capture = "const _submittedDraftTextForClear=$('msg').value||'';" + assert capture in MESSAGES_JS, "send() must still capture the send-time draft text" + capture_idx = MESSAGES_JS.index(capture) + # The textarea wipe sits immediately after the capture (same 120-char window). + window_after_capture = MESSAGES_JS[capture_idx : capture_idx + 120] + assert "$('msg').value='';autoResize();" in window_after_capture, ( + "the composer textarea wipe must sit immediately after the capture" ) + upload_idx = MESSAGES_JS.index("uploaded=await uploadPendingFiles();") + clear_idx = MESSAGES_JS.index(main_clear) + # THE FIX: capture+wipe happen before the upload await (closes the race)... + assert capture_idx < upload_idx, ( + "composer must be captured+cleared BEFORE the uploadPendingFiles() await " + "so a re-entrant send can't re-read stale DOM text (salvage of #4750)" + ) + # ...and the captured text is still what feeds the persisted-draft clear. + assert capture_idx < clear_idx, ( + "the captured send-time draft text must precede the persisted-draft clear" + ) + # The files snapshot is finalized just above the persisted-draft clear. + window_before_clear = MESSAGES_JS[clear_idx - 700:clear_idx] + assert ( + "const _submittedDraftFilesForClear=Array.isArray(_failedSendFilesSnapshot)?[..._failedSendFilesSnapshot]:[];" + in window_before_clear + ), "the files snapshot for the persisted-draft clear must precede it" def test_restore_persist_chains_after_the_clear_promise():