Skip to content

fix(webui): guard submitEdit against re-entry (destructive double-submit) - #3

Open
alanjds wants to merge 13 commits into
masterfrom
claude/webui-edit-double-submit
Open

alanjds wants to merge 13 commits into
masterfrom
claude/webui-edit-double-submit

Conversation

@alanjds

@alanjds alanjds commented Aug 24, 2026

Copy link
Copy Markdown
Owner

The report

On a laggy instance, one user clicked "Send edit" repeatedly because the UI had not acknowledged the first click. Their network log:

POST /api/session/truncate   5189ms  8336ms  6658ms  7449ms  8053ms  8541ms   ×7
GET  /api/session?messages=1 6400ms

Seven concurrent truncates from one intended edit.

Why

submitEdit's only re-entry guard was S.busy — but send() does not set that until the last line, after two multi-second awaits:

if(!S.session || S.busy) return;              // S.busy still false
const absoluteKeepCount = _oldestIdx + msgIdx;
await _ensureAllMessagesLoaded();             // ~6.4s on a long session
await api('/api/session/truncate', ...);      // 5-8s under load
await send();                                 // S.busy finally set

On a 2000-message session that is a 10s+ window in which every further click runs the whole destructive path again.

The hazard is worse than the load

absoluteKeepCount is captured before the awaits deliberately — at click time _oldestIdx is the loaded window's offset and msgIdx is window-relative, so their sum is the true absolute index. That ordering is pinned by test_submit_edit_captures_absolute_before_await (the nesquena#2184 pattern).

But the first call's _ensureAllMessagesLoaded() sets _oldestIdx = 0. A second call entering after that computes 0 + msgIdx from a msgIdx that is still window-relative — a far smaller keep_count. Truncating a long session to that would delete most of its history.

The reporter's session came through intact, so this is a latent hazard rather than a confirmed loss — but the mechanism is there and truncate deletes messages.

The fix

A module-scope _submitEditInFlight flag, raised before the first await and cleared in a finally.

Guarded at the function rather than the click handler: the edit is also submittable with Enter (the keydown handler clicks the button), so a handler-only guard leaves that path open, and a future caller would silently reopen the hole. The finally means an early return or a throw cannot wedge editing off for the rest of the page's life.

Deliberately not changed

  • The pre-await capture stays. Recomputing keep_count after _ensureAllMessagesLoaded() looks like the obvious fix and is wrong — it pairs _oldestIdx=0 with a window-relative msgIdx and breaks Fork from here uses local window index in long sessions nesquena/hermes-webui#2184 outright. (I proposed exactly that before reading the test, and it would have been a regression.)
  • regenerateResponse has the same S.busy-only shape, but no longer client-side truncates — it delegates to the atomic startRegeneration and validates the clicked index against latestAssistantIndex, so a double click is refused rather than destructive. Left alone to keep this diff to the fault.
  • No UI feedback. Extra clicks are now harmless, but the button still does not visibly acknowledge the first one — which is what provoked the clicking. Worth a follow-up; it is a UX change, not this correctness fix.

Verification, and its limits

Not reproduced end-to-end. This environment cannot run a real agent turn, so the seven-truncate storm comes from the reporter's network log rather than a local repro. The guard is verified by source-level test only.

test_submit_edit_is_guarded_against_reentry pins the flag, that it is raised before the first await, and that it is cleared in a finally after send(). Confirmed to fail when the guard is reverted — the three pre-existing tests in that file pass either way, so they would not have caught this.

Broad sweep: 2457 passed, 13 failed — all pre-existing test_update_banner_fixes.py ordering pollution, identical on a clean tree. One earlier run of the same sweep showed a 14th failure that did not reproduce across two further runs and whose name I did not capture; flagging it rather than claiming a clean baseline.

ruff check clean, node --check static/ui.js clean.


Generated by Claude Code

claude and others added 13 commits August 24, 2026 16:07
…mit)

Clicking "Send edit" a second time while the first is still working starts a
second full truncate. Observed in the wild on a laggy instance: seven concurrent
POST /api/session/truncate, 5.2s / 8.3s / 6.7s / 7.4s / 8.1s / 8.5s, from one
user clicking repeatedly because the UI had not yet acknowledged the first click.

submitEdit's only guard was S.busy — but send() does not set that until the LAST
line of the function, after two multi-second awaits:

    if(!S.session || S.busy) return;              // S.busy still false
    const absoluteKeepCount = _oldestIdx + msgIdx;
    await _ensureAllMessagesLoaded();             // ~6.4s on a long session
    await api('/api/session/truncate', ...);      // 5-8s under load
    await send();                                 // S.busy finally set

On a 2000-message session that is a 10s+ window in which every further click
runs the whole destructive path again.

The load is the visible symptom; the hazard is worse. absoluteKeepCount is
captured BEFORE the awaits deliberately — at click time _oldestIdx is the loaded
window's offset and msgIdx is window-relative, so the sum is the true absolute
index (pinned by test_submit_edit_captures_absolute_before_await, the nesquena#2184
pattern). But the first call's _ensureAllMessagesLoaded() sets _oldestIdx = 0.
A second call entering after that computes `0 + msgIdx` from a msgIdx that is
still window-relative — a far smaller keep_count. Truncating a long session to
that would delete most of its history.

Fix: a module-scope _submitEditInFlight flag, raised before the first await and
cleared in a finally. Guarded at the function rather than at the click handler
because the edit is also submittable with Enter (the keydown handler clicks the
button), so a handler-only guard would leave that path open, and because a future
caller would silently reopen the hole. The finally means an early return or a
throw cannot wedge editing off for the rest of the page's life.

Deliberately NOT changed:
- The pre-await capture stays where it is. Recomputing keep_count after
  _ensureAllMessagesLoaded() looks like the obvious fix and is wrong: it would
  pair _oldestIdx=0 with a window-relative msgIdx and break nesquena#2184 outright.
- regenerateResponse has the same S.busy-only shape, but no longer client-side
  truncates (it delegates to the atomic startRegeneration) and validates the
  clicked index against latestAssistantIndex, so a double click is refused
  server-side rather than being destructive. Left alone to keep this diff to the
  fault.
- No UI feedback added. Extra clicks are now harmless, but the button still does
  not visibly acknowledge the first one, which is what provoked the clicking.
  Worth a follow-up; it is a UX change, not this correctness fix.

Not reproduced end-to-end: this environment cannot run a real agent turn, so the
seven-truncate storm comes from the reporter's network log, not from a local
repro. The guard is verified by source-level test only.

test_submit_edit_is_guarded_against_reentry pins the flag, that it is raised
before the first await and cleared in a finally after send(). Confirmed to FAIL
when the guard is reverted. The three pre-existing tests in that file still pass.
Broad sweep: 2457 passed, 13 failed — all pre-existing test_update_banner_fixes
ordering pollution, identical on a clean tree. One earlier run of the same sweep
showed a 14th failure that did not reproduce across two further runs and whose
name I did not capture; flagging it rather than claiming a clean baseline.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_018y9pr7ppZgDuCZCT6x96kY
…x is provably identical; compression-anchor coverage (nesquena#6826 r3)
Removes the local re-import of api.streaming._session_payload_with_full_messages
inside test_in_tail_duplicate_guard_refuses_bounded_and_full_revision_accepted;
it is already imported at module level (line 17) and unused in the function body,
tripping F401 on CI's hosted lint. Mechanical maintainer fix per auto-fix policy.

Co-authored-by: webtecnica <webtecnica@users.noreply.github.com>
Bounded sidecar-anchored tail read for regenerate (closes nesquena#6826). Thanks @webtecnica — six rounds of adversarial re-gate to convergence.
…ail read (nesquena#7204, @webtecnica)

Adds the CHANGELOG entry for nesquena#7204 (closes nesquena#6826). Release-metadata only;
the code change ships via the merge of nesquena#7204 (contributor webtecnica).
Release exp-v0.52.264: fast regenerate via bounded sidecar-anchored tail read (nesquena#7204, @webtecnica)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants