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
55 changes: 55 additions & 0 deletions packages/core/src/config/config.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -5124,6 +5124,61 @@ describe('Server Config (config.ts)', () => {
);
});

it('makes a concurrent caller join the in-flight initialization', async () => {
const config = new Config({
...baseParams,
});

// Make the first flight hang until we release it, so the second call
// arrives while initialization is still running.
let release!: () => void;
const gate = new Promise<void>((resolve) => {
release = resolve;
});
const initializeInternal = vi
.spyOn(
config as unknown as {
initializeInternal: () => Promise<void>;
},
'initializeInternal',
)
.mockImplementation(() => gate);

const first = config.initialize();
// Second call lands mid-flight → joins the first flight instead of
// bouncing off the already-set flag.
const second = config.initialize();
release();
await Promise.all([first, second]);
expect(initializeInternal).toHaveBeenCalledOnce();

await expect(config.initialize()).rejects.toThrow(
'Config was already initialized',
);
});

it('shares a failed in-flight initialization with concurrent callers', async () => {
const config = new Config({
...baseParams,
});

vi.spyOn(
config as unknown as {
initializeInternal: () => Promise<void>;
},
'initializeInternal',
).mockRejectedValue(new Error('startup discovery exploded'));

const first = config.initialize();
const second = config.initialize();
const [firstError, secondError] = await Promise.all([
first.catch((error: unknown) => error),
second.catch((error: unknown) => error),
]);
expect(firstError).toBeInstanceOf(Error);
expect(secondError).toBe(firstError);

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.

[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');

});

it('should skip implicit startup discovery in bare mode', async () => {
const extensionRefreshSpy = vi
.spyOn(ExtensionManager.prototype, 'refreshCache')
Expand Down
12 changes: 11 additions & 1 deletion packages/core/src/config/config.ts
Original file line number Diff line number Diff line change
Expand Up @@ -2991,14 +2991,24 @@ export class Config {
}

/**
* Must only be called once, throws if called again.
* 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.
* @param options Optional initialization options including sendSdkMcpMessage callback
*/
async initialize(options?: ConfigInitializeOptions): Promise<void> {
if (isDerivedConfig(this)) {
throw new Error('Derived Configs cannot be initialized');
}
if (this.initialized) {
// Joining the in-flight run matters: callers that swallow the old
// 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) {

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.

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

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.

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

await this.initializationPromise;
return;
}
throw Error('Config was already initialized');
}
if (this.shutdownRequested) {
Expand Down
Loading