[metascroy]

Adds backend-scoped cache clearing with a shared identity for loading, specialization, and deletion.

• clear_cache(coreai_assets_dir) clears every discoverable keyed entry in an explicit backend assets root. It is not an SDK-wide cache wipe.
• clear_cache_for_pte(loader, dir) and clear_cache_for_pte(pte_path, dir) inspect all Core AI delegate manifests and bundle digests before deleting anything. They clear every matching stored device and specialization variant in that root without constructing a Program/Method, materializing assets, or querying the current device or specialization policy. A null directory selects the backend default root.
• Composite keys contain a model hash, readable device component, and specialization hash. The model prefix permits all-variant matching without attribution sidecars. Key construction, validation, and parsing share one module.
• An immutable loader-owned snapshot supplies both SDK specialization options and their stable descriptor. Default options retain the existing default specialization hash; non-default descriptors use versioned deterministic encoding. The coordinator reads the descriptor once before lookup.
• Inventory includes both bookmark files and staged entries, so interrupted source preparation remains discoverable. Older single-hash entries are not discovered by the new inventory.
• Per key, clearing evicts the SDK entry, removes staging, then removes the bookmark. Errors preserve retryable state. Keys are processed in sorted order; missing entries are no-ops. Lock stripes remain bounded, and collisions intentionally serialize.
• Specialization uses the default SDK cache with storage-pressure-only purging. Deleting staged source does not make the entry eligible for purging, while storage pressure can reclaim orphaned artifacts. Restoring an existing persistent entry does not update its retention policy.
• Callers must unload affected models and stop concurrent loads, inference, and maintenance until clearing returns. Per-key locks are not a whole-root barrier. Unkeyed temporaries, lock files, and unrelated files are not removed.

Stack: 9 of 9, based on #23394. Review relative to that base.

Test plan:

• All five native CTest targets passed: host regressions, PTE export, Swift bridge, real-SDK source lifetime, and runtime smoke.
• Regressions cover specialization isolation, descriptor stability, policy flags, all-variant clearing, staging-only cleanup, filename limits, and retry behavior. Real-SDK tests cover restoration after staged-source deletion.
• Complete CoreAI Python suite: 124 passed.
• Lint, iOS syntax checks, and whitespace checks passed. The standalone stress runner rebuilt successfully; it was not rerun for this update.

[pytorch-bot]

🔗 Helpful Links

🧪 See artifacts and rendered test results at hud.pytorch.org/pr/pytorch/executorch/23395

• 📄 Preview Python docs built from this PR

Note: Links to docs will display an error until the docs builds have been completed.

❌ 1 New Failure, 1 Unclassified Failure

As of commit 5320ce4 with merge base 61ec450 ():

NEW FAILURE - The following job has failed:

• pull / test-pybindings-no-aten / linux-job (gh)
  RuntimeError: Command bash /__w/_temp/exec_script failed with exit code 1

UNCLASSIFIED FAILURE - DrCI could not classify the following job because the workflow did not run on the merge base. The failure may be pre-existing on trunk or introduced by this PR:

• Core AI / test-coreai / test-coreai (gh) (this job did not run on the merge base, so DrCI cannot tell whether the failure is pre-existing)
  backends/apple/coreai/runtime/test/coreai_cache_test.mm:677:72: error: code should be clang-formatted [-Wclang-format-violations]

This comment was automatically generated by Dr. CI and updates every 15 minutes.

[metascroy]

@claude review this PR

[claude]

Claude finished @metascroy's task in 2m 32s —— View job

---

This PR adds root-wide and per-PTE Core AI cache clearing; it looks correct, but I'd fix four design issues before merge: the cache key is computed by a copy of the load-path logic, eviction is tested through a function only the tests call, clear_cache has no way to clear the default root, and the README describes private internals.

Detailed review

PR Review: #23395

Summary

The PR adds clear_cache and two clear_cache_for_pte overloads, which remove SDK entries, keyed staging and bookmarks. It also adds the supporting storage, bookmark and coordinator code. The eviction ordering and retry behavior look sound. The problems are in how the code is structured, how it's tested, and how it's documented.

API Design

• clear_cache can't clear the default root (runtime/coreai_cache.h:30, coreai_cache.mm:86). clear_cache_for_pte treats nullptr as the backend's default root, but clear_cache(nullptr) returns InvalidArgument. Apps that never set coreai_assets_dir are the most common case. To wipe everything, they would have to rebuild the NSCachesDirectory/executorch_coreai path themselves, which ties them to a private layout. The default root belongs to the backend, so clear_cache(nullptr) (or clear_cache() with a default argument) should resolve it through default_coreai_assets_root(), the same way the PTE overloads do. If refusing null was meant as a safety guard, please say so in the header.
• Platform or OS mismatch returns an error instead of doing nothing (coreai_cache.mm:66-76). On a host where a PTE could never load, clear_cache_for_pte fails with DelegateInvalidCompatibility. That happens for an AOT delegate built for the other platform, or one whose minimum_version is newer than this OS. Since all manifests are validated up front, one such delegate also blocks clearing the PTE's other delegates. No cache entry can exist in these cases, so returning Ok for them seems like the better maintenance behavior. Low priority; please confirm which behavior you intend.

Code Quality

• Key derivation is a copy of the load path (coreai_cache.mm:48-80 vs coreai_backend.mm:274-290). The TARGET_OS_* platform block, the minimum_version check, ETCoreAIDeviceArchitectureName(), select_assets and bookmark_key are all repeated. If the two copies ever drift (a new key field, different platform naming, a key that depends on options), clear_cache_for_pte will compute a key that doesn't exist and return Ok without removing anything. That failure is silent and no test would catch it. Pull out one helper, for example in coreai_assets or coreai_load_coordinator, that maps a parsed manifest to (selected, platform, architecture) or straight to the bookmark key, and call it from both init and clear_pte. Fix this →
• The README section reads like an implementation note (README.md:189-219). It names private symbols (inspect_coreai_pte, CoreAIPteData, clear_keys, "verified program image and selected processed buffers"), and coreai_cache.h:33-46 repeats some of this. Users only need to know what gets removed, what is kept, the quiescence requirement, and that failed clears can be retried. Please cut the internal lifetime and reader details. There's also a broken line wrap at README.md:196-197 ("root-wide clearing remains\navailable").
• remove_storage_staging (coreai_storage.mm:302-308) checks the key with its own copy of digest() from coreai_bookmarks.mm. Its only caller passes lock.key_, which lock_bookmark has already checked. Either remove the duplicate or share one helper.
• clear_cache_for_pte(DataLoader&, ...) (coreai_cache.mm:96-109) has two branches that both end in resolve_bookmark_root(..., false) and clear_pte. Choose the root string first and resolve it once.

Testing

• The eviction tests exercise a function production never calls (coreai_load_coordinator.mm:119-130). Nothing in the runtime calls evict_bookmark_model; only coreai_eviction_test.mm does. It also behaves differently from clear_bookmark_assets, which is what the public APIs use:

  • it calls lock_bookmark with create=true, which creates the root, bookmarks/ and locks/ directories when they're missing;
  • it never removes staging.

  As a result, the busy/unknown-error retry, prepared-pin and lock-contention tests (coreai_eviction_test.mm:208-345) cover this side path, not the real clear path. Point those tests at clear_bookmark_assets(root, key, fake.loader), which already accepts a loader, and delete evict_bookmark_model along with its header declaration. Fix this →

Recommendation

Request Changes

The main blocker is the copied key derivation. If it drifts from the load path, clearing will silently do nothing. Moving the eviction tests onto clear_bookmark_assets and deciding how clear_cache handles a null root should land in the same pass. The rest are small cleanups.

[metascroy]

@claude review this code

Updated head: 5b46516cd72fb1e4f443349b5b4b5b637c67dd21. The stack and parent PR have been refreshed; please review against the current base.

Context on the previous findings: the actual key hash already used the same bookmark_key function; duplicated environment/asset selection was a future-drift risk, not an established current mismatch. Selection is now shared, with end-to-end tests that load portable/AOT models and clear their entries through the public PTE API. Eviction tests now exercise production clearing, the test-only helper is removed, default-root clearing is supported with isolated tests, and public documentation is shorter.

Two intentional contracts remain: (1) incompatible delegates fail before any entry is cleared rather than being silently skipped. Incompatibility does not prove cache absence, and minimum OS is not part of the cache key; success after skipping could conceal incomplete clearing. (2) key validation remains at the destructive storage boundary and has direct invalid-key tests. Please evaluate those contracts explicitly rather than treating their removal as a required cleanup.

[claude]

Claude finished @metascroy's task in 3m 50s —— View job

---

Head 5b46516 resolves the earlier findings, but clear_bookmark_assets still has no way to clear an entry it can't read or evict, and contract (1) holds for minimum OS but not for platform/architecture mismatches or a host without the Core AI SDK.

Detailed review

PR Review: #23395

Summary

The PR adds clear_cache and two clear_cache_for_pte overloads. They remove the SDK entry, keyed staging and the bookmark for an assets root or for the delegates in one PTE. Selection is now shared with the load path, the eviction tests now run through clear_bookmark_assets, and clear_cache(nullptr) works. Three reviewers went through the diff independently and I re-checked each finding below against the code. The two remaining problems are recovery from a damaged entry and parts of contract (1). Nobody read the test bodies in coreai_cache_test.mm line by line.

API Design

• A damaged entry can't be cleared by the API meant to recover it (runtime/coreai_load_coordinator.mm:154-157). All three reviewers found this. There are two triggers:
  • Bad backend-owned files. clear_bookmark_assets returns as soon as read_bookmark fails. read_bookmark returns InvalidExternalData when bookmarks/<key>.bookmark is empty, larger than kBookmarkByteLimit, or not a regular file (coreai_bookmarks.mm:242-246). The non-destructive staging check on the next line fails the same way when staging/<key> holds a symlink or another non-file entry. In both cases neither staging nor the bookmark is removed. acquire_bookmark_model (:125-126) fails on the same file, so the key stops loading and stops clearing. The only fix is deleting files by hand, which the README tells users not to do while anything is running.
    • These bytes belong to the backend, and an unreadable bookmark can't be passed to the SDK anyway. Skipping eviction for it is the same outcome the README already accepts for lost bookmarks.
    • Fix: when read_bookmark returns InvalidExternalData, skip eviction and remove staging and the bookmark. Unlinking symlinks and odd entries under staging/<key> without following them is safe too. Add a test that clears a 0-byte bookmark. Fix this →
  • Permanent SDK rejection (:73-79, suspected; it depends on SDK behavior). Suppose both evictModelWithBookmark and the probe restore return errors for a bookmark the SDK can no longer decode, for example after an OS update. Then evict_one keeps the files on every retry, and loads keep failing on the same restore error. Keeping files after a transient busy error is right. A deterministic error needs an escape hatch, or at least a documented recovery step: this API offers no force option, and the README tells users not to delete files by hand while anything is running. Please document which SDK errors you treat as retryable.
• clear_cache_for_pte fails when the SDK is unavailable, even on an empty or missing root (coreai_cache.mm:66-67, through select_runtime_assets at coreai_load_coordinator.mm:94-97). Two reviewers found this. Selection requires ETCoreAIIsAvailable() before the root is examined. On a pre-27 OS, any PTE with a Core AI delegate therefore returns NotSupported, which contradicts "missing roots/entries are no-ops". clear_cache removes staging-only entries without the SDK (RemovesStagingOnlyEntriesWithoutSdkAvailability), so the two APIs are inconsistent. Your incompatibility argument doesn't cover this case: without the SDK, no bookmark could have been written on this OS.
  • Fix: return Ok early when the root doesn't exist. For the SDK check, compute keys without requiring the SDK: platform and architecture come from the manifest and the build. Only the eviction step needs a loader, and clear_bookmark_assets already checks for it lazily.

Evaluation of the intentional contracts

(1) An incompatible delegate fails the whole PTE clear. This is sound for minimum OS but not for platform or architecture mismatches.

• Minimum OS: agree. bookmark_key (coreai_bookmarks.mm:57-61) leaves out minimum_version, so a PTE with a lower minimum OS and the same bundle digest can own that key. Skipping would report success while the entry remains.
• Platform and architecture: the stated reason doesn't hold. Both are fields of the key, and select_assets rejects the mismatch before any key exists. If this PTE's delegate has no assets for the running platform or architecture, it has no key here, so absence is provable.
• Cost of the current behavior: take a PTE with one delegate built for macOS and one built AOT for iOS. On macOS, the first delegate loads lazily and gets cached. clear_cache_for_pte then always returns DelegateInvalidCompatibility, so that entry can only be removed with root-wide clear_cache. No test covers this. RejectsInvalidLaterDelegatesBeforeMutation only checks that nothing is cleared.
• Suggestion: keep failing on parse errors, invalid manifests and minimum-OS mismatches. Skip delegates that have no assets for the running platform or architecture. Supporting that needs select_assets to return a separate "no assets for this host" result instead of DelegateInvalidCompatibility.

(2) Key validation at remove_storage_staging: agree.

• It is the destructive boundary, and the check costs almost nothing. The direct invalid-key tests are appropriate.
• On the production path, lock_bookmark already rejects non-digest keys, so this check is defense in depth, not dead code.
• The only cleanup left is optional: coreai_storage.mm:334-342 copies the hex check from digest() in coreai_bookmarks.mm:35-42. Sharing one predicate would keep the two copies from drifting apart.

Recommendation

Needs Discussion

Nothing is unsafe. Before merge, please decide how to recover entries the clear path can't read or evict (clearing an invalid bookmark is a small fix), and narrow contract (1) so that platform/architecture mismatches and an unavailable SDK don't block clearing.