Skip to content

fix: fallback behaviour added to atomic writes#5748

Open
suleman1412 wants to merge 13 commits intoprefix-dev:mainfrom
suleman1412:5724-atomic-writes
Open

fix: fallback behaviour added to atomic writes#5748
suleman1412 wants to merge 13 commits intoprefix-dev:mainfrom
suleman1412:5724-atomic-writes

Conversation

@suleman1412
Copy link
Copy Markdown
Contributor

Description

Implemented a fallback behaivour for when the parent directory is not writeable, then falls back to $TMPDIR

Added tests to check :

  1. NamedTempFile behaivour when created in the same as the parent directory
  2. To check the prefix of the generated tempfile
  3. To check the file is created in $TMPDIR when the parent is not writeable, implemented permissions control and tested to check the file is deleted propoerly
    Fixes atomic writes in pixi assume the pixi.toml path is writeable #5724

How Has This Been Tested?

AI Disclosure

  • [ x ] This PR contains AI-generated content.
    • [ x ] I have tested any AI-generated content in my PR.
    • [ x ] I take responsibility for any AI-generated content in my PR.

Tools: Claude

Checklist:

  • [ x ] I have performed a self-review of my own code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • [ x ] I have added sufficient tests to cover my changes.
  • I have verified that changes that would impact the JSON schema have been made in schema/model.py.

@suleman1412 suleman1412 changed the title 5724 atomic writes edit: fallback behaviour added to atomic writes Mar 22, 2026
@suleman1412 suleman1412 changed the title edit: fallback behaviour added to atomic writes fix: fallback behaviour added to atomic writes Mar 22, 2026
Copy link
Copy Markdown
Contributor

@baszalmstra baszalmstra left a comment

Choose a reason for hiding this comment

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

You have to check whether this actually works. Not just of the temp file is in the right directory. The assumption is that if the directory is not writable you still can overwrite the file. We need to check this.

@suleman1412
Copy link
Copy Markdown
Contributor Author

@baszalmstra persist fails in a readonly directory. In such cases we can fallback to using fs:write() instead, which wouldnt be non atomic but should work.

@kratsg
Copy link
Copy Markdown

kratsg commented Mar 23, 2026

@baszalmstra persist fails in a readonly directory. In such cases we can fallback to using fs:write() instead, which wouldnt be non atomic but should work.

Note the previous behavior was fine (I'd imagine the fallback should be to the original behavior that existed in pixi before the PR that introduced atomic writes). Perhaps with a warning to indicate lack of atomicity?

@baszalmstra
Copy link
Copy Markdown
Contributor

Yep, that sounds good to me!

@suleman1412
Copy link
Copy Markdown
Contributor Author

suleman1412 commented Mar 23, 2026

hi @baszalmstra, if directory is readonly then we fallback to use write along with a warning about the loss of atomicity. Addded e2e tests for the same as well, please have a look.

@suleman1412 suleman1412 requested a review from baszalmstra March 23, 2026 18:31
Comment thread crates/pixi_utils/src/atomic_write.rs Outdated
/// the same directory and then renaming it to the target path.
tempfile::Builder::new()
.prefix(&prefix)
.tempfile_in(target_dir)
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.

Cant we just return an error from this function and catch the "permission denied error".

@suleman1412 suleman1412 requested a review from baszalmstra March 29, 2026 10:03
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.

atomic writes in pixi assume the pixi.toml path is writeable

3 participants