diff --git a/CHANGELOG.md b/CHANGELOG.md index 54bb2b92..9025e1c7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -5,6 +5,8 @@ ### Fixed +- **Editing and resubmitting a message can no longer truncate most of a long session's history if you click "Send edit" more than once.** `submitEdit` was re-entrant: its only guard was `S.busy`, which the send path doesn't set until after two multi-second awaits (`_ensureAllMessagesLoaded` on a long session, then the truncate round-trip). On a laggy instance that left a 10s+ window in which a second click started another full truncate — and because the first call sets `_oldestIdx = 0`, the second call recomputed `keep_count` from a still-window-relative index, producing a far smaller keep count that could delete most of a 2000-message session. A `_submitEditInFlight` guard is now acquired synchronously before the awaits and released in a `finally` (so an early return or throw can't wedge editing off), while the absolute keep count is still captured up front so a single legitimate submit is unaffected. The Enter-to-submit path clicks the same button, so it's covered too. Thanks @alanjds. (#7277) + - **Read-only child sessions stay grouped after their writable siblings in the sidebar lineage view.** When a session lineage is collapsed, read-only child sessions (e.g. shared/observed sessions) could interleave with writable children instead of sorting after them, making the group order unstable. The comparator now orders writable children first and read-only children after, within each lineage, with a stable total order that tolerates missing order keys and both the `read_only` and `is_read_only` shapes. Session open/selection is unaffected (still keyed by `session_id`, independent of row position). Thanks @igorroncevic. (#5888) - **Inline event-handler arguments across the UI are now escaped for the JavaScript context, closing an attribute-injection class.** `esc()` only escapes for HTML, but the browser decodes attribute entities *before* it parses an inline handler's JavaScript, so a value interpolated as `onclick="fn('${esc(id)}')"` let a quote in the data break out of the JS string literal (the #3797 bug class). A shared `jsArg()` helper (`esc(JSON.stringify(String(v)))` — `JSON.stringify` supplies its own quotes, `esc()` then makes the result attribute-safe) now wraps **every** inline-handler string argument across cron/kanban/checkpoint/gateway/passkey/notes actions, session handoff hints, and onboarding device-code copy. The one-off `_kanbanJsArg` that previously fixed this for kanban buttons alone is removed in a clean cutover. Thanks @zicochaos. (#7335, closes #3797) diff --git a/static/ui.js b/static/ui.js index d7e3f602..35a4fc7c 100644 --- a/static/ui.js +++ b/static/ui.js @@ -19367,38 +19367,65 @@ function autoResizeTextarea(ta) { ta.style.height = Math.min(ta.scrollHeight, 300) + 'px'; } +// #2184 follow-up: submitEdit is re-entrant and destructive, and nothing stopped +// a second invocation. Its only guard was S.busy, which send() does not set until +// the LAST line — after two multi-second awaits (_ensureAllMessagesLoaded on a long +// session, then the truncate round-trip). On a laggy instance that leaves a 10s+ +// window in which every further click on "Send edit" starts another full truncate. +// Observed in the wild: seven concurrent POST /api/session/truncate, 5.2-8.5s each, +// from one user clicking repeatedly because the UI had not yet acknowledged the first. +// +// That is not merely wasteful, it is a data-loss hazard. `absoluteKeepCount` is +// deliberately captured BEFORE the awaits (see test_submit_edit_captures_absolute_ +// before_await): at click time `_oldestIdx` is the loaded window's offset and +// `msgIdx` is window-relative, so their sum is the true absolute index. But the +// first call's _ensureAllMessagesLoaded() sets `_oldestIdx = 0`, so a SECOND call +// entering afterwards computes `0 + msgIdx` from a still-window-relative msgIdx — +// a far smaller keep_count. Truncating a 2000-message session to that would delete +// most of its history. +// +// Guard re-entry at the function itself rather than at the click handler: the edit +// can also be submitted with Enter (the keydown handler clicks the button), and a +// future caller would silently reopen the hole. Cleared in a finally so an early +// return or a throw cannot wedge editing off for the rest of the page's life. +let _submitEditInFlight = false; async function submitEdit(msgIdx, newText) { - if(!S.session || S.busy) return; - const initialSid = S.session.session_id; - const absoluteKeepCount = _oldestIdx + msgIdx; - // #5924: capture the deliberate-pick signal up front (pre-network), scoped to - // initialSid — a non-default session model (vs profile default), which is - // inference-free and survives the failed send's marker consumption. See - // _deliberateSessionModelPick. null → no re-arm → server resolution runs. - const _recoveryPick=_deliberateSessionModelPick(initialSid); - if(typeof _ensureAllMessagesLoaded==='function'){ - await _ensureAllMessagesLoaded(); - } - if(!S.session || S.session.session_id !== initialSid) return; + if(!S.session || S.busy || _submitEditInFlight) return; + _submitEditInFlight = true; try { - await api('/api/session/truncate', {method:'POST', body:JSON.stringify({ - session_id: initialSid, - keep_count: absoluteKeepCount - })}); - // #5924 SILENT-race guard: a session switch during the truncate await must not - // let this recovery apply session A's intent (truncate/re-arm/send) to the - // newly-visible session. + const initialSid = S.session.session_id; + const absoluteKeepCount = _oldestIdx + msgIdx; + // #5924: capture the deliberate-pick signal up front (pre-network), scoped to + // initialSid — a non-default session model (vs profile default), which is + // inference-free and survives the failed send's marker consumption. See + // _deliberateSessionModelPick. null → no re-arm → server resolution runs. + const _recoveryPick=_deliberateSessionModelPick(initialSid); + if(typeof _ensureAllMessagesLoaded==='function'){ + await _ensureAllMessagesLoaded(); + } if(!S.session || S.session.session_id !== initialSid) return; - S.messages = S.messages.slice(0, absoluteKeepCount); - renderMessages(); - $('msg').value = newText; - // #5924 (Facet 1 + Facet 4): edit-resubmit is a recovery send. Re-arm the - // Re-arm the single-shot explicit-pick marker from the captured non-default - // pick — only if still safe at fire time (session unchanged, current model - // still matches, no newer onchange marker to clobber). See _reArmRecoveryPick. - _reArmRecoveryPick(initialSid, _recoveryPick); - await send(); - } catch(e) { setStatus(t('edit_failed') + e.message); } + try { + await api('/api/session/truncate', {method:'POST', body:JSON.stringify({ + session_id: initialSid, + keep_count: absoluteKeepCount + })}); + // #5924 SILENT-race guard: a session switch during the truncate await must not + // let this recovery apply session A's intent (truncate/re-arm/send) to the + // newly-visible session. + if(!S.session || S.session.session_id !== initialSid) return; + S.messages = S.messages.slice(0, absoluteKeepCount); + renderMessages(); + $('msg').value = newText; + // #5924 (Facet 1 + Facet 4): edit-resubmit is a recovery send. Re-arm the + // Re-arm the single-shot explicit-pick marker from the captured non-default + // pick — only if still safe at fire time (session unchanged, current model + // still matches, no newer onchange marker to clobber). See _reArmRecoveryPick. + _reArmRecoveryPick(initialSid, _recoveryPick); + await send(); + } catch(e) { setStatus(t('edit_failed') + e.message); } + } finally { + _submitEditInFlight = false; + } } async function regenerateResponse(btn) { diff --git a/tests/test_issue_edit_regenerate_absolute_keep_count.py b/tests/test_issue_edit_regenerate_absolute_keep_count.py index 2640d708..1d4ca41e 100644 --- a/tests/test_issue_edit_regenerate_absolute_keep_count.py +++ b/tests/test_issue_edit_regenerate_absolute_keep_count.py @@ -41,3 +41,53 @@ def test_submit_edit_captures_absolute_before_await(): assert cap first_await = re.search(r"\bawait\b", body) assert first_await and cap.start() < first_await.start() + + +def test_submit_edit_is_guarded_against_reentry(): + """A second submitEdit while the first is in flight must be refused. + + submitEdit is destructive (it POSTs /api/session/truncate) and its only guard + was S.busy, which send() does not set until the last line — after + _ensureAllMessagesLoaded() and the truncate round-trip, both of which take + seconds on a long session. Observed in the wild: one user clicking "Send edit" + repeatedly because the UI had not acknowledged the first click produced SEVEN + concurrent truncate POSTs (5.2-8.5s each). + + The hazard is not just load. absoluteKeepCount is deliberately captured before + the awaits (test_submit_edit_captures_absolute_before_await) because at click + time _oldestIdx is the window offset and msgIdx is window-relative. But the + first call's _ensureAllMessagesLoaded() sets _oldestIdx = 0, so a second call + entering after that computes `0 + msgIdx` from a still-window-relative msgIdx — + a much smaller keep_count that would truncate away most of the transcript. + """ + body = _function_body(UI_JS, "submitEdit") + + assert "let _submitEditInFlight = false;" in UI_JS, ( + "submitEdit needs a module-scope in-flight flag; a local cannot survive " + "across invocations" + ) + assert "if(!S.session || S.busy || _submitEditInFlight) return;" in body, ( + "submitEdit must refuse re-entry while a previous call is still in flight — " + "S.busy alone is set too late (only by send(), on the last line)" + ) + + # The flag must be raised before the first await, or the window it closes is + # exactly the window that was open before. + raise_pos = body.index("_submitEditInFlight = true;") + first_await = body.index("await ") + assert raise_pos < first_await + + # ...and cleared in a finally, so an early return or a throw cannot wedge + # editing off permanently for the rest of the page's life. + assert "} finally {" in body + finally_pos = body.index("} finally {") + clear_pos = body.index("_submitEditInFlight = false;", finally_pos) + assert clear_pos > finally_pos + assert clear_pos > body.index("await send();"), ( + "the flag must be cleared after the send completes, not partway through" + ) + + # Guard at the function, not the click handler: the edit is also submittable + # via Enter (the keydown handler clicks the button), so a handler-only guard + # would leave that path open. + assert "await submitEdit(msgIdx, newText);" in UI_JS