Skip to content

fix(host-core): replace the transcript file atomically on Windows - #671

Merged
vastsa merged 1 commit into
vastsa:mainfrom
zhangqingkun976:fix/windows-atomic-transcript-swap
Sep 20, 2026
Merged

vastsa merged 1 commit into
vastsa:mainfrom
zhangqingkun976:fix/windows-atomic-transcript-swap

Conversation

@zhangqingkun976

@zhangqingkun976 zhangqingkun976 commented Sep 20, 2026 •

Copy link
Copy Markdown
Contributor

The Windows arm of swap_into_place deleted the target before renaming the temp
file over it:

#[cfg(windows)]
let _ = fs::remove_file(path);
fs::rename(tmp, path)...

Between those two calls the transcript does not exist, and if the rename fails
the transcript is gone while the temp file is left behind — a session can lose
its whole history to a crash or a concurrent reader in that window. The comment
above the function calls this out as D010 ("Windows post-MVP"); this is the
follow-up.

std::fs::rename needs no help here. On Windows it is
MoveFileExW(..., MOVEFILE_REPLACE_EXISTING), which replaces an existing target
in place; on POSIX it is rename(2), which does the same. Measured on
Windows 11 with a probe over the three cases that matter:

rename-over-existing:      ok=true   target="new"
rename-over-open-target:   ok=true   target="new2"   # target held open by a reader
rename-with-locked-source: ok=false  target="live transcript"  # Os { code: 32 }

So the delete was not making room for anything: it was only a window in which the
live file could be lost. Removing it drops the cfg(windows) split entirely —
one fs::rename, the same statement this function already used on POSIX.

Four tests come with it. The one that matters,
swap_failure_on_a_locked_temp_keeps_the_live_transcript, holds the temp file
open with share_mode(FILE_SHARE_READ) so the replacing move cannot proceed, and
asserts that the live transcript survives. Against the old code it fails with

called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound }

— the remove-then-rename had already deleted it.

Verified on Windows 11 on top of ff9bc894 (the touched file is byte-identical
to the base this was written against):

cargo fmt --all -- --check                                            exit 0
cargo test -p host-core --bin pi-desktop-host-core transcripts::      23 passed
cargo clippy -p host-core --all-targets                               one pre-existing
                                                                      warning
                                                                      (user_skills.rs:921,
                                                                      file untouched)

Cargo.toml is untouched: the new test needs no crate feature, because
share_mode is part of std::os::windows::fs::OpenOptionsExt.

For the branch history: an earlier revision of this branch used MoveFileExW /
ReplaceFileW directly and added two windows-sys features. The probe above is
why that came back out — it re-implemented what std::fs::rename already does,
and the actual defect was only the delete.

The Windows arm of `swap_into_place` deleted the target before renaming the temp
file over it:

    #[cfg(windows)]
    let _ = fs::remove_file(path);
    fs::rename(tmp, path)...

Between those two calls the transcript does not exist, and if the rename fails
the transcript is gone while the temp file is left behind — a session can lose
its whole history to a crash or a concurrent reader in that window. The comment
above the function calls this out as D010 ("Windows post-MVP"); this is the
follow-up.

`std::fs::rename` needs no help here. On Windows it is
`MoveFileExW(..., MOVEFILE_REPLACE_EXISTING)`, which replaces an existing target
in place; on POSIX it is `rename(2)`, which does the same. Measured on
Windows 11 with a probe over the three cases that matter:

    rename-over-existing:      ok=true   target="new"
    rename-over-open-target:   ok=true   target="new2"   # target held open by a reader
    rename-with-locked-source: ok=false  target="live transcript"  # Os { code: 32 }

So the delete was not making room for anything: it was only a window in which the
live file could be lost. Removing it drops the `cfg(windows)` split entirely —
one `fs::rename`, the same statement this function already used on POSIX.

Four tests come with it. The one that matters,
`swap_failure_on_a_locked_temp_keeps_the_live_transcript`, holds the temp file
open with `share_mode(FILE_SHARE_READ)` so the replacing move cannot proceed, and
asserts that the live transcript survives. Against the old code it fails with

    called `Result::unwrap()` on an `Err` value: Os { code: 2, kind: NotFound }

— the remove-then-rename had already deleted it.

Verified on Windows 11 on top of `ff9bc894` (the touched file is byte-identical
to the base this was written against):

    cargo fmt --all -- --check                                            exit 0
    cargo test -p host-core --bin pi-desktop-host-core transcripts::      23 passed
    cargo clippy -p host-core --all-targets                               one pre-existing
                                                                          warning
                                                                          (user_skills.rs:921,
                                                                          file untouched)

`Cargo.toml` is untouched: the new test needs no crate feature, because
`share_mode` is part of `std::os::windows::fs::OpenOptionsExt`.

For the branch history: an earlier revision of this branch used `MoveFileExW` /
`ReplaceFileW` directly and added two `windows-sys` features. The probe above is
why that came back out — it re-implemented what `std::fs::rename` already does,
and the actual defect was only the delete.
@zhangqingkun976
zhangqingkun976 force-pushed the fix/windows-atomic-transcript-swap branch from 18a5326 to 3fec0dc Compare September 20, 2026 06:43
@zhangqingkun976

Copy link
Copy Markdown
Contributor Author

Branch updated (force-push to the same branch, still one commit, still one file).

The first revision of this branch wrapped the swap in
MoveFileExW(MOVEFILE_REPLACE_EXISTING) with a ReplaceFileW fallback and two
added windows-sys features. Measuring what std::fs::rename already guarantees
on Windows showed that was redundant, and that the actual defect was only the
preceding remove_file. The branch is now +102/−5 — the deleted line, its
comment, and four tests; the Cargo.toml change and the cfg(windows) split are
gone. The probe output and the verification numbers are in the updated
description.

@vastsa vastsa left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

已逐项审查:移除 Windows 先删目标的窗口,失败时保留旧 transcript 和临时文件,测试覆盖目标存在/不存在/失败及 Windows 锁定场景;未发现原则性问题。可合入。

@vastsa
vastsa merged commit 68c634e into vastsa:main Sep 20, 2026
1 check failed
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