Skip to content

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 one stage_and_rename(path, content, &WriteOpts).
  • One blocking core: stage_and_rename_blocking. The async writer runs it under spawn_blocking, or the async version is kept and the blocking one shares the stage_path and stage_open_options helpers. 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_atomic becomes fs::atomic_write_cache_entry (durable: false, stage_prefix: ".socket-dl-"). The duplicate stage, rename and cleanup code is deleted from blob_fetcher.rs. get writes a patch view's blobs under their claimed hash without checking the content, overwriting already-verified blobs in place #726 then reuses this writer for get'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::fs tests 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_e2e stage-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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

No one assigned

    Labels

    agent:triagedarch-auditFiled by a scheduled architecture audit routine (see the architecture review discussion)priority:p3refactorStructural change: duplicated code or logic, missing abstraction, layering, dead code

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions