* fix(#5472): harden failed-send draft restore — order persist after clear, honest test Two NIT-level follow-ups from the Opus review of #5484 (both on already-shipped v0.51.842 code; neither is a functional break): 1. Draft clear/save ordering. send() fires an unawaited _clearComposerDraft POST (text:'') at send time, and the failed-send restore fired a separate _saveComposerDraftNow POST (text:<draft>). Under HTTP/2 multiplexing those two same-origin writes can be reordered, leaving the SERVER draft empty after a reload (the in-memory composer restore was always fine). send() now captures the clear POST's promise and passes it to the restore helper, which chains the re-persist after the clear resolves (falls back to immediate persist if the promise is absent/rejects). 2. Honest happy-path test. test_send_still_clears_composer_on_the_happy_path asserted a bare $('msg').value='' string that matches ~13 sites, so it would pass even if the MAIN-path clear were removed. It now anchors to the unique main-path _clearComposerDraft line and window-checks the composer wipe immediately precedes it. Tests: +2 (promise-ordering behavioral via node-vm microtask ordering; absent-promise fallback), signature/call assertions updated. 14 in the #5472 file, 93 across the related composer/send/draft/upload surface — all green. * fix(#5472): make deferred draft persist stale-aware (Codex #5488 catch) The promise-ordering fix introduced a regression: the deferred _persist saved the CAPTURED snapshot and its _saveComposerDraftNow clearTimeout cancelled any newer debounced save. So if the user edited the restored composer before the clear POST settled, the delayed persist overwrote their edit with the stale original text (lost on reload). Restructure the helper so the deferred persist is stale-aware at FIRE time: - If the failed session is still visible, re-read the LIVE composer (.value + current S.pendingFiles) instead of the captured snapshot — a post-restore edit is captured, not clobbered. - If we restored the visible session but the user has since switched away, SKIP the stale write entirely (the session-switch save path already saved that session). - Background failure (sid never visible): persist the captured snapshot, since there's no live composer to read — it's the only copy. The visible-composer restore + session guard are unchanged in behavior (moved above the persist). Helper now returns whether it restored the VISIBLE composer. Tests: +3 regression cases (post-restore edit captured; switch-away skip; background snapshot persist), signature/guard assertions updated. 17 in the #5472 file, 87 across the related surface — all green.
This commit is contained in:
nesquena-hermes authored and GitHub committed
1 parent
4e8363035e
commit
5c54fd598c
2 files changed
+267
-32
No files matched your search
+58
-24
@@ -1230,35 +1230,64 @@ function applySessionTitleUpdate(sid, titleText, options={}){
|
||||
// BEFORE slash rewrites (/moa, bundles) mutate the payload and BEFORE
|
||||
// uploadPendingFiles() drains S.pendingFiles — so we restore what the user
|
||||
// actually typed, not the transformed send payload.
|
||||
function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid){
|
||||
function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid, clearPromise){
|
||||
const restore=String(draftText||'');
|
||||
const files=Array.isArray(filesSnapshot)?filesSnapshot.filter(Boolean):[];
|
||||
if(!restore&&!files.length) return false;
|
||||
// Persist the draft for the failed send's own session so it survives a reload
|
||||
// regardless of which session is currently visible. Files staged in the
|
||||
// composer are File objects (not serializable), so only text is persisted;
|
||||
// the in-memory S.pendingFiles restore below covers the immediate re-send.
|
||||
if(sid&&typeof _saveComposerDraftNow==='function'){
|
||||
try{ _saveComposerDraftNow(sid, restore, []); }catch(_){ }
|
||||
}
|
||||
|
||||
// Only mutate the VISIBLE composer / staged tray when the failed send belongs
|
||||
// to the session the user is currently looking at — otherwise a background
|
||||
// send failure would pollute another session's composer. (Codex #5484 catch.)
|
||||
const visibleSid=(S.session&&S.session.session_id)||null;
|
||||
if(sid&&visibleSid&&sid!==visibleSid) return false;
|
||||
const inp=$('msg');
|
||||
if(!inp) return false;
|
||||
// Do not clobber a new message the user began typing during the async window.
|
||||
if(String(inp.value||'').trim()) return false;
|
||||
inp.value=restore;
|
||||
if(typeof autoResize==='function') autoResize();
|
||||
if(typeof updateSendBtn==='function') updateSendBtn();
|
||||
// Re-stage the originally attached files so a one-key resend keeps them.
|
||||
if(files.length){
|
||||
S.pendingFiles=files;
|
||||
if(typeof renderTray==='function') renderTray();
|
||||
const belongsToVisible=!(sid&&visibleSid&&sid!==visibleSid);
|
||||
let restoredVisible=false;
|
||||
if(belongsToVisible){
|
||||
const inp=$('msg');
|
||||
// Do not clobber a new message the user began typing during the async window.
|
||||
if(inp && !String(inp.value||'').trim()){
|
||||
inp.value=restore;
|
||||
if(typeof autoResize==='function') autoResize();
|
||||
if(typeof updateSendBtn==='function') updateSendBtn();
|
||||
// Re-stage the originally attached files so a one-key resend keeps them.
|
||||
if(files.length){
|
||||
S.pendingFiles=files;
|
||||
if(typeof renderTray==='function') renderTray();
|
||||
}
|
||||
restoredVisible=true;
|
||||
}
|
||||
}
|
||||
return true;
|
||||
|
||||
// Persist the failed session's draft so it survives a reload, ordered AFTER the
|
||||
// send-time _clearComposerDraft POST (text:'') resolves — otherwise the two
|
||||
// same-origin writes can be reordered under HTTP/2 multiplexing and leave the
|
||||
// server draft empty. (Opus #5484 NIT.) Because the persist is deferred, it
|
||||
// must be STALE-AWARE at fire time (Codex #5488 catch): if the failed session
|
||||
// is still visible, re-read the LIVE composer so a post-restore edit is
|
||||
// captured rather than clobbered by the original snapshot; if we restored the
|
||||
// visible session but the user has since switched away, skip entirely (the
|
||||
// session-switch save path already persisted this session's composer).
|
||||
if(sid&&typeof _saveComposerDraftNow==='function'){
|
||||
const _persist=()=>{
|
||||
try{
|
||||
const stillVisible=(S.session&&S.session.session_id)===sid;
|
||||
if(stillVisible){
|
||||
const inp=$('msg');
|
||||
const liveText=inp?String(inp.value||''):restore;
|
||||
_saveComposerDraftNow(sid, liveText, S.pendingFiles?[...S.pendingFiles]:[]);
|
||||
} else if(!restoredVisible){
|
||||
// Background failure (sid was never the visible session): no live
|
||||
// composer to read, so persist the captured snapshot — it's the only copy.
|
||||
_saveComposerDraftNow(sid, restore, []);
|
||||
}
|
||||
// else: restored the visible composer, then the user switched away — the
|
||||
// session-switch save path already saved sid's composer; skip stale write.
|
||||
}catch(_){ }
|
||||
};
|
||||
if(clearPromise&&typeof clearPromise.then==='function') clearPromise.then(_persist,_persist);
|
||||
else _persist();
|
||||
}
|
||||
|
||||
return restoredVisible;
|
||||
}
|
||||
|
||||
async function send(){
|
||||
@@ -1550,8 +1579,13 @@ async function send(){
|
||||
if(!msgText){setComposerStatus('Nothing to send');return;}
|
||||
|
||||
$('msg').value='';autoResize();
|
||||
// Clear persisted composer draft since message was sent.
|
||||
if (activeSid && typeof _clearComposerDraft === 'function') _clearComposerDraft(activeSid);
|
||||
// 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
|
||||
// text:<draft>) can be reordered under HTTP/2 multiplexing and leave the
|
||||
// server draft empty after a reload. (Opus #5484 NIT.)
|
||||
let _composerDraftClearPromise=null;
|
||||
if (activeSid && typeof _clearComposerDraft === 'function') _composerDraftClearPromise=_clearComposerDraft(activeSid);
|
||||
const displayText=_slashDisplayTextOverride||text||(uploaded.length?`Uploaded: ${uploadedNames.join(', ')}`:'(file upload)');
|
||||
const userMsg={role:'user',content:displayText,attachments:uploaded.length?uploadedNames:undefined,_ts:Date.now()/1000};
|
||||
S.toolCalls=[]; // clear tool calls from previous turn
|
||||
@@ -1735,7 +1769,7 @@ async function send(){
|
||||
// composer text + attachments (cleared at send time) would otherwise be
|
||||
// lost. Put back the ORIGINAL captured draft (not the mutated /moa/bundle
|
||||
// payload) and re-stage files so the user can re-send without retyping.
|
||||
_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid);
|
||||
_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid, _composerDraftClearPromise);
|
||||
if(typeof clearOptimisticSessionStreaming==='function') clearOptimisticSessionStreaming(activeSid);
|
||||
// Reconcile with server truth after immediately clearing the optimistic spinner.
|
||||
if(typeof renderSessionList==='function') void renderSessionList();
|
||||
|
||||
@@ -48,17 +48,22 @@ def _helper_body() -> str:
|
||||
|
||||
def test_helper_has_new_three_arg_signature_and_guards():
|
||||
body = _helper_body()
|
||||
assert "function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid)" in body
|
||||
assert "function _restoreComposerDraftAfterFailedSend(draftText, filesSnapshot, sid, clearPromise)" in body
|
||||
# No-op when there is nothing to restore (no text AND no staged files).
|
||||
assert "if(!restore&&!files.length) return false;" in body
|
||||
# Session-aware: never mutate a different session's visible composer.
|
||||
assert "const visibleSid=(S.session&&S.session.session_id)||null;" in body
|
||||
assert "if(sid&&visibleSid&&sid!==visibleSid) return false;" in body
|
||||
assert "const belongsToVisible=!(sid&&visibleSid&&sid!==visibleSid);" in body
|
||||
# Never clobber a message the user began typing during the async window.
|
||||
assert "if(String(inp.value||'').trim()) return false;" in body
|
||||
assert "if(inp && !String(inp.value||'').trim()){" in body
|
||||
# Restores text and re-stages files.
|
||||
assert "inp.value=restore;" in body
|
||||
assert "S.pendingFiles=files;" in body
|
||||
# The deferred persist is stale-aware: re-reads the LIVE composer when the
|
||||
# failed session is still visible (Codex #5488 catch), rather than the
|
||||
# captured snapshot.
|
||||
assert "const stillVisible=(S.session&&S.session.session_id)===sid;" in body
|
||||
assert "const liveText=inp?String(inp.value||''):restore;" in body
|
||||
|
||||
|
||||
def test_send_captures_immutable_snapshot_before_rewrites_and_upload():
|
||||
@@ -80,16 +85,46 @@ def test_send_captures_immutable_snapshot_before_rewrites_and_upload():
|
||||
def test_error_branch_restores_original_snapshot_not_mutated_payload():
|
||||
start = MESSAGES_JS.find("S.messages.push({role:'assistant',content:`**Error:** ${errMsg}`});")
|
||||
assert start != -1, "the /api/chat/start error branch must still push an Error turn"
|
||||
window = MESSAGES_JS[start:start + 900]
|
||||
window = MESSAGES_JS[start:start + 1100]
|
||||
assert (
|
||||
"_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid);"
|
||||
"_restoreComposerDraftAfterFailedSend(_failedSendDraftText, _failedSendFilesSnapshot, activeSid, _composerDraftClearPromise);"
|
||||
in window
|
||||
), "the send-error path must restore the ORIGINAL captured snapshot (not `text`)"
|
||||
|
||||
|
||||
def test_send_still_clears_composer_on_the_happy_path():
|
||||
assert "$('msg').value='';autoResize();" in MESSAGES_JS
|
||||
assert "if (activeSid && typeof _clearComposerDraft === 'function') _clearComposerDraft(activeSid);" in MESSAGES_JS
|
||||
# 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.)
|
||||
main_clear = (
|
||||
"if (activeSid && typeof _clearComposerDraft === 'function') "
|
||||
"_composerDraftClearPromise=_clearComposerDraft(activeSid);"
|
||||
)
|
||||
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 "$('msg').value='';autoResize();" in window_before, (
|
||||
"the main-path composer wipe must precede the persisted-draft clear"
|
||||
)
|
||||
|
||||
|
||||
def test_restore_persist_chains_after_the_clear_promise():
|
||||
# NIT 1: the restore's re-persist must be ordered AFTER the send-time clear
|
||||
# POST resolves (avoids an HTTP/2 reorder leaving the server draft empty).
|
||||
body = _helper_body()
|
||||
assert "clearPromise" in body, "helper must accept the clear promise for ordering"
|
||||
assert "clearPromise.then(_persist,_persist)" in body, (
|
||||
"re-persist must chain off the clear promise (both fulfill and reject → persist)"
|
||||
)
|
||||
# And send() must pass the captured clear promise into the restore call.
|
||||
assert "let _composerDraftClearPromise=null;" in MESSAGES_JS
|
||||
call = (
|
||||
"_restoreComposerDraftAfterFailedSend(_failedSendDraftText, "
|
||||
"_failedSendFilesSnapshot, activeSid, _composerDraftClearPromise);"
|
||||
)
|
||||
assert call in MESSAGES_JS, "send() must pass the clear promise into the restore helper"
|
||||
|
||||
|
||||
# ---------------------------------------------------------------------------
|
||||
@@ -197,4 +232,170 @@ def test_does_not_pollute_a_different_visible_session():
|
||||
def test_noop_when_nothing_to_restore():
|
||||
out = _run_helper_in_node("", [], "", visible_sid="sid-1")
|
||||
assert out["ret"] is False
|
||||
assert out["saved"] is None
|
||||
|
||||
|
||||
def test_persist_is_ordered_after_the_clear_promise():
|
||||
"""Behavioral: the re-persist must run AFTER the clear promise resolves.
|
||||
|
||||
Simulates the send-time clear POST as a promise that records the persist
|
||||
order. The re-persist must observe that the clear has already resolved.
|
||||
"""
|
||||
node = shutil.which("node")
|
||||
if not node: # pragma: no cover
|
||||
pytest.skip("node not available")
|
||||
body = _helper_body()
|
||||
harness = textwrap.dedent(
|
||||
"""
|
||||
const order = [];
|
||||
const state = {input: {value: ""}, pendingFiles: []};
|
||||
const $ = (id) => (id === 'msg' ? state.input : null);
|
||||
const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-1'}};
|
||||
function autoResize(){}
|
||||
function updateSendBtn(){}
|
||||
function renderTray(){}
|
||||
function _saveComposerDraftNow(sid, text, files){ order.push('persist:' + text); }
|
||||
|
||||
%(helper)s
|
||||
|
||||
// Clear POST resolves on a microtask; record its completion first.
|
||||
const clearPromise = Promise.resolve().then(() => { order.push('clear'); });
|
||||
_restoreComposerDraftAfterFailedSend('hello', [], 'sid-1', clearPromise);
|
||||
// Flush microtasks, then report ordering.
|
||||
Promise.resolve().then(() => Promise.resolve()).then(() => {
|
||||
console.log(JSON.stringify({order}));
|
||||
});
|
||||
"""
|
||||
) % {"helper": body}
|
||||
proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30)
|
||||
assert proc.returncode == 0, f"node harness failed: {proc.stderr}"
|
||||
out = json.loads(proc.stdout.strip())
|
||||
assert out["order"] == ["clear", "persist:hello"], (
|
||||
f"persist must run after the clear resolves, got {out['order']}"
|
||||
)
|
||||
|
||||
|
||||
def test_persist_still_fires_when_clear_promise_absent():
|
||||
"""Fallback: with no clear promise, the persist happens immediately (sync)."""
|
||||
out = _run_helper_in_node("no clear promise", [], "", visible_sid="sid-1")
|
||||
assert out["ret"] is True
|
||||
assert out["saved"] == {"sid": "sid-1", "text": "no clear promise", "files": []}
|
||||
|
||||
|
||||
def test_deferred_persist_captures_a_post_restore_edit_not_the_stale_snapshot():
|
||||
"""Codex #5488 regression: if the user edits the restored composer before the
|
||||
deferred persist fires, the persist must save the EDITED text — not clobber it
|
||||
with the original failed-send snapshot."""
|
||||
node = shutil.which("node")
|
||||
if not node: # pragma: no cover
|
||||
pytest.skip("node not available")
|
||||
body = _helper_body()
|
||||
harness = textwrap.dedent(
|
||||
"""
|
||||
let saved = null;
|
||||
const state = {input: {value: ""}, pendingFiles: []};
|
||||
const $ = (id) => (id === 'msg' ? state.input : null);
|
||||
const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-1'}};
|
||||
function autoResize(){}
|
||||
function updateSendBtn(){}
|
||||
function renderTray(){}
|
||||
function _saveComposerDraftNow(sid, text, files){ saved = {sid, text}; }
|
||||
|
||||
%(helper)s
|
||||
|
||||
// Clear POST settles on a microtask; the deferred persist runs after it.
|
||||
const clearPromise = Promise.resolve();
|
||||
_restoreComposerDraftAfterFailedSend('original', [], 'sid-1', clearPromise);
|
||||
// Synchronously the composer shows the restored text...
|
||||
const afterRestore = state.input.value;
|
||||
// ...then the user edits it BEFORE the deferred persist fires.
|
||||
state.input.value = 'edited after restore';
|
||||
Promise.resolve().then(() => Promise.resolve()).then(() => {
|
||||
console.log(JSON.stringify({afterRestore, saved}));
|
||||
});
|
||||
"""
|
||||
) % {"helper": body}
|
||||
proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30)
|
||||
assert proc.returncode == 0, f"node harness failed: {proc.stderr}"
|
||||
out = json.loads(proc.stdout.strip())
|
||||
assert out["afterRestore"] == "original", "composer should show the restored text synchronously"
|
||||
assert out["saved"] == {"sid": "sid-1", "text": "edited after restore"}, (
|
||||
f"deferred persist must save the LIVE edited text, not the stale snapshot; got {out['saved']}"
|
||||
)
|
||||
|
||||
|
||||
def test_deferred_persist_skips_when_user_switched_away_after_restore():
|
||||
"""Codex #5488 regression: if we restored the visible session then the user
|
||||
switched to a different session before the deferred persist fires, the stale
|
||||
persist must be SKIPPED (the session-switch save path already saved it)."""
|
||||
node = shutil.which("node")
|
||||
if not node: # pragma: no cover
|
||||
pytest.skip("node not available")
|
||||
body = _helper_body()
|
||||
harness = textwrap.dedent(
|
||||
"""
|
||||
let saveCalls = [];
|
||||
const state = {input: {value: ""}, pendingFiles: []};
|
||||
const $ = (id) => (id === 'msg' ? state.input : null);
|
||||
const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-1'}};
|
||||
function autoResize(){}
|
||||
function updateSendBtn(){}
|
||||
function renderTray(){}
|
||||
function _saveComposerDraftNow(sid, text, files){ saveCalls.push({sid, text}); }
|
||||
|
||||
%(helper)s
|
||||
|
||||
const clearPromise = Promise.resolve();
|
||||
_restoreComposerDraftAfterFailedSend('original', [], 'sid-1', clearPromise);
|
||||
// User switches to a different session before the deferred persist fires.
|
||||
S.session = {session_id: 'sid-2'};
|
||||
state.input.value = 'draft for sid-2';
|
||||
Promise.resolve().then(() => Promise.resolve()).then(() => {
|
||||
console.log(JSON.stringify({saveCalls}));
|
||||
});
|
||||
"""
|
||||
) % {"helper": body}
|
||||
proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30)
|
||||
assert proc.returncode == 0, f"node harness failed: {proc.stderr}"
|
||||
out = json.loads(proc.stdout.strip())
|
||||
# No persist for sid-1 with the stale 'original' text, and crucially no write
|
||||
# of sid-2's live composer under sid-1 (that would corrupt sid-1's draft).
|
||||
assert out["saveCalls"] == [], (
|
||||
f"deferred persist must skip entirely after a switch-away; got {out['saveCalls']}"
|
||||
)
|
||||
|
||||
|
||||
def test_background_failure_persists_snapshot_since_no_live_composer():
|
||||
"""A failed send for a NON-visible session (background) has no live composer
|
||||
to read, so the deferred persist saves the captured snapshot for that sid."""
|
||||
node = shutil.which("node")
|
||||
if not node: # pragma: no cover
|
||||
pytest.skip("node not available")
|
||||
body = _helper_body()
|
||||
harness = textwrap.dedent(
|
||||
"""
|
||||
let saveCalls = [];
|
||||
const state = {input: {value: "visible session draft"}, pendingFiles: []};
|
||||
const $ = (id) => (id === 'msg' ? state.input : null);
|
||||
// The visible session is sid-2; the failed send was for sid-1 (background).
|
||||
const S = {pendingFiles: state.pendingFiles, session: {session_id: 'sid-2'}};
|
||||
function autoResize(){}
|
||||
function updateSendBtn(){}
|
||||
function renderTray(){}
|
||||
function _saveComposerDraftNow(sid, text, files){ saveCalls.push({sid, text}); }
|
||||
|
||||
%(helper)s
|
||||
|
||||
const ret = _restoreComposerDraftAfterFailedSend('bg failed msg', [], 'sid-1', null);
|
||||
Promise.resolve().then(() => {
|
||||
console.log(JSON.stringify({ret, saveCalls, visibleUntouched: state.input.value}));
|
||||
});
|
||||
"""
|
||||
) % {"helper": body}
|
||||
proc = subprocess.run([node, "-e", harness], capture_output=True, text=True, timeout=30)
|
||||
assert proc.returncode == 0, f"node harness failed: {proc.stderr}"
|
||||
out = json.loads(proc.stdout.strip())
|
||||
assert out["ret"] is False, "a background (non-visible) failure must not report a visible restore"
|
||||
assert out["visibleUntouched"] == "visible session draft", "the visible composer must be untouched"
|
||||
assert out["saveCalls"] == [{"sid": "sid-1", "text": "bg failed msg"}], (
|
||||
f"background failure must persist the snapshot for its own sid; got {out['saveCalls']}"
|
||||
)
|
||||
Reference in new issue
Block a user