Skip to content

perf(cli): defer startup prefetch tasks#6303

Open
water-in-stone wants to merge 15 commits into
QwenLM:mainfrom
water-in-stone:perf/speed-start
Open

perf(cli): defer startup prefetch tasks#6303
water-in-stone wants to merge 15 commits into
QwenLM:mainfrom
water-in-stone:perf/speed-start

Conversation

@water-in-stone

Copy link
Copy Markdown
Contributor

… (#3222)

What this PR does

This PR moves interactive telemetry SDK startup off the pre-render REPL critical path and starts it after the first Ink render alongside the
existing post-render startup prefetch work. It also keeps IDE startup awaited for prompt-interactive sessions that auto-submit an initial
prompt, while avoiding a duplicate post-render IDE connection in that path.

Why it's needed

Interactive startup should render the REPL before optional background work such as telemetry initialization, update checks, IDE connection for
ordinary TUI sessions, and housekeeping completes. At the same time, prompt-interactive and non-interactive flows must keep their first-
request correctness guarantees, especially IDE context and MCP tool availability.

Reviewer Test Plan

How to verify

Confirm that ordinary interactive startup defers telemetry initialization and IDE connection until after first render, while prompt-
interactive still awaits IDE context before the auto-submitted first request and does not connect IDE a second time after render. Confirm that
non-interactive behavior is unchanged and still waits for MCP readiness before the first model request.

Local verification commands run:

cd packages/core && npm run typecheck
cd packages/cli && npm run typecheck
cd packages/cli && npx vitest run src/startup/startup-prefetch.test.ts src/gemini.test.tsx src/core/initializer.test.ts
cd packages/core && npx vitest run src/config/config.test.ts -t "telemetry"  

All commands passed locally.

Evidence (Before & After)

Before: Telemetry and checkForUpdates were loaded before rendering in interactive CLI startup.

After: Telemetry and checkForUpdates are no longer loaded before rendering in interactive CLI startup.

Tested on

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

Environment (optional)

Local npm workspace on macOS; verification used package-level TypeScript checks and targeted Vitest suites.

Risk & Scope

  • Main risk or tradeoff: Interactive telemetry events emitted before the post-render telemetry startup can be no-ops because this change
    intentionally does not add buffering.

  • Not validated / out of scope: Manual TUI timing capture, Windows-specific behavior, Linux-specific behavior, and full integration suites
    were not run locally.

  • Breaking changes / migration notes: None expected. Non-interactive, stream-json, ACP/Zed, and MCP first-request readiness semantics are
    preserved.

Linked Issues

Related to #3011 and #3222.

中文说明

What this PR does

本 PR 将交互式遥测 SDK 的启动从预渲染 REPL 关键路径中移出,改为在首次 Ink 渲染后与现有的渲染后启动预取工作一同启动。同时,对于自动提交初始提示词的 prompt-interactive 会话,保持 IDE 启动的 await 行为,并避免在该路径中重复进行渲染后的 IDE 连接。

Why it's needed

交互式启动应在渲染 REPL 之后再执行可选的后台工作,如遥测初始化、更新检查、普通 TUI 会话的 IDE 连接以及内务处理。与此同时,prompt-interactive 和非交互式流程必须保持其首次请求的正确性保证,尤其是 IDE 上下文和 MCP 工具可用性。

Reviewer Test Plan

How to verify

确认普通交互式启动将遥测初始化和 IDE 连接延迟到首次渲染之后,而 prompt-interactive 在自动提交的首次请求前仍会 await IDE 上下文,且不会在渲染后再次连接 IDE。确认非交互式行为保持不变,仍在首次模型请求前等待 MCP 就绪。

本地验证命令运行通过:

cd packages/core && npm run typecheck
cd packages/cli && npm run typecheck
cd packages/cli && npx vitest run src/startup/startup-prefetch.test.ts src/gemini.test.tsx src/core/initializer.test.ts
cd packages/core && npx vitest run src/config/config.test.ts -t "telemetry"

所有命令在本地均通过。

Evidence (Before & After)

修改前:对于普通的交互式的命令行启动方式,会在 Render 前加载 telemetry, 并执行 checkForUpdates
修改前:对于普通的交互式的命令行启动方式,不会在 Render 前加载 telemetry 和 checkForUpdates

Tested on

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

Environment (optional)

macOS 上的本地 npm workspace;验证使用了包级别的 TypeScript 检查和针对性的 Vitest 套件。

Risk & Scope

-主要风险或权衡:在渲染后遥测启动之前发出的交互式遥测事件可能为 no-op,因为本次变更有意不添加缓冲机制。

  • 未验证 / 超出范围:手动 TUI 时序捕获、Windows 特定行为、Linux 特定行为以及完整集成套件未在本地运行。

  • 破坏性变更 / 迁移说明:无预期变更。非交互式、stream-json、ACP/Zed 以及 MCP 首次请求就绪语义保持不变。

Linked Issues

关联 #3011#3222

heyang.why added 6 commits July 4, 2026 17:13
…up prefetch

Address three test coverage gaps identified during code review:

- Assert mockStartEarlyStartupPrefetches in both kitty protocol tests
  (C1: API preconnect call was wired but never verified)
- Add Zed/ACP integration test verifying deferIdeConnection is false
  when getExperimentalZedIntegration returns true (C2: Zed path was
  entirely untested)
- Assert mockStartBackgroundHousekeeping in startup-prefetch test
  (C3: unconditional housekeeping dispatch was never verified)
@water-in-stone water-in-stone changed the title Perf/speed start perf(cli): defer startup prefetch tasks Jul 4, 2026
@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Thanks for the PR!

Template looks good ✓ — all required headings present with bilingual description.

Problem: The interactive startup path blocks on telemetry SDK init, update checks, and IDE connection before first paint. This is observable — running qwen interactively shows the REPL only after these tasks complete. Related to parent issue #3011 on startup performance. This is a real optimization, not theoretical hardening.

Direction: Aligned. Startup performance directly affects user experience for a CLI tool. Deferring non-critical work past first render is a well-established pattern. CHANGELOG doesn't have a direct reference to this specific optimization, but the area is relevant — #3219 (startup profiler) and #3221 (lazy tool registration) already landed as part of the same initiative.

Approach: The scope feels right. The new startup-prefetch.ts (112 lines) cleanly consolidates scattered fire-and-forget operations into one module with a runDeferredTask helper, idempotency via WeakSet, and proper error swallowing. The connectIdeForStartup extraction from initializer.ts is a minimal, well-scoped refactor. The deferTelemetryInitialization flag on ConfigParameters is a clean 3-line change in core. Test coverage looks comprehensive — new unit tests for the prefetch module, updated tests for initializer, gemini, startInteractiveUI, and core config.

One question: the gemini.test.tsx adds a new vi.mock('./acp-integration/acpAgent.js') — is this addressing a pre-existing test gap or related to the startup changes?

Moving on to code review. 🔍

中文说明

感谢贡献!

模板完整 ✓ — 所有必要标题齐全,包含双语说明。

问题:交互式启动路径在首次渲染前阻塞了遥测 SDK 初始化、更新检查和 IDE 连接。这是可观测的——交互式运行 qwen 时,REPL 需要等这些任务完成才显示。关联父 issue #3011(启动性能优化)。这是真实的性能优化,不是理论性加固。

方向:对齐。启动性能直接影响 CLI 工具的用户体验。将非关键工作延迟到首次渲染后是成熟的优化模式。CHANGELOG 没有直接引用此优化,但相关领域已有基础——#3219(启动性能分析器)和 #3221(延迟工具注册)是同一优化计划的一部分。

方案:范围合理。新的 startup-prefetch.ts(112 行)将分散的 fire-and-forget 操作整合到一个模块中,使用 runDeferredTask 辅助函数、WeakSet 实现幂等性、以及正确的错误吞没。connectIdeForStartupinitializer.ts 中提取是最小化的、范围清晰的重构。核心模块的 deferTelemetryInitialization 标志仅有 3 行改动。测试覆盖全面。

一个问题:gemini.test.tsx 新增了 vi.mock('./acp-integration/acpAgent.js')——这是修复已有的测试缺口还是与启动变更相关?

进入代码审查 🔍

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Code Review

The implementation is clean and well-structured. A few observations:

startup-prefetch.ts — solid design. WeakSet idempotency is the right call (handles React StrictMode, test re-entries). runDeferredTask provides uniform error swallowing and profiler events. The fire-and-forget contract is clear: callers get a void return, failures go to debug logs only.

initializer.ts — the connectIdeForStartup extraction is minimal and correct. The deferIdeConnection option defaults to false, preserving backward compatibility for all non-TUI paths. Clean.

config.ts (core)deferTelemetryInitialization is a 3-line addition: field + conditional guard. No behavioral change for non-interactive or ACP paths. The cross-package surface is minimal.

gemini.tsx — the deferIdeConnection computation logic (interactive && !zed && !input) correctly gates the deferred path. Only plain TUI without an initial prompt defers; prompt-interactive, -p, stream-json, and ACP all keep awaited semantics. The housekeeping scheduler migration from inline void import(...) to the prefetch module is a clean consolidation.

startInteractiveUI.tsx — replaces inline checkForUpdates() with startPostRenderPrefetches(). The connectIde: options.postRenderConnectIde ?? true default is correct.

No correctness bugs, security holes, or regressions found. No AGENTS.md violations — the new module follows the project's conventions (collocated tests, ESM, kebab-case naming, vi.hoisted() mocks).

The new vi.mock('./acp-integration/acpAgent.js') in gemini.test.tsx appears to be addressing a pre-existing mock gap in the import boundary test — the ACP module may have been added to gemini.tsx in a prior PR without a corresponding mock. Minor, not blocking.

Test Results

All 66 unit tests pass:

✓ src/startup/startup-prefetch.test.ts (12 tests) 18ms
✓ src/core/initializer.test.ts (13 tests) 7ms
✓ src/gemini.test.tsx (29 tests) 5236ms
✓ src/config/config.test.ts - telemetry (12 tests) 13ms

Typecheck passes for both packages/cli and packages/core.

Build + bundle succeed.

Real-Scenario Testing

Non-interactive mode (-p)

Before (installed build):

$ qwen -p 'what is 1+1?'
Warning: QWEN_HOME points to "..." but no settings.json was found there...
2

After (this PR via npm run dev):

$ npm run dev -- -p 'what is 1+1?'
> @qwen-code/qwen-code@0.19.6 dev
> node scripts/dev.js -p what is 1+1?

Warning: QWEN_HOME points to "..." but no settings.json was found there...
DEV is set to true, but the React DevTools server is not running.
2

Both complete correctly with identical output. Non-interactive path is unaffected by this change (no deferred telemetry/IDE, as designed).

Interactive mode (-i)

$ npm run dev -- -i

REPL rendered within 4 seconds (detected via capture-pane -e). The Ink TUI uses the alternate screen buffer which doesn't produce clean capture-pane text, but the REPL starts and accepts input normally. Ctrl-C exits cleanly.

Prompt-interactive mode

The deferIdeConnection logic correctly evaluates to false when config.getQuestion() returns a value, keeping the awaited IDE path. Not directly observable in this environment (requires an IDE to connect to), but verified through unit test assertions.

中文说明

代码审查

实现干净且结构清晰。几点观察:

startup-prefetch.ts — 设计扎实。WeakSet 幂等性是正确的选择(处理 React StrictMode、测试重复调用)。runDeferredTask 提供统一的错误吞没和性能分析事件。fire-and-forget 的契约清晰:调用者获得 void 返回值,失败仅记录在调试日志中。

initializer.tsconnectIdeForStartup 提取是最小化且正确的。deferIdeConnection 默认值为 false,保持了所有非 TUI 路径的向后兼容性。干净。

config.ts(core)deferTelemetryInitialization 是 3 行新增:字段 + 条件守卫。非交互式和 ACP 路径无行为变化。跨包影响面最小。

gemini.tsxdeferIdeConnection 的计算逻辑(interactive && !zed && !input)正确地把控了延迟路径。仅有无初始提示的普通 TUI 会延迟;prompt-interactive、-p、stream-json 和 ACP 都保持等待语义。housekeeping 调度器从内联 void import(...) 迁移到 prefetch 模块是干净的整合。

未发现正确性 bug、安全漏洞或回归。无 AGENTS.md 违规。

gemini.test.tsx 中新增的 vi.mock('./acp-integration/acpAgent.js') 看起来是修复先前 PR 遗漏的 mock 缺口——不阻塞。

测试结果

全部 66 个单元测试通过。CLI 和 Core 的 TypeScript 类型检查通过。构建和打包成功。

真实场景测试

非交互式模式(-p):before/after 均正确输出答案 "2",行为一致。

交互式模式(-i):REPL 在 4 秒内渲染完成,正常接受输入,Ctrl-C 干净退出。

Prompt-interactive 模式:deferIdeConnection 在有初始提示时正确计算为 false,保持等待 IDE 路径(通过单元测试断言验证)。

Qwen Code · qwen3.7-max

@qwen-code-ci-bot

Copy link
Copy Markdown
Collaborator

Stepping back to look at the whole picture: this PR does exactly what a startup performance optimization should — it moves the right things off the critical path while carefully preserving correctness on every non-TUI codepath.

The implementation is straightforward and doesn't try to do more than needed. A single new module (startup-prefetch.ts, 112 lines) with two entry points, a runDeferredTask helper, and WeakSet idempotency. The existing scattered fire-and-forget code is replaced, not duplicated. The connectIdeForStartup extraction is the minimum refactor needed to share IDE connection logic between the awaited and deferred paths. The core change is 3 lines.

What I appreciate: the author clearly mapped every codepath (plain TUI, prompt-interactive, -p, stream-json, ACP/Zed) and made sure each one keeps its correctness guarantees. The deferIdeConnection flag in gemini.tsx — computed as interactive && !zed && !input — is the kind of precise gating that shows real understanding of the startup flow. The test suite (66 tests) covers the new module, the modified initializer, the main entry point, and the core config.

The design doc is thorough (perhaps more thorough than needed for 112 lines of code), but it documents the architectural decisions and tradeoffs well — useful for future maintainers.

Real-scenario testing confirms non-interactive mode works identically before and after, and interactive REPL renders within 4 seconds. The Ink alternate screen buffer makes clean capture-pane text difficult, but the functional behavior is verified.

My independent proposal would have been essentially the same approach — consolidate fire-and-forget operations, add deferred options. The PR's implementation exceeds that with the profiler event integration and idempotency guards.

Approving. ✅

中文说明

回顾全局:这个 PR 完美地完成了启动性能优化应该做的事——将正确的操作移出关键路径,同时仔细保留了每条非 TUI 代码路径的正确性。

实现直截了当,没有过度设计。一个 112 行的新模块,两个入口点,一个 runDeferredTask 辅助函数,和 WeakSet 幂等性。现有的分散 fire-and-forget 代码被替换而非重复。connectIdeForStartup 提取是共享 IDE 连接逻辑所需的最小重构。核心模块变更仅 3 行。

值得赞赏的是:作者清晰地映射了每条代码路径(普通 TUI、prompt-interactive、-p、stream-json、ACP/Zed),并确保每条路径都保持了正确性保证。

真实场景测试确认非交互模式前后行为一致,交互式 REPL 在 4 秒内渲染完成。

批准。✅

Qwen Code · qwen3.7-max

@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. ✅

debugLogger.warn(
`${name} failed: ${err instanceof Error ? err.message : String(err)}`,
);
});

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] runDeferredTask interpolates err.message into a string before passing it to debugLogger.warn, which drops the stack trace. The debugLogger's formatArgs already handles Error objects specially (extracting arg.stack), but it never sees the original error here.

Suggested change
});
debugLogger.warn(`${name} failed:`, err);

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed. runDeferredTask now passes the original error object to the logger:

debugLogger.warn(`${name} failed:`, err);

This preserves stack traces through formatArgs, and the startup prefetch tests assert the logger receives the original Error object.

preconnectApi(authType, { resolvedBaseUrl, proxy });
recordStartupEvent('startup_prefetch_started', { name: 'api_preconnect' });
} catch (error) {
debugLogger.debug(

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] api_preconnect records startup_prefetch_started but never emits the corresponding startup_prefetch_completed or startup_prefetch_failed event. All post-render tasks go through runDeferredTask which records all three lifecycle events. This asymmetry means profiling consumers see api_preconnect as perpetually in-flight.

Consider either wrapping the call in runDeferredTask for consistency, or removing the manual recordStartupEvent here so only runDeferredTask-managed tasks appear in the startup_prefetch_* event stream.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed. The manual startup_prefetch_started event for api_preconnect has been removed, so only tasks managed by runDeferredTask enter the balanced startup_prefetch_* lifecycle stream.

There is also a regression test asserting that api_preconnect does not emit an unbalanced lifecycle event.

});
}

runDeferredTask('background_housekeeping', async () => {

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 original code in gemini.tsx guarded housekeeping with if (config.isInteractive()) and a comment explaining that non-interactive modes don't create the file-history dirs this cleans. That guard was dropped in the migration.

This is safe today since startPostRenderPrefetches is only called from startInteractiveUI, but the function's signature accepts any Config — a future non-interactive caller would silently start housekeeping. Consider restoring the guard for defense-in-depth, or adding a comment noting the implicit invariant.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed. background_housekeeping is now guarded by config.isInteractive() inside startPostRenderPrefetches(), preserving the previous invariant even if a future non-interactive caller invokes this helper directly.

The startup prefetch tests cover the non-interactive case and assert housekeeping is not scheduled.

@wenshao wenshao requested a review from doudouOUC July 4, 2026 15:52

@doudouOUC doudouOUC 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.

Startup deferral is well-structured with good idempotency guards. A few findings target files outside this PR's diff (WebShellSidebar, autofix workflow, errorReporting, acpAgent) — flagging them here since inline comments aren't possible on those files:

  • WebShellSidebar (WebShellSidebar.tsx): handleArchive/handleUnarchive only reload the target list, not the source list — sessions appear in both active and archived sections simultaneously. confirmDeleteSession is missing the busySessionIdRef guard that archive/unarchive both have, allowing concurrent operations.
  • Autofix workflow (qwen-autofix.yml): The claim step removes autofix/approved before posting the claim comment. If the comment fails, the issue becomes invisible to all future automated scans (stuck state).
  • Test gaps: Archive/unarchive error paths, archived session deletion, actions.ts archive/unarchive, and loadSession/resumeSession concurrent isolation are all untested.
  • acpAgent: newSession still assigns this.settings before the first await, inconsistent with loadSession/resumeSession.

For files in the PR diff:

  • startup-prefetch.ts:96-99 — IDE connection failure in deferred mode is only debugLogger.warn. The symptom (IDE features don't work) and cause (deferred connection swallowed) are far apart for debugging.
  • config.ts:2014deferTelemetryInitialization defers for qwen -i "prompt" even though the first model request fires before post-render prefetches run. Consider excluding prompt-interactive from deferral, or add a comment explaining the accepted trade-off.

— qwen3.7-max via Qwen Code /review

Tighten the startup prefetch follow-up fixes from review while keeping
prompt-interactive telemetry on the fast interactive startup path.

- Preserve Error objects when deferred startup tasks fail
- Remove the unbalanced api_preconnect profiler lifecycle event
- Guard background housekeeping so it only runs for interactive configs
- Document and test prompt-interactive telemetry deferral semantics
Ensure sessions launched with an initial interactive prompt have
telemetry ready before the auto-submitted first request runs.

- Exclude prompt-interactive startup from telemetry deferral
- Pass a post-render telemetry option through interactive UI startup
- Skip duplicate post-render telemetry startup for initial prompts
- Update tests to cover the first-prompt telemetry guarantee

Note: Plain interactive TUI startup still defers telemetry post-render.

@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.

No review findings. Downgraded from Approve to Comment: CI still running.

The deferred-startup design is well-structured — WeakSet idempotency guards are correct, all five startup paths (interactive, prompt-interactive, non-interactive, stream-json, ACP/Zed) maintain their correctness guarantees, and the new test suite covers the critical paths. A few low-confidence defensive-programming suggestions were identified but not flagged as inline comments (default-direction mismatch in startInteractiveUI options, duplicated telemetry defer condition across modules, and fragile ide_connect/telemetry_init ordering in startPostRenderPrefetches).

— qwen3.7-max via Qwen Code /review

@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.

No review findings. Downgraded from Approve to Comment: CI still running.

— qwen3.7-max via Qwen Code /review

}

if (options.initializeTelemetry) {
runDeferredTask('telemetry_init', () => {

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] telemetry_init is the only synchronous task among the deferred tasks. initializeTelemetry(config) runs as a single microtask inside Promise.resolve().then(task), blocking the event loop until OTel SDK setup completes (resource attributes, exporters, BatchSpanProcessor, PeriodicExportingMetricReader). Every other deferred task starts with await import(...) which yields to the event loop before doing real work.

This has two consequences:

  1. Performance: The sync microtask can delay React's post-mount useEffect state updates and pending I/O, partially undermining the "fast time-to-interactive" goal.
  2. Ordering dependency: Because telemetry_init is declared before ide_connect and runs synchronously, it completes before connectIdeForStartuplogIdeConnection executes. logIdeConnection guards on isTelemetrySdkInitialized() and silently drops the event if false. This ordering is accidental — if initializeTelemetry is ever made async or the task declarations are reordered, IDE connection telemetry events will be silently dropped with no test catching it.
Suggested change
runDeferredTask('telemetry_init', () => {
if (options.initializeTelemetry) {
runDeferredTask('telemetry_init', async () => {
const { initializeTelemetry: initTelemetry } = await import(
'@qwen-code/qwen-code-core'
);
initTelemetry(config);
});
}

Making this async with a dynamic import (consistent with the other tasks) yields to the event loop before running, and also removes the static initializeTelemetry import at the top of the file. To preserve the ordering guarantee with ide_connect, either sequence them explicitly or add a comment documenting the dependency.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not addressed in this commit.

I kept telemetry_init synchronous for now because changing it to an async dynamic import also changes the implicit ordering with deferred ide_connect; logIdeConnection currently depends on telemetry already being initialized. Fixing this properly should sequence telemetry and IDE connection explicitly, or introduce a shared readiness contract, instead of only adding an async import and risking dropped IDE telemetry events.

This should be handled as a separate follow-up so the scheduling and telemetry-ordering behavior can be reviewed together.

}

if (options.connectIde && config.getIdeMode()) {
runDeferredTask('ide_connect', async () => {

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 deferred IDE connection runs fire-and-forget via runDeferredTask with no timeout. If the IDE IPC endpoint is unresponsive (VS Code extension host restarted, stale socket file, JetBrains backend overloaded), the connect() promise hangs indefinitely inside runDeferredTask. The catch handler only fires on rejection, not on hang. The result: startup_prefetch_started with name: ide_connect is recorded but neither completed nor failed ever fires, and there is no diagnostic trail explaining why IDE context is missing.

This is a new failure mode introduced by the PR. The old code awaited connect() inside initializeApp() before first paint — a hang was immediately visible as "the TUI never appears." The new code makes the same hang invisible.

Consider wrapping the connection in a timeout (e.g., Promise.race with a 10s setTimeout that rejects), so the profiler captures the outcome and debugLogger.warn fires on timeout.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6cba2c578.

Deferred ide_connect is now wrapped in a 10s timeout. If the IDE IPC promise hangs, the timeout rejects through runDeferredTask, so the profiler records startup_prefetch_failed and the debug logger emits a clear timeout diagnostic instead of leaving the task permanently in-flight.

A unit test covers the hanging connection case.

Comment thread packages/cli/src/gemini.tsx Outdated
initializationResult!,
{
postRenderConnectIde: deferIdeConnection,
postRenderInitializeTelemetry: !input,

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 telemetry deferral decision is split across two files with no type-level or comment-level link:

  • cli/config/config.ts decides whether to skip eager init: deferTelemetryInitialization: interactive && !isAcpMode && !question
  • This line decides whether to run post-render init: postRenderInitializeTelemetry: !input

The invariant deferTelemetryInitialization === postRenderInitializeTelemetry must hold for the interactive TUI path, or telemetry either never initializes (silent data loss) or initializes twice (masked today by the telemetryInitialized guard in sdk.ts, but that guard is an implementation detail that could change).

Today the conditions are equivalent (interactive && !isAcpMode && !question simplifies to !question on the interactive path, matching !input). But this relies on three unstated premises: ACP mode never reaches this branch, question and input are the same value, and no future condition is added to either side independently.

Consider adding a linking comment at both sites, or computing the deferral flag once and passing it through to both loadCliConfig and startInteractiveUI.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6cba2c578 with the Config-backed option.

Config now stores the telemetry deferral decision as telemetryInitializationDeferred, and startInteractiveUI() defaults post-render telemetry startup from config.isTelemetryInitializationDeferred() instead of recomputing a separate !input condition in gemini.tsx.

This keeps the eager-skip and post-render-init decisions tied to the same source of truth.

expect(config.getTelemetryEnabled()).toBe(true);
});

it('should initialize telemetry before prompt-interactive startup', async () => {

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] This test verifies deferTelemetryInitialization: false for prompt-interactive, but there is no corresponding positive test verifying deferTelemetryInitialization: true for the ordinary interactive case (no -p, no -i "prompt", not ACP). The positive path is the primary motivation for the feature — deferring telemetry for ordinary TUI startup — yet it has no test. A regression that accidentally always sets false would go undetected.

Consider adding a test with process.argv = ['node', 'script.js', '-i'] (interactive, no question) that asserts deferTelemetryInitialization: true.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6cba2c578.

Added a positive loadCliConfig test for ordinary interactive startup that sets process.stdin.isTTY = true, uses no initial prompt, and asserts deferTelemetryInitialization: true is passed to Config.

const { initializeApp } = await import('./core/initializer.js');
const initializationResult = await initializeApp(config, settings);
let input = config.getQuestion();
const deferIdeConnection =

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.

[Critical] deferIdeConnection only treats a CLI question as an immediate first request, but interactive sessions with --input-file can submit as soon as RemoteInputWatcher.setSubmitFn() is installed. Since IDE connection is now fire-and-forget post-render, an IDE-enabled driver can send that first request before IDE context is connected, regressing the same first-request guarantee this PR preserves for prompt-interactive startup.

Suggested change
const deferIdeConnection =
const hasImmediateInteractiveSubmit = Boolean(
input || config.getInputFile?.(),
);
const deferIdeConnection =
config.isInteractive() &&
!config.getExperimentalZedIntegration() &&
!hasImmediateInteractiveSubmit;

— gpt-5 via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6cba2c578.

deferIdeConnection now treats config.getInputFile?.() as an immediate interactive-submit source, so interactive sessions using --input-file keep the IDE connection awaited before the first remote prompt can run. The post-render prefetch receives connectIde: false in that case to avoid a duplicate connection attempt.

Added a gemini.test.tsx regression test for interactive + input file.

# Conflicts:
#	packages/cli/src/gemini.test.tsx
experimentalLsp: undefined,
channel: undefined,
chatRecording: undefined,
sessionId: undefined,

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.

[Critical] [build] fallbackModel is missing from this mock CliArgs object. The same omission exists at line 1511. CliArgs.fallbackModel is required (no ?), so tsc --build reports TS2345 on both lines. Line 1267 already includes fallbackModel: undefined — these two were missed.

Suggested change
sessionId: undefined,
sessionId: undefined,
fallbackModel: undefined,

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in 6cba2c578.

Both affected mock CliArgs objects now include fallbackModel: undefined, and packages/cli typecheck passes with tsc --noEmit.


await vi.dynamicImportSettled();

expect(mockCheckForUpdates).toHaveBeenCalledTimes(1);

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] handleAutoUpdate is mocked (line 20) and wired into the handleAutoUpdate module mock (line 46), but no test asserts it was actually called. This test verifies checkForUpdates was called but skips the second half of the update pipeline — a regression that drops handleAutoUpdate from the .then() chain would pass all tests.

Suggested change
expect(mockCheckForUpdates).toHaveBeenCalledTimes(1);
expect(mockCheckForUpdates).toHaveBeenCalledTimes(1);
expect(mockHandleAutoUpdate).toHaveBeenCalledWith(
null,
expect.any(Object),
'/repo',
);

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Not addressed in this commit.

I did not add this assertion because the current test intentionally uses a never-resolving checkForUpdates() promise to verify that startPostRenderPrefetches() starts work without awaiting completion. In that specific test, handleAutoUpdate should not run.

A separate test that resolves checkForUpdates() and asserts the handleAutoUpdate(info, settings, projectRoot) handoff would be useful, but it is a test-strengthening follow-up rather than part of the first-request correctness fixes in 6cba2c578.

heyang.why added 2 commits July 6, 2026 10:27
Keep deferred startup work from weakening first-request behavior in
interactive sessions that submit prompts automatically or remotely.

- Store telemetry deferral on Config and reuse that decision at render time
- Keep IDE startup awaited for prompt-interactive and input-file sessions
- Add a timeout for deferred IDE connection failures
- Cover ordinary interactive telemetry deferral and IDE startup edge cases
@water-in-stone

Copy link
Copy Markdown
Contributor Author

Thanks for the detailed review. Status after 6cba2c578:

Fixed in this PR:

  • Deferred IDE failures now have better diagnostics: deferred ide_connect has a 10s timeout, so hangs record startup_prefetch_failed and emit a clear debug warning.
  • Prompt-interactive telemetry is no longer deferred: the telemetry deferral decision is stored on Config, and prompt-interactive startup keeps telemetry initialized before the first auto-submitted prompt.
  • --input-file interactive sessions also keep the IDE connection awaited before first remote submit.

Not fixed here:

  • The WebShellSidebar, autofix workflow, archive/unarchive test gaps, and acpAgent settings-order notes are outside this startup-prefetch PR's diff and should be tracked separately if we want to pursue them.

});
}
startPostRenderPrefetches(config, settings, {
connectIde: options.postRenderConnectIde ?? true,

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 default options.postRenderConnectIde ?? true makes post-render IDE connection opt-out. Combined with initializeApp()'s default deferIdeConnection: false (eager), any future caller of startInteractiveUI that omits options will get a double IDE connection: once awaited in initializeApp, once fire-and-forget in startPostRenderPrefetches. IdeClient.connect() has no re-entry guard.

The sole production caller in gemini.tsx passes the option correctly, masking this footgun.

Suggested change
connectIde: options.postRenderConnectIde ?? true,
connectIde: options.postRenderConnectIde ?? false,

Changing the default to false makes the safe/already-connected state the default, matching initializeApp's eager-connect behavior. The gemini.tsx caller already passes postRenderConnectIde: deferIdeConnection explicitly, so no caller change is needed.

— qwen3.7-max via Qwen Code /review

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Addressed in d05359056.

startInteractiveUI() now defaults postRenderConnectIde to false, so the already-connected/eager IDE path is the safe default and future callers that omit options will not accidentally connect twice. The production caller in gemini.tsx still passes postRenderConnectIde: deferIdeConnection explicitly, so ordinary TUI startup keeps the deferred IDE behavior when intended.

Updated startInteractiveUI tests to assert the safer default. Verified with:

cd packages/cli && npx vitest run src/gemini.test.tsx
cd packages/cli && npm run typecheck
git diff --check

heyang.why and others added 2 commits July 6, 2026 14:17
Default startInteractiveUI to the already-connected IDE path so future
callers do not accidentally connect twice when initializeApp used its
eager default.

- Change the post-render IDE connection default to false
- Update startInteractiveUI tests to assert the safer default
@water-in-stone

Copy link
Copy Markdown
Contributor Author

@wenshao @doudouOUC All the comments have been addressed. Please take another look. Thanks.

@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.

No review findings. Downgraded from Approve to Comment: CI still running.

The startup deferral architecture is well-structured: WeakSet idempotency guards are correct, the runDeferredTask abstraction cleanly separates fire-and-forget work from the critical path, and all five startup flows (interactive, prompt-interactive, non-interactive, ACP/Zed, input-file) are properly handled with comprehensive test coverage (77/77 passing). Typecheck and ESLint clean.

— qwen3.7-max via Qwen Code /review

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants