[metascroy]

Adds the filesystem layer the Core AI runtime uses for its asset and bookmark cache (runtime/coreai_storage.{h,mm}, runtime/coreai_file.h).

• Prepare an absolute, application-chosen storage root. Creating a root also excludes it from backup; inspecting an existing root changes nothing.
• Enumerate directories through descriptors and return sorted names, failing on operational errors rather than returning partial listings.
• Write files relative to a directory descriptor and fsync each file and its parent directory.
• Publish data atomically: write a temporary file, fsync + F_FULLFSYNC it, renameat it into place and sync the directory. A failure before the rename keeps the old bytes and removes the temporary file.
• Trust model: the application owns the cache directory; the helpers do not defend against other processes of the same user swapping paths inside it.
• Fault injection uses storage_fault(), which compiles to a constant 0 unless COREAI_ASSETS_TESTING is defined (only in coreai_host_test).
• Tests: the storage suite in coreai_host_test.
• README: the "Asset storage" root, durability and backup-exclusion text.

No changes outside backends/apple/coreai.

Stack: 2 of 9, based on #23387. Review only this PR's commit. Next: #23389.

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/23388

• 📄 Preview Python docs built from this PR

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

❌ 1 Unclassified Failure

As of commit 8b95a2e with merge base 61ec450 ():

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_storage_test.mm:167:23: 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 1m 28s —— View job

---

This PR adds descriptor-based storage primitives for the Core AI cache. It doesn't need to block on anything, but the rename in publish_storage_data isn't flushed to the device, storage_children may wrongly reject NFC-normalized names, and write_storage_file plus several other entry points have no tests.

Detailed review

PR Review: #23388

Summary

The PR adds the fsync-based write, atomic-publish, directory-listing and backup-exclusion helpers for the Core AI asset cache, with host tests. It has one durability gap in atomic publication, one possible false rejection in directory listing, and several gaps in test coverage.

Code Quality

• coreai_storage.mm:279-291: the rename isn't fully flushed. The comment says bookmarks "get a full device flush", and the README says publication uses F_FULLFSYNC. In practice only the temp file's data gets F_FULLFSYNC. After renameat, the directory is synced with plain fsync through sync_storage_directory, and on Darwin plain fsync doesn't flush the drive cache. After a power loss, APFS keeps the update atomic, but the new name can be lost and readers fall back to the old bytes. That defeats the point of the full flush. Issue F_FULLFSYNC on root_fd after the rename, either in this path or through a full_sync flag on sync_storage_directory. If losing the new name is acceptable, change the comment and README to say only the file's bytes get a full flush. Fix this →
• coreai_storage.mm:224-231: the name round-trip check may reject valid names (low confidence). On macOS, -[NSString fileSystemRepresentation] (backed by CFStringGetFileSystemRepresentation) returns decomposed UTF-8. APFS keeps names in whatever normalization they were written with, so an entry written in NFC (for example a precomposed é) would fail strcmp here. With the default skip_invalid_names = false, that one entry makes the whole listing fail with InvalidExternalData. If the goal is only to reject invalid UTF-8, the name == nil check already covers it. Otherwise, compare against -UTF8String. Please confirm the decomposition behavior on the macOS 27 runner.
• coreai_storage.h:40-47: most of StorageOperation is unused here. BeforeSDK, AfterSDK, BeforeEvict, AfterEvict and Remove aren't used in this PR; only Rename is wired. Consider adding each value in the stack PR that first uses it so this commit can be reviewed on its own.

API Design

• coreai_storage.h:26: open_storage_shared_file returns a raw int. Every other entry point returns Error or Result, and the repo already has FileDescriptor. Callers have to know to check errno and wrap the descriptor themselves. Returning Result<int> (or a FileDescriptor) would match the rest of the header and make descriptor leaks less likely.

Testing

• Several public entry points are untested. coreai_storage_test.mm exercises prepare_storage_root, storage_children and publication only for a fault at Rename. Missing coverage:
  • write_storage_file: missing parent directories get created, the O_EXCL refusal to overwrite an existing file, and rejection of .., a//b, a leading or trailing /, and empty paths.
  • open_storage_shared_file: both the create path and the open-existing path.
  • storage_children(fd, /*skip_invalid_names=*/true).
  • A write or fsync failure before the rename. The PR body claims the old bytes are kept and the temp file is removed in that case, but write_file has no fault point, so only the rename failure is tested.

Recommendation

Approve (with minor changes)

Nothing blocks approval. The F_FULLFSYNC gap is worth fixing, or at least documenting accurately, before later PRs in the stack start relying on bookmark durability.

[metascroy]

@claude review this code

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

Context on the previous findings: retaining the raw descriptor return from open_storage_shared_file is intentional. The downstream bookmark caller immediately wraps it in FileDescriptor and checks it; returning Result<int> would not itself enforce ownership, and we did not establish a leak. Storage-operation enum values used later in the stack are not speculative dead APIs. The Unicode, publication durability, and test-coverage concerns were addressed. Full-flush/fault-injection tests verify ordering and error handling, not physical power-loss survival.

[claude]

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

---

This revision fixes the earlier durability, Unicode and coverage points, but three issues remain: a failed write_storage_file leaves a partial file that blocks retries, storage_children moves the caller's directory offset, and a path starting with ~ is accepted as absolute. Nothing blocks the stack; the first two are worth fixing before later PRs depend on them.

Detailed review

PR Review: #23388

Summary

The PR adds descriptor-based storage primitives for the Core AI cache: root preparation, backup exclusion, listing, fsync'd writes and atomic F_FULLFSYNC publication. The previous findings are resolved: the post-rename F_FULLFSYNC on root_fd, the byte-preserving UTF-8 listing, and the new tests. I left out the raw int return and the enum values, per your note. Three reviewers looked at all 9 changed files. They couldn't build anything because the runner is Linux, so all findings come from reading the code.

Thread Safety

• coreai_storage.mm:204-213: storage_children changes the caller's directory offset. All three reviewers flagged this. F_DUPFD_CLOEXEC gives a descriptor that shares the file offset with fd (the comment at :212 says so), so rewinddir and readdir move the caller's offset too. If two threads list the same root descriptor at once, each one's rewind or read moves the other's offset. That thread can then skip or repeat entries and still return Ok, which breaks the "no partial listings" guarantee. openat(fd, ".", O_RDONLY | O_DIRECTORY | O_CLOEXEC) gives a private offset, so the rewinddir workaround is no longer needed. Whether this actually happens depends on whether later PRs share the root fd across threads; if they never do, say so in the header. Fix this →

API Design

• coreai_storage.mm:167-199: a failed write_storage_file leaves a truncated file under its final name. All three reviewers flagged this. The file is created with O_CREAT | O_EXCL. If write, fsync, close or the parent sync then fails (for example ENOSPC partway through a large asset), the function returns AccessFailed but leaves the partial file behind. Two things follow:

  • A reader sees a truncated file at the final name.
  • Every retry of the same path fails with EEXIST, and that is the same AccessFailed the test at coreai_storage_test.mm:139-142 treats as "won't overwrite".

  publish_storage_data cleans up its temporary file, but this path doesn't, and the header (coreai_storage.h:27) doesn't mention it. Either unlinkat the file on failure after creation, or document that callers must write into a staging directory they discard. No test injects a fault into write_storage_file. Fix this →

• coreai_storage.mm:62: isAbsolutePath returns YES for paths starting with ~. Only one reviewer raised this, and I'm moderately confident. Nothing here expands ~. prepare_storage_root(@"~/Library/Caches/x", true) would pass the check, and createDirectoryAtPath: and stat would then most likely resolve it against the current directory. Use [path hasPrefix:@"/"] to enforce the documented "absolute path" contract, and add a ~ case to the test at coreai_storage_test.mm:48.

• coreai_storage.mm:66-90: the same bad input returns different errors depending on create. When the path is an existing regular file, create=false returns InvalidArgument (S_ISDIR check), but create=true fails earlier in createDirectoryAtPath: with AccessFailed. Callers that tell configuration errors apart from I/O errors will see two different errors for one input.

Testing

• coreai_storage_test.mm:110-112: the invalid-UTF-8 test probably always skips on CI. APFS rejects names that aren't valid UTF-8 with EILSEQ, so on the macOS 27 runner this hits GTEST_SKIP. That leaves the InvalidExternalData and skip_invalid_names branches (coreai_storage.mm:230-236) unexercised, even though a skip doesn't count as a failure. If those branches only matter for non-APFS volumes, consider testing on a disk image formatted with a filesystem that accepts such names. Otherwise, note that the branch is defensive only.
• Directory fsyncs can't be observed. sync_storage_directory, together with the pre-rename parent sync at :199 and the per-component syncs at :164, doesn't go through storage_call. So FullyFlushesPublicationBeforeAndAfterRename would still pass if any of those syncs were removed, and no test can inject a directory-sync failure.

Code Quality

• coreai_storage.mm:145: dup(root_fd) doesn't set FD_CLOEXEC. storage_children uses F_DUPFD_CLOEXEC. Use the same here so a concurrent fork/exec doesn't inherit the directory fd.
• coreai_storage.mm:286-293, 304-305: the rename failure isn't logged, and cleanup overwrites errno. A failed renameat sets AccessFailed without a log message, and the following unlinkat and fsync can then overwrite errno. As a result, EXDEV, EISDIR and ENOSPC all look the same in the field. Log errno before the cleanup runs.
• coreai_storage.mm:190-192, 297-299: there's no fallback when F_FULLFSYNC isn't supported (low confidence). The root is chosen by the application. On volumes that don't support F_FULLFSYNC (SMB, some exFAT), fcntl fails with ENOTSUP, so every publication fails, including after a rename that already succeeded. SQLite falls back to fsync in this case. If roots are always local APFS, say so in the README instead.

Recommendation

Approve (with minor changes)

Nothing blocks approval. Fix the partial-file cleanup in write_storage_file and the shared offset in storage_children before downstream PRs depend on them.