Support multiple Matrix homeservers - #33
Closed
fmartingr wants to merge 8 commits into
Closed
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/.
Route inbound Application Service traffic to the correct homeserver by matching the presented hs_token against each server's HSToken, resolving a serverID, and scoping all lookups to that server's KV namespace. - api.go: MatrixAuthorizationRequired matches the bearer token against every server's hs_token (constant-time), injects the resolved serverID into the request context, and enforces the per-server Enabled flag on top of the global enable_sync switch. - matrix_webhook.go: dedup transactions per (serverID, txnID); thread serverID through processMatrixEvent, room-mapping lookup, ghost-user detection, and Matrix-initiated DM handling. - plugin.go/bridge_utils.go/sync_to_mattermost.go: construct inbound bridges scoped to the originating server; resolve the per-server domain for the matrix_event_id_<domain> post property. - kvstore: add SetAtomicWithRetries for concurrency-safe upserts of the shared channel_mapping value across servers. Also adds an admin-only /matrix server command group and backing registry mutation API for local multi-server testing until the System Console UI lands.
Route Mattermost->Matrix traffic to the homeserver(s) each channel is mapped to, and register the plugin once per Matrix server so loop prevention and sync cursors are per-remote. Completes the multi-server core (phases 1-3). Implementation: - Per-server shared-channels registration with a distinct SiteURL per server (primary keeps the legacy empty SiteURL for upgrade compatibility); store each returned remoteID on its registry entry; add remoteToServerID / ownRemoteIDs maps and remoteIDForServer / serverIDForRemoteID / isOwnRemoteID. - Outbound fan-out: resolveOutboundServers() + per-server newMattermostToMatrixBridge(serverID); posts/edits/deletes/reactions/files/ profile-images/DMs target the mapped server(s) only. - Per-server pending-file and post trackers (keyed by (serverID, postID)). - Set-based loop prevention (isOwnRemoteID) across N servers. - Server-aware username round-trip (reconstruct from the registry per server). - /matrix map refuses when >1 server is registered (use /matrix server map); /matrix server add registers the added server immediately. - Bump min_server_version to 11.8.0 (SiteURL per-remote registration). Bugs fixed (each a single-server assumption leaking into a per-server path, surfaced by the multi-server integration tests): - matrix_event_id_<domain> post-property key resolved per server from the registry (ServerName), not the flat-config URL host - fixes collisions between homeservers sharing a host but differing by port. - serverDomainForID prefers the homeserver ServerName for ghost recognition, so inbound loop prevention recognizes our own ghosts under delegation. - Inbound bridge attributes posts/users to the originating server's remote (remoteIDForServer), not the primary. - SiteURL keyed by homeserver hostname so servers sharing a ServerName do not collapse onto one remote. Tests: - Multi-server integration suites for outbound and inbound, per event type (message / edit / delete / reaction add+remove / file / profile / loop / DM), each asserting delivery and attribution on both servers. - Shared-channel suites (one channel bridged to a room on each server), both directions, asserting per-server remote / user / ghost attribution. - Unit tests for outbound routing resolution, remote-ID maps, ghost-domain recognition, and the /matrix map guard. Docs: local-development multi-server section; server autocomplete lists "map".
The System Console can only configure a single homeserver, which blocks running multiple Matrix servers. Move server configuration (add, remove, list, status, map, unmap, registration) to admin-only /matrix server slash commands backed by the server registry, drop the now-redundant flat matrix_server_* settings from plugin.json, make shared-channels ping health-check the specific remote's owning server, and simplify the v3 migration to seed the registry directly from legacy config.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThis PR changes the bridge from a single Matrix homeserver configuration to managed multi-homeserver support. It adds server-scoped KV data, routing, authentication, commands, migrations, synchronized state, Docker services, documentation, and extensive unit and integration tests. ChangesMulti-Matrix server support
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: 16
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
server/command/command.go (2)
926-948: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick wingolangci-lint (staticcheck QF1012) fails on these three lines.
Use
fmt.Fprintf(&responseText, ...)instead ofresponseText.WriteString(fmt.Sprintf(...)).🔧 Proposed fix
- responseText.WriteString(fmt.Sprintf("**Current Channel:** %s → %s\n\n", channelName, renderRooms(current))) + fmt.Fprintf(&responseText, "**Current Channel:** %s → %s\n\n", channelName, renderRooms(current)) @@ - responseText.WriteString(fmt.Sprintf("**All Mappings (%d channels):**\n", len(mappings))) + fmt.Fprintf(&responseText, "**All Mappings (%d channels):**\n", len(mappings)) @@ - responseText.WriteString(fmt.Sprintf("• %s → %s%s\n", channelName, renderRooms(entries), currentMarker)) + fmt.Fprintf(&responseText, "• %s → %s%s\n", channelName, renderRooms(entries), currentMarker)🤖 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 926 - 948, Replace the three responseText.WriteString(fmt.Sprintf(...)) calls in the channel-mapping rendering flow with fmt.Fprintf(&responseText, ...) calls, preserving each existing format string and arguments, including the current-channel and all-mappings output.Source: Linters/SAST tools
485-513: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRead-modify-write on the shared channel-mapping key isn't atomic.
Get→Upsert→Setcan lose an entry if two/matrix server mapinvocations for different servers target the same channel concurrently.KVStoreexposesSetAtomicWithRetriesprecisely for this shared-key pattern (seeserver/store/kvstore/kvstore.go); the same applies to the unmap path at Lines 688-691.♻️ Sketch
- channelMappings, err := kvstore.ParseChannelServerMappings(existingMapping) - ... - channelMappings = kvstore.UpsertChannelServerMapping(channelMappings, serverID, roomIdentifier) - mappingValue, err := kvstore.MarshalChannelServerMappings(channelMappings) - if err == nil { - err = c.kvstore.Set(mappingKey, mappingValue) - } + err := c.kvstore.SetAtomicWithRetries(mappingKey, func(old []byte) ([]byte, error) { + existing, perr := kvstore.ParseChannelServerMappings(old) + if perr != nil { + return nil, perr + } + return kvstore.MarshalChannelServerMappings( + kvstore.UpsertChannelServerMapping(existing, serverID, roomIdentifier)) + })🤖 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 485 - 513, Replace the non-atomic Get/Upsert/Set sequence in the channel-mapping flow with KVStore.SetAtomicWithRetries, preserving UpsertChannelServerMapping and the existing error handling. Apply the same atomic read-modify-write approach to the corresponding unmap path around its channel-mapping update, so concurrent mappings cannot overwrite one another.
🧹 Nitpick comments (9)
server/command/command.go (1)
546-582: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueSame alias is resolved up to three times.
ResolveRoomAlias(roomIdentifier)is called at Lines 521, 556 and 578 for the same identifier. Resolve once up front and reuse the result for the reverse mapping, the bridge alias and the member sync.🤖 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 546 - 582, Resolve roomIdentifier once before the reverse-mapping, bridge-alias, and member-sync logic, then reuse the resolved room ID across all three paths. Update the calls in the surrounding command flow, including syncChannelMembersToMatrixRoom setup, to avoid repeated ResolveRoomAlias invocations while preserving fallback behavior when resolution fails.server/dm_room_creation_test.go (1)
108-131: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract the repeated per-server test setup into a shared helper.
The
setTestMatrixClient+SetServerDomain+plugin.configuration = &configuration{}+seedServerEntry(...)+ reverse-mappingkvstore.Set(kvstore.BuildMattermostUserKey(...))block is duplicated verbatim across all three test cases (lines 108-131, 252-275, 380-403). Any future change to the seeding contract (e.g., a new requiredServerConfigfield) needs to be updated in three places.♻️ Suggested helper extraction
func seedDMTestPlugin(t *testing.T, plugin *Plugin, container *matrixtest.Container, matrixUserID, matrixID string) { setTestMatrixClient(plugin, createMatrixClientWithTestLogger(t, container.ServerURL, container.ASToken, testRemoteID)) plugin.GetMatrixClient().SetServerDomain(container.ServerDomain) plugin.configuration = &configuration{} seedServerEntry(plugin, kvstore.ServerConfig{ ServerID: testServerID, ServerURL: container.ServerURL, ServerName: container.ServerDomain, ASToken: container.ASToken, HSToken: container.HSToken, Enabled: true, RemoteID: testRemoteID, }) err := plugin.kvstore.Set(kvstore.BuildMattermostUserKey(testServerID, matrixUserID), []byte(matrixID)) require.NoError(t, err) }Also applies to: 252-275, 380-403
🤖 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/dm_room_creation_test.go` around lines 108 - 131, The per-server setup is duplicated across the DM room creation tests. Add a shared seedDMTestPlugin helper that performs the Matrix client setup, server-domain configuration, plugin configuration initialization, server entry seeding, and reverse user mapping with require.NoError; replace the duplicated setup blocks in all three test cases with calls to this helper.server/plugin.go (2)
300-310: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
remoteIDForServerperforms a KV read on every bridge construction.Both bridge constructors call this, and bridges are built per inbound event and per outbound fan-out target, so each event now incurs a registry read plus JSON decode. The remote IDs are already held in memory (
remoteToServerID); consider maintaining the inverseserverID → remoteIDmap ininitMatrixClientsand reading it undermatrixClientsLock, falling back to the registry only on a miss. Note the current form also silently swallows read errors.🤖 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 300 - 310, Update remoteIDForServer to consult an in-memory serverID-to-remoteID map maintained by initMatrixClients under matrixClientsLock, avoiding KV reads for known servers; retain a registry lookup only when the map misses, and propagate or otherwise handle registry read errors instead of silently ignoring them.
242-246: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueStale comment:
p.remoteIDno longer exists.The struct no longer carries a single
remoteIDfield, so the "Falls back top.remoteIDin the single-server case" note is misleading —remoteIDForServerreturns""when the server is unknown. Same for the "Rebuilt from the registry ininitMatrixClient" comments on lines 46 and 51 and line 168 (the function isinitMatrixClients).🤖 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 242 - 246, Update the comments around RemoteID and the initMatrixClients setup to remove references to the deleted p.remoteID field and the singular initMatrixClient function. Document that remoteIDForServer returns an empty string for unknown servers, and use the correct initMatrixClients symbol wherever referenced.server/post_tracker_test.go (1)
9-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test that proves the new per-server namespacing.
All existing cases use a single
testTrackerServer, so they would still pass iftrackerKeyignoredserverID. A two-server case pins the actual contract this change introduces.🧪 Suggested additional test
func TestPostTracker_ServerIsolation(t *testing.T) { tracker := NewPostTracker(DefaultPostTrackerMaxEntries) const postID = "shared_post" require.NoError(t, tracker.Put("srv1", postID, 100)) require.NoError(t, tracker.Put("srv2", postID, 200)) got1, ok1 := tracker.Get("srv1", postID) got2, ok2 := tracker.Get("srv2", postID) require.True(t, ok1) require.True(t, ok2) require.Equal(t, int64(100), got1) require.Equal(t, int64(200), got2) tracker.Delete("srv1", postID) _, ok1 = tracker.Get("srv1", postID) _, ok2 = tracker.Get("srv2", postID) require.False(t, ok1) require.True(t, ok2, "deleting one server's entry must not affect another's") }🤖 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/post_tracker_test.go` around lines 9 - 31, Add a TestPostTracker_ServerIsolation test covering identical post IDs on two distinct servers. Verify Put/Get retain independent timestamps for each server, then confirm deleting the entry for one server removes only that server’s entry while the other remains available.server/sync_to_matrix.go (1)
309-313: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winCache
serverDomain()for these event paths. It reads and parses the server registry on every call, so the sync/delete/reaction handlers are paying that cost repeatedly. If the domain is stable per bridge, keep it onBridgeUtilsand reuse it.🤖 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/sync_to_matrix.go` around lines 309 - 313, Cache the stable server domain on BridgeUtils instead of invoking serverDomain() repeatedly in the sync, delete, and reaction event paths. Initialize the cached value when the bridge is created, then update the relevant handlers to reuse that field when constructing matrix_event_id_ property keys.server/bridge_utils.go (2)
158-222: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated registry-lookup logic across
matrixUsernamePrefix,serverDomain, andreconstructMatrixUserIDFromUsername.
matrixUsernamePrefix(167-179) andserverDomain(201-210) each independently dokvstore.Get(KeyServersConfig)→ParseServersConfig→ServerConfigForID, andreconstructMatrixUserIDFromUsername(line 558-565 below) repeats the same three-step lookup a third time with different error handling. Extracting a single helper would remove the duplication and, as a side effect, unify error visibility (see the note on line 555-570) and give a single place to add caching later if this read-per-call pattern (documented as intentional for freshness) ever needs to trade some freshness for reduced KV I/O on the per-postserverDomain()path.♻️ Suggested consolidation
+// getServerConfigEntry centralizes the KV read + parse + lookup shared by +// matrixUsernamePrefix, serverDomain, and reconstructMatrixUserIDFromUsername. +func (s *BridgeUtils) getServerConfigEntry() (kvstore.ServerConfig, bool) { + data, err := s.kvstore.Get(kvstore.KeyServersConfig) + if err != nil { + s.logger.LogError("Failed to read server registry", "server_id", s.serverID, "error", err) + return kvstore.ServerConfig{}, false + } + servers, err := kvstore.ParseServersConfig(data) + if err != nil { + s.logger.LogError("Corrupt server registry", "server_id", s.serverID, "error", err) + return kvstore.ServerConfig{}, false + } + return kvstore.ServerConfigForID(servers, s.serverID) +}🤖 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 158 - 222, Extract the shared KeyServersConfig read, ParseServersConfig call, and ServerConfigForID lookup into a BridgeUtils helper that resolves the current server configuration for s.serverID and centralizes error logging. Update matrixUsernamePrefix, serverDomain, and reconstructMatrixUserIDFromUsername to use this helper while preserving each method’s existing fallback behavior and live-read semantics.
555-570: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRegistry lookup errors are silently swallowed here, unlike the sibling helpers.
matrixUsernamePrefixandserverDomainboth explicitlyLogErroron a KV read failure vs. a parse failure vs. a missing entry. Here,regErr/parseErrare dropped on the floor (theif ... err == nilguards just skip the block), so a transient KV failure and a genuinely-missing registry entry both collapse into the same generic "No registry entry for server" warning below (line 567-568). This makes it harder to distinguish a transient backend issue from a real configuration gap when debugging. Rolling this into the shared helper suggested at lines 158-222 fixes it for free.🤖 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 555 - 570, Update the registry lookup in the reconstruction flow to reuse the shared helper used by matrixUsernamePrefix and serverDomain, such as the helper covering lines 158-222, so KV read failures, parse failures, and missing entries are logged distinctly. Preserve the existing serverURL/configuredServerName assignment and generic warning behavior only for a genuinely unavailable registry entry.server/servers_test.go (1)
142-145: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDoc comment describes the wrong function.
This block documents
TestDeriveServerIDbut sits onTestManagedServers; the actualTestDeriveServerIDat Line 335 is undocumented.♻️ Move the comment
-// TestDeriveServerID verifies the deterministic serverID derivation: same -// hostname (regardless of scheme/port/path/case) yields the same 26-char base32 -// ID, distinct hostnames yield distinct IDs, and an unusable URL errors. +// TestManagedServers covers the registry lifecycle: add/upsert, per-server +// SiteURL and remote assignment, and permanent removal. func TestManagedServers(t *testing.T) {🤖 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_test.go` around lines 142 - 145, Move the existing deterministic server ID documentation from above TestManagedServers to immediately above TestDeriveServerID. Update the TestManagedServers comment as needed so it documents that test, while keeping the TestDeriveServerID behavior description with its actual function.
🤖 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 `@docker/mattermost-bridge-registration2.yaml`:
- Around line 2-6: Update the url setting in the Mattermost bridge registration
to use a portable address reachable by the local Docker Compose network, such as
the synapse2 service/container hostname, instead of the personal Tailscale
hostname. Keep the existing plugin path and token values unchanged.
In `@docs/local-development.md`:
- Around line 94-96: Update the three fenced command examples in the local
development documentation to specify an explicit language identifier, such as
text, on each opening fence, including the examples around the visible /matrix
server command and the referenced sections.
- Around line 17-20: Update the local-development setup steps to remove the
obsolete Matrix Server URL and registration-file generation instructions.
Document the supported /matrix server add and /matrix server registration
commands using the plugin’s server-management flow for both the primary server
and the second server, including copying each generated registration file to the
expected Docker configuration location.
In `@server/command/command.go`:
- Around line 701-706: The unmap flow around the roomMappingKey deletion must
remove both reverse mappings created by mapChannelToRoom for alias-based
mappings. Resolve the stored alias to its room ID, then delete the alias-keyed
and resolved-room-ID-keyed BuildRoomMappingKey entries before returning success,
while preserving the existing warning behavior for deletion failures.
- Around line 1454-1462: Remove the "matrix.org" fallback in
buildRegistrationYAML and propagate the failure when both ServerName and
ExtractServerDomain(server.ServerURL) are unusable. Update
executeServerRegistrationCommand to detect this condition and return an
ephemeral error instead of emitting a misleading registration YAML file.
In `@server/command/server_command_test.go`:
- Around line 273-281: Add a targeted gosec suppression for the fixture token
values in TestBuildRegistrationYAML, applying //nolint:gosec with the
test-fixture explanation to the kvstore.ServerConfig literal. Keep the test
behavior and token values unchanged.
In `@server/hooks.go`:
- Around line 326-330: Update the GetMatrixRoomID error branch in the remote
user invite flow to log the returned error at an error-appropriate level and
preserve it as a failure instead of treating it as an unmapped channel. Keep the
existing matrixRoomID == "" handling for unmapped channels, since
GetMatrixRoomID returns no error in that case.
- Around line 190-215: Update the per-server upload loop in
OnSharedChannelsAttachmentSyncMsg to track UploadMedia failures instead of
silently continuing: retain the existing per-server logging and processing for
successful uploads, but return a non-nil error after processing targets when at
least one server failed so shared-channel synchronization retries and does not
post without the attachment.
In `@server/matrix_mentions_integration_test.go`:
- Around line 97-98: Handle errors in both channel-mapping setup blocks at
server/matrix_mentions_integration_test.go lines 97-98 and 298-299: capture the
error from BuildSingleChannelMapping and assert it with require.NoError(t, err),
then assert the kvstore.Set result with require.NoError. You may extract a
shared mapChannelToRoom helper, but both sites must use equivalent error-checked
behavior.
In `@server/multi_server_integration_test.go`:
- Around line 52-61: Suppress gosec G101 for the test-only ASToken and HSToken
literals in the MatrixTestConfig initializations within the multi-server
integration test, using a targeted nolint directive with the stated test-only
rationale; do not broaden the global lint policy.
In `@server/servers.go`:
- Around line 180-189: Update Plugin.persistServers to persist servers_config
through kvstore.KVStore.SetAtomicWithRetries instead of kvstore.KVStore.Set,
preserving the existing JSON marshaling and error-wrapping behavior. Ensure the
atomic operation writes the marshaled data and retains the failed-persistence
context.
In `@spec/2026-07-16-multi-matrix-server-support-phase-2.md`:
- Around line 48-51: Update the test requirement wording in the multi-matrix
server support specification by replacing “Unknown//invalid token” with
“Unknown/invalid token,” without changing the surrounding behavior or
assertions.
- Around line 17-20: Update the server registry write paths for `/matrix server
add`, updates, and migrations to validate that `hs_token` is non-empty and
unique across all registered servers before persistence. Reject duplicate or
empty tokens consistently, while preserving existing valid writes and
token-based `serverID` resolution.
In `@spec/2026-07-16-multi-matrix-server-support-phase-3.md`:
- Around line 45-49: Update the min_server_version section to reflect
plugin.json’s current minimum of 11.8.0, and document the confirmed Mattermost
version/reason that supports SiteURL on RegisterPluginOpts for per-remote
registration. Remove the stale 10.7.1 value so the specification and manifest
remain consistent.
In `@spec/2026-07-16-multi-matrix-server-support-phase-4.md`:
- Around line 42-45: Resolve the server-removal policy in the phase-4
specification before exposing DELETE: choose either cleanup or orphan-marking,
define behavior for retries and idempotent repeated deletion, and specify the
exact namespaced KV keys and shared-channel state affected. Add tests covering
the selected cleanup set and re-adding a server with mapped channels.
In `@spec/2026-07-16-multi-matrix-server-support-phase-6.md`:
- Around line 22-24: Define one canonical `/matrix server` command contract and
apply it consistently across the specification, local-development guide, and
plugin administrator help. Update the specification at
spec/2026-07-16-multi-matrix-server-support-phase-6.md lines 22-24,
docs/local-development.md lines 98-101, and plugin.json line 22 to use the exact
syntax and list all supported subcommands, including `unmap` and `status`;
ensure autocomplete and tests follow the same contract.
---
Outside diff comments:
In `@server/command/command.go`:
- Around line 926-948: Replace the three
responseText.WriteString(fmt.Sprintf(...)) calls in the channel-mapping
rendering flow with fmt.Fprintf(&responseText, ...) calls, preserving each
existing format string and arguments, including the current-channel and
all-mappings output.
- Around line 485-513: Replace the non-atomic Get/Upsert/Set sequence in the
channel-mapping flow with KVStore.SetAtomicWithRetries, preserving
UpsertChannelServerMapping and the existing error handling. Apply the same
atomic read-modify-write approach to the corresponding unmap path around its
channel-mapping update, so concurrent mappings cannot overwrite one another.
---
Nitpick comments:
In `@server/bridge_utils.go`:
- Around line 158-222: Extract the shared KeyServersConfig read,
ParseServersConfig call, and ServerConfigForID lookup into a BridgeUtils helper
that resolves the current server configuration for s.serverID and centralizes
error logging. Update matrixUsernamePrefix, serverDomain, and
reconstructMatrixUserIDFromUsername to use this helper while preserving each
method’s existing fallback behavior and live-read semantics.
- Around line 555-570: Update the registry lookup in the reconstruction flow to
reuse the shared helper used by matrixUsernamePrefix and serverDomain, such as
the helper covering lines 158-222, so KV read failures, parse failures, and
missing entries are logged distinctly. Preserve the existing
serverURL/configuredServerName assignment and generic warning behavior only for
a genuinely unavailable registry entry.
In `@server/command/command.go`:
- Around line 546-582: Resolve roomIdentifier once before the reverse-mapping,
bridge-alias, and member-sync logic, then reuse the resolved room ID across all
three paths. Update the calls in the surrounding command flow, including
syncChannelMembersToMatrixRoom setup, to avoid repeated ResolveRoomAlias
invocations while preserving fallback behavior when resolution fails.
In `@server/dm_room_creation_test.go`:
- Around line 108-131: The per-server setup is duplicated across the DM room
creation tests. Add a shared seedDMTestPlugin helper that performs the Matrix
client setup, server-domain configuration, plugin configuration initialization,
server entry seeding, and reverse user mapping with require.NoError; replace the
duplicated setup blocks in all three test cases with calls to this helper.
In `@server/plugin.go`:
- Around line 300-310: Update remoteIDForServer to consult an in-memory
serverID-to-remoteID map maintained by initMatrixClients under
matrixClientsLock, avoiding KV reads for known servers; retain a registry lookup
only when the map misses, and propagate or otherwise handle registry read errors
instead of silently ignoring them.
- Around line 242-246: Update the comments around RemoteID and the
initMatrixClients setup to remove references to the deleted p.remoteID field and
the singular initMatrixClient function. Document that remoteIDForServer returns
an empty string for unknown servers, and use the correct initMatrixClients
symbol wherever referenced.
In `@server/post_tracker_test.go`:
- Around line 9-31: Add a TestPostTracker_ServerIsolation test covering
identical post IDs on two distinct servers. Verify Put/Get retain independent
timestamps for each server, then confirm deleting the entry for one server
removes only that server’s entry while the other remains available.
In `@server/servers_test.go`:
- Around line 142-145: Move the existing deterministic server ID documentation
from above TestManagedServers to immediately above TestDeriveServerID. Update
the TestManagedServers comment as needed so it documents that test, while
keeping the TestDeriveServerID behavior description with its actual function.
In `@server/sync_to_matrix.go`:
- Around line 309-313: Cache the stable server domain on BridgeUtils instead of
invoking serverDomain() repeatedly in the sync, delete, and reaction event
paths. Initialize the cached value when the bridge is created, then update the
relevant handlers to reuse that field when constructing matrix_event_id_
property keys.
🪄 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: 2dc698f9-8987-404e-90b5-3e7780bce541
⛔ Files ignored due to path filters (1)
webapp/package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (58)
README.mddocker-compose.ymldocker/element-config.jsondocker/element2-config.jsondocker/mattermost-bridge-registration2.yamldocker/synapse2_config.yamldocs/local-development.mdplugin.jsonserver/api.goserver/api_test.goserver/bridge_utils.goserver/command/command.goserver/command/command_test.goserver/command/server_command_test.goserver/command/slash_command_test.goserver/configuration.goserver/dm_room_creation_test.goserver/dm_support_test.goserver/hooks.goserver/hooks_ping_test.goserver/matrix_mentions_integration_test.goserver/matrix_util.goserver/matrix_webhook.goserver/matrix_webhook_test.goserver/migrations.goserver/migrations_test.goserver/mocks/mock_kvstore.goserver/multi_server_inbound_integration_test.goserver/multi_server_inbound_same_channel_integration_test.goserver/multi_server_integration_test.goserver/multi_server_outbound_integration_test.goserver/multi_server_outbound_same_channel_integration_test.goserver/outbound_routing_test.goserver/plugin.goserver/plugin_integration_test.goserver/post_tracker.goserver/post_tracker_test.goserver/servers.goserver/servers_test.goserver/store/kvstore/constants.goserver/store/kvstore/kvstore.goserver/store/kvstore/schema.goserver/store/kvstore/schema_test.goserver/store/kvstore/startertemplate.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
Comment on lines
+17
to
+20
| An AS appends the fixed suffix `/_matrix/app/v1/transactions/{txnId}` to the registration | ||
| `url`, so a discriminator cannot sit mid-path. **Give each server a unique `hs_token`** and | ||
| match the presented bearer token against every server's token to resolve `serverID` — zero | ||
| URL restructuring. (Fallback if needed: put `serverID` as the first path segment via the |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Enforce unique, non-empty hs_token values at registry write time.
Token-only routing is ambiguous if /matrix server add, update, or migration accepts duplicates or empty values. Reject those inputs before persistence; otherwise events can be routed into the wrong server namespace.
🤖 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-2.md` around lines 17 - 20,
Update the server registry write paths for `/matrix server add`, updates, and
migrations to validate that `hs_token` is non-empty and unique across all
registered servers before persistence. Reject duplicate or empty tokens
consistently, while preserving existing valid writes and token-based `serverID`
resolution.
Comment on lines
+48
to
+51
| - Two-server unit tests: transactions with distinct `hs_token`s route to distinct | ||
| namespaces; colliding `txnID`s across servers are NOT deduped against each other. | ||
| - Single-server behavior unchanged. | ||
| - Unknown//invalid token → 401. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Fix the invalid-token test wording.
Change Unknown//invalid token to Unknown/invalid token.
🤖 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-2.md` around lines 48 - 51,
Update the test requirement wording in the multi-matrix server support
specification by replacing “Unknown//invalid token” with “Unknown/invalid
token,” without changing the surrounding behavior or assertions.
Comment on lines
+45
to
+49
| ## `min_server_version` | ||
|
|
||
| Confirm the earliest Mattermost server version that ships the `SiteURL` field on | ||
| `RegisterPluginOpts` (per-remote registration) and bump `plugin.json` `min_server_version` | ||
| accordingly (currently `10.7.1`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Synchronize the version decision with plugin.json.
This specification still says the current minimum is 10.7.1, while the manifest now requires 11.8.0. Record the confirmed reason and final value here so the compatibility contract is not contradictory.
🤖 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-3.md` around lines 45 - 49,
Update the min_server_version section to reflect plugin.json’s current minimum
of 11.8.0, and document the confirmed Mattermost version/reason that supports
SiteURL on RegisterPluginOpts for per-remote registration. Remove the stale
10.7.1 value so the specification and manifest remain consistent.
Comment on lines
+42
to
+45
| Mirror Mattermost Connected Workspaces: removing a server **stops syncing** and tears down | ||
| its shared-channels remote; it does **not** delete channel content and does not hard-block. | ||
| Provide a guard/warning when channels are still mapped, and clean up (or orphan-mark) that | ||
| server's namespaced KV keys. Finalize exact cleanup batch here. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Resolve deletion behaviour before exposing DELETE.
“Clean up (or orphan-mark)” leaves two incompatible outcomes, especially for re-adding a server and preserving mapped channels. Choose one policy, define retry/idempotency semantics, and test the exact cleanup set.
🤖 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 42 - 45,
Resolve the server-removal policy in the phase-4 specification before exposing
DELETE: choose either cleanup or orphan-marking, define behavior for retries and
idempotent repeated deletion, and specify the exact namespaced KV keys and
shared-channel state affected. Add tests covering the selected cleanup set and
re-adding a server with mapped channels.
Comment on lines
+22
to
+24
| 3. **`/matrix servers`**: new subcommand listing configured servers (id, domain, enabled, | ||
| mapped-channel count). Add to dispatch (`command.go:762-853`) and autocomplete | ||
| (`command.go:266-293`). |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use one canonical /matrix server command contract.
The supplied implementation context and local guide use /matrix server with subcommands, while this specification introduces a top-level /matrix servers. The manifest help also omits supported subcommands such as unmap and status. Align the specification, help text, local guide, autocomplete, and tests with one exact syntax.
spec/2026-07-16-multi-matrix-server-support-phase-6.md#L22-L24: define the canonical command form.docs/local-development.md#L98-L101: document the same form and complete subcommand set.plugin.json#L22-L22: update the administrator help text to match the supported commands.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~22-~22: Loose punctuation mark.
Context: ... is configured. 3. /matrix servers: new subcommand listing configured serve...
(UNLIKELY_OPENING_PUNCTUATION)
📍 Affects 3 files
spec/2026-07-16-multi-matrix-server-support-phase-6.md#L22-L24(this comment)docs/local-development.md#L98-L101plugin.json#L22-L22
🤖 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-6.md` around lines 22 - 24,
Define one canonical `/matrix server` command contract and apply it consistently
across the specification, local-development guide, and plugin administrator
help. Update the specification at
spec/2026-07-16-multi-matrix-server-support-phase-6.md lines 22-24,
docs/local-development.md lines 98-101, and plugin.json line 22 to use the exact
syntax and list all supported subcommands, including `unmap` and `status`;
ensure autocomplete and tests follow the same contract.
Addresses 17 confirmed findings from the PR #33 review: orphaned reverse room mapping on unmap, silent matrix.org fallback in registration YAML generation, non-atomic reads-modify-writes on the shared channel-mapping and server-registry KV keys, swallowed errors in attachment sync and remote-user-invite, staticcheck/gosec lint violations, discarded test-setup errors, and a non-portable Tailscale URL plus stale setup instructions in the local dev docs.
The multi-server support work they described (phases 1-6) has been implemented; the standalone planning docs are no longer needed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
server/command/command.go (1)
703-713: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winAlias-resolution failure during unmap cleanup is silently swallowed.
If
matrixClient.ResolveRoomAlias(matrixRoomIdentifier)errors here, the resolved-room-ID reverse mapping is left behind with no log entry — every other failure path in this function (Lines 657, 689, 699, 731) logs a Warn/Error. This silently defeats the purpose of the fix (avoiding orphaned reverse mappings) with no operator visibility into the failure.Suggested fix
if strings.HasPrefix(matrixRoomIdentifier, "#") { - if resolvedRoomID, err := matrixClient.ResolveRoomAlias(matrixRoomIdentifier); err == nil && resolvedRoomID != "" { - if err := c.kvstore.Delete(kvstore.BuildRoomMappingKey(serverID, resolvedRoomID)); err != nil { - c.client.Log.Warn("Failed to remove resolved room-ID mapping", "error", err, "room_id", resolvedRoomID, "channel_id", args.ChannelId) - } - } + resolvedRoomID, resolveErr := matrixClient.ResolveRoomAlias(matrixRoomIdentifier) + if resolveErr != nil { + c.client.Log.Warn("Failed to resolve alias for reverse-mapping cleanup; resolved-room-ID mapping may remain orphaned", "error", resolveErr, "alias", matrixRoomIdentifier, "channel_id", args.ChannelId) + } else if resolvedRoomID != "" { + if err := c.kvstore.Delete(kvstore.BuildRoomMappingKey(serverID, resolvedRoomID)); err != nil { + c.client.Log.Warn("Failed to remove resolved room-ID mapping", "error", err, "room_id", resolvedRoomID, "channel_id", args.ChannelId) + } + } }🤖 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 703 - 713, Update the alias cleanup block in the unmap flow around ResolveRoomAlias to log a warning when alias resolution fails, including the error and relevant matrix room/channel identifiers. Preserve the existing resolved-room-ID deletion and warning behavior when resolution succeeds.
🤖 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/command/command.go`:
- Around line 486-498: Update the SetAtomicWithRetries closure in the /matrix
map flow to recover from ParseChannelServerMappings errors by treating corrupt
existing data as an empty mapping and continuing with
UpsertChannelServerMapping. Preserve normal parsed mappings when parsing
succeeds, then marshal and save the updated mapping instead of returning the
parse error.
---
Nitpick comments:
In `@server/command/command.go`:
- Around line 703-713: Update the alias cleanup block in the unmap flow around
ResolveRoomAlias to log a warning when alias resolution fails, including the
error and relevant matrix room/channel identifiers. Preserve the existing
resolved-room-ID deletion and warning behavior when resolution succeeds.
🪄 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: 64530bd6-2245-4801-adc9-3ff55174399e
📒 Files selected for processing (11)
docker-compose.ymldocker/mattermost-bridge-registration2.yamldocs/local-development.mdserver/command/command.goserver/command/server_command_test.goserver/hooks.goserver/matrix_mentions_integration_test.goserver/multi_server_integration_test.goserver/plugin.goserver/servers.goserver/servers_test.go
🚧 Files skipped from review as they are similar to previous changes (9)
- docker/mattermost-bridge-registration2.yaml
- docs/local-development.md
- docker-compose.yml
- server/servers_test.go
- server/command/server_command_test.go
- server/servers.go
- server/multi_server_integration_test.go
- server/plugin.go
- server/hooks.go
/matrix status probed each enabled homeserver sequentially via TestConnection, whose HTTP client allows 30s per request. With more than one server registered, an unreachable homeserver could push the command past Mattermost's slash-command timeout, so the multi-server status output added in 7b8996c was unusable in exactly the setup it was written for. Probes now run concurrently under a shared 8s deadline and a server that misses it renders as timed out, rather than being reported as healthy or as a failure that was never observed. Counting mapped channels scanned the whole channel_mapping_ keyspace once per server, in both /matrix status and /matrix server list, giving O(servers x channels) KV reads for the same data. Replace it with a single scan returning per-server counts, and log rather than silently swallow a ListKeysWithPrefix failure that would otherwise pass a partial count off as complete. Also recover from a corrupt channel-mapping value in /matrix server map instead of aborting with no recovery path, matching /matrix unmap, and fix three issues found while reviewing the branch: resolveOutboundServers treated an isDirectChannel lookup error as "not a DM" and silently dropped the event, a doc comment sat on the wrong test function, and a test fixture discarded a mapping-build error.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Route Mattermost ↔ Matrix traffic through any number of Matrix homeservers instead of a single global one (MM-64622). Server configuration is now a registry (keyed by a deterministic
serverID) managed with new/matrix serveradmin slash commands, since the System Console can only edit one server. Existing single-homeserver installs are migrated automatically and behave unchanged.What's included
servers_configKV entry replaces the flatmatrix_server_*plugin.json settings; a v3 migration seeds it from the legacy config on upgrade.hs_token; all reads/writes for that request are namespaced byserverID.remoteID; ping health-checks target the specific pinged remote's server./matrix server add|remove|list|status|map|unmap|registrationto manage homeservers and channel mappings.docs/local-development.md.