watcher/linux: fix and test for issue 73, non-assoc rename events

Will · Will · commit b8b0d8d9638b · 2025-05-26T11:30:48.000-04:00
diff --git a/changelog.md b/changelog.md
@@ -8,6 +8,7 @@
 - **PR#76, jordan-woyak**
   Linux: Fix `epoll_wait` failing with `EINTR`.
   `epoll_wait` can spontaneously fail with `EINTR`.
+- Fixed Issue 73 (Rename events are occasionally split into two events on linux).
 
 ## 0.13.5
 - C++17 compatibility fixes on Linux (avoid captured structured bindings).
diff --git a/devel/include/detail/wtr/watcher/adapter/linux/fanotify/watch.hpp b/devel/include/detail/wtr/watcher/adapter/linux/fanotify/watch.hpp
@@ -149,7 +149,7 @@ pathof(fanotify_event_metadata const* const mtd, int* ec) -> std::string
   /*  Directory name */
   int fd = open_by_handle_at(AT_FDCWD, dir_fh, ofl);
   if (fd <= 0) {
-    *ec = errno;
+    *ec = -errno;
     return {};
   }
   char fs_ev_pidpath[32] = {0};
@@ -188,6 +188,9 @@ struct Parsed {
   unsigned this_len = 0;
 };
 
+// The error code is -errno if there was a system error which prevents
+// correctly parsing the event, or a positive error if the event was
+// malformed or unexpected (without a system error).
 inline auto
 parse_ev(fanotify_event_metadata const* const m, size_t read_len, int* ec)
   -> Parsed
@@ -213,10 +216,11 @@ parse_ev(fanotify_event_metadata const* const m, size_t read_len, int* ec)
     auto e = ev(ev(pathof(m, ec), et, pt), ev(pathof(n, ec), et, pt));
     return {e, nn, here_to_nnn};
   };
-  return ! n                        ? one(m)
+  return et != ev_et::rename        ? one(m)
+       : ! n                        ? (*ec = 2, one(m))
        : isfromto(m->mask, n->mask) ? assoc(m, n)
        : isfromto(n->mask, m->mask) ? assoc(n, m)
-                                    : one(m);
+       : (*ec = 2, one(m));
 }
 
 inline auto is_newdir = [](::wtr::watcher::event const& ev) -> bool
@@ -275,13 +279,13 @@ inline auto do_ev_recv = [](auto const& cb, sysres& sr) -> result
       else {
         int ec = 0;
         auto r = parse_ev(mtd, read_len, &ec);
-        if (ec) return result::w_sys_bad_fd;
+        if (ec < 0) return result::w_sys_bad_fd;
         if (is_newdir(r.ev))
           walkdir_do(r.ev.path_name.c_str(), [&](auto dir) {
             do_mark(dir, sr.ke.fd, cb);
             cb({dir, r.ev.effect_type, r.ev.path_type});
           });
-        else
+        else if (ec == 0)
           cb(r.ev);
         mtd = r.next;
         read_len -= r.this_len;
diff --git a/devel/include/detail/wtr/watcher/adapter/linux/inotify/watch.hpp b/devel/include/detail/wtr/watcher/adapter/linux/inotify/watch.hpp
@@ -187,7 +187,8 @@ struct parsed {
 inline auto parse_ev = [](
                          ke_in_ev::paths const& dm,
                          inotify_event const* const in,
-                         inotify_event const* const tail) -> parsed
+                         inotify_event const* const tail,
+                         int* ec) -> parsed
 {
   using ev = ::wtr::watcher::event;
   using ev_pt = enum ev::path_type;
@@ -212,7 +213,7 @@ inline auto parse_ev = [](
   return ! isassoc(in, next) ? one(in, next)
        : isfromto(in, next)  ? assoc(in, next)
        : isfromto(next, in)  ? assoc(next, in)
-                             : one(in, next);
+                             : (*ec = 1, one(in, next));
 };
 
 struct defer_dm_rm_wd {
@@ -346,13 +347,14 @@ inline auto do_ev_recv = [](auto const& cb, sysres& sr) -> result
       else if (msk & IN_Q_OVERFLOW)
         send_msg(result::w_sys_q_overflow, "", cb);
       else if (is_real_event(msk)) {
-        auto parsed = parse_ev(sr.ke.dm, in_ev, in_ev_tail);
+        int ec = 0;
+        auto parsed = parse_ev(sr.ke.dm, in_ev, in_ev_tail, &ec);
         if (msk & IN_ISDIR && msk & IN_CREATE)
           walkdir_do(parsed.ev.path_name.c_str(), [&](auto dir) {
             do_mark(dir, sr.ke.fd, sr.ke.dm, cb);
             cb({dir, parsed.ev.effect_type, parsed.ev.path_type});
           });
-        else
+        else if (! ec)
           cb(parsed.ev);
         in_ev_next = parsed.next;
       }
diff --git a/devel/include/detail/wtr/watcher/adapter/linux/watch.hpp b/devel/include/detail/wtr/watcher/adapter/linux/watch.hpp
@@ -29,8 +29,6 @@ inline auto watch =
       if (ep_c < 0) {
         if (errno != EINTR) sr.ok = result::e_sys_api_epoll;
       }
-      if (ep_c < 0)
-        sr.ok = result::e_sys_api_epoll;
       else
         for (int n = 0; n < ep_c; ++n)
           if (is_ev_of(n, sr.il.fd))
diff --git a/include/wtr/watcher.hpp b/include/wtr/watcher.hpp
@@ -1135,7 +1135,7 @@ pathof(fanotify_event_metadata const* const mtd, int* ec) -> std::string
   /*  Directory name */
   int fd = open_by_handle_at(AT_FDCWD, dir_fh, ofl);
   if (fd <= 0) {
-    *ec = errno;
+    *ec = -errno;
     return {};
   }
   char fs_ev_pidpath[32] = {0};
@@ -1174,6 +1174,9 @@ struct Parsed {
   unsigned this_len = 0;
 };
 
+// The error code is -errno if there was a system error which prevents
+// correctly parsing the event, or a positive error if the event was
+// malformed or unexpected (without a system error).
 inline auto
 parse_ev(fanotify_event_metadata const* const m, size_t read_len, int* ec)
   -> Parsed
@@ -1199,10 +1202,11 @@ parse_ev(fanotify_event_metadata const* const m, size_t read_len, int* ec)
     auto e = ev(ev(pathof(m, ec), et, pt), ev(pathof(n, ec), et, pt));
     return {e, nn, here_to_nnn};
   };
-  return ! n                        ? one(m)
+  return et != ev_et::rename        ? one(m)
+       : ! n                        ? (*ec = 2, one(m))
        : isfromto(m->mask, n->mask) ? assoc(m, n)
        : isfromto(n->mask, m->mask) ? assoc(n, m)
-                                    : one(m);
+       : (*ec = 2, one(m));
 }
 
 inline auto is_newdir = [](::wtr::watcher::event const& ev) -> bool
@@ -1261,13 +1265,13 @@ inline auto do_ev_recv = [](auto const& cb, sysres& sr) -> result
       else {
         int ec = 0;
         auto r = parse_ev(mtd, read_len, &ec);
-        if (ec) return result::w_sys_bad_fd;
+        if (ec < 0) return result::w_sys_bad_fd;
         if (is_newdir(r.ev))
           walkdir_do(r.ev.path_name.c_str(), [&](auto dir) {
             do_mark(dir, sr.ke.fd, cb);
             cb({dir, r.ev.effect_type, r.ev.path_type});
           });
-        else
+        else if (ec == 0)
           cb(r.ev);
         mtd = r.next;
         read_len -= r.this_len;
@@ -1466,7 +1470,8 @@ struct parsed {
 inline auto parse_ev = [](
                          ke_in_ev::paths const& dm,
                          inotify_event const* const in,
-                         inotify_event const* const tail) -> parsed
+                         inotify_event const* const tail,
+                         int* ec) -> parsed
 {
   using ev = ::wtr::watcher::event;
   using ev_pt = enum ev::path_type;
@@ -1491,7 +1496,7 @@ inline auto parse_ev = [](
   return ! isassoc(in, next) ? one(in, next)
        : isfromto(in, next)  ? assoc(in, next)
        : isfromto(next, in)  ? assoc(next, in)
-                             : one(in, next);
+                             : (*ec = 1, one(in, next));
 };
 
 struct defer_dm_rm_wd {
@@ -1625,13 +1630,14 @@ inline auto do_ev_recv = [](auto const& cb, sysres& sr) -> result
       else if (msk & IN_Q_OVERFLOW)
         send_msg(result::w_sys_q_overflow, "", cb);
       else if (is_real_event(msk)) {
-        auto parsed = parse_ev(sr.ke.dm, in_ev, in_ev_tail);
+        int ec = 0;
+        auto parsed = parse_ev(sr.ke.dm, in_ev, in_ev_tail, &ec);
         if (msk & IN_ISDIR && msk & IN_CREATE)
           walkdir_do(parsed.ev.path_name.c_str(), [&](auto dir) {
             do_mark(dir, sr.ke.fd, sr.ke.dm, cb);
             cb({dir, parsed.ev.effect_type, parsed.ev.path_type});
           });
-        else
+        else if (! ec)
           cb(parsed.ev);
         in_ev_next = parsed.next;
       }
diff --git a/tool/test/rename-events-are-assiciated-events b/tool/test/rename-events-are-assiciated-events
@@ -0,0 +1,57 @@
+#! /usr/bin/env bash
+# shellcheck source=tool/test/.ctx
+
+ec=0
+
+(
+  desc='Rename events are associated events'
+  count=1000
+  read -r -d '' expect << .
+[
+  {
+    "effect_type": "create",
+    "path_name": "s/self/live@d",
+    "path_type": "watcher"
+  },
+  {
+    "effect_type": "create",
+    "path_name": "d/f.0",
+    "path_type": "file"
+  },
+$(for i in $(seq 1 $count); do cat << eol
+  {
+    "associated": {
+      "effect_type": "rename",
+      "path_name": "d/f.$i",
+      "path_type": "file"
+    },
+    "effect_type": "rename",
+    "path_name": "d/f.$((i - 1))",
+    "path_type": "file"
+  },
+eol
+done)
+  {
+    "effect_type": "destroy",
+    "path_name": "s/self/die@d",
+    "path_type": "watcher"
+  }
+]
+.
+  echo -n "$desc ... "
+
+  . "$(dirname "$0")/.ctx"
+
+  actual=$(
+    watch-async "$testdir" -s 3 > "$testdir.json"
+    touch f.0
+    for i in $(seq 1 $count)
+    do mv f.$((i - 1)) f.$i
+    done
+    wait # for the watcher
+    show-events "$testdir" | without-effect-time
+  )
+
+  check-result "$expect" "$actual"
+)
+ec=$((ec + $?))

Original file line number	Diff line number	Diff line change
`@@ -29,8 +29,6 @@ inline auto watch =`
`29`	`29`	`if (ep_c < 0) {`
`30`	`30`	`if (errno != EINTR) sr.ok = result::e_sys_api_epoll;`
`31`	`31`	`}`
`32`		`- if (ep_c < 0)`
`33`		`- sr.ok = result::e_sys_api_epoll;`
`34`	`32`	`else`
`35`	`33`	`for (int n = 0; n < ep_c; ++n)`
`36`	`34`	`if (is_ev_of(n, sr.il.fd))`