Offline storage: data-safety fixes + batched flush (empty-filter guard, store-failure propagation, event-loss fix, one-transaction flush)#1491
Open
bmehta001 wants to merge 22 commits into
Conversation
…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>
lalitb
reviewed
Jun 22, 2026
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>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR hardens the offline-storage layer against two data-loss scenarios by (1) preventing accidental full-queue deletes when a delete predicate is empty and (2) ensuring SQLite write failures are surfaced to callers/observers instead of being treated as successful stores.
Changes:
- Add an empty-filter guard to
MemoryStorage::DeleteRecords(whereFilter)and a unit test to prevent unintended full deletes. - Propagate synchronous disk
StoreRecord()failures throughOfflineStorageHandler::StoreRecordso callers don’t count failed persistence as success. - Treat
SqliteStatement::execute()failures inOfflineStorage_SQLite::StoreRecordas hard failures: log, notifyOnStorageFailed, and returnfalsewithout updating size estimates.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| tests/unittests/MemoryStorageTests.cpp | Adds coverage to ensure DeleteRecords({}) does not wipe the in-memory queue. |
| lib/offline/MemoryStorage.cpp | Guards empty whereFilter to avoid “match-all” deletion behavior. |
| lib/offline/OfflineStorageHandler.cpp | Returns disk StoreRecord() result to propagate synchronous persistence failures to callers. |
| lib/offline/OfflineStorage_SQLite.cpp | Checks execute() result and reports SQLite insert failures via logging + OnStorageFailed. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
bmehta001
added a commit
to bmehta001/cpp_client_telemetry
that referenced
this pull request
Jun 23, 2026
OfflineStorage_SQLite::StoreRecord: return false when the INSERT execute() fails (was ignoring the result and returning true even on a real write failure). The Flush() reserve/confirm-delete logic in this PR relies on the disk backend reporting per-record failure; without this, a failed sqlite3_step would still be treated as persisted and dropped from memory. (This is the same one-line fix as PR microsoft#1491, included here so this PR is correct on its own; trivial overlap that resolves cleanly at merge.) OfflineStorageTests.cpp NoopTaskDispatcher: own queued tasks and delete them on Cancel()/Join()/destruction instead of dropping the Task* (PAL::scheduleTask allocates with new and expects the dispatcher to take ownership), so the helper cannot leak. 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>
bmehta001
changed the title
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>
bmehta001
changed the title
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>
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>
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>
…fier - 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>
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>
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
Offline-storage data-safety + flush performance (combines the data-safety fixes with the SQLite batch-flush optimization, formerly PR #1496 and #1497).
MemoryStorage::DeleteRecords(filter)DeleteAllRecords()), mirroring the SQLite backend.OfflineStorage_SQLite::StoreRecordsBEGIN EXCLUSIVE/COMMIT(one fsync) instead of one transaction per record. ~11× (200 recs) / ~40× (1000 recs) faster (measured on the SDK's vendored sqlite). All-or-nothing: any insert failure rolls the batch back (newSqliteDB::rollback), so it can be re-queued without duplicate rows (no uniquerecord_id).OfflineStorage_SQLite::StoreRecordfalseand reportsOnStorageFailedon a real write failure (was ignoring theexecute()result). Validation runs before the transaction; write failures are reported after it closes.OfflineStorageHandler::StoreRecordtrue).OfflineStorageHandler::FlushStoreRecords(), and re-queues the whole batch if it doesn't fully persist — realizing the batch speedup and keeping the no-loss / no-duplicate guarantee. Also null-guards the size read (disk-only storage).Tests
MemoryStorageTests.DeleteRecordsWithEmptyFilterDoesNotDeleteAllOfflineStorageHandlerFlushTests.FailedDiskStoreDuringFlushReturnsRecordsToMemory— records the disk store rejects remain in memory afterFlush(). Verified it fails against the previousGetRecords()-based Flush.OfflineStorageTests_SQLite.StoreRecordsBatchStoresAllRecordsUnitTests: 527/527 pass.History
The batching half (PR #1497) was driven to a clean Copilot review over ~9 rounds, then folded here so the batched
StoreRecords()and the Flush data-loss fix cooperate (all-or-nothing batch + re-queue) rather than conflict.Additional correctness fixes (bundled)
Three small, unrelated correctness fixes (happy to split out if reviewers prefer):
nativeInitializePrivacyGuard[WithoutCommonDataContext]storedJStringToStdString(...).c_str()intoInitializationConfiguration'sconst char*fields; the temporarystd::stringdied at end-of-statement, soconfigpointed at freed memory beforePrivacyGuardwas constructed. Now the converted strings are held in locals that outlivemake_shared<PrivacyGuard>(config).GetAppLocalTempDirectoryleaked aRoInitializereference on the UWP path (no matchingRoUninitialize). Balanced withRoUninitialize()when the call succeeded, releasing the WinRTStorageFolderfirst so it isn't destroyed in an uninitialized apartment.TransmitProfilesJSONpowerStatemap was missinglow_battery, so profiles using it silently fell back to default. Mappedlow_battery->PowerSource_LowBattery.Validation: full WSL
UnitTests530/530 pass (covers #312 + no regression). The JNI (#1334) and UWP (#1333) paths are platform-specific and not in the desktop build; both are minimal lifetime fixes.Concurrency / lifecycle fixes (bundled)
OfflineStorageHandler::GetAndReserveRecordswrotem_lastReadCount/m_readFromMemoryunsynchronized whileLastReadRecordCount()/IsLastReadFromMemory()read them from the upload path. Both members are nowstd::atomic(all uses are by-value loads/stores/fetch-add).SqliteDBhad no destructor, so aSqliteDBdestroyed without an explicitshutdown()leaked its handle + prepared statements. Added~SqliteDB()calling the idempotentshutdown()(finalize + close + release instance count).Validation: full WSL
UnitTests530/530 pass, including the SQLite instance-count suite (confirms~SqliteDBdoesn't double-release). #1221 is a memory-model fix (TSan-only repro); #1134's leak is sanitizer-detected (how it was reported), so both are validated by no-regression + correctness rather than a new deterministic test.