Skip to content

Default Win32 transport to WinHTTP and harden request lifetimes - #1520

Merged
bmehta001 merged 290 commits into
microsoft:mainfrom
bmehta001:bhamehta/winhttp-default-windows-transport
Sep 22, 2026
Merged

bmehta001 merged 290 commits into
microsoft:mainfrom
bmehta001:bhamehta/winhttp-default-windows-transport

Conversation

@bmehta001

@bmehta001 bmehta001 commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Make WinHTTP the default Win32 desktop telemetry transport while preserving WinInet as an explicit compatibility option.
  • Select and link exactly one Win32 HTTP backend across CMake, Visual Studio projects, samples, tests, and the vcpkg packaging path.
  • Add bounded request draining so cancellation and shutdown complete without deadlocks, use-after-free, or duplicate terminal callbacks across WinHTTP, WinInet, Apple, Curl, and C API transports.
  • Harden TLS and response handling: Curl always verifies certificates and hostnames, Windows Microsoft-root checks operate on negotiated HTTPS certificates, and transport setup/response failures retain accurate outcomes.
  • Strengthen CI diagnostics and deterministic cancellation coverage, including stale macOS kqueue event handling in the test reactor.

Notable fixes

  • Prevent WinHTTP teardown deadlocks when sends fail synchronously or cancellation races with callbacks.
  • Keep WinHTTP callback and request state alive until native handles finish closing.
  • Guarantee exactly one terminal completion for Apple and Curl setup, cancellation, callback, and worker-failure paths.
  • Correct Curl HTTP-version selection, callback signatures, socket handling, option validation, and request-body lifetime.
  • Make MSVC Annex K overlap validation overflow-safe and avoid process-terminating invalid-parameter paths.
  • Improve Windows test timeout/minidump diagnostics and remove timing or external-network dependencies from cancellation tests.

Compatibility notes

  • The supported Windows floor remains Windows 10 and Windows Server 2016, matching the repository support matrix. Windows 7, Windows 8, and Windows 8.1 are not supported; legacy target macros retained in project files are for binary compatibility and do not extend that support matrix.
  • The managed wrapper now requires .NET Framework 4.8. Applications targeting .NET Framework 4.0 through 4.7.2 cannot reference the new assembly and must remain on an earlier SDK release or update their target framework.
  • WinInet rejects HTTPS requests with http.msRootCheck=true before any headers are sent because WinInet cannot safely evaluate the negotiated certificate before committing sensitive headers. Use the default WinHTTP transport when this policy is required.
  • The in-repository vcpkg port now fails clearly when its pinned fallback source predates the Windows transport selector. The release pin and SHA remain a separate post-release update.
  • Win32 desktop builds now use WinHTTP by default. Set MATSDK_USE_WININET=ON in CMake or MATSDK_USE_WININET=true in Visual Studio builds to retain WinInet.
  • http.sslVerify=false remains accepted for configuration compatibility but is ignored. Curl always enables peer and hostname verification; development environments using private or self-signed certificates must configure http.sslCaInfo with a trusted CA bundle.
  • When http.msRootCheck=true, HTTPS requests fail closed if the negotiated server chain cannot be retrieved or the Microsoft-root policy cannot be evaluated. Plain HTTP requests are unaffected.
  • Apple CancelAllRequests() now retires created-but-unsent requests instead of waiting indefinitely; a later send receives exactly one Aborted callback.
  • The vcpkg pin and SHA will be updated in a separate PR.

Validation

  • Visual Studio 2026, x64 Release, WinHTTP: all 639 unit tests passed.
  • Visual Studio 2026, x64 Release, WinInet: the pre-send MS-root rejection regression passed.
  • Linux/WSL Debug: 14 targeted worker, dispatcher, storage, manager-lifetime, and Curl tests passed.
  • Linux/WSL with -fno-exceptions: the Curl transport translation unit compiled successfully; CI now enforces this configuration.
  • GitHub Actions covers WinHTTP and WinInet Windows matrices, Linux/macOS/iOS builds, vcpkg consumers, Android, public headers, and CodeQL.

bmehta001 and others added 30 commits June 23, 2026 11:33
…(was PR microsoft#1497)

Combine the batched-flush perf work into this PR and make it cooperate with the
Flush() data-loss fix, so both land together.

OfflineStorage_SQLite: StoreRecords() now inserts the whole batch in a single
BEGIN EXCLUSIVE / COMMIT (one fsync) instead of one transaction per record
(~11x at 200 records, ~40x at 1000 vs the SDK's vendored sqlite). Shared per-record
logic is factored into isValidRecord / insertRecordUnsafe / checkStorageSizeLimits.
The batch is all-or-nothing: if any insert fails, the transaction is rolled back
(new SqliteDB::rollback / DbTransaction::markForRollback) and the size estimate is
undone, so callers can re-queue the whole batch without risking duplicate rows
(the events table has no unique record_id constraint).

OfflineStorageHandler::Flush() now uses the batched StoreRecords() to persist a
drained batch in one transaction. Because StoreRecords() is all-or-nothing, on
failure nothing is committed and Flush returns every record to the in-memory queue
for retry -- realizing the batching speedup while keeping the no-event-loss /
no-duplicate guarantee.

StoreRecords/StoreRecord report write failures via OnStorageFailed after the
transaction closes; validation runs before the transaction. Adds
OfflineStorageTests_SQLite.StoreRecordsBatchStoresAllRecords. Full UnitTests (527)
pass. Closes PR microsoft#1497.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…cords

StoreRecords() previously filtered out invalid records and committed the valid
ones, so it could return a count < records.size() even though some records were
persisted. OfflineStorageHandler::Flush() treats totalSaved < records.size() as a
batch failure and re-queues ALL drained records, which would duplicate the valid
records that were actually stored.

Make StoreRecords() truly all-or-nothing: if ANY input record is invalid, store
nothing and return 0 (invalids are still reported via isValidRecord()). Combined
with the existing rollback-on-write-failure, StoreRecords() now returns either
records.size() (whole batch committed) or 0 (nothing committed), so Flush's
re-queue-all-on-short-return can never duplicate records.

Adds OfflineStorageTests_SQLite.StoreRecordsBatchWithAnyInvalidStoresNothing.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Flush() re-queued the whole drained batch whenever StoreRecords() returned a
count < records.size(). Both disk backends are all-or-nothing (SQLite rolls back;
Room returns 0 on a failed JNI batch), so the only meaningful "failure" value is
0. Room also caps its returned count at min(size, INT32_MAX); keying off
< records.size() would treat that capped count as a failure and re-queue
already-persisted records (duplicates). Key the re-queue off totalSaved == 0
instead, which is the true "nothing committed" signal. (The cap only matters for
a batch larger than the RAM queue could ever hold.)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…profile

Three small correctness fixes bundled with the offline-storage work:

- microsoft#1334: PrivacyGuard JNI use-after-free. nativeInitializePrivacyGuard[WithoutCommonDataContext]
  assigned JStringToStdString(...).c_str() into InitializationConfiguration's const char*
  fields; the temporary std::string was destroyed at the end of the statement, leaving the
  config pointing at freed memory before PrivacyGuard was constructed. Hold the converted
  strings in locals that outlive the make_shared<PrivacyGuard>(config) call.

- microsoft#1333: GetAppLocalTempDirectory leaked a RoInitialize reference on the UWP path (no matching
  RoUninitialize). Balance it with RoUninitialize() when the call succeeded, releasing the
  WinRT StorageFolder first so it is not destroyed in an uninitialized apartment.

- microsoft#312: TransmitProfiles JSON powerState map was missing the low_battery key, so profiles
  using it silently fell back to default. Map low_battery -> PowerSource_LowBattery.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…wn leak (microsoft#1134)

- microsoft#1221: OfflineStorageHandler::GetAndReserveRecords wrote m_lastReadCount and
  m_readFromMemory with no synchronization while IsLastReadFromMemory() and
  LastReadRecordCount() read them from the upload path (TSan-reported on iOS).
  Make both members std::atomic so every access is well-defined; all uses are
  by-value loads/stores/fetch-add, so no other change is needed.

- microsoft#1134: SqliteDB had no destructor, so a SqliteDB destroyed without an explicit
  shutdown() (e.g. when the owning OfflineStorage_SQLite is torn down without
  Shutdown()) leaked its open handle and prepared statements -- the one-time
  sqlite allocation seen under ASan. Add ~SqliteDB() that calls the existing
  idempotent shutdown() (finalizes statements, closes the db, releases the
  instance count); an earlier explicit shutdown() makes it a no-op.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Utils.cpp microsoft#1333: the explicit RoUninitialize() only ran on the normal return
path, so a throwing WinRT call (e.g. TemporaryFolder access) between
RoInitialize() and it would leave a successful RoInitialize() unbalanced.
Move the balance into an RAII guard so it runs on every exit path including
exceptions; the WinRT StorageFolder is still released in an inner scope before
the guard runs, so it is not destroyed in an uninitialized apartment.
  Verified against lib/utils/Utils.cpp:105-127.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
load_Json_RuleWithLowBatteryPowerState_MapsToPowerSourceLowBattery loads a
profile whose rule uses "powerState": "low_battery" and asserts the parsed
rule maps to PowerSource_LowBattery. Verified it fails against the pre-fix code
(the key was absent from transmitProfilePowerState, so powerState fell back to
the default PowerSource_Any) and passes with the fix. Full UnitTests: 531/531.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OfflineStorageHandler::Flush() called m_offlineStorageDisk->Flush() in the
CFG_BOOL_CHECKPOINT_DB_ON_FLUSH branch without a null check. With RAM-only
storage (no disk backend, e.g. HAVE_MAT_STORAGE disabled) m_offlineStorageDisk
is null, so enabling that config would dereference null and crash. Guard the
call with m_offlineStorageDisk, matching the null checks elsewhere in Flush().
  Verified at lib/offline/OfflineStorageHandler.cpp:221-225.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Adds BasicFuncTests.teardownDuringInFlightUpload_ShutsDownCleanly: uploads are
pointed at the /slow/ endpoint with large payloads and MAX_TEARDOWN_TIME is 0,
so FlushAndTeardown() returns while an upload is still outstanding. Under a
sanitizer this guards the teardown-vs-upload path exercised by the shutdown
safety changes in this PR. Motivated by microsoft#1391; the specific reported
use-after-free did not reproduce in the loopback harness, so this is a
defensive smoke test rather than a microsoft#1391 regression.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
OfflineStorageHandler::Flush() early-returned when m_logManager.StartActivity()
failed (LogManager shutting down) without posting m_flushComplete or clearing
m_flushPending. If a memory-overflow async flush was scheduled and then ran
after teardown had begun, WaitForFlush() -- called from Shutdown() and the
destructor -- would block forever on m_flushComplete, deadlocking teardown.

This is the hang the new teardownDuringInFlightUpload_ShutsDownCleanly smoke
test exposed in CI (a 6-hour stall on the Linux/Windows/macOS test jobs): the
large-payload + MAX_TEARDOWN_TIME=0 configuration reliably races an in-flight
memory flush against teardown.

Signal completion (post m_flushComplete, clear m_flushPending, cancel the
handle) on the early-return path so WaitForFlush() cannot hang. Verified: the
full FuncTests suite (40 tests) now completes; previously it hung indefinitely
after sendOneEvent_immediatelyStop.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…microsoft#1481)

The EDEADLK self-join was a symptom of using std::async(std::launch::async) for
the HTTP send: the returned std::future joins its worker thread on destruction, so
when the async callback caused the operation to be destroyed on that same worker
thread (OnHttpResponse -> EventsUploadContext::clear()), ~future self-joined and
aborted the process out of the noexcept destructor.

Rather than detect-and-defer that self-join (the previous approach: published
thread id + atomic flag + heap-move the future to a detached helper, with OOM/
thread-exhaustion fallbacks), remove the joining future entirely:

- CurlHttpOperation now derives from enable_shared_from_this. SendAsync runs Send()
  on a detached std::thread that holds a shared_ptr keepalive to the operation, so
  the operation (and its curl handle, response buffer, and by-reference request
  body) stays alive until the worker finishes -- the same lifetime guarantee the
  destructor's result.wait() used to provide.
- There is no future, so ~CurlHttpOperation never joins anything and is safe on any
  thread, including the worker thread itself. The destructor drops to plain curl
  cleanup.
- Removes the future member, the m_asyncThreadId/m_asyncThreadIdSet machinery, and
  the <future>/<new> includes. Net -54 lines in the client.

Adds HttpClientCurlTests.SendAsync_DestroyOnWorkerThread_NoSelfJoin, which drops the
last external reference from inside the callback (on the worker thread) -- the exact
microsoft#1481 trigger. It aborts the process on the old std::async code and passes on this
fix.

Verified on Linux GCC 13: all HttpClientCurlTests (12) pass including the new
regression; the full FuncTests suite (39) passes with the curl client.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…xceptions, tidy test

- requestBody use-after-free (comments 1 & 3): the old blocking destructor kept the
  by-reference body alive because destroying the request waited for Send(). With the
  self-keepalive worker the operation can outlive the request, so a reference into
  CurlHttpRequest::m_body could dangle mid-send. CurlHttpOperation now takes the body
  by value and owns it, so it is valid for the operation's whole lifetime regardless
  of when the request is released. Costs one body copy per request (the prior
  zero-copy relied on the blocking wait that caused microsoft#1481).
- Detached-worker exceptions (comment 2): an exception escaping Send()/callback would
  call std::terminate, whereas the old std::async captured (and effectively swallowed)
  it. Wrap the worker body in try/catch to preserve the non-terminating behavior.
- Test (comment 4): replace the raw new/delete shared_ptr box with a
  shared_ptr<shared_ptr<CurlHttpOperation>> whose contained pointer is reset in the
  callback, so it cannot leak if SendAsync throws.

Verified on Linux GCC 13: all HttpClientCurlTests (12) pass including the self-join
regression; full FuncTests (39) pass with the by-value body.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…est host, tidy comment

HttpClient_Curl.cpp:84 (comment 3547544648): the operation takes the request
body by value, so hand it curlRequest->m_body via std::move instead of copying.
m_body is a per-send copy of the EventsUploadContext body (the retry source of
truth), so moving it is safe and avoids duplicating peak upload memory.

HttpClientCurlTests.cpp:150 (comment 3547544635): replace the fixed port 9 URL
with an RFC 6761 .invalid host so Send() fails fast and deterministically on any
environment (a fixed port could happen to be open). connTimeout=1 still bounds it.

HttpClient_Curl.hpp:183 (comment 3547544604): the destructor comment now says the
request body is owned (by value), not by-reference, matching the current design.

Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass
(incl. SendAsync_DestroyOnWorkerThread_NoSelfJoin) and full FuncTests 39/39 pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses Copilot review comment (WorkerThread.cpp self-Join detach path):
WorkerThread::Join() deletes any tasks still queued behind the shutdown
sentinel only after a successful join(). On the self-Join path (a task on the
worker thread triggers the dispatcher's own teardown) Join() detaches instead
of joining and deliberately skips that cleanup, because the still-running
worker may access the queues. As a result, future-dated timer tasks left in
m_timerQueue when the worker breaks on the shutdown sentinel were leaked.

Fix: when the worker processes the Shutdown item it now drains and deletes any
remaining m_queue/m_timerQueue entries under m_lock before exiting. This closes
the detach-path leak without racing Join() (the worker owns the queues while it
runs) and matches the join()-path behavior of dropping un-run work at shutdown.

Validated on Linux (WSL, Debug): PalTests + TransmissionPolicyManagerTests
(47) pass and full FuncTests (40, incl. the teardown smoke test) pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
… harden test promise

HttpClient_Curl.hpp SendAsync (comment 3547753859): if std::thread creation
throws (e.g. resource exhaustion) the exception previously escaped SendAsync(),
which both violates the IHttpClient::SendRequestAsync contract that the callback
is always invoked and, on the PAL worker thread (no try/catch), would terminate
the process. The worker body is now a named lambda; thread start is wrapped in
try/catch and on failure the operation runs synchronously as a fallback so the
callback still fires and no exception escapes.

HttpClientCurlTests.cpp (comment 3547753886): the regression test captured the
stack std::promise by reference, so if the ASSERT timed out and the test
returned early, the detached worker could call set_value() on a destroyed
promise. The promise is now heap-owned (shared_ptr) and captured by value, so an
early return cannot turn into a use-after-scope.

Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass and
FuncTests compiles clean.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses Copilot review comment (BasicFuncTests.cpp:582): the test rewrote the
base URL from /simple/ to /slow/ only when /simple/ was found, so if the base URL
format ever changed the rewrite would silently no-op and the test would pass
without exercising teardown during an in-flight upload. Replaced the conditional
rewrite with an ASSERT_NE on the find result so the coverage fails loudly instead
of lapsing silently.

Validated on Linux (WSL, Debug): the test still runs against /slow/ and passes.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses three Copilot review comments (TransmissionPolicyManager.cpp:119,
202, 266). This PR changed these LOG_TRACE format strings from %d to %lld but
passed std::chrono::milliseconds::rep directly. That rep is implementation-
defined and is long on LP64 (Linux/macOS), so %lld (which expects long long)
is a -Wformat mismatch -- an error under the project's -Wall -Werror in
logging-enabled (HAVE_MAT_LOGGING) builds, and formally UB in the varargs call.

Cast each count() to long long so the format always matches on every data model.
This mirrors the cast this PR already applies to delta (static_cast<unsigned
long long> with %llu) a few lines up.

Verified: clang 18 -Wall -Werror -Wextra flags the uncast %lld as
"format specifies type 'long long' but the argument has type 'rep' (aka 'long')"
and accepts the cast form. TransmissionPolicyManagerTests (40) pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…thread-start catch, fix test comment

HttpClient_Curl.hpp (comment 3548205832): WaitOnSocket() uses std::numeric_limits
but the header only included <numeric>, not <limits> -- it had relied on <future>
(removed by this PR) to pull <limits> transitively. Added an explicit <limits>
include so the header is self-contained.

HttpClient_Curl.hpp SendAsync (comment 3548205850): the thread-start fallback only
caught std::system_error, but std::thread construction can also throw std::bad_alloc
while allocating the callable. Broadened the catch to const std::exception& so any
thread-start failure still falls back to a synchronous run and never escapes
SendAsync() (which would terminate on the PAL worker thread).

HttpClientCurlTests.cpp (comment 3548205863): dropped the misleading "connTimeout=1
bounds it" note -- CurlHttpOperation ignores its httpConnTimeout arg (WaitOnSocket
uses the HTTP_CONN_TIMEOUT constant), so the .invalid host's immediate name-
resolution failure, not the timeout, is what makes Send() fail fast.

Validated on Linux (WSL, Debug): all 12 HttpClientCurl* unit tests pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…n SendAsync

Comment 3548251461: SendAsync() called shared_from_this() unconditionally. Every
CurlHttpOperation is created via make_shared (HttpClient_Curl.cpp:89), so this is
safe today, but if a future caller ever constructs one outside a shared_ptr
(stack / unique_ptr) shared_from_this() throws std::bad_weak_ptr, which would
escape SendAsync() BEFORE the thread-start try/catch and could terminate the
caller thread -- breaking the "SendAsync never lets an exception escape / the
callback is always invoked" property established in the earlier rounds.

Guarded shared_from_this() with a std::bad_weak_ptr catch that falls back to a
synchronous run (the caller owns the non-shared object for the duration). Also
extracted the shared Send()+callback body into RunSendAndCallback() so the
detached worker, the thread-start fallback, and this new no-shared fallback all
use one implementation.

Added regression test SendAsync_NotSharedOwned_RunsSynchronouslyNoThrow.

Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…iew round 7)

Comment 3548399891: the note claimed curlRequest->m_body was a "per-send copy of
the EventsUploadContext body (the retry source of truth)". That's inaccurate --
the encoder MOVES ctx->body into the request (SimpleHttpRequest::SetBody does
m_body = std::move(body), IHttpClient.hpp:310) and then clears ctx->body
(HttpRequestEncoder.cpp:165-167), so m_body is the sole owner of the payload and
ctx->body is not a retained retry buffer. Reworded to describe the actual
ownership and why moving m_body is safe (the request is single-use and released
with the EventsUploadContext). No code change.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment 3548517158: on the (practically unreachable) 15s-timeout path the detached
worker could still be running when the fixture tears down -- and the fixture holds
HttpClient_Curl m_client (its dtor calls curl_global_cleanup) plus the
m_headers/m_body the worker may still read -- risking a secondary crash unrelated
to the regression.

On timeout, best-effort cancel the still-running operation and wait briefly before
failing, so the worker is much less likely to outlive teardown. The cancel handle
is a std::weak_ptr so it does not keep the operation alive (an owning ref would
defeat the test: the callback's box->reset() must remain the last external ref).

Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass (NoSelfJoin
normal path still ~45ms).

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…t#1481 review round 9)

Comment 3548602231: the worker lambda was constructed before the try/catch.
Copying callback (a std::function) into it can throw std::bad_alloc, which
would escape SendAsync() despite the intent that any failure fall back to a
synchronous run. Construct the lambda inline inside the std::thread() call within
the try so a throwing capture-copy is caught alongside a thread-start failure;
the catch now calls RunSendAndCallback(callback) directly (self keeps this
operation alive for the synchronous run). This also drops the separate named
worker variable.

Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The cross-platform native callback and teardown changes require final human validation despite no confirmed blocking defect.

Review effort: Balanced
Findings: None

Resolved since last review (1)

Prevent concurrent log calls from dereferencing the debug stream while another PAL instance shuts logging down.

Files changed:
- lib/pal/PAL.cpp: hold the logging mutex across state checks and writes

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 306b454f-fc1d-4429-923b-04d0894b1e35
Preserve the independently validated SQLite shutdown fix while adding serialized PAL logging teardown.

Files changed:
- docs/Offline-storage-settings.md: document shutdown retry ownership
- lib/offline/SQLiteWrapper.hpp: retain temporary storage until successful shutdown
- tests/unittests/OfflineStorageTests_SQLite.cpp: cover failed shutdown retry
- lib/pal/PAL.cpp: serialize logging teardown

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 306b454f-fc1d-4429-923b-04d0894b1e35

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the substantial hardening work here. The new response-size limits, stronger curl TLS defaults, WinHTTP certificate checks, and broader cancellation tests are valuable. I found several correctness, security, and compatibility concerns that should be addressed before merging. The most important are a WinInet credential-exposure window, callback and worker lifetime bugs, no-exception build failures, and a Windows 7 TLS regression. I left specific mitigation suggestions inline.

Comment thread lib/http/HttpClient_WinInet.cpp
Comment thread lib/http/HttpClient_WinHttp.cpp
Comment thread lib/pal/WorkerThread.cpp Outdated
Comment thread lib/pal/WorkerThread.cpp
Comment thread lib/http/HttpClient_Curl.cpp
Comment thread lib/offline/OfflineStorageHandler.cpp Outdated
Comment thread lib/pal/TaskDispatcher_CAPI.cpp Outdated
Comment thread Solutions/net48/net48.vcxproj
Comment thread tools/ports/cpp-client-telemetry/portfile.cmake
Comment thread lib/http/HttpClientManager.cpp Outdated
bmehta001 and others added 5 commits September 21, 2026 12:46
Preserve the modules failure-settlement changes while incorporating the latest modules master updates.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Prevent callback and worker lifetime races, reject unsafe WinInet MS-root requests before sending, preserve queued records and rejected host cancellations, and verify Curl's exception-disabled path.

Document the Windows and .NET support floors and make unsupported pinned vcpkg transport selection fail clearly.

Files changed: HTTP transports and manager, worker/C API dispatchers, offline storage, regression tests, Windows documentation, vcpkg port, and Linux CI.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Install the development package required during configuration so the focused Curl compile gate runs on clean hosted runners.

Files changed: .github/workflows/build-posix-latest.yml

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Include MatsdkOptions.cmake in the vcpkg capability check so local and current sources are accepted while stale pinned releases still fail clearly.

Files changed: tools/ports/cpp-client-telemetry/portfile.cmake

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Expect WinInet to reject Microsoft-root enforcement before transmission, matching the documented security behavior while preserving WinHTTP certificate validation coverage.

Files changed: tests/functests/APITest.cpp

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The change spans transport lifetimes, storage semantics, packaging, and multiple platform toolchains, warranting final human validation.

Review effort: Balanced
Findings: None

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for addressing the earlier review. Most of the original concerns are now resolved. I found three remaining edge cases in the updated lifetime and storage fixes. They can still cause use-after-free, stalled shutdown, or record loss, so I recommend addressing them before merging.

Comment thread lib/http/HttpClientManager.cpp
Comment thread lib/http/HttpClient_WinHttp.cpp Outdated
Comment thread lib/offline/OfflineStorageHandler.cpp Outdated
@bmehta001 bmehta001 linked an issue Sep 22, 2026 that may be closed by this pull request
Wait for peer callbacks while allowing reentrant destruction, and keep queued response work independent of the manager lifetime.

Restore state-callback accounting after exceptions on both Windows transports, preserve terminal response delivery, and recover memory-only records when a later disk batch throws.

Files changed: HttpClientManager, WinHTTP/WinInet transports, OfflineStorageHandler, and focused unit regressions.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot-Session: 3cf2bbe2-fa21-4ec0-86a3-b3d61d0e882d
@bmehta001 bmehta001 linked an issue Sep 22, 2026 that may be closed by this pull request

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing the three previous findings. I confirmed those fixes and their focused tests. I found two remaining reentrant exception paths in the new lifetime handling that can still cause use-after-free or process termination.

Comment thread lib/http/HttpClientManager.cpp Outdated
Comment thread lib/http/HttpClient_WinInet.cpp Outdated
Keep failure routing in callback-owned state and recheck manager attachment before the completion route, preventing access after a failure listener destroys the manager.

Make WinInet setup and state-callback guards non-throwing so terminal completion cannot escape during callback unwinding.

Files changed: HttpClientManager.cpp/.hpp, HttpClient_WinInet.cpp, HttpClientManagerTests.cpp, and HttpClientTests.cpp.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for fixing the manager failure-route lifetime issue and the WinInet double-exception termination path. One exception-safety gap remains in the new WinInet completion handling.

Comment thread lib/http/HttpClient_WinInet.cpp
Install no-throw terminal guards in WinInet and WinHTTP so response construction or header-processing failures always close native handles and drain the request registry. Contain post-claim exceptions at the transport callback boundary and cover both implementations with deterministic fault injection.

Files changed:
- lib/http/HttpClient_WinInet.{cpp,hpp}: guarantee terminal cleanup and expose the internal test hook.
- lib/http/HttpClient_WinHttp.{cpp,hpp}: apply the same invariant to the default Windows transport.
- tests/unittests/HttpClientTests.cpp: verify finalization faults cannot strand CancelAllRequests.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot review overview

🔵 Needs a closer look

The broad cross-platform concurrency, native transport, storage, packaging, and framework changes require final human validation despite extensive tests.

Review effort: Balanced
Findings: None

@bmehta001
bmehta001 merged commit d834f65 into microsoft:main Sep 22, 2026
53 checks passed
@bmehta001
bmehta001 deleted the bhamehta/winhttp-default-windows-transport branch September 22, 2026 07:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

100% CPU spin on CancelAllRequests GetAppLocalTempDirectory leaks a RoInitialize call

3 participants