Clean up DeadlineMiddleware/Interceptor edge cases#4547
Merged
pepone merged 7 commits intoicerpc:mainfrom Apr 30, 2026
Merged
Conversation
DeadlineMiddleware used the decoded DateTime.MinValue as its "field absent" sentinel. A peer encoding ticks=0 decodes to exactly that value, so an explicitly encoded MinValue deadline was indistinguishable from an absent field and bypassed deadline enforcement entirely. Check field presence explicitly via TryGetValue, then decode and enforce. Add a regression test encoding ticks=0 (DateTime(0, Utc) so ToUniversalTime doesn't shift it) and assert the middleware returns DeadlineExceeded and does not call the next dispatcher. Closes icerpc#4419
There was a problem hiding this comment.
Pull request overview
Fixes deadline enforcement in IceRpc.Deadline by ensuring an explicitly encoded ticks=0 (which decodes to DateTime.MinValue) is treated as an expired deadline instead of being mistaken for an absent field.
Changes:
- Switched
DeadlineMiddlewarefrom a decoded-value sentinel check (DateTime.MinValue) to an explicitTryGetValuepresence check before decoding/enforcing the deadline. - Added a regression test covering an explicitly encoded
ticks=0deadline and asserting it returnsDeadlineExceededwithout invoking the next dispatcher.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| tests/IceRpc.Deadline.Tests/DeadlineMiddlewareTests.cs | Adds a regression test for an explicitly encoded DateTime.MinValue (ticks=0) deadline to ensure it is enforced as expired. |
| src/IceRpc.Deadline/DeadlineMiddleware.cs | Fixes the sentinel collision by checking field presence with TryGetValue and decoding/enforcing only when present. |
externl
approved these changes
Apr 23, 2026
CancellationTokenSource.CancelAfter(TimeSpan) rejects values greater than int.MaxValue milliseconds (~24.8 days). A peer-encoded deadline thousands of years in the future (or a caller-supplied IDeadlineFeature near DateTime.MaxValue) produced a TimeSpan that CancelAfter rejected with ArgumentOutOfRangeException, leaking as a generic InternalError response on the server side. - DeadlineMiddleware: clamp the computed timeout before PerformDispatchAsync. - DeadlineInterceptor: clamp the timeout in PerformInvokeAsync for the peer-deadline-via-feature path, and reject a configured defaultTimeout greater than the CancelAfter maximum at construction time. At the clamp bound (~24.8 days) the deadline is effectively infinite for RPC purposes; matches the precedent set by HttpClient.Timeout. Closes icerpc#4420
pepone
changed the title
Add the same CancelAfter-maximum cap to DeadlineFeature.FromTimeout so that the failure surfaces at the call site instead of inside the interceptor or as a later DateTime overflow. Also reject non-positive timeouts here, matching the DeadlineInterceptor constructor's validation of its defaultTimeout. Document the cap on: - DeadlineFeature.FromTimeout (param docs + ArgumentException). - DeadlineInterceptor constructor's defaultTimeout param (+ class-level remark about silent clamping for extreme IDeadlineFeature values). - DeadlineMiddleware class remarks (silent clamping of peer deadlines).
- Split the InfiniteTimeSpan exception out of the constraint clause in DeadlineInterceptor's defaultTimeout doc so it reads behavior → constraint → explicit exception. - Assert the next dispatcher is invoked in the middleware extreme-future clamp test so a regression that swallowed the request would be caught.
InsertCreativityHere
approved these changes
Apr 29, 2026
Member
InsertCreativityHere
left a comment
InsertCreativityHere
left a comment
There was a problem hiding this comment.
Looks good to me!
I think the added comments could have some fluff pruned out of them though. : v)
InsertCreativityHere
approved these changes
Apr 29, 2026
Co-authored-by: Austin Henriksen <austin.r.henriksen79@gmail.com>
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.
Two related edge-case fixes on the deadline middleware/interceptor. Kept in one PR because they touch the same files and share the same theme.
Commit 1 — DateTime.MinValue sentinel (closes #4419)
DeadlineMiddlewareused the decodedDateTime.MinValueas its "field absent" sentinel. A peer encoding `ticks=0` decodes to exactly that value, so an explicit MinValue deadline was indistinguishable from an absent field and bypassed deadline enforcement entirely.Switched to explicit `TryGetValue` on the fields dictionary, then decode + enforce. Added a regression test encoding `new DateTime(0, DateTimeKind.Utc)`.
Commit 2 — extreme future deadline clamp (closes #4420)
`CancellationTokenSource.CancelAfter(TimeSpan)` rejects values greater than `int.MaxValue` ms (~24.8 days). A peer-encoded deadline thousands of years in the future (or a caller-supplied `IDeadlineFeature` near `DateTime.MaxValue`) produced a TimeSpan that `CancelAfter` rejected with `ArgumentOutOfRangeException`, leaking as a generic `InternalError` response.
DeadlineMiddleware: clamp the computed timeout beforePerformDispatchAsync.DeadlineInterceptor: clamp the timeout inPerformInvokeAsync; reject configureddefaultTimeoutbeyond theCancelAftermax at construction.At the clamp bound (~24.8 days) the deadline is effectively infinite for RPC purposes; matches the precedent set by
HttpClient.Timeout.Closes #4419
Closes #4420
Test plan
dotnet build src/IceRpc.Deadline/IceRpc.Deadline.csproj(clean)dotnet test tests/IceRpc.Deadline.Tests/IceRpc.Deadline.Tests.csproj— 17/17 passed