fix: [SDK-5065] bound the crash-record cache on iOS - #1725
Conversation
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
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 <cursoragent@cursor.com>
f15096d to
f6cbab2
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Multi-model review (Opus 5, GPT 5.6 Sol, Grok 4.6) of the crash-record retention bound.
Act on
directoryEntries()drops files whose mtime cannot be read, so they are never counted, expired, overflow-evicted, or reaped as.otlp.tmp. All three reviewers flagged this: the oldisOldEnoughonly hid them from reads; with caps they now sit outside the bound.
Consider
enforceAccumulationCaps(crash-pathsave) scans the directory but only runs overflow, not expiry. Two reviewers say that violates the ILogFileStore “both selectors on every scan” contract; one argued the write-pathkeepNamesplit is intentional.- Post-write trim on the crashing thread is extra Foundation + Kotlin work (full listing, possible sort/unlinks). Two reviewers worry a pre-retention backlog can stall exception handling.
Noted
- Second directory listing in
deleteUnrecognizedEntriesis redundant if reclaim never selects.otlp.tmp. - Byte-budget eviction, failed-unlink occupancy, and future-dated mtimes are untested on iOS.
Dismissed
- The KMP pin also bringing #21 retry/backoff is already called out in the PR description.
Sent by Cursor Automation: PR Reviews
| private func directoryEntries() throws -> [CrashDirEntry] { | ||
| try fileURLs().compactMap { url in | ||
| let values = try? url.resourceValues(forKeys: [.contentModificationDateKey, .fileSizeKey]) | ||
| guard let modifiedAt = values?.contentModificationDate else { |
There was a problem hiding this comment.
warning (3/3) — Files whose mtime cannot be read are dropped from every policy decision, not just from reads.
try? resourceValues plus this guard omits the file from selectExpiredOwned, selectOverflowOwned, isWithinCaps, readableEntries, and the .otlp.tmp reap. The old isOldEnough only made such a file unreadable; with caps it now occupies disk without counting toward either bound. A missing size is also charged as 0 bytes when mtime happens to be present.
Fall back to lastModifiedMs: 0 (and a recorded size) so the entry stays owned/tmp-eligible, rather than vanishing from the snapshot.
| private func enforceAccumulationCaps(keepName: String) { | ||
| guard let entries = try? directoryEntries() else { | ||
| return | ||
| } | ||
| guard !CrashRetention.shared.isWithinCaps( | ||
| entries: entries, | ||
| policy: Self.retentionPolicy | ||
| ) else { | ||
| return | ||
| } | ||
| let overflow = CrashRetention.shared.selectOverflowOwned( | ||
| entries: entries, | ||
| nowMs: Self.nowMillis(), | ||
| keepName: keepName, | ||
| policy: Self.retentionPolicy | ||
| ) | ||
| for entry in overflow { | ||
| remove(name: entry.name) | ||
| } |
There was a problem hiding this comment.
warning (2/3) — This scan path runs overflow only, not expiry.
ILogFileStore asks implementations to run both selectExpiredOwned and selectOverflowOwned on every directory scan. Here isWithinCaps can early-return a directory of in-window-but-expired records, so save() never clears them; when caps are already full, expired entries still consume slots and each crash pays a listing+sort to evict one.
One reviewer treated the write-path keepName / no-expiry split as intentional. If it is, a short comment here would close the gap with the contract quoted above reclaim. Otherwise the entries are already in hand — one selectExpiredOwned (excluding keepName) before overflow would match the read/cleanup paths.
| 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) |
There was a problem hiding this comment.
warning (2/3) — Crash-path save() now does a full directory listing (and possibly sort/unlinks) after the durable write.
save must finish synchronously on the crashing thread. enforceAccumulationCaps always contentsOfDirectorys, builds CrashDirEntrys, and crosses into Kotlin isWithinCaps. An inherited over-cap backlog — or a directory padded with foreign files this store deliberately will not reap — is not bounded by the new caps, so this can stall chaining to the previous exception handler.
Consider enqueueing trim on ioQueue after fsync+rename, or skipping the listing unless a cheap local counter says the directory is over cap.
…d 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 <cursoragent@cursor.com>
`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 <cursoragent@cursor.com>
1 participant
Description
One Line Summary
Bounds the iOS crash-record cache — age ceiling, count/byte caps, write-size limit — using the retention policy shared from the KMP module.
Relates to SDK-5065.
Note
Dependency resolved. OneSignal-KMP-SDK#20 has merged and the
OneSignal-KMP-SDKsubmodule is now pinned to KMPmain(64ce06b), so this builds against merged code rather than a branch head.The bug
FileLogStorehas no retention at all:save()enforces no size limitlistReadable()has only the lowerminAgeMillisgate — no ceilingdeleteUnrecognizedEntries()reaps only.otlp.tmp, never owned records at any ageSo a crash record that fails to upload is re-read and re-POSTed on every launch, indefinitely, and the directory grows unbounded. The path is under
.cachesDirectory, so iOS reclaims the disk under pressure — but the repeated re-upload is unmitigated, and a stuck record keeps being sent until the OS happens to purge.This is the same defect Android hit when OpenTelemetry's
disk-bufferingwas removed and its retention went with it. Android rebuilt the policy over several review rounds. Rather than reimplement it here and let the two drift, the policy moved tocommonMainand this adopts it.What changed
Decisions come from
CrashRetention; this file keeps only the I/O — snapshot the directory intoCrashDirEntrys, apply what the selectors return.Retention runs on all three paths, which is the part Android got wrong first:
save()listReadable()deleteUnrecognizedEntries()Submodule pin
The
OneSignal-KMP-SDKpin moves87e87fd→64ce06b(KMPmain), picking up:64ce06b— the sharedCrashRetention/CrashRetentionPolicy/CrashDirEntrypolicy this PR consumes (KMP Leak on init in iOS #20)73b0802— bounded retry/backoff in the shared export path (KMP Chinese (Traditional) is showing up as Chinese (Simplified) #21)KMP #21 classifies the sender's
-1as retryable and anything unrecognised — including-3— as permanent. That lines up with #1727, already onmain, which stopped overloading-1for an unbuildable request and reports-3instead.Deliberately unchanged: foreign-file policy
The shared
selectUnrecognizedreaps any non-owned file. iOS stays narrower and keeps reaping only its own.otlp.tmp. 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.testFileStoreDeletesOnlyInterruptedTemporaryWritespins that intent and still passes unchanged.Testing
FileLogStoreRetentionTests— 10 cases covering the write-size limit and its exact boundary, the age ceiling and the record just inside it, oldest-first eviction past the count cap, never evicting the just-written record, inherited over-cap backlogs reclaimed by both the read and cleanup paths, and the preserved foreign-file behavior.Full
OneSignalOSCoresuite against the merged pin: 112 tests, 0 failures (xcodebuild test-without-building,UnitTestApp_TestPlan_Reduced, iPhone 17 Pro simulator).OneSignalOSCorealso builds clean foriphonesimulator.Verified the tests actually bite, by perturbing the code and confirming the expected cases go red:
save()→testRefusesPayloadOverThePerRecordLimitfailsCrashRetentionPolicybounds while the tests keep asserting againstCrashRetention.shared.defaultPolicy→ 5 of 10 fail (age ceiling, both over-cap backlog cases, count-cap eviction, keep-just-written), confirming the shared selectors are what drive the behavior rather than incidental local logicBoth perturbations were reverted; the numbers above are from the restored tree.
The policy decisions themselves are unit-tested in the shared module (20 cases, green on both
iosSimulatorArm64and Android JVM).