Skip to content

Commit c0d1bc7

Browse files
committed
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
1 parent 81049ed commit c0d1bc7

6 files changed

Lines changed: 365 additions & 54 deletions

File tree

‎ARCHITECTURE.md‎

Lines changed: 27 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -108,7 +108,7 @@ which store is installed.
108108
| File | Role |
109109
| --- | --- |
110110
| `mount.ts` | HTTP surface: parsing, validation, status codes. |
111-
| `artifacts.ts` | The core domain — create, revise, list, get, archive, serialize. |
111+
| `artifacts.ts` | The core domain — create, revise, find-or-version, list, get, archive, serialize. |
112112
| `uploads.ts` | `createFileArtifact`, the MIME policies, and the size caps. |
113113
| `download.ts` | One download path over the three storage conventions. |
114114
| `content-store.ts` | The two shipped `ContentStore` implementations. |
@@ -163,6 +163,32 @@ version fail loudly rather than corrupt history.
163163
**Archival is a soft hide.** `archived_at` null means visible; a timestamp means
164164
hidden from discovery. Deep links to archived artifacts still load.
165165

166+
**Find-or-version is a package primitive, not a constraint.** The only
167+
uniqueness the schema enforces is `(artifact_id, version)` — there is no
168+
constraint on `(tenant_id, title, kind)`, so "find by title, create if
169+
absent, add a version if present" is not naturally atomic: a plain
170+
read-then-write of that pattern races, and two concurrent callers can both
171+
observe NOT FOUND and both create, leaving two artifacts with the same
172+
title. A uniqueness constraint on `(tenant_id, title, kind)` was considered
173+
and rejected for now — this package has no way to confirm that no tenant
174+
already holds duplicate `(title, kind)` rows created before this primitive
175+
existed, and a migration that fails partway through a production deploy over
176+
real duplicate data is a worse outage than the race it closes. Instead,
177+
`findOrVersionArtifact(db, args)` (in `artifacts.ts`) closes the race with a
178+
transaction-scoped advisory lock keyed by `hashtext(tenantId, kind, title)`,
179+
in its own lock-space namespace (the two-`int4`-argument form of
180+
`pg_advisory_xact_lock`, disjoint from the single-`bigint` form
181+
`runArtifactMigrations` uses). Collision semantics: whichever concurrent
182+
caller acquires the lock first creates the artifact; every other caller for
183+
the identical `(tenantId, kind, title)` blocks, then finds the row the
184+
winner just committed and revises it. Two overlapping callers always
185+
converge on ONE artifact with two versions, never two rows — callers for a
186+
different tenant, kind, or title never contend with each other. If the
187+
`(tenant_id, title, kind)` triple is later confirmed duplicate-free in
188+
production, a follow-up migration can still add the hard constraint; the
189+
helper's serialization would make that migration a no-op for any writer that
190+
already goes through it.
191+
166192
**`upload` is never a standalone resource.** There is no `POST /uploads`; every
167193
upload eagerly mints its artifact, and the row is reachable only through
168194
`source.upload.id`. `mail_attachment_ref` carries no bytes at all — the file

‎CHANGELOG.md‎

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,23 @@ always called out under their own heading.
99

1010
## [Unreleased]
1111

12+
### Added
13+
14+
- `findOrVersionArtifact(db, args)` — the atomic primitive behind "find an
15+
artifact by title, create it if absent, add a version if present." The
16+
schema's only uniqueness is `(artifactId, version)`; nothing constrains
17+
`(tenantId, title, kind)`, so that common pattern raced between the read
18+
and the write when hand-rolled outside the package. This closes the race
19+
with a transaction-scoped advisory lock keyed on `(tenantId, kind, title)`:
20+
concurrent callers for the same triple always converge on one artifact —
21+
the first to acquire the lock creates it, every other caller revises the
22+
row the first one just committed. A uniqueness constraint on
23+
`(tenant_id, title, kind)` was considered instead but rejected for now: this
24+
package cannot verify that no existing tenant already has duplicate
25+
`(title, kind)` rows, and a migration that fails on real data is worse than
26+
the race it would close. See the "Find-or-version" section of
27+
ARCHITECTURE.md.
28+
1229
### Changed
1330

1431
- The repository root **is** the `@corbits/artifacts` package. The previous

‎README.md‎

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -249,6 +249,15 @@ legacy zoneless columns with `USING col AT TIME ZONE 'UTC'` (existing walls were
249249
always documented as UTC). Do not edit shipped migrations to roll back — ship a
250250
new reverse cast if you must.
251251

252+
`(tenant_id, title, kind)` is **not** a uniqueness constraint — only
253+
`(artifact_id, version)` is. Use `findOrVersionArtifact(db, args)` for "find
254+
by title, create if absent, add a version if present": it takes a
255+
transaction-scoped advisory lock so concurrent callers for the same
256+
`(tenantId, kind, title)` converge on one artifact instead of racing into
257+
two. See ARCHITECTURE.md's "Find-or-version" note for the collision
258+
semantics and why this is a package primitive rather than a schema
259+
constraint.
260+
252261
## Working on it
253262

254263
```sh

‎src/artifacts.test.ts‎

Lines changed: 116 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
ArtifactSizeError,
66
createArtifact,
77
findArtifactByTitle,
8+
findOrVersionArtifact,
89
getArtifactVersion,
910
listArtifactVersions,
1011
MAX_ARTIFACT_CONTENT_BYTES,
@@ -324,6 +325,121 @@ describe("find by title", () => {
324325
});
325326
});
326327

328+
describe("find-or-version", () => {
329+
test("creates when no match exists", async () => {
330+
const db = await testDb();
331+
const result = await findOrVersionArtifact(db, {
332+
scope: SCOPE,
333+
ownerPrincipalId: SCOPE.principalId,
334+
kind: "document",
335+
title: "Report",
336+
content: "v1",
337+
source: { origin: "agent" },
338+
});
339+
340+
expect(result.outcome).toBe("created");
341+
expect(result.artifact.version).toBe(1);
342+
expect(result.artifact.content).toBe("v1");
343+
});
344+
345+
test("revises the existing match instead of creating a second artifact", async () => {
346+
const db = await testDb();
347+
const seeded = await seedArtifact(db, { title: "Report", content: "v1" });
348+
349+
const result = await findOrVersionArtifact(db, {
350+
scope: SCOPE,
351+
ownerPrincipalId: SCOPE.principalId,
352+
kind: "document",
353+
title: "Report",
354+
content: "v2",
355+
source: { origin: "agent" },
356+
});
357+
358+
expect(result.outcome).toBe("revised");
359+
expect(result.artifact.id).toBe(seeded.id);
360+
expect(result.artifact.version).toBe(2);
361+
expect(result.artifact.content).toBe("v2");
362+
363+
const rows = await db.select().from(artifact).where(eq(artifact.tenantId, "acme"));
364+
expect(rows.length).toBe(1);
365+
});
366+
367+
test("a different kind with the same title creates a separate artifact", async () => {
368+
const db = await testDb();
369+
await seedArtifact(db, { title: "Report", kind: "document" });
370+
371+
const result = await findOrVersionArtifact(db, {
372+
scope: SCOPE,
373+
ownerPrincipalId: SCOPE.principalId,
374+
kind: "csv-export",
375+
title: "Report",
376+
content: "csv body",
377+
source: { origin: "agent" },
378+
});
379+
380+
expect(result.outcome).toBe("created");
381+
const rows = await db.select().from(artifact).where(eq(artifact.title, "Report"));
382+
expect(rows.length).toBe(2);
383+
});
384+
385+
test("an archived match does not get silently revived — a fresh artifact is created", async () => {
386+
const db = await testDb();
387+
const archived = await seedArtifact(db, { title: "Report" });
388+
await setArtifactArchived(db, archived, true);
389+
390+
const result = await findOrVersionArtifact(db, {
391+
scope: SCOPE,
392+
ownerPrincipalId: SCOPE.principalId,
393+
kind: "document",
394+
title: "Report",
395+
content: "fresh",
396+
source: { origin: "agent" },
397+
});
398+
399+
expect(result.outcome).toBe("created");
400+
expect(result.artifact.id).not.toBe(archived.id);
401+
});
402+
403+
// The race the ticket describes: two callers both see "no match" under a
404+
// plain read-then-write, and both create. The advisory lock this helper
405+
// takes must serialize them instead, so the second caller's lookup runs
406+
// AFTER the first caller's write is committed and finds it.
407+
test("concurrent calls for the same (tenant, kind, title) converge on one artifact", async () => {
408+
const db = await testDb();
409+
410+
const [first, second] = await Promise.all([
411+
findOrVersionArtifact(db, {
412+
scope: SCOPE,
413+
ownerPrincipalId: SCOPE.principalId,
414+
kind: "document",
415+
title: "Report",
416+
content: "from first",
417+
source: { origin: "agent" },
418+
}),
419+
findOrVersionArtifact(db, {
420+
scope: SCOPE,
421+
ownerPrincipalId: SCOPE.principalId,
422+
kind: "document",
423+
title: "Report",
424+
content: "from second",
425+
source: { origin: "agent" },
426+
}),
427+
]);
428+
429+
expect(first.artifact.id).toBe(second.artifact.id);
430+
expect([first.outcome, second.outcome].sort()).toEqual(["created", "revised"]);
431+
expect([first.artifact.version, second.artifact.version].sort()).toEqual([1, 2]);
432+
433+
const rows = await db.select().from(artifact).where(eq(artifact.tenantId, "acme"));
434+
expect(rows.length).toBe(1);
435+
const versions = await db
436+
.select()
437+
.from(artifactVersion)
438+
.where(eq(artifactVersion.artifactId, first.artifact.id));
439+
expect(versions.length).toBe(2);
440+
});
441+
});
442+
327443
describe("serialization", () => {
328444
test("a null source reads as an unknown origin", () => {
329445
expect(normalizeSource(null)).toEqual({ origin: "unknown" });

0 commit comments

Comments
 (0)