Repository navigation
fix(studio-server): history copies a changing file without stalling its sweep - #5627
Conversation
terencechotai2-commits
left a comment
There was a problem hiding this comment.
Verdict at 9705d48: needs changes. The fix is right, but the PR breaks CI. The blobStore change fixes the hang, and I reproduced both the hang and the fix. The PR also carries two lines of formatter churn in projectHistory.test.ts that fail format:check. That failure takes down Preflight, and with it the two required Windows checks. Revert those two lines and I'd approve.
Blockers
- Unrelated reformat of
projectHistory.test.tsturns required CI red.- Lines 845 and 1389 change
Date.now() < until; );toDate.now() < until;);. The repo'soxfmt0.41.0 wants the original form back. Locally,npx oxfmt --check packages/studio-server/src/history/projectHistory.test.tsfails, and runningoxfmtrewrites exactly those two lines. - In CI,
FormatandPreflight (lint + format)fail with "Format issues found in … projectHistory.test.ts". Preflight gates the Windows lanes, so they are skipped (LANE_RESULT: skipped). The required checks Tests on windows-latest and Render on windows-latest then fail after 4 s. - This also matters on its own: Windows is the platform where
snapshotalways takes the stream path, because libuv has no forced clone there. So the Windows lanes are the only CI coverage of that path, and right now they never ran. - Fix: revert the two lines, or run
bun run format.
- Lines 845 and 1389 change
Lows
-
A failed
putnow leaves anincoming-*temp file inblobs/.- Cause:
createWriteStream(to)opens (creates) the destination before the read stream's open error reachespipeline.putnever removestempwhensnapshotthrows. - The common trigger is a project file deleted between the sweep's listing and its copy. That race is routine:
storeIfPresenttreats ENOENT as "removed, the next sweep records it". - I probed
openBlobStoredirectly on ext4 and on xfs:put(missing)rejects ENOENT and leaves a 0-byteincoming-<uuid>.- The same happens for EISDIR and for an unreadable (EACCES) source.
- A read error mid-stream would leave a partial file.
- At the base,
copyFileopens the source first, so the ENOENT case left nothing (checked:dst exists false). This is new on every file system, including the clone ones, because the FICLONE_FORCE attempt fails ENOENT and then falls through to the stream. - These files are never read as blobs:
openBlobStoregets ENOTDIR fromreaddiron them and skips them. But nothing counts or prunes them, so they pile up for the life of the history. Suggested fix:
try { await snapshot(absPath, temp); } catch (error) { await rm(temp, { force: true }); throw error; } - Cause:
-
The
Commentscheck is red (not required). The comment ratchet reports thatblobStore.tscomment share rose from 6.5% to 7.7% because of 2 added lines (thesnapshotdoc comment). Trimming it to one line, or folding it into the name, clears the check. -
Note: on the stream path, blobs now get mode 0644 instead of the source file's mode.
copyFilecopied the mode;createWriteStreamuses 0666 masked by umask. Probe: a 0755 source gave a 755 copy withcopyFileand 644 with the stream.writeTocopies the blob's mode onto the restored file, so on non-clone disks Undo now restores an executable file without its exec bit. Mode was already lossy, because blobs are deduped by content. Project files here are html/css/media, so I'm only noting it.
Checked and fine
- Root cause. On ext4 (and tmpfs), libuv's byte-copy loop in
fs.copyFilenever returns once the source is shortened under it. The sweep awaits that copy, and every queued history operation waits behind the sweep. PlainCOPYFILE_FICLONEdoesn't help, because it silently falls back to the same byte copy.FICLONE_FORCEeither clones atomically or throws, and the stream path stops at the real EOF. - Every path settles.
pipeline()rejects and destroys both streams on any read error, open error or write error, so there is no'finish'-style wait left. I walked these cases:- Shortened: the read returns 0, EOF, and the stream resolves.
- Grown: the stream reads to the new end and resolves. The probe stored 72 MB from a 64 MB file grown mid-copy, and the hash matched.
- Deleted or renamed: the open fd keeps reading the old inode.
- Source EACCES/EISDIR, destination EACCES: these reject with the right
code, sostoreIfPresent's ENOENT handling still works. - fd count: 23 before and 23 after all the error cases, so no leaked descriptors.
- Unchanged by this PR: a source that never stops growing would keep the stream reading, and concurrent
puts of the same content each use their own temp and rename onto the same hash path.
- Data integrity.
- The hash is still taken from the copy. A torn copy (a writer mid-rewrite) is stored under its own correct hash, never under the source's.
- The sweep records the listing stat taken before the copy, or
""when that stat is withinRACY_MS. A file changed during the copy therefore shows a new stat on the next sweep and is stored again. - On a non-clone disk, the timeline can briefly hold an intermediate version. The old byte copy had the same behaviour for in-place writes that didn't shorten the file.
- The temp file sits in
blobs/, on the same file system asblobs/xx/<hash>, so the finalrenameis atomic. Partial temps are never renamed into place, becauserenameruns only aftersnapshotresolves. cloneOrCopyandwriteToare unchanged and only read immutable blobs, so Undo writes whole blobs through temp+rename as before.
- Clone fallback. A failed
FICLONE_FORCEleaves no destination behind (libuv unlinks it), andcreateWriteStreamtruncates the file anyway. - Speed. Copying 1 GB on this ext4 box took 3.1 s with the stream and 9.3 s with
copyFile, so the stream path is no slower. - The test.
- It reproduces the hang. With the base
blobStore.tson an ext4TMPDIRit failed 2/2 with "the sweep never returned", and it passes at the head. - On xfs (
/tmp) it passes on the base too, because that disk takes the clone path, as the PR body says. Ubuntu runners use ext4, so CI exercises it. - Mutation: swapping
COPYFILE_FICLONE_FORCEforCOPYFILE_FICLONEinsnapshotmakes it fail with "the sweep never returned". Always taking the stream path passes, which is expected because that is also a valid fix.
- It reproduces the hang. With the base
Checks run
vitest run src/history/projectHistory.test.ts -t shortened:- at the head on ext4: pass;
- with the base
blobStore.tson ext4: fail 2/2 ("the sweep never returned"); - with the base on xfs: pass;
- with the FICLONE (non-force) mutation on ext4: fail;
- with the always-stream mutation: pass.
- Full
src/historyVitest suite at the head: 176 passed, 2 skipped, on both ext4 and xfs. tsc --noEmitinpackages/studio-server: clean.oxlinton both changed files: clean.oxfmt --check: fails onprojectHistory.test.ts(Blocker 1).- A scratch probe of
openBlobStore().putcovering a missing, directory and unreadable source, a grown source, a read-only store dir, and an fd count. It was deleted afterwards. - A plain-Node probe of base
copyFileENOENT leftovers, file mode, and 1 GB copy timing. gh pr checks --required: 8 pass. Tests on windows-latest and Render on windows-latest fail because their lanes were skipped after Preflight failed on format. Not required:Format,Preflight,Commentsandpreview-regressionare red, and the Studio edit-accuracy shards were still pending.
— tai2
Edit accuracy: accurate 271 (base branch 271), smooth 164 of thoseThe gate passes. Quarantined, measured but not gated (0) Unstable (1)
|
|
Thanks, tai2. All four are fixed at the new head. Blocker (format). The two Low 1 (partial temp). Low 2 (comments). The doc comment is gone, and the name says it: Low 3 (mode). Fixed rather than noted: the stream opens its copy with the source's mode, as On a Linux machine: the three copy tests pass 3 runs in a row on ext4, the |
terencechotai2-commits
left a comment
There was a problem hiding this comment.
Approving f611743c. My blocker and the lows from 9705d489 are fixed. The delta is two commits on top of 9705d489 (60132f66, f611743c), touching only blobStore.ts and projectHistory.test.ts.
- Blocker (format churn): fixed. Lines 845 and 1389 are back to
until; );, andoxfmt --checkis clean on both files. Preflight no longer stops the Windows lanes, so CI now covers the stream path there. - Partial copy left on failure: fixed. A failed
cloneOrStreamnow removesincoming-*before rethrowing. The new test "leaves no partial copy in the store" covers both a missing source (ENOENT) and an unreadable one (EACCES). Dropping the.catch(rm)fails it. - Mode on the stream path: fixed. The stream copy now creates the temp file with the source's
mode. The new test "keeps the file's mode" checks0o755. Note: on this box/tmpis xfs, so the copy always clones and keeps the mode by itself. Dropping{ mode }passed locally for that reason. The ext4 Linux CI runner takes the stream path, so the test means something there. Commentscheck: the explanatory doc comment on the copy helper was dropped and the helper renamedcloneOrStream. I'd have kept one line on whycopyFilealone can hang, but that's not blocking.
Checks run
vitest run src/historyatf611743c: 178 passed, 2 skipped.oxfmt --checkon both changed files: clean.- Mutants: dropping the cleanup
.catchwas caught. Dropping{ mode }was not caught here, because xfs clones. gh pr checks --requiredat this head: green before posting, including both Windows checks.
— tai2
History copies every changed project file into its blob store with
fs.copyFile. When the copy is a byte copy (no clone) and another writer shortens the file while it is being copied,fs.copyFilenever returns: libuv's copy loop asks for the remaining bytes past the new end of the file, gets 0 back, and loops forever. The sweep waits on that copy, and every history operation queues behind the sweep, so undo, redo and the next sweep hang with it. The stuck copy also holds a threadpool thread for good.A byte copy is what Linux ext4 does (CI runners included), and what macOS does on any volume that is not APFS. On xfs, btrfs and APFS the copy is a clone, which is one atomic snapshot, so the bug never shows there.
What changes
blobStore.putcopies a project file throughcloneOrStream: it first tries a forced clone (COPYFILE_FICLONE_FORCE), which is still one snapshot. Where the file system cannot clone, it streams the file instead, and a stream stops at the file's real end. A file shortened mid-copy is stored as whatever was read, and the next sweep sees the file's new stat and stores its real contents.copyFiledid, so Undo still restores an executable file with its exec bit.incoming-*file is left in the store.cloneOrCopyis unchanged: it copies blobs out of the store, and those never change.What I measured
fs.copyFilecopies it is still copying after 5 s on ext4 (8/8 runs, with and withoutCOPYFILE_FICLONE), on tmpfs (12/12) and on a non-clone copy on macOS (2/2). On xfs the copy clones and returns every time (14/14).src/historysuite at this head: 178 passed, 2 skipped, on ext4 and on xfs. The three copy tests pass 3 runs in a row on ext4.tsc --noEmit, oxlint,oxfmt --check(0.41.0) and the comment ratchet are clean.The test
"a file shortened while the history copies it in" writes a 64 MB file, starts a flush, and waits on the real condition (the store's temp copy passing 1 MB, or the sweep finishing first on a disk that clones). Only then does it shorten the file. It fails within 10 s instead of hanging the run.
Fails without the fix
blobStore.ts, the new test fails on ext4 3/3 with "the sweep never returned".incoming-*file is left).What I did NOT exercise