Add findOrVersionArtifact to close the tenant+title+kind race

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,52 @@ 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 — 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. 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
+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
 `source.upload.id`. `mail_attachment_ref` carries no bytes at all — the file

CHANGELOG.md

@@ -9,6 +9,24 @@ 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:
+  `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
 
 - The repository root **is** the `@corbits/artifacts` package. The previous

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

src/artifacts.test.ts

@@ -5,6 +5,7 @@ import {
   ArtifactSizeError,
   createArtifact,
   findArtifactByTitle,
+  findOrVersionArtifact,
   getArtifactVersion,
   listArtifactVersions,
   MAX_ARTIFACT_CONTENT_BYTES,
@@ -324,6 +325,159 @@ 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);
+  });
+
+  // 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", () => {
   test("a null source reads as an unknown origin", () => {
     expect(normalizeSource(null)).toEqual({ origin: "unknown" });