Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 8 additions & 8 deletions packages/cli/src/ui/opentui/commands-dispatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -340,14 +340,14 @@ export class OpenTuiSlashDispatcher {
}

/**
* Startup-window self-heal: the first dispatcher can attach a registry
* built while config.initialize() was still in flight — the second
* initialize() call throws "already initialized", the catch proceeds, and
* the skill loaders run before the skill manager exists, so builtin
* commands resolve but every skill (e.g. /qc-helper) reports "Unknown
* command" until the config-ready dispatcher replaces this one. One
* bounded retry per dispatcher lifetime: wait for the skill manager, then
* reload the registry so the re-parse sees the complete list.
* Startup-window self-heal: the dispatcher can be attached with a registry
* snapshot taken before config.initialize() finished — the skill manager
* does not exist yet, so builtin commands resolve but every skill (e.g.
* /qc-helper) reports "Unknown command". A concurrent initialize() call
* now joins the in-flight run instead of throwing, so only a failed first
* flight still lands the loader in its partial-commands catch. One bounded
* retry per dispatcher lifetime: wait for the skill manager, then reload
* the registry so the re-parse sees the complete list.
*/
private async ensureCommandsLoaded(): Promise<boolean> {
if (this.startupRetryUsed || !this.services.config) {
Expand Down
37 changes: 37 additions & 0 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5157,6 +5157,37 @@ describe('Server Config (config.ts)', () => {
);
});

it('rejects a joining caller whose signal is already aborted', async () => {
const config = new Config({
...baseParams,
});

let release!: () => void;
const gate = new Promise<void>((resolve) => {
release = resolve;
});
vi.spyOn(
config as unknown as {
initializeInternal: () => Promise<void>;
},
'initializeInternal',
).mockImplementation(() => gate);

const first = config.initialize();
const controller = new AbortController();
const abortReason = new Error('joining caller already aborted');
controller.abort(abortReason);
// A joining caller cannot have its options honored, so an
// already-aborted signal fails fast instead of blocking on the first
// flight. Assert the rejection while the gate is still held: settling
// the first flight first would let a guard placed after the `await`
// reject with the same reason and pass.
const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
Comment on lines +5185 to +5187

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] R1-1: The new 'rejects a joining caller whose signal is already aborted' test pins that the joining caller rejects with the abort reason, but not that it rejects fast — release() runs before any assertion on joining, so the fail-before-joining ordering is never checked. If options?.signal?.throwIfAborted() is later moved below await this.initializationPromise, the joiner blocks on the full foreign flight before rejecting with the identical abort reason — reintroducing exactly the "aborted-signal joiner hangs on the first flight" behaviour this PR removes — yet this test still passes green, because release() has already let the first flight settle.

Witness:

Probe matrix at PR HEAD (scratch tree):
  original guard position + shipped test               → PASS
  mutant (guard moved below the await) + shipped test  → PASS  ← mutant survives
  mutant + rejection asserted before release()         → FAIL (Test timed out in 60000ms)
  original guard + rejection asserted before release() → PASS

Assert the rejection while the gate is still held — move await expect(joining).rejects.toBe(abortReason); above release();:

const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
await expect(first).resolves.toBeUndefined();

release() must stay in the test so the existing await expect(first).resolves.toBeUndefined(); still holds — dropping it instead of reordering the assertion would hang the test. If you apply this reorder, please prove it kills the mutant: move (or delete) the throwIfAborted() guard below the await and confirm this test then goes red — with the gate still held, the pre-release() rejection assertion never settles and the test times out.

中文说明

新增的 'rejects a joining caller whose signal is already aborted' 测试只钉住了「加入方会以中止原因 reject」,没有钉住「它 reject 得快」——release() 先于对 joining 的任何断言执行,因此「先失败、后加入」的顺序属性从未被检查。如果日后有人把 options?.signal?.throwIfAborted() 移到 await this.initializationPromise 之后,加入方会先阻塞在别人的完整初始化上、再以同一个 abortReason reject——恰好重新引入本 PR 要消除的「已中止信号的加入方挂在第一次初始化上」行为——而这条测试仍然通过,因为在断言 rejection 之前 release() 已让第一次初始化落定。

见证(PR HEAD 上的探针矩阵,独立 scratch 树):原守卫位置 + 现有测试 → 通过;变异体(守卫移到 await 之后)+ 现有测试 → 通过(变异体存活);变异体 + 在 release() 之前断言 rejection → 失败(60 秒超时);原守卫 + 在 release() 之前断言 rejection → 通过。

修复:在门控仍然持有时断言 rejection——把 await expect(joining).rejects.toBe(abortReason); 移到 release(); 之前:

const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
await expect(first).resolves.toBeUndefined();

前提约束:release() 必须保留,现有的 await expect(first).resolves.toBeUndefined(); 依赖它——只删 release() 而不是调整断言顺序会让测试挂起。修复见证:应用该重排后,请证明它能杀死变异体——把 throwIfAborted() 守卫移到 await 之后(或删除),确认该测试变红:门控仍被持有时,release() 之前的 rejection 断言永远不会落定,测试超时。

— qwen3.8-max via Qwen Code /review (v0.23.0)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Applied in 82d82c7 — the reorder is exactly as suggested:

const joining = config.initialize({ signal: controller.signal });
await expect(joining).rejects.toBe(abortReason);
release();
await expect(first).resolves.toBeUndefined();

release() stays, so await expect(first).resolves.toBeUndefined() still holds.

On the requested mutation witness: I did not run the mutant probe, so I am not claiming a reproduced red. The argument the reorder rests on is structural — options?.signal?.throwIfAborted() sits ahead of every await in initialize() (config.ts:3013, before await this.initializationPromise), so the joining promise settles on the first microtask and the pre-release() assertion resolves without the first flight finishing. Move the guard below that await and the assertion has nothing to settle it while the gate is held, which is the 60s timeout your matrix recorded. CI on this head is the check that the shipped ordering is green.

The comment above the assertions now says why the order matters, so the next reader does not "tidy" it back.

中文说明

已在 82d82c7 中按建议重排,release() 保留,原有的 await expect(first).resolves.toBeUndefined() 仍然成立。

关于要求的变异见证:我没有跑变异体探针,因此不声称复现了红。重排所依赖的是结构性理由——options?.signal?.throwIfAborted() 位于 initialize() 中所有 await 之前(config.ts:3013,在 await this.initializationPromise 之前),所以加入方的 promise 在第一个 microtask 就落定,release() 之前的断言无需等第一次初始化结束即可完成。把守卫移到该 await 之后,门控仍被持有时该断言就没有任何东西能让它落定,正是你矩阵里记录的 60 秒超时。当前 head 的 CI 负责验证重排后的测试是绿的。

断言上方的注释已写明顺序为什么重要,避免后来者把它「整理」回去。

await expect(first).resolves.toBeUndefined();
});

it('shares a failed in-flight initialization with concurrent callers', async () => {
const config = new Config({
...baseParams,
Expand All @@ -5177,6 +5208,12 @@ describe('Server Config (config.ts)', () => {
]);
expect(firstError).toBeInstanceOf(Error);
expect(secondError).toBe(firstError);

// A failed-and-settled first flight still flips `initializationSettled`,
// so a later call must throw rather than re-join the stale rejection.
await expect(config.initialize()).rejects.toThrow(
'Config was already initialized',
);
});

it('should skip implicit startup discovery in bare mode', async () => {
Expand Down
9 changes: 8 additions & 1 deletion packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2997,7 +2997,8 @@ export class Config {
/**
* Must only be called once, throws if called again after the first call
* settled. Callers arriving while the first call is still in flight join
* that flight instead of throwing.
* that flight instead of throwing; a joining caller's options are ignored
* — the first caller's options win.
* @param options Optional initialization options including sendSdkMcpMessage callback
*/
async initialize(options?: ConfigInitializeOptions): Promise<void> {
Expand All @@ -3010,6 +3011,12 @@ export class Config {
// a config whose chat had not started yet, and the first prompt died
// with "Chat not initialized" (#11002).
if (!this.initializationSettled) {
// A joining caller's options cannot be honored, so an already-aborted
// signal must fail fast instead of blocking on the foreign flight.
options?.signal?.throwIfAborted();
this.debugLogger.debug(
'Config.initialize() called while initialization is in flight; joining the existing run',
);
await this.initializationPromise;
return;
}
Expand Down
Loading