Skip to content

fix: make file replacements atomic - #568

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

fix: make file replacements atomic#568
wangl-cc wants to merge 3 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

Summary by Sourcery

集中管理单文件的原子替换,并迁移写入文件的流程,以保护现有内容不被部分或失败的更新覆盖。

新特性:

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

错误修复:

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

改进优化:

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

构建:

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

测试:

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

Summary by Sourcery

Centralize single-file atomic replacement and migrate file-writing paths to protect existing content from partial or failed updates.

New Features:

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

Bug Fixes:

  • Prevent failed or interrupted downloads and other write operations from replacing valid files with partial content.
  • Correct ETag cache sequencing and remove stale validators when successful responses omit an ETag.

Enhancements:

  • Apply atomic replacement consistently to CLI configuration, imports, migrations, Copilot caches, installer downloads, and release metadata.
  • Preserve existing Unix permissions while using normal creation permissions for new files and keeping staging 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, cache freshness, and release metadata.

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 81.44330% with 18 lines in your changes missing coverage. Please review.
✅ Project coverage is 72.71%. Comparing base (dc7d1a9) to head (97c7b2f).
⚠️ 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 66.66% 3 Missing and 4 partials ⚠️
crates/maa-cli/src/config/import.rs 40.00% 2 Missing and 1 partial ⚠️
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.71%   +0.64%     
==========================================
  Files          72       72              
  Lines        6679     6854     +175     
  Branches     6679     6854     +175     
==========================================
+ Hits         4814     4984     +170     
  Misses       1523     1523              
- Partials      342      347       +5     

☔ 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