Repository navigation
fix(studio-server): a failed save reports its real error when a later file was never written - #5364
Conversation
… file was never written
Edit accuracy: accurate 2061 (base branch 2061), smooth 1675 of thoseThe gate passes. Quarantined, measured but not gated (0) |
jrusso1020
left a comment
There was a problem hiding this comment.
Approve at df622359. A file that was never written is now restored only when its bytes actually differ from the original. So a persistent write failure reports its real error, and a genuine rollback failure still throws "rollback did not complete".
Checked
- The rule (
applyFileMutations.ts:70-82):- A file whose write completed is still restored only when it holds our bytes. That keeps the earlier rule that a save landing after ours wins.
- A file whose write never completed is restored when its bytes differ from
before. That covers a partial write, and also a writer that renamed and then threw. When it still holdsbefore, nothing is written. beforeis the content read just before the write (:107), so the comparison checks against what was actually on disk.
- Errors aren't swallowed. A failed restore of a written file, or a failed read of an unwritten one (for example, deleted mid-batch), still goes into
rollbackErrors, and the call throws theAggregateError. The original error stays first in the list. Receipts are cleared on every path, as before. - Agrees with #5279:
timeline undonow restores a batch through this sameapplyFileMutationscall (a2Commands.ts:55/246/302), so it takes on the same all-or-nothing rule. Nothing in studio-server or the CLI checks forAggregateErroror its message, so the only visible change is the more accurate error. - The trade-off in the body is accurate. If another program saves the unwritten file while our write is failing, its save is still overwritten, as on main. The window is now narrower, not wider.
- Refactor: pulling out
rollBackand the two type aliases changes no behavior. The.fallowrcentry adds the test file to the same list as its neighbours.
Tests:
applyFileMutations.test.ts9/9 locally (Linux, non-root, so thechmod 555case really ran).- 5 of 5 mutants are caught:
- the old condition
- never restoring an unwritten file
- always restoring a written file
- always restoring an unwritten file
- dropping the receipt clear
CI: every required check is green. Studio: timeline viewport gate is red but not required: its interaction p95 came in at 58.4 ms against a 58.3 ms budget, and this PR touches only the save-rollback helper.
— Rames
…lback-only-written
jrusso1020
left a comment
There was a problem hiding this comment.
Re-approve at 750c003e. This head merges main (2c082b3f) into df622359, which I approved in review 5474382939.
- The PR's three files have the same blob shas at both heads:
.fallowrc.jsonc5f0e5f59,applyFileMutations.ts31bf142b,applyFileMutations.test.ts8808bd72. - Diffing
750c003eagainst itsmainparent shows exactly those three files, with the same +1/0, +26/−16 and +74/−1 as before. There is no conflict-resolution residue. - None of the 102 files that came in from
mainoverlap the PR's files.
My earlier review stands unchanged. This approval waits for the required checks at this head.
— Rames
What
When a multi-file save fails because a later file cannot be written at all (for example its folder is read-only), the error now says what actually happened: the write failed and the earlier files were put back. It no longer claims "rollback did not complete" when every file on disk is in its original state.
Why
applyFileMutations(used by Studio element edits and byhyperframes timelineapply, ids, undo and the single-row edit verbs) rolls back a failed batch by rewriting each attempted file's original bytes. It did that even for the file whose write had just failed and never landed. With a persistent failure, that rewrite fails for the same reason, so the call threw anAggregateError"File mutation failed and rollback did not complete" while disk was consistent: file 1 restored, file 2 untouched. Callers and users were told the project might be half-saved when it was not.Reproduce on main: two files, the second in a folder made read-only with
chmod 555, apply a change to both. Before this change the call throws theAggregateError; after, it throws the originalEACCESand both files hold their original bytes.Related work
Found in review of the timeline undo batch work; independent of it.
How
The rollback now decides per file from what is on disk:
Tradeoff, kept on purpose: when a never-written file's bytes differ from the original, the code cannot tell a writer that failed halfway (an injected non-atomic writer; the existing partial-write test pins restoring that) from another save that landed while the write was failing (on Windows the rename retries for about 100 ms while another program holds the file). It restores the original in both cases, as main did for every unwritten file. The production writer is atomic, so this only matters in that race. Main overwrote unconditionally, so this change narrows that window and does not widen it. Keeping such a save would mean dropping the partial-write guarantee, which is a separate decision.
A real rollback failure (a written file that cannot be restored) still throws "rollback did not complete". The rollback loop moved into its own function to keep
applyFileMutationsunder the complexity gate; the repeated attempted-mutation and writer types each became one alias.Test plan
EACCES, not anAggregateError; file 1 is back to its original bytes, file 2 is untouched, and file 1's write receipt is cleared. Skipped on Windows and as root, wherechmod 555does not block writes. Red on main withexpected AggregateError: File mutation failed and … to not be an instance of AggregateError.