[metascroy]

Acquires a prepared Core AI model for a selected bundle: restore it from the persisted bookmark, or materialize the source and specialize it (runtime/coreai_load_coordinator.{h,mm}).

• acquire_bookmark_model restores from the persisted bookmark; on a miss it materializes the source, specializes it with the persistent default SDK cache and publishes the new bookmark atomically.
• Restore errors fail without a source fallback. Sources and SDK pins are retained across completion.
• The backend that uses this lands in the next PR.
• Tests:
  • An acquisition-only fake SDK loader (runtime/test/coreai_fake_loader.{h,mm}) runs the coordinator in coreai_host_test without Swift or CoreAI.
  • The acquisition suite calls acquire_bookmark_model directly: cold specialization, warm restores, restore errors and misses, bookmark-save failures, same-key versus independent-key contention across processes, and a crash after the SDK call.
• README: the "Asset storage" acquisition flow.

No changes outside backends/apple/coreai.

Stack: 6 of 9, based on #23391. Review only this PR's commit. Next: #23393.

Test plan: built locally for macOS 27.0; ctest -N lists coreai_host_test and coreai_swift_bridge_test, which the Core AI workflow runs on the macOS 27 runner. lintrunner is clean.

[pytorch-bot]

🔗 Helpful Links

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

• 📄 Preview Python docs built from this PR

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

⏳ No Failures, 2 Pending

As of commit 7872a72 with merge base 61ec450 ():
💚 Looks good so far! There are no failures yet. 💚

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 41s —— View job

---

The coordinator logic looks correct, but I'd ask for changes: the specialize-error and empty-bookmark paths have no tests, the fake loader carries unused knobs, and a repeated bookmark-save failure leaks one SDK cache entry per load attempt.

Detailed review

PR Review: #23392

Summary

This PR adds acquire_bookmark_model. Under the per-key lock it restores the model from a persisted bookmark. If the bookmark is missing or the restore misses, it materializes the source, specializes it and publishes the new bookmark. I found no correctness bugs in the coordinator. The issues are test gaps, unused test scaffolding, and one design question about save failures.

Code Quality

• runtime/test/coreai_acquisition_fixture.mm:164,220: the acquisition crash child uses the flag --bookmark-crash. That falls inside the --bookmark- prefix that bookmark_child_mode claims (coreai_bookmark_fixture.mm:96). It only works because main runs acquisition_child_mode first. If the two dispatch calls in coreai_host_test.mm are ever reordered, bookmark_child_mode takes the flag, rejects it and the crash test fails in a confusing way. Renaming it to --acquisition-crash would match --acquisition-parallel. Fix this →
• runtime/test/coreai_acquisition_fixture.mm:228 and coreai_host_test.mm:16: set_acquisition_test_executable only forwards to set_bookmark_test_executable, and main calls both with argv[0]. Per CLAUDE.md, a trivial single-use helper like this should go. Moving the existing set_bookmark_test_executable(argv[0]) call above both child-mode dispatches does the same job.
• runtime/test/coreai_fake_loader.{h,mm}: nothing in this PR uses emptyBookmark, specializeError, evictError, onEvict, specializationBookmark or the evictModelWithBookmark body. The PR body calls this an "acquisition-only fake", so either test these knobs here (see Testing) or add them in the PR that needs them.

Testing

• Untested coordinator paths (runtime/coreai_load_coordinator.mm), even though the fake already has the knobs for some of them:
  • Specialization returns an SDK error (specializeError). This should give Internal and leave the bookmark untouched.
  • copyBookmarkData returns empty data (emptyBookmark). write_bookmark rejects that with InvalidExternalData (coreai_bookmarks.mm:231), and that path is never exercised.
  • The loader breaks its contract, for example a Hit with a nil model. This is the Invalid Core AI acquisition result check at coreai_load_coordinator.mm:46-49.
  • The loader == nil guard.
• Contention test is in-process only: the PR body says it tests "same-key versus independent-key contention across processes". In fact parallel_child runs two threads inside one child process (coreai_acquisition_fixture.mm:250-258). That checks that per-descriptor flock serializes threads, but nothing tests two processes specializing the same key at once. The existing bookmark lock test covers the lock primitive across processes, so this may be fine. If so, please fix the description; otherwise add a two-process variant.

Performance

• Repeated save failures leak SDK cache entries (runtime/coreai_load_coordinator.mm:90-91; needs discussion, low confidence): if write_bookmark fails after specialization, the new model is dropped and its SDK entry is orphaned. The next load has no bookmark, so it specializes again and creates another entry. CoreAIAcquisitionSaveFailureTest/Cold shows this: it expects specializations to be calls + 1. With a persistent failure such as a full disk or a read-only bookmarks directory, every load attempt adds one more untracked entry to the persistent SDK cache. The backend never reclaims these ("does not track historical or unrecorded SDK entries"). Two options:

  • Best-effort evictModelWithBookmark: on publish failure, after releasing the model's pin.
  • Return the model anyway, so the load succeeds and the next load retries publication.

  If the SDK de-duplicates specializations by source, this doesn't matter. Please confirm that either way.

Recommendation

Request Changes

Nothing here is a correctness bug in the coordinator. Before merging I'd ask for the missing error-path tests (specialize error, empty bookmark), the unused fake knobs to be removed or tested, and a decision on the save-failure leak.

[metascroy]

@claude review this code

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

Context on the previous findings: a fresh UUID from the fake does not establish that the SDK leaks a distinct entry on every failed bookmark save. A real-SDK experiment found that evicting the first specialization's bookmark invalidated subsequent bookmarks too, consistent with cache reuse, but not a universal guarantee. We intentionally return save errors without automatic retry or blind eviction; a post-rename flush failure can already leave a published bookmark.

This revision also removes staged sources only after durable bookmark publication. Cleanup errors are returned while the bookmark remains available, without automatic eviction or retries. Real SDK regression tests on the tested macOS 27 system cover portable and AOT weighted models, live inference after source removal, fresh-process bookmark restoration, and misses after explicit eviction. This is empirical evidence, not a promise covering reboots, OS upgrades, or all devices. The missing acquisition failure-path tests and test-fixture issues were addressed.

[claude]

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

---

The earlier findings are fixed, but I'd hold this until one new problem is settled. If staging cleanup fails partway and the SDK later misses, that model can't be loaded again, and nothing in the runtime can clear the damaged entry.

Detailed review

PR Review: #23392

Summary

This PR adds acquire_bookmark_model. It restores a model from a persisted bookmark, or materializes and specializes the source, publishes a new bookmark, and then removes the staged source. I'm treating the save-error, no-retry and no-eviction behaviour you described as settled. That leaves one problem that blocks merging: the recovery path after a partial cleanup failure (below). The rest are smaller issues in tests, docs and test infrastructure.

Code Quality

• A partial cleanup failure followed by an SDK miss leaves the key permanently unloadable (runtime/coreai_load_coordinator.mm:73-77,92, runtime/coreai_assets.mm:452-456). All three reviewers found this.
  • What breaks:
    1. remove_bookmark_staging deletes entries depth-first. If it fails partway (for example CleanupPartialRemoveFailurePreservesDurableBookmark, which injects a Remove fault at match_index = 2), staging/<key> is left half-deleted.
    2. Warm hits return before looking at staging, so the damaged entry is never revisited.
    3. When the SDK later evicts its entry and the restore misses, prepare_source_bundle sees EntryState damaged and returns InvalidExternalData ("Cannot replace a damaged Core AI stored bundle") on every load. This happens even though the packaged named data could rebuild the source.
  • Docs: the README says this lasts "until it is explicitly cleared", but this PR adds no way to clear it. remove_bookmark_staging's only caller is the success path. The application would have to rm files inside the assets root itself.
  • Stale rationale: the reason given at coreai_assets.mm:452 ("Recovery sources may still be in use by a previously acquired SDK model") no longer holds. This PR removes sources under the same lock right after publication, and the Swift regression test shows that persistent models don't need the source.
  • Same root cause, smaller cost: any cleanup failure, or a crash between write_bookmark and remove_bookmark_staging, leaves a full extracted bundle (possibly GBs) in staging/<key> for as long as hits continue. The tests lock that state in with EXPECT_TRUE(staging_key_exists()) after warm restores.
  • Possible fixes: either one fixes both consequences without adding retries or eviction:
    • While holding the lock, treat a damaged keyed entry as removable on the cold path and re-materialize it.
    • Do a best-effort remove_bookmark_staging on a hit, while the lock is already held.
  • Fix this →
• Cleanup failure after durable publication fails a load that has a usable model (coreai_load_coordinator.mm:92; needs discussion). The model is specialized and the bookmark is durable, yet a staging cleanup error makes the function return AccessFailed and drop the model. Callers can't tell this case apart from a real publication failure.
  • If returning the error is intentional, the header comment at coreai_load_coordinator.h:15 should say that an error can mean "published, but cleanup failed".
  • Otherwise, log the cleanup error and return the model.
• The README and PR body describe behaviour this PR doesn't have:
  • The README's new paragraph says a load "then binds the requested function and ordered I/O on that same acquired model". The Asset storage section says the model "is retained through function binding". Nothing in this PR calls acquire_bookmark_model, and there is no binding until [CoreAI] Integrate the delegate execute path #23393.
  • The PR body still says "Sources and SDK pins are retained across completion", but 61e2774 removes sources.

Infrastructure

• pytest collects the ctest driver (runtime/test/test_sdk_source_lifetime.py). coreai.yml:83 runs python -m pytest backends/apple/coreai, and the test_*.py name means pytest imports this file and finds zero tests. It actually runs through CTest only. A name like run_sdk_source_lifetime.py would avoid the confusion.
• Possible SDK cache leak on CI runners (test_sdk_source_lifetime.py:52-56; low confidence). Eviction only runs if bookmark.is_file(). The bookmark is written just after AIModel.specialize returns (SDKSourceLifetimeTest.swift:87-88). If the probe persists the entry but then fails or hits the 60 s timeout before that write, the entry stays in the default persistent cache. The model contents are random per run, so leaked entries won't dedupe on reused macOS runners. Also, a RuntimeError from evict inside finally replaces the original failure.

Testing

• Partial cleanup followed by an SDK miss is untested. The cleanup-failure tests only follow up with a restore hit (coreai_acquisition_test.mm:387-446). The README's statement about the miss case is never checked.
• The real-SDK regression test bypasses the bridge and the coordinator (SDKSourceLifetimeTest.swift:86-100). It calls AIModel.specialize and AIModel(resolvingBookmark:) directly, with options copied by hand from ETCoreAIModel.swift:398-408. So it checks the SDK assumption, not the backend's path:
  • If the bridge's specialize options change, nothing catches it.
  • The order "publish, then remove the source, while the model stays live" is only tested against the fake loader.
  • Driving it through CoreAISwiftPreparedModel and the bridge loader would close that gap.
• The BeforeSDK fault point is never exercised (coreai_load_coordinator.mm:24). The crash child returns 4 for any boundary other than AfterSDK (coreai_acquisition_fixture.mm:237), so the boundary parameter of spawn_acquisition_crash has only one usable value. Either test BeforeSDK or drop the parameter.

Recommendation

Request Changes

The partial-cleanup lockout is the only blocker. It makes a model permanently unloadable after one transient filesystem error plus a routine SDK cache miss, and the documented workaround ("explicitly cleared") has no API behind it. The remaining items are minor.