dirstate: deal with read-race for pure rust code path (rhg)

If we cannot read the dirstate data, this is probably because a writing process
wrote it under our feet. So refresh the docket and try again a handful of time.

rust/hg-core/src/errors.rs

@@ -1,214 +1,220 b''
 use crate::config::ConfigValueParseError;
 use crate::exit_codes;
 use std::fmt;
 
 /// Common error cases that can happen in many different APIs
 #[derive(Debug, derive_more::From)]
 pub enum HgError {
     IoError {
         error: std::io::Error,
         context: IoErrorContext,
     },
 
     /// A file under `.hg/` normally only written by Mercurial is not in the
     /// expected format. This indicates a bug in Mercurial, filesystem
     /// corruption, or hardware failure.
     ///
     /// The given string is a short explanation for users, not intended to be
     /// machine-readable.
     CorruptedRepository(String),
 
     /// The respository or requested operation involves a feature not
     /// supported by the Rust implementation. Falling back to the Python
     /// implementation may or may not work.
     ///
     /// The given string is a short explanation for users, not intended to be
     /// machine-readable.
     UnsupportedFeature(String),
 
     /// Operation cannot proceed for some other reason.
     ///
     /// The message is a short explanation for users, not intended to be
     /// machine-readable.
     Abort {
         message: String,
         detailed_exit_code: exit_codes::ExitCode,
         hint: Option<String>,
     },
 
     /// A configuration value is not in the expected syntax.
     ///
     /// These errors can happen in many places in the code because values are
     /// parsed lazily as the file-level parser does not know the expected type
     /// and syntax of each value.
     #[from]
     ConfigValueParseError(ConfigValueParseError),
 
     /// Censored revision data.
     CensoredNodeError,
+    /// A race condition has been detected. This *must* be handled locally
+    /// and not directly surface to the user.
+    RaceDetected(String),
 }
 
 /// Details about where an I/O error happened
 #[derive(Debug)]
 pub enum IoErrorContext {
     /// `std::fs::metadata`
     ReadingMetadata(std::path::PathBuf),
     ReadingFile(std::path::PathBuf),
     WritingFile(std::path::PathBuf),
     RemovingFile(std::path::PathBuf),
     RenamingFile {
         from: std::path::PathBuf,
         to: std::path::PathBuf,
     },
     /// `std::fs::canonicalize`
     CanonicalizingPath(std::path::PathBuf),
     /// `std::env::current_dir`
     CurrentDir,
     /// `std::env::current_exe`
     CurrentExe,
 }
 
 impl HgError {
     pub fn corrupted(explanation: impl Into<String>) -> Self {
         // TODO: capture a backtrace here and keep it in the error value
         // to aid debugging?
         // https://doc.rust-lang.org/std/backtrace/struct.Backtrace.html
         HgError::CorruptedRepository(explanation.into())
     }
 
     pub fn unsupported(explanation: impl Into<String>) -> Self {
         HgError::UnsupportedFeature(explanation.into())
     }
 
     pub fn abort(
         explanation: impl Into<String>,
         exit_code: exit_codes::ExitCode,
         hint: Option<String>,
     ) -> Self {
         HgError::Abort {
             message: explanation.into(),
             detailed_exit_code: exit_code,
             hint,
         }
     }
 }
 
 // TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
 impl fmt::Display for HgError {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             HgError::Abort { message, .. } => write!(f, "{}", message),
             HgError::IoError { error, context } => {
                 write!(f, "abort: {}: {}", context, error)
             }
             HgError::CorruptedRepository(explanation) => {
                 write!(f, "abort: {}", explanation)
             }
             HgError::UnsupportedFeature(explanation) => {
                 write!(f, "unsupported feature: {}", explanation)
             }
             HgError::CensoredNodeError => {
                 write!(f, "encountered a censored node")
             }
             HgError::ConfigValueParseError(error) => error.fmt(f),
+            HgError::RaceDetected(context) => {
+                write!(f, "encountered a race condition {context}")
+            }
         }
     }
 }
 
 // TODO: use `DisplayBytes` instead to show non-Unicode filenames losslessly?
 impl fmt::Display for IoErrorContext {
     fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
         match self {
             IoErrorContext::ReadingMetadata(path) => {
                 write!(f, "when reading metadata of {}", path.display())
             }
             IoErrorContext::ReadingFile(path) => {
                 write!(f, "when reading {}", path.display())
             }
             IoErrorContext::WritingFile(path) => {
                 write!(f, "when writing {}", path.display())
             }
             IoErrorContext::RemovingFile(path) => {
                 write!(f, "when removing {}", path.display())
             }
             IoErrorContext::RenamingFile { from, to } => write!(
                 f,
                 "when renaming {} to {}",
                 from.display(),
                 to.display()
             ),
             IoErrorContext::CanonicalizingPath(path) => {
                 write!(f, "when canonicalizing {}", path.display())
             }
             IoErrorContext::CurrentDir => {
                 write!(f, "error getting current working directory")
             }
             IoErrorContext::CurrentExe => {
                 write!(f, "error getting current executable")
             }
         }
     }
 }
 
 pub trait IoResultExt<T> {
     /// Annotate a possible I/O error as related to a reading a file at the
     /// given path.
     ///
     /// This allows printing something like “File not found when reading
     /// example.txt” instead of just “File not found”.
     ///
     /// Converts a `Result` with `std::io::Error` into one with `HgError`.
     fn when_reading_file(self, path: &std::path::Path) -> Result<T, HgError>;
 
     fn when_writing_file(self, path: &std::path::Path) -> Result<T, HgError>;
 
     fn with_context(
         self,
         context: impl FnOnce() -> IoErrorContext,
     ) -> Result<T, HgError>;
 }
 
 impl<T> IoResultExt<T> for std::io::Result<T> {
     fn when_reading_file(self, path: &std::path::Path) -> Result<T, HgError> {
         self.with_context(|| IoErrorContext::ReadingFile(path.to_owned()))
     }
 
     fn when_writing_file(self, path: &std::path::Path) -> Result<T, HgError> {
         self.with_context(|| IoErrorContext::WritingFile(path.to_owned()))
     }
 
     fn with_context(
         self,
         context: impl FnOnce() -> IoErrorContext,
     ) -> Result<T, HgError> {
         self.map_err(|error| HgError::IoError {
             error,
             context: context(),
         })
     }
 }
 
 pub trait HgResultExt<T> {
     /// Handle missing files separately from other I/O error cases.
     ///
     /// Wraps the `Ok` type in an `Option`:
     ///
     /// * `Ok(x)` becomes `Ok(Some(x))`
     /// * An I/O "not found" error becomes `Ok(None)`
     /// * Other errors are unchanged
     fn io_not_found_as_none(self) -> Result<Option<T>, HgError>;
 }
 
 impl<T> HgResultExt<T> for Result<T, HgError> {
     fn io_not_found_as_none(self) -> Result<Option<T>, HgError> {
         match self {
             Ok(x) => Ok(Some(x)),
             Err(HgError::IoError { error, .. })
                 if error.kind() == std::io::ErrorKind::NotFound =>
             {
                 Ok(None)
             }
             Err(other_error) => Err(other_error),
         }
     }
 }

rust/hg-core/src/repo.rs

@@ -1,619 +1,688 b''
 use crate::changelog::Changelog;
 use crate::config::{Config, ConfigError, ConfigParseError};
 use crate::dirstate::DirstateParents;
 use crate::dirstate_tree::dirstate_map::DirstateMapWriteMode;
 use crate::dirstate_tree::on_disk::Docket as DirstateDocket;
 use crate::dirstate_tree::owning::OwningDirstateMap;
 use crate::errors::HgResultExt;
 use crate::errors::{HgError, IoResultExt};
 use crate::lock::{try_with_lock_no_wait, LockError};
 use crate::manifest::{Manifest, Manifestlog};
 use crate::revlog::filelog::Filelog;
 use crate::revlog::revlog::RevlogError;
 use crate::utils::debug::debug_wait_for_file_or_print;
 use crate::utils::files::get_path_from_bytes;
 use crate::utils::hg_path::HgPath;
 use crate::utils::SliceExt;
 use crate::vfs::{is_dir, is_file, Vfs};
 use crate::{requirements, NodePrefix};
 use crate::{DirstateError, Revision};
 use std::cell::{Ref, RefCell, RefMut};
 use std::collections::HashSet;
 use std::io::Seek;
 use std::io::SeekFrom;
 use std::io::Write as IoWrite;
 use std::path::{Path, PathBuf};
 
+const V2_MAX_READ_ATTEMPTS: usize = 5;
+
 /// A repository on disk
 pub struct Repo {
     working_directory: PathBuf,
     dot_hg: PathBuf,
     store: PathBuf,
     requirements: HashSet<String>,
     config: Config,
     dirstate_parents: LazyCell<DirstateParents>,
     dirstate_data_file_uuid: LazyCell<Option<Vec<u8>>>,
     dirstate_map: LazyCell<OwningDirstateMap>,
     changelog: LazyCell<Changelog>,
     manifestlog: LazyCell<Manifestlog>,
 }
 
 #[derive(Debug, derive_more::From)]
 pub enum RepoError {
     NotFound {
         at: PathBuf,
     },
     #[from]
     ConfigParseError(ConfigParseError),
     #[from]
     Other(HgError),
 }
 
 impl From<ConfigError> for RepoError {
     fn from(error: ConfigError) -> Self {
         match error {
             ConfigError::Parse(error) => error.into(),
             ConfigError::Other(error) => error.into(),
         }
     }
 }
 
 impl Repo {
     /// tries to find nearest repository root in current working directory or
     /// its ancestors
     pub fn find_repo_root() -> Result<PathBuf, RepoError> {
         let current_directory = crate::utils::current_dir()?;
         // ancestors() is inclusive: it first yields `current_directory`
         // as-is.
         for ancestor in current_directory.ancestors() {
             if is_dir(ancestor.join(".hg"))? {
                 return Ok(ancestor.to_path_buf());
             }
         }
         return Err(RepoError::NotFound {
             at: current_directory,
         });
     }
 
     /// Find a repository, either at the given path (which must contain a `.hg`
     /// sub-directory) or by searching the current directory and its
     /// ancestors.
     ///
     /// A method with two very different "modes" like this usually a code smell
     /// to make two methods instead, but in this case an `Option` is what rhg
     /// sub-commands get from Clap for the `-R` / `--repository` CLI argument.
     /// Having two methods would just move that `if` to almost all callers.
     pub fn find(
         config: &Config,
         explicit_path: Option<PathBuf>,
     ) -> Result<Self, RepoError> {
         if let Some(root) = explicit_path {
             if is_dir(root.join(".hg"))? {
                 Self::new_at_path(root.to_owned(), config)
             } else if is_file(&root)? {
                 Err(HgError::unsupported("bundle repository").into())
             } else {
                 Err(RepoError::NotFound {
                     at: root.to_owned(),
                 })
             }
         } else {
             let root = Self::find_repo_root()?;
             Self::new_at_path(root, config)
         }
     }
 
     /// To be called after checking that `.hg` is a sub-directory
     fn new_at_path(
         working_directory: PathBuf,
         config: &Config,
     ) -> Result<Self, RepoError> {
         let dot_hg = working_directory.join(".hg");
 
         let mut repo_config_files = Vec::new();
         repo_config_files.push(dot_hg.join("hgrc"));
         repo_config_files.push(dot_hg.join("hgrc-not-shared"));
 
         let hg_vfs = Vfs { base: &dot_hg };
         let mut reqs = requirements::load_if_exists(hg_vfs)?;
         let relative =
             reqs.contains(requirements::RELATIVE_SHARED_REQUIREMENT);
         let shared =
             reqs.contains(requirements::SHARED_REQUIREMENT) || relative;
 
         // From `mercurial/localrepo.py`:
         //
         // if .hg/requires contains the sharesafe requirement, it means
         // there exists a `.hg/store/requires` too and we should read it
         // NOTE: presence of SHARESAFE_REQUIREMENT imply that store requirement
         // is present. We never write SHARESAFE_REQUIREMENT for a repo if store
         // is not present, refer checkrequirementscompat() for that
         //
         // However, if SHARESAFE_REQUIREMENT is not present, it means that the
         // repository was shared the old way. We check the share source
         // .hg/requires for SHARESAFE_REQUIREMENT to detect whether the
         // current repository needs to be reshared
         let share_safe = reqs.contains(requirements::SHARESAFE_REQUIREMENT);
 
         let store_path;
         if !shared {
             store_path = dot_hg.join("store");
         } else {
             let bytes = hg_vfs.read("sharedpath")?;
             let mut shared_path =
                 get_path_from_bytes(bytes.trim_end_matches(|b| b == b'\n'))
                     .to_owned();
             if relative {
                 shared_path = dot_hg.join(shared_path)
             }
             if !is_dir(&shared_path)? {
                 return Err(HgError::corrupted(format!(
                     ".hg/sharedpath points to nonexistent directory {}",
                     shared_path.display()
                 ))
                 .into());
             }
 
             store_path = shared_path.join("store");
 
             let source_is_share_safe =
                 requirements::load(Vfs { base: &shared_path })?
                     .contains(requirements::SHARESAFE_REQUIREMENT);
 
             if share_safe != source_is_share_safe {
                 return Err(HgError::unsupported("share-safe mismatch").into());
             }
 
             if share_safe {
                 repo_config_files.insert(0, shared_path.join("hgrc"))
             }
         }
         if share_safe {
             reqs.extend(requirements::load(Vfs { base: &store_path })?);
         }
 
         let repo_config = if std::env::var_os("HGRCSKIPREPO").is_none() {
             config.combine_with_repo(&repo_config_files)?
         } else {
             config.clone()
         };
 
         let repo = Self {
             requirements: reqs,
             working_directory,
             store: store_path,
             dot_hg,
             config: repo_config,
             dirstate_parents: LazyCell::new(),
             dirstate_data_file_uuid: LazyCell::new(),
             dirstate_map: LazyCell::new(),
             changelog: LazyCell::new(),
             manifestlog: LazyCell::new(),
         };
 
         requirements::check(&repo)?;
 
         Ok(repo)
     }
 
     pub fn working_directory_path(&self) -> &Path {
         &self.working_directory
     }
 
     pub fn requirements(&self) -> &HashSet<String> {
         &self.requirements
     }
 
     pub fn config(&self) -> &Config {
         &self.config
     }
 
     /// For accessing repository files (in `.hg`), except for the store
     /// (`.hg/store`).
     pub fn hg_vfs(&self) -> Vfs<'_> {
         Vfs { base: &self.dot_hg }
     }
 
     /// For accessing repository store files (in `.hg/store`)
     pub fn store_vfs(&self) -> Vfs<'_> {
         Vfs { base: &self.store }
     }
 
     /// For accessing the working copy
     pub fn working_directory_vfs(&self) -> Vfs<'_> {
         Vfs {
             base: &self.working_directory,
         }
     }
 
     pub fn try_with_wlock_no_wait<R>(
         &self,
         f: impl FnOnce() -> R,
     ) -> Result<R, LockError> {
         try_with_lock_no_wait(self.hg_vfs(), "wlock", f)
     }
 
     pub fn has_dirstate_v2(&self) -> bool {
         self.requirements
             .contains(requirements::DIRSTATE_V2_REQUIREMENT)
     }
 
     pub fn has_sparse(&self) -> bool {
         self.requirements.contains(requirements::SPARSE_REQUIREMENT)
     }
 
     pub fn has_narrow(&self) -> bool {
         self.requirements.contains(requirements::NARROW_REQUIREMENT)
     }
 
     pub fn has_nodemap(&self) -> bool {
         self.requirements
             .contains(requirements::NODEMAP_REQUIREMENT)
     }
 
     fn dirstate_file_contents(&self) -> Result<Vec<u8>, HgError> {
         Ok(self
             .hg_vfs()
             .read("dirstate")
             .io_not_found_as_none()?
             .unwrap_or(Vec::new()))
     }
 
     pub fn dirstate_parents(&self) -> Result<DirstateParents, HgError> {
         Ok(*self
             .dirstate_parents
             .get_or_init(|| self.read_dirstate_parents())?)
     }
 
     fn read_dirstate_parents(&self) -> Result<DirstateParents, HgError> {
         let dirstate = self.dirstate_file_contents()?;
         let parents = if dirstate.is_empty() {
             if self.has_dirstate_v2() {
                 self.dirstate_data_file_uuid.set(None);
             }
             DirstateParents::NULL
         } else if self.has_dirstate_v2() {
             let docket =
                 crate::dirstate_tree::on_disk::read_docket(&dirstate)?;
             self.dirstate_data_file_uuid
                 .set(Some(docket.uuid.to_owned()));
             docket.parents()
         } else {
             crate::dirstate::parsers::parse_dirstate_parents(&dirstate)?
                 .clone()
         };
         self.dirstate_parents.set(parents);
         Ok(parents)
     }
 
     fn read_dirstate_data_file_uuid(
         &self,
     ) -> Result<Option<Vec<u8>>, HgError> {
         assert!(
             self.has_dirstate_v2(),
             "accessing dirstate data file ID without dirstate-v2"
         );
         let dirstate = self.dirstate_file_contents()?;
         if dirstate.is_empty() {
             self.dirstate_parents.set(DirstateParents::NULL);
             Ok(None)
         } else {
             let docket =
                 crate::dirstate_tree::on_disk::read_docket(&dirstate)?;
             self.dirstate_parents.set(docket.parents());
             Ok(Some(docket.uuid.to_owned()))
         }
     }
 
     fn new_dirstate_map(&self) -> Result<OwningDirstateMap, DirstateError> {
         if self.has_dirstate_v2() {
-            self.read_docket_and_data_file()
+            // The v2 dirstate is split into a docket and a data file.
+            // Since we don't always take the `wlock` to read it
+            // (like in `hg status`), it is susceptible to races.
+            // A simple retry method should be enough since full rewrites
+            // only happen when too much garbage data is present and
+            // this race is unlikely.
+            let mut tries = 0;
+
+            while tries < V2_MAX_READ_ATTEMPTS {
+                tries += 1;
+                match self.read_docket_and_data_file() {
+                    Ok(m) => {
+                        return Ok(m);
+                    }
+                    Err(e) => match e {
+                        DirstateError::Common(HgError::RaceDetected(
+                            context,
+                        )) => {
+                            log::info!(
+                                "dirstate read race detected {} (retry {}/{})",
+                                context,
+                                tries,
+                                V2_MAX_READ_ATTEMPTS,
+                            );
+                            continue;
+                        }
+                        _ => return Err(e.into()),
+                    },
+                }
+            }
+            let error = HgError::abort(
+                format!("dirstate read race happened {tries} times in a row"),
+                255,
+                None,
+            );
+            return Err(DirstateError::Common(error));
         } else {
             debug_wait_for_file_or_print(
                 self.config(),
                 "dirstate.pre-read-file",
             );
             let dirstate_file_contents = self.dirstate_file_contents()?;
-            if dirstate_file_contents.is_empty() {
+            return if dirstate_file_contents.is_empty() {
                 self.dirstate_parents.set(DirstateParents::NULL);
                 Ok(OwningDirstateMap::new_empty(Vec::new()))
             } else {
                 let (map, parents) =
                     OwningDirstateMap::new_v1(dirstate_file_contents)?;
                 self.dirstate_parents.set(parents);
                 Ok(map)
-            }
+            };
         }
     }
 
     fn read_docket_and_data_file(
         &self,
     ) -> Result<OwningDirstateMap, DirstateError> {
         debug_wait_for_file_or_print(self.config(), "dirstate.pre-read-file");
         let dirstate_file_contents = self.dirstate_file_contents()?;
         if dirstate_file_contents.is_empty() {
             self.dirstate_parents.set(DirstateParents::NULL);
             self.dirstate_data_file_uuid.set(None);
             return Ok(OwningDirstateMap::new_empty(Vec::new()));
         }
         let docket = crate::dirstate_tree::on_disk::read_docket(
             &dirstate_file_contents,
         )?;
         debug_wait_for_file_or_print(
             self.config(),
             "dirstate.post-docket-read-file",
         );
         self.dirstate_parents.set(docket.parents());
         self.dirstate_data_file_uuid
             .set(Some(docket.uuid.to_owned()));
         let data_size = docket.data_size();
+
+        let context = "between reading dirstate docket and data file";
+        let race_error = HgError::RaceDetected(context.into());
         let metadata = docket.tree_metadata();
+
         let mut map = if crate::vfs::is_on_nfs_mount(docket.data_filename()) {
             // Don't mmap on NFS to prevent `SIGBUS` error on deletion
-            OwningDirstateMap::new_v2(
-                self.hg_vfs().read(docket.data_filename())?,
-                data_size,
-                metadata,
+            let contents = self.hg_vfs().read(docket.data_filename());
+            let contents = match contents {
+                Ok(c) => c,
+                Err(HgError::IoError { error, context }) => {
+                    match error.raw_os_error().expect("real os error") {
+                        // 2 = ENOENT, No such file or directory
+                        // 116 = ESTALE, Stale NFS file handle
+                        //
+                        // TODO match on `error.kind()` when
+                        // `ErrorKind::StaleNetworkFileHandle` is stable.
+                        2 | 116 => {
+                            // Race where the data file was deleted right after
+                            // we read the docket, try again
+                            return Err(race_error.into());
+                        }
+                        _ => {
+                            return Err(
+                                HgError::IoError { error, context }.into()
             )
-        } else if let Some(data_mmap) = self
+                        }
+                    }
+                }
+                Err(e) => return Err(e.into()),
+            };
+            OwningDirstateMap::new_v2(contents, data_size, metadata)
+        } else {
+            match self
             .hg_vfs()
             .mmap_open(docket.data_filename())
-            .io_not_found_as_none()?
+                .io_not_found_as_none()
         {
+                Ok(Some(data_mmap)) => {
             OwningDirstateMap::new_v2(data_mmap, data_size, metadata)
-        } else {
-            OwningDirstateMap::new_v2(Vec::new(), data_size, metadata)
+                }
+                Ok(None) => {
+                    // Race where the data file was deleted right after we
+                    // read the docket, try again
+                    return Err(race_error.into());
+                }
+                Err(e) => return Err(e.into()),
+            }
         }?;
 
         let write_mode_config = self
             .config()
             .get_str(b"devel", b"dirstate.v2.data_update_mode")
             .unwrap_or(Some("auto"))
             .unwrap_or("auto"); // don't bother for devel options
         let write_mode = match write_mode_config {
             "auto" => DirstateMapWriteMode::Auto,
             "force-new" => DirstateMapWriteMode::ForceNewDataFile,
             "force-append" => DirstateMapWriteMode::ForceAppend,
             _ => DirstateMapWriteMode::Auto,
         };
 
         map.with_dmap_mut(|m| m.set_write_mode(write_mode));
 
         Ok(map)
     }
 
     pub fn dirstate_map(
         &self,
     ) -> Result<Ref<OwningDirstateMap>, DirstateError> {
         self.dirstate_map.get_or_init(|| self.new_dirstate_map())
     }
 
     pub fn dirstate_map_mut(
         &self,
     ) -> Result<RefMut<OwningDirstateMap>, DirstateError> {
         self.dirstate_map
             .get_mut_or_init(|| self.new_dirstate_map())
     }
 
     fn new_changelog(&self) -> Result<Changelog, HgError> {
         Changelog::open(&self.store_vfs(), self.has_nodemap())
     }
 
     pub fn changelog(&self) -> Result<Ref<Changelog>, HgError> {
         self.changelog.get_or_init(|| self.new_changelog())
     }
 
     pub fn changelog_mut(&self) -> Result<RefMut<Changelog>, HgError> {
         self.changelog.get_mut_or_init(|| self.new_changelog())
     }
 
     fn new_manifestlog(&self) -> Result<Manifestlog, HgError> {
         Manifestlog::open(&self.store_vfs(), self.has_nodemap())
     }
 
     pub fn manifestlog(&self) -> Result<Ref<Manifestlog>, HgError> {
         self.manifestlog.get_or_init(|| self.new_manifestlog())
     }
 
     pub fn manifestlog_mut(&self) -> Result<RefMut<Manifestlog>, HgError> {
         self.manifestlog.get_mut_or_init(|| self.new_manifestlog())
     }
 
     /// Returns the manifest of the *changeset* with the given node ID
     pub fn manifest_for_node(
         &self,
         node: impl Into<NodePrefix>,
     ) -> Result<Manifest, RevlogError> {
         self.manifestlog()?.data_for_node(
             self.changelog()?
                 .data_for_node(node.into())?
                 .manifest_node()?
                 .into(),
         )
     }
 
     /// Returns the manifest of the *changeset* with the given revision number
     pub fn manifest_for_rev(
         &self,
         revision: Revision,
     ) -> Result<Manifest, RevlogError> {
         self.manifestlog()?.data_for_node(
             self.changelog()?
                 .data_for_rev(revision)?
                 .manifest_node()?
                 .into(),
         )
     }
 
     pub fn has_subrepos(&self) -> Result<bool, DirstateError> {
         if let Some(entry) = self.dirstate_map()?.get(HgPath::new(".hgsub"))? {
             Ok(entry.tracked())
         } else {
             Ok(false)
         }
     }
 
     pub fn filelog(&self, path: &HgPath) -> Result<Filelog, HgError> {
         Filelog::open(self, path)
     }
 
     /// Write to disk any updates that were made through `dirstate_map_mut`.
     ///
     /// The "wlock" must be held while calling this.
     /// See for example `try_with_wlock_no_wait`.
     ///
     /// TODO: have a `WritableRepo` type only accessible while holding the
     /// lock?
     pub fn write_dirstate(&self) -> Result<(), DirstateError> {
         let map = self.dirstate_map()?;
         // TODO: Maintain a `DirstateMap::dirty` flag, and return early here if
         // it’s unset
         let parents = self.dirstate_parents()?;
         let (packed_dirstate, old_uuid_to_remove) = if self.has_dirstate_v2() {
             let uuid_opt = self
                 .dirstate_data_file_uuid
                 .get_or_init(|| self.read_dirstate_data_file_uuid())?;
             let uuid_opt = uuid_opt.as_ref();
             let write_mode = if uuid_opt.is_some() {
                 DirstateMapWriteMode::Auto
             } else {
                 DirstateMapWriteMode::ForceNewDataFile
             };
             let (data, tree_metadata, append, old_data_size) =
                 map.pack_v2(write_mode)?;
 
             // Reuse the uuid, or generate a new one, keeping the old for
             // deletion.
             let (uuid, old_uuid) = match uuid_opt {
                 Some(uuid) => {
                     let as_str = std::str::from_utf8(uuid)
                         .map_err(|_| {
                             HgError::corrupted(
                                 "non-UTF-8 dirstate data file ID",
                             )
                         })?
                         .to_owned();
                     if append {
                         (as_str, None)
                     } else {
                         (DirstateDocket::new_uid(), Some(as_str))
                     }
                 }
                 None => (DirstateDocket::new_uid(), None),
             };
 
             let data_filename = format!("dirstate.{}", uuid);
             let data_filename = self.hg_vfs().join(data_filename);
             let mut options = std::fs::OpenOptions::new();
             options.write(true);
 
             // Why are we not using the O_APPEND flag when appending?
             //
             // - O_APPEND makes it trickier to deal with garbage at the end of
             //   the file, left by a previous uncommitted transaction. By
             //   starting the write at [old_data_size] we make sure we erase
             //   all such garbage.
             //
             // - O_APPEND requires to special-case 0-byte writes, whereas we
             //   don't need that.
             //
             // - Some OSes have bugs in implementation O_APPEND:
             //   revlog.py talks about a Solaris bug, but we also saw some ZFS
             //   bug: https://github.com/openzfs/zfs/pull/3124,
             //   https://github.com/openzfs/zfs/issues/13370
             //
             if !append {
                 log::trace!("creating a new dirstate data file");
                 options.create_new(true);
             } else {
                 log::trace!("appending to the dirstate data file");
             }
 
             let data_size = (|| {
                 // TODO: loop and try another random ID if !append and this
                 // returns `ErrorKind::AlreadyExists`? Collision chance of two
                 // random IDs is one in 2**32
                 let mut file = options.open(&data_filename)?;
                 if append {
                     file.seek(SeekFrom::Start(old_data_size as u64))?;
                 }
                 file.write_all(&data)?;
                 file.flush()?;
                 file.seek(SeekFrom::Current(0))
             })()
             .when_writing_file(&data_filename)?;
 
             let packed_dirstate = DirstateDocket::serialize(
                 parents,
                 tree_metadata,
                 data_size,
                 uuid.as_bytes(),
             )
             .map_err(|_: std::num::TryFromIntError| {
                 HgError::corrupted("overflow in dirstate docket serialization")
             })?;
 
             (packed_dirstate, old_uuid)
         } else {
             (map.pack_v1(parents)?, None)
         };
 
         let vfs = self.hg_vfs();
         vfs.atomic_write("dirstate", &packed_dirstate)?;
         if let Some(uuid) = old_uuid_to_remove {
             // Remove the old data file after the new docket pointing to the
             // new data file was written.
             vfs.remove_file(format!("dirstate.{}", uuid))?;
         }
         Ok(())
     }
 }
 
 /// Lazily-initialized component of `Repo` with interior mutability
 ///
 /// This differs from `OnceCell` in that the value can still be "deinitialized"
 /// later by setting its inner `Option` to `None`. It also takes the
 /// initialization function as an argument when the value is requested, not
 /// when the instance is created.
 struct LazyCell<T> {
     value: RefCell<Option<T>>,
 }
 
 impl<T> LazyCell<T> {
     fn new() -> Self {
         Self {
             value: RefCell::new(None),
         }
     }
 
     fn set(&self, value: T) {
         *self.value.borrow_mut() = Some(value)
     }
 
     fn get_or_init<E>(
         &self,
         init: impl Fn() -> Result<T, E>,
     ) -> Result<Ref<T>, E> {
         let mut borrowed = self.value.borrow();
         if borrowed.is_none() {
             drop(borrowed);
             // Only use `borrow_mut` if it is really needed to avoid panic in
             // case there is another outstanding borrow but mutation is not
             // needed.
             *self.value.borrow_mut() = Some(init()?);
             borrowed = self.value.borrow()
         }
         Ok(Ref::map(borrowed, |option| option.as_ref().unwrap()))
     }
 
     fn get_mut_or_init<E>(
         &self,
         init: impl Fn() -> Result<T, E>,
     ) -> Result<RefMut<T>, E> {
         let mut borrowed = self.value.borrow_mut();
         if borrowed.is_none() {
             *borrowed = Some(init()?);
         }
         Ok(RefMut::map(borrowed, |option| option.as_mut().unwrap()))
     }
 }

tests/test-dirstate-read-race.t

@@ -1,584 +1,597 b''
 ==============================================================================
 Check potential race conditions between a dirstate's read and other operations
 ==============================================================================
 
 #testcases dirstate-v1 dirstate-v2-append dirstate-v2-rewrite
 #testcases pre-all-read pre-some-read
 
 Some commands, like `hg status`, do not need to take the wlock but need to
 access dirstate data.
 Other commands might update the dirstate data while this happens.
 
 This can create issues if repository data is read in the wrong order, or for
 the dirstate-v2 format where the data is contained in multiple files.
 
 This test file is meant to test various cases where such parallel operations
 happen and make sure the reading process behaves fine. We do so with a `hg
 status` command since it is probably the most advanced of such read-only
 command.
 
 It bears simililarity with `tests/test-dirstate-status-race.t ` but tests a
 different type of race.
 
 Setup
 =====
 
   $ cat >> $HGRCPATH << EOF
   > [storage]
   > dirstate-v2.slow-path=allow
   > EOF
 
 #if no-dirstate-v1
   $ cat >> $HGRCPATH << EOF
   > [format]
   > use-dirstate-v2=yes
   > EOF
 #else
   $ cat >> $HGRCPATH << EOF
   > [format]
   > use-dirstate-v2=no
   > EOF
 #endif
 
 #if dirstate-v2-rewrite
   $ d2args="--config devel.dirstate.v2.data_update_mode=force-new"
 #endif
 #if dirstate-v2-append
   $ d2args="--config devel.dirstate.v2.data_update_mode=force-append"
 #endif
 
 
 #if dirstate-v1
   $ cfg="devel.sync.dirstate.pre-read-file"
 #else
 #if pre-all-read
   $ cfg="devel.sync.dirstate.pre-read-file"
 #else
   $ cfg="devel.sync.dirstate.post-docket-read-file"
 #endif
 #endif
 
   $ directories="dir dir/nested dir2"
   $ first_files="dir/nested/a dir/b dir/c dir/d dir2/e f"
   $ second_files="g dir/nested/h dir/i dir/j dir2/k dir2/l dir/nested/m"
   $ extra_files="dir/n dir/o p q"
 
   $ hg init reference-repo
   $ cd reference-repo
   $ mkdir -p dir/nested dir2
   $ touch -t 200001010000 $first_files $directories
   $ hg commit -Aqm "recreate a bunch of files to facilitate dirstate-v2 append"
   $ touch -t 200001010010 $second_files $directories
   $ hg commit -Aqm "more files to have two commit"
   $ hg log -G -v
   @  changeset:   1:9a86dcbfb938
   |  tag:         tip
   |  user:        test
   |  date:        Thu Jan 01 00:00:00 1970 +0000
   |  files:       dir/i dir/j dir/nested/h dir/nested/m dir2/k dir2/l g
   |  description:
   |  more files to have two commit
   |
   |
   o  changeset:   0:4f23db756b09
      user:        test
      date:        Thu Jan 01 00:00:00 1970 +0000
      files:       dir/b dir/c dir/d dir/nested/a dir2/e f
      description:
      recreate a bunch of files to facilitate dirstate-v2 append
   
   
   $ hg manifest
   dir/b
   dir/c
   dir/d
   dir/i
   dir/j
   dir/nested/a
   dir/nested/h
   dir/nested/m
   dir2/e
   dir2/k
   dir2/l
   f
   g
 
 Add some unknown files and refresh the dirstate
 
   $ touch -t 200001010020 $extra_files
   $ hg add dir/o
   $ hg remove dir/nested/m
 
   $ hg st --config devel.dirstate.v2.data_update_mode=force-new
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ hg debugstate
   n 644          0 2000-01-01 00:00:00 dir/b
   n 644          0 2000-01-01 00:00:00 dir/c
   n 644          0 2000-01-01 00:00:00 dir/d
   n 644          0 2000-01-01 00:10:00 dir/i
   n 644          0 2000-01-01 00:10:00 dir/j
   n 644          0 2000-01-01 00:00:00 dir/nested/a
   n 644          0 2000-01-01 00:10:00 dir/nested/h
   r ?????????????????????????????????? dir/nested/m (glob)
   a ?????????????????????????????????? dir/o (glob)
   n 644          0 2000-01-01 00:00:00 dir2/e
   n 644          0 2000-01-01 00:10:00 dir2/k
   n 644          0 2000-01-01 00:10:00 dir2/l
   n 644          0 2000-01-01 00:00:00 f
   n 644          0 2000-01-01 00:10:00 g
   $ hg debugstate > ../reference
   $ cd ..
 
 Actual Testing
 ==============
 
 Race with a `hg add`
 -------------------
 
   $ cp -a reference-repo race-with-add
   $ cd race-with-add
 
 spin a `hg status` with some caches to update
 
   $ hg st >$TESTTMP/status-race-lock.out 2>$TESTTMP/status-race-lock.log \
   > --config rhg.on-unsupported=abort \
   > --config ${cfg}=$TESTTMP/status-race-lock \
   > &
   $ $RUNTESTDIR/testlib/wait-on-file 5 $TESTTMP/status-race-lock.waiting
 
 Add a file
 
   $ hg $d2args add dir/n
   $ touch $TESTTMP/status-race-lock
   $ wait
 
 The file should in a "added" state
 
   $ hg status
   A dir/n
   A dir/o
   R dir/nested/m
   ? p
   ? q
 
 The status process should return a consistent result and not crash.
 
 #if dirstate-v1
   $ cat $TESTTMP/status-race-lock.out
   A dir/n
   A dir/o
   R dir/nested/m
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if rhg
 #if pre-all-read
   $ cat $TESTTMP/status-race-lock.out
   A dir/n
   A dir/o
   R dir/nested/m
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if dirstate-v2-append
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
+  A dir/n
+  A dir/o
+  R dir/nested/m
+  ? p
+  ? q
   $ cat $TESTTMP/status-race-lock.log
-  abort: dirstate-v2 parse error: not enough bytes on disk
 #endif
 #endif
 #else
 #if rust
 #if dirstate-v2-rewrite
   $ cat $TESTTMP/status-race-lock.out
   A dir/n
   A dir/o
   R dir/nested/m
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/n
   A dir/o
   R dir/nested/m
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #endif
 #endif
 
 final cleanup
 
   $ rm $TESTTMP/status-race-lock $TESTTMP/status-race-lock.waiting
   $ cd ..
 
 Race with a `hg commit`
 -----------------------
 
   $ cp -a reference-repo race-with-commit
   $ cd race-with-commit
 
 spin a `hg status with some cache to update
 
   $ hg st >$TESTTMP/status-race-lock.out 2>$TESTTMP/status-race-lock.log \
   > --config rhg.on-unsupported=abort \
   > --config ${cfg}=$TESTTMP/status-race-lock \
   > &
   $ $RUNTESTDIR/testlib/wait-on-file 5 $TESTTMP/status-race-lock.waiting
 
 Add a do a commit
 
   $ hg status
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ hg $d2args commit -m 'racing commit'
   $ touch $TESTTMP/status-race-lock
   $ wait
 
 commit was created, and status is now clean
 
   $ hg log -GT '{node|short} {desc}\n'
   @  02a67a77ee9b racing commit
   |
   o  9a86dcbfb938 more files to have two commit
   |
   o  4f23db756b09 recreate a bunch of files to facilitate dirstate-v2 append
   
   $ hg status
   ? dir/n
   ? p
   ? q
 
 The status process should return a consistent result and not crash.
 
 #if dirstate-v1
   $ cat $TESTTMP/status-race-lock.out
   M dir/o (no-rhg !)
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
   warning: ignoring unknown working parent 02a67a77ee9b! (no-rhg !)
 #else
 #if rhg
 #if pre-all-read
   $ cat $TESTTMP/status-race-lock.out
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if dirstate-v2-append
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
+  ? dir/n
+  ? p
+  ? q
   $ cat $TESTTMP/status-race-lock.log
-  abort: dirstate-v2 parse error: not enough bytes on disk
 #endif
 #endif
 #else
 #if rust
 #if dirstate-v2-rewrite
   $ cat $TESTTMP/status-race-lock.out
   M dir/o
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
   warning: ignoring unknown working parent 02a67a77ee9b!
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #else
   $ cat $TESTTMP/status-race-lock.out
   M dir/o
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
   warning: ignoring unknown working parent 02a67a77ee9b!
 #endif
 #endif
 #endif
 
 final cleanup
 
   $ rm $TESTTMP/status-race-lock $TESTTMP/status-race-lock.waiting
   $ cd ..
 
 Race with a `hg update`
 -----------------------
 
   $ cp -a reference-repo race-with-update
   $ cd race-with-update
 
 spin a `hg status` with some caches to update
 
   $ hg st >$TESTTMP/status-race-lock.out 2>$TESTTMP/status-race-lock.log \
   > --config rhg.on-unsupported=abort \
   > --config ${cfg}=$TESTTMP/status-race-lock \
   > &
   $ $RUNTESTDIR/testlib/wait-on-file 5 $TESTTMP/status-race-lock.waiting
 do an update
 
   $ hg status
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ hg log -GT '{node|short} {desc}\n'
   @  9a86dcbfb938 more files to have two commit
   |
   o  4f23db756b09 recreate a bunch of files to facilitate dirstate-v2 append
   
   $ hg $d2args update --merge ".~1"
   0 files updated, 0 files merged, 6 files removed, 0 files unresolved
   $ touch $TESTTMP/status-race-lock
   $ wait
 #if rhg dirstate-v2-append pre-some-read
   $ hg log -GT '{node|short} {desc}\n'
   @  9a86dcbfb938 more files to have two commit
   |
   o  4f23db756b09 recreate a bunch of files to facilitate dirstate-v2 append
   
   $ hg status
   A dir/o
   R dir/nested/m
   ! dir/i
   ! dir/j
   ! dir/nested/h
   ! dir2/k
   ! dir2/l
   ! g
   ? dir/n
   ? p
   ? q
 #else
   $ hg log -GT '{node|short} {desc}\n'
   o  9a86dcbfb938 more files to have two commit
   |
   @  4f23db756b09 recreate a bunch of files to facilitate dirstate-v2 append
   
   $ hg status
   A dir/o
   ? dir/n
   ? p
   ? q
 #endif
 
 The status process should return a consistent result and not crash.
 
 #if dirstate-v1
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if rhg
 #if pre-all-read
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if dirstate-v2-append
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ! dir/i
   ! dir/j
   ! dir/nested/h
   ! dir2/k
   ! dir2/l
   ! g
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
+  A dir/o
+  ? dir/n
+  ? p
+  ? q
   $ cat $TESTTMP/status-race-lock.log
-  abort: dirstate-v2 parse error: not enough bytes on disk
 #endif
 #endif
 #else
 #if rust
 #if dirstate-v2-rewrite
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ! dir/i
   ! dir/j
   ! dir/nested/h
   ! dir2/k
   ! dir2/l
   ! g
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #endif
 #endif
 
 final cleanup
 
   $ rm $TESTTMP/status-race-lock $TESTTMP/status-race-lock.waiting
   $ cd ..
 
 Race with a cache updating `hg status`
 --------------------------------------
 
 It is interesting to race with "read-only" operation (that still update its cache)
 
   $ cp -a reference-repo race-with-status
   $ cd race-with-status
 
 spin a `hg status` with some caches to update
 
   $ hg st >$TESTTMP/status-race-lock.out 2>$TESTTMP/status-race-lock.log \
   > --config rhg.on-unsupported=abort \
   > --config ${cfg}=$TESTTMP/status-race-lock \
   > &
   $ $RUNTESTDIR/testlib/wait-on-file 5 $TESTTMP/status-race-lock.waiting
 do an update
 
   $ touch -t 200001020006 f
   $ hg $d2args status
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ touch $TESTTMP/status-race-lock
   $ wait
 
 The status process should return a consistent result and not crash.
 
 #if dirstate-v1
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if rhg
 #if pre-all-read
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
 #if dirstate-v2-append
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
+  A dir/o
+  R dir/nested/m
+  ? dir/n
+  ? p
+  ? q
   $ cat $TESTTMP/status-race-lock.log
-  abort: dirstate-v2 parse error: not enough bytes on disk
 #endif
 #endif
 #else
 #if rust
 #if dirstate-v2-rewrite
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #else
   $ cat $TESTTMP/status-race-lock.out
   A dir/o
   R dir/nested/m
   ? dir/n
   ? p
   ? q
   $ cat $TESTTMP/status-race-lock.log
 #endif
 #endif
 #endif
 
 final cleanup
 
   $ rm $TESTTMP/status-race-lock $TESTTMP/status-race-lock.waiting
   $ cd ..