Fix chat.abort during attachment send preprocessing#84306
Closed
nxmxbbd wants to merge 13 commits into
Closed
Conversation
openclaw-barnacle
Bot
added
app: web-ui
Contributor
|
Codex review: needs real behavior proof before merge. Reviewed June 6, 2026, 7:31 AM ET / 11:31 UTC. Summary PR surface: Source +343, Tests +1400, Docs 0, Other +391. Total +2134 across 32 files. Reproducibility: yes. source inspection gives a high-confidence reproduction path: current main awaits attachment preprocessing before registering the abort controller, so chat.abort can miss an in-preprocess chat.send. I did not run a live repro in this read-only review. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Resolve the dirty merge, refresh real behavior proof at the current head or split out the native changes, and land only after maintainers accept the terminal ACK lifecycle contract. Do we have a high-confidence way to reproduce the issue? Yes, source inspection gives a high-confidence reproduction path: current main awaits attachment preprocessing before registering the abort controller, so chat.abort can miss an in-preprocess chat.send. I did not run a live repro in this read-only review. Is this the best way to solve the issue? Yes, early abort registration plus terminal ACK propagation is a maintainable fix shape for the race, but the exact timeout/error ACK contract and node subscription canonicalization need maintainer acceptance before merge. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 74331f632b93. Label changesLabel changes:
Label justifications:
Evidence reviewedPR surface: Source +343, Tests +1400, Docs 0, Other +391. Total +2134 across 32 files. View PR surface stats
What I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
proof: sufficient
Contributor
|
ClawSweeper PR egg ✨ Hatched: 🥚 common Neon Branchling
Hatch commandComment Hatchability rules:
Rarity: 🥚 common. What is this egg doing here?
|
Contributor
Author
|
@clawsweeper hatch |
Contributor
|
🦞👀 I queued a comment sync for this PR. If the egg is hatchable, ClawSweeper will generate the image once and update the existing review comment. |
Member
|
Heads up: this PR needs to be updated against current |
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
the
proof: sufficient
openclaw-barnacle
Bot
removed
the
proof: sufficient
clawsweeper
Bot
added
proof: sufficient
…spatch abort
A chat.send aborted before dispatch (during attachment preprocessing) or after
dispatch reported a non-terminal {status:"started"} for both the chat.send
response and the chat:${runId} dedupe entry. That left the Control UI adopting
an already-aborted run (stuck "busy") and agent.wait callers hanging until their
own timeout instead of observing the abort.
Report the terminal outcome instead, reusing the existing
{status:"timeout", stopReason} shape sessions.abort already writes (no new status
invented): the pre-dispatch abort branches respond terminal and write a terminal
dedupe entry, and the post-dispatch completion guard records the terminal abort
instead of skipping the dedupe write. A "rpc"/"stop" stopReason classifies as a
sticky cancellation so a late started/in_flight write cannot resurrect the run.
UI: preserve terminal ack statuses through normalizeChatSendAck (previously
coerced to "started") and gate adopt-vs-clear on the shared
isNonTerminalAgentRunStatus predicate (three-way: non-terminal adopts, "ok"
completes, terminal does nothing since the abort broadcast already drove the
lifecycle).
Related openclaw#84176.
…t terminal dedupe Follow-up to the chat.send terminal-abort fix: skills.proposals.requestRevision forwards the chat.send ack, so widen SkillsProposalRequestRevisionResultSchema's status union to include the terminal "timeout"/"error" statuses the abort fix can now return through that public method. Also: add a schema regression test for the widened union, add a post-dispatch abort dedupe assertion (duplicate send after a post-dispatch abort observes the terminal outcome), and tighten the dedupe comment to note that maintenance-driven "timeout" aborts are terminal but not sticky. Related openclaw#84176.
…oc the abort ack
Address the remaining low-severity review follow-ups for the chat.send terminal
abort fix:
- gateway: the post-dispatch .catch handler now records the terminal abort
outcome (and skips the redundant chat error broadcast) when the run was already
aborted, matching the .then completion guard, so a dispatch that throws under
abort no longer reports a spurious error instead of the cancellation.
- ui: export normalizeChatSendAck and add tests that it preserves terminal
statuses (timeout/error) instead of coercing them to "started", plus a
controller test that a terminal send ack is not adopted as an active run (the
3-way adopt gate).
- docs: note the terminal {status:"timeout"} ack for sends aborted before/at
dispatch in the Control UI send semantics.
Related openclaw#84176.
…ller signal
A reused idempotencyKey is the chat run id (clientRunId), and the shared
chatAbortedRuns marker is keyed by it. The marker lives ~60min while the
chat:<runId> dedupe lives only ~5min, and it is also reused by any prior or
concurrent same-key run. The post-dispatch completion/throw handlers read that
marker to decide whether to write a terminal abort dedupe, so a stale or
cross-run marker made a fresh same-key run report a false {status:"timeout"} and
the .catch swallow a real error as a false abort.
Fix the post-dispatch guards to read THIS run's own AbortController signal
(activeRunAbort.controller.signal.aborted) instead of the shared marker,
mirroring the pre-dispatch abort checks. Only a genuine abort of this run sets
its controller (abortChatRunById is the sole production setter), so a stale
marker no longer misclassifies it, and the shared marker is left untouched so
server-chat abort-tail suppression and concurrent same-key sends are unaffected.
Also log the swallowed error in the .catch-under-abort branch so a real failure
coinciding with an abort is not lost; skip seeding refreshSessionsAfterChat for
terminal acks since the clearing chat-state event never arrives; and tighten the
dedupe-shape comment.
Adds regression coverage: a fresh same-key run with a stale marker present
completes terminal-ok / surfaces a real error instead of a false timeout (marker
left intact), and terminal acks no longer seed the deferred session refresh.
Issue openclaw#84176.
openclaw-barnacle
Bot
added
docs
clawsweeper
Bot
added
rating: 🦐 gold shrimp
openclaw-barnacle
Bot
added
proof: supplied
clawsweeper
Bot
added
rating: 🦪 silver shellfish
nxmxbbd
marked this pull request as draft
June 5, 2026 21:22
nxmxbbd
marked this pull request as ready for review
June 6, 2026 05:10
clawsweeper
Bot
added
rating: 🌊 off-meta tidepool
Contributor
Author
|
@clawsweeper re-review |
Contributor
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Contributor
Author
|
Closing this since the original issue was fixed on main in 6ace7a6. I'll follow up separately with smaller current-main issues/PRs for any remaining terminal chat.send ACK/client-side gaps that still reproduce. |
This was referenced Jun 6, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
chat.abort/chat.sendrace from [Bug]: chat.abort fails when called immediately after chat.send due to race condition #84176 by registering the chat run abort controller before awaited attachment/model preprocessing, so an immediate abort can find and cancel the just-started run.{runId,status:"timeout"}instead of non-terminal{status:"started"}. This prevents the Control UI andagent.waitfrom treating an already-cancelled run as still active.chatAbortedRunsmarker, so stale or cross-run same-key abort markers no longer force a fresh run to reporttimeoutor swallow its real error. The shared marker remains in place for existing abort-tail suppression.chat.sendACKs now clear the Control UI's optimistic local run/stream state through the shared run lifecycle reconciler, including the queued app-chat path. New visible sends clear stale local-terminal suppression state.chat.sendACKstatusunion to include terminal"timeout"/"error"values in the Control UI ACK normalizer and forwarded skills-revision ACK schema. No new RPC method, config/auth/security/network change, or channel adapter change.AI-assisted; contributor manually reviewed and is responsible for the change.
Linked context
cd4e345a12e274ddcbf53f82c1c06c76121fa0ceReal behavior proof
chat.abortcalled immediately after an attachmentchat.sendnow cancels the in-preprocess run, and the send returns terminal{runId,status:"timeout"}rather than a non-terminalstartedACK. A post-dispatch abort also leaves terminal dedupe for duplicate same-key sends /agent.wait. A fresh same-key send after an older abort is reported from its own run signal, not from a stale shared abort marker. The Control UI clears local optimistic run/stream state whenchat.senditself returns terminaltimeoutorerror, including the queued send path.Current head
cd4e345a12was exercised with real local Gateway processes started from this checkout viagateway run --allow-unconfiguredon loopback, using temporary HOME/state/config directories. The attachment abort proof used the real Gateway WebSocket protocol and productionchat.send/chat.aborthandlers with a real PNG attachment. The browser proof used the built Control UI served by that Gateway, Chromium, and CDP WebSocket frame capture; a second real Gateway WebSocket client issued the immediate abort as soon as the browser's attachmentchat.sendframe was observed. The model catalog was a local test-provider entry with image support and a loopback dummy base URL; the proof intentionally aborts before provider dispatch, so no provider call is part of the expected path.Terminal capture from a real Gateway loopback WebSocket run, copied live output:
Terminal capture from the real Gateway-served Control UI browser run, copied live output:
Redacted browser WebSocket frame capture from the same browser run shows the browser's real attachment send and terminal ACK:
[ { "direction": "browser->gateway", "method": "chat.send", "params": { "sessionKey": "agent:main:main", "message": "see image", "deliver": false, "idempotencyKey": "d50f2f19-010e-4478-9cc8-b04ae4afd540", "attachments": [{ "type": "image", "mimeType": "image/png", "fileName": "dot.png", "content": "<redacted base64 length=92>" }] } }, { "direction": "gateway->browser", "method": "chat.send", "ok": true, "payload": { "runId": "d50f2f19-010e-4478-9cc8-b04ae4afd540", "status": "timeout" } } ]Public screenshot artifact from the same browser run, hosted outside the PR diff:
Control UI idle after terminal attachment-send abort ACK
Artifact branch README: https://raw.githubusercontent.com/nxmxbbd/openclaw/pr-84306-route1-proof-20260606/README.md
Image sha256:
ee2fc617d12bc75eeee38ffb9868bd78ed3f7a216946270e17b75761e24ed713Supplemental current-head focused proof runs:
A real Gateway attachment
chat.sendcan be aborted immediately while preprocessing is still pending and returns terminal{runId,status:"timeout"}to the originalchat.sendcaller. Duplicate same-key sends and waiters observe terminaltimeoutinstead of a stuck non-terminal state. In the real browser run, the Control UI received the terminaltimeoutACK for its own attachment send and returned to idle:chatSending:false,chatRunId:null,chatStream:null, no Stop button, enabled Send button titledSend, and no chat/login error.No real provider call is made in the abort proofs; the target behavior is cancellation before provider dispatch. The browser proof uses a second local Gateway WS client as the immediate abort trigger so the abort lands during preprocessing; it does not claim that a human can click the Control UI Stop button before the non-terminal ACK renders. Not tested: remote/non-loopback Gateway, real model-provider dispatch after preprocessing, or a public recording.
Known trade-offs / out of scope
abortChatRunById.Risk checklist
Did user-visible behavior change? (
Yes/No) Yes. Immediate abort now reaches in-preprocess sends, aborted sends ACK terminaltimeout, and terminal ACKs clear the Control UI local active run.Did config, environment, or migration behavior change? (
Yes/No) NoDid security, auth, secrets, network, or tool execution behavior change? (
Yes/No) NoWhat is the highest-risk area?
Cancellation/idempotency timing around early abort registration, post-dispatch abort detection, dedupe caching, and Control UI optimistic run lifecycle.
How is that risk mitigated?
Abort detection uses each run's own AbortController signal, while the shared abort marker is preserved for abort-tail suppression. Non-aborted sends keep their existing
started/in_flightACK behavior. Focused gateway and UI regressions cover pre-dispatch attachment abort, post-dispatch abort terminal dedupe, stale-marker same-key behavior, terminal ACK UI clearing, queued app-chat clearing, and widened ACK schema acceptance. The added real Gateway/browser proof exercises the user-visible terminal ACK and idle UI path outside Vitest.Tests and validation
Commands run on current proof head
cd4e345a12e274ddcbf53f82c1c06c76121fa0ce:server.chat.gateway-server-chat-b, pre-dispatch attachment abort + loopback smoke) --- 2 tests passed.controllers/chat,app-chat,run-lifecycle) --- 222 tests passed.tsgotypecheck --- exit 0.oxfmt --checkover changed UI files --- clean.git diff --check a3992f4e62..HEAD--- clean.Regression coverage added/updated in this PR: pre-dispatch attachment abort terminal ACK + dedupe, post-dispatch own-signal abort handling, Control UI terminal ACK idle clearing for direct and queued sends, ACK normalizer preservation, and forwarded skills-revision schema acceptance/rejection.
Current review state
Current head
cd4e345a12e274ddcbf53f82c1c06c76121fa0ceis live on this PR. Checks were green after the last push, but the PR still carriedstatus: needs proof; this body refresh adds real local Gateway/browser proof for the qualitative proof blocker without changing code.