Skip to content

Fix crashes and unbounded memory in disk cache trimming - #336

Merged
andyfinnell merged 2 commits into
pinterest:masterfrom
dreed1:fix/nil-safe-bounded-trim
Jul 29, 2026
Merged

andyfinnell merged 2 commits into
pinterest:masterfrom
dreed1:fix/nil-safe-bounded-trim

Conversation

@dreed1

@dreed1 dreed1 commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

trimToSize:, trimToSizeByEvictionStrategy:, and trimToDate: sorted the entire metadata dictionary with keysSortedByValueUsingComparator:, which materializes O(N) temporary storage inside CoreFoundation. On large caches under memory pressure that allocation fails and traps in __CFCreateArrayStorage — and since every subsequent trim hits the same wall, a cache that grows too big to sort can never shrink again.

  • Select eviction candidates in bounded 256-key batches (single pass, binary-search insertion, O(batch) memory) and loop until under the byte target, so trims always make progress. An attempted-keys set guarantees termination when eviction declines ghost entries.
  • trimToDate: is a pure threshold filter; drop its sort entirely.
  • Nil-safe comparators: metadata dates/size are nil when resourceValuesForKeys: fails during the initial disk scan. [NSNumber compare:nil] RAISES NSInvalidArgumentException (a real crash in trimToSize:), and [NSDate compare:nil] returns NSOrderedSame, which made the sorts weakly inconsistent. Unknown-metadata entries now sort first and are evicted before entries with known access times.
  • Guard NSUInteger underflow in the trim loops: stale metadata sizes overshooting byteCount made the break condition unsatisfiable and evicted the entire cache.

Adds a multi-batch trim regression test (300 entries > one batch).

Dan Reed and others added 2 commits July 28, 2026 16:28
trimToSize:, trimToSizeByEvictionStrategy:, and trimToDate: sorted the
entire metadata dictionary with keysSortedByValueUsingComparator:, which
materializes O(N) temporary storage inside CoreFoundation. On large
caches under memory pressure that allocation fails and traps in
__CFCreateArrayStorage — and since every subsequent trim hits the same
wall, a cache that grows too big to sort can never shrink again.

- Select eviction candidates in bounded 256-key batches (single pass,
  binary-search insertion, O(batch) memory) and loop until under the
  byte target, so trims always make progress. An attempted-keys set
  guarantees termination when eviction declines ghost entries.
- trimToDate: is a pure threshold filter; drop its sort entirely.
- Nil-safe comparators: metadata dates/size are nil when
  resourceValuesForKeys: fails during the initial disk scan.
  [NSNumber compare:nil] RAISES NSInvalidArgumentException (a real crash
  in trimToSize:), and [NSDate compare:nil] returns NSOrderedSame, which
  made the sorts weakly inconsistent. Unknown-metadata entries now sort
  first and are evicted before entries with known access times.
- Guard NSUInteger underflow in the trim loops: stale metadata sizes
  overshooting byteCount made the break condition unsatisfiable and
  evicted the entire cache.

Adds a multi-batch trim regression test (300 entries > one batch).
CI was failing at the first xcrun/xcodebuild invocation with
"xcrun: error: missing DEVELOPER_DIR path: /Applications/Xcode_15.2.app".
Root cause: GitHub's macos-latest label now resolves to the macOS 26
runner image, which only ships Xcode 26.x — the pinned Xcode 15.2 (and
several other things the build relied on) no longer exist on the image.

Changes, following TextureGroup/Texture's CI setup:

- DEVELOPER_DIR now points at Xcode_26.5.0.app (the image default;
  see the runner-images manifest linked in the workflow comment).
- Makefile simulator destination bumped from "iPhone 15" to
  "iPhone 17", since the iPhone 15 simulator does not exist under
  Xcode 26 on the image.
- xcpretty replaced with xcbeautify: the runner images stopped
  preinstalling the xcpretty gem (macOS 26 ships only xcbeautify
  3.2.1), so the "xcodebuild | xcpretty" pipes would fail with
  command-not-found. Error propagation is unchanged — the Makefile's
  "SHELL=/bin/bash -o pipefail" still surfaces xcodebuild failures
  through the pipe.
- Removed the platform matrix from the test job; nothing ever
  referenced matrix.platform (the Makefile owns the destination).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

@andyfinnell andyfinnell left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It'd be nice if we could put the cap behind an experiment to determine what performance impact it has, but probably not worth the effort.

Comment thread Source/PINDiskCache.m
Comment thread Source/PINDiskCache.m
@andyfinnell
andyfinnell merged commit 0073330 into pinterest:master Jul 29, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants