feat(serve): establish workspace runtime ownership - #10593
Merged
Conversation
Collaborator
Author
|
Validation report (macOS):
Maintainer awareness: this feature touches core and cross-package runtime infrastructure and adds approximately 1,428 production lines (excluding tests, docs, and generated/schema files). It is a feature foundation rather than a refactor, so it is not subject to the large-refactor hard block, but it should receive maintainer architecture review. Four local review passes completed; the final two were performed from the complete origin/main diff only, without using prior PR review information, and found no unresolved correctness, security, or data-loss issue. |
Collaborator
Collaborator
|
Thanks for the PR! Template looks good ✓ Problem: this is architectural groundwork, not a bug fix — no reproduction, and none is expected. The stated gap (a session-centric lifecycle can treat an ACP process as disposable while workspace-level work is still running) is a real defect class for the daemon's multi-workspace story, and it is anchored in the repo's own design effort ( Direction: this is a re-submission of #8213, which the author closed and reopened here. That predecessor already went through a direction escalation, after which a maintainer engaged substantively — a full-diff review with concrete findings, takeover management, and a sandboxed Size: core infrastructure (cross-package: Approach: the stated scoping is reasonable — foundation layer only, with capability convergence and catalog projection explicitly deferred. The additive surface (coordinator, five-state snapshot, epochs, ensure/status routes, SDK helpers, removal/shutdown admission) matches how this was reviewed on #8213. The part that needs scrutiny is unchanged in scope from the predecessor: the channel-retention policy that makes physical ACP work the authority, where several previously-recoverable failures now terminate a shared child. Risk: no high-risk path signals from the revert-history check (no matches). The real risk is the predecessor's unresolved review findings — the Stage 2 pass will check each one against this diff. Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ 问题： 这是架构层面的基础工作，不是 bug 修复——没有也不需要复现。所述的缺口（以 Session 为中心的生命周期可能在工作区级任务仍在运行时把 ACP 进程视为可回收）是 daemon 多工作区设计中的真实缺陷类别，且锚定在仓库自身的设计工作中（本 PR 扩展了 方向： 本 PR 是 #8213 的重新提交（作者自行关闭旧 PR 后重开）。前身在经历方向性升级后已得到维护者实质参与——完整 diff 审查并提出具体发现、takeover 托管、以及一次通过的沙箱 规模： 核心基础设施（跨包： 方案： 声明的范围合理——只做 foundation 层，capability 收敛与 catalog 投影明确推迟。新增表面（coordinator、五态快照、epoch、ensure/status 路由、SDK 辅助方法、移除/关闭准入）与 #8213 审查时的形态一致。需要审视的部分与前身相同：以物理 ACP 工作为权威的 channel 保留策略——多个原本可恢复的失败现在会终止共享子进程。 风险： 回滚历史信号未命中高风险路径。真正的风险是前身未解决的审查发现——Stage 2 将逐条对照本 diff 核查。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
Code reviewI wrote down my own design before reading the diff: for this problem I would have put a lifecycle snapshot and monotonic epoch in the bridge, a per-workspace coordinator in serve for ensure dedup and keep-alive, a thin REST + SDK surface with structured errors, and admission of workspace removal / daemon shutdown on runtime work. The PR lands on essentially that shape — the coordinator as the single lazily-cached accessor, the five-state snapshot, reservations as the retention authority — and I didn't find a materially simpler path. The reservation counter looks heavyweight at first glance but it is what makes dropping the old per-call preheat dedup (in No correctness blockers found. Two things a maintainer should still weigh, both carried over from the predecessor's review:
What genuinely improved over the predecessor: the CI-blocking Critical is fixed (the integration capability baseline now includes Non-blocking suggestions: (1) wrap sequenceDiagram
participant P1 as Client (SDK or REST)
participant P2 as runtime route
participant P3 as WorkspaceRuntimeCoordinator
participant P4 as Bridge
participant P5 as ACP child
P1->>P2: POST workspace runtime ensure
P2->>P2: resolve runtime, trust gate, drain gate
P2->>P3: ensure (60s observer deadline)
P3->>P4: preheat with 10 min keep-alive lease
P4->>P5: deduplicated spawn, bounded factory plus initialize
P4->>P4: allocate monotonic epoch, publish channel
P3->>P4: lifecycle snapshot
P3-->>P2: status payload (state, epoch, live)
P2-->>P1: 200 status, or 503 retryable while starting
Files changed (30 of 45 shown)
TestingThe author's validation comment reports macOS build and package-suite numbers — that is the author's claim, not evidence from this run. The evidence here is the PR's own CI on the reviewed commit, quoted below; per the gate's rules nothing PR-derived was built or executed in this environment. Two checks that matter for this change — the ubuntu unit suite and Serve A/B — are still running at review time; the table region below is updated by the finalize job once CI settles. The macOS/windows test lanes and the CLI integration lane are skipped for this event. Nothing user-visible in the TUI changes here (daemon REST/SDK/docs only), so live TUI capture is N/A. Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 No failures at review time. The behavioral center of gravity — concurrent ensure dedup, timeout retirement, epoch monotonicity across bridge replacement — is pinned by unit tests, but a green suite does not by itself prove the wire surface behaves that way against the base build. Sandboxed verification would settle this: 中文说明代码审查在读 diff 之前我先写下了自己的设计：对这个问题我会把生命周期快照和单调 epoch 放在 bridge，把按工作区的 coordinator 放在 serve 负责 ensure 去重和保活，提供薄 REST + SDK 表面和结构化错误，并让工作区移除/daemon 关闭的准入基于运行时工作。PR 落点与此基本一致——coordinator 作为单一懒缓存访问器、五态快照、以 reservation 作为保留权威——我没有找到更简单的路径。reservation 计数器初看偏重，但正是它使得移除旧的按调用 preheat 去重（workspace-service 中）变得安全，并保护共享一次物理启动的并发观察者。 未发现正确性阻断项。 两点仍需维护者权衡，均承自前身 PR 的审查：
相对前身的真实改进：阻断 CI 的 Critical 已修复（集成 capability 基线现在包含 非阻断建议：(1) GET 处理器包裹 测试作者的验证评论报告了 macOS 的构建和包级测试数字——那是作者的声明，不是本次运行的证据。这里的证据是受审 commit 上 PR 自身的 CI（如下表引用）；按门禁规则，本环境未构建或执行任何 PR 派生代码。对本改动重要的两个检查——ubuntu 单测套件和 Serve A/B——在审查时仍在运行；CI 落定后 finalize 任务会更新下方表格区域。macOS/windows 测试通道和 CLI 集成通道对本事件为跳过。此处无 TUI 用户可见变化（仅 daemon REST/SDK/文档），因此实时 TUI 抓取为 N/A。 审查时无失败。行为核心——并发 ensure 去重、超时退役、跨 bridge 替换的 epoch 单调性——由单测钉住，但绿色套件本身不能证明 wire 表面相对 base 构建确实如此表现。沙箱验证可以定论： — Qwen Code · qwen3.8-max Reviewed at |
Collaborator
|
Confidence: 3/5 — clean static review with unusually strong test coverage, but the Stage 0 large-core escalation caps auto-approval, and the retirement-policy decisions carried over from #8213 need a maintainer to confirm them. Stepping back: this is a well-executed resubmission. The approach matches what I'd have drawn for this problem — snapshot and epoch in the bridge, one lazily-cached coordinator per workspace, thin REST/SDK surface, admission wired into removal and shutdown — and I didn't find a simpler path it missed. The test delta is genuinely strong for a change this size, the docs move with the code, and the predecessor's CI-blocking Critical is fixed. Nothing in the diff reads like drive-by churn. Why it stops here instead of getting an approval:
For whoever picks this up: the predecessor thread (#8213) holds the maintainer's original findings and engagement; this run's Stage 2 comment maps each of those findings to where it landed in this diff. A ⏸️ Deferring to maintainer — large core-infrastructure feature with design decisions that #8213 explicitly left to a human call. Needs a human decision on this one. 中文说明置信度：3/5 —— 静态审查干净、测试覆盖异常充分，但 Stage 0 的大规模核心升级封顶了自动批准，且承自 #8213 的退役策略决定需要维护者确认。 整体看：这是一次执行良好的重新提交。方案与我对该问题的设计一致——快照和 epoch 在 bridge、每个工作区一个懒缓存 coordinator、薄 REST/SDK 表面、移除与关闭准入接线——我没有找到它遗漏的更简路径。对这个体量而言测试增量确实扎实，文档与代码同步，前身阻断 CI 的 Critical 已修复，diff 中没有夹带无关改动。 停在这里而不批准的原因：
给接手者的提示：前身 #8213 线程里有维护者最初的发现与参与；本次 Stage 2 评论把那些发现逐条映射到了本 diff 的落点。如果单测套件不足以定论， ⏸️ 转交维护者——大规模核心基础设施功能，且 #8213 明确留给人类决定的设计决定仍未确认。这一项需要人类拍板。 — Qwen Code · qwen3.8-max Reviewed at |
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. |
chiga0
reviewed
Aug 31, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
CI 状态 — 🔴 Blocker
ubuntu-latest 测试 (Node 22.x) 失败（suite 90369350463，run 99365058323，~55 min，exit 1）。Windows / macOS / Integration Tests CLI 三个 job 均被跳过。PR 当前 mergeable_state: unstable，CI 通过之前不应合并。CI annotation 只显示 "Process completed with exit code 1"，需查看完整 job log 定位具体失败测试。
Findings
详见内联注释（F2、F3）。
未审查维度
packages/acp-bridge/src/bridge.ts(+824/-533)：~555 KB，API 截断，getWorkspaceRuntimeLifecycleSnapshot实现、preheatkeepAlive 追踪、active-work drain 逻辑未审查packages/acp-bridge/src/bridge.test.ts(+1132/-53)：同上，请求超时packages/cli/src/serve/run-qwen-serve.ts(+65/-1)：仅摘要可见，未确认coordinator.beginDrain()/dispose()在 workspace 替换和 daemon 关闭时是否被调用server/error-response.ts（sendBridgeError）：未获取，影响 F2 严重程度判断- Windows/Linux 平台行为：ubuntu 红灯，Windows 全 skipped
Reviewed with AI assistance.
Collaborator
🩺 serve daemon A/BBuilt the PR base vs this PR head
|
| field | PR base (before) | this PR (after) |
|---|---|---|
features[] |
— | "workspace_runtime" |
— Qwen Code · serve A/B
ytahdn
commented
Aug 31, 2026
ytahdn
left a comment
ytahdn
left a comment
Collaborator
Author
There was a problem hiding this comment.
Static review — feat(serve): establish workspace runtime ownership
Reviewed statically against head tree 78e285f (full diff plus surrounding code in packages/cli/src/serve, packages/acp-bridge, sdk-typescript, and the architecture design doc). No tests executed; all claims below were verified by reading the head tree, not taken from the PR description.
Key semantics independently verified:
- Generation/epoch monotonicity: epochs allocate through a regression-checked source; channelInfo is assigned last in ensureChannel so the fast path never observes a handshaking channel; getWorkspaceRuntimeLifecycleSnapshot throws on epoch-source regression.
- Concurrent ensure/preheat coalesce onto a single physical spawn (inFlightChannelSpawn); the keep-alive window is renewable via a Math.max high-water over the configured idle timeout, keepAliveUntil, and per-request pending deadlines.
- Physical ACP work — not session count — is the authority for retention and for removal/shutdown admission (snapshot activeWork; workspaceRuntime activity feeding workspaceActivity isBusy).
- Trust reconciliation: replacement disposes the previous runtime before the successor activates; generationGuard closes on failed containment; draining blocks replacement and cancelDrain respects a closed guard; post-commit removal never rolls back; fallback activation keeps the entry active with applyError surfaced.
- Error classification flips exactly at guard close: workspace_draining (Retry-After 5) while the guard is open, workspace_runtime_unavailable (Retry-After 1) after close; still-starting vs initialization-failed are distinct 503s with causes reported; no stack traces leak into HTTP bodies.
- MCP authentication safety deadline: timer firing re-checks isDying, live-channel identity, and timer identity; auth-clear in requestWorkspaceStatus only fires when the server appears in the child report with a non-pending state; the owning channel is killed only when authentication is still pending or unobservable.
- Shutdown/killAllSync: shuttingDown is set before snapshotting entries and channels; channels intentionally stay in aliveChannels for a double-terminating-signal killAllSync; mediator pending state is drained before byId.clear(); terminal lifecycle events publish before each bus closes.
- SDK client budgets (62s = 60s server default + 2s headroom) match the coordinator default; the ensure route rejects non-empty bodies so clients cannot tune server timeouts; capability advertisement is conservative (bootstrap predicate vs mounted predicate asymmetry is intentional).
Verdict: no Critical or Important findings.
Minor (non-blocking):
- workspace-service/index.ts preheatAcpChild asserts the active generation only at entry; unlike every sibling mutation in this facade it does not re-assert after the awaited preheat. Today the coordinator re-asserts on its own path so the observable impact is nil, but a post-await re-assert would restore uniformity and keep the invariant local.
- Informational only: the GET runtime-status route does not wrap the coordinator read in try/catch (unreachable under the current coordinator implementation), and the bootstrap vs mounted capability predicates differ by design (conservative advertisement) — both are consistent with the design doc.
Cross-check of the earlier review's findings (F2/F3), verified independently in the head tree:
- F2 (HTTP mapping of WorkspaceRuntimeStillStartingError): the failure scenario does not hold on head — the HTTP error layer maps this class explicitly to 503 with code runtime_still_starting and Retry-After 5 (server/error-response.ts), distinct from the initialization-failed mapping. The residual point is valid only as a test-coverage suggestion: there is no dedicated route-layer test pinning this specific error class.
- F3 (preheat failure racing drain): valid as a minor observability note — the re-assertion inside the catch path can raise the draining classification before the original preheat cause is reported, so the daemon log loses the underlying failure reason while the client still receives a 503 either way. Non-blocking.
CI at posting: Test (ubuntu) passes on the re-run. Note for context: the first Test (ubuntu) attempt failed early this morning inside a window in which main itself and many unrelated branches were also failing (environmental, since recovered — main is green again); the re-run on this head is green. All substantive lanes are green, including Serve A/B and the web-shell E2E smoke (this PR makes no web-shell changes); only the automated review-pr lane was still in progress at posting time, and it does not gate merging.
中文摘要
针对 head tree 78e285f 完成纯静态审查（完整 diff + serve/acp-bridge/SDK 周边代码 + 架构设计文档），未运行任何测试，下述结论均通过在 head tree 中读码独立核实。
已核实的关键语义：
- 代际/epoch 单调：epoch 分配带回退校验；ensureChannel 最后才赋 channelInfo，快路径不会看到握手中的 channel；生命周期快照在 epoch 源回退时直接抛错。
- 并发 ensure/preheat 合并到单一物理 spawn（inFlightChannelSpawn）；keep-alive 窗口可续租，取配置空闲超时、keepAliveUntil 与逐请求 deadline 的高水位。
- 保留与删除准入的权威是物理 ACP 工作量而非会话数（快照 activeWork；workspaceRuntime 活动计入 workspaceActivity 的 isBusy）。
- Trust 协调：替换先处置旧 runtime 再激活继任者；containment 失败即关闭 generationGuard；draining 阻断替换且 cancelDrain 尊重已关闭的 guard；提交后的删除绝不回滚；兜底激活保留 entry 并上抛 applyError。
- 错误分类恰好在 guard 关闭点切换：guard 开启时 workspace_draining（Retry-After 5），关闭后 workspace_runtime_unavailable（Retry-After 1）；still-starting 与初始化失败是不同的 503 且带 cause；HTTP 响应体不泄漏堆栈。
- MCP 认证安全期限：定时器触发时复检 isDying、live channel 同一性与定时器同一性；auth 清除仅在子进程报告中该服务器为非 pending 状态时生效；仅在认证仍 pending 或不可观测时才杀所属 channel。
- shutdown/killAllSync：先置 shuttingDown 再快照 entries 与 channels；channels 故意留在 aliveChannels 以支持重复终止信号场景；byId.clear() 前先排空 mediator pending 状态；总线关闭前先发布终态生命周期事件。
- SDK 客户端预算（62 秒 = 服务端默认 60 秒 + 2 秒余量）与 coordinator 默认值一致；ensure 路由拒绝非空请求体，客户端无法调节服务端超时；能力播报保守（bootstrap 谓词与挂载后谓词有意不对称）。
结论：无 Critical、无 Important。
Minor（不阻塞）：
- workspace-service/index.ts 的 preheatAcpChild 只在入口断言活跃代际，与该 facade 其他所有变更操作不同，await 之后未复断言。当前 coordinator 路径自带 post-await 复断言，实际影响为零；建议补上以恢复一致性、让不变量就地成立。
- 仅提示：GET runtime-status 路由未对 coordinator 读取加 try/catch（当前实现下不可达）；bootstrap 与挂载后的能力谓词不一致属有意设计（保守播报）——两者均与设计文档一致。
对早前另一份 review 的 F2/F3 发现，已在 head tree 独立复核：
- F2（WorkspaceRuntimeStillStartingError 的 HTTP 映射）：所述故障场景在 head 上不成立——HTTP 错误层已把该类显式映射为 503、code 为 runtime_still_starting、Retry-After 5（server/error-response.ts），与初始化失败映射相互独立。剩余的合理部分仅是测试覆盖建议：路由层尚无针对该错误类的专门测试。
- F3（preheat 失败与 drain 并发）：作为可观测性小问题成立——catch 路径内的复断言可能先抛出 draining 分类，导致原始 preheat 失败原因不进日志；客户端两种情况下都收到 503，不阻塞。
发布时 CI：Test (ubuntu) 重跑已通过。背景说明：今晨第一次 Test (ubuntu) 失败发生在 main 自身与大量无关分支同样失败的窗口内（环境问题，现已恢复，main 重新转绿）；本 head 的重跑为绿。实质车道全部为绿（含 Serve A/B 与 web-shell E2E smoke，本 PR 未改 web-shell）；发布时仅剩自动化 review-pr 车道在跑，该车道不作为合并门槛。
Collaborator
Independent Critical-gate review + live daemon e2e at head
|
| # | Probe | Result |
|---|---|---|
| A1 | GET /capabilities |
features[] contains workspace_runtime ✓ |
| A2 | GET /workspace/runtime/status right after ready |
{"state":"starting","runtimeLive":false,"runtimeEpoch":0} — the startup transition is observable ✓ |
| A3 | POST .../runtime/ensure with a JSON body |
400 workspace_runtime_ensure_takes_no_parameters ✓ |
| A4 | POST .../runtime/ensure concurrent with startup preheat |
200 {"state":"idle","runtimeLive":true,"runtimeEpoch":1} — coalesced onto the single physical spawn ✓ |
| A5 | ensure again while warm | 200, epoch still 1 — no respawn ✓ |
| A6 | GET /workspaces/<cwd>/runtime/status (qualified) |
200, identical payload ✓ |
| A7 | qualified status/ensure on an unregistered workspace | 400 workspace_mismatch (workspaceCount: 1) on both — no fallback to primary ✓ |
| A8 | relative-path selector | 400 “must decode to a workspace id or absolute path” ✓ |
| B1/B2 | folderTrust enabled + workspace DO_NOT_TRUST: ensure and status |
403 untrusted_workspace on both; startup preheat not scheduled (log confirms trust gate) ✓ |
No wedges, no unstructured errors, retry semantics as designed. Build note: npm ci && npm run bundle on the head is green except the known sdk-typescript browser-bundle size guard, which main's own #10571 already trips; the guard was relaxed in the scratch checkout only for this run.
Not approving — triage deferred this to a maintainer (large core-infrastructure change plus the #8213 retirement-policy decision), and that human call is still open.
中文摘要
独立 Critical 门禁审查 + 在 head 78e285f 上对真实 daemon 做了 e2e，未发现阻塞项。
- 路由归属：primary 与 workspace-qualified 两组 runtime 路由各自只解析自己的运行时，无活跃运行时返回结构化
503，未注册/相对路径选择器一律400 workspace_mismatch闭合，绝不回退到其他工作区。 - 失败语义：未信任
403、不支持501、ensure 带参400、启动中503 runtime_still_starting（Retry-After 5）、初始化失败503 runtime_initialization_failed（原因只进日志不进响应体）、drain/guard 关闭走既有分类。 - epoch 单调、发布顺序、有界 teardown、启动中止信号、preheat 在 shutdown 时抛错的各个调用点均已逐一核对。
- F2 复核：作者的反驳在 head 上成立——错误层已显式映射并有测试钉住，剩余只是路由层测试覆盖建议；F3 维持轻微可观测性注记。
- e2e：tmux 中起真实 daemon（隔离 HOME + 临时工作区），上表 9 组探针全部符合设计（含 starting 过渡态、并发 ensure 合并、epoch 稳定、未注册不回退、未信任 403 且不预热）。
- 构建：仅 scratch 副本放宽了被 main 自身 fix(sdk): Surface daemon JSON-RPC error details #10571 顶破的 SDK browser-bundle 体积守卫。
不做 Approve——triage 已按大规模核心基础设施 + #8213 退役策略决定转交维护者，仍需人工拍板。
qwen-code-ci-bot
previously requested changes
Aug 31, 2026
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
Collaborator
There was a problem hiding this comment.
Partially reviewed — gaps disclosed.
Not explored to full depth (tool budget reached): chunk 4: did not run the untrusted-preheat scenario end-to-end (static trace only), and did not exhaustively confirm every test-existence claim in design-doc section 16.….
Not reviewed: reverse audit — stopped before round 3 by the review time budget.
中文说明
仅完成部分审查，审查缺口已披露。
未探索到全部深度（达到工具调用预算）：chunk 4：did not run the untrusted-preheat scenario end-to-end (static trace only), and did not exhaustively confirm every test-existence claim in design-doc section 16.…。
未审查：反向审计——评审时间预算不足，未能开始第 3 轮。
— qwen3.8-max via Qwen Code /review (v0.22.3)
chiga0
reviewed
Aug 31, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Round 2 re-review on 86af37b4e940. Both findings from my Round 1 review (5063308039) are now addressed.
✅ F2 — resolved
workspace-runtime.test.ts now adds 'maps observer timeouts to a retryable 503 response', confirming that WorkspaceRuntimeStillStartingError → HTTP 503 + Retry-After: 5 path is tested at the route layer.
✅ F3 — resolved
assertAcceptingWork(cause?) forwards the original preheat error into WorkspaceDrainingError; error-response.ts calls reportBridgeError(err.cause, ...) before the 503 response, so the root cause now surfaces in daemon logs on the concurrent-drain+preheat-failure path.
❓ NEW-Q1 — configuredChannelIdleTimeoutMs() vs resolvedChannelIdleTimeoutMs() in the preserveForFirstStatusRead path
In bridge.ts preheat(), the guard that sets preserveForFirstStatusRead = true reads (paraphrased):
if (no keepAliveMs AND configuredChannelIdleTimeoutMs() === 0 AND no session work)
preserveForFirstStatusRead = true
configuredChannelIdleTimeoutMs() returns only the user-configured value; it does not account for an active keepAliveUntil window. resolvedChannelIdleTimeoutMs() returns Math.max(configured, keepAliveUntil − now, pendingKeepAliveMs) and is the authoritative value used at idle-timer arm time.
When startIdleTimer() runs after the status read, it fires with resolvedChannelIdleTimeoutMs(), not configuredChannelIdleTimeoutMs(). If a concurrent ensure() call has set a keepAlive window while preheat() was evaluating the guard, configuredChannelIdleTimeoutMs() could still return 0 (no explicit config) while resolvedChannelIdleTimeoutMs() returns the remaining keepAlive duration — meaning preserveForFirstStatusRead is correctly set to true but the idle timer fires with a positive delay rather than immediately, which is actually fine.
The more interesting inversion: a status-read path that arrives without a keepAlive window active gets preserveForFirstStatusRead = true (guard fires), but startIdleTimer(0) still runs in withWorkspaceControl's finally block immediately after. If resolvedChannelIdleTimeoutMs() === 0 at that point, the reap fires regardless of the flag. Whether preserveForFirstStatusRead actually suppresses startIdleTimer in the zero-timeout case, or whether it only prevents the flag from being a no-op on the next iteration, is the open question. Could you confirm the interaction between preserveForFirstStatusRead and the startIdleTimer call site, specifically in the zero-configured-timeout case?
(This overlaps with the CI bot's R1-1. The author replied to R1-2 and R1-4 but not R1-1.)
CI / unreviewed dimensions
- No standard test-suite check-runs (
Test (ubuntu-latest, Node 22.x)) appear for86af37b4e9— either fork-gated or not yet triggered. Cannot confirm green tests on new head. packages/acp-bridge/src/bridge.tsis ~555 KB; only the newretireWhenSessionsDrain/preserveForFirstStatusRead/hasNoSessionWorkhunks were reviewed via the diff API. Full-file coverage of that module remains an unreviewed dimension.
Reviewed with AI assistance.
Collaborator
Author
|
关于 NEW-Q1，已按当前 head 具体顺序如下：
现有测试 并发 keep-alive 情况也不会破坏该语义：如果 status 入口处 因此 NEW-Q1 可以确认不是当前实现中的缺陷；关键是 |
chiga0
approved these changes
Aug 31, 2026
chiga0
left a comment
chiga0
left a comment
Collaborator
There was a problem hiding this comment.
Approving on 86af37b4e940. All functional blockers from prior rounds are resolved.
Verified fixed (from CI bot Critical findings on the first commit):
- R1-1 (status read reaps startup-preheated channel):
withWorkspaceStatusReadpassesstatusRead=true;withWorkspaceControlcomputespreservePreheat = statusRead && ci.preserveForFirstStatusRead && resolvedChannelIdleTimeoutMs() <= 0; the finally guard!preservePreheatpreventsstartIdleTimerfrom firing after the first status read. Fixed. - R1-2 (workspace-control timeout kills live sessions):
retireChannelOnTimeoutnow delegates toretireChannelAfterSessionsDrain, which defers toretireWhenSessionsDrain=truewhen session work is active. Fixed. - R1-3 (MCP discovery timeout unconditionally kills channel):
workspaceMcpDiscoveryTimercallback changed fromkillChannelWithLogtoretireChannelAfterSessionsDrain. Fixed. - R1-4 (per-server MCP auth timeout kills channel with live sessions): also changed to
retireChannelAfterSessionsDrain. Fixed.
NEW-Q1 (my Round 2 question): Confirmed not a defect. preheat() finally passes resolvedChannelIdleTimeoutMs() > 0 as armIdleTimer; zero timeout means idle timer is not armed there. The !preservePreheat guard in withWorkspaceControl finally provides the second layer. Existing test covers the path.
Not reviewed:
- R1-5 (CI bot): Docs claim that untrusted workspaces cannot start ACP across five pages -- I did not read those docs and cannot confirm or refute the code-vs-docs gap.
- R1-6 through R1-18: Suggestion-level doc and test coverage findings; I judge none as must-fix for this merge.
- CI test results: No standard test check-runs visible on this fork commit.
Reviewed with AI assistance.
yiliang114
enabled auto-merge
August 31, 2026 09:37
yiliang114
approved these changes
Aug 31, 2026
yiliang114
left a comment
yiliang114
left a comment
Collaborator
There was a problem hiding this comment.
LGTM on 86af37b4e940. Reviewed the full production diff: the lifecycle-work accounting in bridge.ts (runtimeOperationReservations / settleReleasedRuntimeWork), the retireChannelAfterSessionsDrain safe-drain path replacing unconditional channel kills on workspace-control / MCP discovery / MCP auth timeouts, the preserveForFirstStatusRead guard in withWorkspaceControl, epoch monotonicity checks, the new coordinator/routes/error mapping, and the SDK additions all read consistently, and the Round-1 Critical findings are addressed at this head.
Note on CI: the only red job on this head is Test (ubuntu-latest, Node 22.x), failing on a 5s timeout in packages/vscode-ide-companion/src/utils/imageSupport.bundle.test.ts — a package this PR does not touch, so it looks like an unrelated flake. Integration tests and desktop shell jobs are green.
This was referenced Aug 31, 2026
Collaborator
|
Released in v0.23.0. |
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
5 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 establishes workspace runtime ownership as the daemon's lifecycle boundary. A workspace now exposes explicit runtime ensure/status APIs, reports a Bridge-owned physical lifecycle snapshot and monotonically increasing epoch, keeps the ensured runtime warm for a renewable window, and carries runtime activity through workspace removal and daemon shutdown admission.
The implementation makes physical ACP work—not session count—the authority for channel retention. Session create/restore, workspace control, MCP discovery/authentication, generation, and preheat all hold lifecycle work until their real settlement. Primary and workspace-qualified SDK calls use the same owner-aware REST contract, and capability advertisement is conservative when an injected Bridge does not implement lifecycle snapshots.
This is the foundation layer only: runtime ensure currently guarantees an initialized reusable ACP channel. Capability convergence and catalog projections remain follow-up work and are explicitly separated in the design contract.
Why it's needed
The previous session-centric lifecycle could treat an ACP process as disposable when the last session closed even though workspace-level work was still running. It also made management clients infer runtime readiness from session state and made runtime replacement epochs ambiguous. Making the workspace the owner provides one lifecycle truth, prevents cross-workspace fallback, and gives clients a stable way to start and observe a runtime without creating a hidden session.
Reviewer Test Plan
How to verify
Confirm that runtime status is cold before startup, ensure initializes the exact trusted workspace without creating a session, concurrent ensure callers share one physical startup, and later ensure calls renew the warm window. Confirm that primary and qualified routes never fall back to another workspace and reject unavailable, untrusted, draining, or unsupported runtimes with structured responses.
Exercise session create/restore together with workspace status, MCP discovery/authentication, generation, removal, and daemon shutdown. The ACP child must remain alive while any physical work lease is unsettled, timeouts must retire or safely drain the owning channel where the underlying operation cannot be cancelled, and a replacement runtime for the same canonical workspace must advance rather than reset its epoch.
Evidence (Before & After)
N/A — daemon lifecycle, REST/SDK, tests, and documentation only; no TUI change.
Tested on
Environment (optional)
Local workspace build and package-scoped Vitest runs with the repository Node.js 22+ toolchain.
Risk & Scope
Linked Issues
Supersedes #8213.
中文说明
本 PR 做了什么
本 PR 将工作区运行时所有权确立为 daemon 的生命周期边界。工作区现在提供明确的 runtime ensure/status API，报告由 Bridge 掌握的物理生命周期快照和单调递增 epoch，为显式 ensure 的运行时提供可续期保活窗口，并把运行时活动纳入工作区移除及 daemon 关闭的准入流程。
实现改为由真实 ACP 物理工作而不是 Session 数量决定 Channel 是否保留。Session 创建/恢复、工作区控制、MCP discovery/认证、生成以及 preheat 都会持有 lifecycle work，直到真实操作结束。Primary 与 workspace-qualified SDK 调用复用同一套 owner-aware REST 契约；当注入的 Bridge 没有实现生命周期快照时，daemon 会保守地不发布该能力。
本 PR 只落地 foundation 层：当前 runtime ensure 只保证 ACP Channel 已初始化且可复用。Capability 收敛和 Catalog 投影属于后续工作，并已在设计契约中明确分开。
为什么需要它
旧的 Session-centric 生命周期可能在最后一个 Session 关闭时把 ACP 进程视为可回收，即使工作区级任务仍在运行。管理客户端还需要根据 Session 状态猜测运行时是否就绪，运行时替换后的 epoch 也缺少清晰语义。让 Workspace 成为 owner 后，生命周期只有一个事实来源，不会跨工作区兜底，客户端也能在不创建隐藏 Session 的情况下稳定地启动和观察运行时。
Reviewer 测试计划
如何验证
确认运行时启动前状态为 cold；ensure 能初始化精确匹配且受信任的工作区，并且不会创建 Session；并发 ensure 共享一次物理启动；后续 ensure 会续期保活窗口。确认 primary 和 qualified 路由绝不会回退到其他工作区，并对不可用、未信任、draining 或不支持 lifecycle 的运行时返回结构化错误。
组合验证 Session 创建/恢复、工作区状态、MCP discovery/认证、生成、工作区移除和 daemon 关闭。只要任一物理 work lease 尚未结束，ACP 子进程就必须继续存活；对于底层无法取消的操作，超时必须回收或安全 drain 其 owning Channel；同一 canonical workspace 替换运行时后 epoch 必须递增而不是重置。
证据（Before & After）
不适用——本 PR 只修改 daemon 生命周期、REST/SDK、测试和文档，没有 TUI 变化。
测试平台
环境（可选）
使用仓库要求的 Node.js 22+ 工具链，在本地 workspace 完成构建和按 package 执行的 Vitest 验证。
风险与范围
关联事项
替代 #8213。