fix(desktop): queue a busy-raced send as steering instead of dropping it - #3032
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks for moving the final busy-race decision back to turn.message.submit. Runtime Host remains the sole Turn/steering authority, and the main-process fallback is narrowly limited to session_busy.
I found two gaps that should be addressed before merge.
[P2] Rebind optimistic state without overwriting an authoritative live projection
When the fallback returns a Host-generated turnId, the renderer currently performs disarmTurnActive(oldId) followed by armTurnActive(hostId).
Host events may arrive before the IPC response because main emits the status change before returning. If the renderer has already received streamed or terminal state under hostId, the subsequent arm replaces that authoritative projection with a fresh waiting + unconfirmed projection. This can erase visible progress or leave the processing/Stop state stuck indefinitely.
Please use a single conditional rebind instead: rename only the exact unconfirmed + waiting arm created by this send. If an authoritative projection has already arrived, preserve it. The earlier #1954 review implementation had this rebindTurnActive behavior.
The new-chat branch should consume the same result normalization. It currently ignores both steered and a Host-chosen turnId, so a create/send race can leave a ghost optimistic Turn or associate the message with the wrong identity. The clean solution is one shared settlement helper for both new and existing Sessions.
Please cover at least:
- a streamed or terminal Host event arriving before the send response;
steeredon the new-chat path;turn_startedwith a Host-generated ID on the new-chat path.
[P2] Test the real Desktop Skill path
The fallback excludes command.skillIds, but the Desktop Composer represents Skills in the canonical text grammar as /skill:<id> tokens; it does not send skillIds. The new Skill test therefore exercises a protocol branch without a production renderer caller.
In the real path, a busy-raced /skill: send still enters turn.message.submit. A blocked invocation then becomes a generic operation error, while partial Skill-loading feedback can be lost because the submit result does not carry skillInvocation.
For the stated behavior—Skill sends retain their existing semantics—the minimal fix is to recognize canonical Skill tokens in command.text and keep them out of this fallback, then replace or supplement the current test with a /skill:<id> input. A larger future design could carry structured Skill feedback through turn.message.submit, but that is not required for this PR.
More generally, a discriminated result such as started(turnId) | steered | blocked(...) would be cleaner than { turnId, steered?: true }: it removes the ghost-turn state and prevents the two renderer branches from interpreting the contract differently.
The failing streaming-remount E2E calls sessions.steer directly and does not enter this fallback, so I do not consider it a production finding from this change. The check should still be rerun successfully before merge.
中文对照
感谢把 busy race 的最终判断交还给 turn.message.submit。Runtime Host 仍然是 Turn/steering 的唯一 authority,main 也只在 session_busy 时进入 fallback,整体方向正确。
合并前建议解决两个问题:
[P2] 重绑定 optimistic state 时不能覆盖已经到达的权威 projection
Host event 可能早于 IPC response 到达。当前的 disarm(oldId) → arm(hostId) 会把已经存在的 streamed 或 terminal projection 覆盖成新的 waiting + unconfirmed,可能清空可见进度,或者让 processing/Stop 状态永久无法回收。
应该只原子地重命名本次 send 创建的 unconfirmed + waiting arm;如果 Host projection 已经到达,则保持不变。新建聊天与已有 Session 也应该共享同一套结果归并逻辑,因为新建聊天路径目前没有处理 steered 和 Host 返回的新 turnId。
[P2] Skill 测试没有命中 Desktop 的真实路径
Desktop Composer 使用正文 /skill:<id> token,而不是 skillIds。当前守卫和测试因此只覆盖了没有生产 renderer caller 的协议分支。真实 Skill 在竞态时仍会进入 fallback,blocked feedback 会退化成通用错误,部分加载失败的结构化反馈也可能丢失。
最小修复是同时识别正文中的 canonical Skill token,让它保留原有 turn.start 语义,并用 /skill:<id> 补充或替换当前测试。让 turn.message.submit 正式携带 Skill feedback 可以作为后续更完整的设计。
streaming-remount E2E 直接调用 sessions.steer,没有经过本 PR 的 fallback,因此不属于本次生产 finding;但合并前仍应让 CI 重跑至绿色。
Disclosure: I used Codex reviewers and Claude Opus to assist with call-path tracing and adversarial checks. I reviewed the evidence and own this feedback.
|
Both points addressed in 2b7b2eb. Settlement: both send branches now consume one Skill path: the fallback now recognizes canonical Agreed on the discriminated Desktop main suite: 848 pass; typecheck clean. Will rerun the |
|
The failing |
03ee5b6 to
efef85f
Compare
📝 WalkthroughProblem solvedDesktop When Skill and orchestration sends keep the The renderer now:
Source of truth and solution scopeThe PR extends the existing Host routing and Desktop main remains the shared routing point for ordinary text, Skill, orchestration, and related send entrypoints. The preload contract adds only the optional The solution is coherent and targeted. The retry is limited to Simplification opportunitiesNo clear deletion is supported by the current diff. The new tests cover distinct race outcomes:
Further consolidation would risk weakening regression coverage or changing the race handling. Validation and risksThe desktop main suite passed 848 tests. Typechecks passed. A The main user-visible risk is changed Desktop send behavior during No security, licensing, release, or governance effect was identified in the current diff. Required checks are otherwise unverified from direct evidence here. The person performing the merge must review the final diff, and a maintainer makes the final determination. Review-relevant risks
WalkthroughDesktop busy-session sends now use steering or a newly started turn. The bridge reports steering and host-selected turn IDs. Renderer chat actions settle optimistic state for existing sessions and new chats, with tests covering race outcomes and streamed projections. ChangesBusy-session send settlement
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The change preserves the existing send and steering behavior while handling the busy race; no actionable merge-blocking risk remains after normal checks and review. Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.ts (1)
497-554: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAdd attachment coverage for the steering fallback.
This test verifies text forwarding but not attachment forwarding. A regression that drops
content.attachmentsduring thesubmitMessagefallback will pass. Send an approved attachment and assert thatsubmits[0].content.attachmentscontains the resolved attachment reference.As per path instructions, flag tests that do not protect observable behavior.
Source: Path instructions
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b330f50-2be8-4b9e-af35-d39e14f89e6a
📒 Files selected for processing (5)
apps/desktop/src/main/__tests__/app-shell-busy-race-settlement.test.tsapps/desktop/src/main/__tests__/runtime-host-session-execution-ipc-main.test.tsapps/desktop/src/main/runtime-host-session-execution-ipc-main.tsapps/desktop/src/preload/bridge-contract.d.tsapps/desktop/src/renderer/app-shell-chat-actions.ts
Included review availability: Your plan includes up to 3 reviews per rolling hour; 2 remain after this review.
The renderer already routes text at a session it sees as running to sessions:steer, but that reading is renderer-local state: another window, a Bot, or a Goal continuation can open the root Turn first, and the raced sessions:send then failed with session_busy — surfaced as a generic send error toast with the user's message dropped (the surviving slice of apache#1954 after the M5 cutover put an active-root guard on turn.start). Have sessions:send fall back to turn.message.submit on session_busy: the Host resolves the race atomically, queueing the text as steering into the running turn or starting the turn if the session went idle in between. Skill and orchestration sends keep the error — their turn semantics cannot be expressed as a queued message. The renderer skips new-turn bookkeeping for a steered result and re-keys its optimistic state when the fallback started the turn under a Host-chosen id. Fixes apache#1954 Generated-by: Claude Code
…path Review follow-up. Bookkeeping: both send branches now consume one settlement helper. A Host-chosen turnId rebinds only the exact unconfirmed waiting arm this send created — an authoritative live projection that beat the IPC response (main emits the sessions-changed nudge before returning) is preserved instead of being replaced with a fresh waiting arm. The new-chat branch now also understands steered results (navigate, but no ghost optimistic turn) and Host-chosen ids. Skill path: the Desktop composer carries Skills as canonical /skill: tokens in the text, not as skillIds, so the fallback now recognizes SKILL_INVOCATION_TOKEN_SOURCE in command.text and keeps the busy error for those sends too, preserving Skill feedback semantics. Generated-by: Claude Code
efef85f to
39eacf8
Compare
|
LGTM ! |
Summary
Fixes #1954 (the slice that survived the M5 cutover).
Since the production cutover,
turn.startcarries an active-root guard, so the parallel-turn/interleaving symptoms in #1954 no longer occur, and the composer already routes text at a session it sees as running tosessions:steer(the button flips to 插入消息). What remained: that reading is renderer-local state. Another window, a Bot, or a Goal continuation can open the root Turn first, and the racedsessions:sendthen failed withsession_busy— surfaced as a generic send-failure toast with the user's message dropped.What changed:
sessions:sendfalls back toturn.message.submit(placementcurrent_turn) whenturn.startreportssession_busy. The Host resolves the race atomically per feat(runtime-host): establish message authority foundation #1357's submit semantics: an active session queues the text as steering into the running turn; a session that went idle in between starts the turn (turn_started).skillIds),turnOrchestration, and canonical/skill:token sends rethrow the busy error instead — their turn semantics cannot be expressed as a queued message.settleSendBookkeepinghelper: it skips new-turn bookkeeping for asteeredresult (the steering_message event renders the text in the transcript, same as the composer steer path) and re-keys only the exact optimistic state this send created when the fallback started the turn under a Host-chosen id, preserving an authoritative projection that arrived first.Attachments survive the fallback: the steering injection path (
drainSteeringInto→appendImageParts) carriesMessageContent.attachments.Verification
turn_startedfallback, and busy preserved for Skill sends (both theskillIdsand/skill:text forms).app-shell-busy-race-settlement.test.ts): steering into an active turn, Host-id rebind, authoritative-projection-first preserved, and both new-chat outcomes without ghost optimistic turns.@maka/desktopmain suite: 848 pass; desktop typecheck (preload/main/renderer/storybook) clean.streaming-remount.spec.tse2e failure was bisected to a pre-existing virtualization regression, filed as fix(ui): returning to a live conversation intermittently re-streams accumulated output — regression from #2975, flaking CI #3044, fixed by fix(desktop): settle catch-up text before remounting a live conversation #3048; this branch is rebased past that fix.AI use
Select exactly one:
Tool(s) and scope: Claude Code authored the fix, the tests, and this description under human direction and review; commits carry
Generated-by: Claude Codetrailers.Checklist
Does this PR entail a change in behavior?