Skip to content

Refuse restaging over a dest-gone image leave-temp (SBS-1073) - #300

Draft
btsouth wants to merge 2 commits into
mainfrom
cursor/sbs-1073-restage-leave-temp-0e03
Draft

Refuse restaging over a dest-gone image leave-temp (SBS-1073)#300
btsouth wants to merge 2 commits into
mainfrom
cursor/sbs-1073-restage-leave-temp-0e03

Conversation

@btsouth

@btsouth btsouth commented Aug 23, 2026

Copy link
Copy Markdown
Owner

What changed

Fixes SBS-1073: same-uuid stage_full_image_file used fs::write on the fixed {uuid}.cubby.tmp name. After an SBS-1030 dest-gone mid-replace, that temp is the only remaining original (expired revival retries Capture). fs::write truncates on open, so the retry destroyed those bytes before a durable new copy existed.

Backup already refuses this class of hazard with create_new. Image staging uses a fixed temp name (dest-gone sweep computes dest as name.strip_suffix(".tmp"){uuid}.cubby), so unique names / rename-aside would break SBS-1030 promotion. The matching safe pattern is create_new + conflict failure: restaging leaves the leave-temp byte-identical and fails the recapture. Next launch still promotes the leftover via sweep_stale_image_temps.

write_staged_image_temp lives in image_stage.rs so rustc --test src-tauri/src/image_stage.rs can pin the I/O contract on Linux. stage_full_image_file and the expired-revive recapture path call it.

Sibling overwrite sweep

Production write-then-replace temps:

Path Temp name Write Verdict
image_persist::stage_full_image_file fixed {uuid}.cubby.tmp was fs::write this bug — now create_new
backup::write_backup_temp unique .{pid}.{uuid}.tmp create_new already safe
database rolling backup unique *.bak.{pid}.{uuid}.tmp fs::copy to a unique name collision improbable
crypto storage key unique storage.key.{pid}.{uuid}.tmp fs::write to a unique name collision improbable
settings_manager::save fixed settings.json.tmp fs::write same class: dest-gone leftover + a later save truncates the only remaining preferences. Not changed here (SBS-935 family).

No other writer targets {uuid}.cubby.tmp.

Verification

  • I kept this pull request focused.
  • Windows 11 coverage is the GitHub windows-latest test job (same flags as .github/workflows/ci.yml).
  • I added or updated tests where practical.
  • I updated documentation for changed behavior.
  • I did not include clipboard contents, credentials, signing material, or other secrets.

Fail-without-fix

write_staged_image_temp temporarily implemented as fs::write. rustc --test src-tauri/src/image_stage.rs:

running 4 tests
test tests::restaging_does_not_overwrite_a_dest_gone_leave_temp ... FAILED
test tests::restaging_does_not_overwrite_an_existing_temp_beside_a_live_original ... FAILED
test tests::staging_fails_when_the_temp_name_is_a_directory ... ok
test tests::staging_writes_when_the_temp_name_is_free ... ok

failures:
    tests::restaging_does_not_overwrite_a_dest_gone_leave_temp
    tests::restaging_does_not_overwrite_an_existing_temp_beside_a_live_original

test result: FAILED. 2 passed; 2 failed

The dest-gone case panicked at restaging must refuse to open the existing leave-tempfs::write returned Ok and had already truncated the leave-temp.

After create_new (Linux, rustc --test)

$ rustc --test src-tauri/src/image_stage.rs -D warnings -o /tmp/image_stage_test && /tmp/image_stage_test
running 4 tests
test tests::staging_fails_when_the_temp_name_is_a_directory ... ok
test tests::restaging_does_not_overwrite_a_dest_gone_leave_temp ... ok
test tests::restaging_does_not_overwrite_an_existing_temp_beside_a_live_original ... ok
test tests::staging_writes_when_the_temp_name_is_free ... ok
test result: ok. 4 passed; 0 failed

Crate-level tests in image_persist.rs go through stage_full_image_file (encrypted bytes) and apply_existing_image_recapture on an expired revive so a revert to fs::write there still fails.

Related dest-gone contracts still pass:

$ rustc --test src-tauri/src/settings_load.rs && ./settings_load dest_gone
test tests::image_persist_dest_gone_recovers_instead_of_deleting_the_only_copy ... ok
test tests::dest_gone_rename_does_not_touch_an_existing_destination ... ok
test tests::dest_gone_rename_puts_temp_in_place ... ok
test tests::rolling_backup_dest_gone_recovers_instead_of_deleting_the_only_copy ... ok

CI gate (GitHub windows-latest, run 32670607424, commit a64f6df)

Same commands as .github/workflows/ci.yml:

$ cargo fmt --manifest-path src-tauri/Cargo.toml --check
# exit 0 (test job, 2026-08-23T22:30:15Z)

$ cargo test --manifest-path src-tauri/Cargo.toml --all-targets
# Finished `test` profile in 42.43s
# running 526 tests
test image_persist::tests::restaging_an_expired_revive_does_not_overwrite_the_leave_temp ... ok
test image_persist::tests::restaging_does_not_overwrite_a_dest_gone_leave_temp ... ok
test image_persist::tests::restaging_does_not_overwrite_an_existing_temp_beside_a_live_original ... ok
test image_stage::tests::restaging_does_not_overwrite_a_dest_gone_leave_temp ... ok
test image_stage::tests::restaging_does_not_overwrite_an_existing_temp_beside_a_live_original ... ok
test image_stage::tests::staging_fails_when_the_temp_name_is_a_directory ... ok
test image_stage::tests::staging_writes_when_the_temp_name_is_free ... ok
test result: ok. 524 passed; 0 failed; 2 ignored
test result: ok. 13 passed; 0 failed

$ cargo clippy --manifest-path src-tauri/Cargo.toml --all-targets -- -D warnings
# Finished `dev` profile in 12.08s

All 12 checks on the PR head passed (test, shipped-windows, four Check <arch> <features> legs, CodeQL).

Do not merge.

Open in Web Open in Cursor 

Note

Use exclusive-create for staging temp files in stage_full_image_file

  • Introduces image_stage::write_staged_image_temp, which opens the staging temp path with OpenOptions::create_new(true) and refuses to overwrite an existing file, returning a conflict error with text "refusing to overwrite existing staging file"
  • On write or fsync failure after the file is opened, the helper closes the handle and removes the partial file before returning the error
  • stage_full_image_file now calls this helper instead of std::fs::write, so restaging over a dest-gone leave-temp fails instead of truncating it
  • Adds tests covering the exclusive-create refusal (dest absent, dest live, and expired-revive retry) plus the directory-at-temp-path failure case
  • Behavioral Change: restaging when a staging temp already exists now fails instead of overwriting; callers that previously relied on silent overwrite will receive a conflict error

Macroscope summarized a64f6df.

Same-uuid stage_full_image_file used fs::write on a fixed {uuid}.cubby.tmp,
which truncates an SBS-1030 leave-temp that may be the only remaining original.
Match backup: create_new, conflict on collision, leave those bytes untouched.

Co-authored-by: Tyler <tyler@southboundsoftware.com>
@coderabbitai

coderabbitai Bot commented Aug 23, 2026

Copy link
Copy Markdown

Important

  • 🔍 Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro Plus

Run ID: bd039710-69f7-4f13-b23e-4012d896f2e0


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@cloudflare-workers-and-pages

cloudflare-workers-and-pages Bot commented Aug 23, 2026

Copy link
Copy Markdown

Deploying cubby-clipboard with  Cloudflare Pages  Cloudflare Pages

Latest commit: a64f6df
Status: ✅  Deploy successful!
Preview URL: https://8498bd82.cubby-clipboard.pages.dev
Branch Preview URL: https://cursor-sbs-1073-restage-leav.cubby-clipboard.pages.dev

View logs

unwrap_err and {:?} require Debug. StagedImageFile does not implement it;
match the existing is_err / match-Err pattern so the new tests compile.

Co-authored-by: Tyler <tyler@southboundsoftware.com>
@btsouth
btsouth marked this pull request as ready for review August 23, 2026 22:34
@cursor

cursor Bot commented Aug 23, 2026

Copy link
Copy Markdown

Bugbot couldn't run - usage limit reached

Bugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit.

A user or team admin can review and increase usage limits in the Cursor dashboard.

(requestId: serverGenReqId_e8b48b4a-d3b5-4f04-a447-6ff2a9a38185)

@btsouth

btsouth commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

This is marked ready, but the body still says "Do not merge." I am leaving it blocked until that instruction is explicitly cleared.

@btsouth

btsouth commented Aug 23, 2026

Copy link
Copy Markdown
Owner Author

Manual review is complete. The exclusive-create path preserves the existing temp, cleans a newly created partial write, and the integration coverage pins both live and expired recapture. No code findings. I am still honoring the "Do not merge" line; remove it when ready.

@btsouth
btsouth marked this pull request as draft August 23, 2026 22:45
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