[matej21]

Why

The uploads plugin rewrote an upload's meta.json in place. Anything that read the file during that write parsed a partial file: a status poll, or a superseded run checking whether it was superseded.

The stored-upload route test hits exactly this. transport/http/routes/upload.test.ts › "processes a stored upload from the host blob store with the host preprocessors" polls meta.json and fails with "JSON Parse error". It fails in the full bun run test on main and passes when the file runs alone.

What

• FileStore.write(path, content, { atomic: true }):
  • writes a pending file (.<name>.pending-<uuid>) in the same directory and renames it over the target;
  • removes the pending file if the write or the rename fails.
• An atomic write also checks the target's directory against the store's roots, because the rename replaces the entry there instead of writing through a link at it.
• The write-then-rename steps are shared with FileEventStore.replaceLog (platform/replace-file.ts).
• A platform without rename (it is optional on FileSystem) writes in place. Its writeFile must then be atomic to readers. The FileSystem.rename doc says so.
• RuntimeFileStore passes the option through. WriteOptions is exported.
• The uploads plugin writes meta.json atomically.

A retry clears an interrupted run's files except meta.json and the upload itself, so a pending file a crash left behind goes with them. A superseded run whose pending file is cleared that way fails its rename instead of overwriting meta.json. If such a run renames its pending file away between the cleanup's listing and the removal, the cleanup lists the directory again and moves on; a dangling link that is still listed keeps the failure.

Tests

• file-store.test.ts › "SessionFileStore atomic write":

  • A filesystem whose writeFile lands in two halves. A read in between sees the old content, and no pending file is left afterwards.
  • A failed rename leaves no pending file.
  • A platform without rename writes in place.
  • An atomic write whose directory is outside the root is refused, even when its link points back inside.

• upload.test.ts › "never lets a status poll read a meta.json that is half written": the stored-upload flow on a filesystem whose writes land in two halves 30 ms apart, polled every 5 ms.

• uploads-retention.integration.test.ts › "clears for a re-run past a file that another writer renamed away between the listing and its removal".

Mutations, each caught:

• the uploads plugin writes meta.json without atomic (the route test fails 3/3);
• the store ignores atomic (2 fail);
• no pending-file cleanup (1 fails);
• no directory check on an atomic write (1 fails);
• the cleanup does not tolerate a vanished file (1 fails).

Local on fd57e1e:

• bun run ts:build, bun run lint and the test type checks are clean.
• bun run test: 2160 pass, 0 fail.

Status

• Review round 1 (482d8f3): changes, with 1 major, 1 minor and 2 nits. Fixed in 4b55653 and 2b95639.
• Review round 2 (2b95639): approve, with 1 minor (fixed in fd57e1e) and 1 nit (won't fix).
• CI: starts when the PR is marked ready.
• Left before merge: CI green.

🤖 Generated with Claude Code

[matej21]

Review round 1

Reviewer: Claude Opus 5.5 subagent (independent code review), one reviewer
Reviewed: 482d8f3 (565bcf7..482d8f3)
Verdict: changes

#	Severity	Finding
1	major	An atomic write can create and replace files outside the store's roots. The 'create' check follows the final link, but the write creates the pending file in the lexical parent and renames it over the link itself. Repro: root/link points to outside/, and outside/meta.json points back to root/inside.txt. write('link/meta.json', …, { atomic: true }) returns ok and replaces outside/meta.json. Fix: also check the entry's directory ('entry').
2	minor	A re-run's removeUploadFiles can now fail. A run that outlived its runtime renames its pending meta.json between the listing and the removal; remove of a missing file returns Err, and the re-run is abandoned.
3	nit	replaceFile duplicates FileEventStore.replaceLog.
4	nit	A test helper's comment says what, not why.

Checked and fine:

• A symlinked meta.json inside the root.
• A directory at the target.
• Pending-name collisions and concurrent atomic writes.
• Each platform: Node and Bun rename; Kompjutr has no rename and its synchronous writeFile is the fallback.
• Wrappers pass the option through.
• Listings and error mapping.
• Every mutation claimed in the PR is caught.
• Lint is clean.

[matej21]

Response to review round 1

1. Fixed in 4b55653. An atomic write also checks the entry's directory. New test: "refuses an atomic write whose directory is outside the root, though its link points back inside", using the repro above. It fails without the check.
2. Fixed in 2b95639. A removal that fails on a file that is no longer there counts as done. New test: "clears for a re-run past a file that another writer renamed away between the listing and its removal". It fails without the fix.
3. Fixed in 4b55653. replaceViaRename (platform/replace-file.ts) is shared by the file store and the event store's replaceLog.
4. Fixed in 4b55653. The comment now gives the why.

Verified on 2b95639:

• ts:build, lint and the test type checks are clean.
• bun run test: 2160 pass, 0 fail.

[matej21]

Review round 2

Reviewer: Claude Opus 5.5 subagent (independent code review), one reviewer
Reviewed: 2b95639 (the round 1 fixes, in the context of the whole PR)
Verdict: approve (one minor item, one nit)

All four round 1 fixes hold. Removing each fix on a clean copy makes its test fail. replaceLog behaves the same: write, rename, then unlink in a finally, in that order.

#	Severity	Finding
1	minor	removeUploadFiles decides a vanished entry with exists, which follows links. In a read-only upload directory, a dangling link whose removal failed reads as gone, and the cleanup reports success with the link still there. On main this returns Err. A regular file still reports the failure.
2	nit	Without its fix, the new retention test fails by timing out, not on an assertion.

[matej21]

Response to review round 2

1. Fixed in fd57e1e. The cleanup lists the directory again, which does not follow links. A dangling link that is still listed keeps the failure.
   • I checked that the listing includes a dangling link.
   • The retention test still fails when the fix is removed.
2. Won't fix. The timeout message names the state it waited for, and the test fails either way.

Only minor items, so no new round.

Verified on fd57e1e: ts:build and lint are clean, and uploads-retention.integration.test.ts passes 38 of 38.