Attempt to fix the sporadic uvar test failures on Linux

We identify when the universal variable file has changed out from under us by
comparing a bunch of fields from its stat: inode, device, size, high-precision
timestamp, generation. Linux aggressively reuses inodes, and the size may be
the same by coincidence (which is the case in the tests). Also, Linux
officially has nanosecond precision, but in practice it seems to only uses
millisecond precision for storing mtimes. Thus if there are three or more
updates within a millisecond, every field we check may be the same, and we are
vulnerable to the ABA problem. I believe this explains the occasional test
failures.

The solution is to manually set the nanosecond field of the mtime timestamp to
something unlikely to be duplicated, like a random number, or better yet, the
current time (with nanosecond precision). This is more in the spirit of the
timestamp, and it means we're around a million times less likely to collide.
This seems to fix the tests.

configure.ac

@@ -525,6 +525,7 @@ AC_CHECK_FUNCS( wcsdup wcsndup wcslen wcscasecmp wcsncasecmp fwprintf )
 AC_CHECK_FUNCS( futimes wcwidth wcswidth wcstok fputwc fgetwc )
 AC_CHECK_FUNCS( wcstol wcslcat wcslcpy lrand48_r killpg mkostemp )
 AC_CHECK_FUNCS( backtrace backtrace_symbols sysconf getifaddrs )
+AC_CHECK_FUNCS( futimens clock_gettime )
 
 if test x$local_gettext != xno; then
   AC_CHECK_FUNCS( gettext dcgettext )

src/env_universal_common.cpp

@@ -808,6 +808,7 @@ bool env_universal_t::sync(callback_data_list_t *callbacks)
     if (modified.empty())
     {
         this->load_from_path(vars_path, callbacks);
+        UNIVERSAL_LOG("No modifications");
         return false;
     }
     
@@ -823,6 +824,7 @@ bool env_universal_t::sync(callback_data_list_t *callbacks)
     if (success)
     {
         success = this->open_and_acquire_lock(vars_path, &vars_fd);
+        if (! success) UNIVERSAL_LOG("open_and_acquire_lock() failed");
     }
 
     /* Read from it */
@@ -831,11 +833,13 @@ bool env_universal_t::sync(callback_data_list_t *callbacks)
         assert(vars_fd >= 0);
         this->load_from_fd(vars_fd, callbacks);
     }
+    
 
     /* Open adjacent temporary file */
     if (success)
     {
         success = this->open_temporary_file(directory, &private_file_path, &private_fd);
+        if (! success) UNIVERSAL_LOG("open_temporary_file() failed");
     }
     
     /* Write to it */
@@ -843,6 +847,7 @@ bool env_universal_t::sync(callback_data_list_t *callbacks)
     {
         assert(private_fd >= 0);
         success = this->write_to_fd(private_fd, private_file_path);
+        if (! success) UNIVERSAL_LOG("write_to_fd() failed");
     }
     
     if (success)
@@ -854,9 +859,26 @@ bool env_universal_t::sync(callback_data_list_t *callbacks)
             fchown(private_fd, sbuf.st_uid, sbuf.st_gid);
             fchmod(private_fd, sbuf.st_mode);
         }
-
+        
+        /* 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!)
+         
+           It's probably worth finding a simpler solution to this. The tests ran into this, but it's unlikely to affect users.
+         */
+#if HAVE_CLOCK_GETTIME && HAVE_FUTIMENS
+        struct timespec times[2] = {};
+        times[0].tv_nsec = UTIME_OMIT; // don't change ctime
+        if (0 == clock_gettime(CLOCK_REALTIME, &times[1]))
+        {
+            futimens(private_fd, times);
+        }
+#endif
+    
         /* Apply new file */
         success = this->move_new_vars_file_into_place(private_file_path, vars_path);
+        if (! success) UNIVERSAL_LOG("move_new_vars_file_into_place() failed");
     }
     
     if (success)

src/env_universal_common.h

@@ -168,7 +168,7 @@ public:
 };
 
 bool universal_log_enabled();
-#define UNIVERSAL_LOG(x) if (universal_log_enabled()) fprintf(stderr, "UNIVERSAL LOG: %s\n", x)
+#define UNIVERSAL_LOG(x) do { if (universal_log_enabled()) fprintf(stderr, "UNIVERSAL LOG: %s\n", x); } while (0)
 
 /* Environment variable for requesting a particular universal notifier. See fetch_default_strategy_from_environment for names. */
 #define UNIVERSAL_NOTIFIER_ENV_NAME "fish_universal_notifier"

src/fish_tests.cpp

@@ -2563,7 +2563,7 @@ static int test_universal_helper(int *x)
         bool synced = uvars.sync(NULL);
         if (! synced)
         {
-            err(L"Failed to sync universal variables");
+            err(L"Failed to sync universal variables after modification");
         }
         fputc('.', stderr);
     }
@@ -2573,7 +2573,7 @@ static int test_universal_helper(int *x)
     bool synced = uvars.sync(NULL);
     if (! synced)
     {
-        err(L"Failed to sync universal variables");
+        err(L"Failed to sync universal variables after deletion");
     }
     fputc('.', stderr);
 

src/wutil.cpp

@@ -545,15 +545,20 @@ file_id_t file_id_t::file_id_from_stat(const struct stat *buf)
     result.inode = buf->st_ino;
     result.size = buf->st_size;
     result.change_seconds = buf->st_ctime;
+    result.mod_seconds = buf->st_mtime;
     
 #if STAT_HAVE_NSEC
     result.change_nanoseconds = buf->st_ctime_nsec;
+    result.mod_nanoseconds = buf->st_mtime_nsec;
 #elif defined(__APPLE__)
     result.change_nanoseconds = buf->st_ctimespec.tv_nsec;
+    result.mod_nanoseconds = buf->st_mtimespec.tv_nsec;
 #elif defined(_BSD_SOURCE) || defined(_SVID_SOURCE) || defined(_XOPEN_SOURCE)
     result.change_nanoseconds = buf->st_ctim.tv_nsec;
+    result.mod_nanoseconds = buf->st_mtim.tv_nsec;
 #else
     result.change_nanoseconds = 0;
+    result.mod_nanoseconds = 0;
 #endif
 
 #if defined(__APPLE__) || defined(__DragonFly__) || defined(__FreeBSD__) ||  defined(__OpenBSD__) || defined(__NetBSD__)
@@ -590,12 +595,7 @@ file_id_t file_id_for_path(const wcstring &path)
 
 bool file_id_t::operator==(const file_id_t &rhs) const
 {
-    return device == rhs.device &&
+    return this->compare_file_id(rhs) == 0;
-           inode == rhs.inode &&
-           size == rhs.size &&
-           change_seconds == rhs.change_seconds &&
-           change_nanoseconds == rhs.change_nanoseconds &&
-           generation == rhs.generation;
 }
 
 bool file_id_t::operator!=(const file_id_t &rhs) const
@@ -617,7 +617,7 @@ int compare(T a, T b)
     return 0;
 }
 
-bool file_id_t::operator<(const file_id_t &rhs) const
+int file_id_t::compare_file_id(const file_id_t &rhs) const
 {
     /* Compare each field, stopping when we get to a non-equal field */
     int ret = 0;
@@ -627,5 +627,13 @@ bool file_id_t::operator<(const file_id_t &rhs) const
     if (! ret) ret = compare(generation, rhs.generation);
     if (! ret) ret = compare(change_seconds, rhs.change_seconds);
     if (! ret) ret = compare(change_nanoseconds, rhs.change_nanoseconds);
-    return ret < 0;
+    if (! ret) ret = compare(mod_seconds, rhs.mod_seconds);
+    if (! ret) ret = compare(mod_nanoseconds, rhs.mod_nanoseconds);
+    return ret;
+}
+
+
+bool file_id_t::operator<(const file_id_t &rhs) const
+{
+    return this->compare_file_id(rhs) < 0;
 }

src/wutil.h

@@ -143,6 +143,8 @@ struct file_id_t
     uint64_t size;
     time_t change_seconds;
     long change_nanoseconds;
+    time_t mod_seconds;
+    long mod_nanoseconds;
     uint32_t generation;
     
     bool operator==(const file_id_t &rhs) const;
@@ -152,6 +154,9 @@ struct file_id_t
     bool operator<(const file_id_t &rhs) const;
     
     static file_id_t file_id_from_stat(const struct stat *buf);
+    
+    private:
+    int compare_file_id(const file_id_t &rhs) const;
 };
 
 file_id_t file_id_for_fd(int fd);