fix: group invites time mismatch#192
Conversation
|
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📜 Recent review details⏰ Context from checks skipped due to timeout. (4)
📝 WalkthroughSummary by CodeRabbit
Walkthrough
ChangesGroup sync and member cache overhaul
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![indent[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| if !isChatMID(chatMid) || !isUserMID(senderMid) || lc.isOwnMID(senderMid) { | ||
| return | ||
| } | ||
| lc.addGroupMembersToCache(chatMid, senderMid) |
There was a problem hiding this comment.
Hidden join fires on every group message. addGroupMembersToCache returns whether the sender was actually newly added, but we discard it and unconditionally call portal.ProcessChatInfoChange(...). Internally syncParticipants (mautrix portal.go:~4601) always runs Bridge.Matrix.GetPowerLevels + GetMembers before noticing the membership is unchanged — so every steady-state group message pays for two Matrix state lookups. ensureGroupMessageSenderKnown already gates on the same boolean; mirroring it here (if !lc.addGroupMembersToCache(chatMid, senderMid) { return }) drops the work to once per (chat, sender) pair, matching the only state change worth signalling.
| Err(err). | ||
| Object("portal_key", portal.PortalKey). | ||
| Msg("Timed out waiting for startup group portal creation") | ||
| return |
There was a problem hiding this comment.
Single timeout aborts the whole wait loop. waitCtx is built once outside the loop with context.WithTimeout(ctx, groupPortalCreateWait) (30s), so all portals share the same deadline; if the first portal takes the full 30s (or hits ctx cancellation), Wait returns an error and this return skips every remaining portal in portals[]. The messageWithChatInfo wrapper does cover the race, but the bare return reads like a bug — either continue to keep checking the rest, or (better) move the context.WithTimeout(ctx, groupPortalCreateWait) inside the loop so each portal gets its own 30s budget.
| Timestamp: ts, | ||
| PreHandleFunc: func(ctx context.Context, portal *bridgev2.Portal) { | ||
| lc.hiddenJoinGroupMessageSender(ctx, portal, portalIDStr, msg.From, ts) | ||
| }, |
There was a problem hiding this comment.
Duplicate first-message join paths. On line 119 we call ensureGroupMessageSenderKnown(...) (queues a RemoteEventChatInfoChange if addGroupMembersToCache returned true), and here we also wire PreHandleFunc → hiddenJoinGroupMessageSender(...), which synchronously calls portal.ProcessChatInfoChange. For the first message from a new sender both fire and both go through syncParticipants; for every subsequent message the PreHandle one still does the full GetPowerLevels+GetMembers work (see other comment). Consider keeping only one of the two, or at minimum have hiddenJoinGroupMessageSender honour addGroupMembersToCache's return value so it short-circuits after the first message.
| // doesn't exist yet may be created by the first message, which makes the | ||
| // sender's existing membership look like a fresh join. | ||
| lc.wg.Add(1) | ||
| lc.syncChats(ctx) |
There was a problem hiding this comment.
Synchronous syncChats blocks startup of every other login. mautrix's bridge.StartLogins (bridge.go:373) calls login.Client.Connect(ctx) for each login in sequence; previously all four sync goroutines were launched non-blocking and Connect returned almost immediately. Now Connect waits for syncChats to finish queueing every group plus up to 30s for portal creation (waitForGroupPortalCreates). For a single-login bridge that's fine; on a multi-login install all subsequent logins are delayed by that per-login window. If you want the ordering guarantee without blocking startup, you can keep syncChats in a goroutine and have prefetchMessages + pollLoop wait on a shared exsync.Event set when waitForGroupPortalCreates returns.
| } | ||
|
|
||
| func (lc *LineClient) emitMemberChange(chatMid, userMid string, membership event.Membership, ts time.Time) { | ||
| func (lc *LineClient) emitMemberChange(chatMid, userMid string, membership event.Membership, ts time.Time, excludeFromTimeline ...bool) { |
There was a problem hiding this comment.
Nit — variadic bool is a code smell. This is only ever called with zero or one argument; the variadic is doing nothing except hiding an optional bool parameter. Either make it a regular bool (and update existing callers to pass false explicitly) or add a separate emitHiddenMemberChange helper. Easier to grep, easier to read at the call site.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
pkg/connector/sync.go (1)
999-1014: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAvoid replaying hidden joins for already-known senders.
hiddenJoinGroupMessageSenderignoresaddGroupMembersToCache’s result, so every message from a cached sender can re-runProcessChatInfoChange. Gate this on a newly discovered sender or an explicit portal-membership miss, especially becauseensureGroupMessageSenderKnownmay seed the cache before this pre-handle path.🤖 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 `@pkg/connector/sync.go` around lines 999 - 1014, The hidden join handling in hiddenJoinGroupMessageSender is re-triggering ProcessChatInfoChange for senders that are already cached because it ignores the result of addGroupMembersToCache. Update this flow to only emit the hidden join when the sender is newly discovered or when the portal membership lookup is missing, and use the existing hiddenJoinGroupMessageSender, addGroupMembersToCache, and ensureGroupMessageSenderKnown paths to gate the call accordingly.
🤖 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 `@pkg/connector/client.go`:
- Around line 301-304: The startup WaitGroup accounting in connector client
setup is split around syncChats, which can let Disconnect() observe zero before
the remaining goroutines are registered. Update the initialization flow in
Client startup so all four slots are reserved before calling syncChats, using
the existing lc.wg and syncChats logic, then remove the later partial Add call
to keep shutdown and startup ordering safe.
In `@pkg/connector/handle_message.go`:
- Around line 132-134: The group-message pre-handler is emitting hidden joins on
every queued message because PreHandleFunc always calls
hiddenJoinGroupMessageSender, which then unconditionally triggers
ProcessChatInfoChange for non-own senders. Update the flow in handle_message.go
so hiddenJoinGroupMessageSender only runs when addGroupMembersToCache actually
adds a new member, or add a cache membership check before calling it, using the
existing hiddenJoinGroupMessageSender and addGroupMembersToCache paths to
prevent duplicate hidden join events.
In `@pkg/connector/sync.go`:
- Around line 785-788: The `cacheGroupMembersFromMessage` path is still skipping
non-bridgeable group messages because of the `isBridgeableContentType` guard,
which prevents sender caching for fallback/E2EE cases. Remove that early return
so `lc.addGroupMembersToCache(chatMid, msg.From)` runs for every non-system
group message, relying on `addGroupMembersToCache` to ignore non-user MIDs.
---
Nitpick comments:
In `@pkg/connector/sync.go`:
- Around line 999-1014: The hidden join handling in hiddenJoinGroupMessageSender
is re-triggering ProcessChatInfoChange for senders that are already cached
because it ignores the result of addGroupMembersToCache. Update this flow to
only emit the hidden join when the sender is newly discovered or when the portal
membership lookup is missing, and use the existing hiddenJoinGroupMessageSender,
addGroupMembersToCache, and ensureGroupMessageSenderKnown paths to gate the call
accordingly.
🪄 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: 5c7753f7-8fd4-49c8-98dd-1bed8f0c524f
📒 Files selected for processing (3)
pkg/connector/client.gopkg/connector/handle_message.gopkg/connector/sync.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/*.go: Usego fmtfor code formatting across all Go files
Usegoimportswith-local "github.com/highesttt/matrix-line-messenger"flag to group project-local imports correctly
Usezerologfor logging throughout the codebase
Do not useMsgfin logging; useMsgwith structured fields instead
UseStringerinterface where applicable in Go code
Files:
pkg/connector/client.gopkg/connector/handle_message.gopkg/connector/sync.go
**/!(ltsm)/**/*.go
📄 CodeRabbit inference engine (AGENTS.md)
**/!(ltsm)/**/*.go: Runstaticcheckon all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Rungo veton all Go files excludingpkg/ltsmpackage (transpiled WASM code)
Files:
pkg/connector/client.gopkg/connector/handle_message.gopkg/connector/sync.go
🔇 Additional comments (4)
pkg/connector/sync.go (2)
31-32: LGTM!Also applies to: 286-286, 424-424, 468-514, 535-553, 601-634, 761-784, 808-808, 837-983, 1730-1750, 1759-1784, 1897-1943
524-530: 🩺 Stability & AvailabilityMake the portal create path observable before this lookup.
pendingPortalCreatesonly gets populated ifGetExistingPortalByKeycan see the new portal immediately afterQueueRemoteEvent; if portal creation is deferred, first-time group portals will skipRoomCreated.Wait()and startup ordering won’t be enforced here.pkg/connector/sync.go:524-530pkg/connector/handle_message.go (2)
77-100: LGTM!
115-131: LGTM!Also applies to: 136-153
| lc.wg.Add(1) | ||
| lc.syncChats(ctx) | ||
|
|
||
| lc.wg.Add(3) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'wg\.(Add|Done|Wait)\(|func \(lc \*LineClient\) (syncChats|syncDMChats|prefetchMessages|pollLoop|Disconnect)\b' pkg/connectorRepository: beeper/line
Length of output: 6067
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the startup / shutdown flow around Connect and Disconnect.
sed -n '250,360p' pkg/connector/client.go
printf '\n---\n'
sed -n '420,450p' pkg/connector/client.go
printf '\n---\n'
sed -n '1080,1145p' pkg/connector/sync.go
printf '\n---\n'
rg -n -C2 'Connect\(context\.Background\(\)\)|Disconnect\(\)|wg\.Wait\(|wg\.Add\(' pkg/connectorRepository: beeper/line
Length of output: 9939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the startup / shutdown flow around Connect and Disconnect.
sed -n '250,360p' pkg/connector/client.go
printf '\n---\n'
sed -n '420,450p' pkg/connector/client.go
printf '\n---\n'
sed -n '1080,1145p' pkg/connector/sync.go
printf '\n---\n'
rg -n -C2 'Connect\(context\.Background\(\)\)|Disconnect\(\)|wg\.Wait\(|wg\.Add\(' pkg/connectorRepository: beeper/line
Length of output: 9939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the startup / shutdown flow around Connect and Disconnect.
sed -n '250,360p' pkg/connector/client.go
printf '\n---\n'
sed -n '420,450p' pkg/connector/client.go
printf '\n---\n'
sed -n '1080,1145p' pkg/connector/sync.go
printf '\n---\n'
rg -n -C2 'Connect\(context\.Background\(\)\)|Disconnect\(\)|wg\.Wait\(|wg\.Add\(' pkg/connectorRepository: beeper/line
Length of output: 9939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the startup / shutdown flow around Connect and Disconnect.
sed -n '250,360p' pkg/connector/client.go
printf '\n---\n'
sed -n '420,450p' pkg/connector/client.go
printf '\n---\n'
sed -n '1080,1145p' pkg/connector/sync.go
printf '\n---\n'
rg -n -C2 'Connect\(context\.Background\(\)\)|Disconnect\(\)|wg\.Wait\(|wg\.Add\(' pkg/connectorRepository: beeper/line
Length of output: 9939
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the startup / shutdown flow around Connect and Disconnect.
sed -n '250,360p' pkg/connector/client.go
printf '\n---\n'
sed -n '420,450p' pkg/connector/client.go
printf '\n---\n'
sed -n '1080,1145p' pkg/connector/sync.go
printf '\n---\n'
rg -n -C2 'Connect\(context\.Background\(\)\)|Disconnect\(\)|wg\.Wait\(|wg\.Add\(' pkg/connectorRepository: beeper/line
Length of output: 9939
Reserve all four WaitGroup slots before syncChats in pkg/connector/client.go:301-307. Disconnect() can return after syncChats drops the counter to zero, before the later Add(3), so the remaining goroutines may start after shutdown. Use Add(4) up front to cover all startup work.
🤖 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 `@pkg/connector/client.go` around lines 301 - 304, The startup WaitGroup
accounting in connector client setup is split around syncChats, which can let
Disconnect() observe zero before the remaining goroutines are registered. Update
the initialization flow in Client startup so all four slots are reserved before
calling syncChats, using the existing lc.wg and syncChats logic, then remove the
later partial Add call to keep shutdown and startup ordering safe.
| PreHandleFunc: func(ctx context.Context, portal *bridgev2.Portal) { | ||
| lc.hiddenJoinGroupMessageSender(ctx, portal, portalIDStr, msg.From, ts) | ||
| }, |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
Avoid emitting a hidden join for every group message.
PreHandleFunc runs per queued message, while hiddenJoinGroupMessageSender unconditionally calls ProcessChatInfoChange for any non-own group sender. This can repeatedly send identical hidden membership joins for active group members. Please gate this on whether addGroupMembersToCache actually inserted a new member, or check the cache before calling the helper.
🤖 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 `@pkg/connector/handle_message.go` around lines 132 - 134, The group-message
pre-handler is emitting hidden joins on every queued message because
PreHandleFunc always calls hiddenJoinGroupMessageSender, which then
unconditionally triggers ProcessChatInfoChange for non-own senders. Update the
flow in handle_message.go so hiddenJoinGroupMessageSender only runs when
addGroupMembersToCache actually adds a new member, or add a cache membership
check before calling it, using the existing hiddenJoinGroupMessageSender and
addGroupMembersToCache paths to prevent duplicate hidden join events.
| if !isBridgeableContentType(msg) { | ||
| return | ||
| } | ||
| lc.groupMemberCache[chatMid] = members | ||
| lc.cacheMu.Unlock() | ||
| lc.addGroupMembersToCache(chatMid, msg.From) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Cache senders from every non-system group message.
cacheGroupMembersFromMessage is now called for every backfill message, but this guard still drops unsupported/non-bridgeable user messages. Since addGroupMembersToCache already filters non-user MIDs, removing it keeps the member cache complete for fallback/E2EE use.
Proposed fix
if msg.ContentType == 18 {
lc.cacheGroupMembersFromSystemMessage(msg)
return
}
- if !isBridgeableContentType(msg) {
- return
- }
lc.addGroupMembersToCache(chatMid, msg.From)📝 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.
| if !isBridgeableContentType(msg) { | |
| return | |
| } | |
| lc.groupMemberCache[chatMid] = members | |
| lc.cacheMu.Unlock() | |
| lc.addGroupMembersToCache(chatMid, msg.From) | |
| lc.addGroupMembersToCache(chatMid, msg.From) |
🤖 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 `@pkg/connector/sync.go` around lines 785 - 788, The
`cacheGroupMembersFromMessage` path is still skipping non-bridgeable group
messages because of the `isBridgeableContentType` guard, which prevents sender
caching for fallback/E2EE cases. Remove that early return so
`lc.addGroupMembersToCache(chatMid, msg.From)` runs for every non-system group
message, relying on `addGroupMembersToCache` to ignore non-user MIDs.
1 participant
No description provided.