Skip to content

feat(daemon): Profile ACP channel initialization - #7145

Merged
wenshao merged 2 commits into
QwenLM:mainfrom
doudouOUC:perf/acp-channel-initialize-profile
Jul 18, 2026
Merged

feat(daemon): Profile ACP channel initialization#7145
wenshao merged 2 commits into
QwenLM:mainfrom
doudouOUC:perf/acp-channel-initialize-profile

Conversation

@doudouOUC

Copy link
Copy Markdown
Collaborator

What this PR does

This PR adds an opt-in ACP initialize metadata handshake that records a fixed, bounded set of child-process startup phases and attaches validated timings to the existing parent channel.initialize span. It introduces a lightweight ACP-only profiler before the Gemini import, instruments bootstrap configuration and transport/handler stages, validates profiles fail-open, and protects the qwen serve pre-listen bundle from statically loading the ACP/Gemini profiling runtime.

Why it's needed

Cold daemon channel startup currently spends roughly one second inside an opaque initialize boundary. The existing span cannot distinguish process and module loading, configuration, tools, or response transport, so selecting a safe optimization is guesswork. This change provides attributable measurements without changing readiness, initialization ordering, timeouts, cleanup, retry, concurrent preheat, or Session semantics.

Reviewer Test Plan

How to verify

Build the CLI bundle and send an ACP initialize request with _meta.qwen.daemon.channelStartupProfile.v = 1; confirm the response contains a bounded v1 profile with the fixed phase and configuration keys and that the parent initialize span receives only validated fixed-name attributes. Repeat without the metadata and confirm no profile is returned. Run the malformed/legacy compatibility tests and the serve fast-path closure check; initialize must still succeed when profiling or telemetry data is missing, malformed, unsupported, or throws.

Evidence (Before & After)

Before: channel.initialize exposes one aggregate duration and cannot attribute child startup cost.

After: negotiated initialize responses expose fixed, bounded phase timings and enrich the existing parent span; unnegotiated responses and Session behavior are unchanged. Focused validation passed for the ACP bridge (414 tests), CLI profiling/entry/agent paths (356 tests), core configuration/tracing paths (27 tests), and bundle guard (16 tests). npm run build, npm run typecheck, npm run lint:ci, and npm run check:serve-fast-path-bundle also passed on the rebased branch.

Tested on

OS Status
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

Environment (optional)

macOS local source and release bundle with Node.js 24; negotiated, unnegotiated, malformed, telemetry-disabled, safe-mode, and legacy compatibility paths were covered by focused tests.

Risk & Scope

  • Main risk or tradeoff: Observability payload and timing instrumentation could affect startup or compatibility; this is mitigated by opt-in negotiation, fixed bounded fields, fail-open parsing and telemetry, and no new readiness state.
  • Not validated / out of scope: The representative 2C4G control/candidate acceptance run and any P0-B optimization are intentionally not included in this observability PR. Tool descriptor decoupling, caching, initialization parallelism, and Session behavior changes remain out of scope.
  • Breaking changes / migration notes: None. New parents remain compatible with old children, and new children return no profile to old parents that do not negotiate it.

Linked Issues

N/A

中文说明

本 PR 的改动

本 PR 增加了一个可选协商的 ACP initialize 元数据握手:在子进程中采集一组固定且有界的启动阶段耗时,并将经过校验的耗时写入父进程现有的 channel.initialize span。它在导入 Gemini 前启用轻量的 ACP 专用 profiler,对 bootstrap 配置、transport 和 initialize handler 阶段进行观测,以 fail-open 方式校验 profile,同时防止 qwen serve 的 pre-listen bundle 静态加载 ACP/Gemini profiling runtime。

为什么需要

冷启动 daemon channel 目前约有一秒耗时位于不可拆分的 initialize 边界内。现有 span 无法区分进程和模块加载、配置、工具以及响应传输耗时,因此缺少证据来选择安全的优化项。本改动提供可归因的数据,同时不改变 readiness、初始化顺序、超时、清理、重试、并发预热或 Session 语义。

Reviewer 测试计划

如何验证

构建 CLI bundle,并发送带 _meta.qwen.daemon.channelStartupProfile.v = 1 的 ACP initialize 请求;确认响应包含字段固定且有界的 v1 profile,且父进程 initialize span 只接收经过校验、名称固定的属性。去掉该元数据后重复请求,确认响应不包含 profile。运行畸形数据/旧版本兼容测试和 serve fast-path 闭包检查;当 profiling 或 telemetry 数据缺失、畸形、版本不受支持或抛出异常时,initialize 仍必须成功。

证据(改动前后)

改动前:channel.initialize 只暴露一个总耗时,无法归因子进程启动成本。

改动后:协商过的 initialize 响应会暴露固定且有界的阶段耗时,并丰富父进程现有 span;未协商响应和 Session 行为保持不变。ACP bridge(414 个测试)、CLI profiling/入口/agent 路径(356 个测试)、core 配置/tracing 路径(27 个测试)以及 bundle guard(16 个测试)的聚焦验证均通过;rebase 后的分支还通过了 npm run buildnpm run typechecknpm run lint:cinpm run check:serve-fast-path-bundle

已测试系统

系统 状态
🍏 macOS
🪟 Windows ⚠️
🐧 Linux ⚠️

环境(可选)

macOS 本地源码和 release bundle,Node.js 24;聚焦测试覆盖了协商、未协商、畸形数据、telemetry disabled、safe mode 和旧版本兼容路径。

风险与范围

  • 主要风险或取舍:观测 payload 和计时埋点可能影响启动或兼容性;通过可选协商、固定有界字段、fail-open 解析与 telemetry,以及不引入新 readiness 状态来降低风险。
  • 未验证/范围外:代表性 2C4G 机器上的 control/candidate 验收以及任何 P0-B 优化有意不包含在本观测 PR 中。工具描述符解耦、缓存、初始化并行化和 Session 行为变更仍不在范围内。
  • 破坏性变更/迁移说明:无。新父进程兼容旧子进程;新子进程在旧父进程未协商时不会返回 profile。

关联 Issue

N/A

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC
doudouOUC marked this pull request as ready for review July 18, 2026 02:56
@doudouOUC
doudouOUC requested a review from wenshao July 18, 2026 02:56
@qwen-code-ci-bot qwen-code-ci-bot added category/performance Performance and optimization type/enhancement Non-bug improvement or optimization daemon scope/cli labels Jul 18, 2026
@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Thanks for the PR, @doudouOUC!

Template looks good ✓

Problem: This is a real observability gap, not theoretical hardening. The daemon's channel.initialize span is an opaque ~1s boundary with no phase attribution — you genuinely cannot tell whether Gemini import, Config initialization, tool warmup, or transport setup dominates the cost. The PR correctly frames this as an instrumentation prerequisite for future optimization, not an optimization itself.

Direction: Aligned. Startup profiling and performance observability are core to daemon health. The opt-in negotiation via ACP metadata is the right approach — no overhead when unrequested, backward-compatible in both directions. The design doc under docs/design/ is a helpful addition. No CHANGELOG reference needed since this is internal observability infrastructure, not a user-facing feature.

Size: ~877 production logic lines, ~503 test lines, ~104 docs lines. Not a refactor (title is feat), so no hard block. Under the 1000+ advisory threshold for production lines. Touches core paths (packages/core/src/config/config.ts and packages/core/src/telemetry/daemon-tracing.ts) — Tier 2 applies, will evaluate with full confidence in Stage 2.

Approach: The scope feels right for what it claims — a fixed, bounded profiler with ~20 marks, a simple state machine, and fail-open semantics. The dynamic import in cli.ts (only on --acp / --experimental-acp) keeps the profiler off non-ACP paths entirely. The existing recordStartupEvent sink is reused for Config phases rather than inventing a new cross-package mechanism. The try/finally around config.initialize() in acpAgent.ts correctly handles the bootstrap window even on failure. No drive-by refactors or scope creep.

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓

问题: 这是一个真实的可观测性缺口,而非理论性加固。daemon 的 channel.initialize span 是一个约 1 秒的不透明边界,无法区分各阶段耗时。PR 正确地将其定位为未来优化的观测前提,而非优化本身。

方向: 对齐。启动 profiling 和性能可观测性是 daemon 健康的核心。通过 ACP metadata 进行可选协商是正确方案——未请求时零开销,双向兼容。docs/design/ 下的设计文档是有益的补充。

规模: 约 877 行生产代码、约 503 行测试、约 104 行文档。非 refactor(标题为 feat),无硬阻塞。生产行数低于 1000+ 建议阈值。触及核心路径(config.tsdaemon-tracing.ts)——适用 Tier 2,将在 Stage 2 中充分评估。

方案: 范围合理——固定有界的 profiler(约 20 个 mark),简单状态机,fail-open 语义。cli.ts 中的动态 import(仅在 --acp / --experimental-acp 路径)使 profiler 完全不影响非 ACP 路径。复用已有的 recordStartupEvent sink 采集 Config 阶段事件。acpAgent.tsconfig.initialize()try/finally 正确处理了 bootstrap 窗口。无无关改动或范围蔓延。

进入代码审查 🔍

Qwen Code · qwen3.7-max

Reviewed at b3db6d8ad33811c4c5faff49be890b11b7dd098b · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Code Review + Test Results

Independent proposal (before reading the diff): I would have added a simple mark-based profiler gated on ACP route detection, dynamically imported to avoid non-ACP overhead. Protocol negotiation via _meta, fail-open parsing on the parent side, and reuse of the existing recordStartupEvent sink for Config phases. Bounded payload, no new readiness state.

Comparison: The PR matches this approach closely. The profiler is a clean state machine (enabledfrozen) with performance.now() marks at each phase boundary. The parent-side validator in channel-startup-profile.ts correctly rejects NaN, Infinity, negative, and >600s values. The bridge wraps enrichment in try/catch. The gemini.tsx sink correctly forwards to both the existing profiler and the new ACP profiler when either is enabled.

No critical issues found. A few observations:

  • The cli.ts route detection (rawArgv.some(arg => arg === '--acp' || arg === '--experimental-acp')) uses exact string matching, which is correct and won't match --acp=false. The test confirms this. If new ACP entry points are ever added, this check would need updating, but it's well-tested for the current state.

  • The setActiveSpanAttributes addition to the telemetry interfaces (BridgeTelemetry, createDaemonBridgeTelemetry) is optional and fail-open — correct for an enrichment-only feature.

  • config.ts adds recordStartupEvent calls around every Config phase, including in the ripgrep-skip path (emitting paired start/end marks so the profile stays complete in bare/safe mode). This is a nice touch for producing complete: true even when phases are skipped.

Build:npm run build passes.

Typecheck:npm run typecheck passes.

Tests:

Package Files Tests Status
acp-bridge bridge.test.ts, channel-startup-profile.test.ts 414 ✅ pass
cli acp-startup-profiler.test.ts 4 ✅ pass
cli cli.test.ts 42 ✅ pass
cli acpAgent.test.ts 261 ✅ pass
core config.safe-mode.test.ts, daemon-tracing.test.ts 27 ✅ pass
scripts serve-fast-path-bundle-check.test.js 16 ✅ pass

Real-Scenario Testing

This PR adds internal profiling infrastructure with no user-visible behavior change, so the tmux test verifies the non-ACP path is unaffected.

runner@runnervm3jd5f:~/work/qwen-code/qwen-code/.qwen/worktrees/triage$ node packages/cli/dist/src/cli.js --version 2>&1
0.19.11
runner@runnervm3jd5f:~/work/qwen-code/qwen-code/.qwen/worktrees/triage$ node packages/cli/dist/src/cli.js -p 'say hello' 2>&1 | head -5
Hello! How can I help you today?
runner@runnervm3jd5f:~/work/qwen-code/qwen-code/.qwen/worktrees/triage$

Non-ACP path: version command and interactive prompt both work correctly. The profiling module is not loaded (confirmed by the bundle guard test keeping acp-startup-profiler.ts out of the serve fast-path closure).

中文说明

代码审查 + 测试结果

独立方案(读 diff 前): 基于 ACP 路由检测的 mark-based profiler,动态 import 避免非 ACP 路径开销,通过 _meta 协议协商,父端 fail-open 解析,复用现有 recordStartupEvent sink。有界 payload,无新 readiness 状态。

对比: PR 方案与独立方案高度一致。Profiler 是干净的状态机,父端校验正确拒绝 NaN/Infinity/负值/超限值,bridge 用 try/catch 包裹 enrichment,gemini.tsx sink 正确转发到两个 profiler。

无 critical 问题。 cli.ts 路由检测使用精确字符串匹配,不会误匹配 --acp=falsesetActiveSpanAttributes 是可选的 fail-open 接口扩展。config.ts 在 ripgrep skip 路径也发出成对标记,确保 bare/safe mode 下 profile 完整。

构建 ✅,类型检查 ✅,全部 764 个聚焦测试通过 ✅。

非 ACP 路径的 tmux 测试确认 CLI 正常工作,profiling 模块未被加载(bundle guard 测试也确认了这一点)。

Qwen Code · qwen3.7-max

Reviewed at b3db6d8ad33811c4c5faff49be890b11b7dd098b · re-run with @qwen-code /triage

@qwen-code-ci-bot

qwen-code-ci-bot commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Confidence: 4/5 — Clean implementation of focused observability infrastructure. No correctness issues, well-tested, fail-open throughout.

The PR does exactly what it says: adds a bounded, opt-in startup profiler for ACP channel initialization. The profiler is a simple state machine with no I/O beyond performance.now(), the protocol negotiation uses the existing _meta extension point, and every failure path is fail-open. The core changes (config.ts event markers, daemon-tracing.ts span enrichment) are additive and gated — zero overhead when the profiler isn't active, which is the common case.

The test coverage is thorough: malformed profiles, missing profiles, unsupported versions, telemetry failures, Config phase ordering in safe mode, and the serve fast-path bundle boundary. The tmux sanity check confirms the non-ACP path is unaffected.

The one mild reservation: the cli.ts route detection is a hardcoded string check against rawArgv. It's correct today and well-tested, but it's a maintenance coupling that could silently break if new ACP entry points are added. Not blocking — just something to be aware of.

Approving. ✅

中文说明

置信度:4/5 — 聚焦的可观测性基础设施,实现干净。无正确性问题,测试充分,全程 fail-open。

PR 完成了它所声称的:为 ACP channel initialize 添加有界的、可选的启动 profiler。Profiler 是简单状态机,无 I/O(仅 performance.now()),协议协商使用现有 _meta 扩展点,所有失败路径均 fail-open。核心改动(config.ts 事件标记、daemon-tracing.ts span enrichment)是增量的且有门控——profiler 未激活时零开销(常见情况)。

唯一轻微顾虑:cli.ts 路由检测是硬编码字符串匹配,目前正确且有测试覆盖,但如果将来新增 ACP 入口点可能会无声地失效。不阻塞——仅需注意。

批准合并 ✅

Qwen Code · qwen3.7-max

Reviewed at b3db6d8ad33811c4c5faff49be890b11b7dd098b · re-run with @qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM — well-isolated profiling instrumentation with thorough fail-open design. Maintainer should confirm the telemetry path addition before merging. ✅

@wenshao

wenshao commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Review: feat(daemon): Profile ACP channel initialization

Overview

Opt-in ACP initialize metadata handshake: the bridge requests _meta["qwen.daemon.channelStartupProfile"] = { v: 1 }; the child records a fixed set of startup marks (module-global, first-mark-wins, frozen at first response) and returns a bounded v1 profile; the bridge validates it fail-open and copies fixed-name numeric attributes onto the existing channel.initialize span. Core Config.initialize() gains paired config_initialize_* start/end events (emitted even for skipped phases), window-gated on the child side so per-session Config runs can't contaminate the bootstrap profile. The serve fast-path bundle guard is extended to keep the profiler/Gemini/ACP runtimes out of the pre-listen closure.

What I verified (against the actual code, not just the diff)

  • Span enrichment actually lands. withDaemonBridgeSpanwithDaemonSpantracer.startActiveSpan, and setActiveSpanAttributes reads trace.getSpan(otelContext.active()) inside the withSpan callback — so the attributes attach to the live channel.initialize span. Concurrent channel initializes each see their own context.
  • The daemon path matches the detection. spawnChannel.ts spawns the child with --acp and sets QWEN_CODE_NO_RELAUNCH=true, so runCliEntry's raw-argv check fires and the profile isn't split across a memory-relaunch on the target path.
  • The static import into gemini.tsx is cheap. @qwen-code/acp-bridge/bridgeTypes has only type-only imports plus string/number consts, so non-ACP TUI startup doesn't pay for the profiler beyond a no-op flag check per mark.
  • Arithmetic/completeness consistency. unattributedMs/otherMs are only added when their inputs exist, and the complete flag is false in exactly those cases on both child and parent (Object.values sees the explicitly-undefined keys; JSON drops them on the wire). ripgrepProbeMs is nested under toolRegistryMs and correctly excluded from the otherMs subtraction.
  • Fail-open is tested on both sides: parser rejection paths, span-enrichment throw isolation, handler build-throw fallback, and the safe-mode adjacency ordering test (the startupEventSink module mock it relies on already exists in that test file).
  • Bundle boundary: the source assertion not.toContain("from './utils/acp-startup-profiler.js'") correctly permits the dynamic import(...) form, and the metafile guard + tests cover the three new forbidden roots.

No correctness blockers found. The findings below are minor.

Findings

  1. [minor] Raw-argv detection can false-positive outside ACP modepackages/cli/src/cli.ts matches any argv element literally equal to --acp/--experimental-acp, before yargs parsing. qwen --acp false (space-separated) or a literal option value (e.g. -p "--acp") initializes the profiler and installs the startup event sink for a non-ACP session. Impact is negligible (a ~20-entry marks object plus a no-op sink wrapper; no profile is ever emitted), and the --acp=false test documents the = form — just be aware the space form isn't covered. Not worth complexity to fix; a code comment would do.
  2. [minor] Settings-driven ACP yields no profile — if getExperimentalZedIntegration() comes from settings rather than the CLI flags, the profiler is never initialized and a negotiating parent silently gets nothing (fail-open, complete never even sent). The daemon always passes --acp so the target path is fine; consider one sentence in the design doc so nobody debugs the "missing profile" case later.
  3. [nit] Negotiation is unconditional — the bridge requests the profile on every channel initialize even when the telemetry SDK is uninitialized (attributes are then dropped in setActiveSpanAttributes). That's ~1–2 KB extra per initialize response plus a little child work. Uniform behavior is defensible; gating on SDK state would save it if you care.
  4. [nit] Repeat initialize returns the frozen profile againbuildAndFreezeAcpStartupProfile() rebuilds from frozen marks rather than returning undefined once frozen, so a second negotiated initialize on the same child gets the same startup profile. Harmless (the bridge initializes once per channel), but the design doc's "freezes after building the first response" reads slightly differently.
  5. [docs] unattributedMs includes the request-arrival wait — the gap between transportSetupEnd and initializeHandlerStart contains parent-side scheduling and initialize-request transport, not just inter-phase gaps in the child. Worth a sentence in the design doc so the numbers get interpreted correctly when picking the P0-B optimization.
  6. [process] Core files touchedpackages/core/src/config/config.ts and telemetry/daemon-tracing.ts fall under the AGENTS.md maintainer-gate; the delta is purely additive instrumentation (paired events + one optional telemetry method), which is the easy case, but flagging for maintainer awareness per the two-tier rule.

Test coverage

Good and targeted: parser validation matrix (NaN/Infinity/negative/over-cap/unknown-version/extra-fields), freeze + first-mark-wins semantics, bootstrap-window gating, negotiation on/off (including _meta absence and the <2048-byte size bound asserted in both child unit and agent integration tests), telemetry-throw isolation, safe-mode event ordering, and bundle-guard offenders. The transport-estimate clock-skew guards (negative / over-timeout) are covered too.

Verdict

LGTM with minor suggestions. Well-scoped observability change: bounded fixed-name payload, fail-open on every path, no behavior change to readiness/ordering/timeouts, and the module-boundary protections are enforced by tests rather than convention. Findings 1–5 are non-blocking; none require a re-review if addressed.

中文版本

评审:feat(daemon): Profile ACP channel initialization

概述

可选协商的 ACP initialize 元数据握手:bridge 在请求里带上 _meta["qwen.daemon.channelStartupProfile"] = { v: 1 };子进程记录一组固定的启动 mark(模块级全局、首个 mark 生效、首次响应后冻结)并返回有界的 v1 profile;bridge 以 fail-open 方式校验后,把固定名称的数值属性写到现有 channel.initialize span 上。core 的 Config.initialize() 增加成对的 config_initialize_* start/end 事件(跳过的阶段也会发出相邻事件),子进程侧用 bootstrap 窗口做门控,避免会话级 Config 初始化污染启动 profile。serve fast-path bundle 守卫扩展为禁止 profiler/Gemini/ACP runtime 进入 pre-listen 闭包。

已核实的关键点(对照实际代码而非仅 diff)

  • span 富化确实生效withDaemonBridgeSpantracer.startActiveSpansetActiveSpanAttributeswithSpan 回调内读取 active context,属性会落到存活的 channel.initialize span 上;并发 initialize 各自拿到正确的 span。
  • daemon 路径与检测匹配spawnChannel.ts--acp 启动子进程并设置 QWEN_CODE_NO_RELAUNCH=true,rawArgv 检测能命中,且目标路径不会因内存 relaunch 拆分 profile。
  • gemini.tsx 的静态导入很轻bridgeTypes 只有 type-only import 和常量,非 ACP TUI 启动只多付每个 mark 一次布尔检查。
  • 算术/完整性一致unattributedMs/otherMs 仅在输入齐全时写入,父子两侧 complete 标志在缺失场景一致为 false;ripgrepProbeMs 嵌套在 toolRegistryMs 内且未被重复扣减。
  • 两侧 fail-open 均有测试覆盖;safe-mode 顺序测试依赖的 startupEventSink mock 在该测试文件中已存在。
  • bundle 边界:源码断言正确放行动态 import(...) 形式;metafile 守卫覆盖三个新增禁止项。

未发现阻塞性正确性问题,以下均为次要发现。

发现

  1. [minor] rawArgv 检测在非 ACP 模式下可能误触发--acp false(空格形式)或字面值恰为 --acp 的参数值(如 -p "--acp")会在非 ACP 会话中初始化 profiler 并安装事件 sink。影响可忽略(约 20 项 marks 对象 + 空转 sink 包装;不会输出 profile)。不值得为此增加复杂度,加个注释即可。
  2. [minor] 通过 settings 启用 ACP 时不会产生 profile:若 getExperimentalZedIntegration() 来自 settings 而非 CLI 标志,profiler 不会初始化,协商的父进程会静默拿不到 profile。daemon 恒传 --acp,目标路径无影响;建议设计文档补一句,避免以后排查“profile 缺失”。
  3. [nit] 协商无条件发出:即使父进程 telemetry SDK 未初始化,每次 initialize 也会请求 profile(约 1–2 KB 额外响应 + 少量子进程工作)。行为统一可以接受;在意的话可按 SDK 状态门控。
  4. [nit] 重复 initialize 会再次返回冻结的 profilebuildAndFreezeAcpStartupProfile() 冻结后仍会从原 marks 重建而非返回 undefined。无实际危害(bridge 每 channel 只 initialize 一次),但与设计文档“首次响应后冻结”的措辞略有出入。
  5. [docs] unattributedMs 包含等待请求到达的时间transportSetupEndinitializeHandlerStart 之间包含父进程调度与 initialize 请求传输,不只是子进程各阶段之间的缝隙。建议文档补充一句,便于后续选取 P0-B 优化时正确解读数据。
  6. [process] 涉及 core 文件config.tsdaemon-tracing.ts 属于 AGENTS.md 维护者门控范围;改动为纯增量埋点(成对事件 + 一个可选 telemetry 方法),属于低风险情形,但按两级规则提请维护者知悉。

测试覆盖

覆盖到位:解析校验矩阵、冻结与首 mark 语义、bootstrap 窗口门控、协商开/关(含 _meta 缺失与 <2048 字节大小断言)、telemetry 抛错隔离、safe-mode 事件顺序、bundle 守卫;传输估算的时钟偏移守卫(负值/超时)也有覆盖。

结论

LGTM,附少量建议。 范围克制的可观测性改动:payload 固定有界、全路径 fail-open、不改变 readiness/顺序/超时语义,模块边界由测试而非约定保障。发现 1–5 均不阻塞合并。

Co-authored-by: Qwen-Coder <qwen-coder@alibabacloud.com>
@doudouOUC

Copy link
Copy Markdown
Collaborator Author

@wenshao Thanks for the detailed review. I handled the non-blocking findings as follows:

Finding Action
unattributedMs includes request-arrival wait Fixed in b3db6d8ad3: the design now explicitly says the residual includes the wait between transport setup and the initialize request reaching the child handler.
Raw-argv false positive Not taking: the daemon target always passes the exact --acp flag, the off-target effect is negligible, and a code comment would add noise without changing behavior.
Settings-driven ACP has no profile Not taking: collection is intentionally limited to raw ACP flags so it starts before Gemini import; the design already documents that activation boundary, and the daemon target always passes --acp.
Unconditional negotiation when telemetry is disabled Not taking: keeping protocol behavior independent of local telemetry state is intentional, and the response remains opt-in, bounded, and fail-open.
Repeated initialize returns the frozen profile Not taking: returning the same immutable startup measurement is idempotent and harmless; the production bridge initializes once per channel.

No inline review threads were open, so there was nothing to resolve. The documentation-only follow-up passed Prettier and diff checks.

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed. Suggestions are inline. Not reviewed: reverse audit — its prompt was built, but no agent was launched with it that opened its brief, so the reverse-audit pass did not run.

— qwen3.7-max via Qwen Code /review

Comment on lines +1476 to +1477
it('returns the startup profile only when initialize metadata requests v1', async () => {
initializeAcpStartupProfiler();

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

[Suggestion] The 'QwenAgent MCP SSE/HTTP support' describe block's beforeEach does not call resetAcpStartupProfilerForTesting(), so the profiler module state (enabled=true, frozen=true, marks populated) leaks from this test into subsequent tests in the same file. The 'runAcpAgent shutdown cleanup' block already resets correctly in its beforeEach/afterEach.

Failure scenario: a future test added to this or a subsequent describe block that asserts response does not have _meta would get a stale frozen profile attached unexpectedly.

Suggested change
it('returns the startup profile only when initialize metadata requests v1', async () => {
initializeAcpStartupProfiler();
it('returns the startup profile only when initialize metadata requests v1', async () => {
resetAcpStartupProfilerForTesting();
initializeAcpStartupProfiler();

— qwen3.7-max via Qwen Code /review

@wenshao

wenshao commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Re-review (round 2): delta since 34882f3

Verified the follow-up commit b3db6d8ad against the review findings:

  • Delta scope confirmed: exactly one documentation-only commit — two sentences added to docs/design/acp-channel-initialize-profiling.md stating that top-level unattributed time includes the wait between transport setup and the initialize request reaching the child handler. This resolves finding 5 as requested; the wording is accurate against the mark placement in the code (transportSetupEndinitializeHandlerStart).
  • Findings 1–4 ("not taking"): rationales accepted. I spot-checked the claim on finding 2 — the design doc does already document the activation boundary ("initializes the ACP profiler only when the raw arguments contain --acp or --experimental-acp"), so the settings-driven case is derivable from the doc as written. All four were explicitly non-blocking in round 1.
  • CI: green on b3db6d8ad (ubuntu test suite, web-shell E2E smoke, bundle/precheck gates).

Verdict

LGTM — ready to merge. No code changed since the reviewed head; the docs delta is correct and complete. The round-1 process note stands: packages/core/src/config/config.ts and telemetry/daemon-tracing.ts fall under the maintainer gate, but the delta there is purely additive instrumentation.

中文版本

复审(第二轮):自 34882f3 以来的增量

已对照评审发现核实了后续提交 b3db6d8ad

  • 增量范围确认:仅一个纯文档提交——在 docs/design/acp-channel-initialize-profiling.md 中补充两句,说明顶层 unattributed 时间包含 transport 建立到 initialize 请求到达子进程 handler 之间的等待。此项解决了发现 5;措辞与代码中 mark 的位置(transportSetupEndinitializeHandlerStart)一致。
  • 发现 1–4(不采纳):理由均可接受。抽查了发现 2 的说法——设计文档确实已写明激活边界("仅当原始参数包含 --acp--experimental-acp 时初始化 ACP profiler"),settings 驱动的场景可由此推出。这四项在第一轮中均明确为非阻塞。
  • CIb3db6d8ad 上全绿(ubuntu 测试、web-shell E2E smoke、bundle/precheck 门禁)。

结论

LGTM,可以合并。 自上次评审的 head 以来代码零改动,文档增量正确且完整。第一轮的流程提示仍然有效:packages/core/src/config/config.tstelemetry/daemon-tracing.ts 属维护者门控范围,但相关改动为纯增量埋点。

@wenshao
wenshao enabled auto-merge July 18, 2026 05:10
@wenshao

wenshao commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

@qwen-code /triage

@qwen-code-ci-bot qwen-code-ci-bot left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, looks ready to ship. ✅

@wenshao
wenshao added this pull request to the merge queue Jul 18, 2026
Merged via the queue into QwenLM:main with commit b67506b Jul 18, 2026
72 checks passed
@wenshao

wenshao commented Jul 18, 2026

Copy link
Copy Markdown
Collaborator

Local build & real-run verification (merge reference)

Follow-up to my review rounds (round 1, round 2 LGTM) — this round is build + real-execution evidence at the same head b3db6d8ad (unchanged since round 2). Everything below ran against freshly built artifacts (npm ci + full build) in isolated worktrees: PR head vs merge-base 7795a1b6e for A/B.

Verdict: works as designed on every axis I could drive — ready to merge from my side.

Arm A — real --acp child, raw JSON-RPC wire (no mocks)

Spawned the built CLI (packages/cli/dist/index.js --acp, isolated HOME, QWEN_CODE_NO_RELAUNCH=true mirroring spawnChannel.ts) and drove initialize over raw stdio ndjson:

# request _meta result
1 {v:1} opt-in ✅ profile returned: complete:true, 11 phases + 9 config durations, all finite/2-dp/≤600s, Σ(phases) = processToResponseMs exactly (drift ≈ 0) — the partition invariant holds on the wire
2 none ✅ initialize OK, no profile key (opt-in honored)
3 {v:2} unknown version ✅ initialize OK, no profile (version-gated)
4 {v:1}, initialize ×2 ✅ second response returns the byte-identical frozen profile

No paths, extension names, or user-derived values anywhere in the payload — fixed schema only, matching the design doc's privacy claim.

Cold vs warm run of the same build is exactly the attribution this PR exists to surface: first-ever run processToResponseMs=2217.96 with ripgrepProbeMs=737.43 dominating toolRegistryMs=743.42; warm runs drop to single-digit-ms ripgrepProbeMs.

Arm B — backward compatibility (new parent → old child)

Merge-base build (verified feature-absent: 0 channelStartupProfile hits in dist, no acp-startup-profiler.js) receives the opt-in _meta and ignores it gracefully: initialize succeeds, no profile. Old children are safe behind a new daemon.

Arm C — real qwen serve daemon, span enrichment A/B

Launched the real daemon (serve --port 0 --workspace …) with telemetry {enabled, target:local, outfile}, POST /session → daemon spawns a real ACP channel child → inspected the qwen-code.daemon.bridge / channel.initialize span in the outfile:

  • PR daemon: 24 qwen-code.daemon.acp_startup.* attributes (2 profile + 2 child + 10 phase + 9 config + 1 transport), profile.complete=true, response_transport_ms≈0.7ms (sane for a local pipe). unattributedMs correctly appears only as child.unattributed_ms (not duplicated in the phase.* group).
  • merge-base daemon: same span, same operation, 0 acp_startup attributes — purely additive enrichment.

Suites & gates

  • Focused suites at PR head (fresh worktree): acp-bridge 414/414 (incl. new channel-startup-profile.test.ts), core 27/27 (config.safe-mode ordering + daemon-tracing), cli 307/307 (acp-startup-profiler + cli boundary + acpAgent), scripts 16/16Σ 764 passed, 0 failed.
  • npm run check:serve-fast-path-bundle (clean → cli-only build → DEV bundle → boundary check): passedServe fast-path bundle closure check passed., exit 0 — the new forbidden-input rules (Gemini runtime / ACP agent runtime / ACP startup profiler must stay out of the serve pre-listen graph) hold on a real bundle.
  • CI at head: Ubuntu Node 22.x full suite green (31m); macOS/Windows legs classifier-skipped.

Notes (non-blocking, unchanged from round 2)

The five minor findings from round 1 stand as accepted/documented (raw-argv space-form --acp detection, settings-driven Zed ACP not profiled, ~1–2 KB negotiation with telemetry off, frozen profile on repeat initialize — confirmed live in Arm A #4, unattributedMs includes request-arrival wait — documented in the design doc). None affect correctness of what this PR ships.

🇨🇳 中文版报告(点击展开)

本地构建 + 真实运行验证(合并参考)

继两轮 review(round 1round 2 LGTM)之后,本轮为同一 head b3db6d8ad构建 + 真实执行证据。所有验证均在隔离 worktree 中对全新构建产物(npm ci + 完整 build)进行,并与 merge-base 7795a1b6e 做 A/B 对比。

结论:各条路径均按设计工作,我这边认为可以合并。

Arm A — 真实 --acp 子进程、raw JSON-RPC(无 mock)

以隔离 HOME + QWEN_CODE_NO_RELAUNCH=true(对齐 spawnChannel.ts)启动构建产物,通过 stdio ndjson 驱动 initialize

# 请求 _meta 结果
1 {v:1} 协商 ✅ 返回剖面:complete:true,11 个 phase + 9 个 config 耗时,全部有限/两位小数/≤600s,Σ(phases) 与 processToResponseMs 完全相等(漂移≈0)——分区不变量在线上成立
2 ✅ initialize 正常,响应剖面键(opt-in 生效)
3 {v:2} 未知版本 ✅ initialize 正常,无剖面(版本门控)
4 {v:1},initialize 两次 ✅ 第二次返回完全相同的冻结剖面

载荷中无任何路径、扩展名或用户派生值——只有固定字段,与设计文档的隐私声明一致。

冷/暖对比正是该 PR 要暴露的归因:首次运行 processToResponseMs=2217.96,其中 ripgrepProbeMs=737.43toolRegistryMs=743.42 的绝大部分;暖机后 ripgrepProbeMs 降至个位数毫秒。

Arm B — 向后兼容(新 parent → 旧 child)

merge-base 构建(已确认无该特性:dist 中 0 处 channelStartupProfile、无 acp-startup-profiler.js)收到 opt-in _meta 后优雅忽略:initialize 成功、无剖面。旧 child 在新 daemon 后面是安全的。

Arm C — 真实 qwen serve daemon 的 span 增强 A/B

telemetry {enabled, target:local, outfile} 启动真实 daemon,POST /session 触发真实 ACP channel 子进程 spawn,检查 outfile 中 qwen-code.daemon.bridge / channel.initialize span:

  • PR daemon24qwen-code.daemon.acp_startup.* 属性(2 profile + 2 child + 10 phase + 9 config + 1 transport),profile.complete=trueresponse_transport_ms≈0.7ms(本地管道,合理)。unattributedMs 仅出现在 child.unattributed_ms,未在 phase.* 组重复。
  • merge-base daemon:同一 span、同一操作,0acp_startup 属性——纯增量增强。

测试与门禁

  • PR head 聚焦套件(全新 worktree):acp-bridge 414/414(含新增 channel-startup-profile.test.ts)、core 27/27、cli 307/307、scripts 16/16 —— 共 764 通过,0 失败
  • npm run check:serve-fast-path-bundle(clean → cli-only build → DEV bundle → 边界检查):通过 —— Serve fast-path bundle closure check passed.,exit 0 —— 新增的禁止项(Gemini runtime / ACP agent runtime / ACP startup profiler 不得进入 serve pre-listen 图)在真实 bundle 上成立。
  • head 的 CI:Ubuntu Node 22.x 全量套件绿(31 分钟);macOS/Windows 由分类器跳过。

备注(非阻塞,与 round 2 一致)

Round 1 的五个次要发现维持已接受/已文档化状态(raw-argv 空格形式 --acp 检测、settings 驱动的 Zed ACP 不产生剖面、telemetry 关闭时仍有 ~1–2 KB 协商开销、重复 initialize 返回冻结剖面——已在 Arm A #4 实测确认、unattributedMs 含请求到达等待——设计文档已说明)。均不影响本 PR 交付内容的正确性。


Verification environment: macOS (darwin 24.6), Node v22, isolated worktrees + fresh npm ci; probes drive built artifacts only (no source-mode shortcuts). Evidence scripts: raw-ndjson ACP initialize probe + live serve daemon span probe with telemetry outfile.

tanzhenxin added a commit that referenced this pull request Jul 19, 2026
Resolves the ACP bootstrap conflict (keep #7145 profiling wrapper + warning
emission), drops the session.ts warnings block superseded by #7174, and
guards the ACP getWarnings call against stubbed configs.

Claude-Session: https://claude.ai/code/session_01KwsYFzWZ6VLCxVN8MbeFXb
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

category/performance Performance and optimization daemon scope/cli type/enhancement Non-bug improvement or optimization

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants