connectors: announce global-server and reinstall refreshes on the bus
Nightly Build / build (push) Successful in 6m56s

Five call-sites reached into the live-runtime refresh helpers from HTTP
handlers, the same shape as the container remounts. Only three of them
belonged on the bus, and finding out which was the point.

global_enable and global_delete now emit McpGlobalServersChanged, and the
marketplace reinstall emits ConnectorReinstalled. All three are pure
reconciliation: the first only makes a connector appear; the second is
already enforced by stop_server, with the snapshot refresh just tidying
each user's filter; the third pushes metadata and code into what is
already running. The reinstall gains something from being off the
response path, since it re-copies files and restarts servers inside every
live user's container.

global_set_access and user_connectors_set keep calling
refresh_global_mcp_access directly. Their writes *replace* a grant set, so
anyone dropped from the list is being revoked and that refresh is what
enforces it — on a best-effort broadcast a revoked user would keep the
connector until their next login. Both carry a DELIBERATELY SYNCHRONOUS
comment, since they are otherwise indistinguishable from the announced
call-sites and are exactly what a later cleanup would sweep up.

No behaviour change for the two synchronous paths; the three announced
ones now return without waiting for the refresh.
This commit is contained in:
2026-07-26 22:22:29 +01:00
parent 0ba140186f
commit 305bdbdd2b
5 changed files with 59 additions and 18 deletions
+3 -3
View File
@@ -28,14 +28,14 @@ Three global broadcast buses — **never add a fourth without checking these fir
| Bus | Cap | Events | File |
|-----|-----|--------|------|
| `ChatEventBus` | 256 | user message, assistant response, compaction done | `core-api/src/bus.rs` |
| `SystemEventBus` | 64 | provider (un)registered, config key updated, job completed, session cancelled, **user created/deleted/active-changed/mounts-changed** | `core-api/src/system_bus.rs` |
| `SystemEventBus` | 64 | provider (un)registered, config key updated, job completed, session cancelled, **user created/deleted/active-changed/mounts-changed**, **global connectors changed, connector reinstalled** | `core-api/src/system_bus.rs` |
| `GlobalEvent` (per-user) | 512 | all `ServerEvent` variants → WS clients + inbox lifecycle | `core-api/src/events.rs` |
Plus internal `mpsc` queues: per-source `SourceInbox` (message serialization) and a central `notify` queue (background agents → user).
**The user-lifecycle reconciler** is the worked example of the rule. Creating a user, deleting one, deactivating one, or changing a shared-folder/project membership all need Docker work (provision, tear down, stop, recreate with new bind mounts). None of the endpoints that make those changes touches `ContainerManager`: each announces `SystemEvent::User{Created,Deleted,ActiveChanged,MountsChanged}` **after** its DB write, and one subscriber — `skald::wiring::spawn_user_lifecycle`, spawned post-construction because it reacts through `Skald`'s own accessors, holding only a `Weak` — does the reacting, sequentially and best-effort. The payoff is that a *future* endpoint granting membership cannot forget to remount, because remounting was never its job. Reactions never block the HTTP response, and a failure settles at the user's next login or at boot reconciliation.
**The user-lifecycle reconciler** is the worked example of the rule. Creating a user, deleting one, deactivating one, or changing a shared-folder/project membership all need Docker work (provision, tear down, stop, recreate with new bind mounts); enabling or reinstalling a connector needs live runtimes re-snapshotted. None of the endpoints that make those changes touches `ContainerManager` or the refresh helpers: each announces `SystemEvent::User{Created,Deleted,ActiveChanged,MountsChanged}` / `McpGlobalServersChanged` / `ConnectorReinstalled` **after** its DB write, and one subscriber — `skald::wiring::spawn_user_lifecycle`, spawned post-construction because it reacts through `Skald`'s own accessors, holding only a `Weak` — does the reacting, sequentially and best-effort. Being off the response path matters for `ConnectorReinstalled` in particular: it re-copies files and restarts servers inside every live user's container, seconds of work the admin's install no longer waits on. The payoff is that a *future* endpoint granting membership cannot forget to remount, because remounting was never its job. Reactions never block the HTTP response, and a failure settles at the user's next login or at boot reconciliation.
**Where the bus stops: reconciliation rides it, authorization does not.** `SystemEventBus` is a lossy 64-slot broadcast whose contract is *"best-effort, settles at the next login"* — right for a stale mount, wrong for a revocation, where "settles later" *is* the failure. So deactivating or deleting a user splits in two: `Skald::revoke_user_runtime` runs **synchronously in the handler, before it responds** (revoke every session → evict + cancel the `UserContext``UserManager::lock`, in that order, so nothing is left querying a pool we then close and the DEK leaves RAM per §9), while only the container half — stop or remove — rides the bus. Before this, `active = 0` blocked the *next* login but left live sessions working: `login` checks the flag, `require_auth` only maps token → id. **Never put an access revocation on a bus.**
**Where the bus stops: reconciliation rides it, authorization does not.** `SystemEventBus` is a lossy 64-slot broadcast whose contract is *"best-effort, settles at the next login"* — right for a stale mount, wrong for a revocation, where "settles later" *is* the failure. So deactivating or deleting a user splits in two: `Skald::revoke_user_runtime` runs **synchronously in the handler, before it responds** (revoke every session → evict + cancel the `UserContext``UserManager::lock`, in that order, so nothing is left querying a pool we then close and the DEK leaves RAM per §9), while only the container half — stop or remove — rides the bus. Before this, `active = 0` blocked the *next* login but left live sessions working: `login` checks the flag, `require_auth` only maps token → id. Same split for security groups (see the picker section) and for connectors, where the test is worth internalising because the call is literally the same function: `Skald::refresh_global_mcp_access` is **announced** (`McpGlobalServersChanged`) when a global connector is enabled or deleted — the first only makes something *appear*, the second is already enforced by `stop_server` — but **called directly** from `global_set_access` and `user_connectors_set`, where `set_access`/`set_for_user` *replace* a grant set and the refresh is what actually revokes. Both sync call-sites carry a `DELIBERATELY SYNCHRONOUS` comment, because they look identical to the announced ones. **Never put an access revocation on a bus.**
**Before you add a direct function call or a new import between two components, stop and ask:** is one component producing data another needs? If yes, add a variant to an existing bus and spawn a subscriber. Don't call `some_manager.log_thing(...)` from the producer — emit a `ThingHappened` event on `SystemEventBus` and let the manager subscribe.