Repository navigation
fix(studio-server): history waits out a file a save has emptied, and rereads one written mid-copy - #7480
Conversation
…rereads one written mid-copy
Edit accuracy: accurate 267 (base branch 267), smooth 107 of thoseThe gate passes. Quarantined, measured but not gated (8)
Unstable (2)
|
There was a problem hiding this comment.
Changes requested at af0e7006. There are two blockers.
1. The body has no "fails without the fix:" line. This lane requires a fix PR to name a test that fails without the fix, on a line that starts fails without the fix:. The test plan says that "5 of the new tests fail" on main, but it never uses that line. The claim itself holds. I ran the new tests against main's blobStore.ts and projectHistory.ts, and exactly these 5 fail:
files no empty version for a save paused between emptying the file and writing itrecords a file emptied for good by the next sweep, once it has been quiet a moment- the three
a file written to while the history reads itcases
Add a line such as fails without the fix: projectHistory.saving.test.ts › files no empty version for a save paused between emptying the file and writing it.
2. A file still changing when the history first opens is later filed as an outside add, and undoing that entry deletes the file. put() now returns null for a file that keeps changing. storeIfPresent passes that null on, and three callers read null as "the file is not there":
firstOpenskips the file, so it is not intrackedor in the baseline.adopt→recordAdoptedreturns early and drops the file fromadopting, so a large media file being written at open never joins the baseline.takeInUnnamedLedgerleaves the ledger out of the baseline.
On the next sweep, observe finds no known hash and records before: null. The file therefore appears as an entry under "Changed outside the app" that adds a file which existed before the history opened.
I reproduced this with the real engine, stubbing only put() to return null for one file three times during openProjectHistory. That is the same result the new retry loop gives for a file that keeps changing. After flush(), list() shows [["Changed outside the app", [["media.txt", null, "<hash>"]]]]. undo of that entry returns ok: true, and media.txt is deleted. On main, put() never returned null for a file that was there, so the file went into the baseline, possibly with torn bytes but never as an add.
Possible fixes: keep "gone" separate from "still changing" in what storeIfPresent returns, and on the baseline paths (firstOpen, adopt, the ledger) either take the file as it stands or keep it pending, as adopting does, until it settles. Please add a test for it.
The rest looks right to me:
- The empty-only wait is used only by
sweep(). Claims for named paths, writes taken by an open window, andsettle()("every") take the file as it stands. sweepIn(SETTLE_MS)reschedules after a waiting sweep, and it is a no-op when closed.observetreats anullfrom a moving file as "skip, try next sweep", andtrackedstays as it was.- The known limits are stated and pinned.
- At this head the history suite passes: 13 files, 268 passed, 2 skipped.
…its first whole copy
There was a problem hiding this comment.
Approved at 2abfcfc4. Both blockers from my review of af0e7006 are resolved.
- The "fails without the fix:" line is now in the body. It names
projectHistory.saving.test.ts > files no empty version for a save paused between emptying the file and writing it, and that test fails on main, as I checked last round. - A file still changing at first open is no longer filed as an outside add.
storeIfPresentnow returns{hash},"gone"or"changing".firstOpen, the media ledger andrecordAdoptedsend"changing"toadopting, whichsweep()skips and which a START restore refuses while it is non-empty.adoptLaterretries with backoff capped at 30 s, andadopting.jsoncarries the path to the next open. - The new test,
is not filed as added outside: its first whole copy is where its history starts, fails red-first. It fails with this head's tests run againstaf0e7006'sprojectHistory.tsand passes at this head. - The history suite passes at this head: 13 files, 269 passed, 2 skipped. CI, regression, Windows render verification, Comments and CodeQL are green on this head.
Not a blocker, and found by reading the code only: once a retry is scheduled, the promise for that path in adopting has already resolved. A claim() naming the path therefore no longer waits for the adoption. Its sweep skips the path, and the claimed bytes can end up in the baseline instead of in the claim. This only happens when a file is changing at open and is then claimed before it settles. Worth a follow-up, for example keeping the path's promise pending until the retry lands.
…nce watermark Seventeen commits, PRs 7469-7493, issues 7473 and 7486 and twelve pin moves are recorded with their evidence in the continued ledger, DECISIONS, UPSTREAM and ROADMAP. Adopted: heygen-com#7466, heygen-com#7479, heygen-com#7482, heygen-com#7490 and the heygen-com#7441 and heygen-com#7440 docs. Deferred: heygen-com#7469, heygen-com#7471, heygen-com#7476, heygen-com#7480, heygen-com#7468, heygen-com#7487, heygen-com#7491. Not applicable: heygen-com#7470, heygen-com#7351, heygen-com#7475, heygen-com#7483. The watermark moves to 61e720b, PR heygen-com#7493, issue heygen-com#7486; thirteen new PRs are pinned. FORK.md records the subComposition test differences. The core packed-size roadmap row trigger is not met. Co-Authored-By: Claude Sonnet 5.5 <noreply@anthropic.com>
What
The project history no longer files a version of a file that a save had only half finished:
SETTLE_MS(100 ms), the next sweep records it, and a follow-up sweep is scheduled so that happens without another write.Why
A save that truncates a file and then writes it leaves the file empty for a moment. A sweep that lands in that moment, for example a flush or a claim of another file, filed the empty file as the file's new version. After the save finished and was claimed, undoing it brought back an empty file instead of the last whole version.
Repro on main, in package terms: open
index.htmlwithopen(path, "w"), callhistory.flush()andhistory.claim(who, "x", []), write the content, close, claimindex.html, then step back. The file reads'', andhistory.list()shows an outside entry whose new version is the empty file's hash (e3b0c442…).Related work
Refs #5627, which lets the history copy a changing file without stalling its sweep. This PR uses the same approach: it reads again at once and never waits for the writer.
How
storeIfPresentreturns the hash,"gone"or"changing". Every caller reads both:observe: either one is skipped, and the next sweep looks again.firstOpen, the media ledger, and background adoption:"gone"is left out as before, and"changing"stays pending inadoptinguntil a whole copy lands.storeBytescopies a private staged file and ignores the result.blobStore.tsexportsSETTLE_MSandisSettling(stat, now).put()compares size, mtime and ctime before and after the read or the copy.sweep()skips a file that is empty, changed withinSETTLE_MSof the sweep, and not written inside an open window, and it leaves the tracked state alone. Three callers take every file as it is, so a deliberate empty save is still recorded at once:settle()), so an undo still sees the file as it is and refuses to write over itSETTLE_MS = 100. It has to be longer than the 20 ms burst a watcher gathers before it starts a sweep. Otherwise the sweep that a save's own first event triggers would always find the file older than the bound and file it. 100 ms adds at most that much delay before an emptied file is recorded.RACY_MS(2 s) is a separate rule: it decides when a stat can be trusted, not when a file can be filed.files a window's empty write in that window, ended at once by close or flushfails.Test plan
Unit tests added:
blobStore.test.ts(torn small-file read, torn large-file copy, a file that never stops changing), and a newprojectHistory.saving.test.ts(paused-writer repro, half-written save, emptied-for-good file recorded by the next sweep, claimed empty save, window's empty write, undo after a fresh empty).fails without the fix: projectHistory.saving.test.ts > files no empty version for a save paused between emptying the file and writing it
A file that is still changing during every read at first open is not filed as an outside add, and undo leaves it in place (
a file still being written when the history first opens). This test fails at the previous head of this PR, where such a file later showed up as an outside add whose undo deleted it.On main, 5 of the new tests fail: the 3 read tests, the paused-writer repro, and the emptied-for-good test. All pass on the branch.
Breaking each piece in turn turns a test red:
Full
@hyperframes/studio-serversuite: 1649 passed, 2 skipped.tsc --noEmitandoxfmt --checkare clean.Manual testing performed
Comments follow CONTRIBUTING.md "Comments"