From 3f18188c6b4867104c8a20755922d52e85b711d4 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 2 Aug 2026 09:56:12 -0700 Subject: [PATCH 1/3] Add findOrVersionArtifact to close the tenant+title+kind race The schema's only uniqueness is (artifactId, version); nothing constrains (tenantId, title, kind), so "find by title, create if absent, add a version if present" raced between the read and the write whenever a consumer hand-rolled it. findOrVersionArtifact closes that race inside the package with a transaction-scoped advisory lock keyed on (tenantId, kind, title), in its own lock-space namespace so it never collides with the migration runner's lock. Collision semantics: the caller that acquires the lock first creates the artifact; every other concurrent caller for the same triple blocks, then finds and revises the row the first caller just committed. Concurrent callers always converge on one artifact, never two. A uniqueness constraint on (tenant_id, title, kind) was considered and rejected for now: there is no way to confirm existing tenants are free of duplicate (title, kind) rows, and a migration that fails on real data is worse than the race it would close. Documented in ARCHITECTURE.md. CL-5013 --- ARCHITECTURE.md | 28 ++++- CHANGELOG.md | 17 +++ README.md | 9 ++ src/artifacts.test.ts | 116 ++++++++++++++++++++ src/artifacts.ts | 246 +++++++++++++++++++++++++++++++++--------- src/index.ts | 3 + 6 files changed, 365 insertions(+), 54 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 430485c..8cf9ee2 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -154,7 +154,7 @@ which store is installed. | File | Role | | --- | --- | | `mount.ts` | HTTP surface: parsing, validation, status codes; reads `TenantEnv` principal; wires host `requireGrant`. | -| `artifacts.ts` | The core domain — create, revise, list, get, archive, serialize. | +| `artifacts.ts` | The core domain — create, revise, find-or-version, list, get, archive, serialize. | | `uploads.ts` | `createFileArtifact`, the MIME policies, and the size caps. | | `download.ts` | One download path over the three storage conventions. | | `content-store.ts` | The two shipped `ContentStore` implementations. | @@ -209,6 +209,32 @@ version fail loudly rather than corrupt history. **Archival is a soft hide.** `archived_at` null means visible; a timestamp means hidden from discovery. Deep links to archived artifacts still load. +**Find-or-version is a package primitive, not a constraint.** The only +uniqueness the schema enforces is `(artifact_id, version)` — there is no +constraint on `(tenant_id, title, kind)`, so "find by title, create if +absent, add a version if present" is not naturally atomic: a plain +read-then-write of that pattern races, and two concurrent callers can both +observe NOT FOUND and both create, leaving two artifacts with the same +title. A uniqueness constraint on `(tenant_id, title, kind)` was considered +and rejected for now — this package has no way to confirm that no tenant +already holds duplicate `(title, kind)` rows created before this primitive +existed, and a migration that fails partway through a production deploy over +real duplicate data is a worse outage than the race it closes. Instead, +`findOrVersionArtifact(db, args)` (in `artifacts.ts`) closes the race with a +transaction-scoped advisory lock keyed by `hashtext(tenantId, kind, title)`, +in its own lock-space namespace (the two-`int4`-argument form of +`pg_advisory_xact_lock`, disjoint from the single-`bigint` form +`runArtifactMigrations` uses). Collision semantics: whichever concurrent +caller acquires the lock first creates the artifact; every other caller for +the identical `(tenantId, kind, title)` blocks, then finds the row the +winner just committed and revises it. Two overlapping callers always +converge on ONE artifact with two versions, never two rows — callers for a +different tenant, kind, or title never contend with each other. If the +`(tenant_id, title, kind)` triple is later confirmed duplicate-free in +production, a follow-up migration can still add the hard constraint; the +helper's serialization would make that migration a no-op for any writer that +already goes through it. + **`upload` is never a standalone resource.** There is no `POST /uploads`; every upload eagerly mints its artifact, and the row is reachable only through `source.upload.id`. `mail_attachment_ref` carries no bytes at all — the file diff --git a/CHANGELOG.md b/CHANGELOG.md index 3cb7048..069b708 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,6 +9,23 @@ always called out under their own heading. ## [Unreleased] +### Added + +- `findOrVersionArtifact(db, args)` — the atomic primitive behind "find an + artifact by title, create it if absent, add a version if present." The + schema's only uniqueness is `(artifactId, version)`; nothing constrains + `(tenantId, title, kind)`, so that common pattern raced between the read + and the write when hand-rolled outside the package. This closes the race + with a transaction-scoped advisory lock keyed on `(tenantId, kind, title)`: + concurrent callers for the same triple always converge on one artifact — + the first to acquire the lock creates it, every other caller revises the + row the first one just committed. A uniqueness constraint on + `(tenant_id, title, kind)` was considered instead but rejected for now: this + package cannot verify that no existing tenant already has duplicate + `(title, kind)` rows, and a migration that fails on real data is worse than + the race it would close. See the "Find-or-version" section of + ARCHITECTURE.md. + ### Changed - The repository root **is** the `@corbits/artifacts` package. The previous diff --git a/README.md b/README.md index 41e6dde..15f7c08 100644 --- a/README.md +++ b/README.md @@ -262,6 +262,15 @@ legacy zoneless columns with `USING col AT TIME ZONE 'UTC'` (existing walls were always documented as UTC). Do not edit shipped migrations to roll back — ship a new reverse cast if you must. +`(tenant_id, title, kind)` is **not** a uniqueness constraint — only +`(artifact_id, version)` is. Use `findOrVersionArtifact(db, args)` for "find +by title, create if absent, add a version if present": it takes a +transaction-scoped advisory lock so concurrent callers for the same +`(tenantId, kind, title)` converge on one artifact instead of racing into +two. See ARCHITECTURE.md's "Find-or-version" note for the collision +semantics and why this is a package primitive rather than a schema +constraint. + ## Working on it ```sh diff --git a/src/artifacts.test.ts b/src/artifacts.test.ts index 53501bb..5e3dc42 100644 --- a/src/artifacts.test.ts +++ b/src/artifacts.test.ts @@ -5,6 +5,7 @@ import { ArtifactSizeError, createArtifact, findArtifactByTitle, + findOrVersionArtifact, getArtifactVersion, listArtifactVersions, MAX_ARTIFACT_CONTENT_BYTES, @@ -324,6 +325,121 @@ describe("find by title", () => { }); }); +describe("find-or-version", () => { + test("creates when no match exists", async () => { + const db = await testDb(); + const result = await findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "document", + title: "Report", + content: "v1", + source: { origin: "agent" }, + }); + + expect(result.outcome).toBe("created"); + expect(result.artifact.version).toBe(1); + expect(result.artifact.content).toBe("v1"); + }); + + test("revises the existing match instead of creating a second artifact", async () => { + const db = await testDb(); + const seeded = await seedArtifact(db, { title: "Report", content: "v1" }); + + const result = await findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "document", + title: "Report", + content: "v2", + source: { origin: "agent" }, + }); + + expect(result.outcome).toBe("revised"); + expect(result.artifact.id).toBe(seeded.id); + expect(result.artifact.version).toBe(2); + expect(result.artifact.content).toBe("v2"); + + const rows = await db.select().from(artifact).where(eq(artifact.tenantId, "acme")); + expect(rows.length).toBe(1); + }); + + test("a different kind with the same title creates a separate artifact", async () => { + const db = await testDb(); + await seedArtifact(db, { title: "Report", kind: "document" }); + + const result = await findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "csv-export", + title: "Report", + content: "csv body", + source: { origin: "agent" }, + }); + + expect(result.outcome).toBe("created"); + const rows = await db.select().from(artifact).where(eq(artifact.title, "Report")); + expect(rows.length).toBe(2); + }); + + test("an archived match does not get silently revived — a fresh artifact is created", async () => { + const db = await testDb(); + const archived = await seedArtifact(db, { title: "Report" }); + await setArtifactArchived(db, archived, true); + + const result = await findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "document", + title: "Report", + content: "fresh", + source: { origin: "agent" }, + }); + + expect(result.outcome).toBe("created"); + expect(result.artifact.id).not.toBe(archived.id); + }); + + // The race the ticket describes: two callers both see "no match" under a + // plain read-then-write, and both create. The advisory lock this helper + // takes must serialize them instead, so the second caller's lookup runs + // AFTER the first caller's write is committed and finds it. + test("concurrent calls for the same (tenant, kind, title) converge on one artifact", async () => { + const db = await testDb(); + + const [first, second] = await Promise.all([ + findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "document", + title: "Report", + content: "from first", + source: { origin: "agent" }, + }), + findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "document", + title: "Report", + content: "from second", + source: { origin: "agent" }, + }), + ]); + + expect(first.artifact.id).toBe(second.artifact.id); + expect([first.outcome, second.outcome].sort()).toEqual(["created", "revised"]); + expect([first.artifact.version, second.artifact.version].sort()).toEqual([1, 2]); + + const rows = await db.select().from(artifact).where(eq(artifact.tenantId, "acme")); + expect(rows.length).toBe(1); + const versions = await db + .select() + .from(artifactVersion) + .where(eq(artifactVersion.artifactId, first.artifact.id)); + expect(versions.length).toBe(2); + }); +}); + describe("serialization", () => { test("a null source reads as an unknown origin", () => { expect(normalizeSource(null)).toEqual({ origin: "unknown" }); diff --git a/src/artifacts.ts b/src/artifacts.ts index b1c409e..5802b13 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -236,14 +236,79 @@ export class ArtifactNotFoundError extends Error { } /** - * Revise an artifact: bump `version`, append a history row. The artifact row is - * locked `FOR UPDATE` so concurrent writers serialize instead of both computing - * the same next version; the (artifactId, version) unique index is the second - * half of that guard. + * The lock-and-write core of a revision, sharing a caller-supplied `tx` so it + * composes with a lock already held on that transaction (see + * `findOrVersionArtifact`) instead of opening a second one. The artifact row + * is locked `FOR UPDATE` so concurrent writers on the same id serialize + * instead of both computing the same next version; the (artifactId, version) + * unique index is the second half of that guard. * * Archived and skill-draft artifacts present as NOT FOUND — an agent holding a * stale id must not silently revise something the user put away. */ +async function reviseArtifactVersion( + tx: ArtifactTx, + args: { + scope: ResolvedPrincipal; + artifactId: string; + title?: string; + content?: string; + }, + now: Date, +): Promise { + const [existing] = await tx + .select() + .from(artifact) + .where( + and( + eq(artifact.id, args.artifactId), + eq(artifact.tenantId, args.scope.tenantId), + ), + ) + .for("update") + .limit(1); + + if ( + !existing || + existing.archivedAt !== null || + existing.kind === SKILL_DRAFT_KIND + ) { + throw new ArtifactNotFoundError(args.artifactId); + } + + const version = existing.version + 1; + const title = args.title ?? existing.title; + const content = + args.content === undefined + ? existing.content + : normalizeContentForKind(existing.kind, args.content); + if (args.content !== undefined) { + assertArtifactFieldSizes({ content }); + } + + const [updated] = await tx + .update(artifact) + .set({ title, content, version, updatedAt: now }) + .where(eq(artifact.id, args.artifactId)) + .returning(); + if (!updated) throw new ArtifactNotFoundError(args.artifactId); + + await tx.insert(artifactVersion).values({ + artifactId: args.artifactId, + version, + title, + content, + authorId: args.scope.principalId, + createdAt: now, + }); + + return updated; +} + +/** + * Revise an artifact: bump `version`, append a history row. See + * {@link reviseArtifactVersion} for the locking behavior. + */ export async function writeArtifactVersion( db: ArtifactDb, args: { @@ -262,51 +327,8 @@ export async function writeArtifactVersion( const now = new Date(); return await db.transaction(async (tx) => { - const [existing] = await tx - .select() - .from(artifact) - .where( - and( - eq(artifact.id, args.artifactId), - eq(artifact.tenantId, args.scope.tenantId), - ), - ) - .for("update") - .limit(1); - - if ( - !existing || - existing.archivedAt !== null || - existing.kind === SKILL_DRAFT_KIND - ) { - throw new ArtifactNotFoundError(args.artifactId); - } - - const version = existing.version + 1; - const title = args.title ?? existing.title; - const content = - args.content === undefined - ? existing.content - : normalizeContentForKind(existing.kind, args.content); - if (args.content !== undefined) { - assertArtifactFieldSizes({ content }); - } - - await tx - .update(artifact) - .set({ title, content, version, updatedAt: now }) - .where(eq(artifact.id, args.artifactId)); - - await tx.insert(artifactVersion).values({ - artifactId: args.artifactId, - version, - title, - content, - authorId: args.scope.principalId, - createdAt: now, - }); - - return { artifactId: args.artifactId, version, title }; + const row = await reviseArtifactVersion(tx, args, now); + return { artifactId: row.id, version: row.version, title: row.title }; }); } @@ -605,9 +627,14 @@ export async function listArtifacts( return { rows, nextCursor: `${last.cursorAt}__${last.id}` }; } -/** Most recently updated visible artifact with this exact title, or null. */ -export async function findArtifactByTitle( - db: ArtifactDb, +/** + * Shared by `findArtifactByTitle` and `findOrVersionArtifact` — the latter + * runs it against a transaction that already holds the find-or-version + * advisory lock, so it takes `ArtifactDb | ArtifactTx` rather than forcing a + * second, unlocked read. + */ +async function selectArtifactByTitle( + queryable: ArtifactDb | ArtifactTx, tenantId: string, title: string, kind?: string, @@ -621,7 +648,7 @@ export async function findArtifactByTitle( ]; if (kind !== undefined) conditions.push(eq(artifact.kind, kind)); - const [row] = await db + const [row] = await queryable .select({ id: artifact.id, version: artifact.version }) .from(artifact) .where(and(...conditions)) @@ -630,6 +657,119 @@ export async function findArtifactByTitle( return row ? { artifactId: row.id, version: row.version } : null; } +/** Most recently updated visible artifact with this exact title, or null. */ +export async function findArtifactByTitle( + db: ArtifactDb, + tenantId: string, + title: string, + kind?: string, +): Promise<{ artifactId: string; version: number } | null> { + return selectArtifactByTitle(db, tenantId, title, kind); +} + +/** + * Postgres advisory locks taken with the two-`int4`-argument form use a + * lock space that never collides with the single-`bigint`-argument form + * `runArtifactMigrations` uses (see `migrations.ts`'s `LOCK_KEY`) — Postgres + * guarantees the two spaces are disjoint. This namespace is therefore + * `findOrVersionArtifact`'s alone; it must never change (a live change would + * let a deployed writer stop serializing against an in-flight one). + */ +const FIND_OR_VERSION_LOCK_NAMESPACE = 0x0a27_1f05; + +export type FindOrVersionArtifactArgs = { + scope: ResolvedPrincipal; + /** The human who owns a newly created artifact; null for agents with no owning member. Ignored on the revise path — the existing artifact keeps its owner. */ + ownerPrincipalId: string | null; + kind: string; + title: string; + content: string; + /** Ignored on the revise path — only a fresh artifact's provenance. */ + source: Record; +}; + +export type FindOrVersionArtifactResult = { + artifact: ArtifactRow; + /** Whether this call minted a new artifact or appended a version to one that already existed. */ + outcome: "created" | "revised"; +}; + +/** + * The atomic primitive behind "find an artifact by title, create it if + * absent, add a version if present." The schema's only uniqueness is + * (artifactId, version) — nothing constrains (tenantId, title, kind) — so a + * plain read-then-write of that pattern races: two callers can both see NOT + * FOUND and both create, leaving two artifacts with the same title. This + * closes that race INSIDE the package instead of leaving every consumer to + * hand-roll its own locking (as at least one already had to, with a database + * advisory lock wrapping the same lookup-and-write). + * + * The whole lookup-then-write runs in one transaction, serialized on a + * transaction-scoped advisory lock keyed by `(tenantId, kind, title)` (via + * `hashtext`, in the namespace above) — so two concurrent calls for the same + * triple never both pass the "does it exist" check before either writes. + * + * Collision semantics: the caller that acquires the lock first creates the + * artifact; every other concurrent caller for the same `(tenantId, kind, + * title)` blocks on the lock, then — once it can proceed — finds the row the + * first caller just committed and revises it instead of creating a second + * one. Two overlapping callers therefore always converge on ONE artifact: + * the first call's content becomes version 1, and the second's becomes + * version 2 (in whichever order the lock grants), never two rows with the + * same title. A caller for a *different* tenant, kind, or title is never + * blocked by this lock — the key is scoped to the exact triple. + * + * Archived and skill-draft artifacts are invisible to the lookup, same as + * `findArtifactByTitle`: an archived match does not get silently revived, and + * a skill-draft is never adopted as the target of a public write. Both cases + * create a fresh artifact instead. + */ +export async function findOrVersionArtifact( + db: ArtifactDb, + args: FindOrVersionArtifactArgs, +): Promise { + return await db.transaction(async (tx) => { + // Unit-separator-joined so ("ab", "c", "d") cannot hash the same as + // ("a", "bc", "d"). A collision would only cost an unrelated writer a + // needless wait, never an incorrect result -- the query below re-checks + // by real column equality -- but there is no reason to invite one. + const lockKey = `${args.scope.tenantId}${args.kind}${args.title}`; + await tx.execute(sql` + SELECT pg_advisory_xact_lock(${FIND_OR_VERSION_LOCK_NAMESPACE}, hashtext(${lockKey})) + `); + + const existing = await selectArtifactByTitle( + tx, + args.scope.tenantId, + args.title, + args.kind, + ); + + if (existing) { + const row = await reviseArtifactVersion( + tx, + { + scope: args.scope, + artifactId: existing.artifactId, + content: args.content, + }, + new Date(), + ); + return { artifact: row, outcome: "revised" }; + } + + const row = await createArtifact(tx, { + scope: args.scope, + ownerPrincipalId: args.ownerPrincipalId, + kind: args.kind, + title: args.title, + content: args.content, + source: args.source, + }); + return { artifact: row, outcome: "created" }; + }); +} + /** * Run the display-only provenance decorator over serialized rows. One call so * no surface can serialize a row and forget the decorator. Accepts list items diff --git a/src/index.ts b/src/index.ts index a9c46a9..0297aed 100644 --- a/src/index.ts +++ b/src/index.ts @@ -40,6 +40,7 @@ export { createArtifact, DEFAULT_LIST_LIMIT, findArtifactByTitle, + findOrVersionArtifact, getArtifact, getArtifactVersion, listArtifacts, @@ -59,6 +60,8 @@ export type { ArtifactListRow, ArtifactVersionListItem, CreateArtifactArgs, + FindOrVersionArtifactArgs, + FindOrVersionArtifactResult, ListArtifactsFilters, ListArtifactVersionsFilters, SerializedArtifact, From c7cf3164370ef850b790b7947f8d4d59ec5cb4b2 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 2 Aug 2026 10:02:40 -0700 Subject: [PATCH 2/3] Sharpen the find-or-version-vs-constraint rationale The prior wording leaned on "can't verify existing data" as if this were a one-time backfill problem. It isn't: createArtifact is a public, unconditional insert used directly by the import route, uploads, and artifact_link_file, none of which dedupe by title. Two independent creates sharing a title is normal on every one of those paths, so a hard UNIQUE(tenant_id, title, kind) constraint would reject ordinary inserts, not just gate a legacy cleanup. Uniqueness on that triple is a property of the find-or-version pattern, not an invariant of the table. CL-5013 --- ARCHITECTURE.md | 51 ++++++++++++++++++++++++++++++++----------------- CHANGELOG.md | 9 +++++---- 2 files changed, 38 insertions(+), 22 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 8cf9ee2..0d0222b 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -216,24 +216,39 @@ absent, add a version if present" is not naturally atomic: a plain read-then-write of that pattern races, and two concurrent callers can both observe NOT FOUND and both create, leaving two artifacts with the same title. A uniqueness constraint on `(tenant_id, title, kind)` was considered -and rejected for now — this package has no way to confirm that no tenant -already holds duplicate `(title, kind)` rows created before this primitive -existed, and a migration that fails partway through a production deploy over -real duplicate data is a worse outage than the race it closes. Instead, -`findOrVersionArtifact(db, args)` (in `artifacts.ts`) closes the race with a -transaction-scoped advisory lock keyed by `hashtext(tenantId, kind, title)`, -in its own lock-space namespace (the two-`int4`-argument form of -`pg_advisory_xact_lock`, disjoint from the single-`bigint` form -`runArtifactMigrations` uses). Collision semantics: whichever concurrent -caller acquires the lock first creates the artifact; every other caller for -the identical `(tenantId, kind, title)` blocks, then finds the row the -winner just committed and revises it. Two overlapping callers always -converge on ONE artifact with two versions, never two rows — callers for a -different tenant, kind, or title never contend with each other. If the -`(tenant_id, title, kind)` triple is later confirmed duplicate-free in -production, a follow-up migration can still add the hard constraint; the -helper's serialization would make that migration a no-op for any writer that -already goes through it. +and rejected — not just for now, but structurally: `createArtifact` is a +public, unconditional insert with no title lookup of its own, called +directly by the import route, the upload path, and `artifact_link_file`. +Two independent creates sharing a title is normal, intended behavior on +every one of those paths — a coworker uploading `report.pdf` twice, or two +agents each linking a file named `notes.md`, are not bugs. A hard +`UNIQUE(tenant_id, title, kind)` constraint would reject those ordinary +inserts outright, not just gate on a one-time backfill of legacy duplicates. +Uniqueness on that triple is a property of the *find-or-version pattern +specifically*, not an invariant of the table, so it does not belong in the +schema — it belongs exactly where it now lives, inside the one code path +that promises it. (Separately, this package also has no way to confirm +existing tenants are already free of duplicate `(title, kind)` rows, which +would make even a scoped constraint risky to backfill — but that is not the +main reason, and is not by itself decisive: see `0003_schema_invariants` for +this repo's own pattern for guarding a migration against exactly that kind +of bad existing data.) +Instead, `findOrVersionArtifact(db, args)` (in `artifacts.ts`) closes the +race with a transaction-scoped advisory lock keyed by +`hashtext(tenantId, kind, title)`, in its own lock-space namespace (the +two-`int4`-argument form of `pg_advisory_xact_lock`, disjoint from the +single-`bigint` form `runArtifactMigrations` uses). Collision semantics: +whichever concurrent caller acquires the lock first creates the artifact; +every other caller for the identical `(tenantId, kind, title)` blocks, then +finds the row the winner just committed and revises it. Two overlapping +callers always converge on ONE artifact with two versions, never two rows — +callers for a different tenant, kind, or title never contend with each +other. This guarantee holds only for callers that go through +`findOrVersionArtifact`; a caller that instead calls `createArtifact` +directly is unconstrained by design, as above, and a caller that hand-rolls +its own find-then-create against a *different* lock is not serialized +against this one — the primitive closes the race for its own call path, not +for every possible way to write an artifact. **`upload` is never a standalone resource.** There is no `POST /uploads`; every upload eagerly mints its artifact, and the row is reachable only through diff --git a/CHANGELOG.md b/CHANGELOG.md index 069b708..a24a363 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -20,10 +20,11 @@ always called out under their own heading. concurrent callers for the same triple always converge on one artifact — the first to acquire the lock creates it, every other caller revises the row the first one just committed. A uniqueness constraint on - `(tenant_id, title, kind)` was considered instead but rejected for now: this - package cannot verify that no existing tenant already has duplicate - `(title, kind)` rows, and a migration that fails on real data is worse than - the race it would close. See the "Find-or-version" section of + `(tenant_id, title, kind)` was considered instead but rejected: + `createArtifact` is a public, unconditional insert used directly by the + import route, uploads, and `artifact_link_file`, and a shared title across + independent creates on those paths is normal, not a bug a schema + constraint should forbid. See the "Find-or-version" section of ARCHITECTURE.md. ### Changed From 4e8e1c06b7af0c2821b64e403eae8968e9a250b3 Mon Sep 17 00:00:00 2001 From: Sawyer Cutler Date: Sun, 2 Aug 2026 10:28:43 -0700 Subject: [PATCH 3/3] Address review: point createArtifact at the helper, split find-or-version note, add N-caller test - createArtifact's doc comment now says it intentionally skips the title lookup, names its three legitimate duplicate-title callers, and points to findOrVersionArtifact for callers that want convergence instead. - Splits the find-or-version ARCHITECTURE.md paragraph's nested parenthetical aside into plain sentences; content unchanged. - Adds a five-caller concurrency test alongside the existing two-caller one, asserting they all converge on one artifact with five versions. --- ARCHITECTURE.md | 29 +++++++++++++++++------------ src/artifacts.test.ts | 38 ++++++++++++++++++++++++++++++++++++++ src/artifacts.ts | 9 +++++++++ 3 files changed, 64 insertions(+), 12 deletions(-) diff --git a/ARCHITECTURE.md b/ARCHITECTURE.md index 0d0222b..1b477cb 100644 --- a/ARCHITECTURE.md +++ b/ARCHITECTURE.md @@ -215,24 +215,29 @@ constraint on `(tenant_id, title, kind)`, so "find by title, create if absent, add a version if present" is not naturally atomic: a plain read-then-write of that pattern races, and two concurrent callers can both observe NOT FOUND and both create, leaving two artifacts with the same -title. A uniqueness constraint on `(tenant_id, title, kind)` was considered -and rejected — not just for now, but structurally: `createArtifact` is a +title. + +A uniqueness constraint on `(tenant_id, title, kind)` was considered and +rejected — not just for now, but structurally. `createArtifact` is a public, unconditional insert with no title lookup of its own, called directly by the import route, the upload path, and `artifact_link_file`. Two independent creates sharing a title is normal, intended behavior on every one of those paths — a coworker uploading `report.pdf` twice, or two agents each linking a file named `notes.md`, are not bugs. A hard `UNIQUE(tenant_id, title, kind)` constraint would reject those ordinary -inserts outright, not just gate on a one-time backfill of legacy duplicates. -Uniqueness on that triple is a property of the *find-or-version pattern -specifically*, not an invariant of the table, so it does not belong in the -schema — it belongs exactly where it now lives, inside the one code path -that promises it. (Separately, this package also has no way to confirm -existing tenants are already free of duplicate `(title, kind)` rows, which -would make even a scoped constraint risky to backfill — but that is not the -main reason, and is not by itself decisive: see `0003_schema_invariants` for -this repo's own pattern for guarding a migration against exactly that kind -of bad existing data.) +inserts outright, not just gate on a one-time backfill of legacy +duplicates. Uniqueness on that triple is a property of the *find-or-version +pattern specifically*, not an invariant of the table, so it does not belong +in the schema — it belongs exactly where it now lives, inside the one code +path that promises it. + +Separately, this package also has no way to confirm existing tenants are +already free of duplicate `(title, kind)` rows, which would make even a +scoped constraint risky to backfill. That is not the main reason for +rejecting the constraint, and it is not by itself decisive. See +`0003_schema_invariants` for this repo's own pattern for guarding a +migration against exactly that kind of bad existing data. + Instead, `findOrVersionArtifact(db, args)` (in `artifacts.ts`) closes the race with a transaction-scoped advisory lock keyed by `hashtext(tenantId, kind, title)`, in its own lock-space namespace (the diff --git a/src/artifacts.test.ts b/src/artifacts.test.ts index 5e3dc42..4d92b69 100644 --- a/src/artifacts.test.ts +++ b/src/artifacts.test.ts @@ -438,6 +438,44 @@ describe("find-or-version", () => { .where(eq(artifactVersion.artifactId, first.artifact.id)); expect(versions.length).toBe(2); }); + + // Beyond the two-caller case above: each blocked caller is meant to drain + // sequentially off the lock and re-read after the previous commit, no + // matter how many are queued up. Five is enough to prove that generalizes + // without slowing CI down. + test("five concurrent calls for the same (tenant, kind, title) converge on one artifact", async () => { + const db = await testDb(); + const callerCount = 5; + + const results = await Promise.all( + Array.from({ length: callerCount }, (_, i) => + findOrVersionArtifact(db, { + scope: SCOPE, + ownerPrincipalId: SCOPE.principalId, + kind: "document", + title: "Report", + content: `from caller ${i}`, + source: { origin: "agent" }, + }), + ), + ); + + const artifactIds = new Set(results.map((r) => r.artifact.id)); + expect(artifactIds.size).toBe(1); + expect(results.filter((r) => r.outcome === "created").length).toBe(1); + expect(results.filter((r) => r.outcome === "revised").length).toBe(callerCount - 1); + expect(results.map((r) => r.artifact.version).sort((a, b) => a - b)).toEqual([ + 1, 2, 3, 4, 5, + ]); + + const rows = await db.select().from(artifact).where(eq(artifact.tenantId, "acme")); + expect(rows.length).toBe(1); + const versions = await db + .select() + .from(artifactVersion) + .where(eq(artifactVersion.artifactId, results[0]!.artifact.id)); + expect(versions.length).toBe(callerCount); + }); }); describe("serialization", () => { diff --git a/src/artifacts.ts b/src/artifacts.ts index 5802b13..fe9cf20 100644 --- a/src/artifacts.ts +++ b/src/artifacts.ts @@ -187,6 +187,15 @@ export type CreateArtifactArgs = { * Create an artifact AND its version 1 in one transaction. Version 1 is * eager, never lazy: a pinned read of version 1 must resolve for every * artifact, including one that is never revised. + * + * Deliberately does no by-title lookup, so it never dedupes against an + * existing artifact of the same `(tenantId, kind, title)` — correct for its + * three current callers, which each mean "make a new one" regardless of what + * already has this title: the `POST /artifacts` route (`mount.ts`), the + * `artifact_link_file` tool (`linkFileArtifact` in `tools.ts`), and file + * uploads (`createFileArtifact` in `uploads.ts`). A caller that instead wants + * "find by title, or create if absent" — converging on one artifact instead + * of letting duplicates pile up — should use {@link findOrVersionArtifact}. */ export async function createArtifact( tx: ArtifactTx,