From 929bf6b0dedf38d79f0aef3e5acb6d5d6ebe9d4e Mon Sep 17 00:00:00 2001
From: "@permadeath.com" <did:plc:nlzmjyfv6loqtxyzvdcznwgf>
Date: Fri, 11 Sep 2026 20:10:28 -0400
Subject: [PATCH] fix(agentd): drop a forgotten session's lock

The per-session lock map only ever grew; a session taken out of the store now
takes its lock with it, unless another caller is still waiting on it.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Change-Id: Ie57fe9d27d6895ad2cf98348ca885db800711eda
---
 crates/didbot-agentd/src/sessions.rs | 82 +++++++++++++++++++++++++++-
 1 file changed, 80 insertions(+), 2 deletions(-)

diff --git a/crates/didbot-agentd/src/sessions.rs b/crates/didbot-agentd/src/sessions.rs
index f948ae99..d7355d33 100644
--- a/crates/didbot-agentd/src/sessions.rs
+++ b/crates/didbot-agentd/src/sessions.rs
@@ -370,8 +370,12 @@ impl SessionStore {
     /// `false` when there was none.
     pub async fn remove(&self, key: &Key) -> Result<bool, StoreError> {
         let lock = self.lock_for(key);
-        let _held = lock.lock().await;
-        self.forget(key)
+        let gone = {
+            let _held = lock.lock().await;
+            self.forget(key)
+        };
+        self.release(key, &lock);
+        gone
     }
 
     /// Refreshes the session under `key` through `refresh`, and keeps what
@@ -398,8 +402,33 @@ impl SessionStore {
         E: std::error::Error + 'static,
     {
         let lock = self.lock_for(key);
+        let mut forgotten = false;
+        let outcome = self
+            .update_locked(&lock, key, refresh, &mut forgotten)
+            .await;
+        if forgotten {
+            self.release(key, &lock);
+        }
+        outcome
+    }
+
+    /// [`SessionStore::update`], under `lock`. Sets `forgotten` when it
+    /// leaves no session under `key`.
+    async fn update_locked<F, Fut, E>(
+        &self,
+        lock: &tokio::sync::Mutex<()>,
+        key: &Key,
+        refresh: F,
+        forgotten: &mut bool,
+    ) -> Result<Session, UpdateError<E>>
+    where
+        F: FnOnce(Session) -> Fut,
+        Fut: Future<Output = Result<Session, Refusal<E>>>,
+        E: std::error::Error + 'static,
+    {
         let _held = lock.lock().await;
         let Some(before) = self.get(key) else {
+            *forgotten = true;
             return Err(UpdateError::Missing(key.clone()));
         };
 
@@ -433,6 +462,7 @@ impl SessionStore {
                     // this server has already ended, so name it.
                     warn!(account = %key.account, client_id = %key.client_id, path = %self.path_of(key).display(), error = %err, "a revoked session is forgotten but its file is still there");
                 }
+                *forgotten = true;
                 Err(UpdateError::Revoked(why))
             }
             Err(Refusal::Failed(why)) => Err(UpdateError::Failed(why)),
@@ -476,6 +506,27 @@ impl SessionStore {
         Arc::clone(locks.entry(key.clone()).or_default())
     }
 
+    /// Drops `key`'s lock, so a store that signs accounts in and out over a
+    /// long life keeps one entry per session rather than one per session it
+    /// has ever held.
+    ///
+    /// Call it with `lock` no longer held. It keeps the entry unless this is
+    /// the only reference besides the map's, because a caller waiting on the
+    /// same mutex would otherwise find the next one handed a fresh mutex --
+    /// two locks for one session, and two refreshes free to spend one refresh
+    /// token. [`SessionStore::lock_for`] takes the map's own lock too, so a
+    /// caller arriving during this either keeps the entry alive or gets a
+    /// fresh one nobody else holds.
+    fn release(&self, key: &Key, lock: &Arc<tokio::sync::Mutex<()>>) {
+        let mut locks = self
+            .locks
+            .lock()
+            .unwrap_or_else(|poisoned| poisoned.into_inner());
+        if Arc::strong_count(lock) == 2 {
+            locks.remove(key);
+        }
+    }
+
     fn current_ref(&self) -> std::sync::RwLockReadGuard<'_, BTreeMap<Key, Session>> {
         self.current
             .read()
@@ -864,6 +915,33 @@ mod tests {
         assert_eq!(store.get(&key), None, "and it is not held");
     }
 
+    /// The per-session locks are the one map a long-lived store could grow
+    /// without bound: a daemon that signs accounts in and out all day would
+    /// keep an entry for every session it had ever held.
+    #[tokio::test]
+    async fn a_forgotten_session_takes_its_lock_with_it() {
+        let scratch = Scratch::new("sessions-locks");
+        let store = SessionStore::open(&scratch.0).unwrap();
+        let held = session(APP, AGENT, "access-1", "refresh-1");
+        let key = held.key();
+        store.put(held).await.unwrap();
+
+        assert!(store.remove(&key).await.unwrap());
+        assert!(store.locks.lock().unwrap().is_empty(), "removed");
+
+        let missing = store
+            .update(&key, |same| async move { Ok::<_, Refusal<Said>>(same) })
+            .await;
+        assert!(
+            matches!(missing, Err(UpdateError::Missing(_))),
+            "{missing:?}"
+        );
+        assert!(
+            store.locks.lock().unwrap().is_empty(),
+            "and an update of a session nobody holds leaves nothing either"
+        );
+    }
+
     #[tokio::test]
     async fn the_directory_and_every_file_are_shut() {
         let scratch = Scratch::new("sessions-modes");
-- 
2.51.2