fix(studio-server): history refuses once closed and reopens when a new project takes its folder - #4537
Conversation
terencecho
left a comment
There was a problem hiding this comment.
Reviewed bd03bd4c14ff5794ad5a3418ac709be060d877bc. Changes requested. The explicit-close guard and ordinary different-inode replacement path work in focused tests, but the project-identity boundary is still unsafe in reproducible replacement cases, and the new cache can permanently lose history after a transient move.
-
A delete/recreate can restore the old project's files into the new project.
projectHistory.ts:313–318accepts the folder as the original when device/inode match and its history ID is missing. On this ext4 volume, deleting a project and immediately creating a new folder at the same path reused the inode; in an end-to-endopenProjectHistoryreproduction,replacedAtPath()returned false andrestore("start")overwrote the new project'sindex.htmlwith old-project content. The PR body acknowledges inode reuse as a known limit, but this is the exact replacement boundary this change promises to protect, and it occurred under an ordinary delete/recreate. The old path-only implementation also had this exposure; this review is calling out an incomplete safety fix, not attributing the original exposure to this diff. Please require a durable identity that cannot accept a new, ID-less folder merely because its inode was recycled, while retaining the intended handling of a history-ID file removed within the same folder. -
A copied history ID still transfers the original project's history to its replacement. The new
historyCache.ts:12–16correctly notices a different inode and reopens, but the existinghistoryId.ts:33–40accepts a copied.hyperframes/history-idif its recorded directory equals the replacement's pathname. I reproduced: copy the original project including its ID, move the original elsewhere, put the copy at the original path, then request history. The replacement receives the original log (Old change); opening the moved original assigns it a new ID and loses access to its old log. The ID helper predates this PR, but the new cache's promise that a replacement gets its own history is not true in this reachable case. Please make the reopen distinguish a copied ID from the original folder before taking ownership of that log. -
A move during history open can leave a permanent null cache entry. The new constructor
statSync(this.dir)(projectHistory.ts:285) throwsENOENTif the folder moves after the ownership-lock await and before the replacement appears. The preview and dev-server open wrappers convert only the named closed/busy errors into a retry;historyCache.ts:9–12then retains the resolvednullpromise. I reproduced a move during that await: the firstgetreturns null, and even after creating the replacement at the same path, subsequentgetcalls keep returning null for the server's lifetime. Treat this missing-path transition as retryable or evict its cache entry.
Focused tests passed in the isolated worktree: history engine 87 passed/2 skipped, history routes 17, Studio adapter 15, CLI server/history commands 60. These do not cover the replacement witnesses above. The only failing required check at this head was the PR-body capture gate (86 changed Studio/player lines exceed its no-visible-change exemption); it was not a product-test failure. No product files were edited.
— Review by tai (pr-review)
69a7047 to
19e86ab
Compare
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed 19e86abb8492da2825daed4acfbd0cf089e7d720. Changes requested. The ordinary missing/corrupt marker, fresh copied-ID, replacement, and moved-then-restored cases now behave as intended; focused history tests passed. Three data-integrity paths remain:
-
Existing history records without
inostill transfer the old log to a replacement.packages/studio-server/src/history/historyId.ts:28–30treatswas.dir === dirandwas.ino === undefinedas not a copy. Pre-changeproject.jsonrecords have that shape. Move the original, place a copy carrying its.hyperframes/history-idat the original path, and open history: the copy inherits the old log and the moved original gets a new ID. In a controlled Studio API reproduction, GET history offered the old Undo and POST restore replaced the copy's new content with the original's baseline. The fresh-record test does not cover this migration state. Distinguish the copied folder even for legacy records before assigning their log. -
An in-progress Undo/restore can cross a folder replacement.
projectHistory.ts:802–818checkswhereFolder()once, then awaits successive path-based writes inwriteProjectFile(:825–837). In a controlled 320-file restore, I moved the original after its first file was restored and put a different project at the old path. A later file in the replacement changed fromNEW PROJECT LASTto the old project's contents, while the moved original retained its edited file. Check/bind ownership across the entire write, not only before its first await; add a deterministic mid-restore swap test. -
A sweep can ingest the replacement project's bytes into the old log.
projectHistory.ts:390–447checks ownership before listing, then asynchronously observes and hashes files by the mutable path. In a controlled 320-file flush, replacing the folder during the sweep caused the old log to record the SHA-256 of the replacement's file. Keep the sweep tied to its original directory/identity across asynchronous observation and blob writes; test the mid-sweep swap as well.
These are controlled filesystem/API reproductions, not a full UI walk. The build passed, focused history tests passed 161 with 2 skipped; a separate broader test invocation had one build-sensitive, non-history fixture failure. Required CI, including the captures gate, is passing at this head. I downloaded and sampled the two linked videos: their embedded BEFORE/AFTER overlays appear opposite the PR body's link labels, and the recording labeled After in the body is from an earlier head; please correct that evidence, though it is not the reason for this verdict.
— Review by tai (pr-review)
97fd766 to
eb8725b
Compare
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed #4537 at eb8725b3fa7f04556a67cf6167bcf6297a9bebfe. Changes requested. The earlier legacy no-inode reset, between-files restore check, after-copy sweep check, second-history-root behavior, deleted-record recovery and atomic record write are addressed in the inspected paths. The central rule—never attach another folder's history or write through a folder that has been replaced—still has four distinct failures:
-
In-flight restore writes through a replaced folder.
packages/studio-server/src/history/projectHistory.ts:838–850callsassertWritable()only before asynchronousblobs.writeTo. That writer makes a temp copy and renames it to the target path. A controlled witness swapped the original folder for an unrelated folder after the check but beforewriteTo;restore(START)changed the unrelated project'sNEW PROJECTfile toOLD BASELINEwhile the moved original remainedOLD EDIT. The new between-files test swaps after one write completes, so it misses this interval. A check after the rename would detect damage too late; the destructive write must remain bound to the validated folder across the write boundary. -
Copied history marker plus recycled inode reuses old history.
historyId.ts:33–40accepts the saved ID when the recorded inode matches, but an inode is reusable after removal. In an ext4 backup-restore witness, a backup carried the old.hyperframes/history-id; after deleting the original, the restored directory reused its inode on attempt 61. Opening the restored backup kept the old UUID and exposedOld change; restoring rewroteNEW BACKUP CONTENTtoOLD. Please fail closed when a copied/restored marker cannot prove the same folder, rather than treating inode equality alone as enduring identity. -
The ownership-lock wait makes a pre-checked identity stale.
projectHistory.ts:1056–1064calculatesprojectHistoryIdbefore awaitingtakeHistoryOwnership; the constructor at:286–288checks the marker ID, not that the selected folder still has the same inode after the wait. A controlled witness replaced the folder with a copy carrying the same marker (different inode) during the wait. The waiting open adopted the copy under the original UUID, exposed the old Undo history and rewroteNEW CONTENTtoOLD BASELINEon restore. Revalidate/recompute identity after the lock is acquired. -
First-open baseline reads a mutable path.
projectHistory.ts:369–379lists the original folder, then awaitsblobs.putfor each path without checking ownership before persisting that baseline. Swapping the folder at the first actual put recorded the replacement'sNEW PROJECTbytes under the original UUID; reopening the moved original then offered that baseline and restore rewrote itsOLD PROJECTbytes toNEW PROJECT. Discard an incomplete baseline if the source folder changed during its reads.
These are deterministic Bun fixtures exercising the actual history/blob code with controlled filesystem swaps, not observed user incidents or an interactive Studio walk. The isolated worktree lacks dependencies for focused Vitest; required GitHub checks at this head passed, but checks do not cover these interleavings.
— Review by tai (pr-review)
50e7491 to
16c949c
Compare
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed #4537 at 16c949c096135e05b2a5879759a827a04b22c494. Changes requested: the new creation-time identity is checked on open, but not by an already-open history.
historyId.ts:19–25,34–46 now records and compares ino plus birthtimeMs, so reopening a copied folder on a recycled inode can receive a fresh history. But the live Engine.whereFolder() in projectHistory.ts:323–329 compares only dev, ino and the copied history marker; it omits this.folder.birthtimeMs even though the constructor captured it at line 298. list() and restore() trust that check. Delete the original project folder while a history handle remains open, copy back a backup carrying its .hyperframes/history-id, and wait for the original inode to be recycled: the replacement is treated as the old project. A controlled ext4 fixture reproduced the collision on attempt 61; list() exposed the old Old change entry, and restore('start') overwrote the replacement's NEW BACKUP CONTENT with OLD BASELINE. This is a controlled local filesystem witness, not a production incident; inode recycling time varies by filesystem.
Please use the same birthtime identity in the live path check, with a regression that keeps the old handle open across a copied-marker/inode-reuse replacement and verifies both listing and restore refuse it. The reopen-only test changes the recorded creation time after closing the handle, so it does not exercise this path.
— Review by tai (pr-review)
jrusso1020
left a comment
There was a problem hiding this comment.
Review at 16c949c0.
Verdict: REQUEST_CHANGES. I reproduced tai's newest finding on my own: an open history still accepts a copy that carries the project's .hyperframes/history-id when the inode is recycled, and a restore then writes the old project's bytes into it. In my fixture the inode was reused on the first try in 9 of 9 runs, so on ext4 this can happen with an ordinary rm -rf and cp -r.
What would flip this to an approve: compare birthtimeMs in Engine.whereFolder() the same way isRecordedFolder() does on reopen, and add a regression test that keeps the handle open across the replacement and checks that both list() and restore() refuse. I made that one-line change locally. My repro then refused, and all 102 history tests still passed. The Should-fix items below are worth doing in the same push, but I would not hold the approve on them.
tai's findings
Evidence is a mutation (the named test goes red when that guard is removed) or a replay I ran. Mutations ran in a separate worktree at this head; suites were studio-server src/history (102 + 2 skipped), and for host changes the CLI server/command tests (60) and Studio vite.adapter (24), with studio-server dist rebuilt first because both host suites import it through its node export.
Round 1 (bd03bd4c)
- Delete and recreate reuses the inode, ID missing. Resolved. Removing the ID check in
whereFolder()(projectHistory.ts:328) fails "counts a folder whose history id is gone as another project". Replay with real inode reuse (no marker in the new folder):replacedAtPath()true,list()andrestore()throwHistoryClosedError, file untouched. - Copied history ID takes over the original's history on reopen. Resolved. Making
isRecordedFolder()accept any record (historyId.ts:24) fails 6 tests, including both "gives a copy put where the original was its own history" cases. - Move during open leaves a permanent null cache entry. Resolved. Putting back a throwing
statSync(this.dir)in the constructor (projectHistory.ts:294) fails "refuses to open, so a server can retry, once the folder moved away while it waited". Removing the missing-folder refusal (:1066) fails "refuses, so a server can retry, to open a folder that is not there". Dropping theforgetonHistoryClosedErrorfails "tries a history whose folder changed while it opened again" in the CLI (studioServer.ts:440) and "opens it again on the next request" in Studio (vite.adapter.ts:117).
Round 2 (19e86abb)
- Records without
inohand the old log to a copy. Resolved. Accepting a record with noino/bornfails "gives a folder whose history record cannot prove it is that folder a history of its own". - Restore crosses a replacement between files. Resolved. The top-of-write check (
:849) and the check before rename (:861) cover each other: removing either alone leaves the suite green, removing both fails "stops a restore at the next file". - Sweep files the replacement's bytes into the old log. Resolved for the log. Removing the per-file check (
:468) fails "stops a sweep at the next file". See the Nit below about the blob store.
Round 3 (eb8725b3)
- Restore write between check and rename. Resolved. Dropping the
beforeReplacecheck (:861) fails "refuses a restore write whose folder was swapped after its check". - Copied marker plus recycled inode, on reopen. Resolved on reopen only. Dropping the birthtime compare (
historyId.ts:24) fails "gives a folder on a reused inode, created at another time". My replay's reopen step confirms it with a real recycled inode: fresh id, empty list. - Identity stale after the ownership wait. Resolved. Dropping the record check in the constructor (
:296) fails "refuses to open ... once a copy carrying its id took the folder's path while it waited". - First baseline reads a swapped folder. Resolved. Dropping the check (
:383) fails "drops a first baseline read partly from a swapped folder".
- Restore-delete gap (Miguel's addition). Resolved. Dropping the delete check (
:857) fails "refuses a restore delete whose folder was swapped after its check".
Round 4 (16c949c0, this head)
- Live
whereFolder()omits birthtime. Confirmed.projectHistory.ts:327comparesdevandinoonly, while the constructor capturedbirthtimeMsat:298. Repro (tsxscript against this head, ext4 scratch dir, Node 22 reports a real birthtime via statx): open history, record "Old change", copy the project with its marker to a backup and edit the backup, thenrm -rf project && cp -r backup projectwith the handle still open. The inode was reused on the first attempt in 9 of 9 runs (birthtime differed by 8 to 12 ms). tai needed 61 attempts, and a separate 500-attempt run of this fixture earlier in this review got no reuse at all (other suites were running on the same disk then), so how often it happens varies. Every run that reused the inode:replacedAtPath()false,list()returned["Old change"],restore(START)ran and changed the file fromNEW BACKUP CONTENTtoOLD BASELINE. Closing and reopening the same folder gave a new id and an empty list, so the two identity checks disagree about the same folder. The same run with the marker removed from the backup refused correctly.
- Round 2 note on the videos. The body now says the After video was "recorded at
fe90b5e5; the head differs by one line in the sweep". I fetchedfe90b5e5(same parent,162de1a6). It differs from this head by 6 files, +168/-40, including the birthtime identity inhistoryId.ts, thebeforeReplacecheck inblobStore.ts, and the delete check inprojectHistory.ts. The sweep change is the one line that was removed (see Nit 1). The user-visible behavior in the video is probably unchanged, but the sentence is not accurate.
New findings
Nobody raised these in the four earlier rounds. Each one comes with a probe or mutation I ran at this head. Probes live in scratch dirs and import the real modules. Where the swap has to happen at an exact point, the probe uses the same hook points as the PR's swap tests.
Should-fix
-
No test pins the live inode compare, so tai's bug class could come back and the suite would stay green.
projectHistory.ts:327. I removednow.ino === this.folder.inoand rebuilt studio-serverdist. The full studio-server suite (797 tests), the CLI tests and the Studio tests all still passed. With that mutation in place, the common copy case gets adopted: runningcp -a p p2 && mv p p-old && mv p2 pwith the history open gavereplacedAtPath()false, andrestore()wroteOLD BASELINEinto the copy. At this head the same replay refuses. The reason: every live-swap test builds its replacement without the marker, so the ID check always fires first. Please add a live-handle test whose replacement carries the marker on a new inode. The recycled-inode test tai asked for belongs right next to it. -
No test pins the sweep's entry check either.
projectHistory.ts:416. I put back the oldif (!existsSync(this.dir)) return;and all 797 studio-server tests passed. With that mutation, take a project witha.htmlandb.htmland replace it with a new project that has onlya.html. On its next flush, the old history logs "Changed outside the app: b.html deleted". Reopening the moved original then logsb.htmlagain as a second outside change. The removal branch of the loop (:446-449) has no per-event check, so the entry check is the only thing that stops this. A test whose replacement lacks one of the old files would pin it. -
In Studio's dev server, swapping two project folders' names leaves one of them with no history until restart.
vite.adapter.ts:116-117only callsforgetonHistoryClosedError, so aHistoryBusyErrorstays cached asnull. Here is the sequence aftermv a tmp; mv b a; mv tmp b. Theaentry sees a replaced folder, so it closes A's history and reopens ata. That folder is now project B, and B's history is still held by this same process'sbentry. The open waits the full 5 s and fails withHistoryBusyError. Measured throughcreateStudioApi+createViteAdapter:after swap: GET a -> 404 5032ms "This project has no history here." after swap: GET b -> 200 (entries: "edit a") again: GET a -> 404 0msA plain rename (
demotodemo-renamed) hits the same wall. The first request takes 5028 ms and returns 404. Every later request also returns 404, even after a new project takesdemoand the old history closes. The rename case already failed onmain, but this PR's stated goal is that a renamed or replaced folder no longer leaves the server's history stuck. The CLI server already retries onHistoryBusyError(studioServer.ts:439). I tried the sameforgetin the Studio adapter: after the swap, the nextGET areturned 200 with "edit b". The rename then recovered too, but only after the old path's history closed. Until then, every request waited the full 5 s and got a 404. So the"gone"entry also needs to let go of its lock when the same history is wanted under a new path. -
hyperframes historydoes not treat the newHistoryClosedErroras a refusal.packages/cli/src/commands/history.ts:277-280.guarded()handlesRefusal,AmbiguousPreviewServerErrorandHistoryBusyErrorwith exit 2, and prints JSON when--jsonis set.HistoryClosedErrorfalls through. I ranbun run packages/cli/src/cli.ts history --jsonand swapped the folder while the command waited for the lock. Result: exit 1, empty stdout, and one stderr line ending "is now another project.". The control run, where the history is held past the wait, gave exit 2 and{"ok": false, "error": "...open in another process..."}. An agent that reads--jsongets nothing back for the refusal this PR adds. The PR's own mid-command test callswithOwnerdirectly, so it never reachesguarded().
Nit
- Refused reads still leave the other project's bytes in this history's blob store.
fe90b5e5hadif (this.whereFolder() !== "here") break;at the top of the sweep loop (projectHistory.ts:441-450), and this head removed it.observe()now drops each file only after copying it (:467-468). I forced a swap at the second blob copy with 50 edited files. This head made 50 copies and left 49 blobs of the new project's bytes in the old store. With the break put back, it made 2 copies and left 1, and all 102 history tests still passed. A refused first baseline does the same (:382-383): the other project'sb.htmlwas in the store after the refusal. Only a budget fold removes these blobs. checkoutchecks less than every other call, and three of the listed refusals have no test.projectHistory.ts:996checks onlyexistsSync. On a replaced handle,list()threwHistoryClosedError, butcheckout(entry, "after", emptyDir)ran and wroteOLD EDITout. Separately, removingassertOpen()frompeek(:989),next(:1012) orpin(:1020) each leaves all 797 tests green. OnlylistandreadBlobare asserted.- A move in the middle of a write recreates the old path. The body says "writes refuse instead of recreating the old path". I moved the folder away, with nothing put at the path, at the PR's own
beforeWritehook point during a restore ofscenes/intro.html.cloneOrCopythen ranmkdir(dirname(to), { recursive: true })(blobStore.ts:28), which recreatedproject/scenes. The refusal message said "is now another project". The next cacheget()treated the recreated folder as a new project and wrote.hyperframes/history-idinto it. - The body says a history closed at shutdown is not reopened. It is reopened when the folder was replaced.
historyCache.ts:12does not check whether the handle is closed, andreplacedAtPath()(projectHistory.ts:1031) looks only at the folder. My probe ranget(), thencloseAll()(whatshutdown()does atstudioServer.ts:1104), then swapped the folder, then calledget()again.openran a second time, the late call got a handle for the new project, and that history'sowner.pidheld this process's pid after shutdown. The same late call also wrote a fresh.hyperframes/history-idinto the folder.preview.ts:1683-1687closes the HTTP server only after shutdown settles, so a late call like this can actually arrive. - When two processes open a never-opened project at once, one is refused as "another project". The id is minted before the lock (
historyId.ts:38-45) and checked after it (projectHistory.ts:296). Two child processes started at the same instant: 20 of 20 trials gave oneHistoryClosedError: ... is now another project.and one success, and every trial left a second, orphan history directory. That beatsmain, where both processes silently forked. But upgrade day triggers it too, because every legacy record gets re-minted. Through the CLI, the refused side hits Should-fix 4. - Where birthtime falls back to ctime, the first open of an existing project is refused. This is a simulation: I patched
statSyncso thatbirthtimeMsequalsctimeMs, which is what the body says Node reports without statx, and aged the project folder by 100 ms before opening. Creating.hyperframesmoves the root's ctime between the recorded stat and the check after the lock. Result in 3 of 3 runs: the first open threw "is now another project", the second open got a new id, and a reopen after adding a root file got another new id. That left three history dirs with the edit lost. The body mentions "fresh on most reopens" but not the refused first open or the orphan per reopen. This matters for the blocker fix too: oncewhereFoldercompares birthtime, a live handle in these setups turns "replaced" on every root entry change. - Some of the body's "single mutations each fail their test" do not hold as I ran them. "The inode compare": both inode compares survive (
projectHistory.ts:327, andhistoryId.ts:24with onlybornkept). "The open-time id check": droppingreadId(...) !== projectIdfrom the constructor (:296) survives, because the record check covers it. "Each of the two write checks": each one survives on its own, as noted under round 2. None of these is a bug at this head, but readers will rely on those claims about the suite. - Already on
main, but this PR rewrote the line:vite.adapter.ts:122runsvoid histories.closeAll(). If a final commit fails while the dev server stops, the result is an unhandled rejection. To trigger it I opened a window, made the history dir read-only, and emittedclose. The result wasEACCES ... unlink, which crashes a Node process under default settings. Adding.catchasstudioServer.ts:1104does would fix it.
Checked
- I read the full changed files, not only the diff:
projectHistory.ts,historyId.ts,historyCache.ts,blobStore.ts,ownerLock.ts,atomicFile.ts,routes/history.ts,studioServer.ts,historyOwner.ts,vite.adapter.ts,commands/history.ts(guarded), and every changed test. - Every site that decides project identity:
projectHistoryIdandisRecordedFolder(inode + birthtime), the constructor after the lock wait (id + record),whereFolder(dev + inode + id, no birthtime), sweep entry and per-file checks, the first baseline,writeAs, the top ofwriteProjectFile, the delete check,beforeReplace,replacedAtPath, the cache, andcheckout(existsSynconly).whereFolderis the only site whose identity check differs from the reopen path. - Error mapping:
withHistory(routes/history.ts:65-77) turns any refusal into a 409, and the blob route turns a rejectedreadBlobinto a 404. On those routes, a closed or replaced handle never produces a 500 or an unhandled rejection. - Cleanup: close clears the noted timer, and
settleAllclears the quiet, max, idle and claim timers. A failed open releases the owner lock (projectHistory.ts:1082-1085). The cache closes a replaced handle before it reopens, which releases that handle's lock.releasecan safely run twice (ownerLock.ts:83-87). - Tests at this head: studio-server
src/history102 passed and 2 skipped;src/routes348; the full studio-server suite 797 passed and 2 skipped; CLIstudioServer.test.tspluscommands/history.test.ts60 (these needbunon PATH); Studiovite.adapter24. These match the counts in the body.tsc --noEmitexits 0 in studio-server, studio and cli. - Other body claims I checked by mutation: the shared close (removing
??=fails 25 tests), the record write-back (fails "rewrites a history folder removed while open"), the device-number case (comparingdevfails "keeps the history when the disk's device number changed"), and the cache reopen (fails both host tests, but only afterdistis rebuilt, because the host suites import studio-server through itsnodeexport). git merge-tree --write-tree origin/main 16c949c0is clean (origin/mainat8798e40d).- Birthtime on this box: Node 22 and Bun 1.3.14 report the same real
birthtimeMson ext4, and it stays put when the folder's ctime moves. - I also had a separate adversarial pass run in a fresh session. I included only the items above that I re-ran myself.
Not verified
- Real no-statx environments (WSL1, some containers). Nit 6 is a simulation.
- macOS APFS and Windows NTFS identity: inode and birthtime semantics,
rsync -amoving birthtime back, and Windowsinovalues above 2^53. I reasoned about these from the body and did not run them. - A remount while the history is open.
whereFoldercomparesdev, but the record deliberately does not. So the open handle would turn "replaced", and a direct CLI command would fail mid-run while the server reopens the same history. I only read this path. - vfat/exfat and overlayfs inode stability, which needs mounts I did not have.
- The two videos. tai sampled them in round 2; I checked only the
fe90b5e5sentence.
— Rames
…w project takes its folder
16c949c to
c905172
Compare
terencecho
left a comment
There was a problem hiding this comment.
Re-reviewed #4537 at c905172c989144257198e44bc88882eefe03665d: APPROVE on code merit. The remaining live-handle recycled-inode hold from my 16c949c0 review is addressed. Engine.whereFolder() now applies the same sameFolder(ino, birthtimeMs) identity rule as the reopen check, in addition to device and project ID. An already-open history whose path is replaced by a copied-marker folder on a recycled inode therefore refuses list() through assertOpen() and restore() through the writable check, rather than reading old entries or restoring old bytes into the replacement. The earlier four filesystem interleavings were replayed on the prior head and remain guarded; this narrow delta does not change their mechanics. Merge-tree against current main is clean.
Test-evidence precision: the added test holds the handle open and changes its folder stat's birthtimeMs at the same path, then verifies list/restore refusal and unchanged bytes. Removing sameFolder makes it red, so it pins the missing comparison. Despite the test's copy/recycled-inode title, it does not physically copy a folder or cause inode reuse; my earlier controlled ext4 witness, plus the now-shared comparison, establish why that case is covered. The prior should-fixes (Studio busy-cache and CLI JSON handling for HistoryClosedError) are not resolved by this delta, but were not the blocking finding.
CI at this exact SHA has a completed successful earlier run (including Studio/support/runtime, build and typecheck); a newer rerun was still settling at review time, with no reported failure. Focused Vitest could not run in this dependency-free isolated worktree (vitest: command not found); I did not rerun the filesystem witness on this head. Rames's independent changes-requested review remains live and only they can clear it; this approval does not claim the PR is merge-ready.
— Review by tai (pr-review)
jrusso1020
left a comment
There was a problem hiding this comment.
Re-review at c905172c, following my CHANGES_REQUESTED at 16c949c0.
Verdict: APPROVE. An open history now checks the folder with the same inode and creation time rule as reopen, and my ext4 repro of the blocker refuses at this head in 9 of 9 runs.
tai approved this head (5326902146), closing its own recycled-inode hold. This head does what both of our reviews asked for: the birthtime in the live check, plus a test that keeps the handle open and checks both list() and restore(). Credit to tai for finding it first. With this approval, neither of us has a change request left on this head.
Prior blocker
Resolved. whereFolder() now calls sameFolder() (projectHistory.ts:328). That is the same helper isRecordedFolder() uses on reopen (historyId.ts:21-22, :27), so the two checks can no longer drift apart.
Every path that checks identity goes through whereFolder():
list,peek,next,readBlobandpinthroughassertOpen()(:349-352).restore,undoandstepthroughwriteAs()andwriteProjectFile()(:827,:850,:858,:862).- The sweep's entry and per-file checks (
:417,:469) and the first baseline (:384). replacedAtPath()(:1032), which the cache uses to decide whether to reopen (historyCache.ts:12-15).
The user-facing path: POST .../history/restore (routes/history.ts:102-104) calls the adapter's history() (vite.adapter.ts:223-224, studioServer.ts:444), which calls historyCache.get(). There, replacedAtPath() is now true for a recycled inode, so the cache closes the old history and reopens with a fresh id.
Repro at this head, through the real ProjectHistory API on ext4 (same script as last round). The steps: open, record "Old change", copy the project with its marker to a backup and edit it, then rm -rf project && cp -r backup project with the handle still open.
- Head: 9 of 9 runs reused the inode on the first attempt. Every run gave
replacedAtPath()true, andlist()andrestore(START)both threwHistoryClosedError. The file keptNEW BACKUP CONTENT. A reopen got a new id and an empty list, so the live and reopen checks now agree. - Control, with the
16c949c0line put back: 6 of 6 runs reused the inode,replacedAtPath()was false,list()returned["Old change"], andrestorewroteOLD BASELINEinto the copy. One more control run got no reuse in 500 attempts, so how often the inode gets reused still varies.
Miguel's claim holds. The new test (projectHistory.swap.test.ts:183-193) goes red when the birthtime is removed from the live check, and also when the whole identity compare is removed. The test fakes the recycle: a mocked statSync (:17-28) shifts birthtimeMs for the same folder instead of copying it. That is enough to pin the compare, and the ext4 run above covers the real sequence.
Prior should-fixes and nits
Should-fix 1, live inode compare not pinned by a test. Partly resolved. The birthtime half is now pinned. The inode half is not. With the inode removed from whereFolder() (:328) or from sameFolder() (historyId.ts:22), all 103 history tests still pass. Because birthtime is now compared, a plain cp -a copy on ext4 is refused even without the inode check. On a file system with no creation time, though, the inode is the only thing left. On this box, tmpfs (kernel 5.15) reports birthtimeMs 0. I ran cp -a project copy, edited the copy, then mv project project-old && mv copy project with the handle open, on tmpfs:
- Head:
replacedAtPath()true,list()andrestore()refuse, and the copy keeps its content. - Inode compare removed:
replacedAtPath()false,list()returns["Old change"], andrestorewritesOLD BASELINEinto the copy.
A second case in the new test would pin it: same birthtime, different ino, from the same mock. I would not hold the approve on this.
Should-fix 2, sweep entry check not pinned. Unchanged. projectHistory.ts:417. Putting back if (!existsSync(this.dir)) return; leaves 103 of 103 green.
Should-fix 3, Studio's dev server caches a HistoryBusyError as null. Unchanged. vite.adapter.ts:115-118. I re-ran the name-swap probe (mv a tmp; mv b a; mv tmp b). GET a answered 404 after 5036 ms, then 404 again after 1 ms. GET b answered 200 with "edit a".
Should-fix 4, hyperframes history --json exits 1 on HistoryClosedError. Unchanged. commands/history.ts:272-280. I swapped the folder while the command waited: exit 1, empty stdout, one stderr line. In the control, where the history is held past the wait, the command exited 2 and printed the JSON refusal.
Nits
- Blob store keeps the other project's bytes after a refused read. Unchanged,
projectHistory.ts:442-451and:468-469. Not re-run, because the code is the same. checkoutchecks onlyexistsSync(:997). Unchanged. RemovingassertOpen()frompeek(:990),next(:1013) orpin(:1021) each still leaves 103 of 103 green.- A move in the middle of a write recreates the old path. Unchanged,
blobStore.ts:28. The body still says writes refuse "instead of recreating the old path". - The body's "A history closed at shutdown is not replaced" is still inaccurate.
historyCache.ts:12is unchanged. - Two processes opening a never-opened project at once. Unchanged (
historyId.ts:36-50,projectHistory.ts:297). Not re-run. - Where birthtime falls back to ctime. Intentionally left, and now disclosed under Known limits. I checked the new text with the same simulation (
birthtimeMspatched toctimeMs) against an open handle. An in-place write kept the history. An atomic save ofindex.htmlthroughreplaceFileAtomically(the helperroutes/files.tsuses) turnedreplacedAtPath()true, andlist()threw. That matches the body. - Body mutation claims. "The inode compare" still survives, in both places (see Should-fix 1). The others I listed last round sit in code and tests this push did not touch. I did not re-run them.
vite.adapter.ts:122still callsvoid histories.closeAll()with no.catch. Unchanged.
- Round 2 video sentence. Resolved. The body now says later commits add checks that do not change the recorded steps.
New findings
Blocker: none.
Should-fix: none.
Nit
- Parts of the body are out of date after this push. "The history records the folder's identity (device and inode)" now also covers the creation time, and the open handle checks it too. The Tests section does not list the new live test. The counts say studio-server
src/history102, but this head has 103, and the full studio-server suite has 798.
Checked
- Delta:
git diff 16c949c0 c905172cis 3 files, +33/-3 (historyId.ts,projectHistory.ts,projectHistory.swap.test.ts). Both heads have the same parent (162de1a6), so the rewrite is an amend and this diff is the whole change. - I read all of
whereFolder(),sameFolder(),isRecordedFolder(), the constructor, and every caller listed above, plushistoryCache.ts, the vite adapter's open and close, andguarded(). - Tests at this head: studio-server
src/history103 passed, 2 skipped. The full studio-server suite: 798 passed, 2 skipped. Studiovite.adapter*: 24. CLIstudioServer.test.tspluscommands/history.test.ts: 60.tsc --noEmitexits 0 in studio-server, studio and cli. - CI at this head: 48 pass, 12 skipped, none failing or pending.
git merge-tree --write-tree 8798e40da8d56b2f0f3be0b74198d0cbbec1dc06 c905172c989144257198e44bc88882eefe03665dis clean (tree69b7a79c).mainis 7 commits ahead of the base, and none of them touch the PR's files.- Reviews and comments: tai's four CHANGES_REQUESTED up to
16c949c0, then tai's APPROVE at this head, and my CHANGES_REQUESTED at16c949c0. No inline or issue comments.
Mutations (studio-server src/history, 103 tests; source restored after each):
| Mutation | Where | Red |
|---|---|---|
Live check back to the 16c949c0 line (dev + inode) |
projectHistory.ts:328 |
1: the new live test |
| Live check with no identity compare (id only) | projectHistory.ts:328 |
1: the new live test |
sameFolder without birthtime |
historyId.ts:22 |
2: the new live test, and "gives a folder on a reused inode, created at another time, a history of its own" |
| Live check without inode (birthtime kept) | projectHistory.ts:328 |
0 |
sameFolder without inode |
historyId.ts:22 |
0 |
Sweep entry check back to existsSync |
projectHistory.ts:417 |
0 |
No assertOpen() in peek, next or pin |
:990, :1013, :1021 |
0 each |
Probes (real modules, scratch dirs):
| Probe | Head | Mutated |
|---|---|---|
| ext4 recycled inode, marker copied, handle open | refused, 9 of 9 | 16c949c0 line: adopted, 6 of 6 |
cp -a copy on a new inode, ext4 |
refused | inode compare removed: refused (birthtime) |
cp -a copy on a new inode, tmpfs (birthtimeMs 0) |
refused | inode compare removed: adopted, restore wrote OLD BASELINE |
Studio name swap (vite.adapter) |
a 404 after 5036 ms, then 404 |
n/a |
history --json, folder swapped during the wait |
exit 1, empty stdout | n/a |
| ctime used as birthtime, atomic root save, handle open | handle closes (disclosed) | n/a |
Not verified
- One history run failed one test, and I did not capture which one. It was my first run right after building the packages. The next 30 runs all passed: 16 one at a time, 2 while
tscran, and 12 with four suites in parallel. - A real no-statx system (WSL1, old kernels, containers that block statx). Nit 6 is a simulation.
- macOS APFS and Windows NTFS identity, including
rsync -amoving birthtime back and Windowsinovalues above 2^53. - Nits 1, 3 and 5 were not re-run. The code under them is unchanged.
- The two videos.
— Rames
What this fixes
Two ways a project's history could reach the wrong project, both rooted in the history being tied to a folder path rather than the folder itself:
close()(an agent turn or edit window left open across a project rename) could still record and restore. If a new project had been created at the old path, its content could be recorded into the old project's history, or the old history restored into it.Now:
close()every call refuses withHistoryClosedError, including windows begun before it.close()is safe to call twice: both calls settle together, and the ownership lock is released only after both.HistoryClosedError, andlist,peek,next,pinandreadBlobrefuse too;.hyperframes/history-idis missing is another project too. A folder deleted and recreated at the same path can reuse the old inode number (ext4 does), so the id is what tells them apart. A project whose id file was removed (for example bygit clean) gets a fresh history on reopen; its old history stays on disk.historyCachein studio-server keeps a host's histories, one per project folder. WhenreplacedAtPath()says another project now stands at the path, it closes the old history (committing what it had already seen) and opens the new project's own. The preview server and Studio's dev server both use it, instead of each caching an engine forever. A history closed at shutdown is not replaced, so a late request does not reopen it.hyperframes historycommands release the history even when the folder changed during the command.What changes for a user
Each existing project starts with an empty history the first time it is opened after this change, because its record predates the inode. The same happens after a backup restore or a move to another disk. The old history is kept on disk, but it no longer shows in Undo.
Otherwise nothing changes unless a project folder is renamed or replaced while the preview or an agent turn is running. Then undo works on the new project at once, instead of failing until restart.
Known limits
Each of these fails closed (a folder starts a fresh history) or leaves only empty folders; none attaches another project's history or writes its file contents:
rsync -aortar) moves its creation time back and closes an open history the same way.Tests
projectHistory.test.ts: every call refuses after close, through a window begun before it, andreadBlobrejects rather than throwing; two concurrent closes both settle before the lock goes; a new project at the path is refused, the old turn still commits what it saw before the swap, and neither project's files change; a folder moved away refuses a restore and the old path is not recreated; a folder whose history id is gone is refused, with its files left alone; a copy put where the original was gets an empty history and the moved original keeps its own, in either opening order; an open that waited for the lock while the folder was moved away or replaced refuses, and so does an open of a folder that is not there; a changed device number keeps the history; a record without an inode gives the folder a history of its own; opening under a second, empty history root leaves the id alone and the open history keeps recording; a history whose folder record was deleted while open writes it back.projectHistory.swap.test.ts: the folder is swapped between two file writes of a restore, between a write's check and its rename, inside a restore's delete, between two copies of a sweep, and between two copies of the first baseline. Each is refused and neither project's files change; the moved original's baseline has only its own bytes. A file deleted during the first baseline is skipped.projectHistory.test.tsalso: a record whose creation time differs gives the folder a history of its own; a copy carrying the id that took the path during the lock wait is refused.studioServer.test.tsand Studio'svite.adapter.projects.test.ts: after the folder is swapped, the next history request answers 200 from the new project's own history.commands/history.test.ts: a command during which the folder is swapped still releases the history.Results: studio-server
src/history102 (2 skipped on Linux),src/routes348; Studiovite.adapter24; CLIstudioServer.test.ts28,commands/history.test.ts32,src/utils859;tscclean in studio-server, studio and cli. Single mutations each fail their test: the closed check in the queue, the inode compare, accepting a record without an inode, rewriting the id for an empty history root, an inode-only compare, no identity check after the lock wait, no check in the first baseline, no check right before the rename, an unchecked asynchronous delete, failing on a file deleted during the first baseline, the missing-id refusal, each of the two write checks, the per-file write check, the sweep's check after each copy, the record write-back, the shared close,readBlobbeing async, the read-side refusal, the open-time id check, the missing-folder refusal at open, a copy keeping the id at the recorded path, the moved original losing its history, a device-number compare, the cache's reopen (both host tests answer 409), each server's retry after a refused open, and the command's close infinally.Before
Studio's dev server on
main: projectdemohas one edit, its folder is renamed away and a new project is created at the same path. After a reload, Undo in the new project offers the old project's edit; clicking it writes the old project's file into the new one ("Old project, edited").before.webm
After
The same steps on this branch (recorded at
fe90b5e5; later commits add the identity and write checks described above, which do not change these steps): the new project opens with Undo disabled and its own empty history, and its file stays "New project".after.webm