From d51447b9bfcb3c07eb95f83942bd7a8ec342f935 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Tue, 25 Aug 2026 15:10:59 -0500 Subject: [PATCH 1/7] fix: [SDK-5065] bound the crash-record cache on iOS FileLogStore had no retention. save() enforced no size limit, listReadable had only the lower minAgeMillis gate with no ceiling, deleteUnrecognizedEntries reaped only .otlp.tmp and never touched owned records at any age, and there was no count or byte cap anywhere. A record that fails to upload was therefore re-read and re-POSTed on every launch indefinitely, with the directory growing until the OS reclaimed the cache. Android hit the same defect when OpenTelemetry's disk-buffering was removed and rebuilt the policy; this adopts that policy rather than reimplementing it. The decisions come from CrashRetention in the shared module, so both platforms reclaim identically and the rules stay unit-tested in one place. This file keeps only the I/O: snapshot the directory, apply what the selectors return. Retention now runs on all three paths. save() refuses oversized payloads and trims after a write, keeping the record it just wrote. listReadable reclaims before materializing payloads, so an over-cap backlog is never fully loaded. deleteUnrecognizedEntries reclaims too, since it is the only scan that runs when remote logging is disabled and otherwise a directory nothing reads would never be bounded. Foreign-file handling is deliberately left as it was, narrower than Android's shared selector: iOS never ran the OpenTelemetry pipeline, so there is no legacy format sharing this directory, and files we did not write are not ours to assume about. Depends on the CrashRetention API landing in the KMP submodule; the pin bump comes with that merge. Co-authored-by: Cursor --- .../OneSignal.xcodeproj/project.pbxproj | 4 + .../Source/Logging/FileLogStore.swift | 167 +++++++++++-- .../FileLogStoreRetentionTests.swift | 219 ++++++++++++++++++ 3 files changed, 370 insertions(+), 20 deletions(-) create mode 100644 iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift diff --git a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj index a76259a58..6077cdd24 100644 --- a/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj +++ b/iOS_SDK/OneSignalSDK/OneSignal.xcodeproj/project.pbxproj @@ -83,6 +83,7 @@ A5048F01A1B2C3D4E5F6000A /* OSFeatureFlagsTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5048F01A1B2C3D4E5F60009 /* OSFeatureFlagsTests.swift */; }; A5048F01A1B2C3D4E5F6100B /* OSFeatureFlagsRefreshServiceTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A5048F01A1B2C3D4E5F6100A /* OSFeatureFlagsRefreshServiceTests.swift */; }; C781A33FED62B4B54221A09A /* OSLogCrashHandlerTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 3B6A59620B83538CEFF77269 /* OSLogCrashHandlerTests.swift */; }; + 32D3A6EA8AD44274B5CE378A /* FileLogStoreRetentionTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = 9CC252C94ECB485E8D0380E9 /* FileLogStoreRetentionTests.swift */; }; B96A3B6BA8CC49EE4796D9BF /* OSRemoteLoggingController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A72F938F8A3808AC1FF7F3C /* OSRemoteLoggingController.swift */; }; 25898119922BDCDA7AF0B9CC /* OSRemoteLoggingController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A72F938F8A3808AC1FF7F3C /* OSRemoteLoggingController.swift */; }; 9EAF92032D0429FA35E04417 /* OSRemoteLoggingController.swift in Sources */ = {isa = PBXBuildFile; fileRef = 8A72F938F8A3808AC1FF7F3C /* OSRemoteLoggingController.swift */; }; @@ -1816,6 +1817,7 @@ A5048F01A1B2C3D4E5F60009 /* OSFeatureFlagsTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OSFeatureFlagsTests.swift; sourceTree = ""; }; A5048F01A1B2C3D4E5F6100A /* OSFeatureFlagsRefreshServiceTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OSFeatureFlagsRefreshServiceTests.swift; sourceTree = ""; }; 3B6A59620B83538CEFF77269 /* OSLogCrashHandlerTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OSLogCrashHandlerTests.swift; sourceTree = ""; }; + 9CC252C94ECB485E8D0380E9 /* FileLogStoreRetentionTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = FileLogStoreRetentionTests.swift; sourceTree = ""; }; 8A72F938F8A3808AC1FF7F3C /* OSRemoteLoggingController.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OSRemoteLoggingController.swift; sourceTree = ""; }; 7C91A2B0D84F1E9A3C5B6D8E /* OSRemoteLoggingConfiguration.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = OSRemoteLoggingConfiguration.swift; sourceTree = ""; }; C0462F96E1AADF655F3B3765 /* OSRemoteLoggingController.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; path = OSRemoteLoggingController.h; sourceTree = ""; }; @@ -2608,6 +2610,7 @@ A5048F01A1B2C3D4E5F60009 /* OSFeatureFlagsTests.swift */, A5048F01A1B2C3D4E5F6100A /* OSFeatureFlagsRefreshServiceTests.swift */, 3B6A59620B83538CEFF77269 /* OSLogCrashHandlerTests.swift */, + 9CC252C94ECB485E8D0380E9 /* FileLogStoreRetentionTests.swift */, 3C23A21A2FCE0A52001D32E3 /* OneSignalIdentifiersFallbackTests.swift */, 3C23A21E2FCE0AA1001D32E3 /* OSResilientStorageTests.swift */, 3C23A21C2FCE0A83001D32E3 /* OSModelStoreRefreshTests.swift */, @@ -4655,6 +4658,7 @@ A5048F01A1B2C3D4E5F6000A /* OSFeatureFlagsTests.swift in Sources */, A5048F01A1B2C3D4E5F6100B /* OSFeatureFlagsRefreshServiceTests.swift in Sources */, C781A33FED62B4B54221A09A /* OSLogCrashHandlerTests.swift in Sources */, + 32D3A6EA8AD44274B5CE378A /* FileLogStoreRetentionTests.swift in Sources */, 3C23A21B2FCE0A52001D32E3 /* OneSignalIdentifiersFallbackTests.swift in Sources */, ); runOnlyForDeploymentPostprocessing = 0; diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift index e70dbc73c..96c759dc0 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift @@ -35,6 +35,12 @@ import OneSignalCore /// Writes are synchronous and durable because fatal handlers may terminate the /// process immediately after `save` returns. Directory scans and cleanup run on /// a utility queue to keep disk I/O off the caller. +/// +/// This is a bounded cache, not a queue. Retention decisions — which records have aged out, +/// which exceed the accumulation caps — come from `CrashRetention` in the shared module, so +/// iOS and Android reclaim identically; this type contributes only the file I/O. Without +/// those bounds a record that never uploads successfully is re-read and re-sent on every +/// launch for the life of the install. final class FileLogStore: ILogFileStore { /// Complete records use `.otlp`; interrupted durable writes leave /// `.otlp.tmp` files that are safe to reap after the minimum-age gate. @@ -56,11 +62,23 @@ final class FileLogStore: ILogFileStore { guard bytes.size > 0 else { return false } + // Refuse rather than store-then-reclaim. A record this large would either claim the + // whole shared budget or be deleted before it could be uploaded, so losing it loudly + // here beats losing it silently on a later launch. + guard Int64(bytes.size) <= CrashRetention.shared.maxRecordBytes else { + OneSignalLog.onesignalLog( + .LL_WARN, + message: "FileLogStore refusing record of \(bytes.size) bytes, over the " + + "\(CrashRetention.shared.maxRecordBytes)-byte limit" + ) + return false + } do { try createRootDirectory() let timestamp = Int64(Date().timeIntervalSince1970 * 1_000) let id = "\(timestamp)-\(UUID().uuidString)\(Self.ownedFileSuffix)" try writeDurably(bytes.data, to: rootURL.appendingPathComponent(id)) + enforceAccumulationCaps(keepName: id) return true } catch { return false @@ -114,12 +132,19 @@ final class FileLogStore: ILogFileStore { ioQueue.async { var deleted = 0 do { - for url in try self.fileURLs() - where url.lastPathComponent.hasSuffix(Self.temporaryFileSuffix) { - guard try self.isOldEnough(url, minAgeMillis: minAgeMillis) else { - continue - } - try self.fileManager.removeItem(at: url) + // Also the only scan that runs when remote logging is disabled, so it is the + // sole chance to bound a directory `listReadable` never touches. + _ = self.reclaim(entries: try self.directoryEntries()) + + // Deliberately narrower than the shared `selectUnrecognized`, which reaps any + // non-owned file. iOS never ran the OpenTelemetry pipeline, so there is no + // legacy format to clean up here — only this store's own interrupted writes. + // Anything else in the directory belongs to someone we should not assume about. + let now = Self.nowMillis() + for entry in try self.directoryEntries() + where entry.name.hasSuffix(Self.temporaryFileSuffix) + && now - entry.lastModifiedMs >= max(0, minAgeMillis) { + try self.fileManager.removeItem(at: self.rootURL.appendingPathComponent(entry.name)) deleted += 1 } } catch { @@ -133,15 +158,125 @@ final class FileLogStore: ILogFileStore { } private func readableEntries(minAgeMillis: Int64) throws -> [StoredLogFile] { - try fileURLs() - .filter { $0.lastPathComponent.hasSuffix(Self.ownedFileSuffix) } - .filter { try isOldEnough($0, minAgeMillis: minAgeMillis) } - .compactMap { url in + let entries = try directoryEntries() + // Reclaim before reading so payloads are only materialized for records that survive + // both bounds — an over-cap backlog is never fully loaded into memory. + let reclaimed = reclaim(entries: entries) + let now = Self.nowMillis() + + return entries + .filter { CrashRetention.shared.isOwned(name: $0.name, ownedSuffix: Self.ownedFileSuffix) } + .filter { !reclaimed.contains($0.name) } + .filter { now - $0.lastModifiedMs >= max(0, minAgeMillis) } + .compactMap { entry in + let url = rootURL.appendingPathComponent(entry.name) guard let data = try? Data(contentsOf: url) else { return nil } - return StoredLogFile(id: url.lastPathComponent, bytes: data.kotlinByteArray) + return StoredLogFile(id: entry.name, bytes: data.kotlinByteArray) + } + } + + /// Applies the shared retention policy and deletes what it selects. + /// + /// - Returns: names that must be withheld from readers, including any whose unlink failed — + /// a record past the ceiling must not be uploaded even if it could not be removed. + private func reclaim(entries: [CrashDirEntry]) -> Set { + let now = Self.nowMillis() + var withheld = Set() + + let expired = CrashRetention.shared.selectExpiredOwned( + entries: entries, + nowMs: now, + maxAgeMillis: CrashRetention.shared.maxReadAgeMillis, + ownedSuffix: Self.ownedFileSuffix + ) + for entry in expired { + withheld.insert(entry.name) + remove(name: entry.name) + } + + let survivors = entries.filter { !withheld.contains($0.name) } + let overflow = CrashRetention.shared.selectOverflowOwned( + entries: survivors, + maxCount: CrashRetention.shared.maxRecordCount, + maxTotalBytes: CrashRetention.shared.maxTotalBytes, + maxRecordBytes: CrashRetention.shared.maxRecordBytes, + keepName: nil, + ownedSuffix: Self.ownedFileSuffix + ) + for entry in overflow { + withheld.insert(entry.name) + remove(name: entry.name) + } + + if !withheld.isEmpty { + OneSignalLog.onesignalLog( + .LL_DEBUG, + message: "FileLogStore reclaimed \(expired.count) expired and " + + "\(overflow.count) over-cap record(s)" + ) + } + return withheld + } + + /// Trims the directory back inside the accumulation caps after a write, always keeping + /// [keepName]. Runs synchronously on the crashing thread, so it exits on one directory + /// listing in the steady state and only sorts when the caps are actually breached. + private func enforceAccumulationCaps(keepName: String) { + guard let entries = try? directoryEntries() else { + return + } + guard !CrashRetention.shared.isWithinCaps( + entries: entries, + maxCount: CrashRetention.shared.maxRecordCount, + maxTotalBytes: CrashRetention.shared.maxTotalBytes, + maxRecordBytes: CrashRetention.shared.maxRecordBytes, + ownedSuffix: Self.ownedFileSuffix + ) else { + return + } + let overflow = CrashRetention.shared.selectOverflowOwned( + entries: entries, + maxCount: CrashRetention.shared.maxRecordCount, + maxTotalBytes: CrashRetention.shared.maxTotalBytes, + maxRecordBytes: CrashRetention.shared.maxRecordBytes, + keepName: keepName, + ownedSuffix: Self.ownedFileSuffix + ) + for entry in overflow { + remove(name: entry.name) + } + } + + private func remove(name: String) { + do { + try fileManager.removeItem(at: rootURL.appendingPathComponent(name)) + } catch { + OneSignalLog.onesignalLog( + .LL_WARN, + message: "FileLogStore failed to reclaim \(name): \(error.localizedDescription)" + ) + } + } + + /// Snapshots the directory as the platform-neutral entries the shared policy consumes. + private func directoryEntries() throws -> [CrashDirEntry] { + try fileURLs().compactMap { url in + let values = try? url.resourceValues(forKeys: [.contentModificationDateKey, .fileSizeKey]) + guard let modifiedAt = values?.contentModificationDate else { + return nil } + return CrashDirEntry( + name: url.lastPathComponent, + lastModifiedMs: Int64(modifiedAt.timeIntervalSince1970 * 1_000), + lengthBytes: Int64(values?.fileSize ?? 0) + ) + } + } + + private static func nowMillis() -> Int64 { + Int64(Date().timeIntervalSince1970 * 1_000) } private func fileURLs() throws -> [URL] { @@ -150,21 +285,13 @@ final class FileLogStore: ILogFileStore { } return try fileManager.contentsOfDirectory( at: rootURL, - includingPropertiesForKeys: [.contentModificationDateKey, .isRegularFileKey], + includingPropertiesForKeys: [.contentModificationDateKey, .fileSizeKey, .isRegularFileKey], options: [.skipsHiddenFiles] ).filter { (try? $0.resourceValues(forKeys: [.isRegularFileKey]).isRegularFile) == true } } - private func isOldEnough(_ url: URL, minAgeMillis: Int64) throws -> Bool { - let values = try url.resourceValues(forKeys: [.contentModificationDateKey]) - guard let modifiedAt = values.contentModificationDate else { - return false - } - return Date().timeIntervalSince(modifiedAt) * 1_000 >= Double(max(0, minAgeMillis)) - } - private func isSafeEntryId(_ id: String) -> Bool { !id.isEmpty && URL(fileURLWithPath: id).lastPathComponent == id } diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift new file mode 100644 index 000000000..1fb12be09 --- /dev/null +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift @@ -0,0 +1,219 @@ +/* + Modified MIT License + + Copyright 2026 OneSignal + + Permission is hereby granted, free of charge, to any person obtaining a copy + of this software and associated documentation files (the "Software"), to deal + in the Software without restriction, including without limitation the rights + to use, copy, modify, merge, publish, distribute, sublicense, and/or sell + copies of the Software, and to permit persons to whom the Software is + furnished to do so, subject to the following conditions: + + 1. The above copyright notice and this permission notice shall be included in + all copies or substantial portions of the Software. + + 2. All copies of substantial portions of the Software may only be used in connection + with services provided by OneSignal. + + THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR + IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, + FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE + AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER + LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, + OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN + THE SOFTWARE. + */ + +import Foundation +import OneSignalKMP +@testable import OneSignalOSCore +import XCTest + +/// The crash directory is a bounded cache, not a queue. Before retention existed, a record that +/// never uploaded was re-read and re-sent on every launch and the directory grew until the OS +/// reclaimed the cache. These cover the bounds that prevent that; the policy decisions +/// themselves are unit-tested in the shared module. +final class FileLogStoreRetentionTests: XCTestCase { + private var temporaryDirectory: URL! + + override func setUpWithError() throws { + temporaryDirectory = FileManager.default.temporaryDirectory + .appendingPathComponent(UUID().uuidString, isDirectory: true) + try FileManager.default.createDirectory(at: temporaryDirectory, withIntermediateDirectories: true) + } + + override func tearDownWithError() throws { + try? FileManager.default.removeItem(at: temporaryDirectory) + } + + // MARK: - write-time size limit + + func testRefusesPayloadOverThePerRecordLimit() { + let store = makeStore() + let oversized = Data(count: Int(CrashRetention.shared.maxRecordBytes) + 1) + + XCTAssertFalse(store.save(bytes: oversized.kotlinByteArray)) + XCTAssertEqual(ownedFileNames().count, 0) + } + + func testAcceptsPayloadAtTheLimit() { + let store = makeStore() + let atLimit = Data(count: Int(CrashRetention.shared.maxRecordBytes)) + + XCTAssertTrue(store.save(bytes: atLimit.kotlinByteArray)) + XCTAssertEqual(ownedFileNames().count, 1) + } + + // MARK: - age ceiling + + func testListReadableDropsAndDeletesRecordsPastTheAgeCeiling() throws { + let ceiling = CrashRetention.shared.maxReadAgeMillis + try writeRecord(named: "expired.otlp", ageMillis: ceiling + 60_000) + try writeRecord(named: "fresh.otlp", ageMillis: 60_000) + + let readable = try awaitListReadable(minAgeMillis: 0) + + XCTAssertEqual(readable.map { $0.id }, ["fresh.otlp"]) + XCTAssertFalse(fileExists("expired.otlp")) + XCTAssertTrue(fileExists("fresh.otlp")) + } + + func testRecordInsideTheAgeWindowIsRetained() throws { + let ceiling = CrashRetention.shared.maxReadAgeMillis + try writeRecord(named: "edge.otlp", ageMillis: ceiling - 60_000) + + let readable = try awaitListReadable(minAgeMillis: 0) + + XCTAssertEqual(readable.map { $0.id }, ["edge.otlp"]) + XCTAssertTrue(fileExists("edge.otlp")) + } + + // MARK: - accumulation caps + + func testListReadableReclaimsAnInheritedOverCapBacklog() throws { + // The upgrade case: a directory written by a build with no caps. It must be trimmed on + // the first uploader pass rather than waiting for the next crash to trigger a write. + let max = Int(CrashRetention.shared.maxRecordCount) + for index in 0..<(max + 10) { + try writeRecord(named: "seed-\(index).otlp", ageMillis: Int64(1_000 * (index + 1))) + } + + let readable = try awaitListReadable(minAgeMillis: 0) + + XCTAssertEqual(readable.count, max) + XCTAssertEqual(ownedFileNames().count, max) + } + + func testSaveEvictsOldestFirstPastTheCountCap() throws { + let max = Int(CrashRetention.shared.maxRecordCount) + for index in 0.. FileLogStore { + FileLogStore(rootPath: temporaryDirectory.path) + } + + private func writeRecord(named name: String, ageMillis: Int64, bytes: Int = 16) throws { + let url = temporaryDirectory.appendingPathComponent(name) + try Data(count: bytes).write(to: url) + let modified = Date(timeIntervalSinceNow: -Double(ageMillis) / 1_000) + try FileManager.default.setAttributes([.modificationDate: modified], ofItemAtPath: url.path) + } + + private func fileExists(_ name: String) -> Bool { + FileManager.default.fileExists(atPath: temporaryDirectory.appendingPathComponent(name).path) + } + + private func ownedFileNames() -> [String] { + let contents = (try? FileManager.default.contentsOfDirectory(atPath: temporaryDirectory.path)) ?? [] + return contents.filter { $0.hasSuffix(".otlp") } + } + + private func awaitListReadable(minAgeMillis: Int64) throws -> [StoredLogFile] { + let expectation = expectation(description: "listReadable") + var result: [StoredLogFile] = [] + makeStore().listReadable(minAgeMillis: minAgeMillis) { entries, _ in + result = entries ?? [] + expectation.fulfill() + } + wait(for: [expectation], timeout: 5) + return result + } + + private func awaitDeleteUnrecognized(minAgeMillis: Int64) throws -> Int { + let expectation = expectation(description: "deleteUnrecognizedEntries") + var deleted = 0 + makeStore().deleteUnrecognizedEntries(minAgeMillis: minAgeMillis) { count, _ in + deleted = Int(truncating: count ?? 0) + expectation.fulfill() + } + wait(for: [expectation], timeout: 5) + return deleted + } +} From 4184f1be0f6b39ccb4901ea6c6136c8d61a55386 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Wed, 26 Aug 2026 10:33:28 -0500 Subject: [PATCH 2/7] refactor: [SDK-5065] adopt the grouped CrashRetention policy MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shared selectors moved their four bounds into a single CrashRetentionPolicy and selectOverflowOwned gained a nowMs parameter, so these call sites no longer compile against the retention PR. Binds the policy once as a static and passes it through, which is the point of the value type: Kotlin default arguments do not cross the Objective-C boundary, so each site previously restated maxTotalBytes and maxRecordBytes as adjacent same-typed numbers that a copied-and-edited call could silently swap. Also passes nowMs to the write-path overflow call. Ordering now accounts for future-dated records, and the write path enforces caps without running expiry first, so it cannot assume that pass already removed them. Adds the contract note about feeding overflow only the survivors of expiry — the behaviour was already correct here, but only by construction. 112 OSCore tests pass against a framework built from the retention branch head. Co-authored-by: Cursor --- .../Source/Logging/FileLogStore.swift | 34 +++++++++---------- .../FileLogStoreRetentionTests.swift | 16 ++++----- 2 files changed, 25 insertions(+), 25 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift index 96c759dc0..48fc756d7 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift @@ -48,6 +48,11 @@ final class FileLogStore: ILogFileStore { private static let temporaryFileSuffix = ".otlp.tmp" private static let queueLabel = "com.onesignal.logger.file-store" + /// The shared bounds, named once. Kotlin default arguments do not cross the Objective-C + /// boundary, so every selector call must pass this explicitly — binding it here keeps the + /// four numbers from being restated, and possibly transposed, at each call site. + private static let retentionPolicy = CrashRetention.shared.defaultPolicy + private let rootURL: URL private let fileManager: FileManager private let ioQueue = DispatchQueue(label: queueLabel, qos: .utility) @@ -65,11 +70,11 @@ final class FileLogStore: ILogFileStore { // Refuse rather than store-then-reclaim. A record this large would either claim the // whole shared budget or be deleted before it could be uploaded, so losing it loudly // here beats losing it silently on a later launch. - guard Int64(bytes.size) <= CrashRetention.shared.maxRecordBytes else { + guard Int64(bytes.size) <= Self.retentionPolicy.maxRecordBytes else { OneSignalLog.onesignalLog( .LL_WARN, message: "FileLogStore refusing record of \(bytes.size) bytes, over the " - + "\(CrashRetention.shared.maxRecordBytes)-byte limit" + + "\(Self.retentionPolicy.maxRecordBytes)-byte limit" ) return false } @@ -165,7 +170,7 @@ final class FileLogStore: ILogFileStore { let now = Self.nowMillis() return entries - .filter { CrashRetention.shared.isOwned(name: $0.name, ownedSuffix: Self.ownedFileSuffix) } + .filter { CrashRetention.shared.isOwned(name: $0.name, policy: Self.retentionPolicy) } .filter { !reclaimed.contains($0.name) } .filter { now - $0.lastModifiedMs >= max(0, minAgeMillis) } .compactMap { entry in @@ -188,22 +193,22 @@ final class FileLogStore: ILogFileStore { let expired = CrashRetention.shared.selectExpiredOwned( entries: entries, nowMs: now, - maxAgeMillis: CrashRetention.shared.maxReadAgeMillis, - ownedSuffix: Self.ownedFileSuffix + policy: Self.retentionPolicy ) for entry in expired { withheld.insert(entry.name) remove(name: entry.name) } + // Only the survivors of the expiry pass, per the ILogFileStore contract: an expired + // record still in the listing consumes a count slot and budget, so the overflow pass + // would evict live records to make room for ones already being deleted. let survivors = entries.filter { !withheld.contains($0.name) } let overflow = CrashRetention.shared.selectOverflowOwned( entries: survivors, - maxCount: CrashRetention.shared.maxRecordCount, - maxTotalBytes: CrashRetention.shared.maxTotalBytes, - maxRecordBytes: CrashRetention.shared.maxRecordBytes, + nowMs: now, keepName: nil, - ownedSuffix: Self.ownedFileSuffix + policy: Self.retentionPolicy ) for entry in overflow { withheld.insert(entry.name) @@ -229,20 +234,15 @@ final class FileLogStore: ILogFileStore { } guard !CrashRetention.shared.isWithinCaps( entries: entries, - maxCount: CrashRetention.shared.maxRecordCount, - maxTotalBytes: CrashRetention.shared.maxTotalBytes, - maxRecordBytes: CrashRetention.shared.maxRecordBytes, - ownedSuffix: Self.ownedFileSuffix + policy: Self.retentionPolicy ) else { return } let overflow = CrashRetention.shared.selectOverflowOwned( entries: entries, - maxCount: CrashRetention.shared.maxRecordCount, - maxTotalBytes: CrashRetention.shared.maxTotalBytes, - maxRecordBytes: CrashRetention.shared.maxRecordBytes, + nowMs: Self.nowMillis(), keepName: keepName, - ownedSuffix: Self.ownedFileSuffix + policy: Self.retentionPolicy ) for entry in overflow { remove(name: entry.name) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift index 1fb12be09..f2766364e 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift @@ -51,7 +51,7 @@ final class FileLogStoreRetentionTests: XCTestCase { func testRefusesPayloadOverThePerRecordLimit() { let store = makeStore() - let oversized = Data(count: Int(CrashRetention.shared.maxRecordBytes) + 1) + let oversized = Data(count: Int(CrashRetention.shared.defaultPolicy.maxRecordBytes) + 1) XCTAssertFalse(store.save(bytes: oversized.kotlinByteArray)) XCTAssertEqual(ownedFileNames().count, 0) @@ -59,7 +59,7 @@ final class FileLogStoreRetentionTests: XCTestCase { func testAcceptsPayloadAtTheLimit() { let store = makeStore() - let atLimit = Data(count: Int(CrashRetention.shared.maxRecordBytes)) + let atLimit = Data(count: Int(CrashRetention.shared.defaultPolicy.maxRecordBytes)) XCTAssertTrue(store.save(bytes: atLimit.kotlinByteArray)) XCTAssertEqual(ownedFileNames().count, 1) @@ -68,7 +68,7 @@ final class FileLogStoreRetentionTests: XCTestCase { // MARK: - age ceiling func testListReadableDropsAndDeletesRecordsPastTheAgeCeiling() throws { - let ceiling = CrashRetention.shared.maxReadAgeMillis + let ceiling = CrashRetention.shared.defaultPolicy.maxReadAgeMillis try writeRecord(named: "expired.otlp", ageMillis: ceiling + 60_000) try writeRecord(named: "fresh.otlp", ageMillis: 60_000) @@ -80,7 +80,7 @@ final class FileLogStoreRetentionTests: XCTestCase { } func testRecordInsideTheAgeWindowIsRetained() throws { - let ceiling = CrashRetention.shared.maxReadAgeMillis + let ceiling = CrashRetention.shared.defaultPolicy.maxReadAgeMillis try writeRecord(named: "edge.otlp", ageMillis: ceiling - 60_000) let readable = try awaitListReadable(minAgeMillis: 0) @@ -94,7 +94,7 @@ final class FileLogStoreRetentionTests: XCTestCase { func testListReadableReclaimsAnInheritedOverCapBacklog() throws { // The upgrade case: a directory written by a build with no caps. It must be trimmed on // the first uploader pass rather than waiting for the next crash to trigger a write. - let max = Int(CrashRetention.shared.maxRecordCount) + let max = Int(CrashRetention.shared.defaultPolicy.maxRecordCount) for index in 0..<(max + 10) { try writeRecord(named: "seed-\(index).otlp", ageMillis: Int64(1_000 * (index + 1))) } @@ -106,7 +106,7 @@ final class FileLogStoreRetentionTests: XCTestCase { } func testSaveEvictsOldestFirstPastTheCountCap() throws { - let max = Int(CrashRetention.shared.maxRecordCount) + let max = Int(CrashRetention.shared.defaultPolicy.maxRecordCount) for index in 0.. Date: Wed, 26 Aug 2026 13:15:48 -0500 Subject: [PATCH 3/7] chore: [SDK-5065] pin OneSignal-KMP-SDK to merged crash-retention policy Moves the submodule from 87e87fd to 64ce06b, KMP main. This PR previously built only against an unmerged branch head; the pin now sits on merged commits. Brings in the shared CrashRetention policy this PR adopts (KMP #20) and bounded retry/backoff in the export path (KMP #21). Co-authored-by: Cursor --- OneSignal-KMP-SDK | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/OneSignal-KMP-SDK b/OneSignal-KMP-SDK index 87e87fd26..64ce06b3e 160000 --- a/OneSignal-KMP-SDK +++ b/OneSignal-KMP-SDK @@ -1 +1 @@ -Subproject commit 87e87fd264284448541bfc34b7f1b0673bbe2dbb +Subproject commit 64ce06b3ec233cfcbebb6e0acbd3fe23b78c6a4c From abaf2d1c0961dc17a16fb41e38e7cd3e4eb9a02b Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Wed, 26 Aug 2026 14:17:27 -0500 Subject: [PATCH 4/7] fix: [SDK-5065] couple the write suffix to the shared policy and bound the temp sweep FileLogStore checked ownership through CrashRetention's policy but wrote its filenames from a local `.otlp` constant. The two agree today, so nothing is broken, but only iOS can drift: if `ownedSuffix` ever changes, Android follows automatically while iOS would keep writing records its own `isOwned` rejects, hiding every brand-new crash record from readers. The interrupted-write sweep also unlinked with `try` inside its loop, so the first failure aborted the whole pass. A record locked under `completeUntilFirstUserAuthentication` before first unlock would strand every later leftover indefinitely. `remove(name:)` now reports success and the sweep continues per entry, matching Android. Three tests, each verified to fail against the un-fixed code: - the just-written record survives whatever order the backlog lists in. The previous version passed with `keepName` removed entirely: both sort keys clamp to now, so a fresh record ties with a future-dated backlog and its position is filesystem-dependent. Repeating the trial makes a false pass vanishingly unlikely. - the total byte cap evicts oldest-first while the count stays under its bound. This pins the capped budget claim, which nothing covered before: `fileSize` is optional, and a silent `?? 0` would have zeroed every claim undetected. - written names satisfy the policy's own `isOwned`, closing the seam above. Co-authored-by: Cursor --- .../Source/Logging/FileLogStore.swift | 25 +++++-- .../FileLogStoreRetentionTests.swift | 65 ++++++++++++++++--- 2 files changed, 75 insertions(+), 15 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift index 48fc756d7..9f8377610 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift @@ -42,10 +42,13 @@ import OneSignalCore /// those bounds a record that never uploads successfully is re-read and re-sent on every /// launch for the life of the install. final class FileLogStore: ILogFileStore { - /// Complete records use `.otlp`; interrupted durable writes leave - /// `.otlp.tmp` files that are safe to reap after the minimum-age gate. - private static let ownedFileSuffix = ".otlp" - private static let temporaryFileSuffix = ".otlp.tmp" + /// Complete records use the shared policy's suffix; interrupted durable writes leave + /// `.tmp` files alongside them that are safe to reap after the minimum-age gate. + /// + /// Taken from the policy rather than restated, so what this store writes cannot drift out + /// of what `isOwned` accepts — a mismatch would hide brand-new records from every reader. + private static let ownedFileSuffix = CrashRetention.shared.defaultPolicy.ownedSuffix + private static let temporaryFileSuffix = ownedFileSuffix + ".tmp" private static let queueLabel = "com.onesignal.logger.file-store" /// The shared bounds, named once. Kotlin default arguments do not cross the Objective-C @@ -149,8 +152,11 @@ final class FileLogStore: ILogFileStore { for entry in try self.directoryEntries() where entry.name.hasSuffix(Self.temporaryFileSuffix) && now - entry.lastModifiedMs >= max(0, minAgeMillis) { - try self.fileManager.removeItem(at: self.rootURL.appendingPathComponent(entry.name)) - deleted += 1 + // Per-entry, so one undeletable leftover cannot strand the rest of the + // sweep — a file locked before first unlock would otherwise wedge it. + if self.remove(name: entry.name) { + deleted += 1 + } } } catch { OneSignalLog.onesignalLog( @@ -249,14 +255,19 @@ final class FileLogStore: ILogFileStore { } } - private func remove(name: String) { + /// - Returns: whether the file is gone. Callers reclaiming records ignore this — a failed + /// unlink is still withheld from readers — but the temp sweep counts only real deletions. + @discardableResult + private func remove(name: String) -> Bool { do { try fileManager.removeItem(at: rootURL.appendingPathComponent(name)) + return true } catch { OneSignalLog.onesignalLog( .LL_WARN, message: "FileLogStore failed to reclaim \(name): \(error.localizedDescription)" ) + return false } } diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift index f2766364e..305c6d66f 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift @@ -119,18 +119,67 @@ final class FileLogStoreRetentionTests: XCTestCase { XCTAssertTrue(fileExists("seed-0.otlp")) } - func testSaveNeverEvictsTheRecordItJustWrote() throws { + func testSaveEvictsOldestFirstPastTheTotalByteCap() throws { + // Each record sits just under the per-record cap, so the count stays well inside its + // bound and only the combined budget claim can breach — the one bound the count-cap + // tests would not notice being dropped. + let policy = CrashRetention.shared.defaultPolicy + let nearCap = Int(policy.maxRecordBytes) - 12_288 + for index in 0..<5 { + try writeRecord( + named: "big-\(index).otlp", + ageMillis: Int64(10_000 * (index + 1)), + bytes: nearCap + ) + } + + XCTAssertTrue(makeStore().save(bytes: Data("new".utf8).kotlinByteArray)) + + // Four near-cap records plus the new one exhaust the budget; the oldest loses. + XCTAssertEqual(ownedFileNames().count, 5) + XCTAssertFalse(fileExists("big-4.otlp")) + XCTAssertTrue(fileExists("big-0.otlp")) + } + + /// Both sort keys clamp to now, so a record written while the backlog is dated ahead of the + /// clock ties with it, and the order inside that tie group is whatever the filesystem lists. + /// Only the explicit `keepName` reservation guarantees the new record survives; without it + /// eviction is a coin flip, so a single attempt would pass most of the time. Repeating drives + /// the odds of a false pass to nil. + func testSaveNeverEvictsTheRecordItJustWroteWhateverOrderTheBacklogListsIn() throws { let max = Int(CrashRetention.shared.defaultPolicy.maxRecordCount) - for index in 0..<(max + 5) { - try writeRecord(named: "seed-\(index).otlp", ageMillis: Int64(1_000 * (index + 1))) + + for _ in 0..<25 { + for name in try FileManager.default.contentsOfDirectory(atPath: temporaryDirectory.path) { + try FileManager.default.removeItem(at: temporaryDirectory.appendingPathComponent(name)) + } + for index in 0..<(max + 3) { + let ahead = Int64(Date().timeIntervalSince1970 * 1_000) + 3_600_000 + Int64(index) + try writeRecord(named: "\(ahead)-seed\(index).otlp", ageMillis: -60_000, bytes: 1) + } + + XCTAssertTrue(makeStore().save(bytes: Data("new".utf8).kotlinByteArray)) + + let survivors = ownedFileNames() + XCTAssertEqual(survivors.count, max) + // The just-written record is the only one whose name has no "seed" marker. + XCTAssertEqual(survivors.filter { !$0.contains("seed") }.count, 1) } + } + + // MARK: - shared-policy coupling + func testWrittenRecordsUseTheSharedPolicySuffix() { + // The store's own ownership check runs through the policy, so if what it writes ever + // drifted from `ownedSuffix` every brand-new record would be invisible to readers. XCTAssertTrue(makeStore().save(bytes: Data("new".utf8).kotlinByteArray)) - let survivors = ownedFileNames() - XCTAssertEqual(survivors.count, max) - // The just-written record is the only one not named seed-*. - XCTAssertEqual(survivors.filter { !$0.hasPrefix("seed-") }.count, 1) + let policy = CrashRetention.shared.defaultPolicy + let written = (try? FileManager.default.contentsOfDirectory(atPath: temporaryDirectory.path)) ?? [] + XCTAssertEqual(written.count, 1) + let name = written.first ?? "" + XCTAssertTrue(name.hasSuffix(policy.ownedSuffix)) + XCTAssertTrue(CrashRetention.shared.isOwned(name: name, policy: policy)) } // MARK: - foreign entries @@ -192,7 +241,7 @@ final class FileLogStoreRetentionTests: XCTestCase { private func ownedFileNames() -> [String] { let contents = (try? FileManager.default.contentsOfDirectory(atPath: temporaryDirectory.path)) ?? [] - return contents.filter { $0.hasSuffix(".otlp") } + return contents.filter { $0.hasSuffix(CrashRetention.shared.defaultPolicy.ownedSuffix) } } private func awaitListReadable(minAgeMillis: Int64) throws -> [StoredLogFile] { From a714474e42d6353e951e82dd95920e6fefdd88f2 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Wed, 26 Aug 2026 14:40:13 -0500 Subject: [PATCH 5/7] test: [SDK-5065] pin that an undeletable expired record stays withheld MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `reclaim` inserts an expired record's name into the withheld set before attempting the unlink, so a delete that fails still keeps the record away from readers. Nothing pinned that ordering: moving the insert inside the success branch would compile, pass every other test, and hand a permanently undeletable record to the uploader on every pass forever. Unlinks do fail in practice — a read-only directory, a filesystem error, or data protection before first unlock. The test forces one by denying writes on the fixture directory, which fails `removeItem` without making the entries unreadable, and `tearDown` restores the permissions so the fixture is still removable. Mirrors the Android coverage in `FileLogStoreTest`. Co-authored-by: Cursor --- .../FileLogStoreRetentionTests.swift | 29 +++++++++++++++++++ 1 file changed, 29 insertions(+) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift index 305c6d66f..48e822cc6 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift @@ -44,6 +44,12 @@ final class FileLogStoreRetentionTests: XCTestCase { } override func tearDownWithError() throws { + // A test may have made the directory read-only to force an unlink failure; it has to be + // writable again or the fixture outlives the run. + try? FileManager.default.setAttributes( + [.posixPermissions: 0o700], + ofItemAtPath: temporaryDirectory.path + ) try? FileManager.default.removeItem(at: temporaryDirectory) } @@ -89,6 +95,29 @@ final class FileLogStoreRetentionTests: XCTestCase { XCTAssertTrue(fileExists("edge.otlp")) } + func testExpiredRecordThatCannotBeDeletedIsStillWithheldFromReaders() throws { + // An unlink can fail: a read-only directory, a filesystem error, or data protection + // before first unlock. Withholding must not be contingent on the delete succeeding — + // otherwise a permanently undeletable expired record is handed to the uploader on + // every pass, forever. + let ceiling = CrashRetention.shared.defaultPolicy.maxReadAgeMillis + try writeRecord(named: "expired-stuck.otlp", ageMillis: ceiling + 60_000) + try writeRecord(named: "fresh.otlp", ageMillis: 60_000) + // Denying writes on the directory fails the unlink without making the entries + // unreadable, so the reclaim path runs exactly as it would against a stuck record. + try FileManager.default.setAttributes( + [.posixPermissions: 0o500], + ofItemAtPath: temporaryDirectory.path + ) + + let readable = try awaitListReadable(minAgeMillis: 0) + + // The record surviving is the premise of the test, not the behavior under test: if the + // removal had gone through this would only be re-covering the ordinary expiry path. + XCTAssertTrue(fileExists("expired-stuck.otlp")) + XCTAssertEqual(readable.map { $0.id }, ["fresh.otlp"]) + } + // MARK: - accumulation caps func testListReadableReclaimsAnInheritedOverCapBacklog() throws { From cf50a0ac6ed8f21f7ae2c41757fec5ddac1d4cc2 Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Wed, 26 Aug 2026 16:17:56 -0500 Subject: [PATCH 6/7] fix: [SDK-5065] keep records with unreadable attributes inside the caps MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `directoryEntries` dropped any file whose modification date could not be read. A dropped entry is outside every bound at once: uncounted by `isWithinCaps`, unselectable by `selectExpiredOwned` and `selectOverflowOwned`, never reaped as `.otlp.tmp`, never returned to readers. It occupies disk for the life of the install while sitting entirely outside the retention this work exists to enforce, and an unreadable attribute is reachable on iOS — data protection before first unlock is one route. Android does not have this hole: `File.lastModified()` yields 0 on failure rather than dropping the record, so expiry sees `nowMs - 0` and reclaims it. Same shared policy, opposite outcomes. Falling back to 0 puts iOS on the same footing — the entry stays owned and tmp-eligible and is reclaimed as unrecoverably stale. `lengthBytes` already matched Android, whose `File.length()` also returns 0 on failure, so the existing fallback stands. `compactMap` becomes `map` now that nothing is dropped. The attribute read is injectable because this failure cannot be staged on a real filesystem: denying directory access fails `contentsOfDirectory` outright rather than the per-file lookup. The new test pins that such a record is deleted rather than leaked, and that live records are unaffected. Also documents the write path's deliberate overflow-only split, which two reviews have now read as a bug against the `ILogFileStore` contract. Co-authored-by: Cursor --- .../Source/Logging/FileLogStore.swift | 39 +++++++++++++++---- .../FileLogStoreRetentionTests.swift | 34 ++++++++++++++-- 2 files changed, 62 insertions(+), 11 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift index 9f8377610..e66bc1960 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift @@ -56,13 +56,28 @@ final class FileLogStore: ILogFileStore { /// four numbers from being restated, and possibly transposed, at each call site. private static let retentionPolicy = CrashRetention.shared.defaultPolicy + /// Reads the attributes a `CrashDirEntry` is built from. Injectable because the failure that + /// matters here — attributes unreadable while the directory still lists — cannot be staged on + /// a real filesystem: revoking directory access fails the listing itself instead. + typealias AttributeLookup = (URL) -> URLResourceValues? + + static let defaultAttributeLookup: AttributeLookup = { url in + try? url.resourceValues(forKeys: [.contentModificationDateKey, .fileSizeKey]) + } + private let rootURL: URL private let fileManager: FileManager + private let attributeLookup: AttributeLookup private let ioQueue = DispatchQueue(label: queueLabel, qos: .utility) - init(rootPath: String, fileManager: FileManager = .default) { + init( + rootPath: String, + fileManager: FileManager = .default, + attributeLookup: @escaping AttributeLookup = FileLogStore.defaultAttributeLookup + ) { self.rootURL = URL(fileURLWithPath: rootPath, isDirectory: true) self.fileManager = fileManager + self.attributeLookup = attributeLookup try? createRootDirectory() } @@ -234,6 +249,11 @@ final class FileLogStore: ILogFileStore { /// Trims the directory back inside the accumulation caps after a write, always keeping /// [keepName]. Runs synchronously on the crashing thread, so it exits on one directory /// listing in the steady state and only sorts when the caps are actually breached. + /// + /// Overflow only, deliberately: expiry is a scan the crashing thread should not pay for when + /// nothing is over cap, and an under-cap expired record is reclaimed on the next uploader + /// pass by `listReadable` / `deleteUnrecognizedEntries`, both of which run expiry. This is + /// the same split Android's write path makes. private func enforceAccumulationCaps(keepName: String) { guard let entries = try? directoryEntries() else { return @@ -272,15 +292,20 @@ final class FileLogStore: ILogFileStore { } /// Snapshots the directory as the platform-neutral entries the shared policy consumes. + /// + /// An unreadable attribute — data protection before first unlock, a transient I/O error — + /// dates the entry to the epoch instead of omitting it. Omission would put the file outside + /// every bound at once: uncounted by the caps, unselectable by either reclaim pass, and so + /// leaked for the life of the install. Epoch keeps it in the snapshot and reads as + /// unrecoverably stale, which is what Android's `File.lastModified()` already yields on the + /// same failure, so both platforms reclaim it. private func directoryEntries() throws -> [CrashDirEntry] { - try fileURLs().compactMap { url in - let values = try? url.resourceValues(forKeys: [.contentModificationDateKey, .fileSizeKey]) - guard let modifiedAt = values?.contentModificationDate else { - return nil - } + try fileURLs().map { url in + let values = attributeLookup(url) + let modifiedAt = values?.contentModificationDate return CrashDirEntry( name: url.lastPathComponent, - lastModifiedMs: Int64(modifiedAt.timeIntervalSince1970 * 1_000), + lastModifiedMs: modifiedAt.map { Int64($0.timeIntervalSince1970 * 1_000) } ?? 0, lengthBytes: Int64(values?.fileSize ?? 0) ) } diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift index 48e822cc6..4db128716 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift @@ -118,6 +118,22 @@ final class FileLogStoreRetentionTests: XCTestCase { XCTAssertEqual(readable.map { $0.id }, ["fresh.otlp"]) } + func testRecordWithUnreadableAttributesIsReclaimedRatherThanLeaked() throws { + // Data protection before first unlock can deny the modification date while the directory + // still lists. If such a file were dropped from the snapshot it would sit outside every + // bound — uncounted by the caps, unselectable by either reclaim pass — and occupy disk + // permanently. Android's `File.lastModified()` returns 0 on the same failure and reaps + // the file; iOS has to reach the same outcome. + try writeRecord(named: "opaque.otlp", ageMillis: 60_000) + try writeRecord(named: "fresh.otlp", ageMillis: 60_000) + + let readable = try awaitListReadable(minAgeMillis: 0, attributesUnreadableFor: ["opaque.otlp"]) + + XCTAssertFalse(fileExists("opaque.otlp")) + XCTAssertEqual(readable.map { $0.id }, ["fresh.otlp"]) + XCTAssertTrue(fileExists("fresh.otlp")) + } + // MARK: - accumulation caps func testListReadableReclaimsAnInheritedOverCapBacklog() throws { @@ -253,8 +269,15 @@ final class FileLogStoreRetentionTests: XCTestCase { // MARK: - helpers - private func makeStore() -> FileLogStore { - FileLogStore(rootPath: temporaryDirectory.path) + /// - Parameter attributesUnreadableFor: names whose resource values the store should see as + /// unavailable, standing in for a filesystem that denies them. There is no way to stage + /// that on disk: revoking directory access fails the listing instead of the per-file read. + private func makeStore(attributesUnreadableFor denied: Set = []) -> FileLogStore { + FileLogStore(rootPath: temporaryDirectory.path) { url in + denied.contains(url.lastPathComponent) + ? nil + : FileLogStore.defaultAttributeLookup(url) + } } private func writeRecord(named name: String, ageMillis: Int64, bytes: Int = 16) throws { @@ -273,10 +296,13 @@ final class FileLogStoreRetentionTests: XCTestCase { return contents.filter { $0.hasSuffix(CrashRetention.shared.defaultPolicy.ownedSuffix) } } - private func awaitListReadable(minAgeMillis: Int64) throws -> [StoredLogFile] { + private func awaitListReadable( + minAgeMillis: Int64, + attributesUnreadableFor denied: Set = [] + ) throws -> [StoredLogFile] { let expectation = expectation(description: "listReadable") var result: [StoredLogFile] = [] - makeStore().listReadable(minAgeMillis: minAgeMillis) { entries, _ in + makeStore(attributesUnreadableFor: denied).listReadable(minAgeMillis: minAgeMillis) { entries, _ in result = entries ?? [] expectation.fulfill() } From 1e9949f432779459b4f788fb4e50f9dc8fe1111d Mon Sep 17 00:00:00 2001 From: AR Abdul Azeez Date: Thu, 27 Aug 2026 11:08:00 -0500 Subject: [PATCH 7/7] fix: [SDK-5065] preserve in-flight temps and crash-safe FileLogStore I/O Unknown mtime stays stale for owned records but no longer makes an in-flight .otlp.tmp look old enough to reap; save() logs through OSCrashLogger, and already-gone unlinks count as success against a racing reclaim. Co-authored-by: Cursor --- .../Source/Logging/FileLogStore.swift | 128 ++++++++++---- .../FileLogStoreRetentionTests.swift | 167 ++++++++++++++++-- 2 files changed, 248 insertions(+), 47 deletions(-) diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift index e66bc1960..e289fffe1 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCore/Source/Logging/FileLogStore.swift @@ -62,21 +62,32 @@ final class FileLogStore: ILogFileStore { typealias AttributeLookup = (URL) -> URLResourceValues? static let defaultAttributeLookup: AttributeLookup = { url in - try? url.resourceValues(forKeys: [.contentModificationDateKey, .fileSizeKey]) + try? url.resourceValues(forKeys: [ + .contentModificationDateKey, + .fileSizeKey, + .isRegularFileKey + ]) } private let rootURL: URL private let fileManager: FileManager private let attributeLookup: AttributeLookup + /// Crash-path diagnostics. `OneSignalLog` fans out to app listeners and the remote sink. + private let crashWarn: (String) -> Void private let ioQueue = DispatchQueue(label: queueLabel, qos: .utility) + private let inFlightLock = NSLock() + private var inFlightNames = Set() init( rootPath: String, fileManager: FileManager = .default, + crashWarn: ((String) -> Void)? = nil, attributeLookup: @escaping AttributeLookup = FileLogStore.defaultAttributeLookup ) { self.rootURL = URL(fileURLWithPath: rootPath, isDirectory: true) self.fileManager = fileManager + let crashLogger = OSCrashLogger() + self.crashWarn = crashWarn ?? { crashLogger.warn(message: $0) } self.attributeLookup = attributeLookup try? createRootDirectory() } @@ -89,9 +100,8 @@ final class FileLogStore: ILogFileStore { // whole shared budget or be deleted before it could be uploaded, so losing it loudly // here beats losing it silently on a later launch. guard Int64(bytes.size) <= Self.retentionPolicy.maxRecordBytes else { - OneSignalLog.onesignalLog( - .LL_WARN, - message: "FileLogStore refusing record of \(bytes.size) bytes, over the " + crashWarn( + "FileLogStore refusing record of \(bytes.size) bytes, over the " + "\(Self.retentionPolicy.maxRecordBytes)-byte limit" ) return false @@ -100,8 +110,12 @@ final class FileLogStore: ILogFileStore { try createRootDirectory() let timestamp = Int64(Date().timeIntervalSince1970 * 1_000) let id = "\(timestamp)-\(UUID().uuidString)\(Self.ownedFileSuffix)" - try writeDurably(bytes.data, to: rootURL.appendingPathComponent(id)) - enforceAccumulationCaps(keepName: id) + let targetURL = rootURL.appendingPathComponent(id) + let tmpName = targetURL.appendingPathExtension("tmp").lastPathComponent + try withInFlightNames([id, tmpName]) { + try writeDurably(bytes.data, to: targetURL) + enforceAccumulationCaps(keepName: id) + } return true } catch { return false @@ -164,8 +178,13 @@ final class FileLogStore: ILogFileStore { // legacy format to clean up here — only this store's own interrupted writes. // Anything else in the directory belongs to someone we should not assume about. let now = Self.nowMillis() + let inFlight = self.snapshotInFlightNames() for entry in try self.directoryEntries() where entry.name.hasSuffix(Self.temporaryFileSuffix) + // Unknown mtime is stored as 0, which would otherwise look older than any + // age gate — including an in-flight write whose attributes cannot be read. + && entry.lastModifiedMs > 0 + && !inFlight.contains(entry.name) && now - entry.lastModifiedMs >= max(0, minAgeMillis) { // Per-entry, so one undeletable leftover cannot strand the rest of the // sweep — a file locked before first unlock would otherwise wedge it. @@ -211,12 +230,13 @@ final class FileLogStore: ILogFileStore { let now = Self.nowMillis() var withheld = Set() + let inFlight = snapshotInFlightNames() let expired = CrashRetention.shared.selectExpiredOwned( entries: entries, nowMs: now, policy: Self.retentionPolicy ) - for entry in expired { + for entry in expired where !inFlight.contains(entry.name) { withheld.insert(entry.name) remove(name: entry.name) } @@ -228,10 +248,10 @@ final class FileLogStore: ILogFileStore { let overflow = CrashRetention.shared.selectOverflowOwned( entries: survivors, nowMs: now, - keepName: nil, + keepName: inFlight.first { CrashRetention.shared.isOwned(name: $0, policy: Self.retentionPolicy) }, policy: Self.retentionPolicy ) - for entry in overflow { + for entry in overflow where !inFlight.contains(entry.name) { withheld.insert(entry.name) remove(name: entry.name) } @@ -271,41 +291,65 @@ final class FileLogStore: ILogFileStore { policy: Self.retentionPolicy ) for entry in overflow { - remove(name: entry.name) + remove(name: entry.name, crashSafe: true) } } /// - Returns: whether the file is gone. Callers reclaiming records ignore this — a failed /// unlink is still withheld from readers — but the temp sweep counts only real deletions. + /// An already-removed file is success: crash-path eviction and the async reclaim can + /// target the same name. @discardableResult - private func remove(name: String) -> Bool { + private func remove(name: String, crashSafe: Bool = false) -> Bool { do { try fileManager.removeItem(at: rootURL.appendingPathComponent(name)) return true } catch { - OneSignalLog.onesignalLog( - .LL_WARN, - message: "FileLogStore failed to reclaim \(name): \(error.localizedDescription)" - ) + if fileLogStoreIsAlreadyRemoved(error) { + return true + } + let message = "FileLogStore failed to reclaim \(name): \(error.localizedDescription)" + if crashSafe { + crashWarn(message) + } else { + OneSignalLog.onesignalLog(.LL_WARN, message: message) + } return false } } /// Snapshots the directory as the platform-neutral entries the shared policy consumes. /// - /// An unreadable attribute — data protection before first unlock, a transient I/O error — - /// dates the entry to the epoch instead of omitting it. Omission would put the file outside - /// every bound at once: uncounted by the caps, unselectable by either reclaim pass, and so - /// leaked for the life of the install. Epoch keeps it in the snapshot and reads as - /// unrecoverably stale, which is what Android's `File.lastModified()` already yields on the - /// same failure, so both platforms reclaim it. + /// Unreadable attributes — data protection before first unlock, a transient I/O error — + /// keep the entry in the snapshot rather than omitting it. Omission would put the file + /// outside every bound at once. Missing mtime is stored as 0: owned records then read as + /// unrecoverably stale (matching Android's `File.lastModified()`), while the temp sweep + /// treats 0 as unknown and leaves `.otlp.tmp` alone so an in-flight write is not reaped. + /// Entries are dropped only when `isRegularFile` is known false, not when that bit cannot + /// be read — the latter used to discard the file before this fallback could run. private func directoryEntries() throws -> [CrashDirEntry] { - try fileURLs().map { url in + guard fileManager.fileExists(atPath: rootURL.path) else { + return [] + } + return try fileManager.contentsOfDirectory( + at: rootURL, + includingPropertiesForKeys: [ + .contentModificationDateKey, + .fileSizeKey, + .isRegularFileKey + ], + options: [.skipsHiddenFiles] + ).compactMap { url in let values = attributeLookup(url) - let modifiedAt = values?.contentModificationDate + if values?.isRegularFile == false { + return nil + } + let name = url.lastPathComponent return CrashDirEntry( - name: url.lastPathComponent, - lastModifiedMs: modifiedAt.map { Int64($0.timeIntervalSince1970 * 1_000) } ?? 0, + name: name, + lastModifiedMs: values?.contentModificationDate.map { + Int64($0.timeIntervalSince1970 * 1_000) + } ?? 0, lengthBytes: Int64(values?.fileSize ?? 0) ) } @@ -315,17 +359,22 @@ final class FileLogStore: ILogFileStore { Int64(Date().timeIntervalSince1970 * 1_000) } - private func fileURLs() throws -> [URL] { - guard fileManager.fileExists(atPath: rootURL.path) else { - return [] - } - return try fileManager.contentsOfDirectory( - at: rootURL, - includingPropertiesForKeys: [.contentModificationDateKey, .fileSizeKey, .isRegularFileKey], - options: [.skipsHiddenFiles] - ).filter { - (try? $0.resourceValues(forKeys: [.isRegularFileKey]).isRegularFile) == true + private func withInFlightNames(_ names: [String], _ body: () throws -> Void) rethrows { + inFlightLock.lock() + inFlightNames.formUnion(names) + inFlightLock.unlock() + defer { + inFlightLock.lock() + inFlightNames.subtract(names) + inFlightLock.unlock() } + try body() + } + + private func snapshotInFlightNames() -> Set { + inFlightLock.lock() + defer { inFlightLock.unlock() } + return inFlightNames } private func isSafeEntryId(_ id: String) -> Bool { @@ -396,3 +445,12 @@ final class FileLogStore: ILogFileStore { } } } + +private func fileLogStoreIsAlreadyRemoved(_ error: Error) -> Bool { + let nsError = error as NSError + if nsError.domain == NSCocoaErrorDomain { + return nsError.code == CocoaError.fileNoSuchFile.rawValue + || nsError.code == CocoaError.fileReadNoSuchFile.rawValue + } + return nsError.domain == NSPOSIXErrorDomain && nsError.code == Int(ENOENT) +} diff --git a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift index 4db128716..85a8ddaac 100644 --- a/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift +++ b/iOS_SDK/OneSignalSDK/OneSignalOSCoreTests/FileLogStoreRetentionTests.swift @@ -26,6 +26,7 @@ */ import Foundation +import OneSignalCore import OneSignalKMP @testable import OneSignalOSCore import XCTest @@ -267,17 +268,124 @@ final class FileLogStoreRetentionTests: XCTestCase { XCTAssertEqual(ownedFileNames().count, max) } + func testInterruptedWriteWithUnreadableAttributesIsPreserved() throws { + // Epoch for a missing mtime makes an owned record stale enough to reclaim, but the + // same sentinel would make an in-flight `.otlp.tmp` look old enough to delete. + try writeRecord(named: "in-flight.otlp.tmp", ageMillis: 60_000) + + let deleted = try awaitDeleteUnrecognized( + minAgeMillis: 0, + attributesUnreadableFor: ["in-flight.otlp.tmp"] + ) + + XCTAssertEqual(deleted, 0) + XCTAssertTrue(fileExists("in-flight.otlp.tmp")) + } + + func testEntryIsKeptWhenRegularFileBitCannotBeRead() throws { + // `isRegularFile == true` dropped the URL when the bit was nil, before the attribute + // fallback could date the record. Keep unless the bit is known false. + try writeRecord(named: "present.otlp", ageMillis: 60_000) + let lookup: FileLogStore.AttributeLookup = { url in + guard url.lastPathComponent == "present.otlp" else { + return FileLogStore.defaultAttributeLookup(url) + } + var values = URLResourceValues() + values.contentModificationDate = Date(timeIntervalSinceNow: -60) + return values + } + + let readable = try awaitListReadable(minAgeMillis: 0, attributeLookup: lookup) + + XCTAssertEqual(readable.map { $0.id }, ["present.otlp"]) + XCTAssertTrue(fileExists("present.otlp")) + } + + func testDirectoryNamedLikeARecordIsNotTreatedAsOne() throws { + try FileManager.default.createDirectory( + at: temporaryDirectory.appendingPathComponent("folder.otlp"), + withIntermediateDirectories: true + ) + + let readable = try awaitListReadable(minAgeMillis: 0) + + XCTAssertEqual(readable.map { $0.id }, []) + XCTAssertTrue(fileExists("folder.otlp")) + } + + // MARK: - crash-path logging + + func testSaveDoesNotFanOutThroughOneSignalLogWhenRefusingAnOversizedRecord() { + let listener = FileLogStoreLogListener() + OneSignalLog.debug().__add(listener) + defer { OneSignalLog.debug().__remove(listener) } + var crashWarnings: [String] = [] + let oversized = Data(count: Int(CrashRetention.shared.defaultPolicy.maxRecordBytes) + 1) + + XCTAssertFalse( + makeStore(crashWarn: { crashWarnings.append($0) }) + .save(bytes: oversized.kotlinByteArray) + ) + + XCTAssertTrue(listener.entries.isEmpty) + XCTAssertEqual(crashWarnings.count, 1) + XCTAssertTrue(crashWarnings[0].contains("refusing record")) + } + + // MARK: - concurrent reclaim + + func testAlreadyRemovedFileIsASuccessfulCleanup() throws { + try writeRecord(named: "interrupted.otlp.tmp", ageMillis: 60_000) + + let deleted = try awaitDeleteUnrecognized( + minAgeMillis: 0, + fileManager: FileLogStoreMissingItemFileManager() + ) + + XCTAssertEqual(deleted, 1) + } + + func testSaveSurvivesATempSweepOfItsInFlightWrite() { + let fileManager = FileLogStoreReentrantCleanupFileManager() + let store = FileLogStore(rootPath: temporaryDirectory.path, fileManager: fileManager) + fileManager.onTemporaryWrite = { + let done = DispatchSemaphore(value: 0) + store.deleteUnrecognizedEntries(minAgeMillis: 0) { _, _ in done.signal() } + XCTAssertEqual(done.wait(timeout: .now() + 5), .success) + } + + XCTAssertTrue(store.save(bytes: Data("new".utf8).kotlinByteArray)) + XCTAssertEqual(ownedFileNames().count, 1) + XCTAssertEqual( + ((try? FileManager.default.contentsOfDirectory(atPath: temporaryDirectory.path)) ?? []) + .filter { $0.hasSuffix(".otlp.tmp") } + .count, + 0 + ) + } + // MARK: - helpers /// - Parameter attributesUnreadableFor: names whose resource values the store should see as /// unavailable, standing in for a filesystem that denies them. There is no way to stage /// that on disk: revoking directory access fails the listing instead of the per-file read. - private func makeStore(attributesUnreadableFor denied: Set = []) -> FileLogStore { - FileLogStore(rootPath: temporaryDirectory.path) { url in + private func makeStore( + attributesUnreadableFor denied: Set = [], + fileManager: FileManager = .default, + crashWarn: ((String) -> Void)? = nil, + attributeLookup: FileLogStore.AttributeLookup? = nil + ) -> FileLogStore { + let lookup = attributeLookup ?? { url in denied.contains(url.lastPathComponent) ? nil : FileLogStore.defaultAttributeLookup(url) } + return FileLogStore( + rootPath: temporaryDirectory.path, + fileManager: fileManager, + crashWarn: crashWarn, + attributeLookup: lookup + ) } private func writeRecord(named name: String, ageMillis: Int64, bytes: Int = 16) throws { @@ -298,26 +406,61 @@ final class FileLogStoreRetentionTests: XCTestCase { private func awaitListReadable( minAgeMillis: Int64, - attributesUnreadableFor denied: Set = [] + attributesUnreadableFor denied: Set = [], + attributeLookup: FileLogStore.AttributeLookup? = nil ) throws -> [StoredLogFile] { let expectation = expectation(description: "listReadable") var result: [StoredLogFile] = [] - makeStore(attributesUnreadableFor: denied).listReadable(minAgeMillis: minAgeMillis) { entries, _ in - result = entries ?? [] - expectation.fulfill() - } + makeStore(attributesUnreadableFor: denied, attributeLookup: attributeLookup) + .listReadable(minAgeMillis: minAgeMillis) { entries, _ in + result = entries ?? [] + expectation.fulfill() + } wait(for: [expectation], timeout: 5) return result } - private func awaitDeleteUnrecognized(minAgeMillis: Int64) throws -> Int { + private func awaitDeleteUnrecognized( + minAgeMillis: Int64, + attributesUnreadableFor denied: Set = [], + fileManager: FileManager = .default + ) throws -> Int { let expectation = expectation(description: "deleteUnrecognizedEntries") var deleted = 0 - makeStore().deleteUnrecognizedEntries(minAgeMillis: minAgeMillis) { count, _ in - deleted = Int(truncating: count ?? 0) - expectation.fulfill() - } + makeStore(attributesUnreadableFor: denied, fileManager: fileManager) + .deleteUnrecognizedEntries(minAgeMillis: minAgeMillis) { count, _ in + deleted = Int(truncating: count ?? 0) + expectation.fulfill() + } wait(for: [expectation], timeout: 5) return deleted } } + +private final class FileLogStoreLogListener: NSObject, OSLogListener { + var entries: [String] = [] + + func onLogEvent(_ event: OneSignalLogEvent) { + entries.append(event.entry) + } +} + +/// `removeItem` reports the file already gone, the way a racing crash-path eviction looks. +private final class FileLogStoreMissingItemFileManager: FileManager { + override func removeItem(at url: URL) throws { + throw CocoaError(.fileNoSuchFile) + } +} + +/// Invokes a hook after the durable write has created its `.tmp`, so a concurrent temp sweep +/// can race the in-flight name. +private final class FileLogStoreReentrantCleanupFileManager: FileManager { + var onTemporaryWrite: (() -> Void)? + + override func setAttributes(_ attributes: [FileAttributeKey: Any], ofItemAtPath path: String) throws { + try super.setAttributes(attributes, ofItemAtPath: path) + if path.hasSuffix(".tmp") { + onTemporaryWrite?() + } + } +}