[ECO-5646] Fix behaviour when internet connection is not available#1320
[ECO-5646] Fix behaviour when internet connection is not available#1320
Conversation
github-actions
bot
temporarily deployed
to
staging/pull/1320/features
- Updated code to use defaultRealtimeHost in connecting state when timed out
sacOO7
force-pushed
the
fix/primary-fallback-on-reconnection
branch
from
4c40fd1 to
957977c
Compare
WalkthroughHost selection in the realtime workflow now defaults connectingHost to the default realtime host and only switches to a fallback for non-timeout triggers; disconnect handling replaces a (retry, clearKey) tuple with an explicit clear-key action plus a single CheckInstantRetryFlag(); two tests and a test helper were added/updated. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Workflow as RealtimeWorkflow
participant Transport
rect rgba(200,230,255,0.4)
Note over Workflow: SetConnectingStateCommand
Workflow->>Workflow: connectingHost = default realtime host
alt Triggered by non-timeout message
Workflow->>Workflow: connectingHost = fallback host
else Triggered by timeout
Workflow->>Workflow: keep default host
end
Workflow->>Transport: connect(connectingHost)
end
rect rgba(230,240,220,0.4)
Note over Workflow: SetDisconnectedStateCommand
alt cmd.ClearConnectionKey == true
Workflow->>Workflow: Clear connection key explicitly
end
Workflow->>Workflow: retryInstantly = CheckInstantRetryFlag()
alt retryInstantly && CanConnectToAbly()
Workflow->>Workflow: Trigger SetConnectingStateCommand
else
Workflow->>Workflow: Remain Disconnected / schedule retry
end
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro Disabled knowledge base sources:
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧬 Code graph analysis (1)src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/AblyRealtimeTestExtensions.cs (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
🔇 Additional comments (2)
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 |
github-actions
bot
temporarily deployed
to
staging/pull/1320/features
github-actions
bot
temporarily deployed
to
staging/pull/1320/features
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs (1)
346-385: Consider reducing the ConditionalAwaiter timeout and adding explanatory comments.The test logic correctly validates that the client always tries the default host first after non-recoverable disconnects. However, there are a few concerns:
Timeout value: The
ConditionalAwaitertimeout of 120 seconds (line 374) is very conservative—6× theConnectionStateTtlof 20 seconds. The actual wait should be around 20-25 seconds based on the retry configuration. Consider reducing this to 30-40 seconds to fail faster if something goes wrong, improving test suite performance.Test complexity: The test uses an event handler to create a disconnect injection loop, relying on
ConnectionStateTtlexpiry to break the cycle. This timing-sensitive approach could be brittle. Consider adding a comment explaining the test strategy:// Strategy: Inject non-recoverable disconnects on each Disconnected->Connecting transition // to create a retry loop. The loop continues until ConnectionStateTtl expires (20s), // forcing transition to Suspended. Verify that all connection attempts use the default host.Comment precision: Line 357's comment states "limited disconnected retries upto 20 seconds" but doesn't clarify that this is achieved by reducing
ConnectionStateTtl, not by counting retries.Apply this diff to improve clarity:
- // Reduced connectionStateTTL for limited disconnected retries upto 20 seconds + // Reduce ConnectionStateTtl to 20 seconds to force Suspended state after retries client.State.Connection.ConnectionStateTtl = TimeSpan.FromSeconds(20);Consider this diff to reduce the timeout:
- await new ConditionalAwaiter(() => client.Connection.State == ConnectionState.Suspended, null, 120); + await new ConditionalAwaiter(() => client.Connection.State == ConnectionState.Suspended, null, 40);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs(3 hunks)src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs (3)
src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs (7)
Task(133-192)Task(197-252)Task(254-279)Task(287-560)Task(686-698)Task(700-915)Task(1042-1070)src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/AblyRealtimeTestExtensions.cs (3)
Task(23-31)Task(33-44)FakeProtocolMessageReceived(12-15)src/IO.Ably.Tests.Shared/Infrastructure/AblyRealtimeSpecs.cs (6)
AblyRealtime(66-72)AblyRealtime(74-85)AblyRealtime(87-95)AblyRealtime(99-105)AblyRealtime(107-113)AblyRealtime(134-141)
src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs (3)
src/IO.Ably.Shared/ClientOptions.cs (1)
FullRealtimeHost(175-197)src/IO.Ably.Shared/Realtime/AttemptsHelpers.cs (2)
AttemptsHelpers(8-79)GetHost(32-78)src/IO.Ably.Shared/Types/ErrorInfo.cs (2)
IsRetryableStatusCode(207-210)IsRetryableStatusCode(246-249)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check
🔇 Additional comments (4)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs (2)
10-10: LGTM!The new using directive is required for the
ConditionalAwaiterused in the test below.
387-393: LGTM!The helper method correctly sends a non-recoverable disconnect message (HTTP 403 Forbidden). The extraction improves test readability and follows established patterns in the codebase.
src/IO.Ably.Shared/Realtime/Workflows/RealtimeWorkflow.cs (2)
729-738: Host selection logic correctly prioritizes default host on timeout reconnects.The implementation aligns with the PR objectives by ensuring that timeout-triggered reconnection attempts always use the default host first, preventing premature fallback host cycling when internet connectivity is lost.
The timeout detection at line 735 uses the established codebase pattern for command trigger identification. String-based trigger messages via
.TriggeredBy("StateName.OnTimeOut()")are the standard convention throughout the workflow, consistently applied by all state timeout handlers (DisconnectedState, SuspendedState, ClosingState). TheContains("OnTimeOut()")check reliably distinguishes timeout-triggered commands because only timeout handlers use this pattern, and no alternative mechanism (enum, boolean flag, or constant) exists in the command structure.
800-850: Refactored retry logic correctly integrates internet connectivity check.The changes successfully implement the PR objectives:
Lines 800-803: Explicit connection key clearing is more readable than the previous tuple-based approach.
Lines 837-850:
CheckInstantRetryFlag()properly gates instant retry behind internet connectivity verification:
- Preserves explicit retry requests via
cmd.RetryInstantly- Calls
CanConnectToAbly()for retryable errors/exceptions (the "internet-up" check)- Returns false otherwise, preventing premature fallback attempts
Verification confirms
CanConnectToAbly()is production-ready:
- Uses a 4-second timeout (reasonable, non-blocking)
- Queries a dedicated internet check endpoint (
internet-up.ably-realtime.com) rather than the main API, correctly distinguishing network unavailability from Ably service issues- Handles exceptions safely, logging appropriately for debugging
- Well-tested with existing test coverage
This ensures the SDK only attempts fallbacks when internet connectivity is confirmed, preventing connections to distant fallback regions when the network is actually down.
sacOO7
force-pushed
the
fix/primary-fallback-on-reconnection
branch
from
0c15dc3 to
9ef34d3
Compare
github-actions
bot
temporarily deployed
to
staging/pull/1320/features
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs (1)
373-373: Clarify timeout units for better readability.The
ConditionalAwaitertimeout parameter120appears on lines 373 and 423. The units (milliseconds vs. seconds) are unclear from the call site alone. Consider adding a comment or using a named constant to improve test readability.Example:
-await new ConditionalAwaiter(() => client.Connection.State == ConnectionState.Suspended, null, 120); +const int timeoutSeconds = 120; +await new ConditionalAwaiter(() => client.Connection.State == ConnectionState.Suspended, null, timeoutSeconds);Also applies to: 423-423
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (2)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/AblyRealtimeTestExtensions.cs(1 hunks)src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/AblyRealtimeTestExtensions.cs (1)
src/IO.Ably.Shared/Types/ProtocolMessage.cs (4)
ProtocolMessage(18-289)ProtocolMessage(101-105)ProtocolMessage(107-111)ProtocolMessage(113-117)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs (1)
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/AblyRealtimeTestExtensions.cs (3)
Task(23-31)Task(41-52)DisconnectWithNonRetryableError(33-39)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (14)
- GitHub Check: check
- GitHub Check: check (net9.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net9.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net7.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net6.0)
- GitHub Check: check (net8.0)
- GitHub Check: check (net9.0)
- GitHub Check: check
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/AblyRealtimeTestExtensions.cs
Outdated
Show resolved
Hide resolved
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs
Show resolved
Hide resolved
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs
Outdated
Show resolved
Hide resolved
src/IO.Ably.Tests.Shared/Realtime/ConnectionSpecs/ConnectionFallbackSpecs.cs
Show resolved
Hide resolved
…retried on primary host first
sacOO7
force-pushed
the
fix/primary-fallback-on-reconnection
branch
from
9ef34d3 to
85fbc68
Compare
2 participants
Fixed #1319
Summary by CodeRabbit
Bug Fixes
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.