fix: make a meta write safe against a second writer (T2.1.13)

.github/workflows/smoke.sh

@@ -426,6 +426,125 @@ if curl -s "$BASE/api/artifacts?project=Acme%20Redesign" -H "$AUTH" | grep -q '"
 echo "ok: PUT preserves / PATCH clears project"
 curl -sf -X DELETE "$BASE/api/artifacts/ci-proj" -H "$AUTH" > /dev/null
 
+# --- two overlapping PATCHes to one slug: both fields land, meta stays readable ---
+# A meta write rewrites the whole record. Before it was serialized, the second writer started
+# from a copy taken before the first one landed and put it back without that field, and on the
+# local backend the two writes interleaved often enough to leave meta.json unparseable, which
+# dropped the artifact from the list and made it 404 on its own DELETE. Five rounds: the
+# corruption reproduced in roughly 4 runs out of 10 when it was one round.
+curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+  -d '{"content":"<h1>race</h1>","type":"html","slug":"ci-race","visibility":"public"}' > /dev/null
+# --max-time on every backgrounded call: a write that never settles would otherwise hang `wait`
+# and burn the job's whole timeout instead of failing.
+for round in 1 2 3 4 5; do
+  curl -sf --max-time 15 -X PATCH "$BASE/api/artifacts/ci-race" -H "$AUTH" -H "$JSON" -d '{"tags":["raced"]}' > /dev/null &
+  tags_pid=$!
+  curl -sf --max-time 15 -X PATCH "$BASE/api/artifacts/ci-race" -H "$AUTH" -H "$JSON" -d '{"project":"Race"}' > /dev/null &
+  proj_pid=$!
+  wait "$tags_pid" || fail "concurrent tags PATCH failed on round $round"
+  wait "$proj_pid" || fail "concurrent project PATCH failed on round $round"
+  [ "$(list_field ci-race tags)" = 'raced' ] || fail "concurrent PATCH lost tags on round $round"
+  [ "$(list_field ci-race project)" = 'Race' ] || fail "concurrent PATCH lost project on round $round"
+  curl -sf -X PATCH "$BASE/api/artifacts/ci-race" -H "$AUTH" -H "$JSON" -d '{"tags":[],"project":""}' > /dev/null
+done
+echo "ok: overlapping PATCHes both land"
+
+# the artifact still answers its own routes, which a corrupt meta.json would not
+code=$(curl -s -o /dev/null -w '%{http_code}' "$BASE/a/ci-race?raw=1")
+expect_code 200 "$code" "artifact still served after overlapping PATCHes"
+
+# a replace rebuilds the record from what it read, so it has to read what the PATCH left
+for round in 1 2 3; do
+  curl -sf --max-time 15 -X PUT "$BASE/api/artifacts/ci-race" -H "$AUTH" -H "$JSON" \
+    -d '{"content":"<h1>race v2</h1>","type":"html"}' > /dev/null &
+  put_pid=$!
+  curl -sf --max-time 15 -X PATCH "$BASE/api/artifacts/ci-race" -H "$AUTH" -H "$JSON" -d '{"tags":["kept"]}' > /dev/null &
+  patch_pid=$!
+  wait "$put_pid" || fail "concurrent PUT failed on round $round"
+  wait "$patch_pid" || fail "concurrent PATCH failed on round $round"
+  [ "$(list_field ci-race tags)" = 'kept' ] || fail "a PUT overwrote a concurrent PATCH on round $round"
+  curl -sf -X PATCH "$BASE/api/artifacts/ci-race" -H "$AUTH" -H "$JSON" -d '{"tags":[]}' > /dev/null
+done
+curl -s "$BASE/a/ci-race?raw=1" | grep -q '<h1>race v2</h1>' || fail "the PUT content did not survive"
+echo "ok: a PUT and a PATCH at once both land"
+
+code=$(curl -s -o /dev/null -w '%{http_code}' -X DELETE "$BASE/api/artifacts/ci-race" -H "$AUTH")
+expect_code 200 "$code" "artifact still deletable after overlapping writes"
+
+# a DELETE beside a PATCH: whichever wins, the row and the artifact have to agree afterwards
+for round in 1 2 3; do
+  curl -sf --max-time 15 -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+    -d '{"content":"<h1>gone</h1>","type":"html","slug":"ci-race-del","visibility":"public"}' > /dev/null
+  curl -s --max-time 15 -o /dev/null -X DELETE "$BASE/api/artifacts/ci-race-del" -H "$AUTH" &
+  del_pid=$!
+  curl -s --max-time 15 -o /dev/null -X PATCH "$BASE/api/artifacts/ci-race-del" -H "$AUTH" -H "$JSON" -d '{"tags":["z"]}' &
+  patch_pid=$!
+  wait "$del_pid"
+  wait "$patch_pid"
+  listed=$(list_field ci-race-del slug)
+  served=$(curl -s -o /dev/null -w '%{http_code}' "$BASE/a/ci-race-del?raw=1")
+  if [ -n "$listed" ] && [ "$served" != "200" ]; then
+    fail "round $round left a listed artifact that does not serve ($served)"
+  fi
+  curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-race-del" -H "$AUTH"
+done
+echo "ok: a DELETE and a PATCH at once leave no ghost row"
+
+# --- a rename holds the destination too, and two renames that cross do not wait on each other ---
+# A rename writes under two names. Holding only the old one let a publish claim the destination
+# between the collision check and the move, which answered 500. Exactly one of the pair wins.
+rename_code=$(mktemp)
+publish_code=$(mktemp)
+for round in 1 2 3; do
+  curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-ren-src" -H "$AUTH"
+  curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-ren-dst" -H "$AUTH"
+  curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+    -d '{"content":"<h1>s</h1>","type":"html","slug":"ci-ren-src","visibility":"public"}' > /dev/null
+  curl -s --max-time 15 -o /dev/null -w '%{http_code}' -X PATCH "$BASE/api/artifacts/ci-ren-src" \
+    -H "$AUTH" -H "$JSON" -d '{"slug":"ci-ren-dst"}' > "$rename_code" &
+  ren_pid=$!
+  curl -s --max-time 15 -o /dev/null -w '%{http_code}' -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+    -d '{"content":"<h1>d</h1>","type":"html","slug":"ci-ren-dst","visibility":"public"}' > "$publish_code" &
+  pub_pid=$!
+  wait "$ren_pid"
+  wait "$pub_pid"
+  pair="$(cat "$rename_code")/$(cat "$publish_code")"
+  [ "$pair" = "200/409" ] || [ "$pair" = "409/201" ] || fail "rename raced a publish and answered $pair on round $round"
+done
+rm "$rename_code"
+rm "$publish_code"
+echo "ok: a rename and a publish claiming one slug leave exactly one winner"
+
+# two renames that cross. --max-time turns a lock-ordering regression into a failure instead of a
+# job that hangs until the runner's own timeout.
+curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+  -d '{"content":"<h1>x</h1>","type":"html","slug":"ci-cross-x","visibility":"public"}' > /dev/null
+curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+  -d '{"content":"<h1>y</h1>","type":"html","slug":"ci-cross-y","visibility":"public"}' > /dev/null
+curl -s --max-time 15 -o /dev/null -X PATCH "$BASE/api/artifacts/ci-cross-x" -H "$AUTH" -H "$JSON" -d '{"slug":"ci-cross-y"}' &
+x_pid=$!
+curl -s --max-time 15 -o /dev/null -X PATCH "$BASE/api/artifacts/ci-cross-y" -H "$AUTH" -H "$JSON" -d '{"slug":"ci-cross-x"}' &
+y_pid=$!
+wait "$x_pid" || fail "a crossed rename never answered (lock ordering)"
+wait "$y_pid" || fail "a crossed rename never answered (lock ordering)"
+echo "ok: two renames that cross both answer"
+curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-cross-x" -H "$AUTH"
+curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-cross-y" -H "$AUTH"
+curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-ren-src" -H "$AUTH"
+curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/ci-ren-dst" -H "$AUTH"
+
+# a slug the caller left out is still a slug the server picks, twice over
+first=$(curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+  -d '{"content":"<h1>n1</h1>","type":"html","slug":null}' | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>process.stdout.write(JSON.parse(s).slug))')
+second=$(curl -sf -X POST "$BASE/api/artifacts" -H "$AUTH" -H "$JSON" \
+  -d '{"content":"<h1>n2</h1>","type":"html","slug":null}' | node -e 'let s="";process.stdin.on("data",d=>s+=d).on("end",()=>process.stdout.write(JSON.parse(s).slug))')
+[ -n "$first" ] && [ -n "$second" ] || fail "a null slug did not publish"
+[ "$first" != "$second" ] || fail "a null slug published to one fixed name ($first)"
+[ "$first" != "null" ] || fail "a null slug became an artifact called null"
+echo "ok: a null slug means the server picks one"
+curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/$first" -H "$AUTH"
+curl -s -o /dev/null -X DELETE "$BASE/api/artifacts/$second" -H "$AUTH"
+
 # zip site: build a tiny site and deploy it
 ZIPDIR=$(mktemp -d)
 mkdir -p "$ZIPDIR/site/css"

docs/deploy.md

@@ -105,6 +105,17 @@ Capability links are unaffected by all of it. They are signed with `sessionSecre
 
 The global config behaves like the auth record, for the same reason. See [storage backends](#storage-backends).
 
+### Two writes to one artifact at the same time
+
+Artifact writes merge, the way auth writes do. A publish, replace, zip deploy, duplicate, patch or
+delete reloads that slug's `meta.json` before it writes. One instance runs the writes to a single
+slug one at a time, so two one-field `PATCH`es to one artifact both land.
+
+On a fleet the one-request window applies again. Two replicas writing to one slug inside it can
+lose a field, and a rename and a publish aiming at one destination can both pass their collision
+check, which leaves one of them serving bytes under the other's record. Neither case can leave a
+`meta.json` that does not parse: a `put` replaces the whole object in one step on every backend.
+
 ## Storage backends
 
 By default artifacts are plain files under `DATA_DIR` — back up that directory and you have

server.js

@@ -447,10 +447,19 @@ function extractSiteFiles(zip) {
   return files;
 }
 
-async function saveZipArtifact(buffer, { slug, title, expiresAt, tags, project, visibility, password }) {
-  if (slug !== undefined && !SLUG_RE.test(slug)) {
+// Chained on the target slug like every other write: the 409 below is a read followed by a
+// write, so a zip deploy and an inline publish naming one slug both used to answer 201 and the
+// loser's bytes stayed on disk with nothing serving them.
+async function saveZipArtifact(buffer, input) {
+  const wanted = wantedSlug(input.slug);
+  if (wanted !== undefined && !SLUG_RE.test(wanted)) {
     throw new ApiError(400, 'slug must match [a-z0-9][a-z0-9-]{2,63}');
   }
+  const finalSlug = wanted || nanoid();
+  return withMetaChain(finalSlug, () => storeZipArtifact(buffer, finalSlug, input));
+}
+
+async function storeZipArtifact(buffer, finalSlug, { title, expiresAt, tags, project, visibility, password }) {
   const expiry = expiresAt !== undefined ? parseExpiresAt(expiresAt) : undefined;
   const tagList = tags !== undefined ? parseTags(tags) : undefined;
   const projectName = project !== undefined ? parseProject(project) : undefined;
@@ -471,7 +480,6 @@ async function saveZipArtifact(buffer, { slug, title, expiresAt, tags, project,
   }
   const files = extractSiteFiles(zip);
 
-  const finalSlug = slug || nanoid();
   if (await readMeta(finalSlug)) {
     throw new ApiError(409, `slug "${finalSlug}" already exists`);
   }
@@ -569,7 +577,67 @@ function isExpired(meta) {
   return Boolean(meta.expiresAt && Date.parse(meta.expiresAt) <= Date.now());
 }
 
-async function saveArtifact({ content, type = 'html', slug, title, expiresAt, frame, tags, project, visibility, password }, { replace = false } = {}) {
+// Every meta write rewrites the whole record, so a write that started from a snapshot taken
+// before another write landed puts that snapshot back and the other one's field is gone. Two
+// PATCHes to one slug did it in one process without any unusual timing, and the dashboard
+// sends a one-field PATCH per control. Chain the read-modify-write per slug, the way
+// lib/auth.js chains auth.json: each one reloads meta inside the chain, so it changes what
+// the backend holds rather than what the request found when it arrived.
+//
+// This covers one process. Two replicas sharing an s3 or postgres store can still write
+// inside the same window and lose a field; what cannot happen any more is the corrupt
+// meta.json that took the artifact out of every route, because the object write itself is
+// now whole (storage/local.js) or already was (s3, postgres, sqlite).
+const metaWriteChains = new Map();
+function withMetaChain(slug, run) {
+  const prev = metaWriteChains.get(slug) || Promise.resolve();
+  const result = prev.then(run);
+  // The chain holds `tail`, which swallows the rejection, so a write that throws (a 404 on a
+  // missing slug, a 409 on a taken one) does not stop the next caller from running.
+  const tail = result.then(() => {}, () => {});
+  metaWriteChains.set(slug, tail);
+  // Drop the entry once nothing is queued behind it, or the map grows one key per slug the
+  // process ever wrote.
+  tail.then(() => {
+    if (metaWriteChains.get(slug) === tail) metaWriteChains.delete(slug);
+  });
+  return result;
+}
+
+// A rename and a copy write under two names, so they hold both chains. Sorted and de-duplicated
+// first: two renames that cross (a to b while b to a) would otherwise take the two chains in
+// opposite orders and wait on each other forever.
+function withMetaChains(slugs, run) {
+  const keys = [...new Set(slugs)].sort();
+  return keys.reduceRight((next, key) => () => withMetaChain(key, next), run)();
+}
+
+// A slug arriving in a JSON body can be a number, and SLUG_RE coerces it on the way through.
+// `123` and `"123"` name one directory and are two different chain keys, so two writers to that
+// namespace both believed they were alone. Settle it to a string before anything keys on it.
+// `null` means the caller left it out, which is how a JS client writes `slug: form.slug || null`;
+// String() would have turned that into an artifact named "null" and 409'd every publish after it.
+function wantedSlug(value) {
+  if (value === undefined || value === null) return undefined;
+  if (typeof value !== 'string' && typeof value !== 'number') {
+    throw new ApiError(400, 'slug must be a string');
+  }
+  return String(value);
+}
+
+// Publish or replace. The slug is settled first so the write can be chained on it: a replace
+// reads the stored record and writes it back carrying the new content, so a PATCH that landed
+// between those two steps used to disappear.
+async function saveArtifact(input, opts = {}) {
+  const wanted = wantedSlug(input.slug);
+  if (wanted !== undefined && !SLUG_RE.test(wanted)) {
+    throw new ApiError(400, 'slug must match [a-z0-9][a-z0-9-]{2,63}');
+  }
+  const finalSlug = wanted || nanoid();
+  return withMetaChain(finalSlug, () => storeArtifact(finalSlug, input, opts));
+}
+
+async function storeArtifact(finalSlug, { content, type = 'html', title, expiresAt, frame, tags, project, visibility, password }, { replace = false } = {}) {
   if (typeof content !== 'string' || !content.trim()) {
     throw new ApiError(400, 'content (non-empty string) is required');
   }
@@ -588,10 +656,6 @@ async function saveArtifact({ content, type = 'html', slug, title, expiresAt, fr
   if (!TYPES.includes(type)) {
     throw new ApiError(400, `type must be one of: ${TYPES.join(', ')}`);
   }
-  if (slug !== undefined && !SLUG_RE.test(slug)) {
-    throw new ApiError(400, 'slug must match [a-z0-9][a-z0-9-]{2,63}');
-  }
-  const finalSlug = slug || nanoid();
   const existing = await readMeta(finalSlug);
   if (existing && !replace) {
     throw new ApiError(409, `slug "${finalSlug}" already exists`);
@@ -683,13 +747,20 @@ async function saveArtifact({ content, type = 'html', slug, title, expiresAt, fr
 // (stored hashed), so a password-visibility copy requires a new password in the body.
 async function duplicateArtifact(sourceSlug, body = {}) {
   if (!SLUG_RE.test(sourceSlug)) throw new ApiError(404, `slug "${sourceSlug}" not found`);
-  const source = await readMeta(sourceSlug);
-  if (!source) throw new ApiError(404, `slug "${sourceSlug}" not found`);
-
-  const targetSlug = body.slug || nanoid();
+  // Validated after the fallback, the way it always was here: an empty or absent slug on a copy
+  // means "pick one", where the same value on a publish is a 400.
+  const targetSlug = wantedSlug(body.slug) || nanoid();
   if (!SLUG_RE.test(targetSlug)) {
     throw new ApiError(400, 'slug must match [a-z0-9][a-z0-9-]{2,63}');
   }
+  // Both names: the target because the 409 below is a read-then-write, and the source because
+  // copySlug walks its directory while a PATCH there may be renaming a scratch file into place.
+  return withMetaChains([sourceSlug, targetSlug], () => copyArtifact(sourceSlug, targetSlug, body));
+}
+
+async function copyArtifact(sourceSlug, targetSlug, body) {
+  const source = await readMeta(sourceSlug);
+  if (!source) throw new ApiError(404, `slug "${sourceSlug}" not found`);
   if (await readMeta(targetSlug)) {
     throw new ApiError(409, `slug "${targetSlug}" already exists`);
   }
@@ -817,23 +888,38 @@ async function listArtifacts({ tag, project } = {}) {
   return items;
 }
 
+// Chained per slug so the record this reads is the one the last write left, not the one the
+// request found when it arrived. Two one-field PATCHes to one artifact, which the dashboard
+// sends a lot of, used to end with only the second field set.
 async function patchArtifact(slug, patch) {
-  const meta = SLUG_RE.test(slug) ? await readMeta(slug) : null;
+  // Checked here rather than after the chain starts, so a caller-supplied path segment that is
+  // not a slug never becomes a chain key.
+  if (!SLUG_RE.test(slug)) throw new ApiError(404, `slug "${slug}" not found`);
+  // A rename writes under the new name too. Holding only the old one let a publish claim the
+  // destination between the collision check and the move, which answered 500 and, when the move
+  // won instead, left a private artifact's bytes under the publish's public record.
+  const newSlug = wantedSlug(patch?.slug);
+  const renaming = newSlug !== undefined && newSlug !== slug;
+  if (renaming && !SLUG_RE.test(newSlug)) {
+    throw new ApiError(400, 'slug must match [a-z0-9][a-z0-9-]{2,63}');
+  }
+  return withMetaChains(renaming ? [slug, newSlug] : [slug], () => applyPatch(slug, patch, newSlug));
+}
+
+async function applyPatch(slug, patch, newSlug) {
+  const meta = await readMeta(slug);
   if (!meta) {
     throw new ApiError(404, `slug "${slug}" not found`);
   }
 
   let activeSlug = slug;
-  if (patch.slug !== undefined && patch.slug !== slug) {
-    if (!SLUG_RE.test(patch.slug)) {
-      throw new ApiError(400, 'slug must match [a-z0-9][a-z0-9-]{2,63}');
+  if (newSlug !== undefined && newSlug !== slug) {
+    if (await readMeta(newSlug)) {
+      throw new ApiError(409, `slug "${newSlug}" already exists`);
     }
-    if (await readMeta(patch.slug)) {
-      throw new ApiError(409, `slug "${patch.slug}" already exists`);
-    }
-    await storage.move(slug, patch.slug);
-    meta.slug = patch.slug;
-    activeSlug = patch.slug;
+    await storage.move(slug, newSlug);
+    meta.slug = newSlug;
+    activeSlug = newSlug;
   }
 
   if (patch.disabled !== undefined) {
@@ -912,8 +998,18 @@ async function patchArtifact(slug, patch) {
   return { slug: meta.slug, url: tokenedUrl(meta), visibility: meta.visibility || 'public' };
 }
 
+// Chained too. A delete running beside a patch emptied the namespace between the patch's read
+// and its write, and the patch then recreated meta.json under the cleared directory. Both
+// requests answered 200 and the list kept a row whose /a/<slug> serves a 404, with no content
+// left to put back. Serialized, the delete either wins (the patch gets a clean 404) or it loses
+// (the delete removes what the patch just wrote).
 async function deleteArtifact(slug) {
-  if (!SLUG_RE.test(slug) || !(await readMeta(slug))) {
+  if (!SLUG_RE.test(slug)) throw new ApiError(404, `slug "${slug}" not found`);
+  return withMetaChain(slug, () => removeArtifact(slug));
+}
+
+async function removeArtifact(slug) {
+  if (!(await readMeta(slug))) {
     throw new ApiError(404, `slug "${slug}" not found`);
   }
   await storage.deleteSlug(slug);