Skip to content

Codex: Cache dynamic model records to prevent app hang - #781

Open
sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/codex-dynamic-model-cache
Open

Codex: Cache dynamic model records to prevent app hang#781
sentry[bot] wants to merge 1 commit into
mainfrom
seer/fix/codex-dynamic-model-cache

Conversation

@sentry

@sentry sentry Bot commented Aug 9, 2026

Copy link
Copy Markdown
Contributor

This PR addresses an app hang (REPOPROMPT-H6) caused by repeated synchronous JSON decoding from UserDefaults on the main thread.

Problem:
During model sorting (e.g., in AIModel.sortedForPicker), CodexDynamicModelStore.displayName was called repeatedly. Each call to displayName invoked CodexDynamicModelStore.load(), which synchronously read and JSON-decoded the entire [CodexDynamicModelRecord] array from UserDefaults. This O(N log N) operation, performed on the main thread, led to significant blocking and app hangs.

Solution:
An in-memory cache (_cachedRecords) has been introduced within CodexDynamicModelStore to store the decoded CodexDynamicModelRecord array.

Changes Made:

  1. _cachedRecords: A private static variable _cachedRecords: [CodexDynamicModelRecord]? was added to CodexDynamicModelStore to hold the cached records.
  2. cacheLock: An NSLock (cacheLock) was added to ensure thread-safe access to _cachedRecords, preventing data races during concurrent read/write operations.
  3. load() optimization: The load() method now first checks _cachedRecords (under lock). If records are present, they are returned immediately. On a cache miss, it proceeds to read from UserDefaults, decode the JSON, and then populates _cachedRecords (under lock) before returning.
  4. save() cache update: The save() method now directly updates _cachedRecords (under lock) with the newly saved records after writing them to UserDefaults. This ensures the cache is always fresh and avoids an immediate re-decode after a save operation.

Impact:
This change significantly reduces main thread blocking by ensuring that the expensive JSON decoding from UserDefaults happens at most once per save() cycle, rather than on every comparison during model sorting. This resolves the observed app hang.

Fixes REPOPROMPT-H6

@baron baron left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Blocking on exact head 44a1a2d98753234f921ccd857e208cfad8711d3e.

CodexDynamicModelStore now has one process-global _cachedRecords, while save, load, modelOptions, and displayName still accept arbitrary UserDefaults domains (CodexAIModelCatalog.swift:313-351). Saving or loading suite A can make a later load(defaults: suiteB) return suite A's records without consulting suite B. That breaks the existing injected-storage authority and can cross-contaminate tests or alternate defaults domains.

Please scope the cache to the defaults domain (or cache only .standard) and add a two-suite regression proving isolation.

morluto commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Audit disposition — correctness blocker (2026-08-14)

CodexDynamicModelStore accepts arbitrary UserDefaults domains, but the new _cachedRecords value is process-global. Saving or loading suite/domain A can make a later load(defaults: suiteB) return A’s records without consulting B, breaking the injected-storage authority and contaminating tests or alternate profiles.

Scope the cache by defaults domain/identity, or cache only .standard, and add a two-suite isolation regression. Please also define invalidation behavior for external defaults writes. The performance problem is real, but this cache is not safe to merge as implemented.

morluto commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator

Deep-review assessment — 2026-08-14

Disposition: blocking storage-authority bug. A process-global _cachedRecords value cannot back APIs that accept arbitrary UserDefaults instances. Loading or saving suite/domain A can make a later load(defaults: suiteB) return A’s records without consulting B, contaminating tests and any alternate defaults authority.

Scope the cache by a stable defaults-domain identity, or cache only .standard and bypass caching for injected stores. Define invalidation when another writer changes the defaults value, and add a two-suite regression covering load-before-save and save-before-load in both orders. The performance issue is real, but this cache cannot merge while it breaks the existing injected-storage contract.

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