Merge pull request #11492

po/de.po

@@ -1025,8 +1025,8 @@ msgid "Error reading script file '%s':"
 msgstr "Die Script-Datei '%s' kann nicht gelesen werden:"
 
 #, c-format
-msgid "Error when renaming history file: %s"
-msgstr "Fehler beim Umbenennen der Verlaufsdatei: %s"
+msgid "Error when renaming file: %s"
+msgstr ""
 
 #, c-format
 msgid "Error while reading file %ls\n"
@@ -1249,14 +1249,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "Funktionen auflisten oder entfernen"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr "Sperren der Geschichts-Datei dauerte zu lang (%.3f Sekunden)."
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr "Die Datei für universelle Variablen zu sperren dauerte zu lange (%.3f Sekunden)"
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr "Logikoperationen werden nicht unterstützt, nimm stattdessen `test`"
 
@@ -1519,6 +1511,10 @@ msgstr "Derzeit ausgewertete Funktion stoppen"
 msgid "Stop the innermost loop"
 msgstr "Innerste Schleife beenden"
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr "Ereignisweitergabe vorübergehend blockieren"
 
@@ -1623,22 +1619,10 @@ msgstr "Konnte Verzeichnis %ls abgeleitet von %ls nicht finden: '%ls'."
 msgid "Unable to locate the %ls directory."
 msgstr ""
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr "Die Universal-Variablen-Datei '%s' kann nicht geöffnet werden: %s"
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr ""
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr "Konnte Datei '%ls' nicht zu '%ls' umbenennen: %s"
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr "Die Universal-Variablen-Datei '%ls' kann nicht geschrieben werden: %s"
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr ""
 
@@ -79192,6 +79176,30 @@ msgstr ""
 msgid "~ expansion"
 msgstr ""
 
+#, c-format
+#~ msgid "Error when renaming history file: %s"
+#~ msgstr "Fehler beim Umbenennen der Verlaufsdatei: %s"
+
+#, c-format
+#~ msgid "Locking the history file took too long (%.3f seconds)."
+#~ msgstr "Sperren der Geschichts-Datei dauerte zu lang (%.3f Sekunden)."
+
+#, c-format
+#~ msgid "Locking the universal var file took too long (%.3f seconds)."
+#~ msgstr "Die Datei für universelle Variablen zu sperren dauerte zu lange (%.3f Sekunden)"
+
+#, c-format
+#~ msgid "Unable to open universal variable file '%s': %s"
+#~ msgstr "Die Universal-Variablen-Datei '%s' kann nicht geöffnet werden: %s"
+
+#, c-format
+#~ msgid "Unable to rename file from '%ls' to '%ls': %s"
+#~ msgstr "Konnte Datei '%ls' nicht zu '%ls' umbenennen: %s"
+
+#, c-format
+#~ msgid "Unable to write to universal variables file '%ls': %s"
+#~ msgstr "Die Universal-Variablen-Datei '%ls' kann nicht geschrieben werden: %s"
+
 #~ msgid "builtin\n"
 #~ msgstr "Eingebauter Befehl\n"
 

po/en.po

@@ -1023,7 +1023,7 @@ msgid "Error reading script file '%s':"
 msgstr ""
 
 #, c-format
-msgid "Error when renaming history file: %s"
+msgid "Error when renaming file: %s"
 msgstr ""
 
 #, c-format
@@ -1246,14 +1246,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "List or remove functions"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr ""
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr ""
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr ""
 
@@ -1516,6 +1508,10 @@ msgstr "Stop the currently evaluated function"
 msgid "Stop the innermost loop"
 msgstr "Stop the innermost loop"
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr "Temporarily block delivery of events"
 
@@ -1620,22 +1616,10 @@ msgstr ""
 msgid "Unable to locate the %ls directory."
 msgstr "Unable to locate the %ls directory."
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr ""
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr ""
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr ""
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr "Unable to write to universal variables file '%ls': %s"
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr ""
 
@@ -79188,6 +79172,10 @@ msgstr ""
 msgid "~ expansion"
 msgstr ""
 
+#, c-format
+#~ msgid "Unable to write to universal variables file '%ls': %s"
+#~ msgstr "Unable to write to universal variables file '%ls': %s"
+
 #~ msgid "empty"
 #~ msgstr "empty"
 

po/fr.po

@@ -1124,7 +1124,7 @@ msgid "Error reading script file '%s':"
 msgstr ""
 
 #, c-format
-msgid "Error when renaming history file: %s"
+msgid "Error when renaming file: %s"
 msgstr ""
 
 #, c-format
@@ -1347,14 +1347,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "Lister ou supprimer des fonctions"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr "Le verrouillage du fichier d’historique a pris trop de temps (%.3f secondes)."
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr "Le verrouillage du fichier des variables universel a pris trop de temps (%.3f secondes)."
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr ""
 
@@ -1617,6 +1609,10 @@ msgstr "Arrêter la fonction actuellement en évaluation"
 msgid "Stop the innermost loop"
 msgstr "Arrêter la boucle interne"
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr "Bloquer temporairement la distribution des événements"
 
@@ -1721,22 +1717,10 @@ msgstr "Impossible de localiser le dossier %ls obtenu de $%ls: '%ls'."
 msgid "Unable to locate the %ls directory."
 msgstr "Impossible de localiser le dossier %ls."
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr ""
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr ""
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr "Impossible de renommer le fichier '%ls' en '%ls': %s"
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr "Impossible d’écrire dans le fichier de variables universelles '%ls': %s"
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr ""
 
@@ -79289,6 +79273,22 @@ msgstr ""
 msgid "~ expansion"
 msgstr ""
 
+#, c-format
+#~ msgid "Locking the history file took too long (%.3f seconds)."
+#~ msgstr "Le verrouillage du fichier d’historique a pris trop de temps (%.3f secondes)."
+
+#, c-format
+#~ msgid "Locking the universal var file took too long (%.3f seconds)."
+#~ msgstr "Le verrouillage du fichier des variables universel a pris trop de temps (%.3f secondes)."
+
+#, c-format
+#~ msgid "Unable to rename file from '%ls' to '%ls': %s"
+#~ msgstr "Impossible de renommer le fichier '%ls' en '%ls': %s"
+
+#, c-format
+#~ msgid "Unable to write to universal variables file '%ls': %s"
+#~ msgstr "Impossible d’écrire dans le fichier de variables universelles '%ls': %s"
+
 #~ msgid "Check that this terminal type is supported on this system."
 #~ msgstr "Vérifiez que votre type de terminal est supporté sur ce système"
 

po/pl.po

@@ -1019,7 +1019,7 @@ msgid "Error reading script file '%s':"
 msgstr ""
 
 #, c-format
-msgid "Error when renaming history file: %s"
+msgid "Error when renaming file: %s"
 msgstr ""
 
 #, c-format
@@ -1242,14 +1242,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "Wypisz lub usuń funkcje"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr ""
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr ""
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr ""
 
@@ -1512,6 +1504,10 @@ msgstr "Zatrzymaj obecnie używaną funkcję"
 msgid "Stop the innermost loop"
 msgstr ""
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr ""
 
@@ -1616,22 +1612,10 @@ msgstr ""
 msgid "Unable to locate the %ls directory."
 msgstr ""
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr ""
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr ""
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr ""
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr ""
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr ""
 

po/pt_BR.po

@@ -1024,7 +1024,7 @@ msgid "Error reading script file '%s':"
 msgstr ""
 
 #, c-format
-msgid "Error when renaming history file: %s"
+msgid "Error when renaming file: %s"
 msgstr ""
 
 #, c-format
@@ -1247,14 +1247,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "Lista ou remove funções"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr ""
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr ""
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr ""
 
@@ -1517,6 +1509,10 @@ msgstr "Pára a função em execução"
 msgid "Stop the innermost loop"
 msgstr "Pára o laço mais interno"
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr "Bloqueia temporariamente a entrega de eventos"
 
@@ -1621,22 +1617,10 @@ msgstr ""
 msgid "Unable to locate the %ls directory."
 msgstr ""
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr ""
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr ""
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr ""
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr ""
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr ""
 

po/sv.po

@@ -1020,7 +1020,7 @@ msgid "Error reading script file '%s':"
 msgstr ""
 
 #, c-format
-msgid "Error when renaming history file: %s"
+msgid "Error when renaming file: %s"
 msgstr ""
 
 #, c-format
@@ -1243,14 +1243,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "Visa eller ta bort funktioner"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr ""
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr ""
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr ""
 
@@ -1513,6 +1505,10 @@ msgstr "Avbryt den nuvarande funktionen"
 msgid "Stop the innermost loop"
 msgstr "Avbryt den innersta loopen"
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr "Blockera tillfälligt leverans av händelser"
 
@@ -1617,22 +1613,10 @@ msgstr ""
 msgid "Unable to locate the %ls directory."
 msgstr ""
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr ""
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr ""
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr ""
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr ""
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr ""
 

po/zh_CN.po

@@ -1017,8 +1017,8 @@ msgid "Error reading script file '%s':"
 msgstr "读取脚本文件 '%s'出错: "
 
 #, c-format
-msgid "Error when renaming history file: %s"
-msgstr "重命名历史文件时出错:%s"
+msgid "Error when renaming file: %s"
+msgstr ""
 
 #, c-format
 msgid "Error while reading file %ls\n"
@@ -1241,14 +1241,6 @@ msgstr ""
 msgid "List or remove functions"
 msgstr "列出或移除函数"
 
-#, c-format
-msgid "Locking the history file took too long (%.3f seconds)."
-msgstr "锁定历史文件花费了太长时间(%.3f 秒)."
-
-#, c-format
-msgid "Locking the universal var file took too long (%.3f seconds)."
-msgstr "锁定 通用 var 文件花费了太长时间 (%.3f 秒) ."
-
 msgid "Logical operations are not supported, use `test` instead"
 msgstr "不支持逻辑操作,使用`test`替代"
 
@@ -1511,6 +1503,10 @@ msgstr "停止当前求值的函数"
 msgid "Stop the innermost loop"
 msgstr "停止最内层的循环"
 
+#
+msgid "Synchronized file access"
+msgstr ""
+
 msgid "Temporarily block delivery of events"
 msgstr "Temporarily block delivery of events"
 
@@ -1615,22 +1611,10 @@ msgstr "无法定位来自 $ls 的 %ls 目录: %ls ."
 msgid "Unable to locate the %ls directory."
 msgstr "无法找到 %ls 目录 ."
 
-#, c-format
-msgid "Unable to open universal variable file '%s': %s"
-msgstr "无法打开通用变量文件'%s': %s"
-
 #, c-format
 msgid "Unable to read input file: %s"
 msgstr "无法读取输入文件:%s"
 
-#, c-format
-msgid "Unable to rename file from '%ls' to '%ls': %s"
-msgstr "无法将文件从 '%ls' 重命名为 '%ls':%s"
-
-#, c-format
-msgid "Unable to write to universal variables file '%ls': %s"
-msgstr "无法写入通用变量文件 '%ls':%s"
-
 msgid "Unexpected ')' for unopened parenthesis"
 msgstr "未打开的括号' ' "
 
@@ -79187,6 +79171,30 @@ msgstr "缩放"
 msgid "~ expansion"
 msgstr "~ 扩大"
 
+#, c-format
+#~ msgid "Error when renaming history file: %s"
+#~ msgstr "重命名历史文件时出错:%s"
+
+#, c-format
+#~ msgid "Locking the history file took too long (%.3f seconds)."
+#~ msgstr "锁定历史文件花费了太长时间(%.3f 秒)."
+
+#, c-format
+#~ msgid "Locking the universal var file took too long (%.3f seconds)."
+#~ msgstr "锁定 通用 var 文件花费了太长时间 (%.3f 秒) ."
+
+#, c-format
+#~ msgid "Unable to open universal variable file '%s': %s"
+#~ msgstr "无法打开通用变量文件'%s': %s"
+
+#, c-format
+#~ msgid "Unable to rename file from '%ls' to '%ls': %s"
+#~ msgstr "无法将文件从 '%ls' 重命名为 '%ls':%s"
+
+#, c-format
+#~ msgid "Unable to write to universal variables file '%ls': %s"
+#~ msgstr "无法写入通用变量文件 '%ls':%s"
+
 #~ msgid "builtin\n"
 #~ msgstr "内建\n"
 

src/env/environment.rs

@@ -8,7 +8,6 @@
 use crate::common::{str2wcstring, unescape_string, wcs2zstring, UnescapeStringStyle};
 use crate::env::{EnvMode, EnvVar, Statuses};
 use crate::env_dispatch::{env_dispatch_init, env_dispatch_var_change};
-use crate::env_universal_common::CallbackDataList;
 use crate::event::Event;
 use crate::flog::FLOG;
 use crate::global_safety::RelaxedAtomicBool;
@@ -351,22 +350,23 @@ pub fn universal_sync(&self, always: bool) -> Vec<Event> {
         }
         UVARS_LOCALLY_MODIFIED.store(false);
 
-        let mut callbacks = CallbackDataList::new();
-        let changed = uvars().sync(&mut callbacks);
+        let (changed, callbacks) = uvars().sync();
         if changed {
             default_notifier().post_notification();
         }
         // React internally to changes to special variables like LANG, and populate on-variable events.
         let mut result = Vec::new();
-        for callback in callbacks {
-            let name = callback.key;
-            env_dispatch_var_change(&name, self);
-            let evt = if callback.val.is_none() {
-                Event::variable_erase(name)
-            } else {
-                Event::variable_set(name)
-            };
-            result.push(evt);
+        if let Some(callbacks) = callbacks {
+            for callback in callbacks {
+                let name = callback.key;
+                env_dispatch_var_change(&name, self);
+                let evt = if callback.val.is_none() {
+                    Event::variable_erase(name)
+                } else {
+                    Event::variable_set(name)
+                };
+                result.push(evt);
+            }
         }
         result
     }
@@ -792,8 +792,7 @@ pub fn env_init(paths: Option<&ConfigPaths>, do_uvars: bool, default_paths: bool
         UVAR_SCOPE_IS_GLOBAL.store(true);
     } else {
         // Set up universal variables using the default path.
-        let mut callbacks = CallbackDataList::new();
-        uvars().initialize(&mut callbacks);
+        let callbacks = uvars().initialize().unwrap_or_default();
         for callback in callbacks {
             env_dispatch_var_change(&callback.key, vars);
         }

src/env_universal_common.rs

@@ -1,41 +1,24 @@
 #![allow(clippy::bad_bit_mask)]
 
 use crate::common::{
-    str2wcstring, timef, unescape_string, valid_var_name, wcs2zstring, UnescapeFlags,
-    UnescapeStringStyle,
+    unescape_string, valid_var_name, wcs2zstring, UnescapeFlags, UnescapeStringStyle,
 };
 use crate::env::{EnvVar, EnvVarFlags, VarTable};
-use crate::fallback::fish_mkstemp_cloexec;
-use crate::fds::{open_cloexec, wopen_cloexec};
 use crate::flog::{FLOG, FLOGF};
+use crate::fs::{lock_and_load, rewrite_via_temporary_file, PotentialUpdate};
 use crate::path::path_get_config;
-use crate::path::{path_get_config_remoteness, DirRemoteness};
 use crate::wchar::{decode_byte_from_char, prelude::*};
-use crate::wcstringutil::{join_strings, string_suffixes_string, LineIterator};
-use crate::wutil::{
-    file_id_for_file, file_id_for_path, file_id_for_path_narrow, wdirname, wrealpath, wrename,
-    wstat, wunlink, FileId, INVALID_FILE_ID,
-};
-use errno::{errno, Errno};
-use libc::{EINTR, LOCK_EX};
-use nix::{fcntl::OFlag, sys::stat::Mode};
+use crate::wcstringutil::{join_strings, LineIterator};
+use crate::wutil::{file_id_for_file, file_id_for_path_narrow, wrealpath, FileId, INVALID_FILE_ID};
 use std::collections::hash_map::Entry;
 use std::collections::HashSet;
 use std::ffi::CString;
 use std::fs::File;
 use std::io::{Read, Write};
 use std::mem::MaybeUninit;
-use std::os::fd::AsRawFd;
-use std::os::unix::prelude::MetadataExt;
-
-// Pull in the O_EXLOCK constant if it is defined, otherwise set it to 0.
-#[cfg(any(apple, bsd))]
-const O_EXLOCK: OFlag = OFlag::O_EXLOCK;
-
-#[cfg(not(any(apple, bsd)))]
-const O_EXLOCK: OFlag = OFlag::empty();
 
 /// Callback data, reflecting a change in universal variables.
+#[derive(Clone, Debug, Eq, PartialEq)]
 pub struct CallbackData {
     // The name of the variable.
     pub key: WString,
@@ -73,16 +56,21 @@ pub struct EnvUniversal {
 
     // Whether it's OK to save. This may be set to false if we discover that a future version of
     // fish wrote the uvars contents.
+    // Only update if last_read_file_id is updated as well.
     ok_to_save: bool,
 
-    // If true, attempt to flock the uvars file.
-    // This latches to false if the file is found to be remote, where flock may hang.
-    do_flock: bool,
-
     // File id from which we last read.
+    // Only update if ok_to_save is updated as well.
     last_read_file_id: FileId,
 }
 
+struct UniversalReadUpdate {
+    export_generation_increment: u64,
+    new_vars: VarTable,
+    callbacks: CallbackDataList,
+    ok_to_save: bool,
+}
+
 impl EnvUniversal {
     // Construct an empty universal variables.
     pub fn new() -> Self {
@@ -93,7 +81,6 @@ pub fn new() -> Self {
             modified: Default::default(),
             export_generation: 1,
             ok_to_save: true,
-            do_flock: true,
             last_read_file_id: INVALID_FILE_ID,
         }
     }
@@ -154,90 +141,93 @@ pub fn get_table(&self) -> &VarTable {
 
     /// Initialize this uvars for the default path.
     /// This should be called at most once on any given instance.
-    pub fn initialize(&mut self, callbacks: &mut CallbackDataList) {
-        // Set do_flock to false immediately if the default variable path is on a remote filesystem.
-        // See #7968.
-        if path_get_config_remoteness() == DirRemoteness::remote {
-            self.do_flock = false;
-        }
-        self.initialize_at_path(callbacks, default_vars_path());
+    pub fn initialize(&mut self) -> Option<CallbackDataList> {
+        self.initialize_at_path(default_vars_path())
     }
 
     /// Initialize a this uvars for a given path.
     /// This is exposed for testing only.
-    pub fn initialize_at_path(&mut self, callbacks: &mut CallbackDataList, path: WString) {
+    pub fn initialize_at_path(&mut self, path: WString) -> Option<CallbackDataList> {
         if path.is_empty() {
-            return;
+            return None;
         }
         assert!(!self.initialized(), "Already initialized");
         self.vars_path = path;
 
-        if self.load_from_path(callbacks) {
-            // Successfully loaded from our normal path.
-        }
+        self.load_from_path()
     }
 
-    /// Reads and writes variables at the correct path. Returns true if modified variables were
-    /// written.
-    pub fn sync(&mut self, callbacks: &mut CallbackDataList) -> bool {
+    /// Reads and writes variables at the correct path.
+    /// If there is an existing variables file with an unknown format, parsing it is attempted,
+    /// but it will not be overwritten.
+    /// Returns whether data was read, and the callbacks.
+    pub fn sync(&mut self) -> (bool, Option<CallbackDataList>) {
         if !self.initialized() {
-            return false;
+            return (false, None);
         }
 
         FLOG!(uvar_file, "universal log sync");
-        // Our saving strategy:
-        //
-        // 1. Open the file, producing an fd.
-        // 2. Lock the file (may be combined with step 1 on systems with O_EXLOCK)
-        // 3. After taking the lock, check if the file at the given path is different from what we
-        // opened. If so, start over.
-        // 4. Read from the file. This can be elided if its dev/inode is unchanged since the last read
-        // 5. Open an adjacent temporary file
-        // 6. Write our changes to an adjacent file
-        // 7. Move the adjacent file into place via rename. This is assumed to be atomic.
-        // 8. Release the lock and close the file
-        //
-        // Consider what happens if Process 1 and 2 both do this simultaneously. Can there be data loss?
-        // Process 1 opens the file and then attempts to take the lock. Now, either process 1 will see
-        // the original file, or process 2's new file. If it sees the new file, we're OK: it's going to
-        // read from the new file, and so there's no data loss. If it sees the old file, then process 2
-        // must have locked it (if process 1 locks it, switch their roles). The lock will block until
-        // process 2 reaches step 7; at that point process 1 will reach step 2, notice that the file has
-        // changed, and then start over.
-        //
-        // It's possible that the underlying filesystem does not support locks (lockless NFS). In this
-        // case, we risk data loss if two shells try to write their universal variables simultaneously.
-        // In practice this is unlikely, since uvars are usually written interactively.
-        //
-        // Prior versions of fish used a hard link scheme to support file locking on lockless NFS. The
-        // risk here is that if the process crashes or is killed while holding the lock, future
-        // instances of fish will not be able to obtain it. This seems to be a greater risk than that of
-        // data loss on lockless NFS. Users who put their home directory on lockless NFS are playing
-        // with fire anyways.
         // If we have no changes, just load.
         if self.modified.is_empty() {
-            self.load_from_path_narrow(callbacks);
+            let callbacks = self.load_from_path_narrow();
             FLOG!(uvar_file, "universal log no modifications");
-            return false;
+            return (false, callbacks);
         }
 
-        let directory = wdirname(&self.vars_path).to_owned();
-
         FLOG!(uvar_file, "universal log performing full sync");
 
-        // Open the file.
-        let Some(mut vars_file) = self.open_and_acquire_lock() else {
-            FLOG!(uvar_file, "universal log open_and_acquire_lock() failed");
-            return false;
+        let rewrite = |old_file: &File,
+                       tmp_file: &mut File|
+         -> std::io::Result<PotentialUpdate<Option<UniversalReadUpdate>>> {
+            match self.load_from_file(old_file) {
+                Some(potential_update) => {
+                    if potential_update.do_save {
+                        let contents = Self::serialize_with_vars(&potential_update.data.new_vars);
+                        tmp_file.write_all(&contents)?;
+                    }
+                    Ok(PotentialUpdate {
+                        do_save: potential_update.do_save,
+                        data: Some(potential_update.data),
+                    })
+                }
+                None => {
+                    if self.ok_to_save {
+                        let contents = Self::serialize_with_vars(&self.vars);
+                        tmp_file.write_all(&contents)?;
+                    }
+                    Ok(PotentialUpdate {
+                        do_save: self.ok_to_save,
+                        data: None,
+                    })
+                }
+            }
         };
 
-        // Read from it.
-        self.load_from_file(&mut vars_file, callbacks);
-
-        if self.ok_to_save {
-            self.save(&directory)
-        } else {
-            true
+        let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone());
+        match rewrite_via_temporary_file(&real_path, rewrite) {
+            Ok((file_id, potential_update)) => {
+                self.last_read_file_id = file_id;
+                self.ok_to_save = potential_update.do_save;
+                self.modified.clear();
+                match potential_update.data {
+                    Some(UniversalReadUpdate {
+                        export_generation_increment,
+                        new_vars,
+                        callbacks,
+                        ok_to_save,
+                    }) => {
+                        assert_eq!(potential_update.do_save, ok_to_save);
+                        self.export_generation += export_generation_increment;
+                        self.vars = new_vars;
+                        (true, Some(callbacks))
+                    }
+                    None => (true, None),
+                }
+            }
+            Err(e) => {
+                FLOG!(uvar_file, "universal log sync failed:", e);
+                (false, None)
+            }
         }
     }
 
@@ -364,38 +354,66 @@ fn initialized(&self) -> bool {
         !self.vars_path.is_empty()
     }
 
-    fn load_from_path(&mut self, callbacks: &mut CallbackDataList) -> bool {
+    fn load_from_path(&mut self) -> Option<CallbackDataList> {
         self.narrow_vars_path = wcs2zstring(&self.vars_path);
-        self.load_from_path_narrow(callbacks)
+        self.load_from_path_narrow()
     }
 
-    fn load_from_path_narrow(&mut self, callbacks: &mut CallbackDataList) -> bool {
+    fn load_from_path_narrow(&mut self) -> Option<CallbackDataList> {
         // Check to see if the file is unchanged. We do this again in load_from_file, but this avoids
         // opening the file unnecessarily.
         if self.last_read_file_id != INVALID_FILE_ID
             && file_id_for_path_narrow(&self.narrow_vars_path) == self.last_read_file_id
         {
             FLOG!(uvar_file, "universal log sync elided based on fast stat()");
-            return true;
+            return None;
         }
 
-        let Ok(mut file) = open_cloexec(&self.narrow_vars_path, OFlag::O_RDONLY, Mode::empty())
-        else {
-            return false;
-        };
-
         FLOG!(uvar_file, "universal log reading from file");
-        self.load_from_file(&mut file, callbacks);
-        true
+        match lock_and_load(&self.vars_path, |f| {
+            Ok(self.load_from_file(f).map(|update| update.data))
+        }) {
+            Ok((
+                file_id,
+                Some(UniversalReadUpdate {
+                    export_generation_increment,
+                    new_vars,
+                    callbacks,
+                    ok_to_save,
+                }),
+            )) => {
+                self.export_generation += export_generation_increment;
+                self.vars = new_vars;
+                self.ok_to_save = ok_to_save;
+                self.last_read_file_id = file_id;
+                Some(callbacks)
+            }
+            Ok((_, None)) => {
+                // WARNING: Do not update self.file_id here, as this would interfere with
+                // self.ok_to_save.
+                None
+            }
+            Err(e) => {
+                FLOG!(uvar_file, "Failed to load from universal variable file:", e);
+                None
+            }
+        }
     }
 
-    // Load environment variables from the opened [`File`] `file`. It must be mutable because we
-    // will read from the underlying fd.
-    fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList) {
+    /// Load environment variables from the [`file`](`File`).
+    /// If the file has not been modified since the last read, [`None`] is returned.
+    /// In this case, `self.ok_to_save` needs to be consulted to decide whether to rewrite the file.
+    /// Otherwise, the [`Some`] variant contains updated data and indicates whether the file should
+    /// be rewritten..
+    // IMPORTANT: Callers of this code assume that a return value of None means that the file id has
+    // not changed. Do not return None in other situations without modifying the callers
+    // accordingly. Otherwise, problems with self.ok_to_save are expected to occur.
+    fn load_from_file(&self, file: &File) -> Option<PotentialUpdate<UniversalReadUpdate>> {
         // Get the dev / inode.
         let current_file_id = file_id_for_file(file);
         if current_file_id == self.last_read_file_id {
             FLOG!(uvar_file, "universal log sync elided based on fstat()");
+            None
         } else {
             // Read a variables table from the file.
             let mut new_vars = VarTable::new();
@@ -403,176 +421,36 @@ fn load_from_file(&mut self, file: &mut File, callbacks: &mut CallbackDataList)
 
             // Hacky: if the read format is in the future, avoid overwriting the file: never try to
             // save.
-            if format == UvarFormat::future {
-                self.ok_to_save = false;
-            }
+            let do_save = format != UvarFormat::future;
 
             // Announce changes and update our exports generation.
-            self.generate_callbacks_and_update_exports(&new_vars, callbacks);
+            let (export_generation_increment, callbacks) =
+                self.generate_callbacks_and_update_exports(&new_vars);
 
             // Acquire the new variables.
-            self.acquire_variables(new_vars);
-            self.last_read_file_id = current_file_id;
+            self.acquire_variables(&mut new_vars);
+            Some(PotentialUpdate {
+                do_save,
+                data: UniversalReadUpdate {
+                    export_generation_increment,
+                    new_vars,
+                    callbacks,
+                    ok_to_save: do_save,
+                },
+            })
         }
     }
 
-    // Functions concerned with saving.
-    fn open_and_acquire_lock(&mut self) -> Option<File> {
-        // Attempt to open the file for reading at the given path, atomically acquiring a lock. On BSD,
-        // we can use O_EXLOCK. On Linux, we open the file, take a lock, and then compare fstat() to
-        // stat(); if they match, it means that the file was not replaced before we acquired the lock.
-        //
-        // We pass O_RDONLY with O_CREAT; this creates a potentially empty file. We do this so that we
-        // have something to lock on.
-        let mut locked_by_open = false;
-        let mut flags = OFlag::O_RDWR | OFlag::O_CREAT;
-
-        if !O_EXLOCK.is_empty() && self.do_flock {
-            flags |= O_EXLOCK;
-            locked_by_open = true;
-        }
-
-        loop {
-            let mut file =
-                match wopen_cloexec(&self.vars_path, flags, Mode::from_bits_truncate(0o644)) {
-                    Ok(file) => file,
-                    Err(nix::Error::EINTR) => continue,
-                    Err(err) => {
-                        if !O_EXLOCK.is_empty() {
-                            if flags.contains(O_EXLOCK)
-                                && [nix::Error::ENOTSUP, nix::Error::EOPNOTSUPP].contains(&err)
-                            {
-                                // Filesystem probably does not support locking. Give up on locking.
-                                // Note that on Linux the two errno symbols have the same value but on BSD they're
-                                // different.
-                                flags &= !O_EXLOCK;
-                                self.do_flock = false;
-                                locked_by_open = false;
-                                continue;
-                            }
-                        }
-                        FLOG!(
-                            error,
-                            wgettext_fmt!(
-                                "Unable to open universal variable file '%s': %s",
-                                &self.vars_path,
-                                err.to_string()
-                            )
-                        );
-                        return None;
-                    }
-                };
-
-            // Lock if we want to lock and open() didn't do it for us.
-            // If flock fails, give up on locking forever.
-            if self.do_flock && !locked_by_open {
-                if !flock_uvar_file(&mut file) {
-                    self.do_flock = false;
-                }
-            }
-
-            // Hopefully we got the lock. However, it's possible the file changed out from under us
-            // while we were waiting for the lock. Make sure that didn't happen.
-            if file_id_for_file(&file) != file_id_for_path(&self.vars_path) {
-                // Oops, it changed! Try again.
-                drop(file);
-            } else {
-                return Some(file);
-            }
-        }
-    }
-
-    fn open_temporary_file(
-        &mut self,
-        directory: &wstr,
-        out_path: &mut WString,
-    ) -> Result<File, Errno> {
-        // Create and open a temporary file for writing within the given directory. Try to create a
-        // temporary file, up to 10 times. We don't use mkstemps because we want to open it CLO_EXEC.
-        // This should almost always succeed on the first try.
-        assert!(!string_suffixes_string(L!("/"), directory));
-
-        let mut attempt = 0;
-        let tmp_name_template = directory.to_owned() + L!("/fishd.tmp.XXXXXX");
-        let result = loop {
-            attempt += 1;
-            let result = fish_mkstemp_cloexec(wcs2zstring(&tmp_name_template));
-            match (result, attempt) {
-                (Ok(r), _) => break r,
-                (Err(e), 10) => {
-                    FLOG!(
-                        error,
-                        // We previously used to log a copy of the buffer we expected mk(o)stemp to
-                        // update with the new path, but mkstemp(3) says the contents of the buffer
-                        // are undefined in case of EEXIST, but left unchanged in case of EINVAL. So
-                        // just log the original template we pass in to the function instead.
-                        wgettext_fmt!(
-                            "Unable to create temporary file '%ls': %s",
-                            &tmp_name_template,
-                            e.to_string()
-                        )
-                    );
-                    return Err(e);
-                }
-                _ => continue,
-            }
-        };
-
-        *out_path = str2wcstring(result.1.as_bytes());
-        Ok(result.0)
-    }
-
-    /// Writes our state to the fd. path is provided only for error reporting.
-    fn write_to_file(&mut self, file: &mut File, path: &wstr) -> std::io::Result<()> {
-        let contents = Self::serialize_with_vars(&self.vars);
-
-        let res = file.write_all(&contents);
-        match res.as_ref() {
-            Ok(()) => {
-                // Since we just wrote out this file, it matches our internal state; pretend we read from it.
-                self.last_read_file_id = file_id_for_file(file);
-            }
-            Err(err) => {
-                let error = Errno(err.raw_os_error().unwrap());
-                FLOG!(
-                    error,
-                    wgettext_fmt!(
-                        "Unable to write to universal variables file '%ls': %s",
-                        path,
-                        error.to_string()
-                    ),
-                );
-            }
-        }
-
-        // We don't close the file.
-        res
-    }
-
-    fn move_new_vars_file_into_place(&mut self, src: &wstr, dst: &wstr) -> bool {
-        let ret = wrename(src, dst);
-        if ret != 0 {
-            let error = errno();
-            FLOG!(
-                error,
-                wgettext_fmt!(
-                    "Unable to rename file from '%ls' to '%ls': %s",
-                    src,
-                    dst,
-                    error.to_string()
-                )
-            );
-        }
-        ret == 0
-    }
-
-    // Given a variable table, generate callbacks representing the difference between our vars and
-    // the new vars. Also update our exports generation count as necessary.
+    /// Given a variable table, generate callbacks representing the difference between our vars and
+    /// the new vars.
+    /// Returns by how much the exports generation count should be incremented, as well as a
+    /// callback list.
     fn generate_callbacks_and_update_exports(
-        &mut self,
+        &self,
         new_vars: &VarTable,
-        callbacks: &mut CallbackDataList,
-    ) {
+    ) -> (u64, CallbackDataList) {
+        let mut export_generation_increment = 0;
+        let mut callbacks = CallbackDataList::new();
         // Construct callbacks for erased values.
         for (key, value) in &self.vars {
             // Skip modified values.
@@ -587,7 +465,7 @@ fn generate_callbacks_and_update_exports(
                     val: None,
                 });
                 if value.exports() {
-                    self.export_generation += 1;
+                    export_generation_increment += 1;
                 }
             }
         }
@@ -606,7 +484,7 @@ fn generate_callbacks_and_update_exports(
             let export_changed = old_exports != new_entry.exports();
             let value_changed = existing.is_some_and(|v| v != new_entry);
             if export_changed || value_changed {
-                self.export_generation += 1;
+                export_generation_increment += 1;
             }
             if existing.is_none() || export_changed || value_changed {
                 // Value is set for the first time, or has changed.
@@ -616,11 +494,11 @@ fn generate_callbacks_and_update_exports(
                 });
             }
         }
+        (export_generation_increment, callbacks)
     }
 
-    // Given a variable table, copy unmodified values into self.
-    fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) {
-        // Copy modified values from existing vars to vars_to_acquire.
+    /// Copy modified values from existing vars to `vars_to_acquire`.
+    fn acquire_variables(&self, vars_to_acquire: &mut VarTable) {
         for key in &self.modified {
             match self.vars.get(key) {
                 None => {
@@ -628,15 +506,11 @@ fn acquire_variables(&mut self, mut vars_to_acquire: VarTable) {
                     vars_to_acquire.remove(key);
                 }
                 Some(src) => {
-                    // The value has been modified. Copy it over. Note we can destructively modify the
-                    // source entry in vars since we are about to get rid of this->vars entirely.
+                    // The value has been modified. Copy it over.
                     vars_to_acquire.insert(key.clone(), src.clone());
                 }
             }
         }
-
-        // We have constructed all the callbacks and updated vars_to_acquire. Acquire it!
-        self.vars = vars_to_acquire;
     }
 
     fn populate_1_variable(
@@ -747,81 +621,6 @@ fn read_message_internal(file: &File, vars: &mut VarTable) -> UvarFormat {
 
         Self::populate_variables(&contents, vars)
     }
-
-    // Write our file contents.
-    // Return true on success, false on failure.
-    fn save(&mut self, directory: &wstr) -> bool {
-        use crate::common::ScopeGuard;
-        assert!(self.ok_to_save, "It's not OK to save");
-
-        // Open adjacent temporary file.
-        let mut private_file_path = WString::new();
-        let Ok(mut private_file) = self.open_temporary_file(directory, &mut private_file_path)
-        else {
-            return false;
-        };
-        // unlink pfp upon failure. In case of success, it (already) won't exist.
-        let delete_pfp = ScopeGuard::new(private_file_path, |path| {
-            wunlink(&path);
-        });
-        let private_file_path = &delete_pfp;
-
-        // Write to it.
-        if let Err(e) = self.write_to_file(&mut private_file, private_file_path) {
-            FLOG!(uvar_file, "universal log write_to_file() failed:", e);
-            return false;
-        }
-
-        let real_path = wrealpath(&self.vars_path).unwrap_or_else(|| self.vars_path.clone());
-
-        // Ensure we maintain ownership and permissions (#2176).
-        if let Ok(md) = wstat(&real_path) {
-            // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73
-            if unsafe { libc::fchown(private_file.as_raw_fd(), md.uid(), md.gid()) } == -1 {
-                FLOG!(uvar_file, "universal log fchown() failed:", errno());
-            }
-            if let Err(e) = private_file.set_permissions(md.permissions()) {
-                FLOG!(uvar_file, "universal log setting permissions failed:", e);
-            }
-        }
-
-        // Linux by default stores the mtime with low precision, low enough that updates that occur
-        // in quick succession may result in the same mtime (even the nanoseconds field). So
-        // manually set the mtime of the new file to a high-precision clock. Note that this is only
-        // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other
-        // platforms we tend to notice the file has changed due to a different inode (or file size!)
-        //
-        // The current time within the Linux kernel is cached, and generally only updated on a timer
-        // interrupt. So if the timer interrupt is running at 10 milliseconds, the cached time will
-        // only be updated once every 10 milliseconds.
-        //
-        // It's probably worth finding a simpler solution to this. The tests ran into this, but it's
-        // unlikely to affect users.
-        #[cfg(any(target_os = "linux", target_os = "android"))]
-        {
-            let mut times: [libc::timespec; 2] = unsafe { std::mem::zeroed() };
-            times[0].tv_nsec = libc::UTIME_OMIT; // don't change ctime
-            if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } == 0 {
-                unsafe {
-                    libc::futimens(private_file.as_raw_fd(), &times[0]);
-                }
-            }
-        }
-
-        // Apply new file.
-        if !self.move_new_vars_file_into_place(private_file_path, &real_path) {
-            FLOG!(
-                uvar_file,
-                "universal log move_new_vars_file_into_place() failed"
-            );
-            return false;
-        }
-
-        // Success at last. All of our modified variables have now been written out.
-        self.modified.clear();
-        ScopeGuard::cancel(delete_pfp);
-        true
-    }
 }
 
 /// Return the default variable path, or an empty string on failure.
@@ -1002,29 +801,6 @@ fn encode_serialized(vals: &[WString]) -> WString {
     join_strings(vals, UVAR_ARRAY_SEP)
 }
 
-/// Try locking the file.
-/// Return true on success, false on error.
-fn flock_uvar_file(file: &mut File) -> bool {
-    let start_time = timef();
-    while unsafe { libc::flock(file.as_raw_fd(), LOCK_EX) } == -1 {
-        if errno().0 != EINTR {
-            return false; // do nothing per issue #2149
-        }
-    }
-    let duration = timef() - start_time;
-    if duration > 0.25 {
-        FLOG!(
-            warning,
-            wgettext_fmt!(
-                "Locking the universal var file took too long (%.3f seconds).",
-                duration
-            )
-        );
-        return false;
-    }
-    true
-}
-
 fn skip_spaces(mut s: &wstr) -> &wstr {
     while s.starts_with(L!(" ")) || s.starts_with(L!("\t")) {
         s = &s[1..];

src/fallback.rs

@@ -5,12 +5,8 @@
 
 use crate::wchar::prelude::*;
 use crate::widecharwidth::{WcLookupTable, WcWidth};
-use errno::{errno, Errno};
 use once_cell::sync::Lazy;
 use std::cmp;
-use std::ffi::CString;
-use std::fs::File;
-use std::os::fd::FromRawFd;
 use std::sync::atomic::{AtomicIsize, Ordering};
 
 /// Width of ambiguous East Asian characters and, as of TR11, all private-use characters.
@@ -115,32 +111,6 @@ pub fn fish_wcswidth(s: &wstr) -> isize {
     result
 }
 
-// Replacement for mkostemp(str, O_CLOEXEC)
-// This uses mkostemp if available,
-// otherwise it uses mkstemp followed by fcntl
-pub fn fish_mkstemp_cloexec(name_template: CString) -> Result<(File, CString), Errno> {
-    let name = name_template.into_raw();
-    #[cfg(not(apple))]
-    let fd = {
-        use libc::O_CLOEXEC;
-        unsafe { libc::mkostemp(name, O_CLOEXEC) }
-    };
-    #[cfg(apple)]
-    let fd = {
-        use libc::{FD_CLOEXEC, F_SETFD};
-        let fd = unsafe { libc::mkstemp(name) };
-        if fd != -1 {
-            unsafe { libc::fcntl(fd, F_SETFD, FD_CLOEXEC) };
-        }
-        fd
-    };
-    if fd == -1 {
-        Err(errno())
-    } else {
-        unsafe { Ok((File::from_raw_fd(fd), CString::from_raw(name))) }
-    }
-}
-
 pub fn wcscasecmp(lhs: &wstr, rhs: &wstr) -> cmp::Ordering {
     wcscasecmp_fuzzy(lhs, rhs, std::convert::identity)
 }

src/flog.rs

@@ -124,6 +124,8 @@ pub fn all_categories() -> Vec<&'static category_t> {
 
         (profile_history, "profile-history", "History performance measurements");
 
+        (synced_file_access, "synced-file-access", "Synchronized file access");
+
         (iothread, "iothread", "Background IO thread events");
         (fd_monitor, "fd-monitor", "FD monitor events");
 

src/fs.rs

@@ -0,0 +1,432 @@
+use crate::{
+    common::{str2wcstring, wcs2zstring, ScopeGuard},
+    fds::wopen_cloexec,
+    path::{path_remoteness, DirRemoteness},
+    wchar::prelude::*,
+    wutil::{
+        file_id_for_file, file_id_for_path, wdirname, wrename, wunlink, FileId, INVALID_FILE_ID,
+    },
+    FLOG, FLOGF,
+};
+use errno::errno;
+use libc::{c_int, fchown, flock, LOCK_EX, LOCK_SH};
+use nix::{fcntl::OFlag, sys::stat::Mode};
+use std::{
+    ffi::CString,
+    fs::File,
+    io::Seek,
+    os::{
+        fd::{AsRawFd, FromRawFd},
+        unix::fs::MetadataExt,
+    },
+};
+
+// Replacement for mkostemp(str, O_CLOEXEC)
+// This uses mkostemp if available,
+// otherwise it uses mkstemp followed by fcntl
+fn fish_mkstemp_cloexec(name_template: CString) -> std::io::Result<(File, CString)> {
+    let name = name_template.into_raw();
+    #[cfg(not(apple))]
+    let fd = {
+        use libc::O_CLOEXEC;
+        unsafe { libc::mkostemp(name, O_CLOEXEC) }
+    };
+    #[cfg(apple)]
+    let fd = {
+        use libc::{FD_CLOEXEC, F_SETFD};
+        let fd = unsafe { libc::mkstemp(name) };
+        if fd != -1 {
+            unsafe { libc::fcntl(fd, F_SETFD, FD_CLOEXEC) };
+        }
+        fd
+    };
+    if fd == -1 {
+        Err(std::io::Error::from(errno()))
+    } else {
+        unsafe { Ok((File::from_raw_fd(fd), CString::from_raw(name))) }
+    }
+}
+
+/// Creates a temporary file created according to the template and its name if successful.
+pub fn create_temporary_file(name_template: &wstr) -> std::io::Result<(File, WString)> {
+    let (fd, c_string_template) = loop {
+        match fish_mkstemp_cloexec(wcs2zstring(name_template)) {
+            Ok(tmp_file_data) => break tmp_file_data,
+            Err(e) => match e.kind() {
+                std::io::ErrorKind::Interrupted => {}
+                _ => {
+                    FLOG!(
+                        error,
+                        wgettext_fmt!(
+                            "Unable to create temporary file '%ls': %s",
+                            name_template,
+                            e
+                        )
+                    );
+
+                    return Err(e);
+                }
+            },
+        }
+    };
+    Ok((fd, str2wcstring(c_string_template.to_bytes())))
+}
+
+/// Use this struct for all accesses to file which need mutual exclusion.
+/// Otherwise, races on the file are possible.
+/// The lock is released when this struct is dropped.
+pub struct LockedFile {
+    /// This is the file which requires mutual exclusion.
+    /// It should only be accessed through this struct,
+    /// because the locks used here do not protect from other accesses to the file.
+    data_file: File,
+    /// The file descriptor of the parent directory, used for locking.
+    /// The lock is not placed on the data file directly due to issues with renaming.
+    /// If the data file is renamed after opening and before locking it,
+    /// There are two independent files around, whose locks do not interact.
+    /// In some cases this can be identified by checking file identifiers and timestamps,
+    /// but even with such checks races and corresponding file corruption can occur.
+    /// It is simpler to lock a different path, which does not change.
+    ///
+    /// We may fail to lock (e.g. on lockless NFS - see issue #685.
+    /// In that case, we proceed as if locking succeeded.
+    /// This might result in corruption,
+    /// but the alternative of not being able to access the file at all is not desirable either.
+    _locked_fd: File,
+}
+
+pub const LOCKED_FILE_MODE: Mode = Mode::from_bits_truncate(0o600);
+
+pub enum LockingMode {
+    Shared,
+    Exclusive(WriteMethod),
+}
+pub enum WriteMethod {
+    Append,
+    RenameIntoPlace,
+}
+
+impl LockingMode {
+    pub fn flock_op(&self) -> c_int {
+        match self {
+            Self::Shared => LOCK_SH,
+            Self::Exclusive(_) => LOCK_EX,
+        }
+    }
+
+    pub fn file_flags(&self) -> OFlag {
+        match self {
+            Self::Shared => OFlag::O_RDONLY,
+            Self::Exclusive(WriteMethod::Append) => {
+                OFlag::O_WRONLY | OFlag::O_APPEND | OFlag::O_CREAT
+            }
+            Self::Exclusive(WriteMethod::RenameIntoPlace) => OFlag::O_RDONLY | OFlag::O_CREAT,
+        }
+    }
+}
+
+impl LockedFile {
+    /// Creates a [`LockedFile`].
+    /// Use this for any access to a which requires mutual exclusion, as it ensures correct locking.
+    /// Use [`LockingMode::Exclusive`] if you want to modify the file in any way.
+    /// Otherwise you should use [`LockingMode::Shared`].
+    /// Two modes of modification are supported:
+    /// - Appending
+    /// - Writing to a temporary file which is then renamed into place.
+    /// File flags are derived from the [`LockingMode`].
+    /// `file_name` should just be a name, not a full path.
+    pub fn new(locking_mode: LockingMode, file_path: &wstr) -> std::io::Result<Self> {
+        let dir_path = wdirname(file_path);
+
+        if path_remoteness(dir_path) == DirRemoteness::remote {
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::Unsupported,
+                "Directory considered remote. Locking is disabled on remote file systems.",
+            ));
+        }
+
+        // We start by locking the directory.
+        // This is required to avoid racing modifications by other threads/processes.
+        let dir_fd = wopen_cloexec(dir_path, OFlag::O_RDONLY, Mode::empty())?;
+
+        // Try locking the directory. Retry if locking was interrupted.
+        while unsafe { flock(dir_fd.as_raw_fd(), locking_mode.flock_op()) } == -1 {
+            let err = std::io::Error::last_os_error();
+            if err.kind() != std::io::ErrorKind::Interrupted {
+                return Err(err);
+            }
+        }
+
+        // Open the data file
+        let data_file = wopen_cloexec(file_path, locking_mode.file_flags(), LOCKED_FILE_MODE)?;
+
+        Ok(Self {
+            data_file,
+            _locked_fd: dir_fd,
+        })
+    }
+
+    pub fn get(&self) -> &File {
+        &self.data_file
+    }
+
+    pub fn get_mut(&mut self) -> &mut File {
+        &mut self.data_file
+    }
+}
+
+/// Runs the `load` function, which should see a consistent state of the file at `path`.
+/// To ensure a consistent state, we use locks to prevent modifications by others.
+/// If locking is unavailable, the `load` function might be executed multiple times,
+/// until it manages a run without the file being modified in the meantime, or until the maximum
+/// number of allowed attempts is reached.
+/// If the file does not exist this function will return an error.
+pub fn lock_and_load<F, UserData>(path: &wstr, load: F) -> std::io::Result<(FileId, UserData)>
+where
+    F: Fn(&File) -> std::io::Result<UserData>,
+{
+    match LockedFile::new(LockingMode::Shared, path) {
+        Ok(locked_file) => {
+            return Ok((
+                file_id_for_file(locked_file.get()),
+                load(locked_file.get())?,
+            ));
+        }
+        Err(e) => {
+            FLOGF!(
+                synced_file_access,
+                "Error acquiring shared lock on the directory of '%s': %s",
+                path,
+                e,
+            );
+            // This function might be called when the file does not exist.
+            if e.kind() == std::io::ErrorKind::NotFound {
+                // There is no point in continuing in this function if the file does not
+                // exist.
+                return Err(e);
+            }
+        }
+    }
+
+    FLOG!(
+        synced_file_access,
+        "flock-based locking is disabled. Using fallback implementation."
+    );
+
+    // Fallback implementation for situations where locking is unavailable.
+    let max_attempts = 1000;
+    for _ in 0..max_attempts {
+        let initial_file_id = file_id_for_path(path);
+        // If we cannot open the file, there is nothing we can do,
+        // so just return immediately.
+        let file = wopen_cloexec(path, OFlag::O_RDONLY, Mode::empty())?;
+        let loaded_data = match load(&file) {
+            Ok(update_data) => update_data,
+            Err(_) => {
+                // Retry if load function failed. Because we do not hold a lock, this might be
+                // caused by concurrent modifications.
+                continue;
+            }
+        };
+
+        let final_file_id = file_id_for_path(path);
+        if initial_file_id != final_file_id {
+            continue;
+        }
+        // If the file id did not change, we assume that we loaded a consistent state.
+        return Ok((final_file_id, loaded_data));
+    }
+    Err(std::io::Error::new(std::io::ErrorKind::Other, "Failed to update the file. Locking is disabled, and the fallback code did not succeed within the permissible number of attempts."))
+}
+
+pub struct PotentialUpdate<UserData> {
+    pub do_save: bool,
+    pub data: UserData,
+}
+
+/// Use this function for updating a file based on its current content,
+/// for files which might be accessed at the same time by other threads/processes.
+/// The basic principle is to create a temporary file, take a lock on the file to be rewritten,
+/// do the rewrite (which might involve reading the old file contents),
+/// and finally release the lock.
+/// Error handling (especially the case where locking is not possible) makes the implementation
+/// non-straightforward.
+///
+/// # Arguments
+///
+/// - `path`: The path to the file which should be updated.
+/// - `rewrite`: The function which handles reading from the file and writing to a temporary file.
+/// The first argument is for the file to read from, the second for the temporary file to write to.
+/// On success, the value returned by `rewrite` is included in this functions return value. Be
+/// careful about side effects of `rewrite`. It might get executed multiple times. Try to avoid
+/// side effects and instead extract any data you might need and return them on success.
+/// Then, apply the desired side effects once this function has returned successfully.
+///
+/// # Return value
+///
+/// On success, the [`FileId`] of the rewritten file is returned, alongside the value returned by
+/// `rewrite`. Note that if locking is unavailable, the [`FileId`] might be the id of a different
+/// version of the file, which was written after this function renamed the temporary file to `path`
+/// but before we obtained the [`FileId`] from `path`. This is a race condition we do not detect.
+pub fn rewrite_via_temporary_file<F, UserData>(
+    path: &wstr,
+    rewrite: F,
+) -> std::io::Result<(FileId, PotentialUpdate<UserData>)>
+where
+    F: Fn(&File, &mut File) -> std::io::Result<PotentialUpdate<UserData>>,
+{
+    /// Updates the metadata of the `new_file` to match the `old_file`.
+    /// Also updates the mtime of the `new_file` to the current time manually, to work around
+    /// operating systems which do not update the internal time used for such time stamps
+    /// frequently enough. This is important for [`FileId`] comparisons.
+    fn update_metadata(old_file: &File, new_file: &File) {
+        // Ensure we maintain the ownership and permissions of the original (#2355). If the
+        // stat fails, we assume (hope) our default permissions are correct. This
+        // corresponds to e.g. someone running sudo -E as the very first command. If they
+        // did, it would be tricky to set the permissions correctly. (bash doesn't get this
+        // case right either).
+        if let Ok(md) = old_file.metadata() {
+            // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73
+            if unsafe { fchown(new_file.as_raw_fd(), md.uid(), md.gid()) } == -1 {
+                FLOG!(
+                    synced_file_access,
+                    "Error when changing ownership of file:",
+                    errno::errno()
+                );
+            }
+            if let Err(e) = new_file.set_permissions(md.permissions()) {
+                FLOG!(synced_file_access, "Error when changing mode of file:", e);
+            }
+        } else {
+            FLOG!(synced_file_access, "Could not get metadata for file");
+        }
+        // Linux by default stores the mtime with low precision, low enough that updates that occur
+        // in quick succession may result in the same mtime (even the nanoseconds field). So
+        // manually set the mtime of the new file to a high-precision clock. Note that this is only
+        // necessary because Linux aggressively reuses inodes, causing the ABA problem; on other
+        // platforms we tend to notice the file has changed due to a different inode.
+        //
+        // The current time within the Linux kernel is cached, and generally only updated on a timer
+        // interrupt. So if the timer interrupt is running at 10 milliseconds, the cached time will
+        // only be updated once every 10 milliseconds.
+        #[cfg(any(target_os = "linux", target_os = "android"))]
+        {
+            let mut times: [libc::timespec; 2] = unsafe { std::mem::zeroed() };
+            times[0].tv_nsec = libc::UTIME_OMIT; // don't change atime
+            if unsafe { libc::clock_gettime(libc::CLOCK_REALTIME, &mut times[1]) } == 0 {
+                unsafe {
+                    // This accesses both times[0] and times[1]. Check `utimensat(2)` for details.
+                    libc::futimens(new_file.as_raw_fd(), &times[0]);
+                }
+            }
+        }
+    }
+
+    /// Renames a file from `old_name` to `new_name`.
+    fn rename(old_name: &wstr, new_name: &wstr) -> std::io::Result<()> {
+        if wrename(old_name, new_name) == -1 {
+            let error_number = errno::errno();
+            FLOG!(
+                error,
+                wgettext_fmt!("Error when renaming file: %s", error_number.to_string())
+            );
+            return Err(std::io::Error::from(error_number));
+        }
+        Ok(())
+    }
+
+    const TMP_FILE_SUFFIX: &wstr = L!(".XXXXXX");
+    let tmp_file_template = path.to_owned() + TMP_FILE_SUFFIX;
+    let (tmp_file, tmp_name) = create_temporary_file(&tmp_file_template)?;
+    // Ensure that the temporary file is unlinked when this function returns.
+    let mut tmp_file = ScopeGuard::new(tmp_file, |_| {
+        wunlink(&tmp_name);
+    });
+
+    // We want to rewrite the file.
+    // To avoid issues with crashes during writing,
+    // we write to a temporary file and once we are done, this file is renamed such that it
+    // replaces the original file.
+    // To avoid races, we need to have exclusive access to the file for the entire
+    // duration, which we get via an exclusive lock on the parent directory.
+    // Taking a shared lock first and later upgrading to an exclusive one could result in a
+    // deadlock, so we take an exclusive one immediately.
+    match LockedFile::new(LockingMode::Exclusive(WriteMethod::RenameIntoPlace), path) {
+        Ok(locked_file) => {
+            let potential_update = rewrite(locked_file.get(), &mut tmp_file)?;
+            if potential_update.do_save {
+                update_metadata(locked_file.get(), &tmp_file);
+                rename(&tmp_name, path)?;
+            }
+            return Ok((file_id_for_path(path), potential_update));
+        }
+        Err(e) => {
+            FLOGF!(
+                synced_file_access,
+                "Error acquiring exclusive lock on the directory of '%s': %s",
+                path,
+                e,
+            );
+        }
+    }
+
+    // If this is reached, we assume that locking is not available so we use a fallback
+    // implementation which tries to avoid race conditions, but in the case of contention it is
+    // possible that some writes are lost.
+
+    FLOG!(
+        synced_file_access,
+        "flock-based locking is disabled. Using fallback implementation."
+    );
+
+    // Give up after this many unsuccessful attempts.
+    // TODO: which value, should this be a constant shared with other retrying logic?
+    let max_attempts = 1000;
+    for _ in 0..max_attempts {
+        // Truncate tmp_file for the next attempt to get rid of data potentially written in the
+        // previous iteration.
+        tmp_file.set_len(0)?;
+        tmp_file.seek(std::io::SeekFrom::Start(0)).unwrap();
+
+        // If the file does not exist yet, this will be `INVALID_FILE_ID`.
+        let initial_file_id = file_id_for_path(path);
+        // If we cannot open the file, there is nothing we can do,
+        // so just return immediately.
+        let old_file = wopen_cloexec(path, OFlag::O_RDONLY | OFlag::O_CREAT, LOCKED_FILE_MODE)?;
+        let opened_file_id = file_id_for_file(&old_file);
+        if initial_file_id != INVALID_FILE_ID && initial_file_id != opened_file_id {
+            // File ID changed (and not just because the file was created by us).
+            continue;
+        }
+        let Ok(potential_update) = rewrite(&old_file, &mut tmp_file) else {
+            // Retry if rewrite function failed. Because we do not hold a lock, this might be
+            // caused by concurrent modifications.
+            continue;
+        };
+
+        if potential_update.do_save {
+            update_metadata(&old_file, &tmp_file);
+        }
+
+        let mut final_file_id = file_id_for_path(path);
+        if opened_file_id != final_file_id {
+            continue;
+        }
+
+        // If we reach this point, the file ID did not change while we read the old file and wrote
+        // to `tmp_file`. Now we replace the old file with the `tmp_file`.
+        // Note that we cannot prevent races here.
+        // If the file is modified by someone else between the syscall for determining the [`FileId`]
+        // and the rename syscall, these modifications will be lost.
+
+        if potential_update.do_save {
+            // Do not retry on rename failures, as it is unlikely that these will disappear if we retry.
+            rename(&tmp_name, path)?;
+            final_file_id = file_id_for_path(path);
+        }
+        // Note that this might not match the version of the file we just wrote.
+        // (If we did write.)
+        return Ok((final_file_id, potential_update));
+    }
+    Err(std::io::Error::new(std::io::ErrorKind::Other, "Failed to update the file. Locking is disabled, and the fallback code did not succeed within the permissible number of attempts."))
+}

src/history.rs

@@ -2,60 +2,50 @@
 //!
 //! 1. All history files are append-only. Data, once written, is never modified.
 //!
-//! 2. A history file may be re-written ("vacuumed"). This involves reading in the file and writing a
-//! new one, while performing maintenance tasks: discarding items in an LRU fashion until we reach
-//! the desired maximum count, removing duplicates, and sorting them by timestamp (eventually, not
-//! implemented yet). The new file is atomically moved into place via rename().
+//! 2. A history file may be re-written ("vacuumed"). This involves reading in the file and writing
+//!    a new one, while performing maintenance tasks: discarding items in an LRU fashion until we
+//!    reach the desired maximum count, removing duplicates, and sorting them by timestamp
+//!    (eventually, not implemented yet). The new file is atomically moved into place via `rename()`.
 //!
-//! 3. History files are mapped in via mmap(). Before the file is mapped, the file takes a fcntl read
-//! lock. The purpose of this lock is to avoid seeing a transient state where partial data has been
-//! written to the file.
+//! 3. History files are mapped in via `mmap()`. This allows only storing one `usize` per item (its
+//!    offset), and lazily loading items on demand, which reduces memory consumption.
 //!
-//! 4. History is appended to under a fcntl write lock.
-//!
-//! 5. The chaos_mode boolean can be set to true to do things like lower buffer sizes which can
-//! trigger race conditions. This is useful for testing.
-//!
-//! Locking on remote filesystems may hang for an unacceptably long time. For that reason, fish
-//! does not take locks on the file if it believes the history file is on a remote filesystem,
-//! or if the mmap fails with ENODEV, or if the first lock attempt takes excessively long.
-//! Eliding locks means that two concurrent shell sessions with a remote history file may, in
-//! rare cases with multiple simultaneous shell sessions, lose a history item; this is
-//! considered preferable to hanging the the shell waiting for a lock.
+//! 4. Accesses to the history file need to be synchronized. This is achieved by functionality in
+//!    `src/fs.rs`. By default, `flock()` is used for locking. If that is unavailable, an imperfect
+//!    fallback solution attempts to detect races and retries if a race is detected.
 
 use crate::{
-    common::cstr2wcstring, env::EnvVar, wcstringutil::trim,
-    wutil::fileid::file_id_for_path_or_error,
+    common::cstr2wcstring,
+    env::EnvVar,
+    fs::{
+        lock_and_load, rewrite_via_temporary_file, LockedFile, LockingMode, PotentialUpdate,
+        WriteMethod, LOCKED_FILE_MODE,
+    },
+    wcstringutil::trim,
 };
 use std::{
     borrow::Cow,
     collections::{BTreeMap, HashMap, HashSet},
     ffi::CString,
     fs::File,
-    io::{BufRead, Read, Seek, SeekFrom, Write},
+    io::{BufRead, Read, Write},
     mem::MaybeUninit,
     num::NonZeroUsize,
     ops::ControlFlow,
-    os::{fd::AsRawFd, unix::fs::MetadataExt},
     sync::{Arc, Mutex, MutexGuard},
     time::{Duration, SystemTime, UNIX_EPOCH},
 };
 
 use bitflags::bitflags;
-use libc::{fchown, flock, EINTR, LOCK_EX, LOCK_SH, LOCK_UN};
 use lru::LruCache;
 use nix::{fcntl::OFlag, sys::stat::Mode};
 use rand::Rng;
 
 use crate::{
     ast::{self, Kind, Node},
-    common::{
-        str2wcstring, unescape_string, valid_var_name, wcs2zstring, CancelChecker,
-        UnescapeStringStyle,
-    },
+    common::{str2wcstring, unescape_string, valid_var_name, CancelChecker, UnescapeStringStyle},
     env::{EnvMode, EnvStack, Environment},
     expand::{expand_one, ExpandFlags},
-    fallback::fish_mkstemp_cloexec,
     fds::wopen_cloexec,
     flog::{FLOG, FLOGF},
     global_safety::RelaxedAtomicBool,
@@ -64,18 +54,13 @@
     operation_context::{OperationContext, EXPANSION_LIMIT_BACKGROUND},
     parse_constants::{ParseTreeFlags, StatementDecoration},
     parse_util::{parse_util_detect_errors, parse_util_unescape_wildcards},
-    path::{
-        path_get_config, path_get_data, path_get_data_remoteness, path_is_valid, DirRemoteness,
-    },
+    path::{path_get_config, path_get_data, path_is_valid},
     threads::{assert_is_background_thread, iothread_perform},
     util::{find_subslice, get_rng},
     wchar::prelude::*,
     wcstringutil::subsequence_in_string,
     wildcard::{wildcard_match, ANY_STRING},
-    wutil::{
-        file_id_for_file, file_id_for_path, wgettext_fmt, wrealpath, wrename, wstat, wunlink,
-        FileId, INVALID_FILE_ID,
-    },
+    wutil::{file_id_for_file, wgettext_fmt, wrealpath, wstat, wunlink, FileId, INVALID_FILE_ID},
 };
 
 mod file;
@@ -139,14 +124,6 @@ pub enum SearchDirection {
 /// Default buffer size for flushing to the history file.
 const HISTORY_OUTPUT_BUFFER_SIZE: usize = 64 * 1024;
 
-/// The file access mode we use for creating history files
-const HISTORY_FILE_MODE: Mode = Mode::from_bits_truncate(0o600);
-
-/// How many times we retry to save
-/// Saving may fail if the file is modified in between our opening
-/// the file and taking the lock
-const MAX_SAVE_TRIES: usize = 1024;
-
 pub const VACUUM_FREQUENCY: usize = 25;
 
 /// Output the contents `buffer` to `file` and clear the `buffer`.
@@ -219,22 +196,6 @@ fn add_item(&mut self, item: HistoryItem) {
     }
 }
 
-/// Returns the path for the history file for the given `session_id`, or `None` if it could not be
-/// loaded. If `suffix` is provided, append that suffix to the path; this is used for temporary files.
-fn history_filename(session_id: &wstr, suffix: &wstr) -> Option<WString> {
-    if session_id.is_empty() {
-        return None;
-    }
-
-    let mut result = path_get_data()?;
-
-    result.push('/');
-    result.push_utfstr(session_id);
-    result.push_utfstr(L!("_history"));
-    result.push_utfstr(suffix);
-    Some(result)
-}
-
 pub type PathList = Vec<WString>;
 pub type HistoryIdentifier = u64;
 
@@ -403,21 +364,38 @@ struct HistoryImpl {
     old_item_offsets: Vec<usize>,
 }
 
-/// If set, we gave up on file locking because it took too long.
-/// Note this is shared among all history instances.
-static ABANDONED_LOCKING: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
-
 impl HistoryImpl {
+    /// Returns the canonical path for the history file, or `Ok(None)` in private mode.
+    /// An error is returned if obtaining the data directory fails.
+    /// Because the `path_get_data` function does not return error information,
+    /// we cannot provide more detail about the reason for the failure here.
+    fn history_file_path(&self) -> std::io::Result<Option<WString>> {
+        if self.name.is_empty() {
+            return Ok(None);
+        }
+
+        let Some(mut path) = path_get_data() else {
+            return Err(std::io::Error::new(std::io::ErrorKind::NotFound, "Error obtaining data directory. This is a manually constructed error which does not indicate why this happened."));
+        };
+
+        path.push('/');
+        path.push_utfstr(&self.name);
+        path.push_utfstr(L!("_history"));
+        if let Some(canonicalized_path) = wrealpath(&path) {
+            Ok(Some(canonicalized_path))
+        } else {
+            Err(std::io::Error::new(
+                std::io::ErrorKind::Other,
+                format!("wrealpath failed to produce a canonical version of '{path}'."),
+            ))
+        }
+    }
+
     /// Add a new history item to the end. If `pending` is set, the item will not be returned by
     /// `item_at_index()` until a call to `resolve_pending()`. Pending items are tracked with an
     /// offset into the array of new items, so adding a non-pending item has the effect of resolving
     /// all pending items.
-    fn add(
-        &mut self,
-        item: HistoryItem,
-        pending: bool, /*=false*/
-        do_save: bool, /*=true*/
-    ) {
+    fn add(&mut self, item: HistoryItem, pending: bool, do_save: bool) {
         // We use empty items as sentinels to indicate the end of history.
         // Do not allow them to be added (#6032).
         if item.contents.is_empty() {
@@ -499,34 +477,18 @@ fn load_old_if_needed(&mut self) {
         self.loaded_old = true;
 
         let _profiler = TimeProfiler::new("load_old");
-        if let Some(filename) = history_filename(&self.name, L!("")) {
-            let Ok(mut file) = wopen_cloexec(&filename, OFlag::O_RDONLY, Mode::empty()) else {
-                return;
-            };
-
-            // Take a read lock to guard against someone else appending. This is released after
-            // getting the file's length. We will read the file after releasing the lock, but that's
-            // not a problem, because we never modify already written data. In short, the purpose of
-            // this lock is to ensure we don't see the file size change mid-update.
-            //
-            // We may fail to lock (e.g. on lockless NFS - see issue #685. In that case, we proceed
-            // as if it did not fail. The risk is that we may get an incomplete history item; this
-            // is unlikely because we only treat an item as valid if it has a terminating newline.
-            let locked = unsafe { Self::maybe_lock_file(&mut file, LOCK_SH) };
-            self.file_contents = HistoryFileContents::create(&mut file);
-            self.history_file_id = if self.file_contents.is_some() {
-                file_id_for_file(&file)
-            } else {
-                INVALID_FILE_ID
-            };
-            if locked {
-                unsafe {
-                    Self::unlock_file(&mut file);
+        if let Ok(Some(history_path)) = self.history_file_path() {
+            match lock_and_load(&history_path, HistoryFileContents::create) {
+                Ok((file_id, file_contents)) => {
+                    self.file_contents = Some(file_contents);
+                    self.history_file_id = file_id;
+                    let _profiler = TimeProfiler::new("populate_from_file_contents");
+                    self.populate_from_file_contents();
+                }
+                Err(e) => {
+                    FLOG!(history_file, "Error reading from history file:", e);
                 }
             }
-
-            let _profiler = TimeProfiler::new("populate_from_file_contents");
-            self.populate_from_file_contents();
         }
     }
 
@@ -575,8 +537,11 @@ fn remove_ephemeral_items(&mut self) {
     }
 
     /// Given an existing history file, write a new history file to `dst`.
-    /// Returns false on error, true on success
-    fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut File) -> bool {
+    fn rewrite_to_temporary_file(
+        &self,
+        existing_file: &File,
+        dst: &mut File,
+    ) -> std::io::Result<()> {
         // We are reading FROM existing_file and writing TO dst
 
         // Make an LRU cache to save only the last N elements.
@@ -584,32 +549,29 @@ fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut
 
         // Read in existing items (which may have changed out from underneath us, so don't trust our
         // old file contents).
-        if let Some(existing_file) = existing_file {
-            if let Some(local_file) = HistoryFileContents::create(existing_file) {
-                let mut cursor = 0;
-                while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) {
-                    // Try decoding an old item.
-                    let Some(old_item) = local_file.decode_item(offset) else {
-                        continue;
-                    };
+        if let Ok(local_file) = HistoryFileContents::create(existing_file) {
+            let mut cursor = 0;
+            while let Some(offset) = local_file.offset_of_next_item(&mut cursor, None) {
+                // Try decoding an old item.
+                let Some(old_item) = local_file.decode_item(offset) else {
+                    continue;
+                };
 
-                    // If old item is newer than session always erase if in deleted.
-                    if old_item.timestamp() > self.boundary_timestamp {
-                        if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) {
-                            continue;
-                        }
-                        lru.add_item(old_item);
-                    } else {
-                        // If old item is older and in deleted items don't erase if added by
-                        // clear_session.
-                        if old_item.is_empty()
-                            || self.deleted_items.get(old_item.str()) == Some(&false)
-                        {
-                            continue;
-                        }
-                        // Add this old item.
-                        lru.add_item(old_item);
+                // If old item is newer than session always erase if in deleted.
+                if old_item.timestamp() > self.boundary_timestamp {
+                    if old_item.is_empty() || self.deleted_items.contains_key(old_item.str()) {
+                        continue;
                     }
+                    lru.add_item(old_item);
+                } else {
+                    // If old item is older and in deleted items don't erase if added by
+                    // clear_session.
+                    if old_item.is_empty() || self.deleted_items.get(old_item.str()) == Some(&false)
+                    {
+                        continue;
+                    }
+                    // Add this old item.
+                    lru.add_item(old_item);
                 }
             }
         }
@@ -654,168 +616,47 @@ fn rewrite_to_temporary_file(&self, existing_file: Option<&mut File>, dst: &mut
                 "Error writing to temporary history file:",
                 err
             );
-
-            false
+            Err(err)
         } else {
-            true
+            Ok(())
         }
     }
 
     /// Saves history by rewriting the file.
-    fn save_internal_via_rewrite(&mut self) {
+    fn save_internal_via_rewrite(&mut self, history_path: &wstr) -> std::io::Result<()> {
         FLOGF!(
             history,
             "Saving %lu items via rewrite",
             self.new_items.len() - self.first_unwritten_new_item_index
         );
 
-        // We want to rewrite the file, while holding the lock for as briefly as possible
-        // To do this, we speculatively write a file, and then lock and see if our original file changed
-        // Repeat until we succeed or give up
-        let Some(possibly_indirect_target_name) = history_filename(&self.name, L!("")) else {
-            return;
-        };
-        let Some(tmp_name_template) = history_filename(&self.name, L!(".XXXXXX")) else {
-            return;
-        };
+        let rewrite =
+            |old_file: &File, tmp_file: &mut File| -> std::io::Result<PotentialUpdate<()>> {
+                self.rewrite_to_temporary_file(old_file, tmp_file)?;
+                Ok(PotentialUpdate {
+                    do_save: true,
+                    data: (),
+                })
+            };
 
-        // If the history file is a symlink, we want to rewrite the real file so long as we can find it.
-        let target_name =
-            wrealpath(&possibly_indirect_target_name).unwrap_or(possibly_indirect_target_name);
+        let (file_id, _) = rewrite_via_temporary_file(history_path, rewrite)?;
+        self.history_file_id = file_id;
 
-        // Make our temporary file
-        let Some((mut tmp_file, tmp_name)) = create_temporary_file(&tmp_name_template) else {
-            return;
-        };
-        let mut done = false;
-        for _i in 0..MAX_SAVE_TRIES {
-            if done {
-                break;
-            }
+        // We've saved everything, so we have no more unsaved items.
+        self.first_unwritten_new_item_index = self.new_items.len();
 
-            let target_file_before = wopen_cloexec(
-                &target_name,
-                OFlag::O_RDONLY | OFlag::O_CREAT,
-                HISTORY_FILE_MODE,
-            );
-            if let Err(err) = target_file_before {
-                FLOG!(history_file, "Error opening history file:", err);
-            }
+        // We deleted our deleted items.
+        self.deleted_items.clear();
 
-            let orig_file_id = target_file_before
-                .as_ref()
-                .map(file_id_for_file)
-                .unwrap_or(INVALID_FILE_ID);
+        // Our history has been written to the file, so clear our state so we can re-reference the
+        // file.
+        self.clear_file_state();
 
-            // Open any target file, but do not lock it right away
-            if !self.rewrite_to_temporary_file(target_file_before.ok().as_mut(), &mut tmp_file) {
-                // Failed to write, no good
-                break;
-            }
-
-            // The crux! We rewrote the history file; see if the history file changed while we
-            // were rewriting it. Make an effort to take the lock before checking, to avoid racing.
-            // If the open fails, then proceed; this may be because there is no current history
-            let mut new_file_id = INVALID_FILE_ID;
-
-            let mut target_file_after = wopen_cloexec(&target_name, OFlag::O_RDONLY, Mode::empty());
-            if let Ok(target_file_after) = target_file_after.as_mut() {
-                // critical to take the lock before checking file IDs,
-                // and hold it until after we are done replacing.
-                // Also critical to check the file at the path, NOT based on our fd.
-                // It's only OK to replace the file while holding the lock.
-                // Note any lock is released when target_file_after is closed.
-                unsafe {
-                    Self::maybe_lock_file(target_file_after, LOCK_EX);
-                }
-                new_file_id = match file_id_for_path_or_error(&target_name) {
-                    Ok(file_id) => file_id,
-                    Err(err) => {
-                        if err.kind() != std::io::ErrorKind::NotFound {
-                            FLOG!(history_file, "Error re-opening history file:", err);
-                        }
-                        INVALID_FILE_ID
-                    }
-                }
-            }
-
-            let can_replace_file = new_file_id == orig_file_id || new_file_id == INVALID_FILE_ID;
-            if !can_replace_file {
-                // The file has changed, so we're going to re-read it
-                // Truncate our tmp_file so we can reuse it
-                if let Err(err) = tmp_file.set_len(0) {
-                    FLOG!(
-                        history_file,
-                        "Error when truncating temporary history file:",
-                        err
-                    );
-                }
-                if let Err(err) = tmp_file.seek(SeekFrom::Start(0)) {
-                    FLOG!(
-                        history_file,
-                        "Error resetting cursor in temporary history file:",
-                        err
-                    );
-                }
-            } else {
-                // The file is unchanged, or the new file doesn't exist or we can't read it
-                // We also attempted to take the lock, so we feel confident in replacing it
-
-                // Ensure we maintain the ownership and permissions of the original (#2355). If the
-                // stat fails, we assume (hope) our default permissions are correct. This
-                // corresponds to e.g. someone running sudo -E as the very first command. If they
-                // did, it would be tricky to set the permissions correctly. (bash doesn't get this
-                // case right either).
-                if let Ok(target_file_after) = target_file_after.as_ref() {
-                    if let Ok(md) = target_file_after.metadata() {
-                        // TODO(MSRV): Consider replacing with std::os::unix::fs::fchown when MSRV >= 1.73
-                        if unsafe { fchown(tmp_file.as_raw_fd(), md.uid(), md.gid()) } == -1 {
-                            FLOG!(
-                                history_file,
-                                "Error when changing ownership of history file:",
-                                errno::errno()
-                            );
-                        }
-                        if let Err(e) = tmp_file.set_permissions(md.permissions()) {
-                            FLOG!(history_file, "Error when changing mode of history file:", e);
-                        }
-                    }
-                }
-
-                // Slide it into place
-                if wrename(&tmp_name, &target_name) == -1 {
-                    FLOG!(
-                        error,
-                        wgettext_fmt!(
-                            "Error when renaming history file: %s",
-                            errno::errno().to_string()
-                        )
-                    );
-                }
-
-                // We did it
-                done = true;
-            }
-        }
-
-        // Ensure we never leave the old file around
-        let _ = wunlink(&tmp_name);
-
-        if done {
-            // We've saved everything, so we have no more unsaved items.
-            self.first_unwritten_new_item_index = self.new_items.len();
-
-            // We deleted our deleted items.
-            self.deleted_items.clear();
-
-            // Our history has been written to the file, so clear our state so we can re-reference the
-            // file.
-            self.clear_file_state();
-        }
+        Ok(())
     }
 
     /// Saves history by appending to the file.
-    fn save_internal_via_appending(&mut self) -> std::io::Result<()> {
+    fn save_internal_via_appending(&mut self, history_path: &wstr) -> std::io::Result<()> {
         FLOGF!(
             history,
             "Saving %lu items via appending",
@@ -824,65 +665,20 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> {
         // No deleting allowed.
         assert!(self.deleted_items.is_empty());
 
-        // If the file is different (someone vacuumed it) then we need to update our mmap.
-        let mut file_changed = false;
+        let mut locked_history_file =
+            LockedFile::new(LockingMode::Exclusive(WriteMethod::Append), history_path)?;
 
-        // Get the path to the real history file.
-        let Some(history_path) = history_filename(&self.name, L!("")) else {
-            // No history should be saved if fish_history is set to the empty string,
-            // so nothing needs to be done here.
-            return Ok(());
-        };
+        // Check if the file was modified since it was last read.
+        let file_id = file_id_for_file(locked_history_file.get());
+        let file_changed = file_id != self.history_file_id;
 
-        // We are going to open the file, lock it, append to it, and then close it
-        // After locking it, we need to stat the file at the path; if there is a new file there, it
-        // means the file was replaced and we have to try again.
-        // Limit our max tries so we don't do this forever.
-        let mut num_attempts = 0;
-        let mut history_file = loop {
-            if num_attempts == MAX_SAVE_TRIES {
-                return Err(std::io::Error::new(
-                    std::io::ErrorKind::Other,
-                    "Number of unsuccessful attempts to open history file exceeds maximum.",
-                ));
-            }
-            // Return immediately if an error occurs here.
-            let mut file = wopen_cloexec(
-                &history_path,
-                OFlag::O_WRONLY | OFlag::O_APPEND,
-                Mode::empty(),
-            )?;
-
-            // Exclusive lock on the entire file. This is released when we close the file (below). This
-            // may fail on (e.g.) lockless NFS. If so, proceed as if it did not fail; the risk is that
-            // we may get interleaved history items, which is considered better than no history, or
-            // forcing everything through the slow copy-move mode. We try to minimize this possibility
-            // by writing with O_APPEND.
-            unsafe {
-                Self::maybe_lock_file(&mut file, LOCK_EX);
-            }
-            let file_id = file_id_for_file(&file);
-            if file_id_for_path(&history_path) == file_id {
-                // File IDs match, so the file we opened is still at that path
-                // We're going to use this fd
-                if file_id != self.history_file_id {
-                    file_changed = true;
-                }
-                break file;
-            }
-            num_attempts += 1;
-        };
-
-        // We (hopefully successfully) took the exclusive lock. Append to the file.
+        // We took the exclusive lock. Append to the file.
         // Note that this is sketchy for a few reasons:
         //   - Another shell may have appended its own items with a later timestamp, so our file may
         // no longer be sorted by timestamp.
         //   - Another shell may have appended the same items, so our file may now contain
         // duplicates.
         //
-        // We cannot modify any previous parts of our file, because other instances may be reading
-        // those portions. We can only append.
-        //
         // Originally we always rewrote the file on saving, which avoided both of these problems.
         // However, appending allows us to save history after every command, which is nice!
         //
@@ -906,7 +702,8 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> {
                 // the file system.
                 // Flushing and syncing each iteration adds overhead,
                 // but hopefully there are not that many items to write when appending.
-                res = drain_buffer_into_file_and_flush(&mut buffer, &mut history_file);
+                res = drain_buffer_into_file_and_flush(&mut buffer, locked_history_file.get_mut());
+
                 if res.is_err() {
                     break;
                 }
@@ -918,11 +715,11 @@ fn save_internal_via_appending(&mut self) -> std::io::Result<()> {
         // Since we just modified the file, update our history_file_id to match its current state
         // Otherwise we'll think the file has been changed by someone else the next time we go to
         // write.
-        // We don't update the mapping since we only appended to the file, and everything we
+        // We don't update `self.file_contents` since we only appended to the file, and everything we
         // appended remains in our new_items
-        self.history_file_id = file_id_for_file(&history_file);
+        self.history_file_id = file_id;
 
-        drop(history_file);
+        drop(locked_history_file);
 
         // If someone has replaced the file, forget our file state.
         if file_changed {
@@ -941,30 +738,41 @@ fn save(&mut self, vacuum: bool) {
             return;
         }
 
-        if history_filename(&self.name, L!("")).is_none() {
+        // Compact our new items so we don't have duplicates.
+        self.compact_new_items();
+
+        if self.name.is_empty() {
             // We're in the "incognito" mode. Pretend we've saved the history.
             self.first_unwritten_new_item_index = self.new_items.len();
             self.deleted_items.clear();
             self.clear_file_state();
+            return;
         }
 
-        // Compact our new items so we don't have duplicates.
-        self.compact_new_items();
+        let history_path = match self.history_file_path() {
+            Ok(history_path) => history_path.unwrap(),
+            Err(e) => {
+                FLOG!(history, "Saving history failed:", e);
+                return;
+            }
+        };
 
         // Try saving. If we have items to delete, we have to rewrite the file. If we do not, we can
         // append to it.
         let mut ok = false;
         if !vacuum && self.deleted_items.is_empty() {
             // Try doing a fast append.
-            if let Err(e) = self.save_internal_via_appending() {
-                FLOG!(history, "Appending failed", e);
+            if let Err(e) = self.save_internal_via_appending(&history_path) {
+                FLOG!(history, "Appending to history failed:", e);
             } else {
                 ok = true;
             }
         }
         if !ok {
             // We did not or could not append; rewrite the file ("vacuum" it).
-            self.save_internal_via_rewrite();
+            if let Err(e) = self.save_internal_via_rewrite(&history_path) {
+                FLOG!(history, "Rewriting history failed:", e)
+            }
         }
     }
 
@@ -975,7 +783,7 @@ fn save_unless_disabled(&mut self) {
             return;
         }
 
-        // We may or may not vacuum. We try to vacuum every kVacuumFrequency items, but start the
+        // We may or may not vacuum. We try to vacuum every `VACUUM_FREQUENCY` items, but start the
         // countdown at a random number so that even if the user never runs more than 25 commands, we'll
         // eventually vacuum.  If countdown_to_vacuum is None, it means we haven't yet picked a value for
         // the counter.
@@ -1040,7 +848,8 @@ fn is_empty(&mut self) -> bool {
         } else {
             // If we have not loaded old items, don't actually load them (which may be expensive); just
             // stat the file and see if it exists and is nonempty.
-            let Some(where_) = history_filename(&self.name, L!("")) else {
+
+            let Ok(Some(where_)) = self.history_file_path() else {
                 return true;
             };
 
@@ -1096,7 +905,7 @@ fn clear(&mut self) {
         self.deleted_items.clear();
         self.first_unwritten_new_item_index = 0;
         self.old_item_offsets.clear();
-        if let Some(filename) = history_filename(&self.name, L!("")) {
+        if let Ok(Some(filename)) = self.history_file_path() {
             wunlink(&filename);
         }
         self.clear_file_state();
@@ -1117,7 +926,7 @@ fn clear_session(&mut self) {
     /// file to the new history file.
     /// The new contents will automatically be re-mapped later.
     fn populate_from_config_path(&mut self) {
-        let Some(new_file) = history_filename(&self.name, L!("")) else {
+        let Ok(Some(new_file)) = self.history_file_path() else {
             return;
         };
 
@@ -1140,7 +949,7 @@ fn populate_from_config_path(&mut self) {
         let mut dst_file = match wopen_cloexec(
             &new_file,
             OFlag::O_WRONLY | OFlag::O_CREAT,
-            HISTORY_FILE_MODE,
+            LOCKED_FILE_MODE,
         ) {
             Ok(file) => file,
             Err(err) => {
@@ -1337,71 +1146,6 @@ fn size(&mut self) -> usize {
         let old_item_count = self.old_item_offsets.len();
         return new_item_count + old_item_count;
     }
-
-    /// Maybe lock a history file.
-    /// Returns `true` if successful, `false` if locking was skipped.
-    ///
-    /// # Safety
-    ///
-    /// `fd` and `lock_type` must be valid arguments to `flock(2)`.
-    unsafe fn maybe_lock_file(file: &mut File, lock_type: libc::c_int) -> bool {
-        assert!(lock_type & LOCK_UN == 0, "Do not use lock_file to unlock");
-
-        // Don't lock if it took too long before, if we are simulating a failing lock, or if our history
-        // is on a remote filesystem.
-        if ABANDONED_LOCKING.load() {
-            return false;
-        }
-        if CHAOS_MODE.load() {
-            return false;
-        }
-        if path_get_data_remoteness() == DirRemoteness::remote {
-            return false;
-        }
-
-        let (ok, start_time) = loop {
-            let start_time = SystemTime::now();
-            if unsafe { flock(file.as_raw_fd(), lock_type) } != -1 {
-                break (true, start_time);
-            }
-            if errno::errno().0 != EINTR {
-                break (false, start_time);
-            }
-        };
-        if let Ok(duration) = start_time.elapsed() {
-            if duration > Duration::from_millis(250) {
-                FLOG!(
-                    warning,
-                    wgettext_fmt!(
-                        "Locking the history file took too long (%.3f seconds).",
-                        duration.as_secs_f64()
-                    )
-                );
-                ABANDONED_LOCKING.store(true);
-            }
-        }
-        ok
-    }
-
-    /// Unlock a history file.
-    ///
-    /// # Safety
-    ///
-    /// `fd` must be a valid argument to `flock(2)` with `LOCK_UN`.
-    unsafe fn unlock_file(file: &mut File) {
-        libc::flock(file.as_raw_fd(), LOCK_UN);
-    }
-}
-
-// Returns the fd of an opened temporary file, or None on failure.
-fn create_temporary_file(name_template: &wstr) -> Option<(File, WString)> {
-    for _attempt in 0..10 {
-        let narrow_str = wcs2zstring(name_template);
-        if let Ok((fd, narrow_str)) = fish_mkstemp_cloexec(narrow_str) {
-            return Some((fd, str2wcstring(narrow_str.to_bytes())));
-        }
-    }
-    None
 }
 
 fn string_could_be_path(potential_path: &wstr) -> bool {
@@ -1524,7 +1268,7 @@ fn imp(&self) -> MutexGuard<HistoryImpl> {
     /// Privately add an item. If pending, the item will not be returned by history searches until a
     /// call to resolve_pending. Any trailing ephemeral items are dropped.
     /// Exposed for testing.
-    pub fn add(&self, item: HistoryItem, pending: bool /*=false*/) {
+    pub fn add(&self, item: HistoryItem, pending: bool) {
         self.imp().add(item, pending, true)
     }
 
@@ -2087,7 +1831,3 @@ pub fn in_private_mode(vars: &dyn Environment) -> bool {
 
 /// Whether to force the read path instead of mmap. This is useful for testing.
 static NEVER_MMAP: RelaxedAtomicBool = RelaxedAtomicBool::new(false);
-
-/// Whether we're in maximum chaos mode, useful for testing.
-/// This causes things like locks to fail.
-pub static CHAOS_MODE: RelaxedAtomicBool = RelaxedAtomicBool::new(false);

src/history/file.rs

@@ -5,7 +5,7 @@
     fs::File,
     io::{Read, Seek, SeekFrom, Write},
     ops::{Deref, DerefMut},
-    os::fd::{AsRawFd, RawFd},
+    os::fd::AsRawFd,
     time::{Duration, SystemTime, UNIX_EPOCH},
 };
 
@@ -15,7 +15,7 @@
 use crate::{
     common::{str2wcstring, subslice_position, wcs2string},
     flog::FLOG,
-    path::{path_get_config_remoteness, DirRemoteness},
+    path::{path_get_data_remoteness, DirRemoteness},
 };
 
 /// History file types.
@@ -43,11 +43,19 @@ unsafe fn new(ptr: *mut u8, len: usize) -> Self {
         Self { ptr, len }
     }
 
-    /// Map a region `[0, len)` from an `fd`.
-    /// Returns [`None`] on failure.
-    pub fn map_file(fd: RawFd, len: usize) -> std::io::Result<Self> {
-        assert!(len != 0);
-        let ptr = unsafe { mmap(std::ptr::null_mut(), len, PROT_READ, MAP_PRIVATE, fd, 0) };
+    /// Map a region `[0, len)` from a locked file.
+    pub fn map_file(file: &File, len: usize) -> std::io::Result<Self> {
+        let ptr = unsafe {
+            mmap(
+                std::ptr::null_mut(),
+                len,
+                PROT_READ,
+                MAP_PRIVATE,
+                file.as_raw_fd(),
+                0,
+            )
+        };
+
         if ptr == MAP_FAILED {
             return Err(std::io::Error::last_os_error());
         }
@@ -57,12 +65,7 @@ pub fn map_file(fd: RawFd, len: usize) -> std::io::Result<Self> {
     }
 
     /// Map anonymous memory of a given length.
-    /// Returns [`None`] on failure.
-    pub fn map_anon(len: usize) -> Option<Self> {
-        if len == 0 {
-            return None;
-        }
-
+    pub fn map_anon(len: usize) -> std::io::Result<Self> {
         let ptr = unsafe {
             mmap(
                 std::ptr::null_mut(),
@@ -74,11 +77,11 @@ pub fn map_anon(len: usize) -> Option<Self> {
             )
         };
         if ptr == MAP_FAILED {
-            return None;
+            return Err(std::io::Error::last_os_error());
         }
 
         // SAFETY: mmap of `len` was successful and returned `ptr`
-        Some(unsafe { Self::new(ptr.cast(), len) })
+        Ok(unsafe { Self::new(ptr.cast(), len) })
     }
 }
 
@@ -113,39 +116,44 @@ pub struct HistoryFileContents {
 }
 
 impl HistoryFileContents {
-    /// Construct a history file contents from a File reference.
-    pub fn create(file: &mut File) -> Option<Self> {
+    /// Construct a history file contents from a [`File`] reference.
+    pub fn create(mut history_file: &File) -> std::io::Result<Self> {
         // Check that the file is seekable, and its size.
-        let len: usize = file.seek(SeekFrom::End(0)).ok()?.try_into().ok()?;
-        if len == 0 {
-            return None;
-        }
-        let map_anon = |file: &mut File, len: usize| {
+        let len: usize = match history_file.seek(SeekFrom::End(0))?.try_into() {
+            Ok(len) => len,
+            Err(err) => {
+                return Err(std::io::Error::new(
+                    std::io::ErrorKind::Unsupported,
+                    format!("Cannot convert u64 to usize: {err}"),
+                ))
+            }
+        };
+        let map_anon = |mut file: &File, len: usize| -> std::io::Result<MmapRegion> {
             let mut region = MmapRegion::map_anon(len)?;
             // If we mapped anonymous memory, we have to read from the file.
-            file.seek(SeekFrom::Start(0)).ok()?;
-            read_zero_padded(&mut *file, region.as_mut()).ok()?;
-            Some(region)
+            file.seek(SeekFrom::Start(0))?;
+            file.read_exact(&mut region)?;
+            Ok(region)
         };
         let region = if should_mmap() {
-            match MmapRegion::map_file(file.as_raw_fd(), len) {
+            match MmapRegion::map_file(history_file, len) {
                 Ok(region) => region,
-                Err(err) if err.raw_os_error() == Some(ENODEV) => {
-                    // Our mmap failed with ENODEV, which means the underlying
-                    // filesystem does not support mapping. Treat this as a hint
-                    // that the filesystem is remote, and so disable locks for
-                    // the history file.
-                    super::ABANDONED_LOCKING.store(true);
-                    // Create an anonymous mapping and read() the file into it.
-                    map_anon(file, len)?
+                Err(err) => {
+                    if err.raw_os_error() == Some(ENODEV) {
+                        // Our mmap failed with ENODEV, which means the underlying
+                        // filesystem does not support mapping.
+                        // Create an anonymous mapping and read() the file into it.
+                        map_anon(history_file, len)?
+                    } else {
+                        return Err(err);
+                    }
                 }
-                Err(_err) => return None,
             }
         } else {
-            map_anon(file, len)?
+            map_anon(history_file, len)?
         };
 
-        region.try_into().ok()
+        region.try_into()
     }
 
     /// Decode an item at a given offset.
@@ -184,13 +192,17 @@ fn infer_file_type(contents: &[u8]) -> HistoryFileType {
 }
 
 impl TryFrom<MmapRegion> for HistoryFileContents {
-    type Error = ();
+    type Error = std::io::Error;
 
-    fn try_from(region: MmapRegion) -> Result<Self, Self::Error> {
+    fn try_from(region: MmapRegion) -> std::io::Result<Self> {
         let type_ = infer_file_type(&region);
         if type_ == HistoryFileType::Fish1_x {
-            FLOG!(error, "unsupported history file format 1.x");
-            return Err(());
+            let error_message = "unsupported history file format 1.x";
+            FLOG!(error, error_message);
+            return Err(std::io::Error::new(
+                std::io::ErrorKind::InvalidInput,
+                error_message,
+            ));
         }
         Ok(Self { region })
     }
@@ -220,7 +232,7 @@ pub fn append_history_item_to_buffer(item: &HistoryItem, buffer: &mut Vec<u8>) {
     }
 }
 
-/// Check if we should mmap the fd.
+/// Check if we should mmap the file.
 /// Don't try mmap() on non-local filesystems.
 fn should_mmap() -> bool {
     if super::NEVER_MMAP.load() {
@@ -228,23 +240,7 @@ fn should_mmap() -> bool {
     }
 
     // mmap only if we are known not-remote.
-    path_get_config_remoteness() != DirRemoteness::remote
-}
-
-/// Read from `fd` to fill `dest`, zeroing any unused space.
-fn read_zero_padded(file: &mut File, mut dest: &mut [u8]) -> std::io::Result<()> {
-    while !dest.is_empty() {
-        match file.read(dest) {
-            Ok(0) => break,
-            Ok(amt) => {
-                dest = &mut dest[amt..];
-            }
-            Err(e) if e.kind() == std::io::ErrorKind::Interrupted => continue,
-            Err(err) => return Err(err),
-        }
-    }
-    dest.fill(0u8);
-    Ok(())
+    path_get_data_remoteness() != DirRemoteness::remote
 }
 
 fn replace_all(s: &mut Vec<u8>, needle: &[u8], replacement: &[u8]) {

src/lib.rs

@@ -48,6 +48,7 @@
 pub mod fds;
 pub mod flog;
 pub mod fork_exec;
+pub mod fs;
 pub mod function;
 pub mod future;
 pub mod future_feature_flags;

src/path.rs

@@ -667,7 +667,7 @@ fn create_dir_all_with_mode<P: AsRef<std::path::Path>>(path: P, mode: u32) -> st
 }
 
 /// Return whether the given path is on a remote filesystem.
-fn path_remoteness(path: &wstr) -> DirRemoteness {
+pub fn path_remoteness(path: &wstr) -> DirRemoteness {
     let narrow = wcs2zstring(path);
     #[cfg(target_os = "linux")]
     {

src/tests/env_universal_common.rs

@@ -3,7 +3,7 @@
 use crate::common::ScopeGuard;
 use crate::common::ENCODE_DIRECT_BASE;
 use crate::env::{EnvVar, EnvVarFlags, VarTable};
-use crate::env_universal_common::{CallbackDataList, EnvUniversal, UvarFormat};
+use crate::env_universal_common::{EnvUniversal, UvarFormat};
 use crate::reader::{reader_pop, reader_push, ReaderConfig};
 use crate::tests::prelude::*;
 use crate::threads::{iothread_drain_all, iothread_perform};
@@ -15,15 +15,14 @@
 
 fn test_universal_helper(x: usize) {
     let _cleanup = test_init();
-    let mut callbacks = CallbackDataList::new();
     let mut uvars = EnvUniversal::new();
-    uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
+    uvars.initialize_at_path(UVARS_TEST_PATH.to_owned());
 
     for j in 0..UVARS_PER_THREAD {
         let key = sprintf!("key_%d_%d", x, j);
         let val = sprintf!("val_%d_%d", x, j);
         uvars.set(&key, EnvVar::new(val, EnvVarFlags::empty()));
-        let synced = uvars.sync(&mut callbacks);
+        let (synced, _) = uvars.sync();
         assert!(
             synced,
             "Failed to sync universal variables after modification"
@@ -32,7 +31,7 @@ fn test_universal_helper(x: usize) {
 
     // Last step is to delete the first key.
     uvars.remove(&sprintf!("key_%d_%d", x, 0));
-    let synced = uvars.sync(&mut callbacks);
+    let (synced, _) = uvars.sync();
     assert!(synced, "Failed to sync universal variables after deletion");
 }
 
@@ -54,8 +53,7 @@ fn test_universal() {
     iothread_drain_all(&mut reader);
 
     let mut uvars = EnvUniversal::new();
-    let mut callbacks = CallbackDataList::new();
-    uvars.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
+    uvars.initialize_at_path(UVARS_TEST_PATH.to_owned());
 
     for i in 0..threads {
         for j in 0..UVARS_PER_THREAD {
@@ -219,11 +217,25 @@ fn test_universal_parsing_legacy() {
 fn test_universal_callbacks() {
     let _cleanup = test_init();
     std::fs::create_dir_all("test/fish_uvars_test/").unwrap();
-    let mut callbacks = CallbackDataList::new();
     let mut uvars1 = EnvUniversal::new();
     let mut uvars2 = EnvUniversal::new();
-    uvars1.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
-    uvars2.initialize_at_path(&mut callbacks, UVARS_TEST_PATH.to_owned());
+    let mut callbacks = uvars1
+        .initialize_at_path(UVARS_TEST_PATH.to_owned())
+        .unwrap_or_default();
+    callbacks.append(
+        &mut uvars2
+            .initialize_at_path(UVARS_TEST_PATH.to_owned())
+            .unwrap_or_default(),
+    );
+
+    macro_rules! sync {
+        ($uvars:expr) => {
+            let (_, cb_opt) = $uvars.sync();
+            if let Some(mut cb) = cb_opt {
+                callbacks.append(&mut cb);
+            }
+        };
+    }
 
     let noflags = EnvVarFlags::empty();
 
@@ -236,8 +248,8 @@ fn test_universal_callbacks() {
     uvars1.set(L!("kappa"), EnvVar::new(L!("1").to_owned(), noflags)); //
     uvars1.set(L!("omicron"), EnvVar::new(L!("1").to_owned(), noflags)); //
 
-    uvars1.sync(&mut callbacks);
-    uvars2.sync(&mut callbacks);
+    sync!(uvars1);
+    sync!(uvars2);
 
     // Change uvars1.
     uvars1.set(L!("alpha"), EnvVar::new(L!("2").to_owned(), noflags)); // changes value
@@ -247,7 +259,7 @@ fn test_universal_callbacks() {
     ); // changes export
     uvars1.remove(L!("delta")); // erases value
     uvars1.set(L!("epsilon"), EnvVar::new(L!("1").to_owned(), noflags)); // changes nothing
-    uvars1.sync(&mut callbacks);
+    sync!(uvars1);
 
     // Change uvars2. It should treat its value as correct and ignore changes from uvars1.
     uvars2.set(L!("lambda"), EnvVar::new(L!("1").to_owned(), noflags)); // same value
@@ -255,7 +267,7 @@ fn test_universal_callbacks() {
 
     // Now see what uvars2 sees.
     callbacks.clear();
-    uvars2.sync(&mut callbacks);
+    sync!(uvars2);
 
     // Sort them to get them in a predictable order.
     callbacks.sort_by(|a, b| a.key.cmp(&b.key));
@@ -308,12 +320,12 @@ fn test_universal_ok_to_save() {
         "UVARS_TEST_PATH should be readable"
     );
 
-    let mut cbs = CallbackDataList::new();
     let mut uvars = EnvUniversal::new();
-    uvars.initialize_at_path(&mut cbs, UVARS_TEST_PATH.to_owned());
+    uvars
+        .initialize_at_path(UVARS_TEST_PATH.to_owned())
+        .unwrap_or_default();
     assert!(!uvars.is_ok_to_save(), "Should not be OK to save");
-    uvars.sync(&mut cbs);
-    cbs.clear();
+    uvars.sync();
     assert!(!uvars.is_ok_to_save(), "Should still not be OK to save");
     uvars.set(
         L!("SOMEVAR"),

src/tests/history.rs

@@ -272,8 +272,6 @@ fn test_history_races() {
     // Ensure history is clear.
     History::new(L!("race_test")).clear();
 
-    // history::CHAOS_MODE.store(true);
-
     let mut children = Vec::with_capacity(RACE_COUNT);
     for i in 0..RACE_COUNT {
         children.push(std::thread::spawn(move || {
@@ -295,7 +293,6 @@ fn test_history_races() {
 
     // Ensure that we got sane, sorted results.
     let hist = History::new(L!("race_test"));
-    history::CHAOS_MODE.store(false);
 
     // History is enumerated from most recent to least
     // Every item should be the last item in some array

src/wutil/tests.rs

@@ -1,11 +1,10 @@
-use crate::fds::AutoCloseFd;
 use crate::tests::prelude::*;
 use crate::util::get_rng;
+use crate::{fds::AutoCloseFd, fs::create_temporary_file};
 use libc::{c_void, O_CREAT, O_RDWR, O_TRUNC, SEEK_SET};
 use rand::Rng;
-use std::{ffi::CString, ptr};
-
-use crate::fallback::fish_mkstemp_cloexec;
+use std::ffi::CString;
+use std::ptr;
 
 use super::*;
 
@@ -62,8 +61,8 @@ fn test_wdirname_wbasename() {
 #[serial]
 fn test_wwrite_to_fd() {
     let _cleanup = test_init();
-    let (_fd, filename) =
-        fish_mkstemp_cloexec(CString::new("/tmp/fish_test_wwrite.XXXXXX").unwrap()).unwrap();
+    let (_file, filename) = create_temporary_file(L!("/tmp/fish_test_wwrite.XXXXXX")).unwrap();
+    let filename = CString::new(filename.to_string()).unwrap();
     let mut rng = get_rng();
     let sizes = [1, 2, 3, 5, 13, 23, 64, 128, 255, 4096, 4096 * 2];
     for &size in &sizes {