[metascroy]

Persists the raw SDK bookmark for a selected Core AI bundle, so later loads can restore a specialized model without re-reading the payload (runtime/coreai_bookmarks.{h,mm}).

• Store raw opaque bookmarks (at most 8 MiB) with atomic publication.
• Keys include the bundle digest, platform, architecture and persistence policy, but not function bindings or delivery location.
• Per-key OS locks coordinate threads and processes; independent keys proceed in parallel.
• SDK acquisition and cache removal are not included here.
• Tests: the bookmark suite in coreai_host_test, including a cross-process lock test that spawns child processes.
• README: the "Asset storage" key derivation, bookmark bound and locking text.

No changes outside backends/apple/coreai.

Stack: 3 of 9, based on #23388. Review only this PR's commit. Next: #23390.

Test plan: built locally for macOS 27.0; ctest -N lists coreai_host_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/23389

• 📄 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

As of commit d2e0ec4 with merge base 61ec450 ():

NEW FAILURE - The following job has failed:

• Apple / build-demo-ios / macos-job (gh)
  RuntimeError: Command bash /Users/runner/work/_temp/exec_script failed with exit code 65

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

---

Summary: The bookmark storage itself looks correct, but this PR adds several functions nothing calls or tests yet (including the whole create=false path), plus a little duplicated test and validation code. Recommendation: Needs Discussion.

Detailed review

PR Review: #23389

Summary

This PR adds raw bookmark persistence for Core AI: a key built from a hash of the manifest, platform and architecture, a per-key flock lock, a bounded read, and an atomic write. I found no correctness bugs in the read, write or lock paths. The problems below are untested code, unused API surface and some duplication.

Testing

• The create=false path has no tests and no callers. In coreai_bookmarks.mm:164-198, lock_bookmark(..., create=false) handles a missing root, a missing entry (entry_exists), and an entry that exists only in staging (where bookmarks_ stays -1). These are the subtlest branches in the file, and no test runs any of them. resolve_bookmark_root, prepare_bookmark_staging and BookmarkLock::root_path() are also never called or tested. Either add tests for these, or move them to the PR in the stack that first uses them.
• The read checks in the header comment are untested. coreai_bookmarks.h:58-59 says empty, inaccessible and non-regular files fail closed. Only the oversized case is tested (RejectsOversizedStoredBookmark). Cheap tests would cover: a 0-byte file, a symlink at bookmarks/<key>.bookmark (O_NOFOLLOW), a directory, and a FIFO (O_NONBLOCK must not hang).
• Thread locking is untested. The README says the locks coordinate threads as well as processes. Only the cross-process case is tested. Two threads locking the same key in one process would show that separate open calls get separate flock locks.

API Design

• resolve_bookmark_root adds nothing. At coreai_bookmarks.mm:148, the public resolve_bookmark_root only forwards to the anonymous resolve_root. Use one function. Also, in resolve_root with create=false, the first stat is repeated inside prepare_storage_root(path, false). The pre-check exists only to let ENOENT through, so a single stat would do.
• prepare_bookmark_staging returns a bare path (coreai_bookmarks.mm:239-244). It opens the staging directory descriptor and closes it straight away, then returns root_path_ + "/staging". Each caller must then append the key (per the header comment). That means the layout rule from the README ("each key determines … a bundle directory in staging") lives in the caller, and the O_NOFOLLOW descriptor checks used everywhere else are lost. Consider returning staging/<key> directly. (Low confidence: the next PR in the stack may need the parent.)

Code Quality

• BookmarkDirectory copies TestDirectory. In test/coreai_filesystem_fixture.{h,mm}, BookmarkDirectory is the same as TestDirectory apart from the prefix, a parent parameter nobody passes, and no URLByResolvingSymlinksInPath. Reuse TestDirectory. If the symlink difference matters, add a parameter and say why.
• component() copies storage_component(). component() in coreai_bookmarks.mm:32 repeats storage_component() from coreai_storage.mm:123, adding only the \\ and length checks. Expose and reuse the storage helper so the two rules can't drift apart.
• openat in read_bookmark isn't retried (coreai_bookmarks.mm:208). Every other open/openat in this file goes through retry_eintr. Also, <cstring> is included but not used.

Recommendation

Needs Discussion. Nothing blocks on correctness. The main question is whether the unused create=false, staging and root functions belong in this PR. If they stay, they need tests.

[metascroy]

@claude review this code

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

Context on the previous findings: the non-creating lookup (create=false) and root/staging APIs have real downstream consumers in cache clearing and model acquisition, so we retained and tested them rather than removing them as unused in this isolated PR. Returning the staging parent is intentional: source preparation owns per-key directory creation and atomic publication. Changing that return value would change the ownership boundary. Bookmark-component validation is deliberately stricter than generic storage-component validation; deduplication must preserve that distinction. The interrupted-open handling and missing coverage were addressed.

[claude]

Claude finished @metascroy's task in 4m 11s —— View job

---

Summary: The fixes from the last review (EINTR retries, create=false and staging tests, the read-rejection tests) are in. What's left: a create=false lock can't be written through, every descriptor is opened before the lock is taken, and both contention tests can pass without ever seeing a blocked lock. Recommendation: Approve, with these as follow-ups before downstream PRs build on this one.

Detailed review

PR Review: #23389

Summary

This PR stores raw Core AI bookmarks under per-key flock locks and publishes them atomically. I accepted the design decisions from your reply as given: keeping the create=false, root and staging APIs, returning the staging parent, and the stricter component validation. The issues below are what remains. I did not build or run anything (this runner is Linux); every finding comes from reading the code at 65217f5.

Thread Safety

• The lock is taken after every descriptor has been opened (coreai_bookmarks.mm:177-197). lock_bookmark does the following before it blocks in flock(LOCK_EX):

  • opens the root,
  • runs entry_exists (when create=false),
  • opens bookmarks/.

  After the lock is acquired, nothing checks that this state is still valid. The README says these locks coordinate eviction (cache clearing in a later PR), so a waiter can wake up with stale state:

  • A writer queued behind an eviction that removed or recreated bookmarks/ publishes into an unlinked directory. The bookmark is lost, which is the "orphans an SDK entry" case the README warns about.
  • A create=false caller queued behind an eviction can get a non-null lock for an entry that no longer exists.

  It is cheap to fix here: take the lock first (the locks/ directory is never removed), then open bookmarks/ and run the existence check. Low confidence on impact, because it depends on what the eviction PR deletes, but this ordering forces every later caller to know about the window.

API Design

• A create=false lock that has no bookmarks/ directory cannot be written through (coreai_bookmarks.mm:182, 239). This is the staging-only case (NoncreatingLookupLocksStagingOnlyEntry), where bookmarks_ is -1.

  • read_bookmark handles that state and returns nil.
  • write_bookmark passes -1 to publish_storage_data, which fails at dup(-1) with the misleading error "Cannot open storage parent" (coreai_storage.mm:145-147).

  Either create bookmarks/ lazily in write_bookmark, or reject the write with a clear error and document in the header that non-creating locks can't write. Add a test for whichever you choose.

• The "non-creating" lookup still writes to disk (coreai_bookmarks.mm:185). Whenever the entry exists, open_child(..., @"locks", true) creates locks/ (and syncs the root) plus the lock file. The header (coreai_bookmarks.h:51-52) only says that missing roots and entries return null. On a read-only or permission-restricted root that has a staged or bookmarked entry, maintenance gets AccessFailed instead of a lock. Document this, or accept it explicitly.

Testing

• Both contention tests can pass without ever exercising the lock.

  • Process test: the child sends 'B' before it calls lock_bookmark (coreai_bookmark_fixture.mm:120-121). Before flock, lock_bookmark still has to resolve the root, set backup exclusion, run mkdirat and fsync. expect_blocked then watches for only 100 ms (kContentionObservation). On a slow runner the child may not have reached flock yet, so the test passes even if the lock is broken.
  • Thread test: same problem. started.set_value() runs before acquire(key), and the check waits 100 ms (coreai_bookmark_test.mm:384-392).
  • Hang risk: if the lock never releases, waiter.get() and the std::async future destructors block forever, so the test hangs instead of failing.

  Fixing the false pass needs a signal sent from inside the lock path, for example a test-only hook just before flock, or proof that the waiter is actually sleeping in flock. A longer observation window only makes the false pass less likely. For the hang, use wait_for with a timeout before calling get().

• The child process builds its manifest by hand (coreai_bookmark_fixture.mm:113-118) rather than calling bookmark_manifest(), which has no gtest dependency. If manifest_dict() changes, the child's "same" key quietly stops matching the parent's. The test then fails at stat(lock_url()) or at the inode comparison, with no hint as to why. Start from bookmark_manifest() and only swap the digest for other.

• Exit code 7 means two things (coreai_bookmark_fixture.mm:120, 122): failing to send 'B', and lock_bookmark failing. Use distinct codes so a CI failure shows which one happened.

Code Quality

• using BookmarkDirectory = TestDirectory; (coreai_filesystem_fixture.h:24) only renames the type, and coreai_bookmark_test.mm uses both names. Use TestDirectory everywhere.
• The README's key description is incomplete (README.md:60-62). The key also includes the AOT/source mode (coreai_bookmarks.mm:58), but the README doesn't say so.
• The encoded.length <= UINT32_MAX check can never fail (coreai_bookmarks.mm:65): every field is capped at 4096 characters.

Recommendation

Approve. Nothing here is a correctness bug in the current code paths. Two things should be fixed before downstream PRs build on this:

• the write-through-create=false behaviour,
• the open-before-lock ordering.

The contention tests also need a real "blocked in flock" signal, so they can fail when the lock is broken.