tests: kill a flaky test that reintroduced the race it was testing for

The previous commit went out with a failing test. It passed on a rerun,
which is worse than failing — a flaky test in a security product either
gets ignored or gets deleted, and both are how a real regression ships.

The offender was rootkit's own thread test. It read /proc/self/task and
took the /proc snapshot at different moments, so a thread started by
another test between the two reads looked like a thread that answered
kill() but was missing from the listing. That is precisely the
start/exit race the hidden-process check exists to avoid, reintroduced
in the test written to prove the check avoids it.

Each thread now reports its own tid via gettid and then parks, so all
eight are demonstrably alive across the whole measurement window. Five
consecutive full runs, 124/124 each.

Also isolated persistence's read-only-scan test behind the env lock:
baseline_path() reads XDG_DATA_HOME and the quarantine tests reassign
it, so two scans either side of that disagreed about first_run. It now
takes the lock, points at its own directory, and additionally asserts
the thing the test was named for — that a read-only scan writes no
baseline file, and that --accept does.

256 tests pass across the workspace.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

crates/houndd/src/persistence.rs

@@ -475,12 +475,39 @@ mod tests {
         // Read-only mode must not have side effects — someone running a
         // check should not silently accept whatever is currently installed
         // as normal.
+        //
+        // The env lock is load-bearing, not decoration. baseline_path()
+        // reads XDG_DATA_HOME, and the quarantine tests reassign it; two
+        // scans either side of that reassignment look at different files
+        // and disagree about first_run. That made this test flaky, which
+        // in a security product is worse than a test that simply fails.
+        let data = std::env::temp_dir().join(format!("hound-pscan-{}", std::process::id()));
+        let _ = std::fs::remove_dir_all(&data);
+        std::fs::create_dir_all(&data).unwrap();
+        let _env_guard = crate::test_util::locked();
+        std::env::set_var("XDG_DATA_HOME", &data);
+
         let scan_a = scan(false);
         assert!(scan_a.total > 0);
+        assert!(scan_a.first_run, "an isolated data dir has no baseline yet");
+        assert!(
+            !baseline_path().exists(),
+            "a read-only scan must not write a baseline"
+        );
+
         let scan_b = scan(false);
         assert_eq!(
             scan_a.first_run, scan_b.first_run,
             "a read-only scan must not change what the next one sees"
         );
+
+        // And an explicit accept does write one.
+        let scan_c = scan(true);
+        assert!(scan_c.first_run);
+        assert!(baseline_path().exists(), "--accept must record the baseline");
+        assert!(!scan(false).first_run, "and the next scan compares against it");
+
+        std::env::remove_var("XDG_DATA_HOME");
+        let _ = std::fs::remove_dir_all(&data);
     }
 }

crates/houndd/src/rootkit.rs

@@ -477,44 +477,47 @@ mod tests {
         // any thread id. A multi-threaded process therefore has ids that
         // answer kill and are absent from a /proc listing — and reporting
         // those as hidden produced dozens of criticals on a healthy laptop.
+        //
+        // Each thread reports its OWN tid rather than the test reading
+        // /proc/self/task afterwards. Reading the task list and the /proc
+        // snapshot at different moments reintroduces exactly the race this
+        // whole check exists to avoid — the test harness starts and stops
+        // threads for other tests throughout, so one captured in the first
+        // read may be gone by the second. Threads that report themselves
+        // and then wait are alive across the entire window by construction.
         let stop = std::sync::Arc::new(std::sync::atomic::AtomicBool::new(false));
+        let reported: std::sync::Arc<std::sync::Mutex<Vec<u32>>> =
+            std::sync::Arc::new(std::sync::Mutex::new(Vec::new()));
+
         let handles: Vec<_> = (0..8)
             .map(|_| {
                 let s = std::sync::Arc::clone(&stop);
+                let r = std::sync::Arc::clone(&reported);
                 std::thread::spawn(move || {
+                    // SAFETY: gettid takes no arguments and cannot fail.
+                    let tid = unsafe { libc::syscall(libc::SYS_gettid) } as u32;
+                    r.lock().expect("tid list poisoned").push(tid);
                     while !s.load(std::sync::atomic::Ordering::Relaxed) {
                         std::thread::sleep(std::time::Duration::from_millis(5));
                     }
                 })
             })
             .collect();
-        std::thread::sleep(std::time::Duration::from_millis(50));
 
+        // Wait until every thread has reported and is therefore parked.
+        let mut waited = 0;
+        while reported.lock().unwrap().len() < 8 && waited < 200 {
+            std::thread::sleep(std::time::Duration::from_millis(5));
+            waited += 1;
+        }
+        let mine: Vec<u32> = reported.lock().unwrap().clone();
+
+        // Snapshot while all eight are demonstrably alive.
         let tids = proc_tids();
         let pids = proc_pids();
-
-        // Our own threads: real task ids that answer kill() and do NOT
-        // appear in a /proc listing. Every one must be accounted for by
-        // proc_tids, or it becomes a critical finding on a clean machine.
-        //
-        // Everything is measured while the threads are still alive and only
-        // asserted afterwards — checking a thread's existence after joining
-        // it tests nothing except that join() works.
-        let mine: Vec<u32> = std::fs::read_dir(format!("/proc/{}/task", std::process::id()))
-            .unwrap()
-            .flatten()
-            .filter_map(|e| e.file_name().to_str().and_then(|s| s.parse::<u32>().ok()))
-            .collect();
         let observed: Vec<(u32, bool, bool, bool)> = mine
             .iter()
-            .map(|tid| {
-                (
-                    *tid,
-                    pid_exists(*tid),
-                    tids.contains(tid),
-                    pids.contains(tid),
-                )
-            })
+            .map(|tid| (*tid, pid_exists(*tid), tids.contains(tid), pids.contains(tid)))
             .collect();
 
         stop.store(true, std::sync::atomic::Ordering::Relaxed);
@@ -522,25 +525,23 @@ mod tests {
             let _ = h.join();
         }
 
+        assert_eq!(mine.len(), 8, "all eight threads should have reported");
         assert!(
             tids.len() > pids.len(),
             "this process alone has 8 extra threads, so tids must exceed pids"
         );
-        assert!(mine.len() >= 9, "expected the leader plus 8 threads, got {}", mine.len());
         for (tid, exists, in_tids, in_pids) in observed {
-            assert!(exists, "thread {tid} was alive and must answer kill()");
+            assert!(exists, "thread {tid} was parked and must answer kill()");
             assert!(
                 in_tids,
                 "thread {tid} answers kill() but proc_tids missed it — it would be \
                  reported as a hidden process"
             );
-            if tid != std::process::id() {
-                assert!(
-                    !in_pids,
-                    "thread {tid} should not be a top-level /proc entry; that is exactly \
-                     why proc_pids alone is insufficient"
-                );
-            }
+            assert!(
+                !in_pids,
+                "thread {tid} is not a top-level /proc entry; that is exactly why \
+                 proc_pids alone is insufficient"
+            );
         }
     }