Summary
OfflineStorageHandler::Flush() drains the entire in-memory queue and persists it to the disk backend in one batch:
auto records = m_offlineStorageMemory->GetRecords(false, EventLatency_Unspecified);
size_t totalSaved = m_offlineStorageDisk->StoreRecords(records);
This batch includes records whose persistence == EventPersistence::EventPersistence_DoNotStoreOnDisk. As a result, when the RAM tier is active (the normal configuration), a DoNotStoreOnDisk record that entered the in-memory queue can be written to disk on a memory-pressure flush and survive a process restart.
By contrast, OfflineStorageHandler::StoreRecord() explicitly skips such records — but only in the disk-only fallback branch (no RAM tier / shutdown):
else // m_offlineStorageMemory == nullptr || m_shutdownStarted
{
if (m_offlineStorageDisk != nullptr)
{
if (record.persistence != EventPersistence::EventPersistence_DoNotStoreOnDisk)
{
return m_offlineStorageDisk->StoreRecord(record);
}
}
}
When the RAM tier is present, StoreRecord() inserts every record into memory unconditionally (no persistence check), so DoNotStoreOnDisk records ride the normal RAM→disk flush path.
Pre-existing
This behavior is not introduced by the batched-flush change in #1491 — the pre-#1491 Flush() on main already called m_offlineStorageDisk->StoreRecords(records) on the full drained batch with no DoNotStoreOnDisk filter. This issue tracks the semantics question separately so #1491 stays focused.
The open question
The DoNotStoreOnDisk filter existing only in the disk-only fallback branch suggests the current flush-to-disk behavior may be intentional (i.e. the flag means "skip only when forced straight to disk without a RAM tier"). But the enum name implies these records should never be written to disk under any configuration. The two readings disagree.
Options for maintainers
- Treat current behavior as intentional and document/rename the enum to make the "disk-only fallback path" semantics explicit, so the name no longer implies a global no-disk contract.
- Enforce a global no-disk contract: during flush, partition
DoNotStoreOnDisk records out of the disk batch and re-insert them into the RAM queue (rather than draining + persisting them). This keeps them in memory until upload or drop, at the cost of possible RAM accumulation if they are never uploaded, and would need a regression test.
Surfaced during the #1491 review (Copilot). Filing as a follow-up per maintainer direction.
Summary
OfflineStorageHandler::Flush()drains the entire in-memory queue and persists it to the disk backend in one batch:This batch includes records whose
persistence == EventPersistence::EventPersistence_DoNotStoreOnDisk. As a result, when the RAM tier is active (the normal configuration), aDoNotStoreOnDiskrecord that entered the in-memory queue can be written to disk on a memory-pressure flush and survive a process restart.By contrast,
OfflineStorageHandler::StoreRecord()explicitly skips such records — but only in the disk-only fallback branch (no RAM tier / shutdown):When the RAM tier is present,
StoreRecord()inserts every record into memory unconditionally (no persistence check), soDoNotStoreOnDiskrecords ride the normal RAM→disk flush path.Pre-existing
This behavior is not introduced by the batched-flush change in #1491 — the pre-#1491
Flush()onmainalready calledm_offlineStorageDisk->StoreRecords(records)on the full drained batch with noDoNotStoreOnDiskfilter. This issue tracks the semantics question separately so #1491 stays focused.The open question
The
DoNotStoreOnDiskfilter existing only in the disk-only fallback branch suggests the current flush-to-disk behavior may be intentional (i.e. the flag means "skip only when forced straight to disk without a RAM tier"). But the enum name implies these records should never be written to disk under any configuration. The two readings disagree.Options for maintainers
DoNotStoreOnDiskrecords out of the disk batch and re-insert them into the RAM queue (rather than draining + persisting them). This keeps them in memory until upload or drop, at the cost of possible RAM accumulation if they are never uploaded, and would need a regression test.Surfaced during the #1491 review (Copilot). Filing as a follow-up per maintainer direction.