fix(core): coalesce concurrent Config.initialize() calls - #11037
Conversation
initialize() set its initialized flag synchronously and only then awaited the work, so a caller arriving while the first call was still in flight got an immediate "Config was already initialized" error. Callers that swallow that error proceed on a config whose chat has not started yet: the OpenTUI submit path then lost the first prompt to "Chat not initialized", reddening the E2E Interactive OpenTUI leg on runs like 33834473606 (#11002) whenever startup initialization outlasted the gap between the prompt appearing and the submission landing. The OpenTUI command-registry loader hits the same hazard and carries a bounded self-heal for it. Join the in-flight call instead: await the first flight's promise and return when it settles, propagating its error if it failed. A call after the first one settled still throws as before, so the double-init contract is unchanged once initialization is over.
|
|
|
Thanks for the PR! Template looks good ✓ — every required heading is there, including the Chinese section. Problem: observed, not theoretical. #11002 is the autofix record of a real red Direction: aligned. Losing the user's first prompt to a startup race is squarely a CLI-core problem, and the ink renderer already closed its variant of the same race in #11000, so leaving the OpenTUI path open is an inconsistency rather than a new direction. Reference CHANGELOG has several analogous entries for this class of fix (a startup race silently unregistering a plugin marketplace, a queued-prompt Esc race letting the next turn finish early), so nothing here is off-mission for an agent CLI. Size: core path touched ( Approach: the scope feels right, and I'd have landed in the same place. Two things I checked specifically because this is core. First, it adds no new state — The narrower alternative — gate the OpenTUI input until initialization completes, the way #11000 did for ink — would heal the submit path and leave the command loader's identical hazard standing, along with the 15-second polling self-heal that exists purely to paper over it. Fixing the invariant is both smaller and broader. One follow-up worth a thought, not a blocker: that self-heal's own comment now documents a hazard this PR removes at the source, so retiring it (and its poll budget) is probably worth a separate PR. Leaving it untouched here is the right call for keeping this diff minimal. Risk: no elevated risk signals — the changed files match none of the revert-correlated paths. Because this is core, I did the consumer work rather than eyeballing it: there are exactly eight Moving on to code review. 🔍 中文说明感谢贡献！ 模板完整 ✓ —— 所有必需小标题都在，包括中文说明部分。 问题： 是已观测到的问题，不是理论性加固。#11002 是一次真实 方向： 对齐。让用户的第一条 prompt 因启动竞态而丢失，属于 CLI 核心问题；ink 渲染器已经在 #11000 关掉了同一竞态的它那一份，所以 OpenTUI 这条路仍然敞开是不一致，而不是新方向。参考 CHANGELOG 里有多条同类修复记录（启动竞态导致插件市场被静默注销、排队 prompt 下按 Esc 让下一轮提前结束），因此这个改动对 agent CLI 来说完全不偏离主线。 规模： 触及核心路径（ 方案： 范围合理，我自己也会落到同一个做法上。因为这是核心模块，我专门确认了两点。第一，它没有引入新状态—— 更窄的替代方案——像 #11000 对 ink 做的那样，让 OpenTUI 输入在初始化完成前保持关闭——只能治好提交路径，命令加载器那份完全相同的隐患仍在，连带那个纯粹为掩盖它而存在的 15 秒轮询自愈逻辑也仍在。修不变量本身既更小也更广。有一个值得考虑的后续（不是阻断项）：那个自愈逻辑自己的注释现在描述的正是本 PR 从源头移除的隐患，所以退役它（连同它的轮询预算）大概值得单开一个 PR。本次不动它是保持 diff 最小化的正确选择。 风险： 无升级风险信号——改动文件未命中任何与 revert 相关的路径。因为这是核心改动，我没有靠目测，而是把下游消费者查清了：非测试源码中恰好有八处 进入代码审查 🔍 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
Code reviewNo blocking findings. The change is correct, and I want to be specific about why rather than just say it looks fine. My independent proposal before reading the diff was: keep the in-flight promise on the instance and hand it to concurrent callers, which is what this does. It also does it without inventing state — The invariant repair is the right one. I checked the two things that would have made this a blocker and neither holds:
One non-blocking note. The joining branch returns before the checks underneath it, so a concurrent caller's The reason I'd still raise it is that the docstring doesn't say so. It documents that a mid-flight caller joins, but a future caller who races with a different Tests are load-bearing, not decorative. I traced both new cases against the pre-fix code rather than trusting the description's mutation claim. Reverting the The added four-line comment in sequenceDiagram
participant P1 as OpenTUI input prompt (input-prompt.tsx)
participant P2 as loadInteractiveCommands (slash-dispatch.ts)
participant P3 as Config.initialize (config.ts)
participant P4 as livePromptEvents (live-session.ts)
participant P5 as GeminiClient (client.ts)
P1->>P2: mount effect loads the command registry
P2->>P3: initialize, this is the first flight
Note over P3: the initialized flag is set synchronously while the flight is still running
P1->>P4: user submits a prompt during that window
P4->>P3: initialize, second call
Note over P3: before this PR the second call threw and the bare catch swallowed it
P3-->>P4: now it awaits the in-flight promise and returns when that settles
P4->>P5: getGeminiClient
P5-->>P4: the chat exists, so the turn runs instead of dying on Chat not initialized
TestingThis is an unattended CI run, so per the gate rules I executed nothing from this PR — no build, no test run, no checkout. The evidence below is this commit's own CI, read through the API. Say plainly what that means: the two new tests have not reported a result yet. The unit suite is still in flight, so nothing here confirms they pass, and I'm not going to imply otherwise. Nine checks are green and none are red. The three that matter most for this diff are all still running: Not verified, and why:
Sandboxed verification would settle the two gaps that matter, and the author has write access so both lanes are open:
Final CI results for
One row per check name (latest run); skipped checks omitted; failures sort first. / 每个检查名一行（取最新一次运行），省略 skipped，失败项排在最前。 中文说明代码审查无阻断性问题。 这个改动是正确的，我想说清楚为什么正确，而不是只说一句"看起来没问题"。 在读 diff 之前我自己的方案是：把进行中的 promise 保存在实例上，交给并发调用者——这正是本 PR 的做法。而且它没有凭空造状态： 修复的不变量选得对。 我专门核查了两件可能构成阻断的事，结论都不成立：
一条非阻断备注。 合并分支在底下那些检查之前就 return 了，所以并发调用者的 我之所以仍然提出来，是因为 docstring 没有说明这一点。它写了进行中的调用者会合并，但未来某个带着不同 测试是承重的，不是装饰。 我没有采信描述里的 mutation 说法，而是把两个新用例对着修复前的代码走了一遍。回退
（时序图见上，图中英文标签保持原样：它展示的是 prompt 挂载 → 命令加载器发起第一次 flight → 用户在该窗口内提交 → 第二次调用合并到同一 flight → chat 已存在因此本轮正常执行。） 测试这是一次无人值守的 CI 运行，因此按门禁规则我没有执行本 PR 的任何内容——不构建、不跑测试、不 checkout。下面的证据来自该 commit 自身的 CI，通过 API 读取。把话说清楚：两个新测试目前还没有出结果。 单元测试仍在进行中，所以此处没有任何东西能确认它们通过，我也不会暗示相反。 九项检查为绿，没有红的。对这个 diff 最关键的三项都还在跑： 未验证项及原因：
沙箱化验证可以解决其中两个真正重要的缺口，且作者具备写权限，两条通道都开放：
（CI 结果表见上方机器可读区域。） — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
|
Confidence: 4/5 — the mechanism is verified and the tests genuinely pin it; the one nit is a missing docstring clause, and the thing I can't confirm is field efficacy, which nobody can confirm from a diff. Stepping back: this is a small, honest fix to a real invariant bug, and it landed in the right place. My independent proposal before reading the diff was the same idea — hold the in-flight promise and hand it to concurrent callers — and the PR's version is better than mine would have been, because it reuses two fields that already existed and were already read by the shutdown path rather than adding a third piece of state beside them. That's the difference between a fix and a fix that leaves the next person two mechanisms to reconcile. The thing I'm most satisfied with is that I didn't have to take the framing on faith. The description makes a fairly elaborate causal claim, and elaborate causal claims are exactly what a reviewer nods along to. So I walked it: the prompt's mount effect really does call the command loader, the loader really does call Six months from now I'd thank the author rather than curse them. The docstring is updated in the same hunk instead of being left to drift, the inline comment names the two consumers and the issue number so the why survives, and the settled-case throw is preserved and re-asserted inside the new test — so nobody can quietly relax Two honest reservations, neither blocking. First, the joining branch steps over everything below it — the caller's On the pattern question, since it's the honest one to ask: this author has 41 open PRs, several from the same day, and this branch is the CI autofix lane for its linked issue. I checked whether that was wearing down my judgement in either direction. It shouldn't count against the PR — the author has admin on this repo, so this is a maintainer's own working queue rather than the drive-by volume the gate exists to be skeptical of, and the linked issue carries the repo's own Scope discipline is worth a word too, because it's the part most PRs get wrong. This fixes the invariant and stops. It does not remove the 15-second polling self-heal in the command dispatcher that exists purely to paper over this hazard, even though doing so would have been defensible and would have made the diff look more thorough — that belongs in a follow-up, and leaving it out is what keeps this reviewable and revertable as one idea. Verdict: approve, deferred. CI is still in flight on this commit (the unit suite, lint/typecheck, and the integration leg), so I'm not posting an approval in this run — approving now would attest to a result that doesn't exist yet. Approval is deferred until CI lands green on 中文说明Confidence: 4/5 —— 机制已核实，测试确实承重；唯一的瑕疵是 docstring 少了一个短句，而我无法确认的是实际效果——这一点任何人都无法从 diff 中确认。 退一步看：这是对一个真实不变量 bug 的小而诚实的修复，而且落在了正确的位置。在读 diff 之前我自己的方案是同一个想法——保存进行中的 promise 并交给并发调用者——而 PR 的版本比我本来会写的更好，因为它复用了两个本来就已存在、并且已被 shutdown 路径读取的字段，而不是在旁边再加第三份状态。这就是"一个修复"与"一个让后来者要去调和两套机制的修复"之间的区别。 我最满意的一点是，我不必采信它的叙述。PR 描述提出了一个相当精细的因果链，而精细的因果链恰恰是审阅者最容易点头放过的东西。所以我把它走了一遍：prompt 的挂载 effect 确实调用了命令加载器；加载器确实在一个空的 六个月后我会感谢作者而不是骂他。docstring 在同一个 hunk 里就更新了，没有被留下慢慢失真；内联注释点明了两个消费者和 issue 编号，因此 why 能存活下来；而已结束情况下的抛错被保留，并在新测试内部重新断言——所以没人能悄悄把 两点诚实的保留意见，都不构成阻断。第一，合并分支跨过了它底下的所有内容——调用者的 关于"模式"这个问题，因为诚实地问就该诚实回答：这位作者有 41 个开启的 PR，其中数个来自同一天，而本分支是其关联 issue 的 CI autofix 通道。我检查了这是否在两个方向上磨损了我的判断。它不应当成为对本 PR 的扣分项——作者在本仓库具备 admin 权限，所以这是维护者自己的工作队列，而不是门禁之所以要保持怀疑的那种顺手刷量的模式；关联 issue 也带着本仓库自己的 范围克制也值得一句，因为那是多数 PR 做错的部分。这个 PR 修好不变量就停了。它没有移除命令分发器里那个纯粹为掩盖此隐患而存在的 15 秒轮询自愈逻辑——尽管那么做也说得过去，而且会让 diff 显得更彻底。那属于后续 PR；把它排除在外，正是让本次改动可以作为"一个想法"被审查、被回滚的原因。 结论：批准，但延后。 该 commit 上 CI 仍在进行（单元测试套件、lint/typecheck、集成 leg），因此本次运行我不发布批准——现在批准等于为一个尚不存在的结果背书。批准延后至 CI 在 — Qwen Code · qwen3.8-max-2026-09-02 Reviewed at |
qwen-code-ci-bot
left a comment
qwen-code-ci-bot
left a comment
There was a problem hiding this comment.
Partially reviewed — gaps disclosed. Suggestions are inline.
Not reviewed: build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan (not a blocker): 16 passed — this review observed 23339, 1945, 28502, 298, 1815, 504, 5982, 94 passed.
中文说明
仅完成部分审查，审查缺口已披露。 建议见行内评论。
未审查（原文为英文）：build-and-test — Integration Tests (CLI, No Sandbox) was skipped in CI and its suite did not run locally.
Test Plan（非阻断）：16 passed — this review observed 23339, 1945, 28502, 298, 1815, 504, 5982, 94 passed。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (!this.initializationSettled) { | ||
| await this.initializationPromise; | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-1: The new join branch discards the joining caller's options without notice — the joiner resolves successfully as if they had been applied, and the refreshed docstring does not say that the first caller's options win. If a caller invokes initialize({ sendSdkMcpMessage }) (the shape of the ACP session path) or initialize({ skipMcpDiscovery: true }) while an options-less first flight is in progress on the same Config, it joins, resolves, and proceeds believing its options were honored — SDK MCP messages go nowhere, or discovery runs anyway. A joiner holding an already-aborted signal is never checked (throwIfAborted runs only on the leader path) and blocks on the foreign flight. This is latent today: every in-tree concurrent joiner (live-session.ts:563, slash-dispatch.ts:59, AppContainer.tsx:1070) passes no options — the hazard begins with the first options-bearing joiner. Document first-caller-wins in the docstring, and fail loud on the join path: honor options?.signal?.throwIfAborted() before the await at minimum, or reject a joiner that passes options it cannot honor.
Witness:
PROBE F1: joiner state while first flight still in flight: pending undefined
PROBE F1: joiner final state: resolved
With the implied one-line fix the same probe inverts: promise rejected "DOMException{ stack: 'AbortError: Th…' }" instead of resolving.
| if (!this.initializationSettled) { | |
| await this.initializationPromise; | |
| return; | |
| } | |
| if (!this.initializationSettled) { | |
| options?.signal?.throwIfAborted(); | |
| await this.initializationPromise; | |
| return; | |
| } |
The known concurrent joiners all call with no arguments (slash-dispatch.ts:59, live-session.ts:563, AppContainer.tsx:1070), so aborting or rejecting only options-bearing joiners cannot break the swallow-callers this fix exists for. Please add a test beside the two added cases: with the first flight held on the gate, config.initialize({ skipMcpDiscovery: true }) must reject rather than resolving with the option dropped — removing the guard must make it go red.
中文说明
新的 join 分支会悄无声息地丢弃加入方传入的 options——加入方会成功 resolve，仿佛这些选项已被应用，而更新后的文档注释也没有说明以第一个调用方的选项为准。若某个调用方在同一个 Config 上、第一个无选项的初始化仍在进行时调用 initialize({ sendSdkMcpMessage })（ACP 会话路径的形态）或 initialize({ skipMcpDiscovery: true })，它会加入、resolve 并继续执行，以为自己的选项已生效——SDK MCP 消息将无处可达，或者发现流程照样执行。携带已中止 signal 的加入方不会被检查（throwIfAborted 只在主导路径上运行），会阻塞在别人的初始化上。目前是潜伏的：树内所有并发加入方（live-session.ts:563、slash-dispatch.ts:59、AppContainer.tsx:1070）都不传选项——隐患从第一个携带选项的加入方开始出现。建议在文档注释中写明「以第一个调用方为准」，并让 join 路径显式失败：至少在 await 之前执行 options?.signal?.throwIfAborted()，或者直接拒绝携带选项的加入方。
已知并发加入方都不带参数调用（slash-dispatch.ts:59、live-session.ts:563、AppContainer.tsx:1070），因此只对携带选项的加入方中止或拒绝，不会破坏本修复所服务的吞错调用方。请补充一个测试：在第一次初始化被门闩挂起时，config.initialize({ skipMcpDiscovery: true }) 必须 reject，而不是带着被丢弃的选项 resolve——移除该守卫后测试应变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (!this.initializationSettled) { | ||
| await this.initializationPromise; | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-2: The join semantics falsify the rationale comment on ensureCommandsLoaded in packages/cli/src/ui/opentui/commands-dispatch.ts (~340-350), which still claims the second initialize() call throws 'already initialized' and the catch proceeds — the mechanism this PR removes. A maintainer debugging OpenTUI startup ordering reads that comment and reasons from a mechanism that no longer exists — e.g. removing the join branch believing the bounded retry already covers the race, or building a second workaround for a race that is gone. The bounded retry itself is not dead — a failed leader still rejects joiners into the same catch — which is exactly why the comment should be rewritten to say concurrent calls now join the in-flight run, and what the retry is still for.
Witness:
witness: not run — nothing runnable discriminates a comment/code contradiction;
settled by quoting commands-dispatch.ts:344-346 against the join branch the diff
adds (config.ts:3008-3011), the closest capability (probe) having no observable
to measure
中文说明
join 语义使得 packages/cli/src/ui/opentui/commands-dispatch.ts（约 340-350 行）上 ensureCommandsLoaded 的注释失效：它仍然声称第二次 initialize() 调用会抛出 'already initialized'、catch 继续执行——而这正是本 PR 移除的机制。维护者调试 OpenTUI 启动顺序时读到该注释，会基于一个已不存在的机制推理——例如以为有界重试已覆盖该竞态而移除 join 分支，或为一个已消失的竞态再建一套补丁。有界重试本身并未失效——失败的主导初始化仍会把加入方以同样的错误拒入同一个 catch——正因如此应重写注释：说明并发调用现在会加入进行中的初始化，并说明保留该重试的原因。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| expect(firstError).toBeInstanceOf(Error); | ||
| expect(secondError).toBe(firstError); | ||
| }); |
There was a problem hiding this comment.
[Suggestion] R1-3: The documented 'throws if called again after the first call settled' contract is only pinned after a successful settle — nothing calls initialize() after a failed-and-settled first flight. A one-token mutation of the new condition — if (!this.initializationSettled) → if (!this.initializationSucceeded) — survives the added tests: in-flight joining and post-success behaviour are identical under it, but a caller arriving after a failed init awaits the stored rejection and receives the stale startup error instead of 'Config was already initialized', changing what retry/diagnostic logic observes with no test going red.
Witness:
mutant (initializationSettled → initializationSucceeded), scratch tree:
PR's own new tests: Tests 3 passed | 618 skipped
PROBE F3: post-failure settle contract (call after failed settle)
× expected [Function] to throw error including 'Config was already initialized'
but got 'startup discovery exploded'
intact PR: the same probe is green
| expect(firstError).toBeInstanceOf(Error); | |
| expect(secondError).toBe(firstError); | |
| }); | |
| expect(firstError).toBeInstanceOf(Error); | |
| expect(secondError).toBe(firstError); | |
| await expect(config.initialize()).rejects.toThrow( | |
| 'Config was already initialized', | |
| ); | |
| }); |
Settle is recorded on failure too — finally { this.initializationSettled = true; } in initialize() (packages/core/src/config/config.ts:3023-3025); the fix must not move that flag to the success-only path. The added assertion is its own witness: it goes red under the initializationSettled → initializationSucceeded mutation of the new condition.
中文说明
文档承诺的「第一次调用落定后再次调用会抛错」只在成功落定后被钉住——没有任何测试在第一次初始化失败并落定之后再调用 initialize()。对新条件做单 token 变异——if (!this.initializationSettled) → if (!this.initializationSucceeded)——新增测试依然全绿：在该变异下，飞行途中加入与成功落定后的行为完全一致，但在失败初始化之后到达的调用方会 await 已存储的 rejection，收到陈旧的启动错误而不是 'Config was already initialized'，重试/诊断逻辑观察到的行为随之改变，而没有任何测试变红。
落定在失败时同样会被记录——initialize() 中的 finally { this.initializationSettled = true; }（packages/core/src/config/config.ts:3023-3025）；修复不得把该标志移到仅成功路径。新增断言本身就是它的见证：在 initializationSettled → initializationSucceeded 变异下它会变红。
— qwen3.8-max via Qwen Code /review (v0.23.0)
| if (!this.initializationSettled) { | ||
| await this.initializationPromise; | ||
| return; | ||
| } |
There was a problem hiding this comment.
[Suggestion] R1-4: The new join path emits no debug breadcrumb — nothing logs that a second caller joined an in-flight initialization, and both in-tree joiners swallow the outcome in bare catch {} blocks. When oncall is paged on "first prompt stalled for tens of seconds at startup", the debug log cannot distinguish "initialization was slow" from "a second caller joined the flight and swallowed its outcome" — precisely the invisible state-machine interaction class behind #11002. Config already uses debugLogger for comparable transitions (Config initialization started in initializeInternal), so one breadcrumb before the await closes the gap.
Witness:
PROBE F4: all debug/info/warn calls observed during the join: []
PROBE F4: join-related breadcrumbs: []
| if (!this.initializationSettled) { | |
| await this.initializationPromise; | |
| return; | |
| } | |
| if (!this.initializationSettled) { | |
| this.debugLogger.debug( | |
| 'Config.initialize() called while initialization is in flight; joining the existing run', | |
| ); | |
| await this.initializationPromise; | |
| return; | |
| } |
中文说明
新的 join 路径没有输出任何调试面包屑——没有任何记录表明第二个调用方加入了正在进行的初始化，而树内两个加入方都用裸 catch {} 吞掉结果。当值班同学被「启动后第一个提示卡住几十秒」呼叫时，调试日志无法区分「初始化慢」与「第二个调用方加入了该初始化并吞掉了结果」——这正是 #11002 背后那类不可见的状态机交互。Config 已对同类转换使用 debugLogger（initializeInternal 中的 Config initialization started），在 await 之前加一条面包屑即可补上该缺口。
— qwen3.8-max via Qwen Code /review (v0.23.0)
Local verification — real OpenTUI sessions, A/B on stock bundlesI built both sides locally and drove the actual CLI. Headline: the failure this PR fixes is reproducible on demand, and the fix removes it. That closes the one gap the PR and the review both flagged as unverifiable ("that window is load-dependent, so it can only be observed over subsequent main runs rather than forced on demand"). Tree. PR head Runtime. macOS 26.6.2 (Darwin 25.6.0), Node 24.18.1, bun 1.3.14, How the race was made deterministic — without touching product code. 1. The CI failure reproduces on demand, and the fix removes itRepo's own leg,
The BEFORE failures are byte-identical to the CI artifact in run 33834473606: assertion Full interactive leg (CI's exclusion list,
The 2 residual failures in runs 2 and 3 are Worth noting the blast radius is wider than the two tests #11002 names: on BEFORE,
2. Mutation matrix — executed, with a counterfactual armSix mutants of the new branch, each run against (a) the shipped suite and (b) the same suite with the two new
6/6 killed by the shipped suite; 4/6 were invisible to the pre-PR suite. The PR's own mutation claim (M1) is confirmed by execution, not by hand-trace. Unmutated: 618 passed. 3. One test-quality gap: the ordering property is never assertedM3 keeps the joining branch but drops the if (this.initialized) {
if (!this.initializationSettled) {
return; // joins the flight, does not wait for it
}
throw Error('Config was already initialized');
}
That matters because M3 fully reintroduces #11002. I built an M3 bundle and drove a real session: same scenario, same env —
Suggestion (non-blocking, one line): in the first test, assert the joiner has not settled before 4. The join is unbounded and silent (observation, non-blocking)The review's point that this is not a new hang is right — the first caller already awaits the same promise. But the user-visible difference is real. Same scenario with the startup dependency held 45 s, screens captured 12 s after the prompt was submitted:
The ink fix (#11000) closes the input until initialization completes, so a user can see they cannot submit yet; the OpenTUI path accepts the keystroke and shows nothing. Worth a follow-up (echo the pending prompt, or bound the join the way Two small things while in here:
5. The red
|
| Scope | Result |
|---|---|
packages/core src/config |
8 files / 812 passed |
packages/cli src/ui/opentui |
67 files / 1132 passed |
packages/cli src/acp-integration + src/nonInteractive + src/commands/mcp |
2718 tests, 3 failed — the acpAgent structural test above, plus 2 SystemController > get_usage_info timeouts that also fail in isolation on files byte-identical to origin/main |
Verdict
Mechanism confirmed by execution rather than by reading; the fix works end-to-end in a real OpenTUI session; no regression is attributable to it, and the red CI job is inherited from main. No blocking findings. One test-quality suggestion (§3, the ordering assertion — worth taking, because the mutant it misses is the one that silently restores the bug) and one UX follow-up (§4), both non-blocking.
中文说明
本地验证 —— 真实 OpenTUI 会话，两臂均为原样打包产物
我在本地把两侧都构建出来并驱动真实 CLI 跑通。结论先说：本 PR 修复的失败可以按需复现，加上修复后消失。 这正好补上了 PR 与评审都标记为「无法验证」的那一处（"该窗口依赖负载，只能在后续 main 运行中观察，无法按需强制触发"）。
代码树：PR head 87d40742be 与 origin/main dfadc11604 合并（干净合并，9267f261f1）。两臂自同一棵树构建，只差 initialize() 那个 hunk —— 已在打包 chunk 的字节层面核对，而非只比对源码。
运行时：macOS 26.6.2（Darwin 25.6.0）、Node 24.18.1、bun 1.3.14，bun dist/cli.js + QWEN_TUI_RENDERER=opentui + QWEN_TUI_RENDERER_STRICT=1 —— 与 E2E Interactive - OpenTUI renderer (bun) 检查项同一套启动方式。
如何在不改动产品代码的前提下把竞态变成确定性事件：QWEN_CODE_LEGACY_MCP_BLOCKING=1（产品自带环境变量，config.ts:3474）会把 MCP 发现改为内联，于是 createToolRegistry 在 initializeInternal 内被 await。再在用户级 settings 里声明一个 stdio MCP server，它等待 6 秒才回应 MCP initialize 握手。这样 config.initialize() 就被产品真实执行的工作阻塞约 6 秒 —— 正是慢速、高负载 runner 会造成的情形。两臂环境变量逐字节相同、QWEN_HOME 相同、测试文件均为仓库原文件。
1. CI 失败可按需复现，修复后消失
仓库自带用例 integration-tests/interactive/mid-turn-submit-interactive.test.ts，--retry=0：
| 臂 | 慢启动探针 | 结果 | ✖︎ Chat not initialized |
|---|---|---|---|
| BEFORE（main） | 关 | 4 通过 | 0 |
| BEFORE（main） | 开 | 4 失败 | 8 |
| AFTER（#11037） | 开 | 4 通过 | 0 |
BEFORE 的失败与运行 33834473606 的 CI 痕迹逐字一致：断言 Held response never reached the screen, so the turn is not mid-stream，屏幕文本 > Start the review.✖︎ Chat not initialized。探针关闭时 BEFORE 通过 —— 这正是 PR 描述的间歇性。
完整 interactive 检查项（沿用 CI 的排除列表，--retry=0）：
| 运行 | 臂 | 探针 | Test Files | Tests | Chat not initialized |
|---|---|---|---|---|---|
| 1 | BEFORE | 开 | 4 失败 / 6 通过 / 1 跳过 | 8 失败 | 12 |
| 2 | AFTER | 开 | 1 失败 / 9 通过 / 1 跳过 | 2 失败 | 0 |
| 3 | AFTER | 关 | 1 失败 / 9 通过 / 1 跳过 | 2 失败 | 0 |
运行 2、3 里残留的 2 个失败是 context-compress-interactive.test.ts（chat_compression telemetry event was not found）。它在探针关闭时同样失败，在 BEFORE 臂关闭探针时也同样失败，属本机环境问题，与本 PR 无关。
另外，影响面比 #11002 点名的两个用例更广：BEFORE 臂上 file-system-interactive 与 protocol-tags-interactive 同样变红 —— 与失败 CI job 日志中携带 Chat not initialized 痕迹的正是同样三个文件。
2. 变异矩阵 —— 实跑，并带反事实对照臂
对新增分支做 6 个变异体，各自跑 (a) PR 提交的套件、(b) 删掉两个新 it() 后的同一套件：
| 变异体 | PR 套件 | 删掉两个新用例后的套件 |
|---|---|---|
| M1 —— 还原 hunk（即 main） | ❌ 两个新用例 | ✅ 616 通过 —— 看不见 |
M2 —— 去掉 !initializationSettled 守卫 |
❌ 新用例 #1 + 既有双重初始化用例 | ❌ 仅既有用例 |
M3 —— 加入飞行但去掉 await |
❌ 仅新用例 #2 | ✅ 看不见 |
| M4 —— 守卫取反 | ❌ 两个新用例 + 既有用例 | ❌ 仅既有用例 |
M5 —— 去掉提前 return |
❌ 仅新用例 #1 | ✅ 看不见 |
M6 —— 吞掉失败的飞行（.catch(() => {})） |
❌ 仅新用例 #2 | ✅ 看不见 |
PR 套件 6/6 全杀；其中 4/6 对 PR 之前的套件完全不可见。PR 自己的变异断言（M1）由实跑证实，而非手工推演。未变异时 618 通过。
3. 一处测试质量缺口：顺序性质从未被断言
M3 保留加入飞行的分支，但去掉 await：
if (this.initialized) {
if (!this.initializationSettled) {
return; // 加入飞行，但不等待
}
throw Error('Config was already initialized');
}makes a concurrent caller join the in-flight initialization 在 M3 下通过 —— 该用例在 await 之前就释放了门闩，也从未断言第二个调用在此之前仍处于未落定状态。只有 shares a failed in-flight initialization with concurrent callers 能杀掉它，而且是因为另一个理由（错误对象同一性）。
这一点重要，因为 M3 会完整重现 #11002。我构建了 M3 的打包产物并驱动真实会话：同样场景、同样环境 —— ✖︎ Chat not initialized，提交的 prompt 丢失。
建议（非阻塞，一行即可）：在第一个用例里断言 joiner 在 release() 之前尚未落定 —— 例如给两个 promise 各挂一个 .then() 往数组里 push，然后在释放门闩前断言数组为空。按现在的写法，那个为该行为命名的用例并没有钉住它。
4. 这次加入飞行是无界且无声的（观察项，非阻塞）
评审说「这不是新的挂起」是对的 —— 第一个调用方本来就在无界等待同一个 promise。但用户可见的差别是真实存在的。同样场景，把启动依赖挂住 45 秒，在提交 prompt 后 12 秒截图：
- BEFORE：快速失败 —— prompt 被回显，并跟一条
✖︎ Chat not initialized。 - AFTER：整个对话区一片空白。提交的 prompt 没有回显，没有转圈，也没有错误。按键被接收了，但在初始化完成前屏幕上没有任何东西确认这一点。
ink 侧的修复（#11000）是在初始化完成前保持输入关闭，用户能看出此刻不能提交；OpenTUI 路径则接收按键却什么也不显示。值得后续跟进（回显待处理的 prompt，或者像 commands-dispatch.ts 已经用 STARTUP_REGISTRY_WAIT_MS = 15_000 给自己的启动等待设上限那样，给这次加入飞行设上限）。这不构成拦下本 PR 的理由 —— 修复之前，同一个 prompt 是直接丢掉的。
顺带两点：
- 我赞成评审提的 docstring 小意见 —— 在
@param options上加一句，说明加入在飞行中的调用时该参数会被忽略。 commands-dispatch.ts:344目前仍写着「第二次initialize()调用抛出 'already initialized'，catch 继续往下走」。本 PR 让这句话不再成立，而它恰恰是 PR 描述引用作为佐证的那条注释。建议在同一个改动里一并更新。
5. 变红的 Test (ubuntu-latest, Node 22.x) 不是本 PR 造成的
运行 33903459396 的第 1 次尝试只失败了两个用例，都与 config.ts 无关：
src/acp-integration/acpAgent.test.ts > QwenAgent runtime-root pinning choke point。main 上既有：origin/main的acpAgent.ts在4595与9200两处都写了runWithAcpRuntimeOutputDir，而该用例只允许第一处。我在acpAgent.ts与acpAgent.test.ts与origin/main逐字节相同的树上本地复现了它。packages/web-shell client/components/MessageList.dom.test.tsx > drops the anchor instead of re-expanding…。在与origin/main逐字节相同的文件上本地通过（163/163），属抖动。
同一个 job 里，packages/core src/config/config.test.ts（618 个用例）通过 —— 也就是说两个新用例在 CI 里确实报了绿，这一点在评审撰写时还无法确认。
6. 回归扫描（合并树）
| 范围 | 结果 |
|---|---|
packages/core src/config |
8 个文件 / 812 通过 |
packages/cli src/ui/opentui |
67 个文件 / 1132 通过 |
packages/cli src/acp-integration + src/nonInteractive + src/commands/mcp |
2718 个用例，3 个失败 —— 上面那个 acpAgent 结构性用例，外加 2 个 SystemController > get_usage_info 超时；后者在与 origin/main 逐字节相同的文件上单独跑同样失败 |
结论
机理由实跑而非阅读确认；修复在真实 OpenTUI 会话中端到端有效；没有可归因于它的回归，变红的 CI job 是从 main 继承来的。无阻塞性发现。 一条测试质量建议（§3 的顺序断言 —— 值得采纳，因为它漏掉的那个变异体恰恰会悄悄把缺陷放回去）和一条 UX 后续项（§4），均非阻塞。
|
Qwen Code review did not complete successfully. The review pipeline failed before a review could be posted. A transient error is retried automatically; if you are seeing this, retry with |
qqqys
left a comment
qqqys
left a comment
There was a problem hiding this comment.
Reviewed at head e33d3c3e (the merge-from-main delta touches nothing this PR owns). No historical blocking issues exist on this PR — triage found no blockers at the feature commit, the four round-1 inline items are explicitly Suggestions, and the maintainer's local A/B on real OpenTUI sessions confirmed the #11002 failure is reproducible before the fix and removed by it. My independent Critical-only pass finds no blocking defect.
The join semantics, verified at this head's code (packages/core/src/config/config.ts:2999-3026):
- Ordering is race-free:
this.initialized = true, theinitializeOnce(options)call, andthis.initializationPromise = initializationall execute in one synchronous turn with no await between them, so no interleaved caller can observeinitialized === truewith the promise still undefined — every joiner awaits a promise that is guaranteed to exist. - A failed first flight is shared, not hidden:
initializationSettledflips in afinally, the joiner'sawaitsurfaces the same rejection (pinned by the new test assertingsecondError === firstError), and only after settle does the classic "Config was already initialized" throw return — the pre-settle contract is unchanged for post-settle callers. - The join branch returns without touching
initializationSucceeded, so success accounting stays owned by the first flight.
Tests: both new cases pin the actual contract — the gate/release test proves initializeInternal runs exactly once while a concurrent caller joins mid-flight, and the failure-sharing test proves the rejection identity is shared.
CI at this head: no failing or cancelled checks at review time; Test (ubuntu-latest, Node 22.x) is still pending, which does not gate this review per policy.
chiga0
left a comment
chiga0
left a comment
There was a problem hiding this comment.
No blocking findings. Approval blockers: none.
What I checked:
- Pre-existing fields:
initializationPromise,initializationSettled,initializationSucceededall in the base; the PR adds only the 8-line join guard. - Synchrony:
initialized=trueandinitializationPromise=initare assigned synchronously before any await, so no window where the concurrent caller sees a null promise. - Failure propagation: same rejection object reaches all joiners (
expect(secondError).toBe(firstError)confirms identity). ✓ - Post-settlement:
finallyfires on both success and failure, so further calls always fall tothrow 'Config was already initialized'. ✓ - Current callers:
slash-dispatch.ts:59,loadInteractiveCommands,AppContainer.tsxall pass no options — signal-ignored concern is latent. - Tests: gate mock discriminates correctly; PR mutation witness is valid.
- Sibling:
activateProvisionalWorkspaceuses same coalesce pattern. ✓
Confirmed suggestions (all non-blocking, from ci-bot R1-1 through R1-4):
R1-1: Join branch discards the joining caller's options silently. Add options?.signal?.throwIfAborted() before the await and document first-caller-wins.
R1-2: commands-dispatch.ts:~344 still reads "the second initialize() call throws 'already initialized'" — mechanism this PR removes. Worth updating.
R1-3 (mutation witness): !this.initializationSettled → !this.initializationSucceeded survives the added tests. After a failed settle initializationSucceeded stays false, so the mutant re-enters the join branch and returns the stale startup error instead of 'Config was already initialized'. Add one line after the failure test: await expect(config.initialize()).rejects.toThrow('Config was already initialized').
R1-4: No debug breadcrumb on the join path. debugLogger.debug(...) before the await closes the gap that made #11002 hard to diagnose.
Not covered: no working tree; rung 3 not applicable.
Reviewed with AI assistance.
| // throw (the OpenTUI submit path, slash-command loading) proceeded on | ||
| // a config whose chat had not started yet, and the first prompt died | ||
| // with "Chat not initialized" (#11002). | ||
| if (!this.initializationSettled) { |
There was a problem hiding this comment.
[Confirmed — Suggestion] R1-1 + R1-4: The join branch silently drops the joining caller's options. throwIfAborted is only reached on the leader path; a joiner with an already-aborted signal hangs on the foreign flight. Latent today (all concurrent callers: slash-dispatch.ts:59, loadInteractiveCommands, pass no options), but the first options-bearing joiner silently misbehaves. Suggested one-liner fix: options?.signal?.throwIfAborted() before the await. Also: no debug breadcrumb is logged when a caller joins — one debugLogger.debug(...) before the await closes the observability gap that made #11002 hard to diagnose.
| // throw (the OpenTUI submit path, slash-command loading) proceeded on | ||
| // a config whose chat had not started yet, and the first prompt died | ||
| // with "Chat not initialized" (#11002). | ||
| if (!this.initializationSettled) { |
There was a problem hiding this comment.
[Confirmed — Suggestion] R1-2: commands-dispatch.ts:~344 still describes the old throw-then-catch-proceeds mechanism removed by this PR. Worth updating to say concurrent calls now join the in-flight run.
| second.catch((error: unknown) => error), | ||
| ]); | ||
| expect(firstError).toBeInstanceOf(Error); | ||
| expect(secondError).toBe(firstError); |
There was a problem hiding this comment.
[Confirmed — Suggestion] R1-3: The mutation !this.initializationSettled → !this.initializationSucceeded survives the added tests (ci-bot mutation witness). After a failed settle, initializationSucceeded stays false, so the mutant re-enters the join branch and callers get the stale startup error instead of 'Config was already initialized'. Fix: add at the end of the failure test:
await expect(config.initialize()).rejects.toThrow('Config was already initialized');
|
Review verified. Final comment below. Post-merge review — no blockers found. The join branch in I also checked every One non-blocking note for the future: a joining caller that passes different |
|
Resolved the four round-1 review suggestions (R1-1 through R1-4) deferred on this PR in follow-up #11075:
中文本条 PR 上搁置的四条 round-1 评审建议（R1-1 ~ R1-4）已在 follow-up #11075 解决：
|
* fix(core): harden Config.initialize() join path Follow-ups to the non-blocking review suggestions left on QwenLM#11037: - a joining caller's options are ignored (first caller wins), so honor an already-aborted signal on the join path instead of blocking on the foreign flight - log a debug breadcrumb when a caller joins an in-flight initialization - pin the post-failure-settle contract: a call after a failed first flight still throws 'Config was already initialized' - update the OpenTUI commands-dispatch comment to describe the join semantics instead of the removed throw-then-catch mechanism * test(core): pin fast rejection for the aborted joining caller The test asserted the abort reason but not the ordering: `release()` ran before the assertion on `joining`, so a `throwIfAborted()` moved below `await this.initializationPromise` would still reject with the identical reason and keep the test green — the exact "aborted joiner blocks on the foreign flight" behaviour this branch removes. Assert the rejection while the gate is still held. The guard sits ahead of any `await` in `initialize()`, so the joining promise settles on the first microtask without the first flight finishing; with the guard moved after the await, the assertion never settles and the test times out instead. --------- Co-authored-by: yiliang114 <jinjing.zzj@gmail.com>
…utant fails (QwenLM#11113) The 'makes a concurrent caller join the in-flight initialization' test released the gate before asserting anything and never checked that the joining caller was still pending mid-flight. A join branch that drops the await (returning immediately) therefore passed this test while silently reproducing QwenLM#11002: the joiner proceeds before initialization completes and dies on 'Chat not initialized'. It was killed only incidentally by the failure-sharing test, for an unrelated reason. Assert nothing has settled while the first flight is still gated, so the named test actually pins the ordering property it describes. Verified: full config suite 621 passed; removing the await now fails this test with ['second'] vs []. Follow-up to QwenLM#11037 (review finding 3, not covered by QwenLM#11075).
|
Released in v0.23.1. |
6 participants
What this PR does
Config.initialize()set itsinitializedflag synchronously and only then awaited the actual initialization work, so a second caller arriving while the first call was still in flight got an immediateConfig was already initializederror instead of the initialization result. Callers that swallow that error (the OpenTUI submit path and the slash-command loader both do) then proceeded on a config whose chat had not started yet.This change makes a caller arriving mid-flight join the first flight: it awaits the in-flight promise and returns when that settles. A call after the first one has settled still throws exactly as before, so the double-init contract is unchanged once initialization is over.
Why it's needed
The
E2E Interactive - OpenTUI renderer (bun)leg reddened on run 33834473606 (issue #11002) with two named test failures ininteractive/mid-turn-submit-interactive.test.ts("exits on a bare quit token…" and "holds a slash command back…"), each failing all three attempts withHeld response never reached the screen, so the turn is not mid-stream.The job log shows why the held response never arrived: every affected session rendered the submitted prompt followed by a rejection —
The OpenTUI input prompt mounts on first paint, before configuration initialization has run anywhere (
initializeAppdoes not call it). Mounting the prompt starts command-registry loading, which is what kicks off the firstconfig.initialize()flight. A prompt submitted while that flight is still running callsconfig.initialize()again on its way to the model; today that call throws, thecatch {}swallows it, and the turn proceeds into a client whose chat does not exist yet —getChat()then throwsChat not initialized, the turn surfaces the error and the submitted prompt is lost.Whether the race is lost depends on whether initialization takes longer than the gap between the prompt appearing and the submission landing, which is why it is intermittent: the same commit passed the scheduled run 33836390526 minutes later, and the leg has been green on every completed main run since. Slow, loaded runners widen the window — in the failing run several unrelated interactive suites were stalled at high poll counts at the same time. The identical failure mode (same assertion, same
✖︎ Chat not initializedartifact) also appears in run 33829764813 tracked by #10990.The ink renderer closed its version of this race in #11000 by keeping the input closed until initialization completes; the OpenTUI path has no such gate, and its own code already documents the same in-flight hazard for the command registry (
commands-dispatch.tsstartup-window self-heal: "the second initialize() call throws 'already initialized', the catch proceeds"). Joining the flight removes the hazard at the source for every caller instead of healing each consumer separately.Reviewer Test Plan
How to verify
cd packages/core && npx vitest run src/config/config.test.ts. The two new cases pin the behaviour: a second caller issued while the first flight is held on a gate settles only when the gate releases andinitializeInternalruns exactly once; when the first flight fails, the concurrent caller receives the same error. After the first flight settles, a further call still rejects withConfig was already initialized(the pre-existing case still passes unchanged).config.tshunk alone makesmakes a concurrent caller join the in-flight initializationfail withConfig was already initialized— the exact error the OpenTUI submit path swallowed in the CI failure.Chat not initialized. That window is load-dependent, so it can only be observed over subsequent main runs rather than forced on demand.Evidence (Before & After)
Before (run 33834473606, job log PTY dumps, one per failing attempt):
After: a concurrent
initialize()caller resolves together with the first flight (new unit coverage); the settled-case throw is preserved.Tested on
Risk & Scope
Session, ACP session creation,llm.tsx, the/mcp reconnectthrowaway config) or deliberately swallows the error and proceeds (OpenTUI submit and command loading) — the latter two strictly benefit from proceeding on a settled config. Options passed by a concurrent caller are ignored; the first flight's options win, same as today when the second call's options were discarded along with the throw.Linked Issues
Fixes #11002
Related: #11000 closed the ink-side variant of this startup race; this is the OpenTUI-side variant it explicitly left open. #10990 shows the same failure mode one run earlier.
中文说明
这个 PR 做了什么
Config.initialize()会同步置位initialized标志，随后才去 await 真正的初始化工作；因此在第一次调用仍在进行时到达的第二个调用，会立刻得到Config was already initialized错误，而不是初始化的结果。吞掉该错误的调用方（OpenTUI 的提交路径与斜杠命令加载器都这么做）会继续在一个 chat 尚未启动的 config 上往前走。本改动让在飞行途中到达的调用加入第一次飞行：await 进行中的 promise，待其落定后返回。第一次调用落定之后再调用，仍然像从前一样抛错——初始化完成后的双重初始化契约不变。
为什么需要它
运行 33834473606（issue #11002）中，
E2E Interactive - OpenTUI renderer (bun)检查项以两个具名测试失败变红（interactive/mid-turn-submit-interactive.test.ts的 "exits on a bare quit token…" 与 "holds a slash command back…"），各重试三次全部失败，断言为Held response never reached the screen, so the turn is not mid-stream。job 日志给出了 held 响应为何从未到达的原因：每个受影响会话都渲染出了提交的 prompt，紧跟着是一条拒绝——
OpenTUI 的输入框在首次绘制时就挂载，而此时没有任何地方运行过配置初始化（
initializeApp不会调用它）。输入框的挂载会启动命令注册表加载，正是它发起了第一次config.initialize()飞行。在这趟飞行仍在进行时提交的 prompt，会在去往模型的路上再次调用config.initialize()；如今这次调用抛错、被catch {}吞掉，turn 继续进入一个 chat 尚不存在的 client——getChat()随后抛出Chat not initialized，turn 把错误浮出，提交的 prompt 丢失。是否输掉这个竞态，取决于初始化是否比「输入框出现到提交到达」的间隔更久，这正是它间歇性的原因：同一提交几分钟后的定时运行 33836390526 通过，此后 main 上每一次完成的运行该检查项都是绿的。慢速、高负载的 runner 会拉大窗口——失败的这次运行里，多个互不相关的交互式套件同时卡在高轮询计数上。完全相同的失败形态（同样的断言、同样的
✖︎ Chat not initialized痕迹）也出现在 #10990 追踪的运行 33829764813 中。ink 渲染器已在 #11000 中通过「初始化完成前保持输入关闭」关掉了它那一侧的同款竞态；OpenTUI 路径没有这样的门，而且它自己的代码已经记录了命令注册表上的同款在飞行途中隐患（
commands-dispatch.ts的 startup-window self-heal 注释："the second initialize() call throws 'already initialized', the catch proceeds"）。加入飞行在源头为所有调用方消除该隐患，而不是逐个修补每个消费者。审阅者测试计划
如何验证
cd packages/core && npx vitest run src/config/config.test.ts。两个新用例钉住该行为：在第一次飞行被门闩挂起时发出的第二个调用，只有在门闩释放后才落定，且initializeInternal恰好运行一次；若第一次飞行失败，并发调用方收到同一个错误。第一次飞行落定之后，再次调用仍以Config was already initialized拒绝（既有用例原样通过）。config.ts的改动块，makes a concurrent caller join the in-flight initialization即以Config was already initialized失败——正是 CI 失败里被 OpenTUI 提交路径吞掉的那个错误。Chat not initialized。该窗口依赖负载，只能在后续 main 运行中观察，无法按需强制触发。证据（改动前与改动后）
改动前（运行 33834473606，job 日志 PTY 转储，每次失败尝试各一条）：
改动后：并发的
initialize()调用与第一次飞行一同落定（新增单元覆盖）；落定后再调用的抛错行为保留。测试环境
风险与范围
Session、ACP 会话创建、llm.tsx、/mcp reconnect的一次性 config），要么刻意吞错继续（OpenTUI 提交与命令加载）——后两者在一个已落定的 config 上继续，严格受益。并发调用方传入的 options 被忽略，以第一次飞行的 options 为准；与现状相同（此前第二次调用的 options 连同抛错一起被丢弃）。关联 Issue
Fixes #11002
相关：#11000 关掉了这一启动竞态在 ink 侧的变体；本 PR 是它明确留下的 OpenTUI 侧变体。#10990 展示了早一次运行的相同失败形态。