Implement P4: Cancellation and Deduplication Handlers #2

jhosm · 2026-02-09T00:22:53Z

Summary

CancellationHandler: DelegatingHandler that wraps each request with a linked CancellationTokenSource, enabling both per-request cancellation (via the caller's token) and bulk cancellation (via ActiveRequestTracker.CancelAll())
DeduplicationHandler: Deduplicates concurrent GET/HEAD requests using ConcurrentDictionary<string, Lazy<Task>> keyed by method+URL+SHA256(sorted headers); buffers and clones responses for each subscriber
ActiveRequestTracker: Thread-safe singleton service for tracking and cancelling all in-flight requests
HttpRequestMessageExtensions.CloneAsync: Deep clone utility for HttpRequestMessage (method, URI, headers, content, Options)
AcdcRequestOptions.Deduplicate: Per-request opt-out key for deduplication
Added InternalsVisibleTo for test project access to internal BuildDeduplicationKey

Test plan

37 new unit tests covering all P4 components (101 total pass)
ActiveRequestTrackerTests — track/untrack/cancel-all, disposed source handling, concurrent thread safety (1000 iterations)
CancellationHandlerTests — linked token propagation, cleanup on success/failure, CancelAll cancels in-flight, external cancellation
DeduplicationHandlerTests — concurrent GET dedup, POST/PUT/DELETE passthrough, HEAD dedup, per-request opt-out, different URLs/headers not deduped, response cloning independence, cleanup allows new waves, error propagation to all subscribers
HttpRequestMessageExtensionsTests — copies method/URI/headers/content/options, null content, independent copy verification

🤖 Generated with Claude Code

…tilities Add thread-safe cancellation and request deduplication to the HTTP handler pipeline: - ActiveRequestTracker: ConcurrentDictionary-backed CTS tracker with Track/Untrack/CancelAll for centralized request cancellation - CancellationHandler: DelegatingHandler that creates linked CancellationTokenSources, enabling per-request and bulk cancellation - DeduplicationHandler: Deduplicates concurrent GET/HEAD requests using ConcurrentDictionary<string, Lazy<Task>> with SHA256-based keys from method+URL+sorted headers; clones buffered responses for each subscriber - HttpRequestMessageExtensions.CloneAsync: Deep clone for HttpRequestMessage including method, URI, headers, content, and Options - AcdcRequestOptions.Deduplicate: Per-request opt-out key for dedup - InternalsVisibleTo for test access to internal BuildDeduplicationKey All 101 tests pass (37 new tests for P4 components). Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

Copilot

Pull request overview

This PR adds HTTP pipeline components to support bulk cancellation of in-flight requests and deduplication of concurrent GET/HEAD calls, plus supporting request option(s), cloning helpers, and unit tests.

Changes:

Introduces CancellationHandler + ActiveRequestTracker to enable CancelAll() over active requests while preserving caller cancellation.
Introduces DeduplicationHandler to coalesce concurrent GET/HEAD requests and clone buffered responses to each subscriber.
Adds HttpRequestMessage.CloneAsync, request option key(s), and a new unit test suite for the above.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 36 comments.

Show a summary per file

File	Description
`src/CSharpAcdc/Handlers/DeduplicationHandler.cs`	Implements concurrent request deduplication and response buffering/cloning.
`src/CSharpAcdc/Handlers/CancellationHandler.cs`	Wraps requests in a linked CTS and tracks them for bulk cancellation.
`src/CSharpAcdc/Cancellation/ActiveRequestTracker.cs`	Tracks CTS instances and provides `CancelAll()` support.
`src/CSharpAcdc/Extensions/HttpRequestMessageExtensions.cs`	Adds deep-ish cloning for `HttpRequestMessage` (headers/content/options).
`src/CSharpAcdc/Extensions/AcdcRequestOptions.cs`	Adds `AcdcRequestOptions.Deduplicate` opt-out.
`src/CSharpAcdc/CSharpAcdc.csproj`	Adds `InternalsVisibleTo` for tests to access internal APIs.
`tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs`	Adds test coverage for dedup behavior and key determinism.
`tests/CSharpAcdc.Tests/Handlers/CancellationHandlerTests.cs`	Adds test coverage for linked token behavior and bulk cancellation.
`tests/CSharpAcdc.Tests/Extensions/HttpRequestMessageExtensionsTests.cs`	Adds test coverage for request cloning behavior.
`tests/CSharpAcdc.Tests/Cancellation/ActiveRequestTrackerTests.cs`	Adds test coverage for tracking and cancel-all semantics under concurrency.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-02-09T00:29:19Z

src/CSharpAcdc/Handlers/DeduplicationHandler.cs

+        var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);
+        return await DeduplicatedResponse.FromResponseAsync(response).ConfigureAwait(false);
+    }


The response returned by base.SendAsync is buffered into DeduplicatedResponse but never disposed. That can leak sockets/handlers because HttpResponseMessage/HttpContent are IDisposable. After buffering headers/content bytes, dispose the original HttpResponseMessage (e.g., via using/try-finally in ExecuteAndBufferAsync or inside FromResponseAsync).

Copilot · 2026-02-09T00:29:20Z

src/CSharpAcdc/Handlers/DeduplicationHandler.cs

+        var sortedHeaders = request.Headers
+            .OrderBy(h => h.Key, StringComparer.OrdinalIgnoreCase)
+            .SelectMany(h => h.Value.Select(v => $"{h.Key}:{v}"));
+
+        var headerString = string.Join(",", sortedHeaders);
+        var hashBytes = SHA256.HashData(Encoding.UTF8.GetBytes(headerString));
+        sb.Append(Convert.ToHexString(hashBytes));


BuildDeduplicationKey sorts by header name, but the key still depends on header value insertion order (and potentially header name casing). Two requests with the same logical headers added in different orders can produce different keys and skip deduplication. To make the key deterministic, normalize the header name (e.g., lowercase) and sort the values for each header before hashing.

Copilot · 2026-02-09T00:29:20Z

src/CSharpAcdc/Cancellation/ActiveRequestTracker.cs

+        foreach (var cts in _activeSources.Keys)
+        {
+            try
+            {
+                cts.Cancel();
+            }
+            catch (ObjectDisposedException)
+            {
+                // CTS may have been disposed between enumeration and cancel
+            }
+        }
+
+        _activeSources.Clear();


CancelAll() calls _activeSources.Clear() after enumerating keys. With concurrent Track() calls, a CTS can be added after enumeration but before Clear(), resulting in it being removed from tracking without being canceled (request continues but is no longer tracked). Prefer canceling via TryRemove in a loop (remove-then-cancel) and avoid Clear(), so newly tracked sources aren’t accidentally dropped uncanceled.

Suggested change

foreach (var cts in _activeSources.Keys)

{

try

{

cts.Cancel();

}

catch (ObjectDisposedException)

{

// CTS may have been disposed between enumeration and cancel

}

}

_activeSources.Clear();

// Remove-then-cancel pattern to avoid dropping newly tracked CTS instances.

while (!_activeSources.IsEmpty)

{

foreach (var kvp in _activeSources.ToArray())

{

var cts = kvp.Key;

if (!_activeSources.TryRemove(cts, out _))

{

// Already removed by another operation; skip.

continue;

}

try

{

cts.Cancel();

}

catch (ObjectDisposedException)

{

// CTS may have been disposed between removal and cancel.

}

}

}

Copilot · 2026-02-09T00:29:20Z

tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs

+    [Fact]
+    public async Task ResponseCloning_ProducesIndependentResponses()
+    {
+        var innerHandler = new DelegateHandler((req, ct) =>
+            Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK)
+            {
+                Content = new StringContent("shared-body"),
+            }));
+
+        var handler = new DeduplicationHandler { InnerHandler = innerHandler };
+        using var client = new HttpClient(handler)
+        {
+            BaseAddress = new Uri("https://api.example.com"),
+        };
+
+        var response1 = await client.GetAsync("/data");
+        var response2 = await client.GetAsync("/data");
+


This test name suggests it validates response cloning for deduplicated subscribers, but the two GetAsync calls are sequential, so the handler will likely issue two separate upstream requests (no in-flight overlap). Make the requests concurrent (and assert the inner handler call count is 1) so the test actually exercises cloning of a shared buffered response.

Copilot · 2026-02-09T00:29:21Z

tests/CSharpAcdc.Tests/Handlers/CancellationHandlerTests.cs

+        // because it's a linked token
+        capturedToken.Should().NotBe(CancellationToken.None);


SendAsync_PassesLinkedToken_Downstream doesn’t currently verify that the downstream token is actually linked to the caller token (it only checks it’s not CancellationToken.None). Consider asserting that canceling the caller CTS cancels the captured token, or at least that capturedToken != cts.Token, so the test fails if the handler accidentally forwards the original token unchanged.

Suggested change

// because it's a linked token

capturedToken.Should().NotBe(CancellationToken.None);

// because it's a linked token, not the original caller token

capturedToken.Should().NotBe(CancellationToken.None);

capturedToken.Should().NotBe(cts.Token);

Copilot · 2026-02-09T00:29:26Z

tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs

+        var innerHandler = new DelegateHandler((req, ct) =>
+        {
+            Interlocked.Increment(ref callCount);
+            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK));


Disposable 'HttpResponseMessage' is created but not disposed.

Copilot · 2026-02-09T00:29:26Z

tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs

+            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK)
+            {
+                Content = new StringContent("ok"),
+            });


Disposable 'HttpResponseMessage' is created but not disposed.

Copilot · 2026-02-09T00:29:26Z

tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs

+            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK)
+            {
+                Content = new StringContent("ok"),
+            });


Disposable 'HttpResponseMessage' is created but not disposed.

Copilot · 2026-02-09T00:29:26Z

tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs

+            Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK)
+            {
+                Content = new StringContent("shared-body"),
+            }));


Disposable 'HttpResponseMessage' is created but not disposed.

Copilot · 2026-02-09T00:29:27Z

tests/CSharpAcdc.Tests/Handlers/DeduplicationHandlerTests.cs

+            return Task.FromResult(new HttpResponseMessage(HttpStatusCode.OK)
+            {
+                Content = new StringContent($"response-{callCount}"),
+            });


Disposable 'HttpResponseMessage' is created but not disposed.

jhosm · 2026-02-12T07:35:08Z

Comprehensive PR Review -- P4: Cancellation and Deduplication Handlers

Reviewed by 5 specialized agents: code-reviewer, thread-safety-reviewer, silent-failure-hunter, type-design-analyzer, and test-coverage-analyzer. Findings are deduplicated and ranked by cross-agent convergence.

Critical Issues

1. DeduplicationHandler captures first caller's CancellationToken (5/5 agents converge)

The Lazy<Task<>> factory at line 21-22 captures the first caller's cancellationToken. If that caller cancels, all coalesced subscribers get TaskCanceledException even if their own tokens are still active. Later callers' tokens are ignored entirely for the upstream request.

Fix: Decouple the upstream request from individual caller tokens. Use a handler-owned CancellationTokenSource (or CancellationToken.None) for the shared upstream call. Individual subscribers should apply their own cancellation to the wait (e.g., await task.WaitAsync(cancellationToken)), not the upstream call.

2. ActiveRequestTracker.CancelAll race condition (4/5 agents converge)

The enumerate-then-Clear pattern at lines 21-36 has a race: a CTS Track'd between the foreach loop and Clear() will be silently removed without being canceled.

Fix: Replace with a remove-then-cancel loop:

while (!_activeSources.IsEmpty)
{
    foreach (var cts in _activeSources.Keys)
    {
        if (_activeSources.TryRemove(cts, out _))
        {
            try { cts.Cancel(); }
            catch (ObjectDisposedException) { }
        }
    }
}

3. Original HttpResponseMessage not disposed in ExecuteAndBufferAsync (3/5 agents converge)

At lines 37-43, the response from base.SendAsync is buffered into DeduplicatedResponse but never disposed. This leaks socket connections held by the response's content stream.

Fix: Add using to the response variable:

using var response = await base.SendAsync(request, cancellationToken).ConfigureAwait(false);

4. _inFlight is instance state on pooled DelegatingHandler (2/5 agents converge)

IHttpClientFactory pools DelegatingHandler instances with ~2-minute lifetime (CLAUDE.md states this explicitly). The _inFlight dictionary at line 10 lives and dies with each handler instance. On pool rotation, dedup state is lost and in-flight requests are orphaned.

Fix: Inject the ConcurrentDictionary as a singleton service (similar to how ActiveRequestTracker is injected into CancellationHandler), or accept the limitation with documentation.

Important Issues

5. CloneAsync doesn't copy Version/VersionPolicy (3/5 agents)

HttpRequestMessageExtensions.cs does not copy request.Version or request.VersionPolicy. Cloned requests silently default to HTTP/1.1, which will cause behavior differences when the original targets HTTP/2.0 or HTTP/3.0.

6. Missing null guards on constructors (3/5 agents)

CancellationHandler constructor at line 9 doesn't validate tracker. A null injection will defer NullReferenceException until the first request.
ActiveRequestTracker.Track/Untrack at lines 11-18 don't validate the cts parameter.

Fix: Add ArgumentNullException.ThrowIfNull() guards.

7. Handlers not sealed (type-design-analyzer)

Both CancellationHandler and DeduplicationHandler are public class rather than public sealed class. Since their behavior is complete and final, sealed prevents subclass invariant violations. CLAUDE.md requires "All DelegatingHandler implementations must be thread-safe" -- sealed helps enforce this.

8. DeduplicatedResponse should be a record (type-design-analyzer, CLAUDE.md compliance)

CLAUDE.md states "Use record types for DTOs and immutable config." DeduplicatedResponse at line 75 is a pure data snapshot class. Additionally, the mutable List<> header properties should be IReadOnlyList<>.

Test Coverage Gaps

No test for CancellationToken capture bug (severity 9/10): The test at CancellationToken_IsLinked only verifies the token is linked, but does not test the scenario where a first caller cancels and other subscribers are affected.
No concurrent stress test for DeduplicationHandler (severity 8/10): The concurrent dedup test only verifies 5 simultaneous callers. A stress test with contention, mixed cancellations, and error cases would expose the token-capture and cleanup races.
No test for failed request cleanup allowing retry (severity 8/10): When the shared dedup request fails, the entry is cleaned up, but there is no test verifying that a subsequent identical request triggers a fresh upstream call.

Strengths

Clean separation of concerns: ActiveRequestTracker is a standalone service, CancellationHandler only manages token lifecycle, DeduplicationHandler manages coalescing.
Thread-safe collection choices (ConcurrentDictionary, Lazy<Task<>>) are correct idioms.
TryRemove(KeyValuePair) in DeduplicationHandler's cleanup is a nice detail preventing removal of replacement entries.
DeduplicatedResponse being private sealed is excellent encapsulation.
Good test coverage for the happy paths (37 tests total).

Recommended Action Plan

Fix CancellationToken capture -- decouple upstream token from individual callers
Fix CancelAll race -- use remove-then-cancel loop
Dispose original response in ExecuteAndBufferAsync
Inject dedup state as singleton or document the pool-rotation limitation
Copy Version/VersionPolicy in CloneAsync
Add null guards to constructors
Seal handler classes and convert DeduplicatedResponse to record
Add tests for cancellation-token propagation, concurrent stress, and failed-request retry

Generated with Claude Code

_{Review produced by: code-reviewer, thread-safety-reviewer, silent-failure-hunter, type-design-analyzer, pr-test-analyzer}

Copilot AI review requested due to automatic review settings February 9, 2026 00:22

Copilot started reviewing on behalf of jhosm February 9, 2026 00:23 View session

Copilot AI reviewed Feb 9, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement P4: Cancellation and Deduplication Handlers #2

Implement P4: Cancellation and Deduplication Handlers #2

Uh oh!

jhosm commented Feb 9, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

Copilot AI Feb 9, 2026

Uh oh!

jhosm commented Feb 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

		// because it's a linked token
		capturedToken.Should().NotBe(CancellationToken.None);

Implement P4: Cancellation and Deduplication Handlers #2

Are you sure you want to change the base?

Implement P4: Cancellation and Deduplication Handlers #2

Uh oh!

Conversation

jhosm commented Feb 9, 2026

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Feb 9, 2026

Choose a reason for hiding this comment

Uh oh!

jhosm commented Feb 12, 2026

Comprehensive PR Review -- P4: Cancellation and Deduplication Handlers

Critical Issues

Important Issues

Test Coverage Gaps

Strengths

Recommended Action Plan

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant