From e391e24200c55f18d1a8271afacd84f6400d1f82 Mon Sep 17 00:00:00 2001 From: memosr Date: Mon, 17 Aug 2026 05:53:04 +0300 Subject: [PATCH] fix(harness): give the workflow registry's temp file a per-process name persist() wrote to a fixed `${registryPath}.tmp`, with a comment claiming it mirrors SessionManager.persist(). It does not: that one uses .tmp-${pid}-${seq}, and so do workspace-context, store-retention and record-archive. This was the only atomic writer left on a shared temp name. writeQueue serializes writers within one instance, but the default registry path is machine-wide (~/.sapiom/harness/workflows.json), so a CLI and a desktop app, or two Studio servers, run two registries over the same file. Concurrent writes then land in the same temp: one renames it away and the other fails, or worse, publishes a half-written file that load() swallows via catch { this.workflows = [] } - silent loss of connected workflows. Add a pid + uuid suffix and clean the temp up on failure. The new test fails on the old code with ENOENT on rename and passes on the new one. The existing atomicity test asserted against the fixed temp name, which would have passed regardless once the name changed, so it now matches on the prefix instead. --- .changeset/proud-ants-study.md | 5 +++ .../src/core/workflow-registry.test.ts | 36 +++++++++++++++++-- .../harness/src/core/workflow-registry.ts | 17 ++++++--- 3 files changed, 51 insertions(+), 7 deletions(-) create mode 100644 .changeset/proud-ants-study.md diff --git a/.changeset/proud-ants-study.md b/.changeset/proud-ants-study.md new file mode 100644 index 000000000..2a72c6298 --- /dev/null +++ b/.changeset/proud-ants-study.md @@ -0,0 +1,5 @@ +--- +"@sapiom/harness": patch +--- + +Give the workflow registry's atomic write a per-process temp file name, so two harness instances sharing the machine-wide workflows.json cannot publish a torn file. diff --git a/packages/harness/src/core/workflow-registry.test.ts b/packages/harness/src/core/workflow-registry.test.ts index 3609b23af..002d11e7f 100644 --- a/packages/harness/src/core/workflow-registry.test.ts +++ b/packages/harness/src/core/workflow-registry.test.ts @@ -351,9 +351,39 @@ describe("WorkflowRegistry", () => { expect(Array.isArray(parsed)).toBe(true); expect(parsed).toHaveLength(1); - // The .tmp file must NOT be left behind (rename completed). - const tmpPath = `${registryPath}.tmp`; - await expect(fs.access(tmpPath)).rejects.toThrow(); + // No temp artefact must be left behind (rename completed). The temp name + // carries a pid + uuid suffix, so match on the prefix rather than one + // fixed path — asserting a fixed name would pass even if a differently + // named temp file were orphaned. + const base = path.basename(registryPath); + const leftovers = (await fs.readdir(path.dirname(registryPath))).filter((name) => + name.startsWith(`${base}.tmp`), + ); + expect(leftovers).toEqual([]); + }); + + it("two registries sharing one path do not collide on the temp file (the path is machine-wide)", async () => { + // writeQueue only serializes writers inside one instance, but the default + // registry path is machine-wide (~/.sapiom/harness/workflows.json), so a + // CLI and a desktop app run two registries over the same file. With a + // fixed .tmp name each would write into the other's temp and rename a + // half-written file over the target, which load() then swallows as an + // empty list — silent loss of connected workflows. + const other = new WorkflowRegistry(registryPath); + await writeMarker(path.join(tmpRoot, "proj-a"), 1); + await writeMarker(path.join(tmpRoot, "proj-b"), 1); + + await Promise.all([registry.scan(tmpRoot), other.scan(tmpRoot)]); + + const parsed = JSON.parse(await fs.readFile(registryPath, "utf8")) as unknown[]; + expect(Array.isArray(parsed)).toBe(true); + expect(parsed).toHaveLength(2); + + const base = path.basename(registryPath); + const leftovers = (await fs.readdir(path.dirname(registryPath))).filter((name) => + name.startsWith(`${base}.tmp`), + ); + expect(leftovers).toEqual([]); }); it("a failed persist does not poison the queue — subsequent writes succeed", async () => { diff --git a/packages/harness/src/core/workflow-registry.ts b/packages/harness/src/core/workflow-registry.ts index 8474bd878..6bc6892a9 100644 --- a/packages/harness/src/core/workflow-registry.ts +++ b/packages/harness/src/core/workflow-registry.ts @@ -9,6 +9,7 @@ * shared app. */ +import { randomUUID } from "node:crypto"; import * as fs from "node:fs/promises"; import * as os from "node:os"; import * as path from "node:path"; @@ -186,10 +187,18 @@ export class WorkflowRegistry { // Atomic write: write to a temp file in the same directory (so rename is // same-filesystem and thus atomic on POSIX), then rename over the target. // A crash mid-write leaves the .tmp file, not a torn workflows.json. - // Mirrors the pattern used by SessionManager.persist(). - const tmpPath = `${this.registryPath}.tmp`; - await fs.writeFile(tmpPath, JSON.stringify(this.workflows, null, 2)); - await fs.rename(tmpPath, this.registryPath); + // The temp name is per-process and unique: writeQueue only serializes + // writers inside one process, and this path is machine-wide, so a second + // harness would otherwise write into the same .tmp concurrently and the + // rename would publish a torn file. Mirrors SessionManager.persist(). + const tmpPath = `${this.registryPath}.tmp-${process.pid}-${randomUUID()}`; + try { + await fs.writeFile(tmpPath, JSON.stringify(this.workflows, null, 2)); + await fs.rename(tmpPath, this.registryPath); + } catch (err) { + await fs.rm(tmpPath, { force: true }).catch(() => {}); + throw err; + } } /** Chains `run` onto the write queue so concurrent mutations never