fix(uploads): unique write_atomic staging name to end routes_uploads flake (LC-208) #274
Loading…
Reference in a new issue
No description provided.
Delete branch "fix/lc-208-write-atomic-unique-staging"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
Investigated the long-standing
routes_uploadsflake (named in CLAUDE.md). Reproduced, diagnosed, fixed, validated against the reproduction recipe.Phase 1 - reproduction
The flake is intra-binary, not cross-binary. Running the single
routes_uploadsbinary in isolation:--test-threads=8, isolated binary, x40--test-threads=1(serialized), x40So concurrency within one binary is the trigger; no other binaries needed. The CLAUDE.md "concurrent-binary load" framing was imprecise. Every captured failure was a 500 Internal Server Error where 200 was expected (a handler-side error), including
tiny_pngtests - not a test-side assertion mismatch.Phase 2 - root cause
The tests share a process-global upload dir (
db::DATA_DIRis aOnceLockset once per process) and reuse identical fixture bytes, so concurrent uploads of the same bytes derive the same content-addressed final path. The dedup check inpost_upload(metadata(final).is_err()) is TOCTOU: both writers see "absent" and both callwrite_atomic(final).write_atomicstaged to a fixed{final}.partial, so both wrote the same staging file and both renamed it; whichever renamed second hit ENOENT after the first consumed the partial, and the upload returned 500. (Thetiny_pngfailures rule out themedium_pngremove_filepath - the collision is the shared original write.)This is a latent production race in
write_atomic(two users uploading identical bytes simultaneously would hit it - one 500s and retries), amplified by the harness. So#[serial]would be the wrong fix: it would hide a real bug behind serialization.Phase 3 - fix
The fix addresses the confirmed mechanism: a per-write unique staging name (a uuid nonce), so concurrent writers of the same final path never share a staging file. Both renames target the same identical-content final path, which is safe (rename is atomic, last writer wins with the same bytes). Fixes every
write_atomiccaller (uploads, admin regen, email attachments, bridge avatar), not just the test path.Validation (same recipe used to find it)
routes_uploadsisolated,--test-threads=8/=16uploads_write_atomic_concurrent)The regression guard drives 32 concurrent
write_atomiccalls to one path and asserts all succeed + no.partialleak. It fails deterministically (20/20) on the pre-fix code and passes post-fix, so it pins the property without depending on the flaky interleave.CLAUDE.md note updated to record the real cause (intra-binary,
write_atomicstaging collision) and that LC-208 fixed it.Scope
This is LC-208's fix, not a follow-up. The diagnosis landed on a small surgical production hardening (one function) plus one regression test, well within a single PR. No
#[serial], no fixture rework, no separate production-bug ticket needed.🤖 Generated with Claude Code
Investigated the long-standing routes_uploads flake (named in CLAUDE.md as "concurrent-binary load"). Reproduced, diagnosed, fixed, validated. Reproduction: the flake is intra-binary, not cross-binary. Running the single routes_uploads binary in isolation at --test-threads=8 fired 4/40 (~10%); serialized at --test-threads=1 it passed 40/40. So concurrency within one binary is the trigger, and the CLAUDE.md "concurrent-binary load" framing was imprecise. Root cause: every captured failure was a 500 (not a test-side assertion mismatch), including tiny_png tests, so the mechanism is the shared content-addressed original write, not the medium_png remove_file. The tests share a process-global upload dir (db::DATA_DIR is a OnceLock set once per process) and reuse identical fixture bytes, so concurrent uploads of the same bytes derive the same content-addressed final path. The dedup check in post_upload (metadata(final).is_err()) is TOCTOU: both writers see "absent" and both call write_atomic(final). write_atomic staged to a fixed {final}.partial, so both wrote the same staging file and both renamed it; whichever renamed second hit ENOENT after the first consumed the partial, and the upload returned 500. This is a latent PRODUCTION race in write_atomic (two users uploading identical bytes at the same instant would hit it; one 500s and must retry), amplified by the test harness. So #[serial] would be the wrong fix - it would hide a real bug. The fix addresses the confirmed mechanism: a per-write unique staging name (a uuid nonce) so concurrent writers of the same final path never share a staging file. Both renames target the same identical-content final path, which is safe (rename is atomic, last writer wins with the same bytes). Fixes every write_atomic caller (uploads, admin regen, email attachments, bridge avatar), not just the test path. Validation against the reproduction recipe: post-fix 0/160 at --test-threads=8 and =16 (vs 4/40 pre-fix). Added a deterministic regression guard (uploads_write_atomic_concurrent.rs) that drives 32 concurrent write_atomic calls to one path: it fails 20/20 on the pre-fix code and passes 20/20 post-fix, so it pins the property without depending on the flaky interleave. Full suite green both modes (standalone + saas, 129 binaries each, zero failures). Updated the CLAUDE.md note to record the real cause (intra-binary, write_atomic staging collision) and that LC-208 fixed it. Scope: this is LC-208's fix, not a follow-up. The diagnosis landed on a small surgical production hardening (one function, unique staging name) plus one regression test, well within a single PR. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>