fix(mcp): rebuild the prompt prefix when the connector set changes

The `## MCP servers` table lives inside the frozen system prefix, which
PrefixCache holds for twenty idle minutes. Refreshing a user's global-access
snapshot fixed what `mcp.tools()` offers but left the table describing the
world before the change, so an admin could enable a connector, ask for it in an
open conversation, and be told in good faith that it does not exist — with the
tools sitting right there. Same gap on a reinstall, whose new
llm_short_description reached the runtime and not the prompt.

Both refreshes now call `invalidate_prefixes()` on the live contexts they were
already iterating, the seam the skill tools use. Order matters and runs against
the intuition: `render_mcp_list` renders the runtime's in-RAM state, not the
DB, so the invalidation goes last — after the snapshot refresh and after the
servers restart. Rebuild earlier and the prefix is repopulated from the very
descriptions being replaced, with nothing left to invalidate it a second time.
In the reinstall that means waiting out a dependency install; those users were
already reading a stale table, and an early rebuild would only freeze the stale
one in place.

Also warn when a feed's connector.json and index disagree on the integer
version. The manifest silently wins, so if the index is the lower of the two
the strict `feed > installed` comparison is false forever: the connector never
offers an Update and nothing anywhere says why.
This commit is contained in:
Daniele
2026-08-10 13:20:26 +01:00
parent 59549d2b3b
commit 2dad4824c9
3 changed files with 48 additions and 1 deletions
+3 -1
View File
@@ -394,7 +394,9 @@ Same economics, other end of the request. `AgentSystemContext::system_context` i
The refresh rule is the only one that is free: **rebuild once the conversation has been idle longer than a provider's cache could survive** (`PREFIX_TTL`, 20 min). The clock is therefore *idle time of this conversation*, not time since a file changed, and reading restarts it — every `get` is a request about to go out. The asymmetry that sets the constant: below a provider's window you pay misses that buy nothing, above it you only pay freshness.
**Writes are deliberately not reacted to, and there is no bus variant for this.** When the agent itself edits an injected file the content is already in the context — its tool call and result sit two messages downstream — so refreshing would repeat what the model just said. A write from *elsewhere* (the same user's Telegram session, a cron job, another member editing `shared-memory/`) is genuinely invisible until the TTL: that is the case where an immediate rebuild costs the most, since a conversation that would notice is by definition a warm one, and the cheaper freshness path already exists — the agent can `read_file`, and a tool result *appends*, which invalidates nothing. The injection header says so in words. Cross-user invalidation would need a `SystemEventBus` variant plus a subscriber per user (the writer lives in a different `UserContext`); it is future work, and this type's key is the seam for it. Note `base` is frozen **whole**: freezing the memory files while letting `__USER_PROFILE__` move would invalidate just as much. The cost is that an `AGENT.md` edit lands at the next rebuild rather than the next round.
**Writes are deliberately not reacted to, and there is no bus variant for this.** When the agent itself edits an injected file the content is already in the context — its tool call and result sit two messages downstream — so refreshing would repeat what the model just said. A write from *elsewhere* (the same user's Telegram session, a cron job, another member editing `shared-memory/`) is genuinely invisible until the TTL: that is the case where an immediate rebuild costs the most, since a conversation that would notice is by definition a warm one, and the cheaper freshness path already exists — the agent can `read_file`, and a tool result *appends*, which invalidates nothing. The injection header says so in words. Cross-user invalidation of a *file* write would need a `SystemEventBus` variant plus a subscriber per user (the writer lives in a different `UserContext`); it is future work, and this type's key is the seam for it. Note `base` is frozen **whole**: freezing the memory files while letting `__USER_PROFILE__` move would invalidate just as much. The cost is that an `AGENT.md` edit lands at the next rebuild rather than the next round.
**What *is* invalidated eagerly: the two generated lists, because a stale one makes the model deny a tool it has.** The TTL is right for injected content the agent can re-read on demand and wrong for an inventory — a model that reads "no such connector" in the `## MCP servers` table does not go looking, it answers the question. So `Skald::invalidate_prompt_prefix` (the skills door, called straight from `skill_register`/`skill_delete`) has two MCP siblings, both looping the `all_live()` they already had: `refresh_global_mcp_access` — the admin enabling or re-granting a global connector, where refreshing the access snapshot alone fixed what `mcp.tools()` *offers* while leaving the table describing the world before it — and `refresh_connector_after_reinstall`, where a reinstall's new `llm_short_description` reached the runtime but not the prompt. **Order is load-bearing and opposite to the intuition**: `render_mcp_list` renders the live runtime's in-RAM state, not the DB, so the invalidation goes **last**, after the snapshot refresh and after the servers restart — rebuild the prefix first and it is repopulated from the very descriptions being replaced, with nothing left to invalidate it again. In the reinstall that means waiting out a global dependency install that can take minutes; correct anyway, since those users were already reading a stale table and an early rebuild would only freeze the stale one in place. The price is a provider cache miss on the next turn of every open conversation of every live user — cross-user by nature, since one admin is changing something for other people, and there is no cheaper direct path the way there is for a user editing their own memory. It buys back the failure the skills doc-comment already describes word for word.
## Approval gate
+32
View File
@@ -235,6 +235,16 @@ impl Skald {
if let Err(e) = ctx.refresh_global_access().await {
tracing::warn!(user = %ctx.user_id, error = %e, "failed to refresh global MCP access");
}
// Then rebuild the frozen prompt prefix, for the reason spelled out in
// `invalidate_prompt_prefix`: refreshing the snapshot fixes what `mcp.tools()`
// *offers*, while the `## MCP servers` table the model reads lives inside
// `base`, which `PrefixCache` holds for twenty idle minutes. Without this the
// admin enables a connector, asks for it in an open conversation, and is told
// in good faith that it does not exist — with the tools sitting right there.
//
// After the refresh, never before: the table is rendered from the access
// snapshot we just replaced.
ctx.sessions.loop_runtime().invalidate_prefixes();
}
}
@@ -252,6 +262,8 @@ impl Skald {
/// re-copy its files/deps into the container (`prepare_local_connector` — a hash
/// no-op when the source is unchanged) and restart that one server. The rebuilt
/// spec now carries the fresh catalog description (see `user_row_spec_resolved`).
/// - **Prompt prefix**: last, invalidate it for every live user, so the
/// `## MCP servers` table stops describing the version that was just replaced.
///
/// Best-effort: the catalog write already committed, so a Docker/MCP hiccup here
/// must not fail the reinstall — anything not refreshed settles at the user's next
@@ -329,6 +341,26 @@ impl Skald {
tracing::warn!(user = %ctx.user_id, connector = %catalog_name, error = %e, "reinstall refresh: failed to restart per-user connector");
}
}
// 3. Rebuild the frozen prompt prefix, for everyone — a reinstall changes the
// connector's `llm_short_description`, which the model reads from the
// `## MCP servers` table inside `base` rather than from the runtime it just
// reconnected to. Restarting the servers alone left the prompt describing the
// old version for up to twenty idle minutes.
//
// **Last, deliberately.** `render_mcp_list` renders the live runtime's in-RAM
// state, so a prefix rebuilt before the restarts above would be repopulated
// from the descriptions we are in the middle of replacing — and nothing would
// invalidate it a second time. That the global dependency install can take
// minutes is not a reason to move this earlier: those users were already
// reading a stale table, and rebuilding it early would only freeze the stale
// one in place.
//
// Everyone, not just the users who run this connector per-user: an enabled
// global connector is in every granted user's table.
for ctx in self.rt_user_contexts().all_live().await {
ctx.sessions.loop_runtime().invalidate_prefixes();
}
}
pub fn sessions(&self) -> &Arc<crate::auth::SessionStore> { &self.rt.sessions }
pub fn config(&self) -> &Arc<GlobalConfigManager> { &self.rt.config }
+13
View File
@@ -393,6 +393,19 @@ fn card_of(h: &Hydrated, installed: bool, installed_version: Option<i64>) -> Mar
let source = norm_source(&h.entry, &h.manifest);
let doc = h.manifest.docs.first().cloned().unwrap_or_default();
// Prefer the manifest's version trio, falling back to the index entry's.
// A disagreement between the two is a malformed feed and is otherwise completely
// invisible: the manifest silently wins, `installed_version` keeps whatever the
// install snapshotted, and if the feed's index is the lower of the two the strict
// `feed > have` below is false forever — the connector never offers an Update and
// nothing anywhere says why. Say it once, in the log.
if let (Some(m), Some(e)) = (h.manifest.version, h.entry.version) {
if m != e {
tracing::warn!(
connector = %h.entry.id, manifest_version = m, index_version = e,
"marketplace feed version desync: connector.json and the index disagree; the manifest wins",
);
}
}
let version = h.manifest.version.or(h.entry.version);
let version_string = h.manifest.version_string.clone().or_else(|| h.entry.version_string.clone());
let version_release_date = h.manifest.version_release_date.clone().or_else(|| h.entry.version_release_date.clone());