Repository navigation
Add command flags - #97
Merged
Merged
Conversation
thomhurst
had a problem deploying
to
Pull Requests
August 9, 2026 23:45 — with 
GitHub Actions
Failure
GitHub Actions
Failure
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe client adds execution flags, fire-and-forget APIs, cancellation overloads, command validation, improved blocking detection, cluster redirect control, telemetry handling, and integration coverage. ChangesCommand execution policies
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant RespireClient
participant RedisCluster
participant RedisNode
Client->>RespireClient: Execute command with flags
RespireClient->>RedisCluster: Route command
RedisCluster->>RedisNode: Send command
RedisNode-->>RedisCluster: Reply, MOVED, or connection close
RedisCluster-->>RespireClient: Complete, redirect, or error
RespireClient-->>Client: Return task result
Possibly related PRs
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 (net8.0)ℹ️ Execution Details
Automated integration testing with Redis containers using TUnit |
Contributor
🧪 Integration Test Results (net10.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: 7d3dd065b6
ℹ️ 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".
| return; | ||
| } | ||
|
|
||
| await SendFireAndForgetAsync(commandValue, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When the raw command is an indefinitely blocking verb such as BLPOP key 0, this path bypasses the catalog API's IsBlocking rejection and queues it on a shared multiplexed connection. The fire-and-forget call then completes immediately, while the server blocks the socket; with the default single connection and disabled response watchdog, every subsequent command stalls indefinitely. Reject known blocking raw commands or route them through a cancelable dedicated connection.
Useful? React with 👍 / 👎.
Reject known blocking raw fire-and-forget commands before they reach shared multiplexed connections. Update the IRespireClient test double so the expanded command-options surface builds in all test projects.
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 00:13 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 29ac6ed7d5
ℹ️ 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".
| CancellationToken cancellationToken) | ||
| { | ||
| var (operation, _, commandValue) = CreateRawCommand(command, args); | ||
| if (RespireCommand.IsBlocking(operation, commandValue.Arguments)) |
There was a problem hiding this comment.
When a raw fire-and-forget command such as CLIENT REPLY OFF or CLIENT REPLY SKIP reaches this check, it is accepted because it is not classified as blocking, even though it changes whether the shared connection emits replies. OFF leaves the connection permanently unable to complete later commands, while SKIP shifts the FIFO reply association after the skipped response, potentially delivering replies to the wrong callers or hanging them. Raw fire-and-forget execution should reject connection-scoped commands just as catalog execution does.
Useful? React with 👍 / 👎.
| { | ||
| var slot = command.TryGetClusterSlot(out var commandSlot) ? commandSlot : (int?)null; | ||
| var connection = await cluster.GetConnectionAsync(slot, cancellationToken).ConfigureAwait(false); | ||
| await connection.SendFireAndForgetAsync(in command, cancellationToken).ConfigureAwait(false); |
There was a problem hiding this comment.
When the cached slot owner is stale during cluster migration or resharding, this sends the command to the old node and installs a discard sentinel for its MOVED or ASK reply. Because the router never observes that reply, the command is not retried on the redirected node and the route is not updated, so writes can be silently lost—and repeated fire-and-forget calls can continue going to the wrong node until an ordinary command refreshes the route. The reply should still be processed internally for redirect handling even though no result is returned to the caller.
Useful? React with 👍 / 👎.
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 |
Reject connection-affinity raw commands before they reach shared connections. In cluster mode, consume replies through the redirect loop before discarding them so MOVED and ASK writes reach the correct node.
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 00:25 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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: c2d77f47ae
ℹ️ 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
+858
to
+860
| using var response = await SendClusterAsync( | ||
| operation, cluster, command, cancellationToken) | ||
| .ConfigureAwait(false); |
There was a problem hiding this comment.
In cluster mode, fire-and-forget SHUTDOWN reaches this path because it is neither connection-scoped nor blocking, but a successful Redis shutdown closes the connection without returning a reply. Awaiting SendClusterAsync therefore completes with RespireConnectionException, which is not caught here, even though the command succeeded; the same fire-and-forget call in non-cluster mode completes after enqueueing. Handle no-reply commands such as SHUTDOWN without awaiting a response, or otherwise suppress their expected connection closure.
Useful? React with 👍 / 👎.
Keep SHUTDOWN on the enqueue-only cluster path because successful shutdown closes the socket without a reply. Ordinary fire-and-forget commands still consume replies for MOVED and ASK handling.
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 00:39 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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 |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Respire/RespireClient.cs (1)
181-187: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winThe cluster-wide path drops
flags.
ExecuteCatalogAsyncvalidatesflagsand then forwards them to the blocking, ordinary, and stored-procedure paths. The cluster-wide branch callsSendClusterWideAsyncwithoutflags.ExecuteRawAsyncat Lines 282-288 andExecuteInterpolatedAsyncat Lines 353-359 have the same gap.A caller who passes
RespireCommandFlags.NoRedirectfor a cluster-wide mutation gets no error and no effect. Either forwardflagstoSendClusterWideAsync, or rejectNoRedirectfor cluster-wide mutations so the policy is never silently ignored.🤖 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 181 - 187, Update the cluster-wide mutation branches in ExecuteCatalogAsync, ExecuteRawAsync, and ExecuteInterpolatedAsync to preserve the validated flags when calling SendClusterWideAsync; ensure RespireCommandFlags.NoRedirect is either forwarded and honored or explicitly rejected rather than silently ignored.
🧹 Nitpick comments (6)
src/Respire/Commands/CommandStructs.cs (1)
223-223: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
Argumentsassumes a single-token verb.
DynamicCommanddoes not know how many leading tokens form the verb.CreateRawCommandinRespireClient.csbuildstokensas all verb words followed by the caller arguments. For a multi-word command such as"CONFIG SET",Argumentsreturns["SET", ...args], so the span includes a verb word.The only current consumer is
ValidateRawFireAndForgetCommand, which scans for aBLOCKargument. No multi-word verb is currently classified asBlockingWhenRequested, so no defect is reachable today. The contract is still wrong and will break if a multi-word blocking verb is added, or if another consumer treatsArgumentsas caller-supplied arguments only.Store the argument start index in
DynamicCommand, or pass the argument span explicitly at the call site inExecuteRawFireAndForgetAsync.♻️ Proposed change: carry the argument start index
-internal readonly struct DynamicCommand(RespireValue[] tokens, int routingKeyIndex) : IRespCommand +internal readonly struct DynamicCommand(RespireValue[] tokens, int routingKeyIndex, int firstArgumentIndex = 1) : IRespCommand { - internal ReadOnlySpan<RespireValue> Arguments => tokens.AsSpan(1); + internal ReadOnlySpan<RespireValue> Arguments => tokens.AsSpan(firstArgumentIndex);Then pass
words.LengthfromCreateRawCommand.🤖 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/Commands/CommandStructs.cs` at line 223, Update DynamicCommand so Arguments begins after the complete verb, not at a hardcoded single-token offset. Carry the argument start index through DynamicCommand and pass words.Length from CreateRawCommand in RespireClient, preserving caller-supplied arguments only for ValidateRawFireAndForgetCommand and other consumers.src/Respire/RespireClient.cs (3)
849-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the standalone and cluster latency difference.
For a standalone client,
SendFireAndForgetAsyncreturns after the write. For a cluster client, this method awaits the full reply so it can followMOVEDandASKredirects. The same publicExecuteFireAndForgetAsynccall therefore has round-trip latency on a cluster and no round-trip latency on a standalone server.The behavior is required for correct redirect handling. State it in the XML documentation on the public
ExecuteFireAndForgetAsyncoverloads at Lines 156-167 and Lines 260-271, so callers do not assume constant-time enqueue.Also note that
catch (RespireServerException)at Line 870 discards redirect-exhaustion errors together with ordinary command errors. Confirm that is intended.🤖 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 849 - 874, Update the XML documentation for both public ExecuteFireAndForgetAsync overloads to state that standalone execution returns after writing, while cluster execution awaits replies to process MOVED and ASK redirects and therefore incurs round-trip latency. Preserve the existing SendFireAndForgetClusterAsync behavior, and confirm that its catch (RespireServerException) intentionally suppresses both ordinary server errors and redirect-exhaustion errors.
816-836: 📐 Maintainability & Code Quality | 🔵 TrivialFire-and-forget sends bypass telemetry.
SendAsyncreachesSendOnConnectionAsync, which starts aRespireTelemetryoperation scope.SendFireAndForgetAsynccallsconnection.SendFireAndForgetAsyncdirectly, so no operation is recorded. Fire-and-forget traffic becomes invisible in metrics and traces.Consider emitting at least a counter for fire-and-forget dispatch, keyed by
operation. Theoperationparameter is already available here.🤖 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 816 - 836, Add telemetry for fire-and-forget dispatches in SendFireAndForgetAsync, using the existing operation parameter as the metric key. Ensure the counter is emitted for every dispatch path, including cluster, post-connect, and direct connection sends, without changing the existing send behavior.
303-321: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueValidation runs after
CreateRawCommandallocates.
ExecuteRawFireAndForgetAsyncbuilds the full token array first, then rejects the command. The order is functionally correct, but it allocates for commands that always throw.ExecuteCatalogFireAndForgetAsyncat Lines 216-221 validates before it buildsCatalogCommand.Validation needs
commandValue.Arguments, so the current order is a deliberate tradeoff. See the separate comment onDynamicCommand.Argumentsabout the verb-token offset that this call site depends on.🤖 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 303 - 321, Rework ExecuteRawFireAndForgetAsync so ValidateRawFireAndForgetCommand runs before CreateRawCommand allocates the full token array, while still validating against the correct command arguments and preserving the existing cluster-wide versus regular dispatch behavior. Account for the DynamicCommand.Arguments verb-token offset when obtaining the validation inputs, as noted in the related comment.src/Respire/IRespireClient.cs (2)
75-115: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdding members to a public interface breaks external implementers.
IRespireClientis public. These new members have no default implementation. Any type outside this repository that implementsIRespireClientfails to compile after this change. The update required forScriptInterceptingClientinRespireDistributedCacheTests.csshows the same break.If the package is already released, plan a major version bump, or give the new members default implementations that throw
NotSupportedException. The existingResolveKeymember at Line 70 already uses the default-implementation pattern.🤖 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/IRespireClient.cs` around lines 75 - 115, Update the newly added members of IRespireClient to use default interface implementations that throw NotSupportedException, matching the existing ResolveKey pattern, so external implementers remain source-compatible. Apply this to the added ExecuteAsync and ExecuteFireAndForgetAsync overloads without changing existing signatures or behavior for implementations that support them.
87-96: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe string overload set omits a cancellation-only form.
The catalog set provides
ExecuteAsync(RespireCommand, RespireValue[], CancellationToken)at Lines 78-79. The string set has no matchingExecuteAsync(string, RespireValue[], CancellationToken). A caller who wants cancellation on a raw string command must also passRespireCommandFlags.None.Add the missing overload for symmetry.
♻️ Proposed addition
ValueTask<RespireResult> ExecuteAsync( string command, RespireCommandFlags flags, params RespireValue[] args); + ValueTask<RespireResult> ExecuteAsync( + string command, RespireValue[] args, CancellationToken cancellationToken); + ValueTask<RespireResult> ExecuteAsync( string command, RespireValue[] args, RespireCommandFlags flags, CancellationToken cancellationToken);🤖 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/IRespireClient.cs` around lines 87 - 96, Add the missing cancellation-only string overload to IRespireClient alongside the existing ExecuteAsync(string, ...) methods: declare ExecuteAsync(string command, RespireValue[] args, CancellationToken cancellationToken), matching the catalog overload and preserving RespireCommandFlags.None as implicit for callers.
🤖 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 876-887: Update SendClusterWideFireAndForgetAsync to attempt
SendFireAndForgetAsync for every connection without aborting when an individual
send fails, matching the failure-handling behavior of SendClusterWideAsync.
Collect each send exception while continuing the fan-out, then aggregate and
propagate the failures after all master connections have been processed.
In `@tests/Respire.Tests/Networking/ClusterTests.cs`:
- Around line 86-92: In the cluster test’s fire-and-forget command flow, await
WaitForCommandsAsync(target, 2) after the second ExecuteFireAndForgetAsync call
and before asserting ReceivedCommands, ensuring the target has recorded both SET
commands before verification.
---
Outside diff comments:
In `@src/Respire/RespireClient.cs`:
- Around line 181-187: Update the cluster-wide mutation branches in
ExecuteCatalogAsync, ExecuteRawAsync, and ExecuteInterpolatedAsync to preserve
the validated flags when calling SendClusterWideAsync; ensure
RespireCommandFlags.NoRedirect is either forwarded and honored or explicitly
rejected rather than silently ignored.
---
Nitpick comments:
In `@src/Respire/Commands/CommandStructs.cs`:
- Line 223: Update DynamicCommand so Arguments begins after the complete verb,
not at a hardcoded single-token offset. Carry the argument start index through
DynamicCommand and pass words.Length from CreateRawCommand in RespireClient,
preserving caller-supplied arguments only for ValidateRawFireAndForgetCommand
and other consumers.
In `@src/Respire/IRespireClient.cs`:
- Around line 75-115: Update the newly added members of IRespireClient to use
default interface implementations that throw NotSupportedException, matching the
existing ResolveKey pattern, so external implementers remain source-compatible.
Apply this to the added ExecuteAsync and ExecuteFireAndForgetAsync overloads
without changing existing signatures or behavior for implementations that
support them.
- Around line 87-96: Add the missing cancellation-only string overload to
IRespireClient alongside the existing ExecuteAsync(string, ...) methods: declare
ExecuteAsync(string command, RespireValue[] args, CancellationToken
cancellationToken), matching the catalog overload and preserving
RespireCommandFlags.None as implicit for callers.
In `@src/Respire/RespireClient.cs`:
- Around line 849-874: Update the XML documentation for both public
ExecuteFireAndForgetAsync overloads to state that standalone execution returns
after writing, while cluster execution awaits replies to process MOVED and ASK
redirects and therefore incurs round-trip latency. Preserve the existing
SendFireAndForgetClusterAsync behavior, and confirm that its catch
(RespireServerException) intentionally suppresses both ordinary server errors
and redirect-exhaustion errors.
- Around line 816-836: Add telemetry for fire-and-forget dispatches in
SendFireAndForgetAsync, using the existing operation parameter as the metric
key. Ensure the counter is emitted for every dispatch path, including cluster,
post-connect, and direct connection sends, without changing the existing send
behavior.
- Around line 303-321: Rework ExecuteRawFireAndForgetAsync so
ValidateRawFireAndForgetCommand runs before CreateRawCommand allocates the full
token array, while still validating against the correct command arguments and
preserving the existing cluster-wide versus regular dispatch behavior. Account
for the DynamicCommand.Arguments verb-token offset when obtaining the validation
inputs, as noted in the related comment.
🪄 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: 0a4d976e-abaa-49f7-9507-df81a74cce62
📒 Files selected for processing (9)
src/Respire/Commands/CommandStructs.cssrc/Respire/IRespireClient.cssrc/Respire/RespireClient.cssrc/Respire/RespireCommand.cssrc/Respire/RespireCommandFlags.cstests/Respire.Extensions.Caching.Tests/RespireDistributedCacheTests.cstests/Respire.Tests/Networking/ClusterTests.cstests/Respire.Tests/Networking/CommandCatalogTests.cstests/Respire.Tests/Networking/FakeRespServer.cs
There was a problem hiding this comment.
💡 Codex Review
Respire/src/Respire/RespireCommand.cs
Lines 74 to 76 in 810b427
Parse BLOCK only in the command's option section
For XREAD STREAMS BLOCK $, BLOCK is a valid stream key rather than the blocking option, but scanning every argument classifies the command as blocking. The new fire-and-forget APIs consequently reject legitimate nonblocking reads whenever a key (or an XREADGROUP group/consumer/key) is named BLOCK; inspect the command grammar and only recognize BLOCK before the STREAMS section.
ℹ️ 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".
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 01:15 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 02:29 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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: 3f4957d007
ℹ️ 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
+175
to
+176
| /// Standalone execution returns after writing. Cluster execution may await replies to process | ||
| /// <c>MOVED</c> and <c>ASK</c> redirects, adding round-trip latency. |
There was a problem hiding this comment.
When a caller awaits a standalone fire-and-forget command and then immediately disposes the client or exits, the command can still be dropped: RespireConnection.SendFireAndForgetAsync returns a completed task after enqueueing and scheduling the flush (RespireConnection.cs:573-580), while disposal aborts the connection before awaiting the flush loop (RespireConnection.cs:1524-1533). This contradicts the new guarantee that standalone execution returns after writing; either await completion of the socket write or document that completion only means the command was queued.
Useful? React with 👍 / 👎.
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 02:42 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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: d3f2ab00d6
ℹ️ 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
+576
to
+579
| if (TryEnqueueForWrite(in command, out var startedBatch, out var writeTask)) | ||
| { | ||
| ScheduleFlush(startedBatch); | ||
| return ValueTask.CompletedTask; | ||
| return new ValueTask(writeTask); |
There was a problem hiding this comment.
When the socket write is backpressured and this normal-capacity fast path succeeds, the supplied cancellation token is never observed and the returned writeTask is awaited directly. Consequently, even a pre-canceled token still enqueues and executes a mutating command, while cancellation during a stalled write cannot release the caller; only the ring-full slow path checks the token before enqueueing. Check cancellation before enqueueing and make the caller's wait cancelable without canceling the underlying socket send.
Useful? React with 👍 / 👎.
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 02:52 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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: 7a1f0f36f1
ℹ️ 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".
| ReadOnlySpan<string> inlineArguments, | ||
| ReadOnlySpan<RespireValue> arguments) | ||
| { | ||
| var behavior = RespireCommand.Classify(operation); |
There was a problem hiding this comment.
When raw fire-and-forget is invoked as ExecuteFireAndForgetAsync("CLIENT LIST") or ("CLIENT", "HELP"), parsing leaves operation as CLIENT, so this classifies the command as connection-scoped and throws. The catalog intentionally classifies exact commands such as CLIENT LIST, CLIENT HELP, CLIENT KILL, and CLIENT UNBLOCK as multiplexed and accepts their fire-and-forget forms, so valid raw commands are rejected solely because of the API shape. Inspect the inline or first supplied CLIENT subcommand before applying the affinity check, as is already done for SCRIPT.
Useful? React with 👍 / 👎.
| var (operation, words, firstArgumentIndex) = ParseRawCommand(command); | ||
| ValidateRawFireAndForgetCommand( | ||
| operation, words.AsSpan(firstArgumentIndex), args); | ||
| var (_, commandValue) = CreateRawCommand(operation, words, firstArgumentIndex, args); |
There was a problem hiding this comment.
When telemetry is enabled for a raw fire-and-forget EVALSHA or FCALL, this discards the stored-procedure name returned by CreateRawCommand; the catalog fire-and-forget path similarly never computes it. The downstream instrumentation therefore names every activity only EVALSHA/FCALL and omits db.stored_procedure.name from both spans and measurements, unlike the equivalent result-returning APIs, so production telemetry can no longer distinguish scripts or functions. Thread the derived name through the fire-and-forget send and instrumentation helpers.
Useful? React with 👍 / 👎.
thomhurst
temporarily deployed
to
Pull Requests
August 10, 2026 04:19 — with
GitHub Actions
Inactive
GitHub Actions
Inactive
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 |
This was referenced Aug 10, 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
RespireCommandFlagswithNoRedirectsupport for raw, catalog, and interpolated command executionRespireResultValidation
Summary by CodeRabbit