fix: [SDK-5065] bound the crash-record cache on iOS - #1725
fix: [SDK-5065] bound the crash-record cache on iOS#1725abdulraqeeb33 wants to merge 12 commits into
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.
There was a problem hiding this comment.
Fixed in cf50a0ac. This was the right call, and the divergence from Android makes it sharper than stated: Android's listEntries uses File.lastModified(), which returns 0L on failure rather than dropping the entry, so nowMs - 0 > maxReadAgeMillis is trivially true and Android reclaims the file. Same shared policy, opposite outcomes — Android cleans up, iOS leaked for the life of the install.
directoryEntries() now dates an unreadable attribute to the epoch instead of omitting it, so iOS reaches the same outcome. compactMap became map, since nothing returns nil any more and the optional-flattening was misleading about whether entries could still vanish.
On the missing-size half: no change needed. Android builds lengthBytes from File.length(), which also yields 0 when the length cannot be determined, so both platforms already agree that an unsized record claims no budget. The asymmetry was only ever in the date — Java folds that failure into a sentinel while URLResourceValues folds it into nil, and the old code escalated nil into dropping the whole record.
Covered by testRecordWithUnreadableAttributesIsReclaimedRatherThanLeaked, verified red against the old return nil.
| 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.
There was a problem hiding this comment.
Deliberate, and now commented at enforceAccumulationCaps — thanks for pushing on it. The split matches pre-deletion Android: the write path runs overflow only so the crashing thread does not pay for an expiry scan when nothing is over cap, and under-cap expired records are reclaimed on the next uploader pass by listReadable / deleteUnrecognizedEntries.
Behavior unchanged, but this is the second independent review to question it against the ILogFileStore wording, which is fair evidence the intent was not legible in the code. The comment now says why and where expiry actually happens.
| 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.
There was a problem hiding this comment.
Accepted trade-off, consistent on both platforms. save() must finish synchronously on the crashing thread, and deferring the trim to ioQueue would mean a crash that kills the process before the queue drains leaves the directory over cap — the failure mode the caps exist to prevent.
The cheap-exit you suggest is already there: isWithinCaps short-circuits before any sort or unlink, so the steady-state cost is one listing rather than a listing plus sort. The unbounded case is an inherited pre-retention backlog, which is bounded once by the first post-upgrade crash and cheap from then on. Android made the same call, and keeping the two in step matters more here than shaving the first-crash cost.
…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>
`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 <cursoragent@cursor.com>
|
I found a few issues worth addressing before merge:
|
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 <cursoragent@cursor.com>
|
Addressed in 1e9949f:
Covered by new cases in |
|
|
|
Both points were right, and the second one is worse than you framed it. Fix is up as OneSignal-KMP-SDK#23. This PR should not merge until that lands and is adopted here — as it stands it ships the data loss. The unknown-mtime case destroys live crash reportsNot just "potential." With try writeRecord(named: "opaque.otlp", ageMillis: 60_000)
...
XCTAssertFalse(fileExists("opaque.otlp"))So a crash captured before a reboot is destroyed on the first post-reboot launch instead of uploaded, and my test pinned that as correct. My reasoning error was justifying it as Android parity: on Android Worth keeping the other constraint in view, since it's why the naive fix is wrong: the previous behavior omitted these entries from the snapshot, which put them outside every bound at once — uncounted by the caps, unselectable by either reclaim pass, leaked for the life of the install. Any fix has to be bounded and non-destructive. The fix dates records by their own filenameRather than an "unknown" state, #23 recovers the write time from the record's name. Records are written as fun effectiveWriteTimeMs(entry: CrashDirEntry): Long? =
entry.lastModifiedMs?.takeIf { it > 0 } ?: leadingMillis(entry.name)The filesystem wins when readable, so the normal path decides exactly as before. This beats an unknown-only design in both directions: a 60-second-old record with unreadable attributes survives, and a genuinely 80-hour-old one still expires. Under unknown-only the latter would have survived until overflow pressure, quietly suspending the age ceiling — trading the data loss for a retention leak.
Verified by perturbation: restoring In-flight namesAlso fixed in #23. AndroidI had the Android path checked too, since it feeds the same policy. The deletion chain is identical and reachable in code, but not realistically triggerable in production: Android also has no test for zero or unknown mtime at all — its fixture helper always sets a real timestamp — so that gap is being closed as part of the adoption. What lands here after #23
One incidental improvement: the temp sweep loses its hand-rolled |
Moves the submodule from 6c6bb90 to 0513bf5, the head of KMP's ar/sdk-5065-unknown-mtime. Brings in CrashDirEntry.lastModifiedMs as a nullable Long, the effectiveWriteTimeMs helper that recovers a write time from the record's own name, and selectOverflowOwned taking a set of protected names (KMP #23). The pin references an unmerged commit so this branch can build and be reviewed ahead of that PR. It has to be re-pointed at the squashed merge commit once KMP #23 lands. Co-authored-by: Cursor <cursoragent@cursor.com>
directoryEntries() reported a missing contentModificationDate as 0.
selectExpiredOwned read that back as an age, so nowMs - 0 always exceeded the
retention window and the record was deleted. Data protection makes attributes
unreadable while the directory still lists, on every reboot until first
unlock, so a crash captured before a reboot was destroyed on the next launch
instead of uploaded.
The missing date is now reported as nil, and every age decision goes through
CrashRetention.effectiveWriteTimeMs, which recovers the write time from the
leading millis in the record's own name. Records this store writes are named
{millis}-{uuid}.otlp, so an unreadable timestamp costs no accuracy: a
minute-old record survives and an 80-hour-old one still expires.
A record neither source can date is withheld from readers rather than treated
as age zero. It has not been shown to clear minAgeMillis, and claiming it has
would hand a possibly half-written record to the uploader. It is not deleted
either, and cannot leak: the accumulation caps count and evict it regardless
of age.
The temp sweep drops its own lastModifiedMs > 0 sentinel for the same shared
gate. .otlp.tmp names parse, so an abandoned write is now reapable before
first unlock instead of skipped until the device is unlocked, while one still
too young is protected by its real age.
selectOverflowOwned takes a set of protected names. Both call sites pass every
in-flight name, not one. The write path matters most: unlike reclaim it
unlinks whatever the selector returns without re-checking the in-flight set,
so a sibling crashing thread's just-written record really was deleted.
Co-authored-by: Cursor <cursoragent@cursor.com>
|
Both of your points are now implemented on this branch, not just planned. Unknown-mtime data loss. Records are now dated by their own One detail worth recording, because it changes where the fix actually lives: removing the Write-path in-flight names. Fixed here rather than deferred. This one turned out to be real deletion, not cap drift. Android does not have this yet — no equivalent registry — so the two platforms currently diverge on the shared contract. Tracked as SDK-5129, scoped to Android. Still gated on KMP #23, and the re-point is not mechanical. This branch pins So when #23 merges this needs a re-pin and a full re-run, not just a pin bump. The eviction-ordering tests here are the most likely to be affected by the tier change. |
Cut narration, design rationale, history and worked examples from the crash-retention comments, keeping only the constraints a caller or maintainer must obey. The removed rationale now lives in the PR description. No behavior change: the diff is comments only. Co-authored-by: Cursor <cursoragent@cursor.com>
Three blocks sat at six lines after the previous trim. Each compresses to four without dropping an invariant: the in-flight reservation and the crash-path sort guard, the unreadable-attribute rules, and the production-shaped-name requirement for dating tests. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Id merge kmp first and update submodule to kmp main |
nan-li
left a comment
nan-li
left a comment
There was a problem hiding this comment.
I don't really have any comments that the review bots haven't already been helpful on. Maybe more like a high-level trains of thought about readability to human reviewers or a future human reader.
Also any intuition on how big is a real iOS crash record is? We've taken the android numbers but if it's a few kb, then 50 records is well under 2 MB and that cap would never trigger. If it's like 400 KB, we would keep four crashes and the count cap wouldn't take any effect.
| /// | ||
| /// 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. | ||
| /// Attributes are routinely unreadable under data protection before first unlock, so the |
There was a problem hiding this comment.
non-blocking but more of a train of thought: the last commits making comments shorter is helpful but shorter isn't necessarily more understandable or clearer to a human reader.
For tests, I would argue the cut comment lines should actually stay, because a test comment is sometimes the only place we know why something exists, or where it regressed before and how.
| /// 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. | ||
| /// Trims back inside the accumulation caps after a write, reserving every in-flight name and |
There was a problem hiding this comment.
more trains of thought as a human reviewer: this method and the comment was hard to parse. Tracing the call and asking AI for explanation is how I was able to understand what this method is for. I think a key idea is this is running in a crashing thread and needs to be as light as possible: with the overflow check only. For a human that is revisiting this method, trimAfterCrashWrite or trimOnCrashPath might be a clearer name
3 participants
Description
One Line Summary
Bounds the iOS crash-record cache (age ceiling, count and byte caps, write-size limit) using the retention policy shared from the KMP module.
Relates to SDK-5065.
Warning
Blocked on OneSignal-KMP-SDK#23, which is unmerged; the submodule is pinned to that branch head (
0513bf5). When #23 merges the pin must be re-pointed at KMPmainand the suite re-run: the shared policy changed behavior after this branch was verified, with eviction going from three tiers to four, protected names no longer claiming byte budget, and foreign-name dating tightened.Details
Motivation
FileLogStorehad no retention: no size limit onsave(), no age ceiling onlistReadable(), no count or byte cap, no eviction anywhere. A record that fails to upload is re-read and re-POSTed on every launch, indefinitely, while the directory grows unbounded. Android hit the same defect when OpenTelemetry'sdisk-bufferingwas removed, so rather than reimplement the policy here and let the two drift, it moved tocommonMainasCrashRetention. This store now contributes only the file I/O.Scope
save()listReadable()deleteUnrecognizedEntries()Non-obvious decisions:
Unreadable attributes are the ordinary Apple case. Data protection denies file attributes while the directory still lists, on every reboot until first unlock. The entry stays in the snapshot, since omitting it would put the file outside every bound at once, and a missing mtime is passed through as
nilrather than a stand-in number: the policy reads whatever it is given back as an age, so the0an earlier revision substituted made every such record look maximally stale and deleted it. Age now falls back to the leading millis in the record's own name, and a record neither source can date is withheld from readers rather than uploaded or reaped, still bounded by the caps.The write suffix is derived from the policy, not a local constant. Ownership is checked through that same
CrashRetentionPolicy, so a local suffix that drifted would hide every brand-new record from every reader.In-flight names are reserved. Concurrent crashing threads each hold a record open. The async reclaim skips in-flight names, and the write path unlinks whatever the selector returns without re-checking, so it reserves every in-flight name and not just the one it wrote.
The temp sweep stays narrower than the shared selector.
selectUnrecognizedreaps any non-owned file; iOS reaps only its own.otlp.tmp, having never run the OpenTelemetry pipeline, and files we did not write are not ours to assume about. It skips this store's in-flight temps and deletes per-entry, so one undeletable leftover cannot strand the pass.The submodule pin moves
87e87fd→0513bf5: the shared policy (KMP #20), bounded retry/backoff in the export path (#21), the logger-contract cleanup (#22), and nullable crash write times (#23).Testing
Unit testing
FileLogStoreRetentionTestsandFileLogStoreRecordDatingTestscover both caps and the age ceiling, the size limit at its exact boundary, in-flight records surviving a concurrent eviction and a racing temp sweep, and name-based dating when attributes are unreadable.Full
OneSignalOSCoresuite green against the current pin: 135 tests, 0 failures (UnitTestApp_TestPlan_Reduced, iPhone 17 Pro simulator). Perturbation-checked, both reverted: dropping the write-size guard reddens one case, and widening the store's bounds while the tests assert againstCrashRetention.shared.defaultPolicyreddens five more. The policy decisions themselves are unit-tested in the shared module.