Repository navigation
fix: reconnect agent after ELS restarts - #14
Conversation
|
Warning Review limit reachedNext included review available in 10 minutes. View limit detailsLimit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (9)
💤 Files with no reviewable changes (1)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request replaces the Visual Studio solution with an ChangesWebSocket reconnection behavior
Merge Risk: ⚪ Minimal · up to The PR adds reconnect behavior and related tests without any identified merge-blocking risk at the current head; it is merge-ready after normal checks and review. Sequence Diagram(s)sequenceDiagram
participant FbWebSocket
participant WebSocketStubServer
participant OnClosed
loop Normal closure
FbWebSocket->>WebSocketStubServer: Connect to /streaming
WebSocketStubServer-->>FbWebSocket: Close connection
FbWebSocket->>FbWebSocket: ShouldReconnect
end
FbWebSocket->>WebSocketStubServer: Connect to /streaming
WebSocketStubServer-->>FbWebSocket: Close with status 4003
FbWebSocket->>OnClosed: Report close status 4003
FbWebSocket->>FbWebSocket: Stop reconnecting
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 17 functions across 5 files. (3 skipped: 3 unsupported.) ✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
@greptileai review this |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Pull request overview
This PR adjusts the FbWebSocket reconnection decision logic so the agent will reconnect after eligible connection closures (including normal closure), while avoiding reconnect when the client is stopping or the server explicitly rejects the request. It also introduces new unit/integration tests and supporting test infrastructure to validate the reconnection behavior.
Changes:
- Updated
FbWebSocket.ShouldReconnectto reconnect on normal closures, but not when stopping or when close code4003is received. - Added unit + integration tests covering reconnect/no-reconnect scenarios, including a WebSocket stub server for integration testing.
- Replaced the legacy
.slnwith an XML-based.slnxincluding both new test projects.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tests/Api.UnitTests/Transport/ReconnectTests.cs | Unit coverage for FbWebSocket.ShouldReconnect logic (stop requested, normal closure, invalid request code). |
| tests/Api.UnitTests/Api.UnitTests.csproj | Adds a unit test project targeting net10.0 and referencing the API project. |
| tests/Api.IntegrationTests/Transport/WebSocketStubServer.cs | Adds an in-process WebSocket stub server used by integration tests. |
| tests/Api.IntegrationTests/Transport/WebSocketReconnectTests.cs | Integration coverage validating reconnect on normal closure and no reconnect on invalid-request close. |
| tests/Api.IntegrationTests/Api.IntegrationTests.csproj | Adds an integration test project (ASP.NET Core framework reference) targeting net10.0. |
| src/Api/Transport/FbWebSocket.cs | Implements updated reconnect decision logic and exposes it for testability. |
| src/Api/Properties/AssemblyInfo.cs | Adds InternalsVisibleTo so tests can call internal APIs. |
| FeatBit.Agent.slnx | New solution format including src + test projects. |
| FeatBit.Agent.sln | Removed legacy Visual Studio solution file. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Suppressed comments (1)
tests/Api.IntegrationTests/Transport/WebSocketReconnectTests.cs:93
- The fixed Task.Delay(250ms) makes this assertion timing-dependent and can cause flakes (either reconnect happens after the delay, or the delay is longer than necessary). Poll ConnectionCount for a short bounded window to assert it never exceeds the initial value after stopping during reconnect.
await reconnecting.Task.WaitAsync(TimeSpan.FromSeconds(5));
await Task.Delay(TimeSpan.FromMilliseconds(250));
Assert.Equal(1, server.ConnectionCount);
}
Theoretically possible, but |
|
Scripts to validate locally Reproducible On Main Branch node scripts\reconnect-test.mjs `
--branch main `
--test-duration 300 `
--downtime 2 `
--recovery-timeout 90 `
--relay-proxy-name "local rp" `
--agent-id "local-test-new-version"Unreproducible On Fix Branch node scripts\reconnect-test.mjs `
--branch fix/does-not-reconnect-when-ELS-restarts `
--test-duration 300 `
--downtime 2 `
--recovery-timeout 90 `
--relay-proxy-name "local rp" `
--agent-id "local-test-new-version" |
fixed featbit/featbit#966
Summary by CodeRabbit
Bug Fixes
Tests
Greptile Summary
The PR updates WebSocket reconnection behavior so eligible closures reconnect repeatedly, shutdown cancels connection attempts, and server rejection prevents retries.
Confidence Score: 4/5
The PR is not yet safe to merge because reconnect work can still publish a connection and trigger synchronization after shutdown begins.
CloseAsync cancels the stop token but neither tracks nor excludes detached reconnect work; ConnectAsync can pass its final cancellation check immediately before shutdown and then invoke OnConnected afterward.
Files Needing Attention: src/Api/Transport/FbWebSocket.cs
Important Files Changed
Sequence Diagram
sequenceDiagram participant Peer participant Socket as FbWebSocket participant Reconnect as ReconnectAsync participant Stop as CloseAsync participant Consumer as OnConnected consumer Peer->>Socket: Close frame Socket->>Reconnect: Start detached reconnect Reconnect->>Socket: ConnectAsync Socket->>Socket: Connect succeeds Socket->>Socket: Check stop token Stop->>Socket: Cancel stop token Socket->>Consumer: Invoke OnConnected Socket->>Socket: Start pipe with cancelled tokenReviews (2): Last reviewed commit: "pr feedback" | Re-trigger Greptile