Repository navigation
Report subscriber reconnect state - #67
Merged
Merged
Conversation
Subscriber reconnect and route replay already restored delivery, but outages were invisible to applications. Publish Reconnecting and Connected around replay.\n\nCloses #5
📝 WalkthroughWalkthroughThe change adds slot-specific reconnect notifications, coordinates subscription and command recovery states in ChangesConnection state recovery
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant RedisConnection
participant SubscriptionHub
participant ClientCore
participant RespireClient
RedisConnection->>SubscriptionHub: Connection closes
SubscriptionHub->>ClientCore: Reconnecting
RedisConnection->>SubscriptionHub: Replacement connection
SubscriptionHub->>SubscriptionHub: Resubscribe channels
SubscriptionHub->>ClientCore: Connected
ClientCore->>RespireClient: Aggregate ConnectionStateChanged
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78f059a37
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .ConfigureAwait(false); | ||
| } | ||
|
|
||
| core.Multiplexer.NotifyStateChanged(RespireConnectionState.Connected); |
There was a problem hiding this comment.
When the replacement subscriber connection closes after the final resubscribe acknowledgment but before this notification, its newly armed watcher can emit Reconnecting first and then this older watcher emits Connected for the already-dead connection. If the subsequent reconnect attempts fail, consumers are left with Connected as the latest state throughout the outage, defeating the delivery-gap signal this change adds. Tie the notification to the current live connection/generation so a superseded watcher cannot report restoration.
Useful? React with 👍 / 👎.
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f4cfc7ba67
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| && ReferenceEquals(Volatile.Read(ref _connection), replacement) | ||
| && replacement.IsConnected) | ||
| { | ||
| core.Multiplexer.NotifyStateChanged(RespireConnectionState.Connected); |
There was a problem hiding this comment.
When an outage drops both the command socket and the subscriber, the command reconnect attempt can fail and remain pending until the next command while the subscriber's retry loop later succeeds. This notification then publishes Connected through the shared public event even though the command socket is still dead and IsConnected is false; because the event contains no connection identity, health consumers can incorrectly treat the client as restored. Coordinate reconnect state across the multiplexer and subscriber rather than independently announcing Connected.
Useful? React with 👍 / 👎.
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c195a4b99c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| else | ||
| { | ||
| _commandState = state; |
There was a problem hiding this comment.
When Connections > 1 and two command slots reconnect concurrently, RespireConnectionMultiplexer.ScheduleReconnect emits Reconnecting for each slot and ReconnectAsync emits Connected as each slot recovers, but this single _commandState value collapses both reconnecting notifications. The first recovered slot therefore changes the aggregate to Connected while the other slot is still dead, and the second recovery is suppressed; track a reconnect count or per-slot state before publishing restoration.
Useful? React with 👍 / 👎.
|
|
||
| try | ||
| { | ||
| handlers?.Invoke(aggregate); |
There was a problem hiding this comment.
When command recovery and subscriber failure overlap, a command Connected notification can update _publishedState and pause after releasing _stateGate; the subscriber then updates the aggregate and publishes Reconnecting, after which this invocation publishes the stale Connected value. Unlike the watcher-generation case, the new cross-component aggregator therefore still lets consumers end an active subscriber outage with Connected as their latest event; serialize event delivery in the same order as aggregate transitions.
Useful? React with 👍 / 👎.
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c0377e911e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+312
to
+316
| if (reconnectGeneration == Volatile.Read(ref _reconnectGeneration) | ||
| && ReferenceEquals(Volatile.Read(ref _connection), replacement) | ||
| && replacement.IsConnected) | ||
| { | ||
| core.NotifySubscriptionStateChanged(RespireConnectionState.Connected); |
There was a problem hiding this comment.
Fresh evidence in the revised code is the remaining check-to-notification window: if the replacement closes after these generation/current/health checks pass, its watcher can publish Reconnecting before this watcher reaches NotifySubscriptionStateChanged, after which this stale invocation publishes Connected and leaves consumers seeing a healthy state during the outage. Make generation validation and publication atomic with respect to newer watcher transitions.
Useful? React with 👍 / 👎.
Comment on lines
+166
to
+168
| states.Should().ContainInOrder( | ||
| RespireConnectionState.Reconnecting, | ||
| RespireConnectionState.Connected); |
There was a problem hiding this comment.
When Redis activates the resubscription, a subsequent message can be processed before the reconnect watcher resumes from the subscription acknowledgment and emits Connected; receiving after-reconnect therefore does not guarantee that this list already contains the restoration event. Under that scheduling the assertion flakes, so wait with a timeout for the expected state transition instead of checking immediately after message delivery.
Useful? React with 👍 / 👎.
Guard generation validation and state publication with one lock, and await the Connected event in the integration test.
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 42919cc1be
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+288
to
+291
| lock (_reconnectStateGate) | ||
| { | ||
| reconnectGeneration = ++_reconnectGeneration; | ||
| core.NotifySubscriptionStateChanged(RespireConnectionState.Reconnecting); |
There was a problem hiding this comment.
If the subscriber closes and this watcher passes the _disposed check just before DisposeAsync sets the flag, disposal can complete before the watcher enters this block; the fire-and-forget watcher then publishes Reconnecting, exits at the subsequent while (!_disposed) check, and never publishes Connected. A health listener can therefore receive a permanent reconnecting state, potentially even after DisposeAsync has returned. Synchronize disposal with this transition or revalidate disposal while publishing it.
Useful? React with 👍 / 👎.
Synchronize disposal with reconnect state transitions so background watchers cannot publish after hub teardown.
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cea61f818
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+295
to
+296
| reconnectGeneration = ++_reconnectGeneration; | ||
| core.NotifySubscriptionStateChanged(RespireConnectionState.Reconnecting); |
There was a problem hiding this comment.
When a ConnectionStateChanged handler acquires an application lock that is already held by a thread concurrently disposing the client, this call executes that handler while _reconnectStateGate is held, while SubscriptionHub.DisposeAsync waits for the same gate; the handler and disposer can therefore deadlock. The Connected notification below has the same issue. Record/enqueue the transition under this gate, but drain the public callbacks only after releasing it.
Useful? React with 👍 / 👎.
…ber-resilience # Conflicts: # src/Respire/PubSub/SubscriptionHub.cs
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
Contributor
🧪 Integration Test Results (net10.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
Redis Container Benchmark Results (net10.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/Respire/Internal/ClientCore.cs (1)
21-21: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
_publishedStateto reflect that it tracks the last queued state.
QueueAggregateStateLockedassigns_publishedStatewhen it enqueues, not whenPublishQueuedStatesinvokes handlers. The deduplication logic is correct, but the name suggests the value tracks delivered states._lastQueuedStatedescribes the actual role.🤖 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 `@src/Respire/Internal/ClientCore.cs` at line 21, Rename the RespireConnectionState field _publishedState to _lastQueuedState throughout ClientCore, including assignments and deduplication checks in QueueAggregateStateLocked and any reads in PublishQueuedStates or related logic. Preserve the existing state-tracking behavior.src/Respire/PubSub/SubscriptionHub.cs (1)
343-361: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd a
_disposedre-check before queuingConnected.The
Reconnectingpath at lines 297-306 re-checks_disposedinside_reconnectStateGate. ThisConnectedpath does not. IfDisposeAsyncclears the queue just before this block acquires the gate, this watcher enqueues and publishesConnectedafter disposal. TodaySubscriptionHubis only disposed byClientCore.DisposeAsync, so the impact is limited to shutdown. The check keeps both paths symmetric and protects against future callers that dispose the hub independently.♻️ Proposed symmetry fix
lock (_reconnectStateGate) { - if (reconnectGeneration == _reconnectGeneration + if (!_disposed + && reconnectGeneration == _reconnectGeneration && ReferenceEquals(Volatile.Read(ref _connection), replacement) && replacement.IsConnected) { publishState = QueueReconnectStateLocked(RespireConnectionState.Connected); } }🤖 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 `@src/Respire/PubSub/SubscriptionHub.cs` around lines 343 - 361, Add a `_disposed` check inside the `_reconnectStateGate` critical section in the Connected path before calling `QueueReconnectStateLocked`, matching the existing Reconnecting path guard. Only queue and publish `Connected` when the hub is not disposed, the reconnect generation and connection still match, and the replacement remains connected.
🤖 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 `@src/Respire/RespireClient.cs`:
- Around line 76-81: Update the XML summary for
RespireClient.ConnectionStateChanged to document that it reflects aggregate
state across all command slots and the dedicated subscriber connection, with
Connected published only after every reconnecting component has recovered; align
the wording with RespireConnectionMultiplexer.ConnectionStateChanged.
In `@tests/Respire.Tests/Networking/ClientCoreTests.cs`:
- Around line 25-26: Update the transition-sequence assertion in ClientCoreTests
to use the order-sensitive CollectionOrdering.Matching option with
IsEquivalentTo, ensuring Reconnecting is asserted before Connected. Apply the
same ordering option to each transition-sequence assertion in the test file.
---
Nitpick comments:
In `@src/Respire/Internal/ClientCore.cs`:
- Line 21: Rename the RespireConnectionState field _publishedState to
_lastQueuedState throughout ClientCore, including assignments and deduplication
checks in QueueAggregateStateLocked and any reads in PublishQueuedStates or
related logic. Preserve the existing state-tracking behavior.
In `@src/Respire/PubSub/SubscriptionHub.cs`:
- Around line 343-361: Add a `_disposed` check inside the `_reconnectStateGate`
critical section in the Connected path before calling
`QueueReconnectStateLocked`, matching the existing Reconnecting path guard. Only
queue and publish `Connected` when the hub is not disposed, the reconnect
generation and connection still match, and the replacement remains connected.
🪄 Autofix
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 Plus
Run ID: c09547e2-6040-4d68-a964-d0675073c02b
📒 Files selected for processing (6)
src/Respire/Infrastructure/RespireConnectionMultiplexer.cssrc/Respire/Internal/ClientCore.cssrc/Respire/PubSub/SubscriptionHub.cssrc/Respire/RespireClient.cstests/Respire.IntegrationTests/PubSubIntegrationTests.cstests/Respire.Tests/Networking/ClientCoreTests.cs
Comment on lines
76
to
81
| /// <summary>Raised when a dead connection is noticed and again when its replacement lands.</summary> | ||
| public event Action<RespireConnectionState>? ConnectionStateChanged | ||
| { | ||
| add => _core.Multiplexer.StateChanged += value; | ||
| remove => _core.Multiplexer.StateChanged -= value; | ||
| add => _core.ConnectionStateChanged += value; | ||
| remove => _core.ConnectionStateChanged -= value; | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Update the XML doc to describe the aggregate semantics.
The event now forwards ClientCore.ConnectionStateChanged, which aggregates every command slot and the dedicated subscriber connection. Connected is published only after all reconnecting parts recover. The current summary still describes a single dead connection and its replacement. The equivalent doc in src/Respire/Infrastructure/RespireConnectionMultiplexer.cs at line 44 was updated; this public one was not.
📝 Proposed doc update
- /// <summary>Raised when a dead connection is noticed and again when its replacement lands.</summary>
+ /// <summary>
+ /// Raised when any client-owned connection (a command connection or the dedicated
+ /// subscriber connection) is noticed dead, and again once every affected connection is
+ /// restored and its subscriptions are replayed.
+ /// </summary>
public event Action<RespireConnectionState>? ConnectionStateChanged📝 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
| /// <summary>Raised when a dead connection is noticed and again when its replacement lands.</summary> | |
| public event Action<RespireConnectionState>? ConnectionStateChanged | |
| { | |
| add => _core.Multiplexer.StateChanged += value; | |
| remove => _core.Multiplexer.StateChanged -= value; | |
| add => _core.ConnectionStateChanged += value; | |
| remove => _core.ConnectionStateChanged -= value; | |
| } | |
| /// <summary> | |
| /// Raised when any client-owned connection (a command connection or the dedicated | |
| /// subscriber connection) is noticed dead, and again once every affected connection is | |
| /// restored and its subscriptions are replayed. | |
| /// </summary> | |
| public event Action<RespireConnectionState>? ConnectionStateChanged | |
| { | |
| add => _core.ConnectionStateChanged += value; | |
| remove => _core.ConnectionStateChanged -= value; | |
| } |
🤖 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 `@src/Respire/RespireClient.cs` around lines 76 - 81, Update the XML summary
for RespireClient.ConnectionStateChanged to document that it reflects aggregate
state across all command slots and the dedicated subscriber connection, with
Connected published only after every reconnecting component has recovered; align
the wording with RespireConnectionMultiplexer.ConnectionStateChanged.
Comment on lines
+25
to
+26
| await Assert.That(states).IsEquivalentTo( | ||
| [RespireConnectionState.Reconnecting, RespireConnectionState.Connected]); |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
TUnit assertions IsEquivalentTo collection order sensitivity
💡 Result:
In the TUnit testing framework, the IsEquivalentTo assertion is order-insensitive by default [1][2]. This means that the assertion checks whether two collections contain the same elements with the same frequency, regardless of their position [2]. To perform an order-sensitive comparison, you must explicitly pass CollectionOrdering.Matching as a parameter to the IsEquivalentTo method [1][2]. Examples of usage: Order-agnostic comparison (default): await Assert.That(new[] { 1, 2, 3 }).IsEquivalentTo(new[] { 3, 2, 1 }); // Passes [1] Order-sensitive comparison: await Assert.That(new[] { 1, 2, 3 }).IsEquivalentTo(new[] { 1, 2, 3 }, CollectionOrdering.Matching); // Passes [1][2] await Assert.That(new[] { 1, 2, 3 }).IsEquivalentTo(new[] { 3, 2, 1 }, CollectionOrdering.Matching); // Fails [1] There is no IgnoringOrder() method in TUnit; because order is already ignored by default, such a method is unnecessary [1].
Citations:
- 1: Fix IsEquivalentTo documentation - remove non-existent IgnoringOrder method TUnit#3718
- 2: https://tunit.dev/docs/assertions/collections/
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- candidate test ---'
sed -n '1,90p' tests/Respire.Tests/Networking/ClientCoreTests.cs
printf '%s\n' '--- TUnit references and ordering options ---'
rg -n -i --glob '*.csproj' --glob '*.props' --glob '*.targets' --glob '*.cs' \
'TUnit|CollectionOrdering|IsEquivalentTo' .Repository: thomhurst/Respire
Length of output: 14169
Use an order-sensitive collection assertion. TUnit’s IsEquivalentTo ignores element order by default. Pass CollectionOrdering.Matching to each transition-sequence assertion so the tests require Reconnecting before Connected.
🤖 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 `@tests/Respire.Tests/Networking/ClientCoreTests.cs` around lines 25 - 26,
Update the transition-sequence assertion in ClientCoreTests to use the
order-sensitive CollectionOrdering.Matching option with IsEquivalentTo, ensuring
Reconnecting is asserted before Connected. Apply the same ordering option to
each transition-sequence assertion in the test file.
Contributor
Redis Container Benchmark Results (net8.0)BenchmarkDotNet v0.15.8, Linux Ubuntu 24.04.4 LTS (Noble Numbat) Runtime=.NET 8.0 IterationCount=3 WarmupCount=1
View raw benchmark output |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 956107409a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
|
|
||
| core.NotifySubscriptionStateChanged(state); |
There was a problem hiding this comment.
Fresh evidence in the revised queue is that a state is removed under _reconnectStateGate but forwarded only after releasing the gate. If DisposeAsync runs in that window, it sets _disposed, clears only the states still queued, and can return before this call forwards the already-dequeued Reconnecting or Connected; observers can therefore receive a transition after disposal, and a dequeued Reconnecting can become permanent because its matching Connected was cleared. Coordinate disposal with the in-flight publisher or revalidate disposal before forwarding the state.
Useful? React with 👍 / 👎.
This was referenced Aug 9, 2026
This branch was previously deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Reconnectingwhen the dedicated subscriber connection diesConnectedonly after all active channel/pattern/sharded routes are replayedRoot cause
The subscriber already reconnected with exponential backoff and replayed its route registry, but that path bypassed the public
ConnectionStateChangedevent. Applications could not detect the message-delivery gap requested by the issue.Validation
dotnet test tests/Respire.IntegrationTests/Respire.IntegrationTests.csproj -c Release --no-restoreReconnecting→Connected, and delivery after replayCloses #5
Summary by CodeRabbit