Skip to content

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

Merged
kuyazee merged 1 commit into
mainfrom
task/atomic-meta-write
Aug 12, 2026
Merged

fix: make a meta write safe against a second writer (T2.1.13)#44
kuyazee merged 1 commit into
mainfrom
task/atomic-meta-write

Conversation

@kuyazee

@kuyazee kuyazee commented Aug 11, 2026

Copy link
Copy Markdown
Collaborator

What the item was

T2.1.13. storage/local.js wrote an object with a bare fs.writeFile, and every meta write was a
full-record rewrite. Two PATCHes to one slug that overlapped interleaved their bytes: the shorter
write landed inside the longer one, meta.json stopped parsing, readMeta swallowed the parse
error and answered null. The artifact then vanished from GET /api/artifacts, answered 404 on
/a/<slug>, and refused its own DELETE with a 404. The only way back was deleting the directory by
hand.

Reproduced red before touching anything, five rounds of two overlapping PATCHes on origin/main:

round 1 tags=[raced] project=[]      <- one field lost
round 2 tags=[] project=[]           <- meta.json unparseable, artifact gone from the list
round 3 tags=[] project=[]
round 4 tags=[] project=[]
round 5 tags=[] project=[]
serve: 404
delete: 404

Same five rounds on this branch: tags=[raced] project=[Race] every round, serve 200, delete 200.

What changed

The object write lands whole. storage/local.js writes a scratch file beside the target and
renames it into place. Rename inside one filesystem swaps the whole object, so a reader gets either
every old byte or every new one. The scratch file is created wx at mode 0600, then takes the
target's existing mode before the rename: a rename brings a new inode, and without that step the
chmod 600 auth.json in docs/deploy.md came undone on the next ordinary write, because a managed
key's lastUsedAt goes through the same path. s3 (one PUT) and postgres/sqlite (one upsert) were
already whole-object writes and are documented as such. The git backend reuses the local store.

Meta writes merge instead of replacing. server.js gained withMetaChain, a per-slug write
queue in the shape of the one lib/auth.js uses for auth.json. Publish, replace, zip deploy,
duplicate, patch and delete all re-read the record inside it, so a write changes what the backend
holds rather than what the request found when it arrived. A rename and a copy write under two names,
so they hold both through withMetaChains, sorted and de-duplicated: two renames that cross
(a to b while b to a) would otherwise take the two queues in opposite orders and wait on
each other forever.

A slug from a JSON body is settled to a string first. SLUG_RE.test(123) coerced on the way
through, so 123 and "123" named one directory and keyed two independent queues, which put both
writers back to believing they were alone. null still means the caller left it out.

Scratch files never travel. A killed process leaves one behind holding the whole record it was
writing, which for meta.json includes the view-password hash. They are swept at startup (root and
one level down, where auth.json and every meta.json live), skipped by copySlug so a duplicate
cannot inherit the source's password hash, and filtered out of the git backend's statusMatrix so
one is never committed and pushed.

Tests

npm test 49 to 55. Six new tests in test/storage-local.test.js cover the torn write, the
leftover scratch file, the failed write, and the file mode.

bash .github/workflows/smoke.sh 154 to 162 ok-lines, all green against the local backend. New
cases: PATCH+PATCH, PUT+PATCH, DELETE+PATCH, a rename racing a publish for one destination, two
renames that cross, and a null slug. Every backgrounded call carries --max-time 15, so a queue
that stopped settling fails the job instead of hanging it.

Browser pass on the local dashboard at 1280x900: created the admin, opened a row menu, moved
release-notes into the Acme project through one PATCH. The row moved, the group count went to 2,
the docs tag survived, the timestamp updated. Console reported 0 errors and 0 warnings.

Review

Four lenses (adversarial twice, security, QA) on a 381-line diff, 27 items. Two were regressions
this change introduced and both are fixed:

  • A delete racing a patch, 17 rounds in 20. The delete removed the namespace between the
    patch's scratch write and its rename, which answered 500, or recreated meta.json under an
    emptied directory and left a listed row serving 404. origin/main scored 0 in 20; the extra
    syscall opened it. Putting deleteArtifact on the same queue closes it: 0 in 20, and the smoke
    suite now covers it.
  • process.umask() called per write. Its no-argument form is umask(0) then umask(old), so
    the mask is 0 for a moment and any directory created in that window comes out world-writable.
    6000 concurrent publishes produced four artifact directories at 0777, and meta.json lives in
    one of those. Read once at module load now. 1200 concurrent publishes: 0755 every time.

Fixed inline, all pre-existing or introduced above:

  • {"slug": null} published an artifact literally named null and 409'd every publish after it.
    null means the caller left it out again, which is how a JS client writes
    { slug: form.slug || null }.
  • A rename held only the old slug, so a publish could claim the destination between the collision
    check and the move: 19 rounds in 20 answered 500 on origin/main, and when the move won instead
    it left a private artifact's bytes under the publish's public record. Now 200/409 or 409/201,
    never both, over 15 rounds.
  • A zip deploy and an inline publish naming one slug both answered 201, 20 rounds in 20, and the
    loser's bytes stayed on disk with nothing serving them. Now exactly one winner over 15 rounds.
  • duplicateArtifact did the same read-then-write on its 409 guard.
  • copySlug and the git backend carried scratch files, so a duplicate could inherit the source's
    password hash and a push could put one in history that outlives the artifact.
  • fs.chmod was passed the file-type bits from stat().mode, and fs.stat().catch(() => null)
    swallowed a permission error and downgraded a hardened file to 0644.
  • The rename target's slug pattern was checked after the queue started, so a 400 waited on I/O and
    a non-slug briefly became a queue key.
  • The unit test for the failed-write cleanup passed for the wrong reason: handing put a number
    throws in argument validation before a file exists, so the cleanup branch never ran. It now makes
    the rename fail instead.
  • Prose: one em dash in an edited comment in storage/sqlstore.js, one run-on in the new
    docs/deploy.md paragraph, and a comment on deleteArtifact that described the wrong symptom.

Filed rather than fixed:

  • T2.1.17: a storage call that never answers wedges every later write to that slug. The queue
    has no ceiling and no timeout, and storage/s3.js passes no AbortSignal on any fetch, so a
    stalled endpoint parks every later write on that slug for the life of the process. Before the
    queue a hung write cost one request. This needs two calls only Z can make, how long a storage
    call may take and whether the answer is a 503, so it is a ticket rather than a guess.

Recorded, no action:

  • No fsync on the file or its parent directory before the rename, so a power cut can still leave
    a zero-length meta.json on ext4 or xfs. Pre-existing (a bare writeFile never synced either);
    the item asked for atomic, not durable. storage/index.js now says so plainly rather than
    implying more.
  • The slug check moved ahead of the content and type checks on POST and PUT, so a request that is
    wrong in two ways reads a different 400 message. Both are 400 and nothing asserts either string.
  • PUT /api/artifacts/AB answers 400 for a path segment that is not a slug while PATCH and
    DELETE answer 404 for the same thing. Pre-existing inconsistency, out of scope here.
  • {"slug": false} answered 201 with a generated slug and now answers 400. docs/api.md documents
    slug as a string, and nothing in the CLI, the MCP tools or the dashboard can send a boolean.
    Reasoning in AGENT-DECISIONS.md, 2026-08-12.

Docs

docs/deploy.md gained a section on two writes to one artifact, next to the one that already
explains the same shape for auth.json, including what a fleet still cannot rely on.

No screenshot artifact: the diff changes no UI, so the browser pass above is a regression check
rather than something to look at.

Two overlapping PATCHes to one slug lost a field on the first round and left an
unparseable meta.json from the second, which took the artifact out of the list,
404'd /a/<slug> and made it refuse its own DELETE.

Two halves:

storage/local.js writes a scratch file and renames it into place, so a reader
sees either every old byte or every new one. It carries the target's existing
mode over, because a rename brings a new inode and would have handed auth.json
back at 0644 after an operator ran the chmod 600 in docs/deploy.md. s3, postgres
and sqlite were already whole-object writes; the git backend reuses local.

server.js gained withMetaChain, a per-slug write queue shaped like the one
lib/auth.js uses for auth.json. Publish, replace, zip deploy, duplicate, patch
and delete all re-read the record inside it, so a write changes what the backend
holds rather than what the request found on arrival. A rename and a copy hold
both names through withMetaChains, sorted so two renames that cross cannot wait
on each other.

A slug from a JSON body is settled to a string first: 123 and "123" named one
directory and keyed two queues. null still means the caller left it out.

npm test 49 to 55, smoke 154 to 162 ok-lines.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant