Multi-Matrix-server support (Phase 1): registry, namespaced KV, v3 migration - #29
Multi-Matrix-server support (Phase 1): registry, namespaced KV, v3 migration#29fmartingr wants to merge 2 commits into
Conversation
…v3 migration) Restructure the plugin internals so a Matrix homeserver is modeled as an entry in a managed registry keyed by a stable serverID, laying the groundwork for supporting multiple homeservers. No cross-server routing yet: the bridge still resolves a single server. Zero behavior change for a single-server operator. - Registry: servers_config KV entry (ServerConfig) reconciled from the flat plugin.json config; serverID derived deterministically from the homeserver hostname so records are re-adopted if a server is re-created with the same URL. - KV namespacing: per-server keys become <prefix><serverID>_<id>; channel_mapping_ value becomes a []ChannelServerMapping (server association in the value). - Client registry: p.matrixClient -> map[serverID]*matrix.Client with accessors. - v3 KV migration: namespaces existing per-server keys and converts channel_mapping values; deterministic, idempotent, and aborts (without bumping the version) on partial failure so it safely retries. Legacy v1/v2 steps stay un-namespaced. - Per-server username prefix resolved from the registry (source of truth). - Tests: registry reconcile/derivation, migration (fresh/upgrade/idempotent/ reset/failure-abort), per-server isolation, corrupt-value handling, command handlers, and a two-container live-Synapse integration suite. Docs under spec/.
📝 WalkthroughWalkthroughThis change adds a server registry and server-scoped Matrix clients and KV mappings, migrates existing KV data to version 3, updates commands and synchronization paths, expands multi-server tests, and adds a Docker Compose Element Web client with local development documentation. ChangesLocal Element Web testing
Multi-server Matrix support
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Plugin
participant ServerRegistry
participant MatrixClients
participant Bridge
participant KVStore
Plugin->>ServerRegistry: reconcile configured homeserver
ServerRegistry-->>Plugin: serverID and server configuration
Plugin->>MatrixClients: create client keyed by serverID
Plugin->>Bridge: provide client and serverID
Bridge->>KVStore: read or write server-scoped mapping
Bridge->>MatrixClients: perform Matrix operation
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
server/store/kvstore/schema.go (1)
37-45: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDoc comment contradicts the actual (and tested) multi-entry behaviour.
This struct's comment states the mapping list "currently always has length 1," but
UpsertChannelServerMapping(lines 98-110 below, same file) is explicitly designed to append rather than replace, preserving other servers' entries — andTestSetChannelRoomMappingPreservesOtherServersproves 2 entries can coexist today. The comment should describe the actual invariant (at most one entry perserverID, not "length 1" overall).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@server/store/kvstore/schema.go` around lines 37 - 45, Update the ChannelServerMapping documentation to remove the claim that the mapping list always has length 1 and state that multiple entries may coexist, with at most one entry per serverID. Keep the field-level comments unchanged.spec/2026-07-16-multi-matrix-server-support-phase-1.md (2)
34-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Length 1 enforced now" isn't actually enforced by the shipped code.
UpsertChannelServerMapping(schema.go) andsetChannelRoomMapping(bridge_utils.go) both append/upsert per-server entries without capping the list at length 1, and a test explicitly verifies a 2-entry list. Worth updating this doc to state the real invariant (at most one entry perserverID).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/2026-07-16-multi-matrix-server-support-phase-1.md` around lines 34 - 35, Update the documentation’s channel_mapping_<channelID> invariant to state that the list may contain multiple entries but has at most one entry per serverID. Align the wording with UpsertChannelServerMapping and setChannelRoomMapping, and remove the inaccurate “length 1 enforced now” claim.
94-103: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win"Files to change" plan doesn't match the actual implementation shipped in this PR.
The plan lists
server/configuration.goas gainingGetMatrixUsernamePrefixForServer(serverID), but the shipped code removes that method entirely and resolves the per-server prefix inserver/bridge_utils.go's newmatrixUsernamePrefix()instead (readingKeyServersConfiglive). The list also omits the newserver/servers.gofile, which is wheregetServers,reconcileServerConfig, andderiveServerIDactually ended up. Worth syncing this section with the final design so later phases (and readers) aren't misled about where this logic lives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@spec/2026-07-16-multi-matrix-server-support-phase-1.md` around lines 94 - 103, Update the “Files to change” plan to match the shipped design: remove the planned configuration.go method reference, describe bridge_utils.go’s matrixUsernamePrefix() as the per-server prefix resolver, and add server/servers.go with getServers, reconcileServerConfig, and deriveServerID. Keep the remaining file responsibilities aligned with the implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@server/bridge_utils.go`:
- Around line 161-188: Update BridgeUtils.matrixUsernamePrefix to cache the last
successfully resolved username prefix per serverID and return that cached value
when the registry read or ParseServersConfig fails. Preserve the static
DefaultMatrixUsernamePrefix only when no successful resolution has ever
occurred, while continuing to update the cache whenever a valid server prefix is
found.
In `@server/command/command.go`:
- Around line 522-528: Update the corrupt channel mapping cleanup branch around
kvstore.Delete so a deletion failure returns an error response instead of the
“Corrupt Mapping Cleared” success response. Preserve the existing success
response only when Delete succeeds, and use the existing command error-response
pattern and symbols in this handler.
In `@server/migrations.go`:
- Around line 500-526: Update runMigrationToVersion3WithResults so an empty
serverID is not automatically treated as a successful no-op: verify the KV store
contains no legacy per-server keys, including v3NamespacedPrefixes and
channel_mapping_ entries, before returning an empty MigrationResult. If legacy
data exists without a configured server, return an error so
runKVStoreMigrationsWithResults does not advance the version marker; retain the
current no-op only when no legacy data is present.
In `@server/plugin.go`:
- Around line 123-129: Update the reinitialization call to p.initMatrixClient in
the activation flow so any error is returned immediately instead of merely
logged, preventing activation and subsequent sync with clients carrying the
pre-registration remote ID. Preserve the successful initialization path and
existing error context while ensuring activation cannot continue until the
clients are rebuilt successfully.
In `@server/servers.go`:
- Around line 77-145: Serialize the entire reconcileServerConfig operation,
including reading existing servers, deriving the entry, persisting
servers_config, and updating p.serverID, using the shared mutex or a dedicated
initializer lock. Ensure concurrent OnConfigurationChange and OnActivate calls
cannot interleave or overwrite a newer RemoteID or configuration.
In `@server/user_remote_detection_test.go`:
- Around line 213-220: In the ghost-user message test, update the SendMessage
assertions to use require.NoError so execution stops when sending fails, then
require or assert that response is non-nil before accessing response.EventID.
Keep the existing EventID validation and logging behavior in the surrounding
test.
In `@spec/2026-07-16-multi-matrix-server-support-phase-4.md`:
- Around line 18-23: Define the URL-edit behavior in the PUT /servers/{serverID}
flow to preserve the hostname-derived serverID invariant: either reject hostname
changes as immutable, or atomically re-key the server and migrate all namespaced
records to deriveServerID(new hostname). Ensure the update cannot leave the old
ID associated with a different hostname or allow the new homeserver to inherit
its mappings.
---
Nitpick comments:
In `@server/store/kvstore/schema.go`:
- Around line 37-45: Update the ChannelServerMapping documentation to remove the
claim that the mapping list always has length 1 and state that multiple entries
may coexist, with at most one entry per serverID. Keep the field-level comments
unchanged.
In `@spec/2026-07-16-multi-matrix-server-support-phase-1.md`:
- Around line 34-35: Update the documentation’s channel_mapping_<channelID>
invariant to state that the list may contain multiple entries but has at most
one entry per serverID. Align the wording with UpsertChannelServerMapping and
setChannelRoomMapping, and remove the inaccurate “length 1 enforced now” claim.
- Around line 94-103: Update the “Files to change” plan to match the shipped
design: remove the planned configuration.go method reference, describe
bridge_utils.go’s matrixUsernamePrefix() as the per-server prefix resolver, and
add server/servers.go with getServers, reconcileServerConfig, and
deriveServerID. Keep the remaining file responsibilities aligned with the
implementation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 190e9d0f-2daa-4488-893e-e2d438e37dc9
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (37)
README.mddocker-compose.ymldocker/element-config.jsonserver/bridge_utils.goserver/command/command.goserver/command/command_test.goserver/configuration.goserver/dm_room_creation_test.goserver/dm_support_test.goserver/hooks.goserver/matrix_mentions_integration_test.goserver/matrix_util.goserver/matrix_webhook.goserver/migrations.goserver/migrations_test.goserver/multi_server_integration_test.goserver/plugin.goserver/plugin_integration_test.goserver/servers.goserver/servers_test.goserver/store/kvstore/constants.goserver/store/kvstore/schema.goserver/store/kvstore/schema_test.goserver/sync_to_matrix.goserver/sync_to_matrix_integration_test.goserver/sync_to_matrix_test.goserver/sync_to_mattermost.goserver/sync_to_mattermost_test.goserver/testhelpers_test.goserver/thread_mapping_test.goserver/user_remote_detection_test.gospec/2026-07-16-multi-matrix-server-support-phase-1.mdspec/2026-07-16-multi-matrix-server-support-phase-2.mdspec/2026-07-16-multi-matrix-server-support-phase-3.mdspec/2026-07-16-multi-matrix-server-support-phase-4.mdspec/2026-07-16-multi-matrix-server-support-phase-5.mdspec/2026-07-16-multi-matrix-server-support-phase-6.md
| // matrixUsernamePrefix returns the username prefix for this bridge's Matrix | ||
| // server, resolved from the managed server registry, which is the source of | ||
| // truth for per-server settings. reconcileServerConfig always populates each | ||
| // entry's prefix (defaulting to DefaultMatrixUsernamePrefix) before bridges run, | ||
| // so a configured server always resolves here. The static default is only for | ||
| // the degenerate case of no registered server (e.g. before the first reconcile). | ||
| // The registry is read live so prefix changes take effect without recreating the | ||
| // bridge. | ||
| func (s *BridgeUtils) matrixUsernamePrefix() string { | ||
| data, err := s.kvstore.Get(kvstore.KeyServersConfig) | ||
| if err != nil { | ||
| // A transient registry read failure must not silently change the prefix: | ||
| // a wrong prefix splits Matrix-user identity (ghosts created and matched | ||
| // under a different prefix). Surface it loudly, mirroring GetMatrixRoomID. | ||
| s.logger.LogError("Failed to read server registry for username prefix; using default", "server_id", s.serverID, "error", err) | ||
| return DefaultMatrixUsernamePrefix | ||
| } | ||
| servers, err := kvstore.ParseServersConfig(data) | ||
| if err != nil { | ||
| s.logger.LogError("Corrupt server registry; using default username prefix", "server_id", s.serverID, "error", err) | ||
| return DefaultMatrixUsernamePrefix | ||
| } | ||
| if server, ok := kvstore.ServerConfigForID(servers, s.serverID); ok && server.UsernamePrefix != "" { | ||
| return server.UsernamePrefix | ||
| } | ||
| // No registry yet (before the first reconcile) or no entry for this server. | ||
| return DefaultMatrixUsernamePrefix | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Transient registry-read failure silently falls back to the exact "wrong prefix" scenario the comment warns against.
The doc comment says a registry-read failure "must not silently change the prefix" and should "surface it loudly, mirroring GetMatrixRoomID" — but unlike GetMatrixRoomID, this method returns only a string, so on KV read/parse failure it can only log and fall back to DefaultMatrixUsernamePrefix, which is a silent prefix change if the registry's actual entry differs from the default. Since this prefix drives ghost-user creation and Matrix-user reconstruction (line 501), a transient KV blip could momentarily split identity resolution.
Consider caching the last successfully-resolved prefix (per serverID) and falling back to that cached value instead of the static default on transient failure, reserving the static default only for the case where no successful read has ever occurred.
🔧 Suggested approach
type BridgeUtils struct {
logger Logger
API plugin.API
kvstore kvstore.KVStore
matrixClient *matrix.Client
serverID string
remoteID string
maxProfileImageSize int64
maxFileSize int64
configGetter ConfigurationGetter
+ // cache of the last successfully-resolved username prefix, used as a
+ // safer fallback than the static default on transient registry errors
+ usernamePrefixMu sync.RWMutex
+ lastKnownUsernamePrefix string
}
func (s *BridgeUtils) matrixUsernamePrefix() string {
data, err := s.kvstore.Get(kvstore.KeyServersConfig)
if err != nil {
s.logger.LogError("Failed to read server registry for username prefix; using last known value", "server_id", s.serverID, "error", err)
- return DefaultMatrixUsernamePrefix
+ return s.fallbackUsernamePrefix()
}
servers, err := kvstore.ParseServersConfig(data)
if err != nil {
s.logger.LogError("Corrupt server registry; using last known username prefix", "server_id", s.serverID, "error", err)
- return DefaultMatrixUsernamePrefix
+ return s.fallbackUsernamePrefix()
}
if server, ok := kvstore.ServerConfigForID(servers, s.serverID); ok && server.UsernamePrefix != "" {
+ s.usernamePrefixMu.Lock()
+ s.lastKnownUsernamePrefix = server.UsernamePrefix
+ s.usernamePrefixMu.Unlock()
return server.UsernamePrefix
}
- return DefaultMatrixUsernamePrefix
+ return s.fallbackUsernamePrefix()
+}
+
+func (s *BridgeUtils) fallbackUsernamePrefix() string {
+ s.usernamePrefixMu.RLock()
+ defer s.usernamePrefixMu.RUnlock()
+ if s.lastKnownUsernamePrefix != "" {
+ return s.lastKnownUsernamePrefix
+ }
+ return DefaultMatrixUsernamePrefix
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/bridge_utils.go` around lines 161 - 188, Update
BridgeUtils.matrixUsernamePrefix to cache the last successfully resolved
username prefix per serverID and return that cached value when the registry read
or ParseServersConfig fails. Preserve the static DefaultMatrixUsernamePrefix
only when no successful resolution has ever occurred, while continuing to update
the cache whenever a valid server prefix is found.
| if delErr := c.kvstore.Delete(channelMappingKey); delErr != nil { | ||
| c.client.Log.Error("Failed to clear corrupt channel mapping", "error", delErr, "channel_id", args.ChannelId) | ||
| } | ||
| return &model.CommandResponse{ | ||
| ResponseType: model.CommandResponseTypeEphemeral, | ||
| Text: fmt.Sprintf("⚠️ **Corrupt Mapping Cleared**\n\nChannel `%s` had an unreadable Matrix room mapping, which has been removed. Use `/matrix map` to remap it if needed.", channelName), | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Report failure when the corrupt record cannot be deleted.
Line 522 logs a deletion failure, but Lines 525-528 still report that the mapping was cleared. Return an error response instead; the corrupt value remains and subsequent /matrix unmap calls will repeat this state.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/command/command.go` around lines 522 - 528, Update the corrupt channel
mapping cleanup branch around kvstore.Delete so a deletion failure returns an
error response instead of the “Corrupt Mapping Cleared” success response.
Preserve the existing success response only when Delete succeeds, and use the
existing command error-response pattern and symbols in this handler.
| // runMigrationToVersion3WithResults migrates to version 3: namespaces every | ||
| // per-server KV key by the single server's serverID and converts each | ||
| // channel_mapping_ value from a bare room ID string into a []ChannelServerMapping | ||
| // JSON array. It is deterministic and idempotent: keys already namespaced and | ||
| // values already converted are skipped, so a direct re-run is a no-op. | ||
| func (p *Plugin) runMigrationToVersion3WithResults() (*MigrationResult, error) { | ||
| p.logger.LogInfo("Running migration to version 3: namespacing keys by serverID") | ||
|
|
||
| // The server registry (and thus the serverID) is established by | ||
| // reconcileServerConfig during initMatrixClient, which runs before | ||
| // migrations. Reconcile again defensively in case migrations run first. | ||
| serverID := p.getSingleServerID() | ||
| if serverID == "" { | ||
| if _, err := p.reconcileServerConfig(); err != nil { | ||
| return nil, errors.Wrap(err, "failed to establish server registry for v3 migration") | ||
| } | ||
| serverID = p.getSingleServerID() | ||
| } | ||
| if serverID == "" { | ||
| // No Matrix server is configured yet (e.g. a fresh install with sync | ||
| // disabled and no server URL). There are no per-server keys to namespace, | ||
| // so the v3 layout is trivially satisfied. The registry entry and its | ||
| // namespaced keys are created later, once a server URL is configured and | ||
| // reconcileServerConfig derives the serverID. | ||
| p.logger.LogInfo("v3 migration: no Matrix server configured; nothing to namespace") | ||
| return &MigrationResult{}, nil | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Skip-on-empty-serverID assumes no legacy data exists — that's unverified.
The early return at lines 518-526 assumes that an empty serverID implies there are no per-server keys to namespace ("fresh install"). But an empty serverID can also occur on an upgrading install whose Matrix server URL has been temporarily cleared (e.g., admin disables the config during the upgrade window) while legacy v1/v2 un-namespaced keys still exist in the KV store. In that case this function returns an empty MigrationResult with no error, the caller (runKVStoreMigrationsWithResults) advances the version marker to 3 anyway, and — per the documented invariant at lines 481-483 that "v3 is the SOLE authority" for namespacing — those legacy keys are never namespaced again. Runtime code using the serverID-scoped key builders would then never find that data.
Consider verifying there truly is no legacy data (e.g., checking whether any v3NamespacedPrefixes or channel_mapping_ keys exist) before treating the missing-serverID case as a no-op, or otherwise returning an error so the version marker is not advanced until a server is configured.
💡 Sketch of a safer guard
if serverID == "" {
- // No Matrix server is configured yet (e.g. a fresh install with sync
- // disabled and no server URL). There are no per-server keys to namespace,
- // so the v3 layout is trivially satisfied. The registry entry and its
- // namespaced keys are created later, once a server URL is configured and
- // reconcileServerConfig derives the serverID.
- p.logger.LogInfo("v3 migration: no Matrix server configured; nothing to namespace")
- return &MigrationResult{}, nil
+ if hasLegacy, err := p.hasAnyV3NamespacedLegacyData(); err != nil {
+ return nil, errors.Wrap(err, "failed to check for legacy per-server data")
+ } else if hasLegacy {
+ // Legacy un-namespaced data exists but no server is configured to
+ // attribute it to; fail so the version marker doesn't advance and this
+ // retries once a server URL is configured.
+ return nil, errors.New("legacy per-server KV data exists but no Matrix server is configured")
+ }
+ p.logger.LogInfo("v3 migration: no Matrix server configured; nothing to namespace")
+ return &MigrationResult{}, nil
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/migrations.go` around lines 500 - 526, Update
runMigrationToVersion3WithResults so an empty serverID is not automatically
treated as a successful no-op: verify the KV store contains no legacy per-server
keys, including v3NamespacedPrefixes and channel_mapping_ entries, before
returning an empty MigrationResult. If legacy data exists without a configured
server, return an error so runKVStoreMigrationsWithResults does not advance the
version marker; retain the current no-op only when no legacy data is present.
| // registerForSharedChannels assigns p.remoteID, but the earlier | ||
| // initMatrixClient built the clients (and reconciled the registry) before it | ||
| // was known. Reinitialize so both the registry's RemoteID and every Matrix | ||
| // client carry the assigned remote ID instead of the initial empty value. | ||
| if err := p.initMatrixClient(); err != nil { | ||
| p.logger.LogWarn("Failed to reinitialize Matrix clients with remote ID", "error", err) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Do not activate with clients carrying the pre-registration remote ID.
If this reinitialisation fails, activation continues with clients created before p.remoteID was assigned. Those clients retain the empty metadata remote ID, so subsequent sync events can no longer be reliably attributed for loop prevention. Return the error here (or otherwise prevent sync until a successful rebuild).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/plugin.go` around lines 123 - 129, Update the reinitialization call to
p.initMatrixClient in the activation flow so any error is returned immediately
instead of merely logged, preventing activation and subsequent sync with clients
carrying the pre-registration remote ID. Preserve the successful initialization
path and existing error context while ensuring activation cannot continue until
the clients are rebuilt successfully.
| func (p *Plugin) reconcileServerConfig() ([]kvstore.ServerConfig, error) { | ||
| config := p.getConfiguration() | ||
|
|
||
| existing, err := p.getServers() | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| // No server URL configured yet (e.g. the plugin is enabled with sync off). | ||
| // There is nothing to register: leave any existing registry untouched and | ||
| // report it as-is rather than writing a useless entry or failing activation. | ||
| // A serverID cannot be derived without a URL, and none is needed yet. | ||
| if config.MatrixServerURL == "" { | ||
| return existing, nil | ||
| } | ||
|
|
||
| // The serverID is derived deterministically from the homeserver hostname, so | ||
| // a server re-created with the same URL re-adopts its namespaced KV records | ||
| // instead of orphaning them. We always derive rather than reuse the stored | ||
| // ID: that determinism is what makes the recovery possible. | ||
| serverID, err := deriveServerID(config.MatrixServerURL) | ||
| if err != nil { | ||
| return nil, err | ||
| } | ||
|
|
||
| persistedRemoteID := "" | ||
| if len(existing) > 0 { | ||
| persistedRemoteID = existing[0].RemoteID | ||
| if existing[0].ServerID != serverID { | ||
| p.logger.LogWarn("Matrix homeserver hostname changed; KV records under the previous serverID are now orphaned and can be recovered by reverting the server URL", | ||
| "previous_server_id", existing[0].ServerID, "new_server_id", serverID) | ||
| } | ||
| } | ||
|
|
||
| // reconcileServerConfig runs during initMatrixClient before | ||
| // registerForSharedChannels has assigned p.remoteID. Preserve the already | ||
| // persisted RemoteID in that window so an early reconcile (or a failed | ||
| // registration) never erases a valid remote identity. | ||
| remoteID := p.remoteID | ||
| if remoteID == "" { | ||
| remoteID = persistedRemoteID | ||
| } | ||
|
|
||
| entry := kvstore.ServerConfig{ | ||
| ServerID: serverID, | ||
| ServerURL: config.MatrixServerURL, | ||
| ServerName: config.MatrixServerName, | ||
| ASToken: config.MatrixASToken, | ||
| HSToken: config.MatrixHSToken, | ||
| UsernamePrefix: config.GetMatrixUsernamePrefix(), | ||
| Enabled: config.EnableSync, | ||
| RemoteID: remoteID, | ||
| } | ||
| servers := []kvstore.ServerConfig{entry} | ||
|
|
||
| data, err := json.Marshal(servers) | ||
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to marshal servers_config") | ||
| } | ||
| if err := p.kvstore.Set(kvstore.KeyServersConfig, data); err != nil { | ||
| return nil, errors.Wrap(err, "failed to persist servers_config") | ||
| } | ||
|
|
||
| p.matrixClientsLock.Lock() | ||
| p.serverID = serverID | ||
| p.matrixClientsLock.Unlock() | ||
|
|
||
| return servers, nil | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate all call sites of reconcileServerConfig to check for concurrent invocation.
rg -n -B3 -A3 '\breconcileServerConfig\s*\(' --type=goRepository: mattermost/mattermost-plugin-matrix-bridge
Length of output: 180
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Files containing reconcileServerConfig =="
rg -n "reconcileServerConfig" --type=go .
echo
echo "== Files containing OnConfigurationChange / configuration handlers =="
rg -n "OnConfigurationChange|configuration change|Init\(|initMatrixClient|registerForSharedChannels|getServers\(" --type=go .
echo
echo "== server/servers.go outline =="
ast-grep outline server/servers.go --view expandedRepository: mattermost/mattermost-plugin-matrix-bridge
Length of output: 5164
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server/plugin.go (relevant slices) =="
cat -n server/plugin.go | sed -n '90,260p'
echo
echo "== server/configuration.go (OnConfigurationChange) =="
cat -n server/configuration.go | sed -n '70,130p'
echo
echo "== server/migrations.go (call site context) =="
cat -n server/migrations.go | sed -n '500,535p'
echo
echo "== server/servers.go (top and reconcile comment context) =="
cat -n server/servers.go | sed -n '1,170p'Repository: mattermost/mattermost-plugin-matrix-bridge
Length of output: 18929
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== Search for locks around configuration changes =="
rg -n "matrixClientsLock|configuration lock|OnConfigurationChange|initMatrixClient\(" server/*.go
echo
echo "== Any goroutine usage around initMatrixClient / reconcileServerConfig =="
rg -n "go .*initMatrixClient|go .*reconcileServerConfig|initMatrixClient\(" server/*.goRepository: mattermost/mattermost-plugin-matrix-bridge
Length of output: 2117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== server/plugin.go around initMatrixClient =="
cat -n server/plugin.go | sed -n '160,210p'
echo
echo "== server/configuration.go around OnConfigurationChange =="
cat -n server/configuration.go | sed -n '80,120p'Repository: mattermost/mattermost-plugin-matrix-bridge
Length of output: 4227
🌐 Web query:
Mattermost plugin OnConfigurationChange OnActivate concurrency serialized lifecycle callback contract
💡 Result:
In Mattermost plugin development, lifecycle callbacks and hook invocations operate under a model where the plugin must manage its own concurrency [1][2]. Lifecycle and Hook Concurrency: Mattermost server plugins are inherently concurrent [1][2]. Hooks (including lifecycle callbacks like OnConfigurationChange and OnActivate) are invoked asynchronously by the server as separate RPC calls [1][3]. There is no automatic serialization of these hooks by the Mattermost server [1][2]. Consequently, if your plugin stores state that is accessed or modified across these hooks, you must implement manual synchronization (e.g., using sync.RWMutex or similar primitives) to prevent race conditions or corrupted reads/writes [1][2]. Lifecycle Callbacks Order: The OnConfigurationChange hook is explicitly called before the OnActivate hook [4]. This design ensures that the plugin's configuration is already available and loaded when OnActivate is executed [4]. Because OnConfigurationChange can be invoked before the plugin is fully activated, your implementation should be prepared to handle configuration updates gracefully—for example, by using a flag to track activation status or by ensuring that any resources initialized in OnActivate are checked for existence before use in OnConfigurationChange [4]. Important Considerations: 1. Serialization: Since the server does not serialize these calls, you are responsible for thread-safe access to any shared state [1][2]. 2. Deadlocks: Avoid calling plugin API methods while holding your own internal locks, as the plugin API may trigger subsequent hooks that attempt to acquire the same lock, leading to a deadlock [2]. 3. Activation: Since plugins are activated concurrently with other plugins [5], do not assume a specific execution order relative to other plugins, even if the order within your own plugin's lifecycle is deterministic [4][5]. Top Results: [4] mattermost/mattermost#10597 [1] https://developers.mattermost.com/integrate/plugins/migration/ [5] mattermost/mattermost#15244 [2] https://github.com/mattermost/mattermost-plugin-starter-template/blob/master/server/configuration.go
Citations:
- 1: https://developers.mattermost.com/integrate/plugins/migration/
- 2: https://github.com/mattermost/mattermost-plugin-starter-template/blob/master/server/configuration.go
- 3: https://developers.mattermost.com/integrate/plugins/components/server/
- 4: MM-14788 Documenting OnConfigurationChange being called before OnActivate. mattermost#10597
- 5: MM-27570,MM-27757 - Activate/Deactivate plugins concurrently mattermost#15244
Serialize server registry reconciliation server/servers.go:77-145
reconcileServerConfig reads servers_config, derives the entry, and writes it back without a lock; the only lock here guards p.serverID. Because Mattermost can invoke OnConfigurationChange and OnActivate concurrently, two reconciles can interleave and drop a newer RemoteID or stale config into the KV record. Guard this path with the same shared mutex or another serialised initializer.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/servers.go` around lines 77 - 145, Serialize the entire
reconcileServerConfig operation, including reading existing servers, deriving
the entry, persisting servers_config, and updating p.serverID, using the shared
mutex or a dedicated initializer lock. Ensure concurrent OnConfigurationChange
and OnActivate calls cannot interleave or overwrite a newer RemoteID or
configuration.
| response, err := suite.plugin.GetMatrixClient().SendMessage(messageReq) | ||
| assert.NoError(t, err, "Should be able to send message as ghost user") | ||
| assert.NotEmpty(t, response.EventID, "Should receive event ID") | ||
|
|
||
| t.Logf("✓ Ghost user %s sent message with event ID: %s", ghostUserID, response.EventID) | ||
|
|
||
| // Test: Get the ghost user's profile to simulate what the bridge would do | ||
| profile, err := suite.plugin.matrixClient.GetUserProfile(ghostUserID) | ||
| profile, err := suite.plugin.GetMatrixClient().GetUserProfile(ghostUserID) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Stop before dereferencing a failed response.
assert.NoError continues after failure, so a failed SendMessage can leave response nil and panic at Line 215. Use require.NoError and assert the response is non-nil first.
Proposed fix
response, err := suite.plugin.GetMatrixClient().SendMessage(messageReq)
-assert.NoError(t, err, "Should be able to send message as ghost user")
+require.NoError(t, err, "Should be able to send message as ghost user")
+require.NotNil(t, response, "Should receive a response")
assert.NotEmpty(t, response.EventID, "Should receive event ID")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| response, err := suite.plugin.GetMatrixClient().SendMessage(messageReq) | |
| assert.NoError(t, err, "Should be able to send message as ghost user") | |
| assert.NotEmpty(t, response.EventID, "Should receive event ID") | |
| t.Logf("✓ Ghost user %s sent message with event ID: %s", ghostUserID, response.EventID) | |
| // Test: Get the ghost user's profile to simulate what the bridge would do | |
| profile, err := suite.plugin.matrixClient.GetUserProfile(ghostUserID) | |
| profile, err := suite.plugin.GetMatrixClient().GetUserProfile(ghostUserID) | |
| response, err := suite.plugin.GetMatrixClient().SendMessage(messageReq) | |
| require.NoError(t, err, "Should be able to send message as ghost user") | |
| require.NotNil(t, response, "Should receive a response") | |
| assert.NotEmpty(t, response.EventID, "Should receive event ID") | |
| t.Logf("✓ Ghost user %s sent message with event ID: %s", ghostUserID, response.EventID) | |
| // Test: Get the ghost user's profile to simulate what the bridge would do | |
| profile, err := suite.plugin.GetMatrixClient().GetUserProfile(ghostUserID) |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@server/user_remote_detection_test.go` around lines 213 - 220, In the
ghost-user message test, update the SendMessage assertions to use
require.NoError so execution stops when sending fails, then require or assert
that response is non-nil before accessing response.EventID. Keep the existing
EventID validation and logging behavior in the surrounding test.
| - `POST /servers` — add a server; **derive `serverID` deterministically from the URL | ||
| hostname** (`deriveServerID`, Phase 1); persist; (re)register shared-channels remote; | ||
| rebuild client registry. Because the ID is derived, a re-added server re-adopts its | ||
| orphaned KV records, and a duplicate URL resolves to an existing `serverID` — treat that as | ||
| an idempotent add/update, not a second entry. | ||
| - `PUT /servers/{serverID}` — edit URL/name/tokens/prefix/enabled. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve the hostname-derived serverID invariant on URL edits.
POST derives serverID from the URL hostname, but PUT allows changing the URL. Define this as either hostname-immutable or an atomic re-key/migration to the newly derived ID; otherwise a new homeserver can inherit the old server’s namespaced mappings.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@spec/2026-07-16-multi-matrix-server-support-phase-4.md` around lines 18 - 23,
Define the URL-edit behavior in the PUT /servers/{serverID} flow to preserve the
hostname-derived serverID invariant: either reject hostname changes as
immutable, or atomically re-key the server and migrate all namespaced records to
deriveServerID(new hostname). Ensure the update cannot leave the old ID
associated with a different hostname or allow the new homeserver to inherit its
mappings.
1 participant
Phase 1 of multi-Matrix-server support (MM-64622, epic MM-64621). Backend groundwork only — a single homeserver is now modeled as a one-element registry keyed by a stable
serverID. Zero behavior change for a single-server operator; no REST API, UI, routing, or slash-command changes yet.What changed
servers_configKV entry (ServerConfig) reconciled from the flatplugin.jsonconfig on every configuration change.plugin.jsonremains the source of truth this phase.serverID: derived from the homeserver hostname (base32(sha256(hostname)), 26-char, drop-in formodel.NewId()), so records are re-adopted if a server is re-created with the same URL.<prefix><serverID>_<id>, with new key builders instore/kvstore/constants.go. Fixed two hardcoded key literals that bypassed the constants.channel_mapping_<channelID>value becomes a[]ChannelServerMapping(server association now lives in the value; length 1 enforced for now).p.matrixClientbecomesmap[serverID]*matrix.ClientwithgetMatrixClient/getSingleServerIDaccessors;serverIDthreaded through all namespaced KV read/write sites.channel_mappingvalues. Deterministic, idempotent, and aborts without bumping the version on partial failure so it safely retries. Legacy v1/v2 steps stay un-namespaced.spec/; added Element client to docker-compose for local testing.Out of scope (later phases)
Cross-server routing, per-server shared-channels registration, REST API, admin UI, slash-command targeting, and server-deletion semantics.
Testing
make check-styleandmake testpass; webapp untouched.