fix(webui): Make same-session refresh transactional - #8939
Conversation
E2E test reportValidated the focused real-daemon/JSDOM scenarios on macOS 26.4.1 with Node.js 22.22.3: QWEN_SANDBOX=false NODE_OPTIONS=--no-experimental-webstorage npx vitest run --root ./integration-tests cli/qwen-serve-webui-same-session-refresh.test.ts --reporter=dotResult: 3/3 passed in 21.26s.
Artifacts: The held-response case prints existing React |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Not explored to full depth (tool budget reached): chunk 9: could not execute packages/webui/src/daemon/session/actions.test.ts — no node_modules in the review worktree or parent checkout, and a full monorepo npm in…; You are review agent reverse-audit — Reverse audit agen...: none — all planned checks completed within budget.; You are review agent reverse-audit — Reverse audit agen...: none — all checks above completed within budget.; Context: PR #8939 makes same-session WebUI refresh/rebind...: I did not run builds/tests for empirical verification (my dimension is static cross-file tracing). Also did not exhaustively walk all 15k lines of DaemonSession…; Context: PR #8939 makes same-session WebUI refresh/rebind...: none material — I did not execute builds/tests (static trace only) and did not page through the untouched bulk of DaemonSessionProvider.test.tsx beyond the di…, and 13 more.
Not reviewed: reverse audit — did not converge within the reverse-audit round cap of 5.
中文说明
未探索到全部深度（达到工具调用预算）：chunk 9：could not execute packages/webui/src/daemon/session/actions.test.ts — no node_modules in the review worktree or parent checkout, and a full monorepo npm in…；You are review agent reverse-audit — Reverse audit agen...：none — all planned checks completed within budget.；You are review agent reverse-audit — Reverse audit agen...：none — all checks above completed within budget.；Context: PR #8939 makes same-session WebUI refresh/rebind...：I did not run builds/tests for empirical verification (my dimension is static cross-file tracing). Also did not exhaustively walk all 15k lines of DaemonSession…；Context: PR #8939 makes same-session WebUI refresh/rebind...：none material — I did not execute builds/tests (static trace only) and did not page through the untouched bulk of DaemonSessionProvider.test.tsx beyond the di…，另有 13 条。
未审查：反向审计——在 5 轮的反审轮数上限内未收敛。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| activePromptsRef.current.has(activeSession.sessionId) || | ||
| activePromptsRef.current.has(`${activeSession.sessionId}:shell`); |
There was a problem hiding this comment.
[Critical] R1-1: The shell-aware hasSessionActivePrompt() folds the ${sessionId}:shell entry into the runnerActiveTurn latch at the turn-terminal re-evaluation, and nothing re-evaluates the latch when the shell command finishes afterwards (sendShellCommand's finally only deletes the map entry and sets the prompt status; user_shell_result is not a terminal event). snapshot().activeTurn stays stale-true on an idle session, and any same-session refresh parks in the queued branch — which arms no watchdog — forever. Probe-reproduced on the unmodified PR: shell in flight → turn_complete → shell resolves → loadSession hangs in queued with the restore RPC never issued; re-evaluating the turn state without the shell entry flips it to a committed refresh while the existing wait-for-shell test still passes.
Failure scenario: user runs a shell command during a turn → both finish → reloadSession() never settles and the UI shows a pending transition indefinitely until a page reload, a prop-driven session change, or another turn's terminal event.
Suggested fix (make the state self-correcting):
// in snapshot():
activeTurn:
runnerActiveTurn ||
hasCurrentSessionActivePromptRef.current(),or re-evaluate runnerActiveTurn and queue a transition pump from the shell-completion path.
中文说明
shell 感知的 hasSessionActivePrompt() 会在 turn 终止事件的重新评估中把 ${sessionId}:shell 条目折叠进 runnerActiveTurn 锁存值，而 shell 命令随后结束时没有任何路径重新评估该锁存值（sendShellCommand 的 finally 只删除 map 条目并设置 prompt 状态；user_shell_result 不是终止事件）。空闲会话上 snapshot().activeTurn 会一直保持过期的 true，任何同会话刷新都会停在不设置 watchdog 的 queued 分支，永远无法推进。已在未修改的 PR 上用探针复现：shell 执行中 → turn_complete → shell 结束 → loadSession 卡在 queued，restore RPC 始终未发出；把 shell 条目从终止重估中去掉后探针变为成功提交，且现有的等待 shell 测试仍然通过。
失败场景：用户在一个 turn 中执行 shell 命令 → 两者都结束 → reloadSession() 永不 settle，UI 无限期显示进行中的转场，直到页面重载、prop 驱动的会话变更或另一个 turn 的终止事件。
建议修复（让状态可自我纠正）：在 snapshot() 中改为 activeTurn: runnerActiveTurn || hasCurrentSessionActivePromptRef.current()（实时读取，shell 条目删除后自动失效），或在 shell 完成路径重新评估 runnerActiveTurn 并触发一次 transition pump。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| intent.timeout = setTimeout(() => { | ||
| if (intent.candidate) { | ||
| retireAttachment(intent.candidate, intent); |
There was a problem hiding this comment.
[Critical] R1-2: A settle path retires neither the staged candidate nor the armed capture. The pump-top deadline check (~3782-3787; the environment check at ~3771-3781 has the same hole) settles the intent via exposeCrossSessionFailure, which does not retire intent.candidate and does not clear runnerControlRef.current.capture — and settleCrossSessionIntent then cancels this watchdog, the only timeout path that retires the candidate first. All other settle sites added in this diff retire the candidate before settling; this shared failure function is the exception. Newly introduced by this diff: pre-PR, intent.candidate did not exist.
Failure scenario: the deadline (default 75s) expires while the staged candidate waits for the source tail to drain — reliably so in throttled/background tabs, where the next event-driven pump lands before the watchdog macrotask → the daemon attachment leaks until the reaper, and the still-armed capture serializes every subsequent live event into an orphan buffer (up to 1024 events / 8 MiB).
Suggested fix — make exposeCrossSessionFailure the single cleanup sink: before settling, retire intent.candidate and clear runnerControlRef.current.capture when it equals intent.capture, mirroring this watchdog callback body.
中文说明
存在一条 settle 路径既不回收已暂存的 candidate、也不清除已挂载的 capture。pump 顶部的 deadline 检查（约 3782-3787 行；约 3771-3781 行的 environment 检查有同样的漏洞）通过 exposeCrossSessionFailure 结束 intent，但该函数既不回收 intent.candidate，也不清除 runnerControlRef.current.capture —— 随后 settleCrossSessionIntent 取消了这个 watchdog（唯一会先回收 candidate 的超时路径）。本 diff 新增的所有其他 settle 点都会先回收 candidate，只有这个共享失败函数例外。这是本 diff 新引入的问题：PR 之前不存在 intent.candidate。
失败场景：默认 75s 的 deadline 在 staged candidate 等待 source tail 排空期间到期（在被节流/后台的标签页中必然发生，因为事件驱动的 pump 会先于 watchdog 宏任务到达）→ daemon 端 attachment 泄漏直到 reaper 回收，且仍然挂载的 capture 会把之后每个 live 事件序列化进一个无人消费的孤儿缓冲区（最多 1024 个事件 / 8 MiB）。
建议修复：让 exposeCrossSessionFailure 成为统一的清理出口 —— 在 settle 之前回收 intent.candidate，并在 runnerControlRef.current.capture === intent.capture 时清除它，与本 watchdog 回调体保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if ( | ||
| intent.sameLogical && | ||
| (candidate.eventEpoch === undefined || |
There was a problem hiding this comment.
[Critical] R1-12: The integrity check hard-requires candidate.eventEpoch, but the daemon's bounded-refresh load path (refreshedReplayFieldsFor in packages/acp-bridge/src/bridge.ts, taken whenever the load carries historyPageSize — which the Web Shell always sends, default 100) returns no eventEpoch in its idle-session success branch. Probe-verified: refreshed.eventEpoch is undefined while loaded.eventEpoch is a string, and adding the epoch to that branch flips the probe to pass. A busy-session refresh succeeds because it falls back to replayFieldsFor, which carries the epoch. Neither test layer catches this: the unit mocks always supply eventEpoch: 'epoch-1', and no integration test sets historyPageSize.
Failure scenario: refreshing an idle session under the default Web Shell configuration fails with 'Session refresh returned an incomplete snapshot' on every retry — the headline feature of this PR regresses on its happy path.
Suggested fix (packages/acp-bridge/src/bridge.ts, refreshedReplayFieldsFor success branch): add eventEpoch: entry.events.epoch (mirroring replayFieldsFor, which includes it in every branch), plus a regression test for bounded-refresh loads carrying eventEpoch.
中文说明
完整性检查硬性要求 candidate.eventEpoch，但 daemon 的有界刷新 load 路径（packages/acp-bridge/src/bridge.ts 中的 refreshedReplayFieldsFor，凡是 load 携带 historyPageSize 就会走该路径 —— Web Shell 总是会发送，默认 100）在其空闲会话成功分支中不返回 eventEpoch。已用探针验证：refreshed.eventEpoch 为 undefined 而 loaded.eventEpoch 是字符串；在该分支补上 epoch 后探针即通过。忙碌会话的刷新能成功，是因为回落到 replayFieldsFor（其每个分支都携带 epoch）。两层测试都抓不到该问题：单测 mock 总是提供 eventEpoch: 'epoch-1'，集成测试没有任何一处设置 historyPageSize。
失败场景：在默认 Web Shell 配置下刷新空闲会话，每次都会以 'Session refresh returned an incomplete snapshot' 失败 —— 本 PR 的核心特性在其正常路径（happy path）上发生回归。
建议修复：在 packages/acp-bridge/src/bridge.ts 的 refreshedReplayFieldsFor 成功分支中加入 eventEpoch: entry.events.epoch（与 replayFieldsFor 保持一致），并补充针对有界刷新 load 携带 eventEpoch 的回归测试。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| retireAttachment(candidate, intent); | ||
| exposeCrossSessionFailure( | ||
| intent, | ||
| new Error('Session refresh failed integrity validation'), | ||
| ); |
There was a problem hiding this comment.
[Critical] R1-19: The three settle sites in this candidate branch (capture-invalid/attachment-change at ~3803-3814, deadline at ~3817-3823, and this commit-integrity failure) retire the candidate but never clear runnerControlRef.current.capture. The capture is installed at staging and deliberately preserved by the RPC .finally hand-off; settleCrossSessionIntent/exposeCrossSessionFailure don't clear it, and the settle's clearTimeout cancels the watchdog that would have. Same leak class as the pump-top settle path, but these triggers need no deadline: staged.repair or a daemon.replay_event_malformed notice fires the integrity site directly, and an event-driven pump commonly lands before the watchdog macrotask.
Failure scenario: after any of these recoverable refresh failures, captureSourceEvent keeps JSON-serializing and buffering every live event (≤1024 events / 8 MiB) into a buffer nobody consumes — pinned until the next refresh staging, session switch, or unmount — plus per-event serialization overhead on the live session.
Suggested fix — at each site, or centrally in exposeCrossSessionFailure, mirror the guarded clear used by cancelCrossSessionTransition:
const control = runnerControlRef.current;
if (control && intent.capture && control.capture === intent.capture) {
control.capture = undefined;
}中文说明
该 candidate 分支中的三个 settle 点（约 3803-3814 行的 capture 失效/attachment 变更、约 3817-3823 行的 deadline，以及此处的 commit 完整性失败）都会回收 candidate，但从不清除 runnerControlRef.current.capture。capture 在 staging 时挂载，并被 RPC 的 .finally 交接逻辑有意保留；settleCrossSessionIntent/exposeCrossSessionFailure 都不清除它，而 settle 的 clearTimeout 又会取消本应清除它的 watchdog。与 pump 顶部 settle 路径属于同一泄漏类别，但这些触发不需要 deadline：staged.repair 或 daemon.replay_event_malformed 通知会直接触发完整性失败点，且事件驱动的 pump 通常先于 watchdog 宏任务到达。
失败场景：上述任一可恢复的刷新失败之后，captureSourceEvent 会继续把每个 live 事件 JSON 序列化并缓冲（≤1024 个事件 / 8 MiB）进一个无人消费的缓冲区 —— 直到下一次 refresh staging、会话切换或卸载才释放 —— 并给 live 会话带来逐事件的序列化开销。
建议修复：在每个失败点（或集中在 exposeCrossSessionFailure 中）仿照 cancelCrossSessionTransition 的带保护清除逻辑清除 capture。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| if ( | ||
| sourceBoundOperationInFlight && | ||
| (current === undefined || |
There was a problem hiding this comment.
[Critical] R1-20: A controlled (prop-driven) session switch rejected by this early-reject is dropped permanently. The provider's controlled effect stamps lastHandledSessionIdRef/lastHandledClientIdRef before the async outcome and swallows the rejection (void request.catch(console.warn)), so nothing retries and no notice or transition is published. The source-bound window during createSession (trackCreate) is new in this diff; pre-PR this interleaving took the legacy path and the request was processed. Probe-flipped both arms: on the PR tree, a prop set during the create window produces no load attempt and no published transition failure, and after the create commits the provider permanently shows the created session while the prop says session B; deleting the early-reject makes the same probe handle the request via the legacy path.
Failure scenario: the Web Shell first-prompt bootstrap opens the up-to-30s source-bound create window; the host sets the sessionId prop inside it (deep-link/back navigation) → silent drop; after the create commits, a permanent created-session-vs-prop divergence with zero user-visible feedback.
Suggested fix: don't stamp the lastHandled* refs until the request settles (or clear them on rejection so the effect can re-fire), or retry once on the source-bound InvalidStateError; additionally route the legacy-path rejection through addNotice for parity with rejectPreflight.
中文说明
被这个 early-reject 拒绝的受控（prop 驱动）会话切换会被永久丢弃。provider 的受控 effect 在异步结果返回之前就记录了 lastHandledSessionIdRef/lastHandledClientIdRef，并且用 void request.catch(console.warn) 吞掉拒绝 —— 因此没有任何重试，也不会发布任何通知或转场状态。createSession（trackCreate）引入的 source-bound 窗口是本 diff 新增的；PR 之前同样的时序会走 legacy 路径，请求会被正常处理。探针双向往返验证：在 PR 代码上，create 窗口内设置 prop 不会产生任何 load 调用、也不发布转场失败，create 提交后 provider 永久显示新建的会话而 prop 指向会话 B；删除该 early-reject 后同一探针会通过 legacy 路径正常处理请求。
失败场景：Web Shell 首个 prompt 的引导流程会打开最长 30s 的 source-bound create 窗口；宿主在此期间设置 sessionId prop（深链/返回导航）→ 切换被静默丢弃；create 提交后，provider 状态与 prop 永久不一致，且用户看不到任何反馈。
建议修复：在请求 settle 之前不要记录 lastHandled* ref（或在被拒绝时清除它们以便 effect 可以重新触发），或对 source-bound 的 InvalidStateError 重试一次；同时把 legacy 路径的拒绝也通过 addNotice 发布，与 rejectPreflight 保持一致。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| it.each([ | ||
| ['epoch mismatch', { eventEpoch: 'epoch-2' }], | ||
| ['incomplete replay', { replaySnapshotComplete: false }], |
There was a problem hiding this comment.
[Suggestion] R1-17: The same-session integrity gate applies its epoch/partial/error/degraded clauses to both load and resume intents, but all five rejection rows run via loadSession; the only same-session resumeSession test fails at the RPC level, and the cross-session resume tests skip the gate (sameLogical: false) — so resume-mode integrity has zero coverage.
Concrete cost: a refactor dropping or misplacing the intent.mode === 'load' guard (or splitting the check and dropping a mode-agnostic clause — epoch, replayPartial, replayError, replayDegraded — from the resume side) would make healthy same-session resumes reject with 'Session refresh returned an incomplete snapshot', breaking the transactional resume this PR introduces, while the suite stays green.
Suggested fix: add a resume-mode case — resumeSession('session-a') with a candidate carrying eventEpoch: 'epoch-2' (or replayPartial: true), asserting the rejection message, the intact source transcript, and the candidate detached.
中文说明
同会话完整性门对 load 与 resume intent 都应用 epoch/partial/error/degraded 条款，但五个拒绝用例全部通过 loadSession 运行；唯一的同会话 resumeSession 测试在 RPC 层就失败了，跨会话 resume 测试则跳过该门（sameLogical: false）—— 因此 resume 模式的完整性路径零覆盖。
具体代价：如果重构丢弃或错放了 intent.mode === 'load' 守卫（或拆分检查并把模式无关条款 —— epoch、replayPartial、replayError、replayDegraded —— 从 resume 一侧丢掉），健康的同会话 resume 会被 'Session refresh returned an incomplete snapshot' 拒绝，破坏本 PR 引入的事务化 resume，而测试套件保持绿色。
建议修复：新增 resume 模式用例 —— 用携带 eventEpoch: 'epoch-2'（或 replayPartial: true）的 candidate 调用 resumeSession('session-a')，断言拒绝消息、source transcript 完整以及 candidate 被 detach。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| readonly replaySnapshotComplete: boolean; | ||
| readonly replayPartial: boolean; | ||
| readonly replayError: string | undefined; |
There was a problem hiding this comment.
[Suggestion] R1-23: The new replayError diagnostic is read only as a boolean (candidate.replayError !== undefined) at its sole consumer (the provider's same-session candidate gate); the string it exists to carry is discarded, and the refresh failure publishes the hardcoded 'Session refresh returned an incomplete snapshot'. The same provider file's transcript-pagination path folds this exact field into its error (page.replayError ?? 'Earlier session history was only partially read') — the author surfaces the field elsewhere; the refresh path drops it.
Concrete cost: when persisted-journal reconstruction fails during load — the exact case partial/replayError were added for — the user/operator sees a generic failure with no way to tell a transient transcript-page read error from real corruption without daemon logs.
Suggested fix: at the gate's exposure site, fold in the available detail — append ': ' + candidate.replayError when present, or ' (partial replay)' / ' (degraded replay)' markers otherwise.
中文说明
新增的 replayError 诊断信息在其唯一消费点（provider 的同会话 candidate 门）只被当作布尔值读取（candidate.replayError !== undefined）；它本应携带的字符串被丢弃，刷新失败对外发布的是硬编码的 'Session refresh returned an incomplete snapshot'。同一个 provider 文件的 transcript 分页路径却把同一字段拼进了错误消息（page.replayError ?? 'Earlier session history was only partially read'）—— 作者在别处展示了该字段，刷新路径却丢掉了它。
具体代价：当 load 期间持久化 journal 重建失败 —— 正是 partial/replayError 被加入的场景 —— 用户/运维只能看到泛化失败，不查 daemon 日志无法区分瞬时的 transcript 分页读取错误和真正的数据损坏。
建议修复：在门对外暴露错误处拼入可用的细节，例如把 candidate.replayError（或 '(partial replay)' 等标记）附加到错误消息中。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| ? `Could not refresh session ${target.sessionId}. The current attachment is still active.` | ||
| : `Could not open session ${target.sessionId}. The current session is still active.`, |
There was a problem hiding this comment.
[Suggestion] R1-26: Every same-session refresh failure surfaces as 'The current attachment is still active' with recoverable: true, regardless of the cause carried in debugMessage — publishCrossSessionFailure is the single notice sink for transition timeout, integrity-validation failure, incomplete snapshot, invalid owner identity, attachment change, and capture invalidation alike.
Concrete cost: on an idle session whose refresh failed integrity validation (e.g. the incomplete-snapshot path above), the notice asserts a busy-attachment condition that does not exist and invites retries that keep failing deterministically, with no hint of the real cause.
Suggested fix: keep the 'still active' wording only for activity-class rejections; for transition-time failures use a cause-neutral same-session message or derive it from the error class.
中文说明
无论 debugMessage 携带的真实原因是什么，所有同会话刷新失败都对外显示 'The current attachment is still active' 且 recoverable: true —— publishCrossSessionFailure 是转场超时、完整性校验失败、快照不完整、owner 身份无效、attachment 变更、capture 失效等所有失败的统一通知出口。
具体代价：在一个空闲会话上，如果刷新因完整性校验失败（例如上面的快照不完整路径），通知却声称存在并不存在的 attachment 忙碌状态，并诱导用户不断重试一个必然继续失败的请求，且看不到任何真实原因的提示。
建议修复：'still active' 措辞只保留给活动类拒绝；对转场期失败使用原因中性的同会话消息，或根据错误类别生成消息。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| () => { | ||
| rawCreateSettled = true; | ||
| setSourceBoundOperationInFlight(false); | ||
| }, | ||
| ); |
There was a problem hiding this comment.
[Suggestion] R1-27: This rejection handler — the only code that releases the source-bound flag when a raw create request fails — is exercised by zero tests: the two new timeout tests only ever resolve the deferred create, and no test rejects createOrAttachSession/createDetachedSession. Mutation-verified: deleting setSourceBoundOperationInFlight(false) here keeps all 330 tests green; a probe rejecting the deferred create flips ([[true]] stuck under the mutation vs [[true],[false]] on the PR code).
Concrete cost: a future edit breaking this branch leaves sourceBoundOperationCountRef stuck at 1 after a routine failed create (daemon 5xx / network error) — every requireStableSession() action and every load/resume/reload throws 'Another session operation is already in progress' until the provider remounts.
Suggested fix: add a test mirroring the timeout tests but rejecting the deferred create — assert the flag sequence [[true],[false]], no retire/detach issued, and a follow-up loadSession/createSession not blocked; ideally also a reject-before-timeout case.
中文说明
这个拒绝处理器 —— 原始 create 请求失败时唯一释放 source-bound 标志的代码 —— 零测试覆盖：两个新的超时测试只会 resolve 延迟的 create，没有任何测试 reject createOrAttachSession/createDetachedSession。已通过变异验证：删除此处的 setSourceBoundOperationInFlight(false) 后全部 330 个测试保持绿色；用 reject 延迟 create 的探针则发生翻转（变异下标志停在 [[true]]，PR 代码下为 [[true],[false]]）。
具体代价：未来破坏该分支的改动会让一次普通的 create 失败（daemon 5xx / 网络错误）后 sourceBoundOperationCountRef 永久停在 1 —— 每个 requireStableSession() 动作和每次 load/resume/reload 都会抛出 'Another session operation is already in progress'，直到 provider 重新挂载。
建议修复：仿照超时测试新增一个 reject 延迟 create 的测试 —— 断言标志序列 [[true],[false]]、不触发 retire/detach、后续 loadSession/createSession 不被阻塞；最好再加一个超时前 reject 的用例。
— qwen3.8-max via Qwen Code /review (v0.21.9)
| replayError: opts.replayError, | ||
| eventEpoch: opts.eventEpoch ?? 'epoch-1', | ||
| lastEventId: opts.lastEventId, |
There was a problem hiding this comment.
[Suggestion] R1-28: opts.eventEpoch ?? 'epoch-1' silently coerces an explicit eventEpoch: undefined, so the suite cannot construct an epoch-less session through the standard factory spelling (the spelling compiles — the project does not enable exactOptionalPropertyTypes) — precisely the session class whose refresh regression is confirmed above (virtual/epoch-less sessions). Today's only epoch overrides are 'epoch-1'/'epoch-2'; no test can reach the epoch-less gate behavior.
Concrete cost: when the epoch-less refresh regression is fixed, the natural regression test createMockSession({ eventEpoch: undefined }) silently exercises the epoch-having happy path, stays green, and blesses the regression it was written to catch.
| replayError: opts.replayError, | |
| eventEpoch: opts.eventEpoch ?? 'epoch-1', | |
| lastEventId: opts.lastEventId, | |
| replayError: opts.replayError, | |
| eventEpoch: Object.hasOwn(opts, 'eventEpoch') ? opts.eventEpoch : 'epoch-1', | |
| lastEventId: opts.lastEventId, |
中文说明
opts.eventEpoch ?? 'epoch-1' 会把显式传入的 eventEpoch: undefined 静默强制转换为默认值，因此测试套件无法通过标准工厂写法构造无 epoch 的会话（该写法可以编译 —— 项目未启用 exactOptionalPropertyTypes）—— 而这正是上面已确认存在刷新回归的会话类别（虚拟/无 epoch 会话）。目前仅有的 epoch 覆盖值是 'epoch-1'/'epoch-2'；没有任何测试能触达无 epoch 的门行为。
具体代价：当无 epoch 刷新回归被修复时，最自然的回归测试 createMockSession({ eventEpoch: undefined }) 会静默走到有 epoch 的正常路径并保持绿色，从而认可了它本应捕获的回归。
建议修复：区分'未提供'与'显式 undefined'，例如 eventEpoch: Object.hasOwn(opts, 'eventEpoch') ? opts.eventEpoch : 'epoch-1'。
— qwen3.8-max via Qwen Code /review (v0.21.9)
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
1e03e90 to
59fb669
Compare
|
Please do not rebase or force-push to an active PR as it invalidates existing review comments. Note for future reference, the bots always squash all changes into a single commit automatically as part of the integration. 中文请勿对活跃的 PR 执行 rebase 或 force-push，因为这会使已有的评审评论失效。另外，供日后参考：作为集成流程的一部分，机器人始终会自动将所有改动压缩（squash）为单个提交。 |
|
Re-run note: re-gated the same head ( Thanks for the PR — this is the next slice of the #8678 session-restore safety series (#8691 and #8833 already merged).
Flagging size/depth for maintainer awareness, and moving on to code review. 🔍 中文说明Re-run 说明：在维护者批准之后，对同一个头（ 感谢贡献——这是 #8678 会话恢复安全系列的下一个切片（#8691 与 #8833 已合并）。
规模/深度已提请维护者关注，继续进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
added
daemon
scope/session-management
Code review (re-run)The design doc and the implementation still line up on this head, and I can corroborate the commit-path description in @yiliang114's approval: What this re-run adds: I independently re-checked the six Critical findings from the earlier multi-agent review pass (raised on the pre-force-push head, in the standing
None of the six has regression-test coverage. Findings 2-6 were probe-reproduced by the review pass on the pre-force-push head; this re-run confirms the relevant code is unchanged on the current head, and finding 1 is additionally verified against current To be explicit about where the approval and this review diverge: the maintainer's verification of the commit CAS, exactly-once tail merge, fail-closed gates, and scheduling is accurate as far as it goes — these findings sit outside the commit path, in the daemon response shape, the shell latch, the settle cleanup, and the controlled bootstrap, and none of them is observable from the commit path itself. sequenceDiagram
participant P1 as WebUI action
participant P2 as Transition coordinator
participant P3 as Source runner
participant P4 as Daemon
participant P5 as Candidate session
P1->>P2: request same-session refresh
P2->>P3: wait until ready and idle
P2->>P3: arm bounded event capture
P2->>P4: load or resume with client id
P4-->>P2: candidate with epoch and watermark
P2->>P2: validate epoch, cursor, replay completeness
P3-->>P2: source processed cursor catches up
P2->>P5: stage replay plus bounded tail
P2->>P5: commit, stop source, detach best effort
Files changed (9)
Testing evidence (PR's own CI)This unattended run does not execute PR code. Check-runs fetched once on the head commit, not polled: The main Checks on
Sandboxed verification: a 中文说明代码审查（re-run）设计文档与实现在该头上仍然一致；@yiliang114 批准中对提交路径的描述我可以印证： 本次 re-run 的增量：独立复核了早前多代理评审（在强推前的头上提出、即仍挂着的
六项均无回归测试覆盖。发现 2-6 已由评审探针在强推前的头上复现；本次 re-run 确认相关代码在当前头未变；发现 1 另在当前 批准与本次评审的分歧点说明：维护者对提交 CAS、恰好一次 tail 合并、fail-closed 门控与调度的核实是准确的；上述发现位于提交路径之外——daemon 响应形状、shell 锁存、settle 清理、受控引导——从提交路径本身不可见。 测试证据（PR 自身 CI）本次无人值守运行不执行 PR 代码。检查只拉取一次、不轮询： 主 沙箱验证： — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 2/5 — significant concerns, leaning against as-is: the six Critical findings from the multi-agent review pass are all still present on this head (independently re-verified), including a daemon-side epoch gap that breaks the headline feature on its happy path in the default Web Shell configuration, and the repo's main CI has never run on this branch. Stepping back: the problem is real and observed, the direction is right, the design is good, and the parts of the previous defer that @yiliang114 was asked to settle have been settled seriously — the commit-path CAS, exactly-once tail merge, and scheduling verification in the approval are accurate, and I corroborated them against the code. That is not where this re-run parts company. It parts company on what the previous run also flagged: a static read cannot attest to every transition of this state machine — and when you probe it, the earlier review pass's findings reproduce against this head one by one. The decisive one is not a judgment call: Two more things before merge could be reconsidered:
@doudouOUC the minimum merge-blocking fix is finding 1 — one line in the daemon's bounded-refresh branch plus a regression test — but please address or rebut each of the six with probe evidence; the review thread has concrete suggested fixes for all of them. @yiliang114 requesting a re-review after the author responds — your commit-path verification stands, my objection is scoped to what it did not cover. 中文说明置信度：2/5 —— 重大顾虑，倾向不合并当前版本：多代理评审的六项 Critical 发现在该头上全部仍然存在（已独立复核），其中包括 daemon 侧 epoch 缺口——它在默认 Web Shell 配置下让核心特性的正常路径直接失败；且仓库主 CI 从未在本分支运行。 退一步看：问题真实且已观测，方向正确，设计良好；上次缓议请 @yiliang114 定夺的部分已得到认真定夺——批准中对提交 CAS、恰好一次 tail 合并与调度的核实是准确的，我也在代码上印证了。本次 re-run 的分歧不在这里。 分歧在于上次运行同样指出的那点：静态阅读无法为该状态机的每个转换背书——而一旦去探针验证，早前评审的发现在这颗头逐个复现。决定性的一项不是判断分歧：当前 重新考虑合并前的另外两点：
@doudouOUC 最小的合并阻断修复是发现 1——daemon 有界刷新分支的一行修复加一个回归测试——但请逐项处理或以探针证据反驳六项发现；评审线程中都有具体修复建议。@yiliang114 请在作者回应后复审——你对提交路径的核实成立，我的异议仅限于其未覆盖的部分。 — Qwen Code · qwen3.8-max Reviewed at |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Approved. Same-session refresh now goes through the restore coordinator with the same transactional guarantees as cross-session switching, and the commit math checks out.
What I verified
- Commit path is a synchronous double-checked CAS (
commitSameSession): intent/lifecycle/env-generation, exact source object + clientId, epoch, deadline, runner readiness, turn state, and processed cursor are re-checked before and after staging, with no await in between — no interleaving window. - No loss/duplication: capture is contiguous-id and bounded (maxQueued + 8 MiB); tail applied at commit is strictly
(watermark, processedEventId]; candidate cursor is advanced to the final processed cursor before the new SSE runner starts, so events beyond it are delivered exactly once.captureSourceEventruns pre-processing andmarkSourceEventProcessedinfinally, so captured-but-unprocessed events are correctly excluded from the tail and re-delivered to the new runner. - Fail-closed everywhere: epoch mismatch, incomplete/partial/degraded replay, capture gaps/id-less frames/overflow, in-place clientId self-heal, and missing cursor/epoch all reject the candidate, detach it best-effort once, and preserve the live source with one recoverable transition failure (never rewriting it as disconnected).
- Scheduling: the watchdog deadline starts with the raw RPC (queue wait excluded), identical signal-free requests coalesce, different same-session intents are latest-wins, cross-session targets supersede a prepared refresh, and a timed-out retry can adopt the still-in-flight raw result while keeping capture armed (
.finallyclearsrawTransitionRefon both success and error paths and re-pumps — no deadlock on abort-during-RPC). createSessionsource-bound exclusion now tracks the raw request rather than the outer action timeout, and a late successful create after timeout is detached once — matching the design doc.- API contract is additive only (optional
partial/replayError, client-derivedreplaySnapshotComplete,eventEpochgetter); no daemon changes required. - Test coverage is thorough: prompt/shell/observer gating, completeness matrix, tail-gap/overflow cases, rebind modern+legacy+failure, timeout-retry adoption, cross-session supersede, plus real-daemon JSDOM tests for atomic replay+tail merge, structured 504 preservation, and transcript continuity across rebind.
CI: green on head sha (42 checks, 0 failures; only the review-bot itself in progress and a cancelled label job).
Non-blocking nits (P2/P3)
clientIdRefrender-time reset usesinitialClientIdDependencyRef, which is frozen at first render. With an explicitclientIdprop on a modern daemon, a committed rebind to a new id gets reset back to the first-render prop on the next render; a later SSE reconnect then requests the stale id. DaemonregisterClientmints a fresh id for unknown requests (load never rejects), so this self-heals with mild attach churn rather than breakage — but worth a follow-up to reconcile the ref with the committed owner.- Refresh on a session with no cursor/epoch yet (e.g. a brand-new zero-event session) fails fail-closed with "cursor is unavailable; session was preserved" — correct direction, slight UX rough edge.
- If the processed cursor advances during staging, commit surfaces as "integrity validation" failure instead of re-waiting; recoverable by retry, acceptable.
|
@qwen-code /triage |
|
Sandboxed verification: ✅ passed — merge-ready (agent verdict) - workflow run Ran the PR in an isolated, token-free container: A/B against the base build, mock-free harness assertions, targeted gates. Advisory evidence for human reviewers — not a review, an approval, or a CI check. Scripted assertions: 12 passed · 0 failed · 12 total 中文 — 判定：✅ 通过 · 可合入（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：12 通过 · 0 失败 · 12 总计 Verification reportPR #8939 — fix(webui): Make same-session refresh transactionalVerdict: 中文摘要
Central claim and A/B proofCentral claim: on a daemon advertising A/B design: head arm = built head webui dist + head SDK dist; base arm = a control dist built from head webui with only the two PR-changed source files (
The two load-bearing cells flip broken→fixed; the third is a boundary the PR deliberately preserves, and it passes on both arms. Unit-level A/B / vacuity (same test files, sources reverted to base in the head tree): webui 27 failed / 305 passed (332); SDK 3 failed / 54 passed (57). The 27 webui failures are exactly the 26 PR-added tests plus the one PR-modified pre-existing test ( Gates at head: webui Reviewer Test Plan walkthrough: step 1 (held response + live work during hold + exactly-once commit) = integration cell 1, head ✓ / base ✗. Step 2 (504 / malformed / partial / epoch / gap / id-less / overflow) = integration cell 2 plus the unit matrix ( Wire oracle (independent harness, real daemon)Harness
FindingsS1 (suggestion, non-blocking) — post-commit retirement detach targets the committed attachment's own clientId; safety rests on the daemon's refcounted client ledger. Not covered
MethodologyEnvironment: CI verify container (node 22, merge-ref checkout at depth 2; Evidence images
Harness scripts and raw logs are in the workflow run artifacts (7-day retention). — Qwen Code · sandboxed verification |
|
Triage re-run completed without a new review.
The stage comments above were updated with the latest result. View workflow run. 上方各阶段评论已更新为最新结果。查看工作流运行。 |
ytahdn
left a comment
ytahdn
left a comment
There was a problem hiding this comment.
LGTM. No critical issues found. Key suggestions: (1) legacyClientIdDependency is misleadingly named — consider effectiveClientIdDependency and add a comment explaining why modern daemons use the initial prop value; (2) commitSameSession pre-staging and final-snapshot guards are ~30 lines each and nearly identical — extract a shared helper; (3) CrossSessionIntent now covers same-session refresh via sameLogical — rename to SessionTransitionIntent or add a comment; (4) structured matchers preferred over JSON.stringify(...).includes() in tests. Questions: confirm exposeCrossSessionFailure clears desiredTransitionRef.current; document best-effort detach failure semantics in commitSameSession.
already have 2 approves， 3ks.
|
Released in v0.21.11. |
4 participants
What this PR does
This PR makes same-logical-session
load, configured reload,resume, and explicit client-ID rebind transactional on daemons that advertise client attachment identity. It keeps the current attachment, transcript, event stream, prompt state, metadata, and controls active while a candidate restore runs; validates the candidate epoch, cursor, replay completeness, ownership, and bounded live tail; then commits the replacement atomically. Failed, timed-out, malformed, superseded, or stale candidates are retired without changing the visible source session.The existing restore coordinator now distinguishes exact replay shapes and serializes ordinary restore RPCs across cross-session and same-session intents. Full loads stage replay plus a contiguous source tail without duplicating replay arrays, while resume/client-ID rebind preserves the visible transcript and invalidates stale pagination ownership. The TypeScript SDK also exposes the existing restore epoch and partial-replay diagnostics needed for fail-closed validation.
Why it's needed
Before this change, refreshing or rebinding the current session used the destructive runner path: it could abort the source SSE, detach the source, clear prompt state, or replace the transcript before the target restore completed. A slow, failed, partial, or stale restore could therefore interrupt an otherwise healthy session and lose live events. Transactional same-session refresh preserves the working source until the replacement is proven complete and current.
Reviewer Test Plan
How to verify
Evidence (Before & After)
Before: five isolated baseline probes on
86a474ba62a50d25e65deac1a0d5111c4a5bb90dshowed same-session load/reload aborting the source event stream, cancelling an admitted prompt, and resume/client-ID rebind detaching or clearing the source before the candidate settled.After: the same five probes pass, 330 WebUI session tests and 57 SDK session-client tests pass, and the real-daemon JSDOM integration passes all three held-response, structured-504, and client-ID-rebind scenarios. N/A for screenshots because the change is ownership/error-path behavior rather than a visual redesign.
Tested on
Environment (optional)
macOS 26.4.1, Node.js 22.22.3, npm 10.9.8, no sandbox for the focused real-daemon integration.
Risk & Scope
client_identityretain the existing destructive behavior; unknown or malformed modern capability/ownership state fails closed and preserves the source.Linked Issues
Refs #8678
中文说明
本 PR 做了什么
本 PR 为明确支持客户端 attachment 身份的 daemon，将同逻辑会话的
load、configured reload、resume和显式 client ID rebind 改为事务化流程。候选 restore 运行期间，当前 attachment、transcript、事件流、prompt 状态、metadata 和控制能力保持可用；候选通过 epoch、cursor、replay 完整性、owner 和有界 live tail 校验后，才原子提交 replacement。失败、超时、malformed、被 supersede 或 stale 的候选只会被回收，不会改变当前可见 source session。现有 restore coordinator 现在按精确 replay shape 区分请求，并在跨会话和同会话 intent 之间串行化普通 restore RPC。完整 load 会在不复制 replay arrays 的前提下 staging replay 与连续 source tail；resume/client-ID rebind 保留可见 transcript，并使旧 pagination owner 失效。TypeScript SDK 同时暴露现有 restore epoch 与 partial-replay 诊断，供 fail-closed 完整性校验使用。
为什么需要
改动前，刷新或 rebind 当前 session 会使用 destructive runner 路径：target restore 完成前就可能 abort source SSE、detach source、清理 prompt 状态或替换 transcript。慢速、失败、partial 或 stale restore 因此会中断原本健康的 session，并可能丢失 live events。事务化同会话刷新会一直保留可用 source，直到 replacement 被证明完整且仍是当前目标。
Reviewer 测试计划
如何验证
证据（前后对比）
改动前：在
86a474ba62a50d25e65deac1a0d5111c4a5bb90d上的五个隔离 baseline probe 显示，same-session load/reload 会 abort source event stream、取消已 admitted prompt，而 resume/client-ID rebind 会在候选 settle 前 detach 或清理 source。改动后：相同五个 probe 全部通过，330 个 WebUI session tests 与 57 个 SDK session-client tests 通过，真实 daemon JSDOM integration 的 held-response、structured-504 和 client-ID-rebind 三个场景全部通过。由于本改动是 ownership/error-path 行为而非视觉重设计，截图为 N/A。
测试平台
环境（可选）
macOS 26.4.1、Node.js 22.22.3、npm 10.9.8；focused real-daemon integration 使用 no sandbox。
风险与范围
client_identity的 daemon 继续使用现有 destructive 行为；未知或 malformed 的现代 capability/owner 状态会 fail closed 并保留 source。关联 Issue
Refs #8678