Merge pull request #7468 from nesquena/stage/7277

Release exp-v0.52.276: guard submitEdit against re-entry (#7277, @alanjds)

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)

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) {

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