diff --git a/CHANGELOG.md b/CHANGELOG.md index bfd002ee9..cd564de4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -8,6 +8,16 @@ Newest first. `Unreleased` is what is on `main` and not yet tagged. ## Unreleased +### A failed tool refresh no longer leaves a connector offering nothing + +Refreshing a connector's tools replaced the list with a delete and then an insert, as two separate +statements. Whenever the second did not land — a pod killed mid-refresh, a dropped connection, or a +server answering `tools/list` with the same tool name twice, which the table refuses — the delete had +already committed on its own. The table is shared, so every replica lost that connector's tools at +once, every grant an administrator had made was silently un-offered, and the Bot was told it holds +none of that vendor's tools. Nothing brought them back until somebody read the error on the Plugins +page and pressed Refresh. The two statements are now one, so a bad refresh is recorded and the tools +already held are left alone, which is what the code always claimed to do. ### A rule tried in dry-run now says what it would have refused a Bot's tools `dry-run` exists so a boundary can be measured against live traffic before it starts refusing diff --git a/server/src/plugins/store.ts b/server/src/plugins/store.ts index 1b8ca1bb1..11867e4ce 100644 --- a/server/src/plugins/store.ts +++ b/server/src/plugins/store.ts @@ -1924,17 +1924,37 @@ export function createPluginStore(options: PluginStoreOptions) { token, }); - await database.delete(mcpTools).where(eq(mcpTools.serverId, serverId)); - if (tools.length > 0) { - await database.insert(mcpTools).values( - tools.map((tool) => ({ - serverId, - name: tool.name, - description: tool.description, - inputSchema: tool.inputSchema, - })), - ); - } + /* + * ONE STEP, because the catch below promises that it is one. + * + * "The tools already held are left alone" is only true while nothing has been written yet. + * As two auto-committed statements the delete landed on its own whenever the insert did not: + * a pod killed mid-refresh, a dropped connection, a statement timeout — or, with no crash at + * all, a server that answers `tools/list` with the same `name` twice, which `mcp_tools`' + * `(server_id, name)` primary key refuses as one multi-row insert. `mcp_tools` is shared, so + * that is every replica at once, and nothing repopulates it: `refreshTools` is only ever + * called by `addServer`, `addCustomServer` and an administrator pressing Refresh. The + * connector kept every grant an administrator had made and offered none of them, and + * `grantedToolGuidance` then told the Bot outright that it holds none of that vendor's tools. + * + * Rolled back together, the vendor's bad answer is recorded in `lastError` and the Bots go + * on using what they were granted, which is what the comment said all along. + */ + await database.transaction(async (transaction) => { + await transaction + .delete(mcpTools) + .where(eq(mcpTools.serverId, serverId)); + if (tools.length > 0) { + await transaction.insert(mcpTools).values( + tools.map((tool) => ({ + serverId, + name: tool.name, + description: tool.description, + inputSchema: tool.inputSchema, + })), + ); + } + }); await database .update(mcpServers)