[Hardik K (hardikkaurani)]

Summary

In HeadersInspectionHandler, there were three issues in header inspection and telemetry:

1. Multi-valued Header Overwrites: HeadersInspectionHandler previously created a new HashSet<String>() per header and called put(header.getFirst(), value) on RequestHeaders and ResponseHeaders. When multiple entries existed for the same header name (e.g., Set-Cookie, Accept, Vary), subsequent values replaced the set instead of accumulating, leaving only the last value. By delegating to add(key, value), all values for the header are preserved.
2. Request Isolation: If the same HeadersInspectionOption instance was reused across requests, headers from previous requests were never cleared and leaked into subsequent requests.
3. Telemetry Attribute Accuracy: The OpenTelemetry span attribute com.microsoft.kiota.handler.headersInspection.enable was hardcoded to true even when inspection was disabled; it now reflects inspectionOption.getInspectRequestHeaders() || inspectionOption.getInspectResponseHeaders().

Changes

• Replaced .put(header.getFirst(), value) with .add(header.getFirst(), header.getSecond()) in HeadersInspectionHandler.java.
• Added .clear() calls to getRequestHeaders() and getResponseHeaders() prior to recording headers for each request/response.
• Updated telemetry attribute to check if request or response inspection is enabled.
• Added unit tests in HeadersInspectionHandlerTest.java verifying preservation of multi-valued headers and header isolation across requests.

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet)! This PR fixes multi-valued header handling in HeadersInspectionHandler by calling add() instead of put() (which was replacing sets and overwriting multi-valued headers such as Set-Cookie or Vary), adds .clear() for per-request header isolation when reusing inspection options, and aligns the telemetry enable attribute with active inspection settings. Unit tests covering both multi-valued headers and request isolation are included. Whenever you have a chance, please take a look. Thank you!

[Copilot]

🟡 Changes recommended

Shared-option capture updates can race, and failed requests retain stale response headers.

2 open issues

• Synchronize shared header capture to prevent concurrent-call races · New
• Clear response headers before proceeding to prevent stale headers · New

_{Review effort: Balanced}

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet) and @andreastman,

I've addressed all Copilot review feedback and resolved the formatting and lint checks. Here's a summary of the changes:

1. Synchronize Shared Header Capture

• Synchronized request and response header capture on the shared inspectionOption instance.
• Prevented race conditions where concurrent requests without request-specific options could clear the internal header map between containsKey() and get() in Headers.addImpl(), avoiding potential NullPointerExceptions and cross-request header contamination.
• Added handlesConcurrentCallsWithoutRaces() to verify race-free execution across concurrent requests using multiple threads.

2. Clear Response Headers Before chain.proceed(request)

• Moved inspectionOption.getResponseHeaders().clear() before chain.proceed(request).
• Ensured that if a request fails (e.g., with an IOException), stale response headers from a previous request using the shared option are not retained.
• Added clearsResponseHeadersWhenRequestFails() to verify that a failed subsequent request clears stale response headers while preserving the request headers captured for that request.

3. Spotless Formatting and CI Checks

• Applied formatting using ./gradlew spotlessApply.
• Verified formatting using ./gradlew spotlessCheck.
• Ran the relevant component checks and tests successfully.

Commits

The PR contains two focused commits:

• 9dbe0f71: fix(http): support multi-valued headers, synchronize shared capture, and prevent stale response headers
• d4bd310c: test(http): add unit tests for multi-valued headers, stale header prevention, and concurrent calls

All local checks and tests pass. The changes are ready for review. Thanks for your time and feedback!

[Copilot]

🟡 Changes recommended

The concurrency test must shut down its executor when workers fail or execution is interrupted.

1 open issue

• Ensure executor shutdown on worker failure or interruption · New

2 resolved since last review

• Synchronize shared header capture to prevent concurrent-call races
• Clear response headers before proceeding to prevent stale headers

_{Review effort: Balanced}

[Copilot]

🟢 Approval recommended

Prior feedback is addressed, regression coverage is added, and no unresolved blocking findings remain.

No open issues 🎉

1 resolved since last review

• Ensure executor shutdown on worker failure or interruption

_{Review effort: Balanced}

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet)

PR has been updated and all checks are passing:

• Executor Lifecycle in Concurrency Test: Wrapped worker task execution and Future resolution in HeadersInspectionHandlerTest in a try ... finally block, ensuring executor.shutdown() and termination cleanup run deterministically under worker failures or thread interruption.
• Verification: Verified spotless formatting via spotlessCheck and 100% test pass rate via Gradle.

Ready for review and merge!

[Copilot]

🟡 Changes recommended

Overlapping calls sharing an option can still combine headers from different responses.

1 open issue

• Clear response map before populating to prevent overlapping response mixing · New

_{Review effort: Balanced}

[Copilot]

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

[Hardik K (hardikkaurani)]

Hi Vincent Biret (@baywet),

I have updated the PR to address the overlapping response mixing finding:

• Clear response map inside lock before populating: Retained the pre-proceed \clear()\ to prevent stale headers on failed requests, and added \inspectionOption.getResponseHeaders().clear()\ inside the post-proceed \synchronized (inspectionOption)\ block so overlapping calls sharing an option replace headers instead of accumulating/mixing them.
• Deterministic overlapping test: Added \overlappingCallsDoNotMixResponseHeaders()\ in \HeadersInspectionHandlerTest.java\ using \CountDownLatch\ coordination to verify that when two overlapping calls both pass \proceed(), the later response replaces earlier headers cleanly rather than mixing them.
• Formatting & Tests: Verified spotless formatting (\spotlessCheck) and 100% test pass rate.

Thank you!

[Vincent Biret (baywet)]

Thanks for the contribution!

I'll disregard my earlier comment. Although it's technically extremely unlikely to get a mixup for response headers, it is theoretically possible.