Preserve cancelled partial turn payloads - #4647
franksong2702 wants to merge 17 commits into
Conversation
b4b1ed3 to
64e5bb0
Compare
|
| Filename | Overview |
|---|---|
| api/streaming.py | Adds two new helpers (_cancel_event_payload, _redacted_session_payload_with_full_messages) and wires them correctly: cancel_stream() embeds the session after save(); worker paths stay metadata-only. All exception paths leave _cancel_session_payload as None, causing graceful fallback. |
| static/messages.js | Refactors the cancel handler to prefer the embedded session payload via _applyCancelSessionPayload, then falls back to the /api/session GET. Logic is sound but the guard inside _applyCancelSessionPayload checks S.session.session_id rather than sessionPayload.session_id, and the null-object check uses the typeof-null quirk without an explicit !== null guard. |
| tests/test_cancelled_turn_status.py | Adds two structural regression tests: one verifying the frontend ordering invariant (embedded payload tried before GET), one verifying worker blocks never call _cancel_event_payload with a session argument. |
| tests/test_issue1361_cancel_data_loss.py | Adds an integration test seeding reasoning/tool-call state, calling cancel_stream(), and asserting the queue item carries partial messages and the cancel marker. Preconditions now use explicit assert hasattr() before assignment. |
| tests/test_live_to_final_anchor_visible_order.py | Updates the anchor-scene ordering test to match refactored _applyCancelSessionPayload internals and adds index-based ordering assertions for embedded → GET → fallback-apply paths. |
| tests/test_session_lost_response_regression.py | Trailing blank-line cleanup only — no logic changes. |
Sequence Diagram
%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant W as Streaming Worker
participant CS as cancel_stream()
participant Q as SSE Queue
participant B as Browser
Note over W,CS: User triggers cancel
CS->>CS: cancel_event.set()
W->>W: detect cancel_event
W->>W: _finalize_cancelled_turn(s)
W->>Q: put cancel [metadata-only, no session]
CS->>CS: acquire locks, snapshot buffers
CS->>CS: _build_partial_message (reasoning + tool calls)
CS->>CS: _cs.messages.append(cancel marker)
CS->>CS: _cs.save()
CS->>CS: "_cancel_session_payload = _redacted_session_payload_with_full_messages(_cs)"
CS->>Q: put cancel [with embedded session snapshot]
Q->>B: SSE cancel event (first arrival)
alt Embedded session payload present
B->>B: _applyCancelSessionPayload(payload.session)
B->>B: "S.session = payload, render partial turn immediately"
else No embedded session
B->>B: _applyCancelSessionPayload(null) → false
B->>B: "GET /api/session?session_id=..."
B->>B: _applyCancelSessionPayload(data.session)
B->>B: render
end
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant W as Streaming Worker
participant CS as cancel_stream()
participant Q as SSE Queue
participant B as Browser
Note over W,CS: User triggers cancel
CS->>CS: cancel_event.set()
W->>W: detect cancel_event
W->>W: _finalize_cancelled_turn(s)
W->>Q: put cancel [metadata-only, no session]
CS->>CS: acquire locks, snapshot buffers
CS->>CS: _build_partial_message (reasoning + tool calls)
CS->>CS: _cs.messages.append(cancel marker)
CS->>CS: _cs.save()
CS->>CS: "_cancel_session_payload = _redacted_session_payload_with_full_messages(_cs)"
CS->>Q: put cancel [with embedded session snapshot]
Q->>B: SSE cancel event (first arrival)
alt Embedded session payload present
B->>B: _applyCancelSessionPayload(payload.session)
B->>B: "S.session = payload, render partial turn immediately"
else No embedded session
B->>B: _applyCancelSessionPayload(null) → false
B->>B: "GET /api/session?session_id=..."
B->>B: _applyCancelSessionPayload(data.session)
B->>B: render
end
Reviews (16): Last reviewed commit: "Merge origin/master into PR #4647" | Re-trigger Greptile
64e5bb0 to
9017940
Compare
|
Pushed a refreshed #4647 follow-up on top of current What changed since the prior head:
Verification:
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed a base refresh for #4647 at What changed:
Local verification:
GitHub checks are now running on the refreshed head. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at What changed:
Local verification:
GitHub checks are now running on the refreshed head. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at What changed:
Local verification:
GitHub checks are now running on the refreshed head. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Follow-up pushed at What changed since the base refresh:
Verification:
GitHub readback: all visible checks are passing; PR is |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at What changed:
Local verification:
GitHub checks are now running on the refreshed head. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
|
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
|
|
Deep-reviewed + gate-clean — held for your visual sign-off (crown-jewel cancel-render surface, so not auto-shipped per the standing rule).
Recommendation to the maintainer: ship — clean UX-recovery win (cancel now shows your preserved partial work instead of a flash of nothing). Awaiting a quick visual confirm before release. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Refreshed this branch against current
Local verification passed:
GitHub checks are running on the refreshed head. |
|
CI readback for the refreshed head
No additional branch changes from me here; this is ready for the pending visual sign-off/reviewer path. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
Local verification passed:
GitHub readback after push: PR open, non-draft, head |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
Local verification passed:
GitHub readback after push: PR open, non-draft, head |
|
CI readback for refreshed head
No additional branch changes from me here; this is ready for the pending visual sign-off/reviewer path. |
Co-Authored-By: Paperclip <noreply@paperclip.ing>
|
Pushed another base refresh for #4647 at
Local verification passed:
GitHub readback after push: PR open, non-draft, head |
|
CI readback for refreshed head
No additional branch changes from me here; this is ready for the pending visual sign-off/reviewer path. |
Release: preserve cancelled partial turn payloads (#4647)
|
Shipped in v0.51.703 🎉 — thank you @franksong2702! Since this PR is from your fork, I shipped it via a maintainer release branch (push-to-fork-branch can't update the PR), preserving your authorship with a Your fix is exactly right: embedding the canonical redacted session snapshot in the terminal cancel event so the preserved partial reasoning/tool/text renders immediately, skipping the GET race that briefly collapsed the turn to a bare "Task cancelled" marker (#4076, #1361). Full gate before ship:
One small maintainer hardening on top of your fix: the embedded snapshot is now only applied when its |
…ot (nesquena#4647) Cancelling a running turn briefly collapsed the already-streamed reasoning, tool cards, and text to a bare "Task cancelled" marker before a follow-up GET /api/session repopulated them — a visible race between the terminal cancel SSE event and the refetch (nesquena#4076, nesquena#1361). The terminal cancel event now carries a canonical, redacted session snapshot captured under the per-session agent lock right after the cancelled turn is persisted (including the recovered partial-assistant message). The frontend applies that embedded snapshot immediately via _applyCancelSessionPayload and skips the GET round-trip. The snapshot is a fully detached deep copy (redaction rebuilds every container) so it cannot alias or be staled by concurrent writes; it is only applied when its session_id matches the active view; and a missing/failed snapshot degrades cleanly to the existing GET fallback. The seven other cancel call sites are standardized via _cancel_event_payload() (message + type + status), backward-compatible (no session key when none). Co-authored-by: franksong2702 <franksong2702@users.noreply.github.com>
Thinking Path
/api/sessionfetch before it could render the canonical persisted state./api/session.What Changed
cancel_stream()after it persists the canonical partial/cancel state, while worker-side cancel exits stay metadata-only so they cannot make the browser trust an incomplete snapshot./api/sessionfallback.master; no contributorCHANGELOG.mdentry is included.Why It Matters
Users should not lose sight of already-streamed reasoning, tool activity, or partial assistant text just because they stopped a run. The cancel terminal event now has enough canonical state for the UI to render the preserved partial turn immediately instead of briefly collapsing to only a cancel marker.
Contract Routing
Task type: streaming/recovery cancellation fix
Touched areas: backend SSE cancel payload, frontend cancel terminal handler, anchor-scene settlement ordering
Relevant public docs:
AGENTS.mdCONTRIBUTING.mddocs/CONTRACTS.mddocs/UIUX-GUIDE.mdDESIGN.mddocs/rfcs/webui-run-state-consistency-contract.mddocs/rfcs/live-to-final-assistant-replies.mdScope boundaries: this preserves already-persisted cancel state in the terminal event; it does not redesign cancellation semantics or the full assistant turn anchor model.
Evidence needed before claiming done: backend cancel payload regression, frontend ordering regression, anchor settlement ordering regression, JS syntax check, adjacent cancel-stream regressions.
Verification
python3 -m py_compile api/streaming.pynode --check static/messages.jsgit diff --check./scripts/test.sh tests/test_cancelled_turn_status.py tests/test_issue1361_cancel_data_loss.py tests/test_live_to_final_anchor_visible_order.py -q— 91 passed../scripts/test.sh tests/test_streaming_race_fix.py tests/test_session_save_mode.py tests/test_issue2157_sessions_list_stale_stream_state.py -q— 33 passed../scripts/test.sh tests/test_cancel_interrupt.py tests/test_issue893_cancel_preserves_partial.py tests/test_issue1298_cancel_and_activity.py tests/test_stale_stream_writeback.py tests/test_sprint51.py -q— 45 passed.Risks / Follow-ups
/api/sessionfallback behavior remains in place if the payload is absent.sessionandsession_idfields, without removing the existing status/message fields.Model Used
OpenAI Codex GPT-5 with local repository inspection, regression tests, and GitHub CLI/API publishing.