diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 059389934..3c14d2093 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -28,6 +28,7 @@ - xAI web search now uses `grok-4.5` (at low reasoning effort) instead of `grok-4.3`. ### Fixed +- Fixed a first-use race in `ArtifactManager` where two concurrent `allocatePath`/`save` callers on a fresh instance both re-seeded `#nextId` across the directory-scan yield and allocated the same artifact id, silently overwriting the first artifact (same tool type) or making `artifact://` resolution ambiguous (different tool types). The initial scan is now memoized as a single in-flight promise so all concurrent callers share one initialization and receive distinct ids ([#4091](https://github.com/can1357/oh-my-pi/issues/4091)). - Fixed discarded `Settings` instances keeping debounced save timers and chained background saves armed; discarding an instance now cancels its pending writes so they cannot race a successor's file locks. - Fixed `error.notify` raising a "Stopped with error" toast for provider failures while an auto-retry or async-delivery continuation was pending; the toast now waits for the true terminal settle. diff --git a/packages/coding-agent/src/session/artifacts.ts b/packages/coding-agent/src/session/artifacts.ts index 0cd61d9a7..a7e9b661f 100644 --- a/packages/coding-agent/src/session/artifacts.ts +++ b/packages/coding-agent/src/session/artifacts.ts @@ -39,7 +39,7 @@ export class ArtifactManager { #nextId = 0; readonly #dir: string; #dirCreated = false; - #initialized = false; + #initPromise: Promise | null = null; /** * @param dir Directory that will hold artifact files. Created lazily on first save. @@ -61,10 +61,11 @@ export class ArtifactManager { await fs.mkdir(this.#dir, { recursive: true }); this.#dirCreated = true; } - if (!this.#initialized) { - await this.#scanExistingIds(); - this.#initialized = true; - } + // Memoize the first-use scan so it runs exactly once. Concurrent callers + // share the in-flight promise instead of each re-seeding #nextId across + // the readdir yield in #scanExistingIds (which would hand duplicate ids). + this.#initPromise ??= this.#scanExistingIds(); + await this.#initPromise; } /** diff --git a/packages/coding-agent/test/artifacts-concurrency.test.ts b/packages/coding-agent/test/artifacts-concurrency.test.ts new file mode 100644 index 000000000..db776e530 --- /dev/null +++ b/packages/coding-agent/test/artifacts-concurrency.test.ts @@ -0,0 +1,72 @@ +import { afterEach, describe, expect, it } from "bun:test"; +import * as os from "node:os"; +import * as path from "node:path"; +import { ArtifactManager } from "@oh-my-pi/pi-coding-agent/session/artifacts"; +import { removeSyncWithRetries } from "@oh-my-pi/pi-utils"; + +describe("ArtifactManager concurrent first-use", () => { + const dirs: string[] = []; + + function freshDir(): string { + const dir = path.join(os.tmpdir(), `omp-artifacts-${crypto.randomUUID()}`, "session"); + dirs.push(path.dirname(dir)); + return dir; + } + + afterEach(() => { + for (const dir of dirs.splice(0)) { + removeSyncWithRetries(dir); + } + }); + + // First-use init (dir scan → #nextId seed) must run exactly once. Two callers + // racing a fresh manager both yield inside #scanExistingIds before either + // marks init done; if the second re-seeds #nextId after the first consumed an + // id, both allocate the same numeric id and the second write clobbers the + // first. Same toolType => file overwrite; the first id resolves to B's bytes. + it("hands concurrent same-toolType savers distinct ids that each resolve to their own content", async () => { + const mgr = new ArtifactManager(freshDir()); + const [idA, idB] = await Promise.all([mgr.save("CONTENT-A", "bash"), mgr.save("CONTENT-B", "bash")]); + + expect(idA).not.toBe(idB); + + const pathA = await mgr.getPath(idA); + const pathB = await mgr.getPath(idB); + expect(pathA).not.toBeNull(); + expect(pathB).not.toBeNull(); + expect(await Bun.file(pathA as string).text()).toBe("CONTENT-A"); + expect(await Bun.file(pathB as string).text()).toBe("CONTENT-B"); + }); + + // Different toolTypes turn a duplicate id into two coexisting files + // (`{id}.bash.log` + `{id}.async.log`); getPath's startsWith(`${id}.`) then + // resolves ambiguously in unspecified readdir order. Distinct ids keep each + // artifact:// pointing at the content its caller wrote. + it("hands concurrent different-toolType savers distinct ids that each resolve to their own content", async () => { + const mgr = new ArtifactManager(freshDir()); + const [idA, idB] = await Promise.all([mgr.save("BASH-BYTES", "bash"), mgr.save("ASYNC-BYTES", "async")]); + + expect(idA).not.toBe(idB); + + const pathA = await mgr.getPath(idA); + const pathB = await mgr.getPath(idB); + expect(await Bun.file(pathA as string).text()).toBe("BASH-BYTES"); + expect(await Bun.file(pathB as string).text()).toBe("ASYNC-BYTES"); + }); + + // The race also re-opens on a fresh manager over a directory that already + // holds artifacts (e.g. after a `#artifactManager = null` reset): the scan + // seeds from maxId, and concurrent callers must still get ids past it. + it("does not reuse ids when racing init over a pre-populated directory", async () => { + const dir = freshDir(); + const seed = new ArtifactManager(dir); + await seed.save("OLD", "bash"); + + const mgr = new ArtifactManager(dir); + const [idA, idB] = await Promise.all([mgr.save("NEW-A", "bash"), mgr.save("NEW-B", "bash")]); + + expect(idA).not.toBe(idB); + expect(await Bun.file((await mgr.getPath(idA)) as string).text()).toBe("NEW-A"); + expect(await Bun.file((await mgr.getPath(idB)) as string).text()).toBe("NEW-B"); + }); +});