agent: Reduce log spam for context servers (#33644)

Bennet Bo Fenner created 5 months ago

Previously we would always run `maintain_servers` even if the settings
did not change. While this would not cause any MCP servers to restart,
we would still go through all configured servers and call the
`command(...)` function on each installed MCP extension. This can cause
lots of logs to show up when an MCP server is not configured correctly.

Release Notes:

- N/A

Change summary

crates/project/src/context_server_store.rs | 52 +++++++++++++++++++----
1 file changed, 42 insertions(+), 10 deletions(-)

Detailed changes

crates/project/src/context_server_store.rs

@@ -135,6 +135,7 @@ pub type ContextServerFactory =
     Box<dyn Fn(ContextServerId, Arc<ContextServerConfiguration>) -> Arc<ContextServer>>;
 
 pub struct ContextServerStore {
+    context_server_settings: HashMap<Arc<str>, ContextServerSettings>,
     servers: HashMap<ContextServerId, ContextServerState>,
     worktree_store: Entity<WorktreeStore>,
     registry: Entity<ContextServerDescriptorRegistry>,
@@ -202,6 +203,11 @@ impl ContextServerStore {
                     this.available_context_servers_changed(cx);
                 }),
                 cx.observe_global::<SettingsStore>(|this, cx| {
+                    let settings = Self::resolve_context_server_settings(&this.worktree_store, cx);
+                    if &this.context_server_settings == settings {
+                        return;
+                    }
+                    this.context_server_settings = settings.clone();
                     this.available_context_servers_changed(cx);
                 }),
             ]
@@ -211,6 +217,8 @@ impl ContextServerStore {
 
         let mut this = Self {
             _subscriptions: subscriptions,
+            context_server_settings: Self::resolve_context_server_settings(&worktree_store, cx)
+                .clone(),
             worktree_store,
             registry,
             needs_server_update: false,
@@ -268,10 +276,8 @@ impl ContextServerStore {
         cx.spawn(async move |this, cx| {
             let this = this.upgrade().context("Context server store dropped")?;
             let settings = this
-                .update(cx, |this, cx| {
-                    this.context_server_settings(cx)
-                        .get(&server.id().0)
-                        .cloned()
+                .update(cx, |this, _| {
+                    this.context_server_settings.get(&server.id().0).cloned()
                 })
                 .ok()
                 .flatten()
@@ -439,12 +445,11 @@ impl ContextServerStore {
         }
     }
 
-    fn context_server_settings<'a>(
-        &'a self,
+    fn resolve_context_server_settings<'a>(
+        worktree_store: &'a Entity<WorktreeStore>,
         cx: &'a App,
     ) -> &'a HashMap<Arc<str>, ContextServerSettings> {
-        let location = self
-            .worktree_store
+        let location = worktree_store
             .read(cx)
             .visible_worktrees(cx)
             .next()
@@ -492,9 +497,9 @@ impl ContextServerStore {
     }
 
     async fn maintain_servers(this: WeakEntity<Self>, cx: &mut AsyncApp) -> Result<()> {
-        let (mut configured_servers, registry, worktree_store) = this.update(cx, |this, cx| {
+        let (mut configured_servers, registry, worktree_store) = this.update(cx, |this, _| {
             (
-                this.context_server_settings(cx).clone(),
+                this.context_server_settings.clone(),
                 this.registry.clone(),
                 this.worktree_store.clone(),
             )
@@ -990,6 +995,33 @@ mod tests {
                 assert_eq!(store.read(cx).status_for_server(&server_2_id), None);
             });
         }
+
+        // Ensure that nothing happens if the settings do not change
+        {
+            let _server_events = assert_server_events(&store, vec![], cx);
+            set_context_server_configuration(
+                vec![(
+                    server_1_id.0.clone(),
+                    ContextServerSettings::Extension {
+                        enabled: true,
+                        settings: json!({
+                            "somevalue": false
+                        }),
+                    },
+                )],
+                cx,
+            );
+
+            cx.run_until_parked();
+
+            cx.update(|cx| {
+                assert_eq!(
+                    store.read(cx).status_for_server(&server_1_id),
+                    Some(ContextServerStatus::Running)
+                );
+                assert_eq!(store.read(cx).status_for_server(&server_2_id), None);
+            });
+        }
     }
 
     #[gpui::test]