Skip to content

fix: race condition stale tokens/in flight requests - #228

Merged
highesttt merged 5 commits into
mainfrom
highest/plat-38184
Jul 30, 2026
Merged

fix: race condition stale tokens/in flight requests#228
highesttt merged 5 commits into
mainfrom
highest/plat-38184

Conversation

@highesttt

Copy link
Copy Markdown
Collaborator

No description provided.

@linear-code

linear-code Bot commented Jul 30, 2026

Copy link
Copy Markdown

PLAT-38184

@indent-zero

indent-zero Bot commented Jul 30, 2026

Copy link
Copy Markdown
Contributor
PR Summary

Merges main (which already contains the "reuse LINE custom reactions" work as squash-merged PR #227) into the branch and leaves this PR's remaining scope as the race-safe auth-recovery refactor plus the migration of every LINE RPC call site onto the shared callLine[Result][Using] helpers.

  • Merge is a clean no-op relative to a5d41d0: reaction.go / reaction_test.go / connector.go now match main verbatim because their content landed via fix: allow reusing LINE custom reactions #227.
  • Widens lineCallDeps.recover to (ctx, failedClient, err) -> (client, error) and adds LineClient.recoverClientAfterAuthError, which serializes on recoverMu, retries stale-token failures with the current access token, and only marks a forced logout when the failed request used the current token.
  • Removes shouldAttemptTokenRecovery, recentReactions, ensureValidTokenWith, and LineClient.isRefreshRequired; isTokenError no longer excludes IsLoggedOut (classification moves into the new recovery path); markLoggedOutByOtherClient gains a Locked variant so callers already holding recoverMu can invalidate atomically.
  • Migrates sync.go, userinfo.go, creategroup.go, e2ee_keys.go, ensureValidToken, and every handlers/*.go media converter (via a new Handler.RecoverClient + handleFinalAuthError) to the unified helpers.
  • handleReceiveAuthError probes the current access token first, then classifies the logged-out branch with the probing client + probe error to keep the classification source-consistent.
  • handleReactionRemove is simplified to a single sender networkid.UserID parameter with the outdated "multiple sender candidates" comment removed.
  • Test suite migrated: forced_logout_test.go cases now exercise the real ensureValidToken (stubbing getProfileWithToken/recoverLineToken), plus new sync/reaction/auth cases covering stale-token retries, concurrent recoveries, and current-token forced logouts.

Issues

All clear! No issues remaining. 🎉

3 issues already resolved
  • In handleReceiveAuthError, when the SSE 401 came from an old (stale) client the follow-up getProfileWithToken probe reuses that stale token instead of the current one, so the probe is guaranteed to fail with "logged out" before recoverClientAfterAuthError detects the staleness and reconnects on the current token. Correctness is unchanged, but the probe is a wasted HTTPS round-trip on every stale-SSE auth error — probing with lc.getAccessToken() first would skip it. (fixed by commit a5d41d0)
  • handleReactionRemove's "multiple sender candidates when the previous reaction's actor is ambiguous" comment and for _, sender := range senders loop no longer reflect reality: both call sites in sync.go (self-removal at line 1947, other-removal at line 1986) pass a single-element slice, and the ambiguity the comment describes went away with the recentReactions dedup that this PR removes. (fixed by commit a5d41d0)
  • ensureValidTokenWith (and, transitively, LineClient.isRefreshRequired) are no longer reachable from production after ensureValidToken was rewritten to use callLineResult — the only remaining references are the tests in forced_logout_test.go. Consider dropping the legacy helper (or migrating the two tests onto the real ensureValidToken) so the test suite reflects the code that actually runs. (fixed by commit a5d41d0)

CI Checks

All CI checks passed on cfe0190.

@coderabbitai

coderabbitai Bot commented Jul 30, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Summary by CodeRabbit

  • New Features

    • Added richer reaction syncing for predefined and paid reactions, preserving reaction identity and metadata across platforms.
  • Bug Fixes

    • Improved handling of expired, stale, and forced logout scenarios so outdated auth failures don’t interrupt active sessions.
    • Enhanced retry behavior for chat/group creation, key registration, contact/profile lookups, and media (audio/image/video/file/album previews) downloads after auth errors.
  • Reliability

    • Centralized and standardized authentication recovery across messaging and synchronization workflows.
    • Improved session invalidation, reconnection, and concurrency handling for consistent SSE behavior.

Walkthrough

The PR centralizes source-aware LINE authentication recovery across API, SSE, message, and media flows. It also adds persisted identity and synchronization support for predefined and paid reactions.

Changes

Source-aware authentication recovery

Layer / File(s) Summary
Centralized token recovery
pkg/connector/auth_recovery.go, pkg/connector/client.go, pkg/connector/*_test.go
Recovery receives the failed client and error, coordinates concurrent refresh and logout transitions, distinguishes stale tokens, and validates current sessions directly.
Handler and media recovery wiring
pkg/connector/handle_message.go, pkg/connector/handlers/*
Handlers retry downloads with recovered clients and apply final logged-out handling through the unified recovery callback.
API call and SSE migration
pkg/connector/sync.go, pkg/connector/creategroup.go, pkg/connector/e2ee_keys.go, pkg/connector/userinfo.go
LINE requests use shared call wrappers, while SSE authentication probes and failures pass originating clients through recovery and session-state handling.

Reaction identity and synchronization

Layer / File(s) Summary
Reaction identity and conversion
pkg/connector/reaction.go, pkg/connector/connector.go, pkg/connector/reaction_test.go
Predefined and paid reactions share validated references, stable identifiers, persisted metadata, and backfill conversion.
Matrix resolution and live synchronization
pkg/connector/reaction.go, pkg/connector/sync.go, pkg/connector/reaction_test.go
Matrix reactions resolve from direct mappings or stored metadata, and live updates emit sender-specific add and removal synchronization events.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant LINE as LINE API/SSE
  participant Connector as LineClient
  participant Recovery as recoverClientAfterAuthError
  participant Session as Session state
  LINE->>Connector: request or SSE auth failure
  Connector->>Recovery: failed client and auth error
  Recovery->>Session: classify and update token/session state
  Session-->>Recovery: replacement client or terminal state
  Recovery-->>Connector: recovery result
  Connector->>LINE: retry with recovered client
Loading

Possibly related PRs

  • beeper/line#206: Overlaps with SSE receive-loop authentication recovery and reconnect handling.
  • beeper/line#214: Overlaps with forced-logout transitions and token/session recovery.
  • beeper/line#227: Overlaps with reaction metadata and synchronization changes.
🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ❓ Inconclusive No meaningful pull request description was provided, so the change intent cannot be assessed from it. Add a brief description of the race condition fix and the affected token-recovery paths.
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title matches the main change: fixing a stale-token/in-flight-request race condition.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch highest/plat-38184

Comment @coderabbitai help to get the list of available commands.

Comment thread pkg/connector/sync.go Outdated
Comment thread pkg/connector/client.go Outdated
Comment thread pkg/connector/sync.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🧹 Nitpick comments (2)
pkg/connector/auth_recovery_test.go (1)

272-343: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Guard the started.Done() call against being reached twice.

started.Done() fires inside the call closure on the old-token branch. It's only reached once per goroutine today because the stale branch always hands back a different token, but any future change that returns a same-token client makes this panic with a negative WaitGroup counter and turns the whole suite red. Using sync.OnceFunc per goroutine (or signaling before callLineUsing) removes that coupling.

🤖 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/auth_recovery_test.go` around lines 272 - 343, The stale-call
synchronization in TestConcurrentOldTokenFailuresWaitForSingleRecovery should
tolerate the callback being invoked more than once. Guard each goroutine’s
started.Done() with a per-goroutine sync.OnceFunc, or signal before entering
callLineUsing, while preserving the existing wait and recovery assertions.
pkg/connector/auth_recovery.go (1)

64-103: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Consider extracting the locked classification into a helper so recoverMu unlocks via defer.

Six manual Unlock() call sites in one function is easy to break on the next edit (an early return added inside the block would deadlock every subsequent recovery).

♻️ Sketch
+func (lc *LineClient) classifyAuthErrorLocked(ctx context.Context, failedClient *line.Client, err error) (*line.Client, bool, error) {
+	lc.recoverMu.Lock()
+	defer lc.recoverMu.Unlock()
+	// ...existing checks, returning (client, done, err)
+}

Then recoverClientAfterAuthError performs recoverLineToken only after the helper returns without holding the lock.

🤖 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/auth_recovery.go` around lines 64 - 103, Refactor
recoverClientAfterAuthError so the recoverMu-protected token/session
classification is extracted into a helper that acquires recoverMu and releases
it with defer. Have the helper return the classification and any replacement
client or error needed by the caller, ensuring recoverLineToken runs only after
the helper has released the lock; preserve the existing stale-token, logout,
stopped, invalidated, and context-cancellation behavior.
🤖 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 774-789: Update ensureValidToken to guard the non-auth warning log
with checks that lc.UserLogin and lc.UserLogin.Bridge are non-nil, matching the
existing pattern in auth_recovery.go; only call Bridge.Log.Warn when the logging
dependencies are available, while preserving the current return behavior.

In `@pkg/connector/handlers/audio.go`:
- Around line 51-55: Extract the shared OBS download-and-recovery flow into
Handler.downloadOBSWithRecovery in pkg/connector/handlers/handler.go, preserving
the initial download, tryRecoverClient retry, handleFinalAuthError call, and
returned client/data/error values. Replace the duplicated sequences in
pkg/connector/handlers/audio.go lines 51-55, file.go lines 42-46, and video.go
lines 53-57 with helper calls assigning their respective data variables; all
three sites require direct changes.

In `@pkg/connector/reaction.go`:
- Around line 97-105: Update lineReactionRef.equal to compare paid reactions
using the stable identity represented by networkEmojiID, rather than full
PaidReactionType struct equality. Preserve the existing predefined-type and
nil-handling behavior, while ignoring Version and ResourceType differences when
ProductID and EmojiID match. Add a regression case to
TestStoredLineReactionForMatrixKey with identical MatrixKey/ProductID/EmojiID
but differing Version, verifying both rows are treated as the same reaction.

In `@pkg/connector/sync.go`:
- Around line 1729-1739: Update the recoverClientAfterAuthError call in the
profile-error handling path to classify the fresher profileErr instead of the
stale SSE err. Preserve the existing recoveredClient and errRecover branching,
ensuring recovery uses the latest profile probe signal.

---

Nitpick comments:
In `@pkg/connector/auth_recovery_test.go`:
- Around line 272-343: The stale-call synchronization in
TestConcurrentOldTokenFailuresWaitForSingleRecovery should tolerate the callback
being invoked more than once. Guard each goroutine’s started.Done() with a
per-goroutine sync.OnceFunc, or signal before entering callLineUsing, while
preserving the existing wait and recovery assertions.

In `@pkg/connector/auth_recovery.go`:
- Around line 64-103: Refactor recoverClientAfterAuthError so the
recoverMu-protected token/session classification is extracted into a helper that
acquires recoverMu and releases it with defer. Have the helper return the
classification and any replacement client or error needed by the caller,
ensuring recoverLineToken runs only after the helper has released the lock;
preserve the existing stale-token, logout, stopped, invalidated, and
context-cancellation behavior.
🪄 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: ece185d9-c542-458f-b8a7-7f2567f2ae61

📥 Commits

Reviewing files that changed from the base of the PR and between cf5e471 and fc15510.

📒 Files selected for processing (20)
  • pkg/connector/auth_recovery.go
  • pkg/connector/auth_recovery_test.go
  • pkg/connector/client.go
  • pkg/connector/connector.go
  • pkg/connector/creategroup.go
  • pkg/connector/e2ee_keys.go
  • pkg/connector/handle_message.go
  • pkg/connector/handlers/audio.go
  • pkg/connector/handlers/file.go
  • pkg/connector/handlers/handler.go
  • pkg/connector/handlers/handler_test.go
  • pkg/connector/handlers/image.go
  • pkg/connector/handlers/post_notification.go
  • pkg/connector/handlers/post_notification_test.go
  • pkg/connector/handlers/video.go
  • pkg/connector/reaction.go
  • pkg/connector/reaction_test.go
  • pkg/connector/sync.go
  • pkg/connector/sync_test.go
  • pkg/connector/userinfo.go
💤 Files with no reviewable changes (1)
  • pkg/connector/handlers/post_notification_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
  • GitHub Check: Lint with 1.25
  • GitHub Check: build-docker
  • GitHub Check: build-docker
  • GitHub Check: Lint with 1.25
🧰 Additional context used
📓 Path-based instructions (3)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Use go fmt for code formatting across all Go files
Use goimports with -local "github.com/highesttt/matrix-line-messenger" flag to group project-local imports correctly
Use zerolog for logging throughout the codebase
Do not use Msgf in logging; use Msg with structured fields instead
Use Stringer interface where applicable in Go code

Files:

  • pkg/connector/handlers/file.go
  • pkg/connector/connector.go
  • pkg/connector/handle_message.go
  • pkg/connector/handlers/audio.go
  • pkg/connector/handlers/image.go
  • pkg/connector/handlers/video.go
  • pkg/connector/creategroup.go
  • pkg/connector/handlers/handler.go
  • pkg/connector/sync_test.go
  • pkg/connector/userinfo.go
  • pkg/connector/auth_recovery.go
  • pkg/connector/handlers/post_notification.go
  • pkg/connector/reaction_test.go
  • pkg/connector/e2ee_keys.go
  • pkg/connector/handlers/handler_test.go
  • pkg/connector/client.go
  • pkg/connector/auth_recovery_test.go
  • pkg/connector/reaction.go
  • pkg/connector/sync.go
**/!(ltsm)/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/!(ltsm)/**/*.go: Run staticcheck on all Go files excluding pkg/ltsm package (transpiled WASM code)
Run go vet on all Go files excluding pkg/ltsm package (transpiled WASM code)

Files:

  • pkg/connector/handlers/file.go
  • pkg/connector/connector.go
  • pkg/connector/handle_message.go
  • pkg/connector/handlers/audio.go
  • pkg/connector/handlers/image.go
  • pkg/connector/handlers/video.go
  • pkg/connector/creategroup.go
  • pkg/connector/handlers/handler.go
  • pkg/connector/sync_test.go
  • pkg/connector/userinfo.go
  • pkg/connector/auth_recovery.go
  • pkg/connector/handlers/post_notification.go
  • pkg/connector/reaction_test.go
  • pkg/connector/e2ee_keys.go
  • pkg/connector/handlers/handler_test.go
  • pkg/connector/client.go
  • pkg/connector/auth_recovery_test.go
  • pkg/connector/reaction.go
  • pkg/connector/sync.go
pkg/connector/connector.go

📄 CodeRabbit inference engine (AGENTS.md)

Implement bridgev2.NetworkConnector and bridgev2.NetworkAPI interfaces in the connector package for bridge logic

Files:

  • pkg/connector/connector.go
🔇 Additional comments (37)
pkg/connector/reaction.go (9)

539-560: 🎯 Functional Correctness

Downstream impact of the equal() issue.

The conflict check here (found.equal(ref), Line 553) inherits the over-strict identity comparison flagged on equal() at Lines 97-105 in this file; see that comment for the fix and rationale.


39-46: LGTM!


59-96: LGTM!


107-113: LGTM!


365-393: LGTM!


412-421: 🎯 Functional Correctness

Zero-value fallback for missing/invalid AtMillis.

When reaction.AtMillis.Int64() fails or returns <= 0, timestamp stays the Go zero value (year 1) and is passed straight into BackfillReaction.Timestamp. If the backfill/history pipeline uses this timestamp for chronological placement, a reaction lacking a valid AtMillis could be sorted far out of place in the room history.

Please confirm how bridgev2.BackfillReaction (and the backfill placement logic) treats a zero-value Timestamp — whether it's special-cased as "unknown" or actually affects ordering.


395-411: LGTM!

Also applies to: 422-434


562-600: LGTM!


707-750: LGTM!

pkg/connector/connector.go (1)

100-111: LGTM!

pkg/connector/reaction_test.go (6)

21-69: LGTM!


437-503: LGTM!


505-577: LGTM!


579-600: LGTM!


735-758: LGTM!


790-828: LGTM!

pkg/connector/auth_recovery.go (3)

30-46: LGTM!


12-12: LGTM!

Also applies to: 134-134, 150-150


105-114: 🩺 Stability & Availability

No unbounded recursion here. This path exits on the same-token IsLoggedOut branch, and stale-token cases return the rotated client instead; recoverToken is also serialized by recoverMu and rate-limited by recentTokenRecoveryWindow.

			> Likely an incorrect or invalid review comment.
pkg/connector/client.go (1)

82-83: LGTM!

Also applies to: 318-359

pkg/connector/auth_recovery_test.go (2)

93-98: LGTM!

Also applies to: 144-145, 162-163, 180-182, 202-211


435-443: LGTM!

Also applies to: 457-517

pkg/connector/handlers/handler.go (1)

22-24: LGTM!

Also applies to: 77-99

pkg/connector/handlers/handler_test.go (1)

14-87: LGTM!

pkg/connector/handlers/image.go (1)

53-61: LGTM!

pkg/connector/handlers/post_notification.go (1)

214-224: LGTM!

pkg/connector/sync.go (5)

120-122: LGTM!

Also applies to: 155-157, 593-595, 759-761, 1185-1187, 2165-2167, 2287-2289, 2330-2332


828-830: LGTM!

Also applies to: 855-858, 1522-1528, 1640-1641


1666-1694: LGTM!

Also applies to: 1705-1728


2029-2062: 🩺 Stability & Availability

No issue: convertReaction never returns nil, nil
convertReaction returns a non-nil *bridgev2.BackfillReaction on success and nil, err on failure, so reaction.Sender.Sender is safe here.

			> Likely an incorrect or invalid review comment.

2064-2105: 🗄️ Data Integrity & Integration

No change needed HasAllReactions: true with an empty Reactions list is the intended authoritative empty state for that sender, while HasAllUsers: false keeps other senders untouched.

			> Likely an incorrect or invalid review comment.
pkg/connector/handle_message.go (1)

30-38: LGTM!

pkg/connector/e2ee_keys.go (2)

36-120: LGTM!


235-260: LGTM!

pkg/connector/sync_test.go (1)

839-862: LGTM!

Also applies to: 864-894, 896-924, 925-961

pkg/connector/userinfo.go (1)

143-157: LGTM!

Also applies to: 199-247, 297-323, 355-361

pkg/connector/creategroup.go (1)

37-39: 🗄️ Data Integrity & Integration

Review stale-token retries on non-idempotent writes. callLineWithRecovery retries once after auth errors; if LINE can fail after accepting CreateChat or RegisterE2EEGroupKey, the helper would replay the write.

Comment thread pkg/connector/client.go
Comment on lines +51 to +55
if newClient, ok := h.tryRecoverClient(ctx, client, err); ok {
client = newClient
audioData, err = client.DownloadOBSWithSIDOptions(ctx, oid, talkMetaMessageID, sid, downloadOptions)
}
h.handleFinalAuthError(ctx, client, err)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Extract the shared OBS download-with-recovery sequence. All three handlers implement the identical tryRecoverClient → retry → handleFinalAuthError sequence around DownloadOBSWithSIDOptions, differing only by variable name. This is security/reliability-critical auth-recovery logic; keeping three copies risks them drifting apart on future fixes.

  • pkg/connector/handlers/audio.go#L51-L55: replace with a call to a shared helper, e.g. client, audioData, err = h.downloadOBSWithRecovery(ctx, client, oid, talkMetaMessageID, sid, downloadOptions).
  • pkg/connector/handlers/file.go#L42-L46: replace with the same shared helper call for fileData.
  • pkg/connector/handlers/video.go#L53-L57: replace with the same shared helper call for videoData.
♻️ Proposed shared helper (add to pkg/connector/handlers/handler.go)
func (h *Handler) downloadOBSWithRecovery(ctx context.Context, client *line.Client, oid, messageID, sid string, opts line.OBSDownloadOptions) (*line.Client, []byte, error) {
	data, err := client.DownloadOBSWithSIDOptions(ctx, oid, messageID, sid, opts)
	if newClient, ok := h.tryRecoverClient(ctx, client, err); ok {
		client = newClient
		data, err = client.DownloadOBSWithSIDOptions(ctx, oid, messageID, sid, opts)
	}
	h.handleFinalAuthError(ctx, client, err)
	return client, data, err
}
📍 Affects 3 files
  • pkg/connector/handlers/audio.go#L51-L55 (this comment)
  • pkg/connector/handlers/file.go#L42-L46
  • pkg/connector/handlers/video.go#L53-L57
🤖 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/handlers/audio.go` around lines 51 - 55, Extract the shared OBS
download-and-recovery flow into Handler.downloadOBSWithRecovery in
pkg/connector/handlers/handler.go, preserving the initial download,
tryRecoverClient retry, handleFinalAuthError call, and returned
client/data/error values. Replace the duplicated sequences in
pkg/connector/handlers/audio.go lines 51-55, file.go lines 42-46, and video.go
lines 53-57 with helper calls assigning their respective data variables; all
three sites require direct changes.

Comment thread pkg/connector/reaction.go
Comment on lines +97 to +105
func (ref lineReactionRef) equal(other lineReactionRef) bool {
if ref.typ.PredefinedReactionType != other.typ.PredefinedReactionType {
return false
}
if ref.typ.PaidReactionType == nil || other.typ.PaidReactionType == nil {
return ref.typ.PaidReactionType == nil && other.typ.PaidReactionType == nil
}
return *ref.typ.PaidReactionType == *other.typ.PaidReactionType
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

equal() is stricter than the stable reaction identity, causing false "conflict" failures.

equal() compares the full PaidReactionType struct (including ResourceType/Version) via ==, but networkEmojiID() — the type's own definition of stable identity, used for EmojiID and DB storage — only encodes ProductID+EmojiID. This is not just theoretical: getPaidReactionMXC (Line 318) caches/derives the icon MXC (which becomes ReactionMetadata.MatrixKey) keyed solely by lineSticonURL(prt.ProductID, prt.EmojiID), so two stored reactions for the same paid sticker can legitimately share a MatrixKey while carrying different Version/ResourceType (e.g., after a sticker-pack version bump between two reacts).

storedLineReactionForMatrixKey (Line 553) uses found.equal(ref) as its sole cross-row conflict check, so this realistic scenario is misclassified as a conflicting/ambiguous reaction and resolveMatrixReaction fails with unsupportedMatrixReactionError, even though the two rows refer to the same reaction identity. TestStoredLineReactionForMatrixKey doesn't exercise this case (its "same" fixture is an exact clone).

Align equal() with the stable identity used everywhere else:

🐛 Proposed fix
 func (ref lineReactionRef) equal(other lineReactionRef) bool {
-	if ref.typ.PredefinedReactionType != other.typ.PredefinedReactionType {
-		return false
-	}
-	if ref.typ.PaidReactionType == nil || other.typ.PaidReactionType == nil {
-		return ref.typ.PaidReactionType == nil && other.typ.PaidReactionType == nil
-	}
-	return *ref.typ.PaidReactionType == *other.typ.PaidReactionType
+	return ref.networkEmojiID() == other.networkEmojiID()
 }

Also consider adding a regression test in reaction_test.go covering two stored rows with the same MatrixKey/ProductID/EmojiID but differing Version.

📝 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.

Suggested change
func (ref lineReactionRef) equal(other lineReactionRef) bool {
if ref.typ.PredefinedReactionType != other.typ.PredefinedReactionType {
return false
}
if ref.typ.PaidReactionType == nil || other.typ.PaidReactionType == nil {
return ref.typ.PaidReactionType == nil && other.typ.PaidReactionType == nil
}
return *ref.typ.PaidReactionType == *other.typ.PaidReactionType
}
func (ref lineReactionRef) equal(other lineReactionRef) bool {
return ref.networkEmojiID() == other.networkEmojiID()
}
🤖 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/reaction.go` around lines 97 - 105, Update
lineReactionRef.equal to compare paid reactions using the stable identity
represented by networkEmojiID, rather than full PaidReactionType struct
equality. Preserve the existing predefined-type and nil-handling behavior, while
ignoring Version and ResourceType differences when ProductID and EmojiID match.
Add a regression case to TestStoredLineReactionForMatrixKey with identical
MatrixKey/ProductID/EmojiID but differing Version, verifying both rows are
treated as the same reaction.

Comment thread pkg/connector/sync.go

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
pkg/connector/forced_logout_test.go (1)

96-164: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win

t.Fatalf inside a non-test goroutine can hang instead of failing.

The getProfileWithToken stub (Line 119) calls t.Fatalf on an unexpected token. This stub is invoked from lc.ensureValidToken(...) running inside the go func() at Lines 138-140, not the main test goroutine. t.Fatalf/FailNow must be called from the goroutine running the test function — calling it elsewhere only terminates that goroutine via runtime.Goexit(), so ensureDone never receives a value and <-ensureDone (Line 150) hangs until the test binary's global timeout instead of reporting a clear assertion failure. This defeats the test's diagnostic value exactly when it should fire.

🐛 Proposed fix: use t.Errorf instead of t.Fatalf in the goroutine-executed stub
 	getProfileWithToken = func(_ context.Context, token string) (*line.Profile, error) {
 		if token == "recovered-token" {
 			return &line.Profile{}, nil
 		}
 		if token != "old-token" {
-			t.Fatalf("profile token = %q, want old-token or recovered-token", token)
+			t.Errorf("profile token = %q, want old-token or recovered-token", token)
 		}
 		return nil, errAuthRequired
 	}
🤖 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/forced_logout_test.go` around lines 96 - 164, Update the
unexpected-token assertion in the getProfileWithToken stub within
TestForcedLogoutWinsOverEnsureValidTokenRefresh to use a non-terminating test
failure, such as t.Errorf, so the ensureValidToken goroutine can return and send
its result through ensureDone. Preserve the existing token validation and
diagnostic message.
🤖 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.

Outside diff comments:
In `@pkg/connector/forced_logout_test.go`:
- Around line 96-164: Update the unexpected-token assertion in the
getProfileWithToken stub within TestForcedLogoutWinsOverEnsureValidTokenRefresh
to use a non-terminating test failure, such as t.Errorf, so the ensureValidToken
goroutine can return and send its result through ensureDone. Preserve the
existing token validation and diagnostic message.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f0c2ac2e-fdce-472f-b387-8d55fa09768a

📥 Commits

Reviewing files that changed from the base of the PR and between fc15510 and a5d41d0.

📒 Files selected for processing (4)
  • pkg/connector/client.go
  • pkg/connector/forced_logout_test.go
  • pkg/connector/sync.go
  • pkg/connector/sync_test.go
💤 Files with no reviewable changes (1)
  • pkg/connector/client.go
📜 Review details
⏰ Context from checks skipped due to timeout. (2)
  • GitHub Check: Lint with 1.25
  • GitHub Check: build-docker
🧰 Additional context used
📓 Path-based instructions (2)
**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/*.go: Use go fmt for code formatting across all Go files
Use goimports with -local "github.com/highesttt/matrix-line-messenger" flag to group project-local imports correctly
Use zerolog for logging throughout the codebase
Do not use Msgf in logging; use Msg with structured fields instead
Use Stringer interface where applicable in Go code

Files:

  • pkg/connector/sync_test.go
  • pkg/connector/forced_logout_test.go
  • pkg/connector/sync.go
**/!(ltsm)/**/*.go

📄 CodeRabbit inference engine (AGENTS.md)

**/!(ltsm)/**/*.go: Run staticcheck on all Go files excluding pkg/ltsm package (transpiled WASM code)
Run go vet on all Go files excluding pkg/ltsm package (transpiled WASM code)

Files:

  • pkg/connector/sync_test.go
  • pkg/connector/forced_logout_test.go
  • pkg/connector/sync.go
🔇 Additional comments (5)
pkg/connector/sync.go (2)

1705-1760: 🎯 Functional Correctness

Final fallback branch still classifies against the stale err/failedClient, not the fresher profileErr/profileClient.

At Line 1734, lc.recoverClientAfterAuthError(ctx, failedClient, err) is reached only after the profile probe (using the current token) has already failed with a non-logged-out auth error (profileErr, Line 1719). This branch still classifies recovery against the original SSE err/failedClient rather than the newer profileErr/profileClient — same issue flagged in a previous review on this function (only the isLoggedOut(profileErr) branch at Lines 1723-1724 was fixed to prefer the profile client/error).

🐛 Proposed fix
-	recoveredClient, errRecover := lc.recoverClientAfterAuthError(ctx, failedClient, err)
+	recoveredClient, errRecover := lc.recoverClientAfterAuthError(ctx, profileClient, profileErr)

1946-1959: LGTM!

Also applies to: 1981-2003, 2065-2103

pkg/connector/forced_logout_test.go (1)

53-94: LGTM!

pkg/connector/sync_test.go (2)

896-923: LGTM!


925-947: 🩺 Stability & Availability

UserLogin is nil-safe on this path markLoggedOutByOtherClientLocked returns early when lc.UserLogin == nil, so this test won’t hit a nil-pointer panic.

			> Likely an incorrect or invalid review comment.

@highesttt
highesttt merged commit 50a04f8 into main Jul 30, 2026
10 checks passed
@highesttt
highesttt deleted the highest/plat-38184 branch July 30, 2026 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

1 participant