Repository navigation
Give utils::fs one stage-and-rename core instead of six writers and a blocking copy #728
Description
[agent] Filed by the scheduled architecture audit routine (CLI and core). Register: discussion #560 register comment.
Kind: refactor (mechanical, no behavior change). Source: review 5.7 and 7.3 "Atomic writes"; register C21.
Problem
On 045d7ec, utils/fs.rs#L434-L691 exports six public atomic writers, which are four boolean policies spelled out as separate functions:
| writer | group-commit capture | fsync + dir fsync | keep mode | durability::record |
|---|---|---|---|---|
atomic_write_bytes |
yes | yes | no | – |
atomic_write_bytes_preserving_mode |
yes | yes | yes | – |
atomic_write_artifact |
no | no | no | yes |
atomic_write_artifact_preserving_mode |
no | no | yes | yes |
atomic_write_unsynced |
no | no | param | no |
atomic_write_sync (blocking) |
no | yes | param | no |
The four async variants also repeat the "read the destination's permissions" prologue.
atomic_write_sync (fs.rs#L590-L641) is a line-for-line blocking copy of stage_and_rename + create_stage + commit_stage (fs.rs#L643-L691). It repeats the stage name format, the Unix mode & 0o777 creation, the set-permissions-before-rename step and the unlink on error. They haven't drifted yet, but every hardening fix must be applied twice. The create_stage mode fix for a secret-bearing .npmrc, for example, exists in both copies only because someone remembered.
There is also a third stage-and-rename implementation: blob_fetcher::write_cache_entry_atomic (blob_fetcher.rs#L451-L503). Its doc comment deliberately makes it lighter (no fsync, a .socket-dl- prefix) and says "do not consolidate into the hardened writer". That policy is legitimate, but it is the same policy as atomic_write_unsynced(…, false) apart from the stage prefix.
Correction to the review: the review said writes bypassing utils::fs "escape group commit". The artifact writers inside utils::fs don't capture either, and nothing under .socket/blobs is inside a group-commit root, so this isn't a defect. Out of scope: the self-update stage (update/download.rs::stage_binary, update/swap.rs), which needs exec bits and its own directory rules.
Impact
Maintenance and hardening drift across three copies of the crash-safety code that every user-owned file write goes through. No behavior change is proposed.
Proposed change
- One private
struct WriteOpts { capture: bool, durable: bool, preserve_mode: bool, record: bool, stage_prefix: &'static str }and onestage_and_rename(path, content, &WriteOpts). - One blocking core:
stage_and_rename_blocking. The async writer runs it underspawn_blocking, or the async version is kept and the blocking one shares thestage_pathandstage_open_optionshelpers. Either way there is one definition of the stage name, the open mode and the set-mode-then-rename order. - Keep the six public names as one-line wrappers, so there's no churn at their ~100 call sites. A later PR may collapse them.
blob_fetcher::write_cache_entry_atomicbecomesfs::atomic_write_cache_entry(durable: false,stage_prefix: ".socket-dl-"). The duplicate stage, rename and cleanup code is deleted fromblob_fetcher.rs.getwrites a patch view's blobs under their claimed hash without checking the content, overwriting already-verified blobs in place #726 then reuses this writer forget's blobs.
Size and scope
utils/fs.rs and api/blob_fetcher.rs: about −90/+50 production lines. No behavior change.
Acceptance criteria
- Only one place in production code builds a
.socket-stage-/.socket-dl-name (grep). - All existing
utils::fstests stay green: the mode-preserving stage creation, the RLIMIT_FSIZE torn-write child test, the group-commit capture and replay tests, and the durability barrier tests. - The
blob_fetcher_edges_e2estage-cleanup tests stay green. - A new unit test asserts that the blocking and async writers produce the same permission bits on a 0600 destination.
Dependencies
None blocking. It pairs with #726 (get blob writer), and either order works: if #726 lands first, it moves write_cache_entry_atomic and this issue just relocates it.
Activity
audit-core: C19/C21 filed (#727, #728), new C42 (#726)
refactor register: re-rank with #726-#728
mikolalysenko commentedon Oct 3, 2026
[agent] Triaged as priority:p3 (mechanical refactor). Pairs with #726 as noted; not clustered since #726 is a correctness fix that does not require this refactor.
Generated by Claude Code
mikolalysenko commentedon Oct 5, 2026
[agent] Claiming this issue for the architecture refactor routine (highest leverage: top eligible queue item; collapses the async and blocking stage-and-rename copies into one core and unblocks #726, and no open PR touches
utils/fs.rs). Branch: arch-refactor/728-stage-rename-core. Claim-ID: 2026-10-05T12:56:02Z-974fbbGenerated by Claude Code
mikolalysenko commentedon Oct 5, 2026
[agent] Draft PR: #858. Since #607 the blob cache writer streams its body and verifies the hash between stage and rename, so it can't become a byte-slice
atomic_write_*wrapper any more; this slice gives it the shared stage-name builder instead, and collapses the async and blockingutils::fswriters into one core.Generated by Claude Code
Share one stage-and-rename core in utils::fs
refactor: #858 for #728 (C21)
Write every utils::fs atomic file through one stage-and-rename core (#…