Conversation
…(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>
… into bhamehta/fix-curl-async-self-join
…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>
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
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
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.
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>
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
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.
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
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
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.
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>
Baiju Meswani (baijumeswani)
left a comment
There was a problem hiding this comment.
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.
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>

Summary
Notable fixes
Compatibility notes
http.msRootCheck=truebefore 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.MATSDK_USE_WININET=ONin CMake orMATSDK_USE_WININET=truein Visual Studio builds to retain WinInet.http.sslVerify=falseremains accepted for configuration compatibility but is ignored. Curl always enables peer and hostname verification; development environments using private or self-signed certificates must configurehttp.sslCaInfowith a trusted CA bundle.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.CancelAllRequests()now retires created-but-unsent requests instead of waiting indefinitely; a later send receives exactly oneAbortedcallback.Validation
-fno-exceptions: the Curl transport translation unit compiled successfully; CI now enforces this configuration.