Skip to content

fix(studio-server): undo and restore keep the media they delete or replace - #6215

Open
miguel-heygen wants to merge 8 commits into
mainfrom
fix/history-undo-uncopied-media
Open

miguel-heygen wants to merge 8 commits into
mainfrom
fix/history-undo-uncopied-media

Conversation

@miguel-heygen

@miguel-heygen miguel-heygen commented Oct 24, 2026 •

Copy link
Copy Markdown
Collaborator

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: writeProjectFile is 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, read and writeTo find the bytes there. After an undo or restore has written all its files, release frees 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.
  • Set-aside bytes do not count toward the history budget (they are the user's own file, undone away), so undoing media larger than the budget never folds history away.
  • The set-aside bytes live apart from blobs/ on purpose: reclaiming old media copies (on open and hyperframes clean) must never take them. prune still 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 built dist/history/index.js (a media file placed, undone, redone) passes: file present after redo.

What I did NOT exercise

  • A media file on a different volume than the history folder (the rename fallback to copy is untested).
  • Deleting a multi-GB file: the file is hashed once before it moves.
  • packages/studio-server route 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.

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 prune and 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 whole writeAs batch, for hashes no remaining path or entry needs.

Either way, add the two cases above to undoUncopied.test.ts.

Otherwise

  • Hooking keep in writeProjectFile covers undo, redo and restore. It runs before the rmSync and before overwrites, which is the right spot.
  • drop now skips aside hashes, so reclaimUncopied on open can't take them, and the reopen test proves that.
  • Budget: bytes() now includes aside copies, so keepWithinBudget can fold entries sooner while undone media sits aside. That's expected, just noting it.

— Somu

@heygengenesis heygengenesis Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Both blockers fixed at 5972ba2. writeTo no longer frees the set-aside copy; a new release frees the copies an undo or restore used, only after every file of that change has been written (so identical bytes at several paths all restore, and a checkout only reads the copy). Added both of your cases to undoUncopied.test.ts: they fail on ca99dc7 (checkout then redo: not kept; two same-bytes files: second write fails) and pass now. History suite 224 passed; packed size 143,681 within the 144,000 budget.

@github-actions

github-actions Bot commented Oct 24, 2026 •

Copy link
Copy Markdown
Contributor

Edit accuracy: accurate 270 (base branch 271), smooth 144 of those

The 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.
Smoothness is reported in the artifact, not gated. A case fails only if it fails 2 of 3 runs.

Regressed (1)

  • seqheldtwice-tlhold-px-r0-root-z100

Marked passing in baseline.json but failing (1)

  • seqheldtwice-tlhold-px-r0-root-z100

Quarantined, measured but not gated (0)

Unstable (3)

  • seqplainkeys-none-center-r0-root-z100: tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo false, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true / tracking 0.06, pressJump 0, drop 0, reload 0, render 0.03, renderKey -, undo true, teleport true
  • seqheldtwice-tlhold-px-r0-root-z100: tracking 120.29, pressJump 312.3, drop 0, reload 0, render 0.02, renderKey -, undo true, teleport false / tracking 0.41, pressJump 60.41, drop 0.35, reload 0.35, render 0.02, renderKey -, undo true, teleport true / tracking 0.41, pressJump 0.11, drop 0.35, reload 0.35, render 0.02, renderKey -, undo true, teleport true
  • seqheldtwice-tlhold-px-r0-nested-z100: tracking 120.29, pressJump 312.3, drop 0.07, reload 0.07, render 0.03, renderKey -, undo true, teleport false / tracking 0.41, pressJump 0.11, drop 0.39, reload 0.39, render 0.02, renderKey -, undo true, teleport true / tracking 0.41, pressJump 0.11, drop 0.39, reload 0.39, render 0.02, renderKey -, undo true, teleport true

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Third finding fixed at 3c749b3: set-aside bytes no longer count toward historyBytes() (the blob store's byte total covers blobs/ only), so undoing media larger than the budget cannot fold history or prune the aside copy. New test with budgetBytes: 1 MiB and a 2 MiB file: fails on the previous head (history folded, redo refused), passes now. History suite 225 passed.

@somanshreddy
somanshreddy dismissed their stale review October 24, 2026 13:13

Both findings fixed at 3c749b3 (verified by probe). Approval follows once Genesis's CR clears. — Somu

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

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.

@miguel-heygen
miguel-heygen force-pushed the fix/history-undo-uncopied-media branch from 3c749b3 to b2d4b27 Compare October 24, 2026 13:16

@terencecho terencecho left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

All three P2s fixed at b5f1c17 by giving the set-aside copies their own ledger apart from the copied blobs.

  1. Alias: put dedups against the ordinary blobs only, so a copied backup with the same bytes as a set-aside file gets its own blob; releasing the set-aside copy no longer takes the only H. Test: keep, put the same bytes, release: still stored.
  2. Credit: size() and bytes() cover ordinary blobs only, so a set-aside copy gives no credit against retained history. Test: both are 0 while it sits aside.
  3. Partial copy: the copy goes to a temp name inside the aside folder and is renamed to its hash only after it finishes; a failed copy removes its temp. A test checks the folder holds only the finished hash. I did not inject an I/O error, so that one is structural, not exercised.
    Tests 1 and 2 fail on the previous head and pass now. History suite 234 passed; packed 143,969 inside the 144,000 budget.

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:

  1. An aside hash that is also an ordinary blob: aside copies now have their own ledger (asideSizes). put only checks sizes, so an ordinary blob of H is created even while H is aside. release only forgets aside, and pathOf prefers blobs/. The new unit test covers this (put, then release, then has/read still hold).
  2. Budget credit: size() reads sizes only, so an aside hash reports 0. It no longer subtracts from historyBytes(), and bytes() excludes aside.
  3. Partial copy published as H: the keep(false) copy now streams to aside/incoming-<uuid>, which is removed on failure, and is renamed to aside/<shard>/H only after it completes. On reopen, the shard loop's readdir of an incoming-* file throws and is caught, so leftover temporaries are never registered. The remove path is a single rename.

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

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

End-to-end check of this fix against the desktop test that found the bug (podcastJoinsHistory.test.mjs, 12 tests), on a Linux box, Node v22.22.2, three runs each:

setup studio-server / hyperframes / core result
desktop main before the 0.8.192 bump 0.8.190 12 pass, 0 fail, 3 of 3 runs
desktop at the 0.8.192 pin (#5586 head), clean install 0.8.192 10 pass, 2 fail, 3 of 3 runs
same, with this PR's head b5f1c17c packed and extracted over both installed @hyperframes/studio-server@0.8.192 copies (tarball sha256 starts 8dbe25927621c30b) 0.8.192 + this fix 12 pass, 0 fail, 3 of 3 runs

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…" (An earlier version of …/joins/<id>.mp4 is not kept, so this cannot be brought back) and "the card's Undo is all or nothing…" (the three join videos gone after the refused Undo). Both pass with the fix.

Not covered: the rest of the desktop suite and the app itself against this head.

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

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.

@somanshreddy somanshreddy left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@miguel-heygen

Copy link
Copy Markdown
Collaborator Author

Thanks Somu, the blocker was real and your probe matches what I reproduced.

  • Leftover incoming-* makes history unopenable: fixed. The open-time scan now reads each folder with entry types and walks directories only; a non-directory entry named incoming-* is removed on open (a crash mid-copy leaves exactly that, and it was never published), anything else non-directory is skipped. The root read is still strict, so an unreadable folder still fails the open with EACCES.
  • Failing-first test: it writes incoming-* into both blobs/ and aside/, opens the store, and expects the open to succeed, the set-aside copy to still be there, and both leftovers gone. With the previous scan it fails with ENOTDIR: scandir .../blobs/incoming-1234, the same error as your probe. A second test pins that an unreadable folder still fails the open with EACCES.
  • Audit of the other readdir calls in the history code: none assume a name is a directory. removeEmptyFolders, fileLeftIn, bytesIn and snapshotRuns read with entry types; the history-root listings in pruneHistories and reclaimUncopied are filtered by isHistoryId; checkout reads the caller's destination folder, where a file correctly fails.
  • Studio-server history suite 237 passed, CLI history test 56 passed.

@somanshreddy
somanshreddy dismissed their stale review October 24, 2026 18:50

Fixed at b9158de: open scan walks directories only and removes leftover incoming-* temps; failing-first test covers both folders. — Somu

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants