feat(web-shell): Share session catalog scheduling - #8891
Conversation
Web Shell session catalog store E2E planBaseline
Scenarios
Result
|
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
0ee5a1c to
968006f
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）为单个提交。 |
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Re-run at the author's request, after Template: complete ✓ — all sections present, bilingual body, test plan. The risk section now also documents the deliberate last-successful-page error semantics that the first pass asked for. Problem: real and statically verifiable on Direction: in scope ✓ — Web Shell is a first-party surface and consolidating its data fetching is legitimate infrastructure work. No auth/sandbox/public-contract concerns. (Reminder: sibling PR #8892 applies the same catalog idea to the CLI — worth evaluating as a pair.) Size: 2,743 production-logic lines + 2,510 test lines + 41 design-doc lines across 32 files, all in Approach: the scope question from the first pass stands on record — an in-flight dedup keyed by query params would have captured most of the traffic win at a fraction of this surface area. A maintainer has since deep-reviewed the substance and accepted the full design, so this stays a discussion note, not a gate. Both CI blockers from the first pass are fixed on head, and the reviewer's P2 (dialog staleness under the 30s zero-subscriber retention) was addressed with a Risk: no high-risk-path matches from the revert-history analysis. The architectural freshness risk is now explicitly documented in the PR body. Moving on to code review. 🔍 中文说明应用户请求重跑，此前已推送 模板： 完整 ✓ —— 各部分齐全、中英双语、含测试计划；风险部分也补上了首轮要求的"保留最后成功页"错误语义说明。 问题： 真实存在且可在 方向： 在范围内 ✓ —— Web Shell 是一方界面，整合数据获取是合理的基础设施工作。无 auth/sandbox/公开契约问题。（提醒：姊妹 PR #8892 将同样的 catalog 思路用于 CLI，建议成对评估。） 规模： 2,743 行生产逻辑 + 2,510 行测试 + 41 行设计文档，共 32 个文件，全部位于 方案： 首轮的范围疑问保留在案——按查询参数做在途去重本可以用小得多的面积拿到大部分流量收益。维护者已深入审查实质并接受了完整设计，因此此项作为讨论备注保留，不作为门禁。首轮的两个 CI 阻塞项已在 head 上修复；reviewer 提出的 P2（30s 零订阅保留期内弹窗可能展示陈旧列表）已按建议以 风险： revert 历史分析未命中高风险路径。架构性新鲜度风险已在 PR 正文中明确记录。 进入代码审查。🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewMy independent baseline is unchanged from the first pass: an in-flight request map keyed by
The base machinery I verified on the first pass — route ownership preserved on both routeKinds, owner-scoped invalidation everywhere, revision-based stale-response rejection — is untouched by the delta and still stands. @yiliang114's substantive review independently confirmed the deeper invariants (query-key coverage of every wire-affecting option, numerically unchanged polling cadence, scheduler slot protection, bounded teardown) and approved this head. sequenceDiagram
participant P1 as UI consumer (sidebar, overview, dialogs)
participant P2 as catalog hooks (useSyncExternalStore)
participant P3 as SessionCatalogStore
participant P4 as scheduler queue (2 slots, 1 background)
participant P5 as DaemonClient
P1->>P2: subscribe or loadOnce(query)
P2->>P3: subscribe / refresh
P3->>P4: ensureScheduled(priority, background)
P4->>P5: fetchPage via legacy or qualified route
P5-->>P3: DaemonSessionListPage
P3->>P3: accept only if revision matches desired
P3-->>P2: snapshot update, resolve waiters
P1->>P3: mutation event via controller.invalidateWorkspace(cwd)
P3->>P4: bump revision, mark stale, reschedule owner workspace
Files changed (30 of 32 shown)
… and 2 more files (sidebar collapse-persist and workspace-removal test updates). TestingUnattended CI run — no PR code was built or executed here; the evidence below is the PR's own CI on head Both checks that failed the first pass are now green, and they failed for reasons traced to concrete fixes rather than a flake re-run: the dialog tests were migrated to the new hook boundary, and the vite proxy bypass stops the dev server from proxying catalog source modules at the daemon. The rest of the in-scope lanes are green with them — unit suite on ubuntu, E2E smoke, visuals capture (no screenshot changed vs base, expected for a scheduling-only change), and Desktop Shell on both OSes. One correction to my first-pass comment: the macOS/Windows unit legs and the CLI integration lane being skipped is by design, not a gap — those jobs are merge-queue-only since #8792 ( Not verified here: live request coalescing, slot reservation, and visibility-resume behavior under real load — unit tests pin the semantics and the smoke suite loads the real modules, but no measured traffic reduction exists. Sandboxed verification is already in flight for exactly this claim (triggered alongside this re-run); its report will post separately, and the coalescing/slot claims are what it should settle.
中文说明代码审查： 我的独立基线与首轮一致——按
首轮验证过的基础机制——两种 routeKind 路由归属保留、失效全部按 owner workspace 作用域、基于 revision 的过期响应丢弃——未被增量触及，仍然成立。@yiliang114 的实质审查独立确认了更深的不变量（查询键覆盖所有影响请求的选项、轮询节奏数值不变、调度器槽位保护、有界清理），并已在该 head 上批准。 测试： 无人值守 CI 运行——未构建或执行任何 PR 代码，证据为 head 更正首轮评论的一处表述：macOS/Windows 单测与 CLI 集成车道被跳过是设计使然而非缺口——自 #8792 起这些任务仅在 merge queue 运行（ci.yml 中 此处未验证：真实负载下的请求合并、槽位保留与可见性恢复行为——单测钉住了语义、smoke 套件加载了真实模块，但尚无流量下降的实测数据。沙箱验证已随本次重跑并行触发，报告会单独发布；合并/槽位声明正是它应当收口的内容。 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — both first-pass blockers fixed with traceable causes, the staleness P2 closed exactly as suggested with tests, CI green on every in-scope lane, and a maintainer deep-reviewed and approved this exact head. Remaining notes are non-blocking. Stepping back, here's where I land. I came in skeptical about scope, and I'm not dropping that note: an in-flight dedup keyed by query params remains the simpler design I would have reached for, and the PR still carries no measured before/after request counts — the traffic-reduction headline is substantiated by test semantics, not by numbers. But a gate also has to recognize when its questions have been answered. Every red check from the first pass is green for a traced reason (dialog tests migrated to the hook boundary; vite bypass for catalog source modules), not a flake re-run. The one semantic hole found in review — dialogs showing a minutes-old retained list under the 30s zero-subscriber retention — is closed by a minimal, tested Approving, pinned to the reviewed commit. Non-blocking follow-ups:
中文说明置信度：4/5 —— 首轮两个阻塞项均以可追溯的原因修复，新鲜度 P2 按建议以带测试的最小方案关闭，所有在范围 CI 车道转绿，维护者已深入审查并批准当前 head。其余备注均不阻塞。 退一步看：我对规模的怀疑仍在案——按查询参数做在途去重依然是我会选择的更简设计，PR 也仍缺少请求数 before/after 实测，流量下降的标题目前由测试语义而非数字支撑。但门禁同样要承认问题已被回答：首轮每一个红色检查都因具体修复而转绿（弹窗测试迁移到 hook 边界、catalog 源模块的 vite bypass），不是重跑碰运气；review 中找到的唯一语义漏洞——30s 零订阅保留期内弹窗可能展示数分钟前的列表——以最小且带测试的 予以批准，锚定在被审查的提交。非阻塞后续：
— Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Requesting changes — the PR's own CI is red on the package it touches, and the failures are PR-caused (same checks green on #8892/#8893/#8882):
Test (ubuntu-latest): web-shell suite fails 12 tests — the dialog tests (DeleteSessionDialog,ReleaseSessionDialog,ResumeDialog) still mocklistWorkspaceSessionswhile the rewireduseScopedSessionsnow callsworkspaceByCwd(cwd).listWorkspaceSessionsPage, so the dialogs render zero sessions. Those three test files need updating as part of this migration.web-shell E2E Smoke (ubuntu-latest): 29 of 32 tests fail (repeatedECONNREFUSED 127.0.0.1:4170while the browser requests the newsession-catalog/*modules) — needs investigation before this can move.
The architecture itself is sound (route ownership and owner-scoped invalidation both verified); full notes in my review comments above. Happy to re-review once CI is green. 🙏
— Qwen Code · qwen3.8-max
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Addressed both CI blockers from the review in commit
I also updated the PR body with the current verification evidence and the deliberate last-successful-page error semantics. The new CI run is in progress. 已在提交 |
🖼️ 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 10 render-shaping files:
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 |
|
[codex] Thanks — won’t take this one. This PR intentionally changes session-list scheduling and catalog state without changing rendered UI; the unchanged screenshots are expected, while the affected non-visual paths are covered by catalog/component tests and the Web Shell smoke test. |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
LGTM on the substance. The catalog is verified sound: the query key covers every wire-affecting option (differing pageSize/cursor correctly do not share), route ownership is exactly preserved (legacy GET /workspace//sessions vs the workspace handle's paged list), mutation/lifecycle invalidations are owner-workspace-scoped with double-session guards, polling cadence per surface is numerically unchanged, the 2-total/1-background scheduler protects explicit work, hidden-pause drops queued background jobs without amplification on rapid tab switches, and teardown on client disconnect is bounded (WeakMap keying, timers self-extinguish). Both prior blockers are fixed on head (dialog tests mock useScopedSessions at the hook boundary — web-shell suite 3125/3125; vite proxy bypass extended to /session-catalog/* — E2E smoke green).
One P2 to fix or explicitly accept: dialogs (Delete/Resume/ReleaseSessionDialog) subscribe with autoLoad but no pageSize/pollIntervalMs, so their query key matches no polling surface and nothing refreshes a retained entry except local-mutation invalidation; combined with the 30s zero-subscriber retention, repeated open/close cycles under 30s apart never refetch — the dialog can show a minutes-old list (missing sessions created via CLI/another tab, or offering sessions deleted elsewhere), a freshness regression vs the old fetch-on-open. Suggested fix: treat a retained page as stale for a NEW autoLoad subscriber when updatedAt exceeds a small max-age, or have the dialogs call reload() on open.
CI note (process, not code): the ubuntu leg is red on 8 CLI-package tests this PR does not touch (update/serve/workspace-registration-store) with flake signatures (15s timeouts, ENOTEMPTY rmdir races, severe runner overload — a clipboard suite took ~170s, collect 9507s) and the identical server-default-bridge-wiring file failed with identical signatures in an unrelated sibling PR in the same window; the PR's own surface is green. Needs a green re-run, not a code change. P3s: cross-workspace initial loads are now serialized by design (1 background slot) — acknowledge in the doc; dispose() is tests-only, acceptable. Ship it after the staleness decision and the re-run.
Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
|
Resolved the merge conflict and pushed the updated branch in
Verification:
中文说明：冲突已通过合并当前 |
yiliang114
left a comment
yiliang114
left a comment
There was a problem hiding this comment.
Re-approving at head 657b8ab: the only PR-owned delta since my approval is the P2 fix, implemented exactly as suggested — a maxAgeMs subscription option plumbed end-to-end with all three dialogs passing { autoLoad: true, maxAgeMs: 1000 }, triggering a non-blocking background refresh of retained pages older than 1s on subscribe, with tests covering no-refetch within maxAge, refetch after 1s, and snapshot update. maxAgeMs is correctly kept out of the query key (staleness policy, not wire-affecting), the trigger reuses requestBackground with its existing dedup/priority guards so no re-subscribe storm, and the rest of the new commit is a clean main merge (first-parent diff purely additive, no PR-owned lines lost). CI green on every in-scope lane; the previously flaky CLI integration lane is now merge-queue-only per main's #8792 change, so it no longer gates this PR. Nothing blocks merge.
|
@qwen-code /triage |
|
Sandboxed verification: ❌ not passed — findings reported (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: 49 passed · 1 failed · 50 total 中文 — 判定：❌ 不通过 · 报告了发现（agent 判定）沙箱验证在隔离、无凭证的容器中执行了该 PR 的代码（与 base 构建 A/B 对照、无 mock harness 断言、定向门禁）。仅作为评审证据，不构成评审、批准或 CI 检查。 脚本断言：49 通过 · 1 失败 · 50 总计 Verification reportPR 8891 deep verification — feat(web-shell): Share session catalog schedulingVerdict: 中文摘要
Scope
Central claim — A/B load-bearing proofReal
Ratio B/A = 3.00×, exactly the consumer count. Route ownership held on both arms: legacy queries hit Secondary claims — wire-oracle cellsAll cells drive the real store + real SDK against the recording fake daemon.
Vacuity / mutation matrix on the new testsBaseline control green (34/34, both new test files). Each mutant is a single-point change to
Targeted gates: full Web Shell unit suite 3164/3164 passed, exit 0 at the merge head (witness FindingsF1 — Suggestion: cross-workspace
|
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship. ✅ Both first-pass CI blockers fixed with traceable causes, the staleness P2 closed as suggested, and CI green on every in-scope lane.
|
Released in v0.21.10. |
3 participants
What this PR does
This PR adds a page-scoped Web Shell session catalog that is isolated per daemon client. Identical session-list queries now share cached pages and in-flight requests, while a client-wide scheduler limits total list concurrency to two and background concurrency to one so explicit user work retains a request slot.
It preserves the existing legacy and workspace-qualified route ownership, complete pagination metadata, polling cadence, and public interfaces. Session-list consumers across the sidebar, overview, split picker, dialogs, scoped hooks, and command-style lookups now use the shared catalog, and lifecycle or mutation events invalidate or conservatively patch the owning workspace before authoritative refreshes converge the catalog.
The catalog pauses pending automatic work while the page is hidden, resumes only overdue active queries when visibility returns, retains successful pages across transient errors, applies a 30-second automatic retry floor, and keeps zero-subscriber entries for 30 seconds to absorb StrictMode remounts and short UI transitions.
Why it's needed
Several Web Shell surfaces can request the same session list independently, multiplying polling traffic and allowing stale responses from one consumer to race newer mutation or lifecycle state from another. The shared scheduler deduplicates equivalent reads, bounds multi-workspace pressure, reserves capacity for user actions, and gives every consumer one consistent invalidation model without changing daemon or SDK APIs.
Reviewer Test Plan
How to verify
Evidence (Before & After)
N/A — this changes internal request scheduling and catalog consistency without changing the rendered UI.
Tested on
Environment (optional)
macOS local workspace with Node.js 22.22.3 and the repository npm workspace dependencies.
Risk & Scope
Linked Issues
N/A
中文说明
本 PR 做了什么
本 PR 在 Web Shell 中新增按 daemon client 隔离的页面级会话目录。相同的 Session List 查询现在共享缓存页面和在途请求，同时客户端级调度器将列表总并发限制为 2、后台并发限制为 1，从而为显式用户操作保留一个请求槽位。
它保留现有 legacy 与 workspace-qualified 路由归属、完整分页元数据、轮询节奏和公开接口。Sidebar、Session Overview、Split picker、dialogs、scoped hooks 与命令式查询等 Session List 消费者现在统一经过共享目录；会话生命周期或 mutation 事件会按 owner workspace 失效或保守 patch，随后通过权威刷新使目录收敛。
页面隐藏时，目录暂停尚未开始的自动任务；恢复可见时只补刷已到期的活跃查询。瞬时错误会保留最后一次成功页面，自动重试最短间隔为 30 秒；零订阅缓存保留 30 秒，以吸收 StrictMode 重挂载和短暂 UI 切换。
为什么需要
多个 Web Shell 界面可能分别请求同一个 Session List，导致轮询流量随已挂载消费者数量增长，并使某个消费者的旧响应与另一个消费者更新的 mutation 或生命周期状态竞争。共享调度器可以去重等价读取、限制多工作区压力、为用户操作保留容量，并在不修改 daemon 或 SDK API 的前提下为所有消费者提供一致的失效模型。
Reviewer 测试计划
如何验证
证据（之前与之后）
N/A——该变更调整内部请求调度和目录一致性，不改变渲染 UI。
测试平台
环境（可选）
macOS 本地工作区，Node.js 22.22.3，使用仓库 npm workspace 依赖。
风险与范围
关联 Issue
N/A