fix: receive loop could break if re-auth was called more than once at…#206
Conversation
|
📝 WalkthroughSummary by CodeRabbit
WalkthroughThis PR refactors the LINE SSE polling loop in ChangesSSE Reconnect Refactor
Estimated code review effort: 2 (Simple) | ~12 minutes Silent Pure-Go Channel Cache
Estimated code review effort: 1 (Trivial) | ~5 minutes Sequence Diagram(s)sequenceDiagram
participant pollLoop
participant getLastOpRevisionWithClient
participant listenSSEWithClient
pollLoop->>listenSSEWithClient: attempt SSE (first client, token A)
listenSSEWithClient-->>pollLoop: io.EOF (disconnect)
pollLoop->>pollLoop: sleep(sseReconnectDelay)
pollLoop->>getLastOpRevisionWithClient: recreate client, re-seed localRev
getLastOpRevisionWithClient-->>pollLoop: localRev
pollLoop->>listenSSEWithClient: attempt SSE (new client, token B)
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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}
| var ( | ||
| getLastOpRevisionWithClient = func(client *line.Client) (int64, error) { | ||
| return client.GetLastOpRevision() | ||
| } |
There was a problem hiding this comment.
Nit — ctx should come first: Go convention is to pass context.Context as the first parameter of a function. The underlying client.ListenSSE(ctx, localRev, handler) already follows that convention, so this wrapper reads a bit awkward. Consider reordering to func(ctx context.Context, client *line.Client, localRev int64, handler func(eventType, data string)) error for consistency.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
pkg/connector/sync_test.go (1)
15-63: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest doesn't cover concurrent re-auth triggering described in the PR.
This test validates that a fresh client picks up a refreshed token across a sequential reconnect (EOF → new token → second attempt). It doesn't simulate the "re-auth triggered more than once at the same time" scenario from the PR title — e.g., overlapping token-recovery calls from
getLastOpRevisionWithClient's recovery path and the SSE loop's recovery path, or concurrent recoverToken invocations from another goroutine. Consider adding a case that exercises overlapping/duplicate recovery triggers to directly cover the regression this PR claims to fix.🤖 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_test.go` around lines 15 - 63, The current test in pollLoop only covers a sequential reconnect path and misses the concurrent re-auth race described in the PR. Extend TestPollLoopRebuildsSSEClientAfterReconnect (or add a companion test) to simulate overlapping recovery triggers from pollLoop, getLastOpRevisionWithClient, and listenSSEWithClient so multiple recoverToken calls can run at the same time. Verify LineClient.pollLoop still rebuilds the SSE client correctly and all attempts see the refreshed token instead of only validating the simple EOF-then-retry flow.pkg/runner.go (1)
484-491: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSilent failure removes observability into pure-Go cache misses.
Errors from
ltsm.NewChannelare now fully discarded with no trace. Since this cache is best-effort, complete silence is reasonable for individual failures, but consider a debug-levelzerologlog (per repo convention) so persistent failures are still discoverable without reverting tofmt.Printfspam.♻️ Suggested tweak
if goKey, ok := r.goKeys[keyID]; ok && goKey.privKey != nil { goChan, err := ltsm.NewChannel(goKey.privKey, peerPubBytes) - if err == nil { - r.goChannels[id] = goChan + if err != nil { + log.Debug().Err(err).Int("key_id", keyID).Msg("pure-Go channel cache creation failed") + } else { + r.goChannels[id] = goChan } }As per coding guidelines, "Use
zerologfor logging throughout the codebase."🤖 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/runner.go` around lines 484 - 491, The pure-Go cache path in runner.go silently drops failures from ltsm.NewChannel, which makes repeated cache misses hard to discover. Keep the best-effort behavior in r.goChannels, but in the goKey/privKey block add a debug-level zerolog log when NewChannel returns an error, using the existing logger pattern in this package so failures are observable without changing control flow.Source: Coding guidelines
pkg/runner_channel_test.go (1)
60-79: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePipe writer isn't closed if
fn()panics.If
fn()panics,deferrestoresos.StdoutbutwritePipeis never closed, leaving theio.Copygoroutine at Lines 59-62 blocked forever. Low practical risk here sincefnonly callsChannelCreate, but worth hardening with adeferfor the pipe close/goroutine drain.🤖 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/runner_channel_test.go` around lines 60 - 79, The stdout capture in the helper around fn(), os.Stdout, and writePipe is not panic-safe because writePipe is only closed after fn returns normally, which can leave the io.Copy goroutine blocked forever if fn panics. Move the pipe close and goroutine drain into a defer alongside restoring oldStdout so the writer is always closed and the done channel is always read, even when ChannelCreate panics.
🤖 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_test.go`:
- Around line 15-63: The current test in pollLoop only covers a sequential
reconnect path and misses the concurrent re-auth race described in the PR.
Extend TestPollLoopRebuildsSSEClientAfterReconnect (or add a companion test) to
simulate overlapping recovery triggers from pollLoop,
getLastOpRevisionWithClient, and listenSSEWithClient so multiple recoverToken
calls can run at the same time. Verify LineClient.pollLoop still rebuilds the
SSE client correctly and all attempts see the refreshed token instead of only
validating the simple EOF-then-retry flow.
In `@pkg/runner_channel_test.go`:
- Around line 60-79: The stdout capture in the helper around fn(), os.Stdout,
and writePipe is not panic-safe because writePipe is only closed after fn
returns normally, which can leave the io.Copy goroutine blocked forever if fn
panics. Move the pipe close and goroutine drain into a defer alongside restoring
oldStdout so the writer is always closed and the done channel is always read,
even when ChannelCreate panics.
In `@pkg/runner.go`:
- Around line 484-491: The pure-Go cache path in runner.go silently drops
failures from ltsm.NewChannel, which makes repeated cache misses hard to
discover. Keep the best-effort behavior in r.goChannels, but in the
goKey/privKey block add a debug-level zerolog log when NewChannel returns an
error, using the existing logger pattern in this package so failures are
observable without changing control flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 33845e41-2dbb-43cd-8c40-209048050f1d
📒 Files selected for processing (4)
pkg/connector/sync.gopkg/connector/sync_test.gopkg/runner.gopkg/runner_channel_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 (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/sync_test.gopkg/runner_channel_test.gopkg/runner.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/sync_test.gopkg/runner_channel_test.gopkg/runner.gopkg/connector/sync.go
🔇 Additional comments (3)
pkg/connector/sync.go (2)
37-46: LGTM!Also applies to: 1163-1171, 1265-1267
1236-1237: 🚀 Performance & ScalabilityCheck whether
newClient()is cheap and side-effect free. It now runs on every loop pass, so any network setup, connection pooling, or other side effects would add avoidable reconnect overhead.pkg/runner_channel_test.go (1)
10-46: LGTM!
1 participant
… a time