Skip to content

Commit e94d0b4

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

2 files changed

Lines changed: 38 additions & 22 deletions

File tree

‎ARCHITECTURE.md‎

Lines changed: 33 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -170,24 +170,39 @@ absent, add a version if present" is not naturally atomic: a plain
170170
read-then-write of that pattern races, and two concurrent callers can both
171171
observe NOT FOUND and both create, leaving two artifacts with the same
172172
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.
173+
and rejected — not just for now, but structurally: `createArtifact` is a
174+
public, unconditional insert with no title lookup of its own, called
175+
directly by the import route, the upload path, and `artifact_link_file`.
176+
Two independent creates sharing a title is normal, intended behavior on
177+
every one of those paths — a coworker uploading `report.pdf` twice, or two
178+
agents each linking a file named `notes.md`, are not bugs. A hard
179+
`UNIQUE(tenant_id, title, kind)` constraint would reject those ordinary
180+
inserts outright, not just gate on a one-time backfill of legacy duplicates.
181+
Uniqueness on that triple is a property of the *find-or-version pattern
182+
specifically*, not an invariant of the table, so it does not belong in the
183+
schema — it belongs exactly where it now lives, inside the one code path
184+
that promises it. (Separately, this package also has no way to confirm
185+
existing tenants are already free of duplicate `(title, kind)` rows, which
186+
would make even a scoped constraint risky to backfill — but that is not the
187+
main reason, and is not by itself decisive: see `0003_schema_invariants` for
188+
this repo's own pattern for guarding a migration against exactly that kind
189+
of bad existing data.)
190+
Instead, `findOrVersionArtifact(db, args)` (in `artifacts.ts`) closes the
191+
race with a transaction-scoped advisory lock keyed by
192+
`hashtext(tenantId, kind, title)`, in its own lock-space namespace (the
193+
two-`int4`-argument form of `pg_advisory_xact_lock`, disjoint from the
194+
single-`bigint` form `runArtifactMigrations` uses). Collision semantics:
195+
whichever concurrent caller acquires the lock first creates the artifact;
196+
every other caller for the identical `(tenantId, kind, title)` blocks, then
197+
finds the row the winner just committed and revises it. Two overlapping
198+
callers always converge on ONE artifact with two versions, never two rows —
199+
callers for a different tenant, kind, or title never contend with each
200+
other. This guarantee holds only for callers that go through
201+
`findOrVersionArtifact`; a caller that instead calls `createArtifact`
202+
directly is unconstrained by design, as above, and a caller that hand-rolls
203+
its own find-then-create against a *different* lock is not serialized
204+
against this one — the primitive closes the race for its own call path, not
205+
for every possible way to write an artifact.
191206

192207
**`upload` is never a standalone resource.** There is no `POST /uploads`; every
193208
upload eagerly mints its artifact, and the row is reachable only through

‎CHANGELOG.md‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -20,10 +20,11 @@ always called out under their own heading.
2020
concurrent callers for the same triple always converge on one artifact —
2121
the first to acquire the lock creates it, every other caller revises the
2222
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
23+
`(tenant_id, title, kind)` was considered instead but rejected:
24+
`createArtifact` is a public, unconditional insert used directly by the
25+
import route, uploads, and `artifact_link_file`, and a shared title across
26+
independent creates on those paths is normal, not a bug a schema
27+
constraint should forbid. See the "Find-or-version" section of
2728
ARCHITECTURE.md.
2829

2930
### Changed

0 commit comments

Comments
 (0)