Skip to content

fix: make file replacements atomic - #568

Open
wangl-cc wants to merge 4 commits into
mainfrom
codex/atomic-file-replacements
Open

wangl-cc wants to merge 4 commits into
mainfrom
codex/atomic-file-replacements

Conversation

@wangl-cc

@wangl-cc wangl-cc commented Jul 27, 2026 •

Copy link
Copy Markdown
Member

Summary

Prevent interrupted downloads and failed writes from replacing valid files with partial content, and centralize the single-file replacement contract in a reusable workspace crate.

Changes

  • add maa-atomic-fs for same-directory staging, file synchronization, and atomic replacement of existing destinations
  • preserve existing Unix permission bits and use normal umask-filtered creation permissions for new write-like files while keeping staging files private during writes
  • migrate maa-cli configuration, import, migration, and Copilot cache writes to the shared helper
  • commit ETag response bodies before updating validators, preserve the previous cache on transfer failure, and remove stale sidecars when a 200 response omits ETag
  • keep 304 and check-interval behavior unchanged, including timestamp refresh handling on Windows
  • atomically replace generated release JSON and shell metadata
  • add regression coverage for failed transfers, ETag ordering and removal, conditional requests, freshness checks, permissions, and replacement behavior

The helper intentionally does not provide multi-file transactions, writer locking, metadata preservation beyond documented Unix permission bits, or parent-directory synchronization. Windows replacement uses the overwrite-capable persistence path from tempfile, covered by the Windows CI jobs.

Validation

  • cargo +nightly fmt --check
  • affected crate Clippy checks with warnings denied
  • cargo test -p maa-atomic-fs (9 passed)
  • cargo test -p maa-installer --features ureq/json (65 passed)
  • cargo test -p xtask (1 passed)
  • cargo x test --no-core-tests
  • design, adversarial, and patch reviews

由 Sourcery 生成的摘要

在新的 maa-atomic-fs crate 中集中实现原子化的单文件替换,并将其应用到下载、配置写入、缓存文件和发布元数据,以在失败时避免破坏已有文件,同时保留预期的权限和 ETag 语义。

新增功能:

  • 引入工作区 crate maa-atomic-fs,提供可复用的原子化单文件写入、拷贝和暂存工具

缺陷修复:

  • 确保下载和配置导入在内容部分写入或失败时,不会用不完整的内容覆盖现有文件
  • 修复 ETag 缓存处理逻辑,确保在更新验证器(validator)之前先提交文件内容,并在响应缺少 ETag 时删除过期的 ETag 边车文件(sidecar)

改进:

  • 在 maa-cli 配置、迁移、Copilot 缓存写入、安装程序下载以及 xtask 发布元数据生成中,统一采用共享的原子化文件替换辅助工具
  • 对已有文件保留其 Unix 权限位,对新的原子化写入使用正常的创建权限

测试:

  • 新增回归测试,覆盖原子替换行为、权限、失败传输、ETag 顺序与移除、条件请求、缓存新鲜度检查以及发布元数据替换等场景
Original summary in English

Sourcery 摘要

集中实现单文件原子替换,并将其应用于文件下载、应用程序数据、缓存和版本发布元数据,以便在写入失败时保留有效内容。

新功能:

  • 新增可复用的 maa-atomic-fs 工作区 crate,用于执行单文件的原子写入、复制和替换。

错误修复:

  • 防止中断的下载和失败的写入使用部分内容替换有效文件。
  • 修正 ETag 缓存更新顺序,并在成功响应未包含 ETag 时移除过时的 ETag sidecar 文件。

改进:

  • 将原子替换一致地应用于 CLI 配置和导入、Copilot 缓存、安装程序下载以及版本发布元数据。
  • 保留现有的 Unix 权限位;对于新文件使用标准创建权限,同时确保暂存文件在写入期间保持私有。

构建:

  • 添加 maa-atomic-fs crate 和工作区依赖项。

测试:

  • 增加针对原子替换、权限、失败的传输、ETag 行为、条件请求、新鲜度检查和版本发布元数据的回归测试覆盖。
Original summary in English

Summary by Sourcery

Centralize single-file atomic replacement and use it across file downloads, application data, caches, and release metadata to preserve valid content when writes fail.

New Features:

  • Add the reusable maa-atomic-fs workspace crate for atomic single-file writes, copies, and replacements.

Bug Fixes:

  • Prevent interrupted downloads and failed writes from replacing valid files with partial content.
  • Correct ETag cache update ordering and remove stale ETag sidecars when successful responses omit an ETag.

Enhancements:

  • Apply atomic replacement consistently to CLI configuration and imports, Copilot caches, installer downloads, and release metadata.
  • Preserve existing Unix permission bits and use standard creation permissions for new files while keeping staged files private during writes.

Build:

  • Add the maa-atomic-fs crate and workspace dependencies.

Tests:

  • Add regression coverage for atomic replacement, permissions, failed transfers, ETag behavior, conditional requests, freshness checks, and release metadata.

新特性:

  • 新增可复用的 workspace crate,用于原子化的单文件写入、复制和替换。

错误修复:

  • 防止下载失败或中断以及其他写入操作,用部分内容替换原本有效的文件。
  • 修正 ETag 缓存顺序,并在成功响应不包含 ETag 时移除过期的验证器。

改进优化:

  • 将原子替换一致地应用于 CLI 配置、导入、迁移、Copilot 缓存、安装器下载以及发布元数据。
  • 在写入过程中保持现有 Unix 权限,同时对新文件使用正常的创建权限,并确保暂存文件在写入期间保持私有。

构建:

  • 添加 maa-atomic-fs crate 以及相关 workspace 依赖。

测试:

  • 增加针对原子替换、权限、失败传输、ETag 行为、条件请求、缓存新鲜度以及发布元数据的回归测试覆盖。
Original summary in English

Stage single-file writes beside their destinations and preserve Unix creation permissions. Commit ETag-backed downloads before updating validators so failed transfers keep the previous cache usable.
Copilot AI review requested due to automatic review settings July 27, 2026 16:01

@sourcery-ai sourcery-ai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hey - 我已经审查了你的更改,看起来非常棒!


Sourcery 对开源项目是免费的——如果你喜欢我们的评审,请考虑分享它们 ✨
帮我变得更有用!请在每条评论上点击 👍 或 👎,我会根据你的反馈改进以后的评审。
Original comment in English

Hey - I've reviewed your changes and they look great!


Sourcery is free for open source - if you like our reviews please consider sharing them ✨
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.

@codecov

codecov Bot commented Jul 27, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 82.47423% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.93%. Comparing base (dc7d1a9) to head (4c90d76).
⚠️ Report is 2 commits behind head on main.

Files with missing lines Patch % Lines
crates/maa-atomic-fs/src/lib.rs 89.70% 1 Missing and 6 partials ⚠️
crates/maa-installer/src/download/etag.rs 71.42% 3 Missing and 3 partials ⚠️
crates/maa-cli/src/config/import.rs 40.00% 3 Missing ⚠️
crates/maa-cli/src/run/preset/copilot.rs 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
+ Coverage   72.07%   72.93%   +0.85%     
==========================================
  Files          72       72              
  Lines        6679     6809     +130     
  Branches     6679     6809     +130     
==========================================
+ Hits         4814     4966     +152     
- Misses       1523     1545      +22     
+ Partials      342      298      -44     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@wangl-cc

Copy link
Copy Markdown
Member Author

@codex review

Copilot AI 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.

Pull request overview

This PR centralizes atomic single-file replacement into a new workspace crate (maa-atomic-fs) and adopts it across the CLI, installer download cache, and release metadata generation to avoid corrupting existing files on interrupted downloads or failed writes.

Changes:

  • Added maa-atomic-fs providing same-directory staging, fsync, and atomic replacement with Unix permission handling.
  • Updated maa-installer ETag download path to commit bodies before validators and to remove stale ETag sidecars when omitted.
  • Migrated maa-cli config/import/migration/Copilot cache writes and xtask release metadata writes to the shared helper (removing the old CLI-local atomic fs module).

Reviewed changes

Copilot reviewed 14 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
xtask/src/release/package.rs Writes release JSON and shell metadata via maa-atomic-fs and adds regression coverage for replacements.
xtask/Cargo.toml Adds maa-atomic-fs dependency to xtask.
crates/maa-installer/src/download/etag.rs Uses atomic writes for cached downloads/ETags; adjusts ETag sidecar update/removal and timestamp refresh logic; adds regression tests.
crates/maa-installer/Cargo.toml Adds maa-atomic-fs dependency to installer crate.
crates/maa-cli/src/run/preset/copilot.rs Switches Copilot cache writes to maa-atomic-fs.
crates/maa-cli/src/main.rs Removes the old atomic_fs module wiring.
crates/maa-cli/src/config/mod.rs Uses maa-atomic-fs for config file writes.
crates/maa-cli/src/config/import.rs Uses maa-atomic-fs for imported config writes/copies.
crates/maa-cli/src/config/asst.rs Uses maa-atomic-fs for legacy tasks migration copy.
crates/maa-cli/src/atomic_fs.rs Removes the CLI-local atomic fs implementation (superseded by shared crate).
crates/maa-cli/Cargo.toml Adds maa-atomic-fs dependency to CLI crate.
crates/maa-atomic-fs/src/lib.rs Introduces the shared atomic file replacement implementation and unit tests.
crates/maa-atomic-fs/Cargo.toml Defines the new maa-atomic-fs crate.
Cargo.toml Registers maa-atomic-fs as a workspace dependency.
Cargo.lock Locks the new crate into the workspace dependency graph.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread crates/maa-installer/src/download/etag.rs
Copilot AI review requested due to automatic review settings July 27, 2026 17:07

Copilot AI 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.

Pull request overview

Copilot reviewed 14 out of 15 changed files in this pull request and generated no new comments.

Comments suppressed due to low confidence (1)

crates/maa-installer/src/download/etag.rs:87

  • The trace log in the 304/Not Modified branch says it "Touched" the destination file, but the code actually updates the mtime on the ETag sidecar file. This can make debugging cache freshness behavior confusing when reading logs.
        StatusCode::NOT_MODIFIED => {
            log::trace!("File {} is up to date", dest.display());
            if set_modified(&etag_file, time::SystemTime::now()).is_ok() {
                log::trace!("Touched {}", dest.display());
            }

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