Skip to content

Conversation

grandizzy
Copy link
Collaborator

@grandizzy grandizzy commented Sep 9, 2025

Motivation

  • some tests are writing files using writeFile cheatcode and then reading with readFile cheatcode and assert content
  • encountered race condition on one project where readFile issued after several writes in the same file failed with
├─ [0] VM::keyExists("<JSON file>", ".addresses.accessManager") [staticcall]
    │   └─ ← [Revert] vm.keyExists: failed parsing JSON: EOF while parsing a value at line 1 column 0
    └─ ← [Revert] vm.keyExists: failed parsing JSON: EOF while parsing a value at line 1 column 0

even if traces were showing write just above the read
image

Solution

  • introduce write_sync to write_all and sync_all on cheatcode writes

PR Checklist

  • Added Tests
  • Added Documentation
  • Breaking changes

0xrusowsky
0xrusowsky previously approved these changes Sep 9, 2025
Copy link
Contributor

@0xrusowsky 0xrusowsky left a comment

Choose a reason for hiding this comment

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

i think this makes sense

@zerosnacks
Copy link
Member

Makes sense to me, small nit

zerosnacks
zerosnacks previously approved these changes Sep 10, 2025
Copy link
Collaborator

@onbjerg onbjerg left a comment

Choose a reason for hiding this comment

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

is it important that we flush all data (incl metadata) to disk for this to work? or can we just do File::sync_data?

@grandizzy
Copy link
Collaborator Author

is it important that we flush all data (incl metadata) to disk for this to work? or can we just do File::sync_data?

good point, yeah, I guess we can omit metadata

@DaniPopes
Copy link
Member

DaniPopes commented Sep 10, 2025

this makes sense however i wonder if we should lock the file instead of syncing during cheatcode execution? this is on stable now so you can just call file.lock() for write, .lock_shared() for read, and .unlock() and should avoid fs

@grandizzy
Copy link
Collaborator Author

this makes sense however i wonder if we should lock the file instead of syncing during cheatcode execution? this is on stable now so you can just call file.lock() for write, .lock_shared() for read, and .unlock() and should avoid fs

warning: current MSRV (Minimum Supported Rust Version) is `1.88.0` but this item is stable since `1.89.0`
   --> crates/cheatcodes/src/fs.rs:154:14
    |
154 |         file.lock_shared()?;

should I bump?

@DaniPopes
Copy link
Member

yes

@grandizzy grandizzy marked this pull request as draft September 10, 2025 16:54
@grandizzy
Copy link
Collaborator Author

grandizzy commented Sep 11, 2025

yes

done, please recheck. CI failure due to new node not supporting wss, will ask to open. thanks

@grandizzy grandizzy marked this pull request as ready for review September 11, 2025 08:05
}
}

impl Cheatcode for readFileBinaryCall {
fn apply(&self, state: &mut Cheatcodes) -> Result {
let Self { path } = self;
let path = state.config.ensure_path_allowed(path, FsAccessKind::Read)?;
Ok(fs::read(path)?.abi_encode())
let file = std::fs::OpenOptions::new().read(true).open(path)?;
file.lock_shared()?;
Copy link
Member

Choose a reason for hiding this comment

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

should we move to common::fs module?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yep, will do, also noticed that writeJson is doing let data_string = fs::read_to_string(&data_path)?; for read which should be locked

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

5 participants