Conversation
StoreRecords() previously inserted each record in its own transaction
(StoreRecord -> DbTransaction -> BEGIN EXCLUSIVE/COMMIT), so a flush of N records
performed N transactions (N WAL fsyncs). Batch the whole vector into a single
transaction, sharing the per-record work via extracted helpers
(isValidRecord / insertRecordUnsafe / checkStorageSizeLimits). StoreRecord()
keeps its own single-record transaction; behavior is unchanged. This implements
the existing in-code TODO ("consider running the batch in transaction").
Measured against the SDK's vendored sqlite (WAL, synchronous=NORMAL,
BEGIN EXCLUSIVE): N=200 ~11x, N=1000 ~40x faster for the insert batch.
MemoryStorage::GetAndReserveRecords: when leaseTimeMs==0 (the flush-read path,
where the record is consumed and popped unconditionally), move the record
straight to the consumer instead of deep-copying it first.
Adds OfflineStorageTests_SQLite.StoreRecordsBatchStoresAllRecordsInOneTransaction.
Full UnitTests suite (524) passes.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR optimizes the offline-storage “flush” path by batching SQLite inserts into a single exclusive transaction and by eliminating an extra deep-copy on the in-memory flush-read path, aiming to reduce fsync overhead and improve throughput without changing functional behavior.
Changes:
- Batch
OfflineStorage_SQLite::StoreRecords()inserts into a singleBEGIN EXCLUSIVE/COMMITcycle via extracted helpers. - Refactor per-record validation/insert/size-limit logic into private helpers (
isValidRecord,insertRecordUnsafe,checkStorageSizeLimits). - Optimize
MemoryStorage::GetAndReserveRecords()to move records directly to the consumer whenleaseTimeMs == 0, avoiding a deep copy.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests/unittests/OfflineStorageTests_SQLite.cpp | Adds a new unit test covering batch storage behavior. |
| lib/offline/OfflineStorage_SQLite.hpp | Declares new private helper methods to support batched inserts. |
| lib/offline/OfflineStorage_SQLite.cpp | Implements single-transaction batch insert path and refactors record storage logic. |
| lib/offline/MemoryStorage.cpp | Avoids a deep copy on the no-reservation (flush-read) path by moving records to the consumer. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+167
to
+168
| // The whole batch is committed in a single transaction. | ||
| EXPECT_THAT(offlineStorage->StoreRecords(batch), kCount); |
Comment on lines
+242
to
+246
| if (!m_db) { | ||
| LOG_ERROR("Failed to store %zu events: Database is not open", records.size()); | ||
| m_observer->OnStorageOpenFailed("Database is not open"); | ||
| return 0; | ||
| } |
Comment on lines
+271
to
+273
| if (stored) { | ||
| checkStorageSizeLimits(); | ||
| } |
| EXPECT_THAT(consumer.records[0].reservedUntil, 0); | ||
| } | ||
|
|
||
| TEST_F(OfflineStorageTests_SQLite, StoreRecordsBatchStoresAllRecordsInOneTransaction) |
- Rename StoreRecords batch test (drop the "InOneTransaction" claim it did not verify) and reword its comment: the test covers batch storage correctness; the single-transaction batching is a perf optimization verified by benchmarking. - Document the intended batching side-effect changes: the DB-open failure is now reported once for the whole batch (was once per record), and the size-full notification / resize check runs once after the batch commit (m_DbSizeEstimate already reflects all inserts; the resize result is unchanged). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+163
to
+165
| bool wantMore = consumer(std::move(record)); | ||
| m_records[latency].pop_back(); | ||
| m_size -= std::min(m_size, recordSize); |
Comment on lines
+242
to
+248
| if (!m_db) { | ||
| // Report once for the whole batch (the previous per-record loop | ||
| // reported once per record). | ||
| LOG_ERROR("Failed to store %zu events: Database is not open", records.size()); | ||
| m_observer->OnStorageOpenFailed("Database is not open"); | ||
| return 0; | ||
| } |
Comment on lines
+167
to
+175
| // Every record in the batch is stored and individually retrievable. (The | ||
| // single-transaction batching is a performance optimization verified by | ||
| // benchmarking; this test covers the batch's storage correctness.) | ||
| EXPECT_THAT(offlineStorage->StoreRecords(batch), kCount); | ||
|
|
||
| TestRecordConsumer consumer; | ||
| EXPECT_THAT(offlineStorage->GetAndReserveRecords(consumer, 100000), true); | ||
| ASSERT_THAT(consumer.records.size(), kCount); | ||
| } |
- Revert the MemoryStorage::GetAndReserveRecords leaseTimeMs==0 move-instead-of- copy optimization. It changed behavior if a leaseTimeMs==0 consumer ever returned false (the record would be consumed instead of kept). It is low-value and only safe under the current "leaseTimeMs==0 consumers never reject" contract, so drop it; this PR is now focused on the transaction batching. - Tighten StoreRecordsBatchStoresAllRecords to assert each record id is individually retrieved (matching the comment), not just the total count. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
changed the title
Comment on lines
+264
to
+267
| for (auto & i : records) { | ||
| if (!isValidRecord(i)) { | ||
| continue; | ||
| } |
Comment on lines
+161
to
+165
| void OfflineStorage_SQLite::insertRecordUnsafe(StorageRecord const& record) | ||
| { | ||
| SqliteStatement(*m_db, m_stmtInsertEvent_id_tenant_prio_ts_data).execute(record.id, record.tenantToken, static_cast<int>(record.latency), static_cast<int>(record.persistence), record.timestamp, record.blob); | ||
| m_DbSizeEstimate += record.id.size() + record.tenantToken.size() + record.blob.size(); | ||
| } |
- Validate all records BEFORE opening the transaction in StoreRecords(): build the list of valid records first (reporting rejects via OnStorageFailed), then open the single BEGIN EXCLUSIVE transaction and insert them. This keeps any observer callback out of the held transaction (matching the single StoreRecord(), which already validates before its transaction) and avoids re-entrancy/lock-duration hazards. - insertRecordUnsafe() now returns whether the INSERT actually succeeded and only increments m_DbSizeEstimate on success. StoreRecords() counts only successful inserts and StoreRecord() returns the real result, so a failed write no longer inflates the stored count or the size estimate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
222
to
240
| { | ||
| #ifdef ENABLE_LOCKING | ||
| LOCKGUARD(m_lock); | ||
| DbTransaction transaction(m_db.get()); | ||
| if (!transaction.locked) | ||
| { | ||
| LOG_ERROR("Failed to store event %s:%s: Database error", tenantTokenToId(record.tenantToken).c_str(), record.id.c_str()); | ||
| m_observer->OnStorageFailed("Database error"); | ||
| return false; | ||
| } | ||
| #endif | ||
| if (!insertRecordUnsafe(record)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| checkStorageSizeLimits(); | ||
|
|
||
| return true; |
Comment on lines
+163
to
168
| if (!SqliteStatement(*m_db, m_stmtInsertEvent_id_tenant_prio_ts_data).execute(record.id, record.tenantToken, static_cast<int>(record.latency), static_cast<int>(record.persistence), record.timestamp, record.blob)) | ||
| { | ||
| LOG_ERROR("Failed to store event %s:%s: database write failed", | ||
| tenantTokenToId(record.tenantToken).c_str(), record.id.c_str()); | ||
| m_observer->OnStorageOpenFailed("Database is not open"); | ||
| return false; | ||
| } |
Comment on lines
+250
to
+254
| if (!m_db) { | ||
| // Report once for the whole batch (the previous per-record loop | ||
| // reported once per record). | ||
| LOG_ERROR("Failed to store %zu events: Database is not open", records.size()); | ||
| m_observer->OnStorageOpenFailed("Database is not open"); |
…cords Move the validation pass ahead of the !m_db check so StoreRecords() reports invalid records consistently with the single StoreRecord() (which validates first) even when the database is not open. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
222
to
240
| { | ||
| #ifdef ENABLE_LOCKING | ||
| LOCKGUARD(m_lock); | ||
| DbTransaction transaction(m_db.get()); | ||
| if (!transaction.locked) | ||
| { | ||
| LOG_ERROR("Failed to store event %s:%s: Database error", tenantTokenToId(record.tenantToken).c_str(), record.id.c_str()); | ||
| m_observer->OnStorageFailed("Database error"); | ||
| return false; | ||
| } | ||
| #endif | ||
| if (!insertRecordUnsafe(record)) { | ||
| return false; | ||
| } | ||
| } | ||
|
|
||
| checkStorageSizeLimits(); | ||
|
|
||
| return true; |
Comment on lines
+273
to
+297
| size_t stored = 0; | ||
| { | ||
| // Batch all inserts into a single transaction: one BEGIN EXCLUSIVE / | ||
| // COMMIT (one fsync) for the whole flush instead of one per record. | ||
| #ifdef ENABLE_LOCKING | ||
| LOCKGUARD(m_lock); | ||
| DbTransaction transaction(m_db.get()); | ||
| if (!transaction.locked) | ||
| { | ||
| LOG_ERROR("Failed to store %zu events: Database error", records.size()); | ||
| m_observer->OnStorageFailed("Database error"); | ||
| return 0; | ||
| } | ||
| #endif | ||
| for (auto * r : valid) { | ||
| if (insertRecordUnsafe(*r)) { | ||
| ++stored; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // Run the size-full notification / resize check once after the batch | ||
| // commit (instead of after every record): m_DbSizeEstimate already | ||
| // reflects all inserts and the resize result is the same. | ||
| if (stored) { |
insertRecordUnsafe() returns false on a failed INSERT but did not notify the
observer, so a write failure could be silently dropped. Both StoreRecord() and
StoreRecords() now call m_observer->OnStorageFailed("Database write failed") when
an insert fails -- after the transaction has closed, so the observer callback
never runs while BEGIN EXCLUSIVE is held.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
| size_t GetRecordCountUnsafe(EventLatency latency) const; | ||
|
|
||
| // Validate a record's required fields; reports OnStorageFailed on rejection. | ||
| bool isValidRecord(StorageRecord const& record); |
| } | ||
|
|
||
| bool OfflineStorage_SQLite::StoreRecord(StorageRecord const& record) | ||
| bool OfflineStorage_SQLite::isValidRecord(StorageRecord const& record) |
isValidRecord does not modify object state; mark it const for const-correctness. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+237
to
+242
| if (!stored) { | ||
| // Report the write failure after the transaction has closed, so the | ||
| // observer callback never runs while BEGIN EXCLUSIVE is held. | ||
| m_observer->OnStorageFailed("Database write failed"); | ||
| return false; | ||
| } |
Comment on lines
+312
to
+314
| if (stored) { | ||
| checkStorageSizeLimits(); | ||
| } |
Comment on lines
+261
to
+265
| std::vector<StorageRecord*> valid; | ||
| valid.reserve(records.size()); | ||
| for (auto & i : records) { | ||
| if (StoreRecord(i)) { | ||
| ++stored; | ||
| if (isValidRecord(i)) { | ||
| valid.push_back(&i); |
Comment on lines
+161
to
+170
| bool OfflineStorage_SQLite::insertRecordUnsafe(StorageRecord const& record) | ||
| { | ||
| if (!SqliteStatement(*m_db, m_stmtInsertEvent_id_tenant_prio_ts_data).execute(record.id, record.tenantToken, static_cast<int>(record.latency), static_cast<int>(record.persistence), record.timestamp, record.blob)) | ||
| { | ||
| LOG_ERROR("Failed to store event %s:%s: database write failed", | ||
| tenantTokenToId(record.tenantToken).c_str(), record.id.c_str()); | ||
| m_observer->OnStorageOpenFailed("Database is not open"); | ||
| return false; | ||
| } | ||
| m_DbSizeEstimate += record.id.size() + record.tenantToken.size() + record.blob.size(); | ||
| return true; |
- Always run checkStorageSizeLimits() even when an insert fails. My earlier early-return on failure skipped it, so a full DB (SQLITE_FULL) could no longer trigger ResizeDb() and storage could get stuck failing writes. StoreRecord() and StoreRecords() now run the size/resize check regardless of insert success. - Use const pointers for the batch validation list (records are not modified). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+271
to
+279
| if (!m_db) { | ||
| LOG_ERROR("Failed to store %zu events: Database is not open", records.size()); | ||
| m_observer->OnStorageOpenFailed("Database is not open"); | ||
| return 0; | ||
| } | ||
|
|
||
| if (valid.empty()) { | ||
| return 0; | ||
| } |
Comment on lines
+237
to
+246
| if (!stored) { | ||
| // Report the write failure after the transaction has closed, so the | ||
| // observer callback never runs while BEGIN EXCLUSIVE is held. | ||
| m_observer->OnStorageFailed("Database write failed"); | ||
| } | ||
|
|
||
| // Always run the size-limit check, even on failure: a full-DB error | ||
| // (e.g. SQLITE_FULL) must still be able to trigger ResizeDb() so storage | ||
| // can recover. | ||
| checkStorageSizeLimits(); |
Comment on lines
+306
to
+315
| if (failed) { | ||
| // Report write failures once, after the transaction has closed. | ||
| m_observer->OnStorageFailed("Database write failed"); | ||
| } | ||
|
|
||
| // Always run the size-full notification / resize check once after the | ||
| // batch (even if every insert failed): a full DB must still be able to | ||
| // trigger ResizeDb() to recover. m_DbSizeEstimate already reflects all | ||
| // successful inserts. | ||
| checkStorageSizeLimits(); |
- In StoreRecords(), return early when no records are valid (already reported) before the DB-open check, matching StoreRecord() which returns after validation without checking DB-open. Log the valid count. - Stop overclaiming "recovery" in the size-check comments: checkStorageSizeLimits() runs after the batch to match the original per-record behavior; reliable full-DB recovery (refreshing the size estimate on failure) is a pre-existing concern out of scope for this batching change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Comment on lines
+291
to
296
| if (!transaction.locked) | ||
| { | ||
| LOG_ERROR("Failed to store %zu events: Database error", records.size()); | ||
| m_observer->OnStorageFailed("Database error"); | ||
| return 0; | ||
| } |
Comment on lines
+308
to
311
| if (failed) { | ||
| // Report write failures once, after the transaction has closed. | ||
| m_observer->OnStorageFailed("Database write failed"); | ||
| } |
| DbTransaction transaction(m_db.get()); | ||
| if (!transaction.locked) | ||
| { | ||
| LOG_ERROR("Failed to store %zu events: Database error", records.size()); |
The transaction-lock failure message in StoreRecords() logged records.size(), but the batch has already been filtered to valid records; log valid.size(). Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
bmehta001
added a commit
to bmehta001/cpp_client_telemetry
that referenced
this pull request
Jun 23, 2026
…(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>
Contributor
Author
|
Folded into #1491. The batched StoreRecords() optimization is combined with #1491's Flush() data-loss fix: StoreRecords() is now all-or-nothing (rolls back on any failure) and Flush() uses it, so the batch speedup and the no-loss/no-duplicate guarantee cooperate instead of conflicting. Driven to a clean Copilot review here over ~9 rounds; the final code lives in #1491. |
bmehta001
added a commit
that referenced
this pull request
Sep 22, 2026
* Fold the SQLite batch-flush optimization into the data-safety change (was PR #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 #1497. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot: make StoreRecords fully all-or-nothing on invalid records 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> * Address Copilot: re-queue the flush batch only on a zero store result 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> * Fix PrivacyGuard JNI UAF, RoInitialize leak, and missing low_battery profile Three small correctness fixes bundled with the offline-storage work: - #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. - #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. - #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> * Fix GetAndReserveRecords data race (#1221) and SQLite shutdown leak (#1134) - #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. - #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> * Balance RoInitialize with an RAII guard (Copilot round-1) Utils.cpp #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> * Add test for low_battery transmit-profile powerState (#312) 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> * Guard checkpoint-on-flush against null disk storage (Copilot round-3) 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> * Add teardown-during-in-flight-upload smoke test 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 #1391; the specific reported use-after-free did not reproduce in the loopback harness, so this is a defensive smoke test rather than a #1391 regression. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix teardown deadlock: always signal flush completion 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> * Replace std::async with a self-keepalive detached worker (real fix for #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 #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> * Address Copilot round on #1481: own the body, catch worker exceptions, 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 #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> * Address Copilot round 2 on #1481: move body, deterministic test 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> * Drain pending tasks in the worker on shutdown to avoid a self-Join leak 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> * Address Copilot round 3 on #1481: guard worker-thread start, 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> * Assert the /slow/ endpoint rewrite in the teardown smoke test 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> * Cast chrono counts to long long in %lld LOG_TRACE calls 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> * Address Copilot round 5 on #1481: <limits> include, broaden 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> * Address Copilot round 6 on #1481: guard shared_from_this() in 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> * Correct the body-move comment in SendRequestAsync (#1481 review 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> * Harden NoSelfJoin test timeout path (#1481 review round 8) 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> * Move worker-lambda construction inside the try in SendAsync (#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> * Drop issue-number references from code comments Reword comments in the curl HTTP client and its tests to describe the behavior without citing tracking numbers; no code changes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Drop issue-number reference from teardown smoke-test comment Reword the comment to describe the test without citing a tracking number; no code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Drop issue-number reference from metastats opt-in comments Reword the three `enabled` comments to describe the behavior without citing a tracking number; no code change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Guarantee the callback fires even when Send() throws Copilot review: the fallback comments state the callback is 'always invoked', but RunSendAndCallback skipped the callback if Send() itself threw (the callback call was inside the same try). If Send() threw, the request was left outstanding and its IHttpClient callback never completed, which could hang the upload/cancel path. Restructured so Send() is guarded on its own, a thrown Send() sets a failure result (res = CURLE_FAILED_INIT), and the callback is then invoked unconditionally (itself guarded so a throwing callback can't escape the detached worker). The 'always invoked' contract now holds literally. Validated on Linux (WSL, Debug): 13 HttpClientCurl* unit tests pass; FuncTests 39/39. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix data-loss and queue-wedge in SQLite batched flush Address two material issues in the offline-storage batched flush found in review: - COMMIT failures were reported as success. StoreRecords/StoreRecord decided success only from per-insert step results; the COMMIT ran in ~DbTransaction and its bool result was discarded. An all-inserts-OK batch whose COMMIT failed (e.g. SQLITE_FULL/IOERR) returned the full count, so Flush -- which drains records from memory before storing and only re-queues on a zero return -- treated the undurable batch as saved and dropped the records. DbTransaction now exposes commit(), which verifies COMMIT, rolls back on failure so the transaction is never left open, and returns false; StoreRecords/StoreRecord report the failure so Flush re-queues the batch. - A single permanently-invalid record wedged the whole batch. Any record failing validation made StoreRecords store nothing and return 0, and Flush re-queued the entire batch, so the poison record was re-drained and re-rejected on every flush, blocking every valid record behind it and growing the in-memory queue without bound. Invalid records are now dropped (reported once) and the valid remainder is stored all-or-nothing. Tests: rewrite the flush regression test to use a real transient failure (an unopenable database) instead of an invalid record; add a test that invalid records are dropped rather than wedging the queue; update the SQLite batch test to expect invalid-dropped / valid-stored. Also drop the issue-number reference from a TransmitProfiles test comment. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix out-of-bounds timer access in transmit-profile debug logging TransmitProfiles::dump() and onTimersUpdated() indexed rule.timers[0..2] unconditionally, but a custom profile rule may carry fewer than three timers -- the JSON parser tolerates rules with 0-2 timers (load() returns true for them). With logging enabled this read past the vector; under the Debug checked STL it aborts with "vector subscript out of range", and in a release build it is an out-of-bounds read. Read out-of-range timer slots as 0, and bound-check currRule against rules.size() before indexing. Exercised by the existing load_Json_ProfileWithInvalidTimers / ProfileWithEmptyTimerArray / RuleWithoutTimers tests, which now pass instead of crashing the Debug unit-test run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address Copilot review: correct Flush comment and size_t format specifier - OfflineStorageHandler::Flush()'s comment claimed the disk StoreRecords() is strictly all-or-nothing (full count or 0). That is no longer accurate: StoreRecords() now drops invalid records and returns the count it durably committed (which may be partial). Reword the comment so the re-queue invariant is described correctly and future maintainers don't rely on the wrong contract. - TransmitProfiles::dump() logged a size_t rule index with %d, which is undefined behavior for printf-style varargs on 64-bit builds. Use %zu. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix use-after-free dispatching OnDestroy after the completion callback The self-keepalive fix keeps the operation alive on the detached worker until Send() and the completion callback finish, so ~CurlHttpOperation can now run after the completion callback. In synchronous-handler builds (USE_SYNC_HTTPRESPONSE_HANDLER, which is defined by default) that callback runs HttpClientManager::onHttpResponse, which deletes the IHttpResponseCallback before returning. The destructor then dispatched OnDestroy through the now-dangling m_callback -- a use-after-free on every completed request (benign until the freed memory is reused; caught by ASAN). Track completion in an atomic flag set right after the completion callback runs, and skip the destructor's OnDestroy dispatch once completed. OnDestroy still fires when the operation is destroyed before completing (aborted, or a construction/dispatch failure), where m_callback is still valid. Add a regression test (SendAsync_NoOnDestroyDispatchAfterCompletion) that keeps the callback alive and asserts OnDestroy is not dispatched after completion; it fails without the guard and passes with it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: include <utility>, correct OnDestroy comment, harden test Follow-ups from code review of the completion-path UAF fix: - HttpClient_Curl.hpp uses std::move but relied on a transitive <utility>; include it directly. - The destructor comment claimed OnDestroy still fires on abort. It does not: every SendAsync path (including abort and the synchronous fallbacks) runs the completion callback and sets m_completed first, so OnDestroy is suppressed for any request that was actually sent. Correct the comment to say so. - Harden SendAsync_NoOnDestroyDispatchAfterCompletion: on the wait_for timeout path, abort the worker and wait so it can't outlive the stack frame whose cb/m_headers/m_body it reads; and let the destructor body finish before asserting so a missing guard is observed rather than raced past. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix use-after-free when the last worker reference is released on its own thread The process-wide PAL WorkerThread is shared by reference count. A task running on the worker thread can drop the last reference (e.g. by tearing down its LogManager/PAL), which ran ~WorkerThread -> Join() synchronously inside the task: Join() detached the thread and returned, freeing the object while threadFunc was still on the stack below the task. threadFunc then kept touching freed members (m_itemInProgress, the locks, and the queues it drains at shutdown) -- a use-after-free / heap corruption confirmed by AddressSanitizer. Give the worker a custom shared_ptr deleter: when the last reference is released on the worker thread itself, detach and defer destruction to the thread, which deletes itself only after its loop has broken and all member access is done. On any other thread the object is deleted immediately as before (~WorkerThread joins the worker first). Add a PalTests regression test that drops the last reference from within a task running on the worker thread; it is clean under AddressSanitizer with the fix and reports heap-use-after-free without it. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix two curl-client lifetime issues found in review - SendRequestAsync moved the request body out of the request, but the request is read again after the send: HttpResponseDecoder emits the request payload on EVT_HTTP_OK / EVT_HTTP_ERROR when requestDone runs the decode chain. Moving it out left those debug events with an empty payload (a curl-only regression vs the WinInet and NSURLSession clients). Copy the body into the operation instead -- it still gets an owned buffer for the detached send, and the request keeps its body for the decoder. - ~HttpClient_Curl ran curl_global_cleanup, but detached workers run curl_easy_cleanup in ~CurlHttpOperation after the request callback has already been removed from HttpClientManager's tracking, so the shutdown drain could return before an operation's easy-handle cleanup finished -- curl_global_cleanup then races easy-handle cleanup (undefined behavior). Track in-flight operations and have ~HttpClient_Curl wait (bounded to 5s) for them before global cleanup. All 14 curl unit tests pass. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden curl-client shutdown: complete-on-send and skip unsafe global cleanup Address review findings on the async self-join fix: - Set m_completed after Send() regardless of whether a completion callback was provided. It was only set inside the non-null-callback branch, so a SendAsync() call with the default null callback left m_completed false and ~CurlHttpOperation would still DispatchEvent(OnDestroy) for a request that had actually been sent -- the use-after-free the guard exists to prevent. - Skip curl_global_cleanup() when the bounded in-flight drain times out. curl_global_cleanup must not run concurrently with the curl_easy_cleanup that in-flight operation destructors run on detached workers; proceeding after a timeout could crash. Leaking libcurl global state once at shutdown is the safer choice in that pathological case. Files: lib/http/HttpClient_Curl.hpp, lib/http/HttpClient_Curl.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden the completion-guard regression test against the timeout path Heap-own the TrackingCallback and capture the shared_ptr by value in the completion lambda so its lifetime is tied to the detached worker. Previously the stack callback was captured by reference: in the timeout/FAIL path the worker can still be running when the test returns, so it could read the callback after destruction (a use-after-free that could crash the whole test process). The final assertion now dereferences the shared_ptr. Files: tests/unittests/HttpClientCurlTests.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Clarify ~CurlHttpOperation comment for never-sent and synchronous-fallback cases The destructor runs after the detached worker releases its reference only when Send() ran asynchronously; it also runs for operations that were never sent or when SendAsync fell back to a synchronous run. Destruction is safe on any thread in all cases. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Correct the OnDestroy comment: suppressed once a send is attempted RunSendAndCallback sets m_completed regardless of the send result (including an immediate curl_easy_init failure), so OnDestroy is dispatched only when the operation is destroyed without SendAsync ever having run -- not on construction failure. Reword the comment to match. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Wait for the operation to be destroyed in the self-join test's timeout path Mirror the stronger teardown from SendAsync_NoOnDestroyDispatchAfterCompletion: on the (unexpected) timeout path, wait for weakOp to expire after Abort so the detached worker cannot outlive fixture teardown (m_client/curl_global_cleanup, m_headers, m_body) and cause secondary crashes that obscure the real failure. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Wait for the operation to be destroyed on the self-join test's success path The callback sets the promise, but the detached worker still holds its self- reference until RunSendAndCallback returns. Wait (bounded) for weakOp to expire before the test returns so the operation's curl_easy_cleanup cannot race with fixture teardown, matching the other async regression test. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Guarantee async ops are destroyed before teardown in both self-join tests Extract a shared DrainOperation helper that waits for the operation to be destroyed and aborts a stuck worker as a fallback, then hard-asserts it is gone. Both async regression tests now ensure the detached worker (and its curl_easy_cleanup) cannot outlive fixture teardown (m_client -> curl_global_cleanup) on either the success or timeout path, rather than returning while the worker might still run. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Hard-stop if a detached curl worker refuses to drain before teardown These directly-constructed operations are not tracked by HttpClient_Curl::m_activeOps, so nothing else bounds the race between a lingering worker's curl_easy_cleanup and the fixture's curl_global_cleanup. DrainOperationOrDie now aborts a stuck worker and, if the operation is still alive afterward (a genuine keepalive/abort regression), records a failure and std::abort()s rather than returning into fixture teardown with an in-flight curl worker. In practice the .invalid host fails DNS in milliseconds so the operation is always gone immediately. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Make worker self-dispose detection survive a prior detach() onLastReferenceReleased() decided whether it was running on its own worker thread via m_hThread.get_id(), which returns the default not-a-thread id after detach(). If Join() had already run on the worker thread (its self-path detaches m_hThread), a later last-reference drop on that same thread would miss the self-check and delete the object while threadFunc was still executing below it -- the same UAF this change set fixes. Capture the worker's id in an atomic at threadFunc start and compare against that instead, so detection is correct regardless of detach ordering. Not reachable through current SDK code (the default WorkerThread is never explicitly Join()-ed), so this is defense-in-depth. Validated: ASAN dispatcher tests (PalTests incl WorkerThreadSelfDisposeOnOwnThreadIsSafe, TaskDispatcherCAPITests) all pass clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Address review: portable worker-id storage and fix thread-id logging UB Two issues raised on the previous commit: - std::atomic<std::thread::id> is not portable (std::thread::id is not guaranteed trivially copyable). Store m_workerId as a plain std::thread::id guarded by the existing recursive m_lock instead; the self-dispose check reads it under the lock. - Passing std::thread::id to LOG_INFO's printf-style '%u' is undefined behavior (varargs). Format the id with std::hash<std::thread::id> and '%zu' at both log sites (the constructor's 'Started new thread' and threadFunc's 'Running thread'). This was pre-existing; the surrounding change touches these lines. Validated: ASAN dispatcher tests (PalTests incl WorkerThreadSelfDisposeOnOwnThreadIsSafe, TaskDispatcherCAPITests) 15/15 pass clean. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Fix curl worker shutdown lifetime Move curl request tracking into shared state captured by detached workers so late callbacks do not dereference HttpClient_Curl after the shutdown drain times out. Preserve the bounded drain before curl_global_cleanup and abandon late callbacks/logging when shutdown cannot safely wait. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Avoid public queue-result dispatcher virtual Keep scheduled-task rejection detection internal by tracking the task lifetime across Queue(), so scheduleTask() returns a no-op handle if the dispatcher deletes the task during shutdown. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Harden batched flush retry handling Add an opt-out for batched storage flushes while keeping batching enabled by default. Report records that cannot be returned to memory after disk flush failure instead of dropping them silently. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> * Simplify curl worker lifetime handling Replace the detached self-keepalive and shutdown-tracker design with an owned worker thread. Normal destruction joins the worker; callback-thread destruction detaches it to avoid EDEADLK, while completion is published before callbacks can release the operation. Files: - lib/http/HttpClient_Curl.hpp: own, publish, join, and self-detach the worker safely - lib/http/HttpClient_Curl.cpp: restore the direct client lifetime model - tests/unittests/HttpClientCurlTests.cpp: cover self-destruction and late OnDestroy suppression Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Preserve curl completion semantics on worker failures Keep OnDestroy delivery exactly once while the response callback is still valid, and complete requests synchronously when callable copying or thread creation fails. Track send attempts independently of thread joinability so failed construction cannot make an operation reusable. Files: - lib/http/HttpClient_Curl.hpp: centralize terminal event/callback delivery and harden thread startup - tests/unittests/HttpClientCurlTests.cpp: verify self-destruction, terminal event delivery, construction failure, and single-use behavior Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Handle latency-off drops and share disk validation OfflineStorageHandler::StoreRecord at lib/offline/OfflineStorageHandler.cpp:263-276 must not treat MemoryStorage's intentional EventLatency_Off false return as a storage failure, or StorageObserver's false path will report a spurious store failure. Keep latency-off records as successful no-op drops while still propagating genuine memory-store failures.\n\nAlso centralize the disk-record validity predicate used by the per-record flush fallback and SQLite batch-store validation into lib/offline/StorageRecordValidation.hpp so those paths cannot drift apart on what counts as a valid disk record.\n\nFiles:\n- lib/offline/OfflineStorageHandler.cpp\n- lib/offline/OfflineStorageHandler.hpp\n- lib/offline/OfflineStorage_SQLite.cpp\n- lib/offline/StorageRecordValidation.hpp\n- tests/unittests/OfflineStorageTests.cpp\n\nCo-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Fix deferred task lifetime tracking and shutdown cleanup TaskDispatcher.hpp now keeps DeferredCallbackHandle tied to TaskLifetimeState instead of a raw Task*. That lets scheduleTask() handles observe when the task is dropped or finishes normally, so a later Cancel() becomes a safe no-op instead of reusing a stale pointer. WorkerThread.cpp now centralizes shutdown sentinel enqueueing and pending-task drain/delete logic in shared helpers so Join() and the self-detach shutdown path cannot drift. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Guard OfflineStorageHandler::Flush against leaking StartActivity on exception Flush() paired ILogManager::StartActivity()/EndActivity() manually -- StartActivity() at the top, EndActivity() on the last line -- with no RAII guard and no try/catch in between. StoreRecords(), the optional checkpoint Flush(), and IOfflineStorageObserver::OnStorageRecordsSaved() are all real throw surfaces (disk I/O, a full/locked DB, or an observer implementation). If any of them threw, EndActivity() was skipped and m_pause_active_count was permanently leaked, so every later FlushAndTeardown()'s PauseActivity()+WaitPause() would deadlock waiting for a count that could never reach zero. This reproduced as a live macOS deadlock on main. Add ActivityGuard, an RAII wrapper matching the existing safe pattern already used by PauseGuard (TransmissionPolicyManager.cpp) and ActiveLoggerCall (Logger.cpp): its destructor calls EndActivity() on every exit path, including exception unwinding. Flush() now constructs the guard instead of calling StartActivity() directly, checks IsActive() instead of the raw bool, and no longer calls EndActivity() explicitly -- the guard's destructor handles it uniformly for both the normal-completion and the StartActivity()-returned-false early-return paths. Validated: full WSL Release build + complete UnitTests suite, 536/536 passed. No exception-injection regression test was added (Flush() has no existing throwing-observer test harness to extend); the fix's correctness rests on C++'s standard guaranteed-destructor-during-unwinding semantics, the same guarantee the two existing PauseGuard/ActiveLoggerCall call sites already rely on. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Default Win32 desktop transport to WinHTTP instead of WinInet WinInet is designed for interactive desktop apps: it depends on a logged-on user and that user's Internet Explorer settings, and Microsoft documents it as unsupported for services and other non-interactive processes. WinHTTP is Microsoft's own recommended replacement for exactly that scenario, and 1DS's dominant embedding scenario (background/service telemetry) is the one WinInet is not designed for. Add lib/http/HttpClient_WinHttp.hpp/.cpp implementing the same IHttpClient/IHttpRequest contract as HttpClient_WinInet using WinHTTP's async API instead. Key differences from a direct port of the WinInet implementation: - WinHttpOpen uses WINHTTP_ACCESS_TYPE_AUTOMATIC_PROXY (falling back to WINHTTP_ACCESS_TYPE_NO_PROXY on an older OS that rejects it) instead of WinInet's INTERNET_OPEN_TYPE_PRECONFIG, so proxy resolution does not require a logged-on user. - WinHTTP's async model has one distinct callback status per stage (SENDREQUEST_COMPLETE -> HEADERS_AVAILABLE -> DATA_AVAILABLE/READ_COMPLETE loop -> REQUEST_ERROR) rather than WinInet's single INTERNET_STATUS_REQUEST_COMPLETE, and a FALSE return from an async-handle call is always a genuine synchronous failure (never ERROR_IO_PENDING as with WinInet). - The response-size cap (MAX_HTTP_RESPONSE_SIZE, see #1508) is enforced the same way, before every read. - The MS-root certificate check rebuilds the chain via CertGetCertificateChain, since WinHttpQueryOption only hands back the leaf certificate rather than WinInet's ready-made chain context. - Request lifetime uses std::enable_shared_from_this / shared_ptr rather than raw-pointer self-ownership: WinHttpCloseHandle on a request with a pending operation blocks the calling thread until that operation's completion callback (which runs on a different WinHTTP-internal thread) finishes running. Holding the shared requests-map mutex across that call -- WinInet's pattern, safe there because its callback runs synchronously on the calling thread -- deadlocks here, since the callback thread needs that same mutex to erase() the completed request. shared_ptr lets cancellation release the map lock before the blocking close, while still safely keeping the wrapper alive against a concurrent natural completion. - CancelAllRequests() waits on a condition variable signaled from erase() instead of polling in a sleep loop. HttpClientFactory now selects WinHTTP by default on Win32 desktop (non-WinRT) builds. Set MATSDK_USE_WININET=ON (CMake) or define HAVE_MAT_WININET_HTTP_CLIENT (legacy MSBuild) to opt back into WinInet, e.g. for IE-integrated proxy/cookie behavior. Both cpp files are always compiled; the choice is made at the factory's #include/#ifdef site, matching the existing pattern for WinRt vs. WinInet. Wired into both build systems: lib/CMakeLists.txt (new source files, winhttp link library, MATSDK_USE_WININET option) and lib/pal/desktop/desktop.vcxitems (new source files; linking uses #pragma comment(lib, "winhttp.lib") in the new .cpp so no individual .vcxproj's AdditionalDependencies needs updating). Validation (Windows x64 Debug, both CMake and the Solutions\MSTelemetrySDK.sln MSBuild path actually used by CI): - UnitTests: 496/496 passed. - FuncTests: 43/43 passed, excluding sendManyRequestsAndCancel, which hits the real production collector over the internet. That specific test hangs identically with the original, unmodified WinInet client under the same back-to-back test sequence, confirming it is pre-existing network/infrastructure flakiness unrelated to this change, not a regression. - Found and fixed two real bugs during validation: (1) WinHttpSetStatusCallback's return value was checked as a boolean, when it actually returns the previous callback function pointer (typically null on first registration) -- this rejected every request immediately after registering the callback; (2) the deadlock described above, reproduced live via a hung sendManyRequestsAndCancel run and confirmed fixed by comparing CPU-active vs. CPU-static process state before and after the shared_ptr change. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Copilot-Session: b12c5862-01e3-45e4-bf91-6389c20cae41 * Leak LogManagerFactory and PAL singletons to avoid static-destruction-order hazard LogManagerFactory::instance() and PAL::GetPAL() used ordinary function-local statics. Their destruction order relative to LogManagerProvider::Release() and PAL::shutdown() (both called during process teardown) is unspecified, since PAL in particular is constructed lazily on first use rather than at a fixed point relative to these teardown calls. A downstream consumer (onnxruntime-genai, see https://github.com/microsoft/onnxruntime-genai/pull/2363) hit this in production as intermittent EXC_BAD_ACCESS crashes on macOS-arm64 at process exit: LogManagerFactory's registries and PAL's ISystemInformation shared_ptr member were sometimes already destroyed by the time teardown code tried to use them, and worked around it in their vendored copy of this SDK by leaking both singletons. Apply the same fix upstream: static T& x = *new T(); deliberately never destroys the object, so its members stay valid for the rest of the process regardless of teardown timing. Both objects are small and fixed-size (one per process), and PAL::shutdown() / Release() already perform the real resource teardown explicitly, so this only avoids the destructor-ordering hazard, not a resource leak in the ordinary sense. Validated: WSL build, 536/536 UnitTests pass. * fix winhttp teardown hang on cancellation WinHTTP cancellation paths could leave the request wrapper in the parent map if HANDLE_CLOSING arrived without a prior terminal callback. That made CancelAllRequests wait forever and matched the Windows CI timeout in sendManyRequestsAndCancel. Handle HANDLE_CLOSING as a terminal signal when the request has not yet completed, so the wrapper erases itself and teardown always drains. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 7fe5faca-d77c-45c4-85d3-0d4a00d68a94 * Fix WinHTTP cancellation completion race Complete cancellation after WinHttpCloseHandle returns so HANDLE_CLOSING cannot dereference a destroyed request wrapper. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix shutdown and flush review findings Serialize WorkerThread joins and protect thread ownership during shutdown. Clear pending flush state on exceptions while holding the flush lock. Remove noexcept from mutex-taking upload state query. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Guard flush exception completion Keep the pending-flush state update synchronized after the flush lock is unwound by an exception. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Join stress-test upload workers before teardown Prevent detached UploadNow threads from outliving the functional test and racing later LogManager lifetimes. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Prevent WinHTTP request wrapper use-after-free Remove completed requests before invoking application callbacks so concurrent teardown cannot destroy the wrapper while its terminal callback is still running. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Align vcpkg iOS deployment target Ensure vcpkg-built Apple libraries match the consumer deployment target and avoid linker warnings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4259-b03b-8eeb87c06837 * Harden Apple packaging integration Propagate the resolved iOS sysroot to embedding builds and keep Apple vendored targets compatible with strict warning settings. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837 * Migrate Apple builds to canonical CMake variables Remove legacy Apple architecture, platform, and deployment-target inputs so standalone scripts and embedding consumers share CMAKE_OSX_* configuration. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 5f341bc5-f8ae-4257-b03b-8eeb87c06837 * Harden teardown and preserve failed flush records Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Remove unused Windows transport dependencies Keep both selectable HTTP backends linked privately while dropping the unused Winsock dependency and headers. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Harden flush and worker teardown recovery Ensure flush completion is signaled when record recovery throws, prevent activity cleanup exceptions from terminating teardown, and make worker task state race-free. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Make activity cleanup non-throwing Prevent PauseGuard and other teardown destructors from terminating the process when activity cleanup encounters a mutex or system error. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Add direct test standard library includes Ensure the offline storage unit tests do not rely on transitive includes.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Rollback batched storage when an insert throws Prevent the transaction destructor from committing a partial batch after an exception, so Flush can safely recover the entire drained batch without duplicate persisted records.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Fix WinHTTP duplicate completion during teardown Join upload workers before SDK teardown and cover in-flight cancellation with a deterministic HTTP test. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Keep WinHTTP callback context alive through close Route callbacks through a weak request reference so late WinHTTP notifications cannot dereference a destroyed wrapper during teardown. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Make cancellation stress test deterministic Avoid external collector network delays so teardown behavior is reproducible in CI. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Avoid fixture socket overflow in cancellation stress test Use a closed localhost port instead of creating hundreds of concurrent fixture connections. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Prepare WinHTTP for bounded cancellation Expose the transport capability required by the upcoming cancellation-drain changes and correct the certificate-check documentation typo. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Align bounded cancellation integration Keep the shared interface and Visual Studio project ready for the upcoming PR 1494 merge. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Prepare request draining for PR 1494 Port bounded pause cancellation and condition-variable callback draining so WinHTTP can use the upcoming manager contract without a merge conflict. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Handle nil Apple responses during cancellation NSURLSession cancellation callbacks may provide no HTTP response. Avoid dereferencing the null response while preserving the aborted result so teardown can complete safely.\n\nCo-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>\nCopilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Fix teardown deadlock when flush is skipped during pause OfflineStorageHandler::Flush() returned early when StartActivity() failed, which happens as soon as FlushAndTeardown() begins pausing the LogManager. That early return left m_flushPending == true and never posted m_flushComplete, so WaitForFlush() blocked forever and Shutdown() never completed. The race needs a flush to be pending when teardown starts, so it only reproduced when an earlier test had already pushed enough records to schedule an async flush -- which is why sendManyRequestsAndCancel hung in the full suite but passed in isolation. It was misread as WinHTTP cancellation not draining; the transport had already finished. Always release the waiters: cancel the pending handle, post the event, and clear the pending flag on the skipped path. Flush body moves to FlushImpl() so EndActivity() is paired with StartActivity() on exactly the path that acquired it. Verified on Windows: the doNothing/killIsTemporary/ sendManyRequestsAndCancel sequence that hung indefinitely now passes, 5/5 repeat runs are stable, functests are 43/43 and unittests 528/528. Files changed: lib/offline/OfflineStorageHandler.cpp lib/offline/OfflineStorageHandler.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Keep Apple requests alive through cancellation callbacks Preserve request lifetime until the asynchronous NSURLSession completion callback has finished, preventing teardown use-after-free and callback drain deadlocks. Also pass task pointers safely to variadic logging. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Fix process-terminating fastfail in oneds_memcpy_s on MSVC oneds_memcpy_s delegated straight to the CRT memcpy_s whenever _MSC_VER or __STDC_LIB_EXT1__ was defined, skipping its own constraint checks. On MSVC the CRT reports a constraint violation through the invalid parameter handler, whose default behaviour terminates the process via __fastfail (STATUS_STACK_BUFFER_OVERRUN / 0xC0000409) rather than returning EINVAL. This crashed AnnexKTests.memcpy_s in Debug builds, which Windows CI does run (test-win-latest.yml builds both Release and Debug). More importantly it was a latent abrupt-termination path in shipped Windows code: any caller passing count > destsz would kill the process instead of getting an error back. The delegate path also never zeroed the destination on error, contradicting the function's documented contract. Validate the arguments before copying on every platform so the documented "return EINVAL and zero the destination" behaviour holds uniformly. Also fix oneds_buffer_region_overlap, which used strict > against a one-past-the-last-byte address and so both missed genuine single-byte overlaps and mis-flagged merely adjacent buffers. Replaced with the standard half-open range test, with an explicit zero-length short circuit. Unit tests: 531/531 pass with no exclusions (previously the suite could not run AnnexKTests at all). Files changed: lib/utils/annex_k.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Fix SQLite batch accounting and benchmark Restore the size estimate when a batched insert transaction rolls back, and make the release performance test measure the batched StoreRecords path instead of timing 1,000 individual transactions. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Select curl HTTP version at runtime instead of forcing HTTP/2 CurlHttpOperation unconditionally set CURLOPT_HTTP_VERSION to CURL_HTTP_VERSION_2_0 with a comment claiming it would "fallback to HTTP/1.1 if not supported". libcurl does not do that: when the linked library was built without HTTP/2, requesting it fails the transfer with CURLE_UNSUPPORTED_PROTOCOL rather than negotiating down. On such a build every upload would fail. Add CurlHttpOperation::GetPreferredHttpVersion(), which probes curl_version_info for CURL_VERSION_HTTP2 and returns CURL_HTTP_VERSION_1_1 when HTTP/2 is unavailable, and use it at setopt time. This also fixes the Linux build. HttpClientCurlTests.cpp came in with the #1481 merge and calls GetPreferredHttpVersion(), which had no implementation, so UnitTests failed to compile and build-tests.sh then exited 127 on the missing binary in all three ubuntu legs. Files changed: lib/http/HttpClient_Curl.hpp Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Prevent SIGPIPE from killing the test process on peer reset BasicFuncTests.teardownDuringInFlightUpload_ShutsDownCleanly intermittently killed the whole test runner on macOS/iOS CI: the process exited with signal SIGPIPE (exit 141) and no crash backtrace, which XCTest reports as an unexpected exit/restart and a ~24s timeout rather than a test failure. Cause: the test HTTP server writes responses from the reactor thread via ::send() with no SIGPIPE protection. That test deliberately cancels an upload that is still in flight against the /slow/ endpoint, so NSURLSession resets the connection while the server is mid-response. ::send() then fails with EPIPE and raises SIGPIPE; the test process installs no handler, so the default disposition terminates it. The race is timing-dependent, which is why it looks flaky and only shows up on the slower Apple CI runners. Fix (test infrastructure only, no SDK behavior change): - Add Socket::setNoSigPipe() and apply SO_NOSIGPIPE to every accepted connection (Apple/BSD, where the option is per-socket). - Pass MSG_NOSIGNAL from Socket::send() on Linux, which has no SO_NOSIGPIPE. Both make a write to a reset peer return EPIPE, which the reactor already handles by closing the connection. Files changed: tests/common/SocketTools.hpp Validated locally on macOS (arm64, Debug): the test reproduced at ~30% (5/15 runs exited 141) before the fix and passed 30/30 after; the full FuncTests suite passes 40/40. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Always release SQLite storage during shutdown A failed database recreate clears m_isOpened while retaining the SqliteDB wrapper. The fixture then removes the database path while the wrapper still owns SQLite state, which triggers Apple's vnode-unlinked warning and poisons the next test. Always shut down and reset the wrapper regardless of the open flag so failed recreates cannot leak storage state across tests. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> * Fix WinHTTP shutdown deadlock when a send fails synchronously BasicFuncTests.sendManyRequestsAndCancel hung indefinitely on Win32 Release CI (54+ minutes against a ~10 minute baseline for the leg). WinHttpRequestWrapper::send() held m_requestsMutex across its entire body, including the synchronous-failure paths that call onRequestComplete(). onRequestComplete() invokes the application callback, which -- as the comment above that call already noted -- can synchronously tear down the client. That teardown reaches HttpClient_WinHttp::CancelAllRequests(), which waits on m_requestsCv. m_requestsMutex is a std::recursive_mutex and m_requestsCv is a std::condition_variable_any. condition_variable_any::wait() releases only ONE level of a recursive mutex, so waiting while the mutex was held twice left it locked. erase(), running on the WinHTTP callback thread, could then never acquire the mutex to remove the request and notify_all(), so the predicate never became true and the untimed wait never woke: a permanent lost-wakeup deadlock. The test provokes this by posting to closed port 127.0.0.1:1, which makes WinHttpSendRequest fail synchronously, and by setting CFG_INT_MAX_TEARDOWN_TIME = i % 2 so alternating iterations take the untimed full-shutdown wait. Split the handle-setup work into sendLocked(), which runs under the lock and only *reports* a synchronous failure, and send(), which completes the request via onRequestComplete() after the lock has been released. Cancellation is still serialized against setup, so a cancel cannot be lost mid-handle-creation. cancel() already called onRequestComplete() outside the lock and is unaffected. Verified on a CI-faithful Win32 Release MSBuild build (the MSBuild project compiles AISendTests/BondDecoderTests/EventDecoderListener, which the CMake build omits -- 44 tests from 5 suites vs 43 from 4 -- which is why earlier CMake-only runs did not reproduce it): - sendManyRequestsAndCancel: hung 54+ min -> passes in 16.9s - FuncTests 44/44 passed, no exclusions - UnitTests 501/501 passed, no exclusions Files changed: lib/http/HttpClient_WinHttp.cpp Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: aefd8d4a-8755-4853-8e6b-267cce60e3a9 * Use stable worker identity for self-cancellation Keep self-thread detection correct after the worker std::thread object is detached, avoiding a potential recursive wait on the execution mutex. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 2bfc77f4-1a25-439f-8552-16750895413b * Bound offline storage flush batches Keep SQLite transactions bounded and requeue only a failed batch so earlier commits remain durable. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilo…
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
OfflineStorage_SQLite::StoreRecords()loopedStoreRecord(), and eachStoreRecord()opens its ownDbTransaction(BEGIN EXCLUSIVE…COMMIT). So a flush of N records did N transactions = N WAL fsyncs.This batches the whole vector into a single transaction, sharing the per-record logic via extracted private helpers (
isValidRecord,insertRecordUnsafe,checkStorageSizeLimits). Implements the existing in-code TODO (// consider running the batch in transaction).Measured against the SDK's own vendored sqlite (WAL,
synchronous=NORMAL,BEGIN EXCLUSIVE):Behavior notes
The batching keeps validation/reporting semantics equivalent to the old per-record path, with these intentional consequences:
BEGIN EXCLUSIVEis held.insertRecordUnsafe()now checks theexecute()result: a failed insert is not counted instoredand does not inflatem_DbSizeEstimate, and the write failure is reported viaOnStorageFailed("Database write failed")after the transaction closes. (Previously the per-record insert result was ignored — this is a correctness improvement that overlaps the separate data-safety PR Offline storage: data-safety fixes + batched flush (empty-filter guard, store-failure propagation, event-loss fix, one-transaction flush) #1491; the two will need a coordinated merge.)ResizeDb()check runs once after the batch (and still runs even if inserts failed, so a full DB can recover).Tests
OfflineStorageTests_SQLite.StoreRecordsBatchStoresAllRecords(asserts each record is individually retrievable).UnitTests: 524/524 pass.