fix(cli): bound the goal-runtime startup wait and skip the no-op Bun memory relaunch - #10128
Conversation
Bun accepts --max-old-space-size but ignores it (its heap limit adapts dynamically), so the autoConfigureMemory relaunch only wastes a process hop under Bun.
|
✅ Qwen Triage finished — CI landed green on ✅ Qwen Triage 已完成 —— |
|
Thanks for the PR! Template looks good ✓ Problem: The failure mode is real in shape: Direction: aligned — startup robustness on the default ink path, extracted standalone from the in-flight migration work per #8662 so it benefits the current renderer now. The Bun half is independently verifiable: Bun is not V8, Size: no core-module paths touched (all Approach: minimal and backwards compatible — the options bag keeps the branch/resume call sites unbounded by design, Risk: no high-risk paths matched (Stage 1e check ran clean). No elevated risk signals. Moving on to code review. 🔍 中文说明感谢贡献! 模板完整 ✓ 问题: 故障形态是真实的: 方向: 对齐——默认 ink 路径的启动健壮性,按 #8662 从进行中的迁移工作里单独提取落地,使当前渲染器立即受益。Bun 部分可独立验证:Bun 不是 V8, 规模: 未触及核心模块路径(全部为 方案: 最小且向后兼容——options 参数使 branch/resume 调用点按设计保持无界, 风险: 未命中高风险路径(Stage 1e 检查通过)。无升级风险信号。 进入代码审查 🔍 — Qwen Code · qwen3.8-max Reviewed at |
Code reviewI wrote down my own proposal before reading the diff: for the hang, wrap the startup wait in a The implementation details check out: Two non-blocking notes:
Nothing blocking. TestingCI on the reviewed commit, read through the API (PR code is never executed in triage): the ubuntu Node 22 unit suite is green, precheck / secret scan / CVE audit / Desktop Shell (ubuntu + windows) all pass, zero failed checks. The macOS/Windows unit legs and the CLI integration tests were skipped by the CI profile classifier, and the web-shell E2E smoke is still in flight under the Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行(取最新一次运行),省略 skipped,失败项排在最前。 The three new unit tests pin the mechanism itself: a never-settling readiness promise resolves What the suite does not settle: that the TUI actually continues into a usable command registry after the timeout fires — the unit tests stop at the helper returning 中文说明代码审查读 diff 前我先独立写下方案:挂起问题用 实现细节正确: 两个非阻塞意见:
无阻塞问题。 测试审查提交上的 CI,经 API 读取(triage 从不执行 PR 代码):ubuntu Node 22 单测套件绿,precheck、密钥扫描、CVE 审计、Desktop Shell(ubuntu + windows)全部通过,无任何失败检查。macOS/Windows 单测腿和 CLI 集成测试被 CI profile 分类器跳过,web-shell E2E smoke 仍在 三个新单测钉住了机制本身:永不落定的就绪 promise 在超时后解析为 套件未覆盖的是:超时触发后 TUI 是否真正继续到可用的命令注册表——单测止步于 helper 返回 — Qwen Code · qwen3.8-max Reviewed at |
|
Confidence: 4/5 — minimal, well-tested robustness fix whose direction is corroborated by the maintainer's own migration branch; the one open question (whether the lease-contention hang actually fires on current main) is noted, not blocking. Stepping back: this PR does two small things and does them right. The bounded goal-runtime wait is exactly the shape I'd have chosen — race against an unref'd timer, degrade instead of fail, original semantics preserved for the callers that opt out, and a test suite that fails without the diff. The worst outcome of adding the bound is a 5-second degraded start with a debug breadcrumb; the worst outcome of not having it is a TUI where The diff carries nothing beyond the stated goal — no drive-bys, no churn. Both fixes are identical to what already went through review on the OpenTUI migration branch, and landing them now benefits the ink renderer directly, as claimed. Reservations, stated plainly: (1) the trigger premise (lease contention → permanent hang) is inferred, not observed — I'd like to see a real-world report eventually, but the defensive bound is cheap insurance either way; (2) the degraded continuation after the timeout has no AppContainer-level test; (3) the degraded state is silent without debug logging. None of these block merge; (2) is the part a maintainer-triggered CI is not fully settled at review time (the unit suite is green; the web-shell E2E smoke is still in flight under 中文说明置信度:4/5 —— 最小化、测试良好的健壮性修复,方向由维护者自己的迁移分支佐证;唯一的疑问(租约争用挂起是否真的会在当前 main 上触发)已注明,不构成阻塞。 退一步看:这个 PR 做了两件小事,都做对了。有界的 goal-runtime 等待正是我会选择的形态——与 unref 定时器赛跑、降级而非失败、为不传超时的调用点保留原语义,且测试套件在没有 diff 时会失败。加上界的最坏结果是带 debug 日志的 5 秒降级启动;不加界的最坏结果是 diff 没有超出既定目标的任何内容——没有顺手改动,没有无关噪音。两个修复与已在 OpenTUI 迁移分支上通过评审的版本一致,现在落地确实如所述直接惠及 ink 渲染器。 保留意见,直说:(1) 触发前提(租约争用 → 永久挂起)是推断而非观测——希望将来看到真实报告,但这个防御性上限无论如何都是便宜的保险;(2) 超时后的降级续行没有 AppContainer 级测试;(3) 降级状态在不开 debug 日志时无任何提示。这些都不阻塞合入;(2) 是维护者若想在合入前拿到有效证据时值得触发 审查时 CI 尚未完全落定(单测套件已绿;web-shell E2E smoke 仍在 — Qwen Code · qwen3.8-max Reviewed at |
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
LGTM, looks ready to ship — CI landed green after the review. ✅
…raming - Route the goal-runtime startup timeout through mergeStartupWarnings so users see why goal features are degraded without QWEN_CODE_DEBUG=1. - Reword the Bun memory-flag comment: the one-process relaunch is unconditional, so the guard only stops forwarding a no-op V8 flag into the relaunch/sandbox child.
|
Thanks for the thorough triage — both non-blocking notes are now addressed in
|
yiliang114
left a comment
There was a problem hiding this comment.
Reviewed at head f8c01dc:
waitForGoalRuntimetimeout race is correct:Promise.racekeeps a handler on the losing promise (a late rejection of the goal-runtime promise cannot become unhandled — pinned by test), the timer isunrefed and cleared infinally, and omittingtimeoutMspreserves the original unbounded semantics for the resume/branch call sites.- Degraded-startup path is visible: a startup warning is surfaced when the goal runtime does not settle within 5s.
- Bun:
'bun' in process.versionsshort-circuitsgetNodeMemoryArgsbefore the os/v8 reads; the relaunch itself is unconditional, so this only stops forwarding a no-op flag. - Verified locally:
vitest run packages/cli/src/ui/utils/goal-runtime.test.ts— 6/6 passed; full monorepo build passes. CI green; PR body matches the template.
qqqys
left a comment
There was a problem hiding this comment.
Approving. Independent review at this head (f8c01dc3) found no blocking (Critical) issues: the goal-runtime startup gate is correctly bounded (single getGoalRuntimeReady() call raced against an unref'd timer, Promise.race preventing unhandled late rejections, timeout cleared in finally), the no-timeout path preserves the original unbounded semantics for other callers, and a timed-out startup degrades gracefully with a surfaced warning instead of leaving the TUI unusable; the Bun relaunch change only stops forwarding a no-op flag. Tests pin the settle/never-settles/late-rejection paths. A maintainer (admin) has approved at this head and all ran CI lanes are green; concur.
…efactor (QwenLM#10290) * ci: quarantine external-context mem0 E2E from push lanes (QwenLM#10272) The interactive external-context-mem0-write suite hangs at CLI startup ('Connecting to MCP servers...') on macOS and ecs-qwen pool runners — bisected to QwenLM#10128, tracked in QwenLM#10272, ubuntu-hosted unaffected. Every push E2E run currently fails on it, masking all other signal. Follow the cron-interactive precedent (QwenLM#6986): exclude it from the push lanes (linux both sandbox legs, macOS) and keep it in the nightly isolated matrix so the regression stays visible and the fix is verified when it lands. * ci: extend quarantine to the platform-stalled interactive/serve set Run 33069559004 shows the stall class is broader than mem0: on macOS and ecs-qwen pool, external-context-auto-recall, context-compress-interactive and qwen-serve-channel-workers also fail while ubuntu-hosted stays green (QwenLM#10198's own ubuntu CI passed at merge). Quarantine them with mem0 per the QwenLM#6986 precedent; nightly keeps them as a canary. * fix(cli): handle synchronous goal runtime unavailability
|
Released in v0.22.3. |
…wenLM#10724) * docs(opentui): Record the migration status through the composition root The design doc still described the state of 2026-08-28, with only the infra batch landed. Record the five batches now on main, name the seams the composition root leaves to renderer activation, and list the two items deferred to that batch. * docs(opentui): Record the composition-root contracts and correct stale activation scope The design doc described QWEN_TUI_RENDERER as an existing opt-in and the activation batch as carrying runtime fixes that already shipped in QwenLM#10128. Both drifted from the code while the batches landed, which is the kind of claim a reviewer had to catch on QwenLM#10696. State the contracts the composition-root review settled so the activation batch inherits them instead of rediscovering them. * docs(opentui): Record measured runtime status and what the batch reviews kept finding Three claims in the design doc described instruments and gates that are not in the tree: the session-replay harness (issue QwenLM#10005 is still open, nothing measures flicker today) and plain-Node loadability, which 0.5.8 fails on Node 24 — verified locally, not just reported in review. The recurring finding classes are recorded so the activation batch does not re-earn them.
What this PR does
Two small startup-path robustness fixes on the default (ink) renderer path. First, the goal-runtime readiness wait at startup is now bounded by a 5-second timeout; when the runtime cannot settle in time the TUI continues with goal features degraded instead of hanging. Second, the automatic memory-flag relaunch is skipped entirely under Bun, where the flag has no effect.
Why it's needed
The goal runtime signals readiness through a session-writer lease. When the lease is contended — a crashed or sibling process still holding it — the readiness promise never settles, and startup blocks before the command registry is populated. The result is a TUI where every slash command, including
/quit, answers "Unknown command" and the only escape is killing the process. Bounding the wait trades degraded goal features for a usable CLI, which is the strictly better outcome for the user. Separately, Bun accepts--max-old-space-sizebut ignores it (its heap limit starts small and adapts dynamically), so the memory-configuration relaunch under Bun only wastes a process hop on every startup.Both fixes are extracted from the OpenTUI migration work (#8677), where they were reviewed and regression-tested; they land here standalone because they benefit the current ink renderer immediately and carry no dependency on the migration batches tracked in #8662.
Reviewer Test Plan
How to verify
cd packages/cli && npx vitest run src/ui/utils/goal-runtime.test.ts src/ui/AppContainer.test.tsx src/gemini.test.tsx— 239 tests pass.Promise<boolean>return may be ignored).Evidence (Before & After)
N/A (no user-visible rendering change; behavior change is only on the previously-hung path, where the CLI now starts with goal features degraded and logs a warning)
Tested on
Environment (optional)
npm run build,npm run typecheck, unit tests under Node 22.23.1.Risk & Scope
Linked Issues
Part of the incremental landing tracked in #8662 (originally scheduled with the renderer-activation batch; pulled forward because both fixes benefit the ink renderer directly).
中文说明
本 PR 做了什么
两个启动路径上的健壮性小修复,都在默认(ink)渲染器路径上。其一,启动时等待 goal runtime 就绪增加 5 秒超时上限:超时后 TUI 以 goal 功能降级的方式继续启动,而不是永久挂起。其二,在 Bun 下完全跳过内存参数重启流程,因为该参数在 Bun 下无效。
为什么需要
goal runtime 通过 session-writer 租约通知就绪状态。当租约被争用——例如崩溃的或并行的兄弟进程仍持有租约——就绪 promise 永不落定,启动会卡在命令注册表填充之前。结果就是整个 TUI 里所有斜杠命令(包括
/quit)都返回 "Unknown command",唯一出路是杀进程。给等待设上限,是用 goal 功能降级换取可用的 CLI,对用户是严格更优的结果。另外,Bun 接受--max-old-space-size但实际忽略它(其堆上限从很小起步并动态自适应),所以 Bun 下的内存配置重启每次启动都白白浪费一次进程跳转。两个修复都提取自 OpenTUI 迁移工作(#8677),在那里已经过评审和回归测试;这里单独落地,因为它们立即惠及当前的 ink 渲染器,且不依赖 #8662 跟踪的任何迁移批次。
评审测试计划
如何验证
cd packages/cli && npx vitest run src/ui/utils/goal-runtime.test.ts src/ui/AppContainer.test.tsx src/gemini.test.tsx——239 个测试通过。Promise<boolean>返回值可忽略)。前后对比证据
N/A(无用户可见渲染变化;行为变化只发生在原先挂起的路径上——现在 CLI 会以 goal 功能降级启动并打印一条 warning 日志)
测试环境
环境(可选)
Node 22.23.1 下的
npm run build、npm run typecheck、单元测试。风险与范围
关联 Issue
属于 #8662 跟踪的增量落地的一部分(原计划随 renderer-activation 批次落地;因两个修复都直接惠及 ink 渲染器而提前)。