Skip to content

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

Open
bmehta001 wants to merge 282 commits into
microsoft:mainfrom
bmehta001:bhamehta/winhttp-default-windows-transport
Open

bmehta001 wants to merge 282 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

  • 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 Debug, WinHTTP: 25 HttpClientTests and APITest.WindowsHttpTransport_MsRoot_Check passed.
  • Linux/WSL Debug: 13 HttpClientTests passed.
  • Win32 Release with WinInet: 578 unit tests and 46 functional tests passed during transport compatibility validation.
  • 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 22, 2026 10:12
…null

~WinInetRequestWrapper closed m_hWinInetSession only inside the
`if (m_hWinInetRequest != nullptr)` block. When HttpOpenRequest fails
after InternetConnect succeeded, the wrapper is destroyed with a null
request handle but a live session handle, leaking an internet handle on
every such failure (accumulates over process lifetime). Close each
handle under its own null check.

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

Two latent data-loss bugs found during a repo-wide review:

1) MemoryStorage::DeleteRecords(whereFilter) matched EVERY record when
   whereFilter was empty (the matcher starts `matched = true` and the
   per-key loop never runs), silently wiping the entire in-memory queue.
   This contradicts the fail-closed OfflineStorage_SQLite::DeleteRecords
   and the Room backend. Guard an empty filter and return without
   deleting; intentional full clears use DeleteAllRecords().

2) OfflineStorage_SQLite::StoreRecord ignored the bool returned by
   SqliteStatement::execute(), returning true and bumping
   m_DbSizeEstimate even on a real write failure (SQLITE_FULL/IOERR/etc).
   The event is silently lost with no OnStorageFailed notification and
   the size estimate drifts. Capture the result; on failure log, notify
   the observer, and return false (skipping the size bump).

Tests: added MemoryStorageTests.DeleteRecordsWithEmptyFilterDoesNotDeleteAll
(fails without the guard -- the queue is wiped to 0; passes with it).
The StoreRecord write-failure path isn't unit-testable here (the insert
is REPLACE INTO with no constraint to violate), so it's covered by build
+ review. Verified locally on Linux: all 9 MemoryStorageTests and 32
OfflineStorageTests_SQLite pass.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Verified TDD: this test fails without the empty-filter guard (the queue
is wiped, GetSize()/GetRecordCount() drop to 0) and passes with it.
Run on Linux host.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
lib/offline/OfflineStorage_SQLite.cpp::StoreRecord now returns false on a write
failure (this PR), but OfflineStorageHandler::StoreRecord ignored the disk
result and always returned true, so a failed synchronous store (RAM queue
disabled or during shutdown) was counted as successfully persisted by
StoreRecords()/StorageObserver.

Return the disk StoreRecord() result in the direct-to-disk path. The memory
path is unchanged: MemoryStorage::StoreRecord returning false means an
intentional latency-Off skip, not a failure, so it must not surface as an error.

Verified at lib/offline/OfflineStorageHandler.cpp:266-275 and
lib/offline/OfflineStorage_SQLite.cpp:180-186.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Combine the Flush() data-loss fix into this storage-data-safety PR (the two are
halves of the same fix: this PR already makes OfflineStorage_SQLite::StoreRecord
report write failures; Flush() must act on that).

OfflineStorageHandler::Flush() previously drained the in-memory queue with
GetRecords() (which removes records) and handed them to StoreRecords() before
confirming persistence. On a partial/total disk write failure the un-persisted
records were already gone from memory and never re-queued -> events lost.

Flush() now drains into a local batch, persists one record at a time, and
re-inserts only the records that fail to persist (so failures are retried, not
lost). Per-record StoreRecord() is used deliberately: a batched StoreRecords()
only returns a count, so on a partial failure we could not tell which records to
re-queue, and re-storing already-saved records would duplicate them (no unique
record_id constraint). Also null-guards the dbSizeBeforeFlush read so Flush() is
safe with disk-only storage (CFG_INT_RAM_QUEUE_SIZE == 0).

Adds OfflineStorageHandlerFlushTests.FailedDiskWriteDuringFlushReturnsRecordsToMemory
(records the SQLite store rejects stay in memory after Flush; verified it fails
against the previous GetRecords()-based Flush). Closes the separate PR microsoft#1496.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The test helper's Cancel() returned true unconditionally, violating the
ITaskDispatcher::Cancel contract (return whether the task was found/cancelled).
Return true only when the task was present in the queue, false otherwise.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Rename FailedDiskWriteDuringFlush... -> FailedDiskStoreDuringFlush... and reword
its comments: the test exercises a disk StoreRecord() rejection (SQLite input
validation), which drives the same Flush() re-queue path as any disk store
failure, not a literal disk write/IO error.

(The reviewer's separate note that Flush() ignores EventPersistence_DoNotStoreOnDisk
is a pre-existing behavior, out of scope for this data-safety change and not
cleanly unit-testable via the public API; tracked as a follow-up.)

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…(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>
Use the published filter snapshot as the single source of truth so registration cannot race the privacy-filter fast path. Limit legacy sample Curl linkage to Curl-based platforms and let SQLite RAII cleanup run when explicit shutdown is skipped.

Rename the .NET Framework 4.8 sample so its project identity matches its actual target.

Files changed:
- Event filter collection
- CMake sample dependency fallback
- SQLite storage and unit coverage
- .NET sample, solution, build script, and Windows guide

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

Copilot-Session: c4cfcad8-1637-4e46-86cf-6bf200244b04
Apply the code, build, test, and reference updates that accompany the .NET sample move. This prevents filter publication races, removes irrelevant Curl linkage on native platforms, and preserves SQLite cleanup without explicit shutdown.

Files changed:
- Event filter collection
- CMake sample dependency fallback
- SQLite storage and unit coverage
- .NET sample metadata, solution, build script, and Windows guide

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

Copilot-Session: c4cfcad8-1637-4e46-86cf-6bf200244b04

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.

🟡 Changes recommended

Memory-only events can reach persistent storage, no-exception compatibility is regressed, and the C# sample writes a property to the wrong event.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (2)

lib/offline/OfflineStorageHandler.cpp:481

  • The per-record fallback also sends EventPersistence_DoNotStoreOnDisk records to m_storage->StoreRecord(). This path is used when batching is unsupported, so memory-only events can still be persisted even if the batched path is corrected; requeue these records without invoking the provider.
    lib/offline/OfflineStorageHandler.cpp:114
  • The newly added raw try/catch syntax bypasses the MATSDK_TRY/MATSDK_CATCH layer used for builds with HAVE_EXCEPTIONS=0 (lib/include/public/ctmacros.hpp:132-138). Convert this and the other new handlers in this file to those macros so the no-exception mini configurations remain buildable.
  • Files reviewed: 136/145 changed files
  • Comments generated: 2
  • Review effort level: Balanced

Comment thread lib/offline/OfflineStorageHandler.cpp Outdated
Comment thread lib/offline/OfflineStorageHandler.cpp Outdated
Partition DoNotStoreOnDisk records out of both batched and per-record persistence so flush returns them to memory instead of writing them to SQLite. Use the repository exception macros so exception-disabled builds retain their supported control flow, and correct the C# sample sequence property target.

Files changed:
- lib/offline/OfflineStorageHandler.cpp
- tests/unittests/OfflineStorageTests.cpp
- examples/cs/SampleCsNet48/Program.cs

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

Copilot-Session: c4cfcad8-1637-4e46-86cf-6bf200244b04

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.

🔵 Needs a closer look

The broad cross-platform concurrency, teardown, transport, storage, and build-system changes warrant final human validation despite no confirmed blocking defect.

Review details
  • Files reviewed: 137/145 changed files
  • Comments generated: 0 new
  • Review effort level: Balanced

Initialize the private native test SDK with Android HTTP, Room, PAL, and cache-path state so tests exercise valid platform services without cross-library JNI callbacks.

Harden Android HTTP singleton lifetime and cancellation results; fail explicitly when the default client is unavailable.

Validate batched disk records consistently, use writable session-test paths, and fix epoll and Android PAL JNI defects exposed by the full device run.

Files changed: Android test bridge/configuration, HttpClient_Android, LogManagerImpl, OfflineStorageHandler, Android PAL, and related unit tests.

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

Copilot-Session: c4cfcad8-1637-4e46-86cf-6bf200244b04

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.

🟡 Changes recommended

The new SQLite batch path uses raw exception syntax that breaks supported no-exception Win32 mini builds.

Get a fresh assessment by requesting another Copilot review.

Review details
  • Files reviewed: 146/155 changed files
  • Comments generated: 1
  • Review effort level: Balanced

Comment thread lib/offline/OfflineStorage_SQLite.cpp Outdated
Close the prior functional test before deleting and reopening its SQLite database, so retry limits are tested against the intended persisted event.

Use the optional-exception abstraction in OfflineStorage_SQLite batch writes so no-exception mini builds remain supported.

Files changed: lib/offline/OfflineStorage_SQLite.cpp, tests/functests/BasicFuncTests.cpp

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

Copilot-Session: c4cfcad8-1637-4e46-86cf-6bf200244b04

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.

🟡 Changes recommended

Four changed raw exception handlers violate the repository abstraction and break supported no-exception mini builds.

Get a fresh assessment by requesting another Copilot review.

Review details

Suppressed comments (1)

lib/api/LogManagerImpl.cpp:996

  • EndActivity() is part of the same library built by the no-exception mini targets, so this new raw handler triggers their warnings-as-errors failure. Replace it with the established MATSDK_TRY/MATSDK_CATCH abstraction from ctmacros.hpp:123-138.
  • Files reviewed: 145/155 changed files
  • Comments generated: 3
  • Review effort level: Balanced

Comment thread lib/api/LogManagerImpl.cpp Outdated
Comment thread lib/pal/TaskDispatcher_CAPI.cpp
Comment thread lib/pal/WorkerThread.cpp Outdated
bmehta001 and others added 3 commits September 18, 2026 10:04
Rename the net40 wrapper and configuration surfaces so modern Visual Studio builds can include the managed projects by default.

Correct the C# sibling project reference and keep the indestructible PAL singleton in static storage so teardown remains safe without a process-lifetime heap allocation.

Files changed: Visual Studio projects and solution; Windows build, CI, deployment, and packaging scripts; Windows setup documentation; PAL singleton storage.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Route Empty() through Size() so the collection has one canonical snapshot-based size calculation.

Files changed: lib/filter/EventFilterCollection.cpp.

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Replace raw handlers in LogManager teardown, activity cleanup, C API dispatch, and worker dispatch with the optional-exception abstraction so mini builds execute the protected operations directly.

Files changed: lib/api/LogManagerImpl.cpp, lib/pal/TaskDispatcher_CAPI.cpp, lib/pal/WorkerThread.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

🟡 Changes recommended

SQLite frees its global temporary-directory buffer before shutdown, creating a dangling-pointer risk during final teardown.

Get a fresh assessment by requesting another Copilot review.

Review effort: Balanced
Findings: 1 High severity

Open (1)
Resolved since last review (3)

Comment thread lib/offline/SQLiteWrapper.hpp
bmehta001 and others added 2 commits September 19, 2026 00:11
Explicitly reference caught exceptions because logging may compile out, otherwise Windows warning-as-error builds cannot validate the transport lifetime fixes.

Files changed:
- lib/http/HttpClient_CAPI.cpp
- lib/http/HttpClientManager.cpp

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

Copilot-Session: 306b454f-fc1d-4429-923b-04d0894b1e35
Keep sqlite3_temp_directory alive when SQLite cannot shut down, allowing a later lifecycle retry without leaving a dangling process-global pointer.

Document host-owned lifecycle requirements for multiple embedded 1DS copies that share one SQLite runtime.

Files changed: lib/offline/SQLiteWrapper.hpp, tests/unittests/OfflineStorageTests_SQLite.cpp, docs/Offline-storage-settings.md

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
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.

2 participants