fix(cli-core): use safe-write for atomic replace, upgrade to 0.2.0 - #989
Merged
Conversation
`fsutil::write_atomic` derived its temp file as a fixed `<path>.tmp`. That
avoided the extension-replacing trap (the doc comment says so explicitly), but
a deterministic name is still shared by every concurrent writer to that path,
and the file was opened with `create(true).truncate(true)` rather than
exclusively. Two writers therefore truncate each other's temp file, and the
first one to rename leaves the second holding a descriptor that now points at
the published target — so the loser writes into the live file and then fails
its own rename with ENOENT.
Measured with four writers racing on one path: 120 of 160 writes failed and 18
of 40 rounds left a target that was neither writer's content.
The workspace already depends on `safe-write` from five other crates, and it
creates a uniquely named temp file, so delegating to it removes the shared name
at the source. Upgrading the workspace pin to 0.2.0 is what makes that
worthwhile: 0.1.x had the extension bug this module was written to avoid, plus
no parent-directory fsync, no cleanup on failure, and a permission reset on
overwrite.
What this changes for the three call sites:
- concurrent writers no longer corrupt or fail. `register_app_in_allowlist`
already held `lock_exclusive`, but `write_state` and `write_token_file` did
not, and both run in the install flow that the module docs describe as
racy.
- a failed write no longer leaves the content behind. `write_token_file`
currently leaves a complete bearer token in `vmm-auth-token.tmp`.
- rewriting an existing file preserves its permissions instead of resetting
them to `0o666 & !umask`.
`lock_exclusive` is untouched. It solves a different problem — serializing a
read-modify-write so two processes cannot each publish a complete file built
from the same stale read — and `safe-write` explicitly does not do that. The
module docs now say so.
The three current call sites all create their parent directory beforehand, and
the allowlist path is validated by the read that precedes the write, so
`safe-write` creating parent directories cannot mask a mistyped path. The only
`mode` in the tree is `0o600`, which is unaffected by the umask.
Two new tests, both confirmed to fail against the previous implementation:
concurrent writers ("No such file or directory") and permission preservation
("widened a credential file to 664").
Contributor
There was a problem hiding this comment.
Pull request overview
This PR fixes a real race/corruption hazard in dstack-cli-core’s atomic file write helper by switching from a deterministic <path>.tmp temp file to the workspace-standard safe-write crate (now pinned to 0.2.0). This improves correctness under concurrent writers and better preserves file permissions on rewrites, which is particularly important for state/allowlist and credential-token writes.
Changes:
- Replace the hand-rolled
write_atomic_innerimplementation withsafe_write::safe_write/safe_write_with_mode. - Clarify module documentation to distinguish “atomic replace” from “read-modify-write serialization” (
lock_exclusive). - Add regression tests for concurrent writers and permission preservation; update existing atomic-write test to assert no temp files remain without relying on a predictable temp name.
Reviewed changes
Copilot reviewed 3 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| dstack/crates/dstack-cli-core/src/fsutil.rs | Delegate atomic replace to safe-write, clarify locking semantics, and add regression tests for concurrency + permission preservation. |
| dstack/crates/dstack-cli-core/Cargo.toml | Add safe-write as a direct dependency for dstack-cli-core. |
| dstack/Cargo.toml | Bump workspace safe-write version from 0.1.3 to 0.2.0. |
| dstack/Cargo.lock | Update lockfile for safe-write 0.2.0 and its dependency changes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
fsutil::write_atomicderives its temp file as a fixed<path>.tmp. That avoids the extension-replacing trap — the doc comment calls it out explicitly — but a deterministic name is still shared by every concurrent writer to that path, and the file is opened withcreate(true).truncate(true)rather than exclusively.So two writers truncate each other's temp file, and the first to rename leaves the second holding a descriptor that now points at the published target — the loser writes into the live file, then fails its own rename with
ENOENT.Measured with four writers racing on one path, using a faithful copy of the current implementation:
This is the failure the module was written to prevent — its own docs note that the auth webhook fails closed on invalid JSON, so a torn allowlist denies keys to every app on the host.
Fix
The workspace already depends on
safe-writefrom five other crates, and it creates a uniquely named temp file, which removes the shared name at the source.write_atomic_inneris replaced by a call to it.Upgrading the workspace pin
0.1.3 → 0.2.0is what makes that worthwhile. 0.1.x had the extension bug this module was written to avoid, plus no parent-directory fsync, no cleanup on failure, and a permission reset on overwrite.What changes for the three call sites
<path>.tmp0o666 & !umaskregister_app_in_allowlistalready heldlock_exclusive, butwrite_stateandwrite_token_filedid not — and both run in the install flow the module docs describe as racy.write_token_filecurrently leaves a complete bearer token invmm-auth-token.tmpif the write fails.What is not changed
lock_exclusiveis untouched. It solves a different problem — serializing a read-modify-write so two processes cannot each publish a complete file built from the same stale read — andsafe-writeexplicitly does not do that. The module docs now state the split.Two behaviours were checked against the actual call sites rather than assumed:
safe-writecreates parent directories. All three call sites create theirs beforehand (install.rs:126-133), and the allowlist path is validated by theread_to_stringthat precedes the write, which is where the friendly "rundstackup installfirst" error comes from. Nothing can mask a mistyped path.safe_write_with_modeapplies the umask, where the old code chmod'd past it. The onlymodein the tree is0o600, which no realistic umask affects.Verification
Two new tests, both confirmed to fail against the previous implementation rather than merely passing:
concurrent_writers_do_not_clobber_each_otherNo such file or directory (os error 2)rewrite_preserves_existing_permissionswidened a credential file to 664atomic_write_replaces_contentsnow asserts the directory is empty apart from the target, since the temp file no longer has a predictable name to check for.16 tests pass in
dstack-cli-coreand 21 indstackup;cargo fmt --checkis clean and clippy reports 0 warnings. All five other crates that depend onsafe-write(dstack-util,kms,gateway,vmm,dstack-attest) were checked against 0.2.0.