[webshell]retry session switch while closing - #8864
Conversation
macOS Desktop regression report
|
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: observed bug, not theory — the PR carries before/after screenshots and concrete repro steps (switching between two persisted sessions without waiting), and the daemon side confirms the mechanism: Direction: aligned — this is a reliability gap in the Web Shell session lifecycle, part of the #8092 effort. No direction concerns. Size: not applicable — Approach: scope feels right. Retrying the daemon's exact close-in-progress response on the existing reconnect backoff is the minimal client-side fix; the alternatives (daemon queuing loads while closing, or a new wire Risk: no elevated risk signals — neither changed file matches the revert-correlated path patterns. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题:已观测到的 bug,不是理论问题——PR 提供了 before/after 截图和具体复现步骤(在两个已保存会话间快速切换),daemon 侧也印证了机制:会话清理期间 方向:对齐——这是 Web Shell 会话生命周期的可靠性问题,属于 #8092 工作的一部分。无方向性顾虑。 规模:不适用—— 方案:范围合理。用现有的重连退避机制重试 daemon 明确的"关闭进行中"响应,是客户端侧的最小修复;备选方案(daemon 在关闭期间排队 load 请求、或为该 404 新增 wire 风险:无升级风险信号——两个改动文件均未命中与 revert 相关的高风险路径。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI traced the mechanism end to end before reading the diff, and the fix matches what I would have proposed. The daemon contract is real and the match is correct. The guard is narrow in the right way. The retry only fires for an explicitly requested session load ( The retry loop can't run away or leak. Backoff reuses The test pins the change. It drives the exact path: first No blockers found. One non-blocking observation: since this 404 carries no CI test evidenceUnattended run — I did not build or execute PR code; the evidence below is the PR's own CI, read via the API at the reviewed commit. At fetch time the gating unit suite ( Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The new unit test substantiates the retry mechanism itself, but what neither static review nor CI can settle is the race under real daemon teardown timing in the Desktop app — the before/after evidence is the author's, who verified on the macOS bundle only (Windows/Linux packages unvalidated per the PR body). Sandboxed verification would settle this: 中文说明代码审查:我先独立推演了机制再看 diff,修复方案与我的独立提议一致。daemon 侧契约真实存在且匹配正确—— 测试证据:无人值守运行——未构建或执行 PR 代码,以上为通过 API 读取的 PR 自身 CI。抓取时 ubuntu 单测套件与 Web-shell Visuals 仍在运行;macOS/Windows 矩阵任务按 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — clean, minimal fix that matches my independent proposal; the only reservation is that real-timing race verification rests on the author's macOS run, and CI hasn't landed yet. Stepping back: this is exactly the kind of fix the Web Shell needs right now — a small, scary-looking bug (error toast + dead-end "session does not exist" screen during ordinary rapid switching) with a root cause that is genuinely transient, and a fix that treats it as transient instead of terminal. I proposed the same approach before reading the diff: catch the daemon's exact close-in-progress 404 on the requested-session path and ride it out on the existing reconnect backoff. The PR does precisely that, in 28 production lines, with no drive-by changes and a unit test that fails without the fix. The parts I pressured hardest all check out: the daemon actually emits this response (four throw sites in The 4 rather than 5: the end-to-end race under real teardown timing is substantiated by the author's before/after on macOS only; Windows/Linux packages are unvalidated, and the mocked unit test can't stand in for live timing. The Approval is deferred until CI lands green on 中文说明置信度:4/5 —— 干净、最小的修复,与我的独立提议一致;唯一的保留是真实时序下的竞态验证依赖作者的 macOS 运行结果,且 CI 尚未结束。 整体来看:这正是 Web Shell 当前需要的修复——一个看起来吓人的 bug(快速切换时出现错误提示和"会话不存在"死胡同页面),根因是真实的瞬态状态,修复方案也把它当作瞬态而非终止状态处理。我在看 diff 之前就提出了相同方案:在请求会话路径上捕获 daemon 明确的"关闭进行中" 404,用现有重连退避等待其完成。PR 正是这么做的,28 行生产代码,无夹带改动,且单测在没有该修复时会失败。我重点核查的部分均通过:daemon 确实会发出该响应( — Qwen Code · qwen3.8-max Reviewed at |
🖼️ web-shell visual previewRendered against a mock daemon (no real backend): the PR base vs this PR head Screenshots · before / afterℹ️ No screenshot changed against the PR base — but this PR edits 1 render-shaping file:
Either the change has no visual effect (logic, plumbing, a state the scenarios never reach), or no scenario renders this UI — in which case the preview cannot see it, and an empty result is a coverage gap rather than a clean bill of health. To make it visible, add a scenario to Full-resolution recordings (.webm) are attached to the workflow run. — Qwen Code · web-shell visuals |
Code Coverage Summary
CLI Package - Full Text ReportCore Package - Full Text ReportFor detailed HTML reports, please see the 'coverage-reports-22.x-ubuntu-latest' artifact from the main CI run. |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): This PR fixes a Web Shell race: switching back to a sessi...: verification of which sessionScope the shipping web-shell/desktop consumers pass to DaemonSessionProvider (reachability of finding 2).
中文说明
未探索到全部深度(达到工具调用预算):This PR fixes a Web Shell race: switching back to a sessi...:verification of which sessionScope the shipping web-shell/desktop consumers pass to DaemonSessionProvider (reachability of finding 2)。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Final closeout for merged PR head
中文摘要PR 已以旧 head 合并。6 条越界或 mutation-only 建议已说明并关闭;经验证的一行恢复修复已转为 follow-up PR #8883,原 PR 的 review threads 已全部关闭。 |
doudouOUC
left a comment
There was a problem hiding this comment.
Reviewed exact head efd58dc7200eeadc741a90243d48b14ff5e177e2. No blocking findings: the prior watchdog/connecting Critical and the autoReconnect: false regression are fixed, superseding switches are covered, and the retry remains scoped to explicit action-driven loads as documented. Exact-head verification: 254/254 focused WebUI tests passed, package typecheck passed, changed-file Prettier and git diff --check passed, and the Web Shell E2E smoke passed. The Ubuntu aggregate job failed in unrelated CLI/Core tests during a broad burst of 15-second timeouts and shared temporary-root errors; the changed WebUI suite itself passed. Approving with two non-blocking cleanup/test-hardening suggestions.
wenshao
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): This PR retries explicit session switches in the Web Shel...: did not run the full DaemonSessionProvider.test.tsx / actions.test.ts suites (only the targeted tests), and did not inspect the sidebar currentSessionIdent…; This PR retries explicit session switches in the Web Shel...: did not run the three new tests or any vitest run — verdicts are static-analysis based.; This PR retries explicit session switches in the Web Shel...: the discriminator's contract with the real daemon (exact 404 body message and status for a closing session) is not verifiable from this repo alone; both the tes…; This PR retries explicit session switches in the Web Shel...: could not execute the new tests (worktree lacks node_modules ; vitest fails at config load on vite-plugin-dts ).; This PR retries explicit session switches in the Web Shel...: did not enumerate the web-shell (packages/web-shell) UI consumers of connection.status transitions in full — verified the status value 'disconnected' is pre-exi…, and 5 more.
中文说明
已审查。 建议见行内评论。 未探索到全部深度(达到工具调用预算):This PR retries explicit session switches in the Web Shel...:did not run the full DaemonSessionProvider.test.tsx / actions.test.ts suites (only the targeted tests), and did not inspect the sidebar currentSessionIdent…;This PR retries explicit session switches in the Web Shel...:did not run the three new tests or any vitest run — verdicts are static-analysis based.;This PR retries explicit session switches in the Web Shel...:the discriminator's contract with the real daemon (exact 404 body message and status for a closing session) is not verifiable from this repo alone; both the tes…;This PR retries explicit session switches in the Web Shel...:could not execute the new tests (worktree lacks node_modules ; vitest fails at config load on vite-plugin-dts ).;This PR retries explicit session switches in the Web Shel...:did not enumerate the web-shell (packages/web-shell) UI consumers of connection.status transitions in full — verified the status value 'disconnected' is pre-exi…,另有 5 条。
— DeepSeek/deepseek-v4-flash via Qwen Code /review (v0.21.8)
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Reviewed. Suggestions are inline. Not explored to full depth (tool budget reached): Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...: did not verify whether reconnectAttempt is reset on the loop's successful-connect path after a closing-retry (i.e., whether attempts accumulated during closin…; Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...: did not trace reloadSession 's exact interaction with the new cleanup identity guard for the live-journal-repair case (repair-triggered pending load present wh…; Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...: didn't read the full context of the 4th closing site at bridge.ts:6457 (verified the message string is identical via grep, but not the surrounding admission log…; Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...: didn't verify the daemon-side HTTP error mapping code (the body shape {error, sessionId} is taken from the PR's own tests).; Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...: can't see the existing PR comments, so can't rule out overlap with issues already discussed., and 5 more.
中文说明
已审查。 建议见行内评论。 未探索到全部深度(达到工具调用预算):Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...:did not verify whether reconnectAttempt is reset on the loop's successful-connect path after a closing-retry (i.e., whether attempts accumulated during closin…;Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...:did not trace reloadSession 's exact interaction with the new cleanup identity guard for the live-journal-repair case (repair-triggered pending load present wh…;Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...:didn't read the full context of the 4th closing site at bridge.ts:6457 (verified the message string is identical via grep, but not the surrounding admission log…;Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...:didn't verify the daemon-side HTTP error mapping code (the body shape {error, sessionId} is taken from the PR's own tests).;Context: PR 8864 (QwenLM/qwen-code), round 2 review of a ...:can't see the existing PR comments, so can't rule out overlap with issues already discussed.,另有 5 条。
— qwen3.8-max via Qwen Code /review (v0.21.8)
|
Released in v0.21.9. |
…osing retry (QwenLM#8884) * fix: add structured error code to SessionNotFoundError responses PR QwenLM#8864 retried session switches while the target session is closing, but relied on fragile string matching against the daemon's error message. This commit: 1. Adds a `code` property to `SessionNotFoundError` — automatically set to `'session_closing'` when the extra message mentions "closing", otherwise `'session_not_found'`. 2. Includes `code` in the HTTP JSON response body so clients can distinguish closing (transient) from genuinely missing sessions without depending on error message text. 3. Updates the WebUI retry check in `DaemonSessionProvider` to use `errorBody.code === 'session_closing'` instead of matching `endsWith('The session is closing; retry after close completes')`. 4. Fixes an inconsistent error message in `rewindSession` that used the short `'The session is closing'` without the retry suffix. Closes: QwenLM#8864 (follow-up) * fix(daemon): expose session closing code * docs(serve): document session closing codes * fix(acp): preserve closing code after restore waits * fix: restore class pin in bridge test and update error taxonomy - Add toBeInstanceOf(SessionNotFoundError) alongside toMatchObject to preserve the envelope type assertion - Document session_closing code in 18-error-taxonomy.md * chore: drop unrelated merge formatting





What this PR does
Keeps an explicit session switch pending when the daemon reports that the target session is still closing, then retries it with the existing reconnect backoff. Watchdog expiry now settles a stranded switch as disconnected, a newer switch supersedes the old retry cleanly, and
autoReconnect={false}still fails fast. Other missing-session responses keep their current terminal behavior.Why it's needed
Switching away from a session starts an asynchronous teardown. Switching back before that teardown finishes returns a transient 404, but the Web Shell treated it as a permanently missing session and showed both an error toast and the “current session does not exist” screen.
Reviewer Test Plan
How to verify
Open two persisted sessions and switch between them repeatedly without waiting between switches. A transient closing response may occur internally, but the requested session should eventually open without an error toast or missing-session screen.
Evidence (Before & After)
Tested on
Environment (optional)
Locally built macOS Desktop app bundle and its packaged runtime with the production Web Shell.
Risk & Scope
Linked Issues
Related to #8092.
中文说明
这个 PR 做了什么
当 daemon 返回目标会话仍在关闭中时,保持显式的会话切换请求,并使用现有的重连退避机制重试。watchdog 到期时,滞留的切换会确定性进入 disconnected;新切换会干净地取代旧重试;
autoReconnect={false}仍会快速失败。其他会话不存在响应保持当前终止行为。为什么需要
切离一个会话会启动异步清理。如果在清理结束前切回,daemon 会返回临时 404,但 Web Shell 之前会把它当作会话永久不存在,同时显示错误提示和“当前会话不存在”页面。
Reviewer Test Plan
如何验证
打开两个已保存的会话,并在不额外等待的情况下反复切换。内部可能短暂出现 closing 响应,但目标会话最终应正常打开,且不显示错误提示或会话不存在页面。
证据(修复前后)
测试平台
环境(可选)
本地构建的 macOS Desktop APP bundle、其内置 runtime 和生产 Web Shell。
风险与范围
关联 Issue
关联 #8092。