Repository navigation
fix(studio-server): undo and restore keep the media they delete or replace - #6215
miguel-heygen wants to merge 8 commits into
Conversation
somanshreddy
left a comment
There was a problem hiding this comment.
Reviewed at ca99dc76. Requesting changes: writeTo deletes the set-aside copy as soon as any write of that hash finishes. That reopens the "not kept" loss through two paths the new tests don't cover. I reproduced both against this head.
Method: a probe vitest file next to undoUncopied.test.ts, using the same helpers, run with NODE_ENV=test vitest run. The control (plain undo → redo) passes, and so do the PR's 3 tests.
1. checkout consumes the aside copy (blocker)
checkout calls blobs.writeTo(hash, join(dest, path)) for every file in the manifest. When the hash is set aside, writeTo now forgets it after copying it into the outside folder.
place join.mp4 → undo (bytes go to aside/) → checkout(made.id, "after", emptyDir) // file lands in emptyDir
→ redo ⇒ Error: An earlier version of join.mp4 is not kept, so this cannot be brought back.
Opening or exporting an old version silently deletes the only copy the redo needs. That's the same P0 loss, reached a different way.
2. Two paths with identical bytes in one change (blocker)
In writeAs, the batch writes path after path. The first writeTo of a set-aside hash forgets it, so the second write of the same hash fails partway through the batch:
place a.mp4 and b.mp4 (same bytes) in one change → undo (ok) → redo
⇒ ENOENT: copyfile '<history>/blobs/5e/5ee8…' -> '<project>/b.mp4.<tmp>'
after the throw: a.mp4 exists, b.mp4 does not, and no redo entry is committed
After forgetting, pathOf falls back to blobs/, which never held the file. The project is also left half-applied and history doesn't know it. A duplicated clip, or a media file copied under a second name, is enough to hit this.
Suggested fix
Don't free the aside copy inside writeTo. Two ways:
- Let
pruneand folding own aside lifetime. The hash stays referenced by the log, so it lives exactly as long as an entry can still need it. Once the file is back in the project,historyBytes()already counts it as current rather than as history. - Or forget only in
writeProjectFile, and only after the wholewriteAsbatch, for hashes no remaining path or entry needs.
Either way, add the two cases above to undoUncopied.test.ts.
Otherwise
- Hooking
keepinwriteProjectFilecovers undo, redo and restore. It runs before thermSyncand before overwrites, which is the right spot. dropnow skips aside hashes, soreclaimUncopiedon open can't take them, and the reopen test proves that.- Budget:
bytes()now includes aside copies, sokeepWithinBudgetcan fold entries sooner while undone media sits aside. That's expected, just noting it.
— Somu
There was a problem hiding this comment.
Verdict: REQUEST CHANGES · 2 blocker · 1 important · 0 nits · head ca99dc76
The guard in writeProjectFile is in the right place, but writeTo drops the set-aside bytes after the first write, so redo still fails in two reachable cases. I reproduced all three findings with vitest probes against this head, built next to undoUncopied.test.ts. ↓ inline
Scope
Read blobStore.ts in full, plus writeAs, writeProjectFile, checkout, keepWithinBudget/foldOldest, reclaimUncopied (in-engine and reclaimUncopied.ts). The reclaim and clean paths only touch blobs/, so aside/ is safe from them as the body says. Did not run the route suites. CI not inspected (reviewer identity).
| read: (hash) => readFile(pathOf(hash)), | ||
| async writeTo(hash, absPath, beforeReplace) { | ||
| await cloneOrCopy(pathOf(hash), absPath, beforeReplace); | ||
| if (aside.has(hash)) await forget(hash); |
There was a problem hiding this comment.
blocker: a redo that writes the same hash to two paths fails after the first write and leaves the redo half applied. writeTo forgets the aside copy after the first path, so the second copyFile hits ENOENT. Repro: one change writes join.mp4 and copy.mp4 with identical bytes, then undo, then redo. You get ENOENT … blobs/c4/c406…, copy.mp4 is back, join.mp4 is missing, and no entry is committed. The gone pre-check in writeAs passed, so nothing guards this.
Fix: don't forget inside BlobStore.writeTo. Once writeAs has finished every write, release the aside hashes that are now in the manifest (a release(hashes) that only touches aside).
| cloneOrCopy(pathOf(hash), absPath, beforeReplace), | ||
| read: (hash) => readFile(pathOf(hash)), | ||
| async writeTo(hash, absPath, beforeReplace) { | ||
| await cloneOrCopy(pathOf(hash), absPath, beforeReplace); |
There was a problem hiding this comment.
blocker: checkout also calls blobs.writeTo (projectHistory.ts:1314), so checking out a side that contains a set-aside file deletes the only kept copy. Repro: place join.mp4, undo it, then checkout(made.id, "after", tmp). The redo that follows fails with An earlier version of join.mp4 is not kept, which is the exact failure this PR fixes. Same fix as above: once forgetting moves out of writeTo, checkout is a pure read again.
| } | ||
| const standing = lstatSync(absPath, { throwIfNoEntry: false }); | ||
| if (standing?.isFile() && isUncopied(path, standing.size)) | ||
| await this.blobs.keep(absPath, hash === null); |
There was a problem hiding this comment.
important: set-aside bytes count toward historyBytes(), and keepWithinBudget folds entries until it fits. Undoing (or restore-overwriting) media larger than the budget (2 GiB default) therefore folds away all unpinned history. Then prune deletes the aside bytes as well. Repro with budgetBytes: 1 MiB and a 2 MiB join.mp4: after the undo, history.list() is empty and the redo throws That change is no longer kept. On main the file was lost but the history stayed. Repeated restores over a few hundred-MB videos reach the same budget, because the remove=false path copies every time. Either leave aside bytes out of historyBytes(), or give them their own cap that refuses the keep instead of folding history.
|
Both blockers fixed at 5972ba2. |
Edit accuracy: accurate 270 (base branch 271), smooth 144 of thoseThe gate fails: 1 case(s) that pass on the base branch fail here; the passing count fell from 271 to 270; baseline.json marks 1 failing case(s) as passing. Regressed (1)
Marked passing in baseline.json but failing (1)
Quarantined, measured but not gated (0) Unstable (3)
|
|
Third finding fixed at 3c749b3: set-aside bytes no longer count toward |
Both findings fixed at 3c749b3 (verified by probe). Approval follows once Genesis's CR clears. — Somu
|
Rebased onto main after #6175 (the only conflict was the studio-server budget line; kept 144000 / 568000). Re-measured on the rebased tree: packed 143,907, unpacked 567,217, both inside the budget. History suite 231 passed in 3 runs, tsc clean. Findings from the earlier reviews are all fixed at this head. |
3c749b3 to
b2d4b27
Compare
terencecho
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES — reviewed at b2d4b27af20fddc671593630e093730fab30e405 against immutable BASE/effective merge-base 0eae4946d33f3d874478fa7a292f3aae6a669ce1.
Reasoning: The checkout-consumption and same-batch duplicate-hash cases from Somu's review and Genesis's review are structurally addressed: writeTo is non-consuming and release follows the complete write batch. Excluding aside bytes also addresses the original media-only folding case. Three additional issues remain in the interactions below; these are source-derived failure sequences, not executed reproductions.
[P2] Preserve copied historical blobs when their hash aliases an aside copy
blobStore.ts:99–101, 115–116, projectHistory.ts:1124–1126.
Record media.mp4=H (2 MiB), undo its creation, create a visible snapshot.bak=H in its own recorded window, then replace the backup with distinct K in another window. Explicitly undo the media-deletion entry, then undo the backup's H→K change. HEAD's put sees sizes.has(H) from aside and discards the copied backup's temporary blob instead of creating ordinary backing. Restoring the media then releases the only stored H, despite the backup's historical reference. The backup is currently K, so the following sweep cannot repair H; the media uses reference-only storage. Its otherwise conflict-free backup undo now refuses “not kept.” BASE retains ordinary blobs/H from the copied backup, so both restores succeed. Preserve/promote the ordinary backing, or make release honor retained copied references; deferring release only until the batch ends is insufficient.
[P2] Subtract only current bytes included in the ordinary blob total
blobStore.ts:86, 133–134, projectHistory.ts:944–954.
Record identical a.mp4=H and b.mp4=H in separate entries, then undo only b. Aside H is excluded from bytes(), but size(H) remains positive and current a contributes that size to historyBytes() subtraction. With H=2 MiB, a 1 MiB history budget, and three distinct 0.75 MiB backup versions (one current, two historical), ordinary retained history is 1.5 MiB while reported history usage is approximately −0.5 MiB. The folding loop therefore keeps unrelated copied history above its budget. BASE has no stored media size to subtract and folds this over-budget ordinary history. Keep aside excluded, but do not grant its size as credit against ordinary retained bytes.
[P2] Publish canonical aside hashes only after preservation copying completes
blobStore.ts:118–131, especially 129, with 41–47 and 73–77.
keep(false) passes the final hash-named aside path directly to cloneOrStream. If forced cloning fails and fallback streaming errors after partial writes, the caller throws before replacing the project file, but a partial canonical aside/H can remain without cleanup. On reopening, openBlobStore registers that path by filename/stat without validating its content against H. At the BlobStore boundary, has(H) then advertises availability, subsequent preservation can skip the intact original, and writeTo(H) can serve the partial bytes as H. BASE's copied-blob path cleans failed temporaries and hashes the completed copy before final rename; it does not introduce this false-available partial media hash. Stage preservation copies and remove failures before publishing the canonical path. This is the conditional failed-stream/reopen case, not a claim that every failed copy corrupts data; no I/O error was injected.
Scope and method: All existing changed BASE bodies were fully read before changed HEAD bodies, with independent adversarial review, supporting history-log/classifier code, package metadata and unchanged lockfile coverage. Tests, builds, CI and runtime probes were not run; author execution claims are not my execution. Eight stale-plan lockfile Reads across two readers were excluded; valid freshly planned rereads supplied complete coverage. Those procedural errors remain recorded, not retroactively cured. The separate-entry all-media redo limitation is not labeled a new BASE-working regression; standalone read-only-reader parity and the transitive state-persistence helper are not claimed audited. This review does not dismiss another reviewer's changes-requested state.
— tai
|
All three P2s fixed at b5f1c17 by giving the set-aside copies their own ledger apart from the copied blobs.
|
somanshreddy
left a comment
There was a problem hiding this comment.
Re-reviewed at b5f1c17c (delta from b2d4b27a: blobStore.ts and tests). Clean for my scope. I'll approve this head once the open change requests clear.
tai's three findings, checked against the new code:
- An aside hash that is also an ordinary blob: aside copies now have their own ledger (
asideSizes).putonly checkssizes, so an ordinary blob ofHis created even whileHis aside.releaseonly forgets aside, andpathOfprefersblobs/. The new unit test covers this (put, then release, thenhas/readstill hold). - Budget credit:
size()readssizesonly, so an aside hash reports 0. It no longer subtracts fromhistoryBytes(), andbytes()excludes aside. - Partial copy published as
H: thekeep(false)copy now streams toaside/incoming-<uuid>, which is removed on failure, and is renamed toaside/<shard>/Honly after it completes. On reopen, the shard loop'sreaddirof anincoming-*file throws and is caught, so leftover temporaries are never registered. Theremovepath is a singlerename.
Re-ran my probes at this head (NODE_ENV=test). Each of these passes:
- control round trip
- checkout after undo, then redo
- same bytes at two paths in one change
- aside survives a reopen, then redo
The history suite passes apart from my one probe.
Still open (follow-up, not a hold; main loses all of these): the same bytes at two paths in separate entries. Undo both, redo a, then redo b, and redo b gives "An earlier version of b.mp4 is not kept", because release(H) runs after a's redo while b's entry still references H. Fix: release only hashes the log no longer references, or let prune own the aside lifetime.
— Somu
|
End-to-end check of this fix against the desktop test that found the bug (
The two failures at the stock 0.8.192 pin are the ones this PR targets: "blending files exactly one entry beside the episode's…" ( Not covered: the rest of the desktop suite and the app itself against this head. |
|
Folded Somu's remaining case in at 34c5c26: the same bytes at two paths in separate entries. The set-aside copy is now held per project path (hash -> paths); an undo or restore releases only the paths it wrote, and the copy goes when the last holder has its file back. Test: place a.mp4 and b.mp4 (same bytes) in two entries, undo both, redo a then b: failed with 'b.mp4 is not kept' on the previous head, passes now. History suite 235 passed, tsc/oxlint/format clean; packed 144,047, budget raised to 144,500 / 568,500. |
…ad of looking empty
somanshreddy
left a comment
There was a problem hiding this comment.
Re-review at 57bdfad6 (delta from 34c5c266: blobStore.ts +8/−2, listDir rethrows everything except ENOENT)
The intent is right: an unreadable history folder shouldn't look empty. But the new rethrow also catches a case the old .catch(() => []) was quietly absorbing, and now that case is fatal.
Blocker: a leftover incoming-* temp file makes history impossible to open
Both put and keep stage bytes in a shard-level temp file: join(dir, "incoming-<uuid>") (L101) and join(asideDir, "incoming-<uuid>") (L147). The scan at L81–83 treats every entry in root as a shard directory and calls listDir(join(root, shard)) on each one. If the process dies mid-copy (crash, kill, or a power loss during a big media copy), the temp file stays behind. On the next open, readdir on that file throws ENOTDIR. That error is now rethrown, so openBlobStore rejects, openProjectHistory rejects (L1411), and every later open of that project fails the same way until someone deletes the file by hand.
Probe (vitest, removed afterwards). Each case writes incoming-1234 into the folder, then calls openBlobStore:
| case | 34c5c266 |
57bdfad6 |
|---|---|---|
stray blobs/incoming-* |
opens | ENOTDIR: scandir …/blobs/incoming-1234 |
stray aside/incoming-* |
opens | ENOTDIR: scandir …/aside/incoming-1234 |
| control (no stray file) | opens | opens |
Suggested fix: keep the strict root read, and don't treat non-shard entries as shards. Either skip incoming-* names (better still, rm them on open, since they're orphans by construction), or stat/withFileTypes and walk directories only. A failing-first test that drops an incoming-* file into each of blobs/ and aside/ before opening would pin it.
Everything else at 34c5c266 still stands as reviewed: the separate aside ledger, path-scoped holds, and release after the write loop. Required CI is green at this head.
— Somu
|
Thanks Somu, the blocker was real and your probe matches what I reproduced.
|
Fixed at b9158de: open scan walks directories only and removes leftover incoming-* temps; failing-first test covers both folders. — Somu
What
Undo, redo and restore no longer lose a media file. Before history deletes or overwrites a video or audio file (or any file over 64 MiB), it sets that file's bytes aside, so the next redo or restore can put the identical file back.
Why
Since project history stopped copying media into its store, an Undo of a change that created a media file deleted the file, and the Redo that followed was refused with "An earlier version of ... is not kept". The same hole lost the replaced bytes when a restore overwrote a media file. History now never applies a change it cannot reverse:
writeProjectFileis the one place every undo, redo and restore writes through, so the fix is there.How
BlobStore.keep(absPath, remove)hashes the file and stores its bytes under<history>/aside/, by rename when the file is deleted (copy across volumes, or when it is only being overwritten).has,readandwriteTofind the bytes there. After an undo or restore has written all its files,releasefrees the aside copies it used, so storage is paid only for files the user actually undid away. A checkout of an old version only reads them, and several paths with identical bytes in one change all get restored before the copy goes.blobs/on purpose: reclaiming old media copies (on open andhyperframes clean) must never take them.prunestill removes them when their entries fold away.What I measured
New
undoUncopied.test.ts, failing first on main with "An earlier version of join.mp4 is not kept" in all three cases: undo then redo returns identical bytes; the same after the history is closed and opened again; undoing a restore returns the bytes the restore overwrote; a checkout of the old version in between does not spend the set-aside copy; two files with identical bytes made in one change both come back. The last two cases failed on the first version of this PR and pass now. A sixth case (media larger than the budget keeps the whole history and the redo) failed on the previous head. The same bytes at two paths in separate entries also come back through undo and redo in any order (each set-aside copy is held per project path and freed when the last path has its file back). History suite 235 passed. An independent undo/redo script over the builtdist/history/index.js(a media file placed, undone, redone) passes: file present after redo.What I did NOT exercise
packages/studio-serverroute suites: they need sibling packages built and were not run here.Size
Studio-server's packed size budget moves from 142,100 to 144,500 bytes (measured 144,047): the set-aside store is about 1.5 KB of code.