Highest/fix reconnect#214
Conversation
|
|
Caution Review failedThe pull request is closed. ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Disabled knowledge base sources:
📝 WalkthroughSummary by CodeRabbit
WalkthroughThe PR adds context-aware LINE RPC calls, cancellable client runs, coordinated token recovery and forced logout handling, login replacement retirement, and independent receive-auth probing across SSE reconnects. Tests cover lifecycle races, cancellation, session invalidation, bridge-state behavior, and probe scheduling. ChangesLINE RPC cancellation
Client authentication lifecycle
Receive authentication probing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Connect
participant LineClient
participant recoverTokenWith
participant markLoggedOutByOtherClient
participant pollLoop
Connect->>LineClient: begin cancellable run
LineClient->>recoverTokenWith: validate or recover token
pollLoop->>LineClient: run receive-auth probe
markLoggedOutByOtherClient->>LineClient: invalidate session and clear tokens
LineClient-->>Connect: cancel active work
recoverTokenWith-->>pollLoop: return cancellation or invalidation
Possibly related PRs
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (1 warning, 2 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning Tools execution failed with the following error: Failed to run tools: 13 INTERNAL: Received RST_STREAM with code 2 (Internal server error) Comment |
![indent-zero[bot]](https://avatars.githubusercontent.com/in/1426746?s=60&v=4){kind=small}
| // Keep connection state in Beeper's bridge status API only. Framework status | ||
| // notices call GetManagementRoom, which creates the LINE bot room when the | ||
| // user doesn't already have one. | ||
| bridge.Config.BridgeStatusNotices = "none" |
There was a problem hiding this comment.
Consider only forcing "none" when the user hasn't configured a value — as written this silently overrides errors/all for anyone who explicitly configured management-room notices, and there's no config knob to opt back in. Something like:
| bridge.Config.BridgeStatusNotices = "none" | |
| if bridge.Config.BridgeStatusNotices == "" { | |
| bridge.Config.BridgeStatusNotices = "none" | |
| } |
If the intent really is to force-off for all deployments regardless of user config, that's fine but probably worth a doc note; if not, this preserves user intent while still defaulting off. connector_test.TestInitDisablesManagementRoomBridgeStatusNotices will need to be adjusted.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/connector/sync.go (1)
1324-1327: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRedundant branch: both outcomes
continue.
waitForSSEReconnect(receiveCtx)returnsfalse(ctx done: parent-cancel or probe-due) ortrue(delay elapsed), but both pathscontinue, so the return value is discarded. This is correct (the loop top re-checksctx.Err()andisReceiveAuthProbeDue), but the split reads like a missingreturnand invites incorrect future edits in this reconnect path. Collapse it.♻️ Simplify the idle-timeout reconnect path
if line.IsSSEIdleTimeout(err) { if probeAndReschedule() { return } - if !waitForSSEReconnect(receiveCtx) { - continue - } + waitForSSEReconnect(receiveCtx) continue }🤖 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 1324 - 1327, Collapse the redundant conditional in the reconnect path by removing the `if !waitForSSEReconnect(receiveCtx)` branch and invoking `waitForSSEReconnect(receiveCtx)` directly, followed by the existing `continue`; preserve the loop behavior and subsequent context/probe checks.
🤖 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.
Nitpick comments:
In `@pkg/connector/sync.go`:
- Around line 1324-1327: Collapse the redundant conditional in the reconnect
path by removing the `if !waitForSSEReconnect(receiveCtx)` branch and invoking
`waitForSSEReconnect(receiveCtx)` directly, followed by the existing `continue`;
preserve the loop behavior and subsequent context/probe checks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e89d0074-7995-478d-a18d-486c700a15d5
📒 Files selected for processing (12)
pkg/connector/auth_recovery_test.gopkg/connector/client.gopkg/connector/client_lifecycle_test.gopkg/connector/connector.gopkg/connector/connector_test.gopkg/connector/forced_logout_test.gopkg/connector/sync.gopkg/connector/sync_test.gopkg/line/client.gopkg/line/methods.gopkg/line/methods_context_test.gopkg/line/sse_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (4)
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
- GitHub Check: Lint with 1.25
- GitHub Check: build-docker
🧰 Additional context used
📓 Path-based instructions (3)
**/*.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/line/methods_context_test.gopkg/line/sse_test.gopkg/line/client.gopkg/connector/connector_test.gopkg/connector/auth_recovery_test.gopkg/line/methods.gopkg/connector/client_lifecycle_test.gopkg/connector/sync.gopkg/connector/sync_test.gopkg/connector/client.gopkg/connector/connector.gopkg/connector/forced_logout_test.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/line/methods_context_test.gopkg/line/sse_test.gopkg/line/client.gopkg/connector/connector_test.gopkg/connector/auth_recovery_test.gopkg/line/methods.gopkg/connector/client_lifecycle_test.gopkg/connector/sync.gopkg/connector/sync_test.gopkg/connector/client.gopkg/connector/connector.gopkg/connector/forced_logout_test.go
pkg/connector/connector.go
📄 CodeRabbit inference engine (AGENTS.md)
Implement
bridgev2.NetworkConnectorandbridgev2.NetworkAPIinterfaces in the connector package for bridge logic
Files:
pkg/connector/connector.go
🔇 Additional comments (32)
pkg/connector/sync.go (5)
1164-1196: LGTM!
1198-1238: LGTM!
1291-1323: LGTM!Also applies to: 1341-1346
1348-1395: LGTM!
1400-1444: LGTM!pkg/connector/sync_test.go (5)
16-95: LGTM!
107-107: LGTM!Also applies to: 157-162, 207-207, 260-260
317-424: LGTM!
426-518: LGTM!
520-526: LGTM!Also applies to: 552-552, 576-613
pkg/line/client.go (1)
313-382: LGTM!pkg/line/methods.go (1)
4-4: LGTM!Also applies to: 103-124, 618-646
pkg/line/methods_context_test.go (1)
1-58: LGTM!pkg/line/sse_test.go (2)
76-137: LGTM! Solid coverage of cancellation-with-cause semantics and heartbeat consumption timing.
107-111: 🎯 Functional CorrectnessConfirm the module targets Go 1.22+
for range 5depends on range-over-integer syntax, so this test needs the module’sgodirective to be 1.22 or newer.pkg/connector/connector_test.go (1)
1-167: LGTM!pkg/connector/client.go (5)
62-67: LGTM!Also applies to: 86-89, 155-198
316-328: LGTM!Forced-logout/connected-state gating correctly serializes via
recoverMu, and thesuperseded/UserLogin == nilguards prevent a stale/retired client from mutating a replacement's sharedUserLoginstate (validated byTestStaleForcedLogoutCancelsOnlyStaleClient).Also applies to: 330-381
507-654: LGTM!
Connect's cancellable-run wiring (beginRun,workersStartedguard,ctx.Err()checks around token validation/startup) correctly ties worker startup to the run context and cleans up viarun.cancel()on early return.
776-812: LGTM!
ensureValidTokenWith/Disconnectcorrectly delegate to the sharedrecoverTokenWith/recoverMuserialization, andDisconnect's finalrecoverMu.Lock()/Unlock()afterwg.Wait()correctly drains any recovery/forced-logout call that began beforesupersededbecame visible.Also applies to: 814-832
443-505: 🩺 Stability & Availability
recoverMuis held across the full refresh/re-login round-trip, so a slow LINE call can blockDisconnect()/retire()on the reconnect path. Confirm this is an intentional trade-off and that both recovery calls are always bounded byctx/HTTP timeouts.pkg/connector/forced_logout_test.go (3)
5-13: LGTM!Also applies to: 27-27, 52-74
76-127: LGTM!Good deterministic race test for "forced logout wins" specifically through the
ensureValidTokenWithrefresh path, complementing the directrunTokenRecoveryversion inauth_recovery_test.go.
155-171: LGTM!Correctly asserts
ExistingLoginretention needed for the retire-before-replace flow inconnector.go'sfinishLogin.pkg/connector/auth_recovery_test.go (2)
248-262: LGTM!Also applies to: 264-281, 283-304
306-342: LGTM!Solid coverage of the "forced logout must win over an in-flight recovery" invariant via
recoverMuserialization.pkg/connector/client_lifecycle_test.go (2)
1-175: LGTM!Good coverage of
beginRun/Disconnectrace ordering (reservation-before-publish, cancel-then-join), stale-vs-current client isolation, and idempotentclaimForcedLogoutStateunder concurrent access.Also applies to: 194-194
176-193: 📐 Maintainability & Code QualityConfirm the module targets Go 1.22+ for
for range 32.for range 32needs Go 1.22+, so this test will not compile if the module or toolchain is pinned lower.pkg/connector/connector.go (4)
166-166: LGTM!Also applies to: 209-209
467-474: LGTM!Retiring the previous client before
NewLogincopies in the freshly-authenticated metadata correctly prevents a late forced-logout/recovery from the old client clobbering the replacement session (see the root-cause note onrecoverMuhold time inclient.go'srunTokenRecovery, which bounds how long this blocking call can take).
496-497: LGTM!Dropping the direct
StateConnectedemission here is consistent with moving that responsibility tosendConnectedStateIfCurrent, which correctly suppresses it for superseded/invalidated clients.
35-39: 🗄️ Data Integrity & Integration
BridgeStatusNotices = "none"is correctnoneis the documented value for disabling bridge status notices, so this line is fine.> Likely an incorrect or invalid review comment.
|
1 participant
No description provided.