Skip to content

fix(studio): fail closed on backup failures - #3961

Merged
miguel-heygen merged 1 commit into
mainfrom
fix/studio-backup-fail-closed
Sep 15, 2026
Merged

miguel-heygen merged 1 commit into
mainfrom
fix/studio-backup-fail-closed

Conversation

@heygengenesis

@heygengenesis heygengenesis Bot commented Sep 15, 2026

Copy link
Copy Markdown
Contributor

What

Fail closed Studio-server mutations when a backup snapshot reports an error. Related: #3957.

Why

The raw file PUT and DELETE routes, and the shared mutation writer, previously continued after snapshotBeforeWrite() returned backup.error, changing the original without a backup.

How

Return HTTP 500 with the existing { error: "backup failed: ..." } shape before the raw writes/deletion. The shared writer returns that same response and all direct callers propagate it.

Source links

Baseline (e2d60cf27c80849cb6c0fa5c22be34c85b8f6e05):

At resulting HEAD (813024861aa518fc74ae11ab85af2ba17fb6c7ea):

Test plan

  • Unit tests added/updated
  • Manual testing performed
  • Documentation updated (not applicable)

Evidence

Before: the baseline links above show the three backup-error warning paths immediately followed by mutation. The supplied investigation confirmed each route returned HTTP 200 with backupPath: null and changed the original; do not rerun that reproduction.

After: /home/ubuntu/.bun/bin/bun run --cwd packages/studio-server test -- src/routes/files.test.ts --reporter=verbose passed 79/79. The three linked ENOTDIR tests require HTTP 500 and an error beginning backup failed: ENOTDIR:, then verify the original bytes are unchanged. Also passed /home/ubuntu/.bun/bin/bun x oxlint packages/studio-server/src/routes/files.ts packages/studio-server/src/routes/files.test.ts (0 warnings/errors) and /home/ubuntu/.bun/bin/bun run --cwd packages/studio-server typecheck.

Fail closed Studio-server mutations when a backup snapshot reports an error. Return the existing backup-failed HTTP 500 response before any raw or structured mutation can change the original.

Co-authored-by: miguel.sierra <229591595+miguel-heygen@users.noreply.github.com>
@miguel-heygen
miguel-heygen enabled auto-merge (squash) September 15, 2026 18:09
@miguel-heygen
miguel-heygen merged commit cccc78c into main Sep 15, 2026
47 checks passed
@miguel-heygen
miguel-heygen deleted the fix/studio-backup-fail-closed branch September 15, 2026 18:21

@miga-heygen miga-heygen 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.

Post-merge advisory review (this PR is already merged into main). Findings flagged for follow-up, not gating.

Reviewed head 81302486 in an isolated worktree.

Strengths

  • packages/studio-server/src/routes/files.ts:2385-2386 — the PUT guard sits after the version check and before ftruncateSync, inside the try whose finally closes the fd, so the early return leaves the file untouched and the descriptor closed.
  • files.ts:2434-2435 — DELETE now returns before either rmSync or unlinkSync.
  • files.ts:459-464 — the shared writer's | Response return forces every caller to handle the early exit at the type level; tsc --noEmit confirms no caller destructures the union unchecked.

Verified

  • All five writeMutationResult callers (:478, :1316, :2772, :2833, :2972) propagate the Response via instanceof before destructuring. No other callers exist.
  • All snapshotBeforeWrite sites in the file now fail closed: the three fixed here, insert-composition :2502-2503 (same backup failed: shape, 500), split-batch :2658-2664 (500), and the rename helper :368-370 (throws). Status code is consistent at 500 across all of them.
  • Fixtures are deterministic: .hyperframes written as a regular file makes mkdirSync(join(projectDir, ".hyperframes", "backup"), { recursive: true }) in helpers/backupJournal.ts:41 throw ENOTDIR, which is not in the swallowed ENOENT/EISDIR set, so error is always populated.
  • Mutation check: reverting each of the three guards to the old console.warn individually makes exactly its matching test fail (PUT → "fails PUT closed", DELETE → "fails DELETE closed", writer → "fails structured DOM mutations closed"); the other 78 stay green. Each test also asserts the original bytes are unchanged.
  • No behaviour change on the success path: the backupPath / version payloads are built from the same values as before.
  • Local: files.test.ts 79/79, tsc --noEmit clean in packages/studio-server.

Should-be-follow-up-ticket / notes (pre-existing, not introduced here)

  • Directory DELETE (:2436-2437) still proceeds without a backup because snapshotBeforeWrite maps EISDIR to a silent backupPath: null. Same as before this PR; flagging only so it isn't mistaken for covered.
  • split-batch uses Failed to create backup for … while the other routes use backup failed: …. Cosmetic inconsistency, not introduced here.

Verdict: COMMENT (post-merge: Ready)
Reasoning: Every backup-error path in this file now returns 500 before any write or unlink, all callers of the shared writer handle the new early return, and each fix is pinned by a test that fails when the guard is reverted.

— Miga

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.

2 participants