From 2c79ce45321626d76857415c4670442e7be769eb Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Wed, 5 Aug 2026 15:53:19 +0200 Subject: [PATCH] fix(coding-agent): avoid custom store lock convoys --- .../coding-agent/src/core/auth-storage.ts | 59 +++++++++++++------ .../coding-agent/src/core/models-store.ts | 17 ++---- .../coding-agent/test/auth-storage.test.ts | 36 +++++++++-- .../coding-agent/test/models-store.test.ts | 2 +- .../coding-agent/test/test-harness.test.ts | 5 +- 5 files changed, 82 insertions(+), 37 deletions(-) diff --git a/packages/coding-agent/src/core/auth-storage.ts b/packages/coding-agent/src/core/auth-storage.ts index 5433c6e7e..ba8c34361 100644 --- a/packages/coding-agent/src/core/auth-storage.ts +++ b/packages/coding-agent/src/core/auth-storage.ts @@ -22,10 +22,16 @@ type LockResult = { const AUTH_FILE_WRITE_OPTIONS = { encoding: "utf-8", mode: 0o600 } as const; +type AuthFileReload = { + controller: AbortController; + promise: Promise; + readers: number; +}; + type AuthFileReadState = { data: AuthStorageData; revision?: string; - reload?: Promise; + reload?: AuthFileReload; }; let sharedAuthFileReadState: { authPath: string; readState: AuthFileReadState } | undefined; @@ -232,16 +238,14 @@ export class AuthStorage implements CredentialStore { private constructor(storage: AuthStorageBackend, authPath?: string) { this.storage = storage; - this.readState = { data: {} }; + this.authPath = authPath; + this.readState = + authPath && sharedAuthFileReadState?.authPath === authPath ? sharedAuthFileReadState.readState : { data: {} }; if (authPath && !sharedAuthFileReadState) { - this.authPath = authPath; sharedAuthFileReadState = { authPath, readState: this.readState }; - } else if (authPath && sharedAuthFileReadState?.authPath === authPath) { - this.authPath = authPath; - this.readState = sharedAuthFileReadState.readState; } - if (this.authPath) { - const revision = getFileRevision(this.authPath); + if (authPath) { + const revision = getFileRevision(authPath); if (revision !== undefined && revision === this.readState.revision) return; } this.reload(); @@ -301,25 +305,44 @@ export class AuthStorage implements CredentialStore { }, options); } - private readLatestData(options?: AuthOperationOptions): Promise { + private async readLatestData(options?: AuthOperationOptions): Promise { options?.signal?.throwIfAborted(); if (!this.authPath) { const reload = this.reloadFromStorageAsync(options); return options?.signal ? reload : reload.catch(() => this.readState.data); } const revision = getFileRevision(this.authPath); - if (revision !== undefined && revision === this.readState.revision) { - return Promise.resolve(this.readState.data); - } - if (options?.signal) return this.reloadFromStorageAsync(options); + if (revision !== undefined && revision === this.readState.revision) return this.readState.data; if (!this.readState.reload) { - const reload = this.reloadFromStorageAsync().catch(() => this.readState.data); + const controller = new AbortController(); + const reload: AuthFileReload = { + controller, + promise: this.reloadFromStorageAsync({ signal: controller.signal }), + readers: 0, + }; this.readState.reload = reload; - void reload.then(() => { - if (this.readState.reload === reload) this.readState.reload = undefined; - }); + void reload.promise.then( + () => { + if (this.readState.reload === reload) this.readState.reload = undefined; + }, + () => { + if (this.readState.reload === reload) this.readState.reload = undefined; + }, + ); + } + + const reload = this.readState.reload; + reload.readers++; + try { + const result = raceWithAbortSignal(reload.promise, options?.signal); + return options?.signal ? await result : await result.catch(() => this.readState.data); + } finally { + reload.readers--; + if (reload.readers === 0 && this.readState.reload === reload) { + this.readState.reload = undefined; + reload.controller.abort(); + } } - return this.readState.reload; } async read(provider: string, options?: AuthOperationOptions): Promise { diff --git a/packages/coding-agent/src/core/models-store.ts b/packages/coding-agent/src/core/models-store.ts index eb497c845..8251b59b2 100644 --- a/packages/coding-agent/src/core/models-store.ts +++ b/packages/coding-agent/src/core/models-store.ts @@ -46,16 +46,15 @@ export class InMemoryCodingAgentModelsStore implements ModelsStore { export class FileModelsStore implements ModelsStore { private readonly storage: AuthStorageBackend; private readonly path: string; - private readonly readState: ModelsFileReadState | undefined; + private readonly readState: ModelsFileReadState; constructor(path: string = join(getAgentDir(), "models-store.json")) { this.path = normalizePath(path); this.storage = new FileAuthStorageBackend(this.path); + this.readState = + sharedModelsFileReadState?.path === this.path ? sharedModelsFileReadState.readState : { data: {} }; if (!sharedModelsFileReadState) { - this.readState = { data: {} }; sharedModelsFileReadState = { path: this.path, readState: this.readState }; - } else if (sharedModelsFileReadState.path === this.path) { - this.readState = sharedModelsFileReadState.readState; } } @@ -118,12 +117,6 @@ export class FileModelsStore implements ModelsStore { } async read(providerId: string, options?: ModelsStoreOperationOptions): Promise { - if (!this.readState) { - return this.storage.withLockAsync( - async (content) => ({ result: structuredClone(this.parse(content)[providerId]) }), - options, - ); - } const entry = (await this.readLatest(this.readState, options))[providerId]; options?.signal?.throwIfAborted(); return entry ? structuredClone(entry) : undefined; @@ -137,7 +130,7 @@ export class FileModelsStore implements ModelsStore { latest = current; return { result: undefined, next: JSON.stringify(current, null, 2) }; }, options); - if (this.readState && latest) this.updateReadState(this.readState, latest); + if (latest) this.updateReadState(this.readState, latest); } async delete(providerId: string, options?: ModelsStoreOperationOptions): Promise { @@ -148,6 +141,6 @@ export class FileModelsStore implements ModelsStore { latest = current; return { result: undefined, next: JSON.stringify(current, null, 2) }; }, options); - if (this.readState && latest) this.updateReadState(this.readState, latest); + if (latest) this.updateReadState(this.readState, latest); } } diff --git a/packages/coding-agent/test/auth-storage.test.ts b/packages/coding-agent/test/auth-storage.test.ts index 8736562b8..592a0b6e3 100644 --- a/packages/coding-agent/test/auth-storage.test.ts +++ b/packages/coding-agent/test/auth-storage.test.ts @@ -81,9 +81,9 @@ describe("AuthStorage", () => { }); const [anthropic, openai, credentials] = await Promise.all([ - first.read("anthropic"), - second.read("openai"), - first.list(), + first.read("anthropic", { signal: new AbortController().signal }), + second.read("openai", { signal: new AbortController().signal }), + first.list({ signal: new AbortController().signal }), ]); expect(anthropic).toEqual({ type: "api_key", key: "new" }); expect(openai).toEqual({ type: "api_key", key: "openai-key" }); @@ -103,14 +103,40 @@ describe("AuthStorage", () => { await otherFirst.read("other"); await otherSecond.read("other"); await otherFirst.list(); - expect(lockSpy).toHaveBeenCalledTimes(4); + expect(lockSpy).toHaveBeenCalledTimes(1); const third = AuthStorage.create(authJsonPath); writeAuthJson({ anthropic: { type: "api_key", key: "newest" } }); const [firstReload, thirdReload] = await Promise.all([first.read("anthropic"), third.read("anthropic")]); expect(firstReload).toEqual({ type: "api_key", key: "newest" }); expect(thirdReload).toEqual({ type: "api_key", key: "newest" }); - expect(lockSpy).toHaveBeenCalledTimes(5); + expect(lockSpy).toHaveBeenCalledTimes(2); + }); + + test("keeps a coalesced reload alive while another credential reader is waiting", async () => { + writeAuthJson({ anthropic: { type: "api_key", key: "old" } }); + const storage = AuthStorage.create(authJsonPath); + writeAuthJson({ anthropic: { type: "api_key", key: "new" } }); + let grantLock: (() => void) | undefined; + const lockGranted = new Promise((resolve) => { + grantLock = resolve; + }); + const release = vi.fn(async () => {}); + const lockSpy = vi.spyOn(lockfile, "lock").mockImplementation(async () => { + await lockGranted; + return release; + }); + const firstController = new AbortController(); + const secondController = new AbortController(); + const first = storage.read("anthropic", { signal: firstController.signal }); + const second = storage.read("anthropic", { signal: secondController.signal }); + + firstController.abort(); + await expect(first).rejects.toMatchObject({ name: "AbortError" }); + grantLock?.(); + await expect(second).resolves.toEqual({ type: "api_key", key: "new" }); + expect(lockSpy).toHaveBeenCalledTimes(1); + expect(release).toHaveBeenCalledTimes(1); }); test("modify persists a credential while preserving unrelated external edits", async () => { diff --git a/packages/coding-agent/test/models-store.test.ts b/packages/coding-agent/test/models-store.test.ts index 975309cfe..30e592f7c 100644 --- a/packages/coding-agent/test/models-store.test.ts +++ b/packages/coding-agent/test/models-store.test.ts @@ -88,7 +88,7 @@ describe("FileModelsStore", () => { const [firstReload, thirdReload] = await Promise.all([first.read("one"), third.read("one")]); expect(firstReload).toMatchObject({ models: [{ id: "newest-model" }] }); expect(thirdReload).toMatchObject({ models: [{ id: "newest-model" }] }); - expect(lockSpy).toHaveBeenCalledTimes(4); + expect(lockSpy).toHaveBeenCalledTimes(3); }); it("keeps a coalesced reload alive while another reader is still waiting", async () => { diff --git a/packages/coding-agent/test/test-harness.test.ts b/packages/coding-agent/test/test-harness.test.ts index 3ac716226..5f1ad41df 100644 --- a/packages/coding-agent/test/test-harness.test.ts +++ b/packages/coding-agent/test/test-harness.test.ts @@ -89,7 +89,10 @@ describe("test harness", () => { }); it("turns a pending terminal response into an error", async () => { - harness = await createHarness({ responses: [{ text: "partial", stopReason: "pending" }] }); + harness = await createHarness({ + responses: [{ text: "partial", stopReason: "pending" }], + settings: { retry: { enabled: false } }, + }); await harness.session.prompt("hi");