Skip to content

fix(server): failed workspace saves preserve existing files - #32853

Open
ruck-syu wants to merge 1 commit into
pingdotgg:mainfrom
ruck-syu:fix/server-workspace-save-atomic
Open

ruck-syu wants to merge 1 commit into
pingdotgg:mainfrom
ruck-syu:fix/server-workspace-save-atomic

Conversation

@ruck-syu

@ruck-syu ruck-syu commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Problem

A failed save from the Files panel can destroy the existing file. WorkspaceFileSystem.writeFile opened the destination with O_TRUNC, so a partial write followed by EFBIG returned an error after replacing the original contents with an incomplete version. A failed save should preserve the previous file.

Change

Write into a private temporary directory beside the resolved destination, then rename the completed file into place. Check existing write access without truncation, preserve POSIX permission bits, and retain the existing workspace, Git-metadata, and symlink guards. Remove temporary output on failure; log cleanup errors without failing a replacement that has already landed.

This fixes the shared server write path used by clients, including editor autosaves and Markdown task-list changes. No RPC or UI changes are needed.

Atomic replacement requires write permission on the parent directory and space for the replacement alongside the original. It creates a new inode: hard links diverge, ownership/group follow the new file, and ACLs, extended attributes, and special mode bits are not copied. The existing symlink itself is preserved when it resolves inside the workspace. There is no in-place fallback that could reintroduce the reported data loss.

Scope and approval

Submitted under the focused obvious-bug exception: this changes one server save operation and its tests to prevent demonstrated data loss. No prior maintainer approval is claimed.

The duplicate search found no open report for failed workspace saves truncating the destination. #14967 and #24310 concern modes/durability in the separate shared atomic writer; this workspace RPC does not use that helper.

Verification

  • Regression cases call the actual service and inject a real partial filesystem write followed by an error. Before the fix, the original becomes the partial replacement and a failed new-file save leaves a partial destination. After the fix, the original is unchanged and a failed new-file save leaves no destination. The table also covers rename, temporary-directory, and cleanup failures. The existing overwrite test now checks executable permission bits; existing symlink and path-guard cases still pass.
  • vp test run apps/server/src/workspace/WorkspaceFileSystem.test.ts: 33 tests pass.
  • Server typecheck, scoped lint, formatting, and git diff --check pass.
  • vp run check:changed upstream/main on the clean PR branch based on 09a54f01bc: all checks pass, including format, lint, both Knip passes, GraphQL generation, typecheck of 3 affected packages, and 35 tests across 2 files.

Runtime reproduction on macOS (Darwin 27), Node v26.7.0 used a disposable workspace and the real WorkspaceFileSystem layer. Only the known-workspace membership check was stubbed; the failing filesystem operation was real. The parent created an existing file of 4,096 O bytes and a replacement of 4,096 N bytes. A child process called service.writeFile under /bin/bash with ulimit -f 1 (verified as 1 KiB on that shell); the limit applied only to that child. Both versions return WorkspaceFileSystemOperationError, operation write-file, with underlying EFBIG:

Before: destination = 1024 bytes, O-bytes=0,    N-bytes=1024
After:  destination = 4096 bytes, O-bytes=4096, N-bytes=0
After, failed new-file save: destination missing
Controls: unlimited 4096-byte save and limited 100-byte save both succeed

A separate real-service run against an existing writable file in a 0555 parent returned EACCES at temporary-directory creation and preserved the original file. This pins down the writable-parent trade-off above.

Windows, real disk-full/quota exhaustion, and power-loss durability were not tested. No fsync guarantee is added. This server-only fix has no render change, so there are no UI captures.

Implementation: GPT-6.1 Sol in T3 Code.

@github-actions github-actions Bot added vouch:unvouched Author is not yet trusted in the VOUCHED list. size:M 30-99 changed lines (additions + deletions). labels Nov 8, 2026
@macroscopeapp

macroscopeapp Bot commented Nov 8, 2026

Copy link
Copy Markdown
Contributor

Approvability

Verdict: Approved at 19beebf

Macroscope's review found this PR approvable — Workspace saves now stage contents in a temporary file before replacing the original, preserving existing contents when writing fails. Existing workspace restrictions remain, permission bits are retained, and temporary cleanup failures do not turn a completed save into an error.

You can add or adjust custom eligibility rules. Learn more.

@coderabbitai

coderabbitai Bot commented Nov 8, 2026 •

Copy link
Copy Markdown
Contributor

Explain Change →

No actionable comments were generated in the recent review.

Walkthrough

WorkspaceFileSystem.writeFile now writes to a temporary file beside the target, restores existing permission bits where supported, and renames the temporary file into place. Tests cover write, rename, cleanup, and temporary-directory creation failures.

Changes: Workspace file replacement

Layer / File(s) Summary
Temporary-file replacement and validation
apps/server/src/workspace/WorkspaceFileSystem.ts, apps/server/src/workspace/WorkspaceFileSystem.test.ts
writeFile writes to a temporary file and renames it over the target. Tests check write, rename, cleanup, and temporary-directory creation failures, destination contents, and permission preservation.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Change: Bug fix

Suggested reviewers: juliusmarminge, stienswout

Merge Risk: Minimal · up to 8e08d

Failed workspace saves now leave the existing file intact instead of partially overwriting it. No actionable merge-blocking issues remain.

Pre-merge checks: Passed 4

Check name Status Explanation
Title check Passed The title "fix(server): failed workspace saves preserve existing files" follows conventional commit form and names the main change: failed saves in WorkspaceFileSystem.writeFile no longer destroy the …
Description check Passed The description follows the template well. It has Problem, Change, Scope, and Verification sections. The Problem section explains the O_TRUNC data loss with EFBIG. The Change section explains the temp…
Linked Issues check Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check Passed Check skipped because no linked issues were found for this pull request.
Full details: Title check

Explanation

The title "fix(server): failed workspace saves preserve existing files" follows conventional commit form and names the main change: failed saves in WorkspaceFileSystem.writeFile no longer destroy the original file. It matches the summary (temporary-file replacement, cleanup warnings, failure tests) and the objectives (write to temp directory beside destination, rename into place, preserve existing file on failure).

Full details: Description check

Explanation

The description follows the template well. It has Problem, Change, Scope, and Verification sections. The Problem section explains the O_TRUNC data loss with EFBIG. The Change section explains the temp-file rename approach and its trade-offs. The Scope section cites the focused obvious-bug exception and links related issues. The Verification section gives specific test counts, a macOS runtime reproduction with before and after results, and untested areas. The template asks for the model and harness used, and the description includes 'Implementation: GPT-6.1 Sol in T3 Code.' The description is mostly complete, so it passes.

Finishing Touches

Generate unit tests (beta)

  • Create a new PR
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
Recent review info

Run configuration

Commits

Reviewing files that changed from the base of the PR and between fdcabc1 and 8e08dc2.

Files selected for processing: 2

  • apps/server/src/workspace/WorkspaceFileSystem.test.ts
  • apps/server/src/workspace/WorkspaceFileSystem.ts

Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.

@maria-rcks

Copy link
Copy Markdown
Collaborator

Note

Written by gpt-6.1-sol on behalf of Maria

Verified in a real dev environment

verified exact head 19beebfabb4a5b803d5204ac28d0439e728d4b4b on linux using a running dev server, chrome/playwright, and real codex gpt-6.1-sol turns. the project was added through the app and the provider created the verification files in a real git repository. no mocks or seeded app data.

reproduced the bug on github/main (633438d54f) first. with 8,192 bytes free on a disposable 16 mib filesystem, a files-panel autosave of 65,536 replacement bytes failed and destroyed the original 4,096 bytes, leaving 12,288 replacement bytes. the identical autosave on this pr failed while preserving all 4,096 original bytes and removing its temporary output. disk reads establish this result; the editor intentionally retains the unsaved draft.

also verified through the authenticated running server: a failed new-file save returns real ENOSPC and leaves no destination or staging directory; successful new-file saves work; executable overwrites retain 0750; internal symlinks remain intact and update their targets; external symlinks and .git/config writes remain rejected. actual ui editor autosave and rendered markdown checkbox check/uncheck both persisted to disk.

command transcript with before/after byte counts and neighboring checks

real app markdown checkbox checked and persisted to disk

WorkspaceFileSystem.test.ts: all 33 tests passed through blacksmith. the existing head has 31 successful ci checks. no source changes or pushes were needed.

Not verified

native electron and ios/android clients, macos/windows or network filesystems, quota-specific errors, process crash/power-loss durability, and a cleanup failure in the running app. cleanup failure is covered by the focused existing test. this adds no fsync durability guarantee.

@ruck-syu
ruck-syu force-pushed the fix/server-workspace-save-atomic branch from 19beebf to fdcabc1 Compare November 13, 2026 18:29
@ruck-syu
ruck-syu force-pushed the fix/server-workspace-save-atomic branch from fdcabc1 to 8e08dc2 Compare November 14, 2026 13:18

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

size:M 30-99 changed lines (additions + deletions). vouch:unvouched Author is not yet trusted in the VOUCHED list.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants