From 8911962de3f0bdd86d64c41a08fd084c99e5446d Mon Sep 17 00:00:00 2001
From: "@permadeath.com" <did:plc:nlzmjyfv6loqtxyzvdcznwgf>
Date: Thu, 3 Sep 2026 00:42:55 -0400
Subject: [PATCH] test(hookd): make the state store's write actually fail

Three tests over a write that cannot land: the checked update surfaces it, the
hook's update still hands back its outcome, and the file already on disk
survives whole rather than being truncated.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
---
 crates/didbot-hookd/src/state.rs | 110 +++++++++++++++++++++++++++++++
 1 file changed, 110 insertions(+)

diff --git a/crates/didbot-hookd/src/state.rs b/crates/didbot-hookd/src/state.rs
index 739a7da0..8bc76952 100644
--- a/crates/didbot-hookd/src/state.rs
+++ b/crates/didbot-hookd/src/state.rs
@@ -444,6 +444,116 @@ mod tests {
         );
     }
 
+    /// A path that is a directory: `rename(2)` will not put a file over one,
+    /// so the write fails at the last step, after the temporary file exists.
+    /// The least clever failure available, and the one that also exercises
+    /// the cleanup of the temporary file.
+    fn a_store_whose_write_cannot_land(name: &str) -> (PathBuf, StateStore) {
+        let dir = temp_dir(name);
+        let path = dir.join(STATE_FILE);
+        fs::create_dir(&path).expect("a directory where the file should go");
+        let store = StateStore::new(&path);
+        (dir, store)
+    }
+
+    #[test]
+    fn a_write_failure_reaches_a_caller_that_asks() {
+        // What `didbot-setup forget` needs: it prints a count of what it
+        // dropped, and a count of what only left memory is a lie about a
+        // destructive operation.
+        let (dir, store) = a_store_whose_write_cannot_land("update-checked");
+
+        let (dropped, written) =
+            store.update_checked(|state| state.insert(&key("sess-1", None), "did:web:a", PDS));
+        assert!(
+            written.is_err(),
+            "a write that never reached the disk was reported as success"
+        );
+        // The edit still happened, and its outcome is still handed back: a
+        // teardown needs the DIDs it removed whether or not the file recording
+        // the removal was written.
+        assert_eq!(dropped, ());
+        assert_eq!(
+            store.load(),
+            HookState::default(),
+            "nothing was written, so nothing is on file"
+        );
+
+        let leftovers: Vec<_> = fs::read_dir(&dir)
+            .expect("read temp dir")
+            .filter_map(Result::ok)
+            .map(|entry| entry.file_name())
+            .filter(|name| name.to_string_lossy().ends_with(".tmp"))
+            .collect();
+        assert!(
+            leftovers.is_empty(),
+            "a failed write left temporary files: {leftovers:?}"
+        );
+
+        let _ = fs::remove_dir_all(&dir);
+    }
+
+    #[test]
+    fn the_hook_s_update_hands_back_its_outcome_when_the_write_fails() {
+        // The invariant the whole split exists to protect: a hook event whose
+        // state write fails still returns, so the session carries on. The
+        // accounts `remove_session` reports are what `SessionEnd` goes on to
+        // delete server-side, and losing them would leak every one of them.
+        let (dir, store) = a_store_whose_write_cannot_land("update-infallible");
+        store.update(|state| state.insert(&key("sess-1", None), "did:web:a", PDS));
+
+        let removed = store.update(|state| {
+            state.insert(&key("sess-1", None), "did:web:a", PDS);
+            state.remove_session("sess-1")
+        });
+        assert_eq!(removed, vec![("did:web:a".to_string(), None)]);
+
+        let _ = fs::remove_dir_all(&dir);
+    }
+
+    /// A failed write must leave the file that was there before it whole:
+    /// stale is a degraded session, corrupt is a broken one.
+    ///
+    /// The failure is a directory nothing may create in, because it is the
+    /// only one that arrives *after* a good file exists. `root` is not subject
+    /// to the mode, so the test probes rather than assumes and steps aside if
+    /// the probe says the setup did not take.
+    #[cfg(unix)]
+    #[test]
+    fn a_failed_write_leaves_the_previous_file_whole() {
+        use std::os::unix::fs::PermissionsExt;
+
+        let dir = temp_dir("readonly");
+        let path = dir.join(STATE_FILE);
+        let store = StateStore::new(&path);
+        let k = key("sess-1", None);
+        store.update(|state| state.insert(&k, "did:web:first", PDS));
+
+        let restore = |dir: &Path| {
+            let _ = fs::set_permissions(dir, fs::Permissions::from_mode(0o700));
+            let _ = fs::remove_dir_all(dir);
+        };
+        fs::set_permissions(&dir, fs::Permissions::from_mode(0o500)).expect("chmod");
+        if fs::File::create(dir.join("probe")).is_ok() {
+            // Running as somebody the mode does not apply to.
+            restore(&dir);
+            return;
+        }
+
+        let (_, written) = store.update_checked(|state| state.insert(&k, "did:web:second", PDS));
+        assert!(
+            written.is_err(),
+            "the read-only directory did not stop the write, so this proves nothing"
+        );
+        assert_eq!(
+            store.load().did(&k),
+            Some("did:web:first"),
+            "the file that was already there did not survive the failed write"
+        );
+
+        restore(&dir);
+    }
+
     #[test]
     fn a_missing_file_is_not_an_error() {
         let dir = temp_dir("missing");
-- 
2.51.2