fix(webui): Fence stale session work by attachment identity - #8833
Merged
Conversation
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
This was referenced Aug 10, 2026
Collaborator
🖼️ 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 |
Contributor
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. |
doudouOUC
marked this pull request as ready for review
August 10, 2026 06:45
doudouOUC
enabled auto-merge
August 10, 2026 06:45
Collaborator
Collaborator
|
Thanks for the PR! Template: complete ✓ — all sections filled in, bilingual body included. Problem: real and well-tracked. This is the PR3a slice of the P1 session-restore effort in #8678, where PR1 already merged via #8691 and the original 5.7k-line #8824 was closed in favor of this split series. The races aren't hypothetical: the PR ships deterministic interleaving tests (two attachments with the same Direction: aligned. Session-restore reliability for the daemon-backed web UI is an active P1 track, and cutting the closed #8824 into independently releasable slices is exactly the shape we want. No direct CHANGELOG reference, but the area is clearly relevant. Size: core paths are not touched (all four files live in Approach: scope feels right. Every hunk either converts an ownership check from Risk: no elevated risk signals — none of the changed files match the revert-correlated paths from the repo's revert history. Moving on to code review. 🔍 中文说明感谢贡献！ 模板：完整 ✓ —— 各部分均已填写，并附中文说明。 问题：真实存在且有据可查。本 PR 是 #8678（P1 会话恢复工作）中的 PR3a 切片；该系列中 PR1 已通过 #8691 合入，原 5.7k 行的 #8824 已关闭并拆分为本系列。这些竞态不是假设：PR 为每个被隔离的表面（metadata、SSE、 方向：对齐。daemon 版 Web UI 的会话恢复可靠性是进行中的 P1 方向，把已关闭的 #8824 拆成可独立发布的切片正是我们期望的形态。CHANGELOG 无直接引用，但该领域明显相关。 规模：未触及核心路径（四个文件全部位于 方案：范围合理。每个改动要么把所有权检查从 风险：无升级风险信号——改动文件均未命中仓库 revert 历史中的高风险路径。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewBefore reading the diff, my own proposal for "late work from a replaced attachment corrupts the new one" would have been: capture the attachment object at the start of each async closure and re-check It is. Walking the fenced surfaces:
I grepped both head files for remaining Tests are the good kind: deterministic deferreds interleaving two same-ID attachments, one regression per fenced surface, including reload-interrupted (source kept, replacement detached), detach-exactly-once, and unmount with a late replacement result. No blockers, no convention violations. Test evidence (PR's own CI)Unattended run — the PR's tests are read from its CI via the API, not re-run here. The main unit suite (
Green CI shows the suite passes with the diff; it can't show the regressions fail without it. Sandboxed verification would settle that: Real-scenario testing: not applicable on this run — unattended CI never drives tmux, and the change is provider-internal concurrency with no user-visible TUI surface of its own. 中文说明代码审查：在读 diff 之前，我自己的方案是——在每个异步闭包开始时捕获 attachment 对象，在每次状态应用前重新检查 逐表面核对后确认贯彻到位：SSE 循环在每轮迭代开始处做同一性检查，sidechannel 与 pending-prompt 发布后补查，出错路径先断再加 notice；被替换时丢弃待发缓冲而不是刷入新 owner 的 store。批量转录的定时 flush 在 runner 不再拥有 provider 时丢弃缓冲，同步 flush 强制应用（resume 游标已越过这些事件，相应注释解释了原因）。metadata/providers/context/commands 在 我 grep 了两个头文件中剩余的 测试是好的形态：确定性 deferred 交错两个同 ID attachment，每个被隔离表面一条回归，包括 reload 被打断（保留源、detach replacement）、恰好 detach 一次、卸载后 replacement 迟到返回。未发现阻塞项或规范违规。 测试证据：无人值守运行，通过 API 读取 PR 自身 CI。主单测套件（ubuntu）在受审提交上为绿，web-shell smoke 与两个 desktop shell 任务亦为绿；macOS/Windows 的 Test 与 CLI 集成测试在该提交的工作流过滤下被跳过——如实报告，无红色项。 绿 CI 只能证明带 diff 时套件通过，无法证明回归测试在去掉 diff 后会失败。沙箱验证可以补齐这一点： 真实场景测试：本次不适用——无人值守 CI 不驱动 tmux，且该改动是 provider 内部并发加固，本身没有用户可见的 TUI 表面。 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 4/5 — solid, well-scoped hardening with deterministic tests and green CI; the only reservations are non-blocking (deferred follow-up surfaces, and the base-build pinning question left to Stepping back: this is what a good slice of a big PR looks like. The parent #8824 was 5.7k lines; this carve-out is 227 production lines doing one coherent thing — ownership identity for async session work — with ~592 lines of tests that encode each race as a deterministic interleave. My independent take on the problem landed on the same mechanism the PR uses (capture the attachment object, re-check identity before applying late results), so I don't think a materially simpler path was missed; a generation counter or clientId comparison would buy nothing extra here. The motivation checks out — this is PR3a of the P1 restore-reliability track in #8678, with PR1 already merged — and the implementation is mechanical in the best sense: every fenced surface gets the same treatment, the cleanup rebuild ( Two reservations, neither blocking:
No blocker found in the diff, CI is green on the reviewed commit, and the PR is honest about what it does and doesn't cover. LGTM. ✅ 中文说明置信度：4/5 —— 扎实、范围清晰的加固，配有确定性测试与绿色 CI；仅有的保留意见均不阻塞（延后的后续表面，以及留给 整体看：这是大 PR 拆片的范例。父 PR #8824 有 5.7k 行；本切片用 227 行生产代码做一件事——为异步会话工作确立所有权身份——并配以约 592 行测试，把每个竞态编码为确定性交错。我对该问题的独立方案与 PR 机制一致（捕获 attachment 对象、在应用迟到结果前重查同一性），因此不认为存在被错过的更简路径；代数计数器或 clientId 比较不会带来额外收益。 动机成立——这是 #8678 中 P1 恢复可靠性方向的 PR3a，PR1 已合入——实现也足够"机械"（褒义）：每个被隔离表面得到同样处理，清理重构（ 两点保留意见，均不阻塞：
diff 中未发现阻塞项，受审提交 CI 为绿，PR 对自身覆盖与未覆盖范围的表述诚实。LGTM。✅ — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
approved these changes
Aug 10, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅
yiliang114
approved these changes
Aug 10, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM. Verified at head 5201bf9: the fence is applied consistently at every async resume point across all six declared surfaces — SSE loop (top-of-iteration fence, re-checks after sidechannel/pending-prompt publishes, replaced-runner exit dropping the pending buffer), batched transcript flush (scheduled flush drops its buffer when ownership is lost, forced/sync flushes still apply), metadata refresh, heartbeat (failure state re-keyed per tick by attachment), and effect cleanup rebuilt around ownsCurrentSession/ownsEmptyState so a replaced runner can no longer detach or reset the replacement. The reload handoff retires exactly once: the skip flag is object-keyed, self-clears via .finally, and the unmount-before-settle path preserves the source and rejects the pending load while the late replacement is detached by the pre-existing disposed/aborted path — no double-retire or leak found. Identity is raw object reference of the SDK client instance (never serialized, StrictMode-stable), and the fence is strictly tighter than the old sessionId comparison so cross-session switching is unaffected. The same-ID replacement tests are genuinely adversarial (stale work resumes only after the replacement commits; they would fail on the pre-fix comparisons). CI green.
P3s only, none blocking: two skipNextCleanupDetachSessionRef clear sites remain sessionId-scoped (safe direction — recoverable extra detach, and replacement always clears the flag first — but worth a comment), the pre-existing unfenced passive-timer re-arm race (not a regression, belongs to the PR3b follow-ups), the deferred prompt-lifecycle surfaces still sessionId-scoped (explicitly out of scope), and terminal frames are fenced only via the shared SSE loop with proxy coverage. Ship it.
Collaborator
|
Released in v0.21.9. |
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
3 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.
What this PR does
This PR fences session-owned asynchronous work by the exact attachment object instead of only by
sessionId. Metadata refreshes, streamed events, terminal stream frames, heartbeat failures, transcript batches, and runner cleanup from a replaced attachment can no longer mutate or detach a newer attachment that happens to use the same session ID. Same-session reload handoff now preserves and retires the intended attachment exactly once, including when the provider unmounts before the replacement load settles.The existing detach-first session switching behavior remains unchanged. This is the independently releasable PR3a hardening slice extracted from #8824; transactional visible-state switching, WebShell coordination, resync/repair staging, and branch adoption remain follow-up work.
Why it's needed
The provider previously treated a session ID as sufficient ownership identity. Reattaching or reloading the same persisted session can create a new client attachment with the same session ID, so late work from the old attachment could overwrite the new model/context/commands, append stale transcript events, turn a late
session_closedor heartbeat 410 into a false disconnect, or clean up the replacement attachment. These races were reproduced deterministically on the pre-fix commit and could corrupt an otherwise successful same-session restore.Reviewer Test Plan
How to verify
Run
cd packages/webui && npx vitest run src/daemon/session/actions.test.ts src/daemon/session/DaemonSessionProvider.test.tsx. Confirm that all 250 tests pass, including same-ID replacement cases for metadata, SSE,session_closed, heartbeat, reload cleanup, and unmount with a late replacement result.Run
cd packages/webui && npm run typecheck && npm run lint && npm run build. Confirm that type checking, linting, declaration generation, and the production bundle complete successfully.The focused regressions use two attachment objects with the same
sessionIdand differentclientIdvalues. Before this fix they deterministically show stale metadata, stale SSE output, a false disconnect fromsession_closed, and a false missing-session state from heartbeat 410; after this fix the replacement remains connected and unchanged.Evidence (Before & After)
N/A — this is non-visual concurrency and ownership hardening covered by deterministic unit tests.
Tested on
Environment (optional)
macOS, Node.js v22.22.3, npm 10.9.8. Verified with the WebUI unit suite, typecheck, ESLint, and Vite production build.
Risk & Scope
Linked Issues
Refs #8678. Extracted from #8824 as the independently releasable PR3a slice.
中文说明
本 PR 做了什么
本 PR 使用精确的 attachment 对象而不再只依赖
sessionId来隔离会话所属的异步工作。被替换 attachment 的 metadata 刷新、流式事件、终止帧、heartbeat 失败、transcript 批次和 runner cleanup，不能再修改或 detach 使用相同 session ID 的新 attachment。same-session reload handoff 现在会准确保留并只回收一次目标 attachment，包括 Provider 在 replacement load 结束前卸载的情况。现有的 detach-first 会话切换行为保持不变。这是从 #8824 拆出的、可独立发布的 PR3a 加固切片；可见状态事务切换、WebShell 协调、resync/repair staging 和 branch adoption 仍作为后续工作。
为什么需要
Provider 之前把 session ID 当作充分的 ownership identity。重新 attach 或 reload 同一个持久化会话时，可能生成具有相同 session ID 的新 client attachment，因此旧 attachment 的迟到工作可能覆盖新会话的 model/context/commands、追加过期 transcript 事件、将迟到的
session_closed或 heartbeat 410 误判为断连，或清理 replacement attachment。这些竞态已经在修复前的提交上被确定性复现，并可能破坏原本成功的 same-session restore。Reviewer 测试计划
如何验证
运行
cd packages/webui && npx vitest run src/daemon/session/actions.test.ts src/daemon/session/DaemonSessionProvider.test.tsx。确认 250 个测试全部通过，其中包括 metadata、SSE、session_closed、heartbeat、reload cleanup，以及卸载后 replacement 迟到返回等 same-ID replacement 场景。运行
cd packages/webui && npm run typecheck && npm run lint && npm run build。确认类型检查、lint、声明生成和生产 bundle 均成功完成。聚焦回归测试使用两个具有相同
sessionId、不同clientId的 attachment 对象。修复前可确定性观察到旧 metadata、旧 SSE 输出、session_closed导致的误断连，以及 heartbeat 410 导致的错误 missing-session 状态；修复后 replacement 会保持 connected 且内容不变。证据（Before & After）
N/A —— 这是由确定性单元测试覆盖的非可视化并发与 ownership 加固。
测试平台
环境（可选）
macOS、Node.js v22.22.3、npm 10.9.8。已通过 WebUI 单测、typecheck、ESLint 和 Vite 生产构建验证。
风险与范围
关联 Issue
Refs #8678。本 PR 是从 #8824 拆出的、可独立发布的 PR3a 切片。