[metascroy]

Materializes the selected Core AI source bundle under the assets root so the SDK can specialize it (runtime/coreai_assets.{h,mm}).

• Read inline bundle files through NamedDataMap and write them unchanged, keyed by the export-time bundle digest (no runtime payload hashing).
• Reuse checks the exact file set, sizes, entry types and directory structure. Incomplete or malformed existing sources fail without being replaced, since an SDK model may still be using them; same-size content changes are not detected.
• Cold publication stages into .staging-<uuid> and publishes with renameatx_np(RENAME_EXCL), all relative to one open root descriptor. If another loader publishes first, its bundle is validated and used.
• SDK acquisition is not included here.
• Tests: the source suite in coreai_host_test, plus the filesystem snapshot helpers used from here on. ExclusivePublicationValidatesWinnerAndRemovesLosingStaging covers the concurrent RENAME_EXCL race.
• README: the "Asset storage" materialization, publication and completeness text.

No changes outside backends/apple/coreai.

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

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

• 📄 Preview Python docs built from this PR

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

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

---

PR Review: #23390

I found no blocking correctness bugs. The main issues are that source publication skips the full device flush that bookmark publication uses, even though a damaged bundle is never replaced; the failure paths described in the README are not tested; and the key contract isn't written down.

Detailed review

Summary

This PR adds prepare_source_bundle, which writes the selected Core AI bundle from NamedDataMap into a .staging-<uuid> directory and publishes it with renameatx_np(RENAME_EXCL). An existing bundle is reused if it has the right file set, sizes, entry types and directories.

Code Quality

• No full flush before rename, and a bad bundle is never replaced (runtime/coreai_assets.mm:511-519, README.md:51, README.md:66-69).
  • The README says "atomic publication also uses F_FULLFSYNC before its rename". publish_storage_data does that, but source publication only runs plain fsync (through write_storage_file) before renameatx_np.
  • On macOS, fsync doesn't flush the drive cache. After a power loss, the renamed <key> directory can survive while file data does not.
  • That breaks in one of two ways:
    • If the sizes come out wrong, the entry is Corrupt, and loading fails every time with "Cannot replace a damaged…". No retry can fix it, because this PR never replaces a damaged entry.
    • If the sizes come out right but the bytes are wrong, the bad bundle is silently reused.
  • Fix: either issue F_FULLFSYNC on the staging tree (or at least on the root before the rename), or reword the README so it doesn't promise it. The "fail without replacement" policy makes the first option the safer one.
  • Fix this →
• Leftover .staging-* directories are never removed (coreai_assets.mm:466-476). StagingDirectory only cleans up when the function returns. If the process is killed or crashes partway through writing a large bundle, its partial staging directory stays in the assets root for good. Nothing in this PR removes old .staging-* entries. If a later PR in the stack sweeps them, say so in the README. Note that the names aren't tied to a key, so a sweeper can't tell which ones belong to a loader that still holds its lock. .staging-<key>-<uuid> would make that possible.
• NamedDataMap errors become InvalidExternalData (coreai_assets.mm:492-498). An AccessFailed or other transient error from get_data is reported as InvalidExternalData, so callers can't tell an I/O failure from a missing key. Pass buffer.error() through, or only remap NotFound.
• Error message mentions bookmarks (coreai_assets.mm:462). "Cannot replace a damaged bookmark recovery source" is returned for every caller, including the plain assets root path that the tests use. It should name the stored bundle in general.

API Design

• The key contract isn't documented (runtime/coreai_assets.h:37-42).
  • Reuse is decided by file set and sizes alone, under staging_root/<key>. Correctness therefore depends on key uniquely identifying manifest.bundle_digests[bundle].
  • The PR body says bundles are "keyed by the export-time bundle digest". But ExplicitStagingKeyReusesSourceWithoutWritesOrRepublication passes an unrelated key (fixture_identity('9')), and the function never checks key against the manifest.
  • Risk: a caller that reuses a key for a different bundle with the same sizes silently gets the old bytes.
  • Fix: either derive the key from manifest.bundle_digests inside the function, or state in the header what key must identify.
  • The staging_root parameter name is also misleading: on the regular path it is the assets root.

Testing

• The failure paths in the README have no tests (runtime/test/coreai_source_test.mm). The README says malformed existing bundles "fail loading without replacement", and the PR body says the concurrent winner "is validated". No test covers any of these:

  • an existing <key> with a missing file, an extra file, a wrong size, a symlink entry, or a regular file at <key>: each should fail with InvalidExternalData and leave the tree byte-identical (snapshot_matches already supports this check);
  • an incomplete winner in the RENAME_EXCL race, which should return "Concurrently published Core AI source is incomplete" and remove the losing staging directory;
  • a get_data buffer whose size doesn't match files[file].

  ExclusivePublicationValidatesWinnerAndRemovesLosingStaging only covers a valid winner.

Performance

• Directory fsyncs scale with file count times depth (runtime/coreai_storage.mm:148-166, called per file from coreai_assets.mm:500). For every file, write_file runs mkdirat, which may return EEXIST, and then fsyncs every ancestor directory again. A compiled .aimodelc with N files at depth d costs about N·(d+1) directory fsyncs on the first (cold) load. Inside a private staging directory that is published by rename, it would be enough to sync each file, then each directory once just before publishing.

Recommendation

Needs Discussion

Nothing here is a crash or a data race. Before merging, it's worth settling the durability-vs-no-replacement point (add F_FULLFSYNC, or reword the README), writing down the key contract, and adding tests for the malformed-bundle and incomplete-winner paths that the README describes.

[metascroy]

@claude review this code

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

Context on the previous findings: cache identity is supplied by the caller and already includes the selected bundle's export-time digest plus platform/SDK context. We documented that contract rather than deriving a digest-only key inside source preparation. The no-replacement policy and absence of runtime content hashing are intentional. Abandoned-temporary recovery must not blindly sweep another active loader's files; it is separate from normal failure cleanup. Batching repeated directory syncs remains a potential optimization requiring measurement and care with durability ordering, not an established correctness defect. Durability, error propagation, and focused failure-path coverage were strengthened.

[claude]

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

---

PR Review: #23390

The fixes from the last round hold up and I found no blockers: the manifest path check doesn't account for macOS filename case folding or Unicode normalization, and a few failure-path tests are weaker than they appear.

Detailed review

Summary

This PR adds prepare_source_bundle, which writes the selected bundle from NamedDataMap into .staging-<uuid> and publishes it with renameatx_np(RENAME_EXCL). An existing <key> is reused only if its file set, sizes and entry types match.

The changes from the last round are correct:

• Durability: files get fsync, the staging directory gets F_FULLFSYNC before the rename, and the root gets F_FULLFSYNC after it.
• Errors: NotFound from named data becomes InvalidExternalData; every other error is passed through unchanged.
• Race test: the RENAME_EXCL race test runs the real EEXIST branch.

The issues below are about manifest path validation and how strong the tests are.

Code Quality

• Names that differ only by case break publication with a misleading error (coreai_assets.mm:76-101, validate_files).
  • The path checks are case-sensitive, but APFS is case-insensitive by default.
  • model.aimodel/A.bin + model.aimodel/a.bin: the second openat(O_CREAT|O_EXCL) gets EEXIST.
  • model.aimodel/Dir/x + model.aimodel/dir/y: both files land in one directory, so the post-write validate_tree fails.
  • In both cases every cold load fails with AccessFailed or InvalidExternalData, when the real problem is the manifest (InvalidProgram).
  • Rejecting collisions under -lowercaseString (or case folding) in validate_files gives a clear, early error.
  • Fix this →
• Non-ASCII names may never validate (suspected; check on a Mac) (coreai_assets.mm:148-159, coreai_storage.mm:227).
  • Files are created via -fileSystemRepresentation, which yields decomposed (NFD) bytes.
  • storage_children turns the readdir bytes back into an NSString, and files[path] / containsObject: compare strings literally.
  • So a manifest name with a precomposed character, e.g. café.bin as Python json writes it, would fail validate_tree right after staging, on every load.
  • Either restrict asset names to ASCII in validate_files, or compare names in one normalization form. No test currently uses a non-ASCII name.
• Cleanup works by path, not through the root descriptor (coreai_assets.mm:229-243). StagingDirectory removes the staging directory with NSFileManager removeItemAtURL: on a path rebuilt from staging_root. That contradicts the "Inspect, stage and publish relative to one root descriptor" comment and the PR description. With a trusted root this is low risk, but the comment overstates what the code does. Unlinking relative to root_fd, or rewording the comment, would resolve it.
• A reused entry is never re-flushed (coreai_assets.mm:445-447, and the EEXIST winner path).
  • If the post-rename root F_FULLFSYNC fails, the call returns AccessFailed, which is intentional per PostRenameFlushFailureLeavesCompleteSourceVisible.
  • The next load then sees the entry as Valid and returns it without flushing again, so a later step (such as persisting a bookmark that points at it) may rely on a publication that was never made durable.
  • Fix with a root F_FULLFSYNC on reuse, or by stating the limitation in the README.

API Design

• The header comment doesn't match the code (coreai_assets.h:37-44).
  • staging_root is the assets root: bundles end up at <root>/<key>/<bundle>, and only .staging-<uuid> is temporary.
  • The header never says key must be exactly 64 lowercase hex characters, even though the function rejects anything else with InvalidArgument. A caller following only the "identify digest and name" wording could pass a readable name.
  • Suggest renaming the parameter to assets_root and stating the key format.

Testing

• RejectsUnselectedAotSource passes for the wrong reason (coreai_source_test.mm:27-33).
  • It passes a nil root and key and nullptr data, and only checks !ok().
  • If the "must be selected" check were deleted, the key check would still fail and the test would still pass.
  • Assert Error::InvalidProgram with a valid root and key. The nullptr NamedDataMap and absent-root cases (around lines 176-178) also check only !ok().
• The rename fault never runs the real renameatx_np error path (coreai_assets.mm:505-508).
  • The Rename fault is a separate storage_fault() check placed before the call, not routed through storage_call.
  • So the non-EEXIST branch ("Cannot publish Core AI stored bundle") is never run.
  • An injected EINTR is not retried, unlike every other operation.
• Several failure paths can't be fault-injected, so they are untested. The mkdirat/openat of the staging directory and the directory fsync inside write_file and sync_storage_directory use retry_eintr rather than storage_call. Their failures, and the cleanup after them, are therefore untested.
• Permission errors aren't tested to stay distinct from "damaged". No test shows that EACCES from inspect_entry or validate_tree comes back as AccessFailed rather than Corrupt. Since a corrupt entry is never replaced, getting that wrong would be permanent.
• ConcurrentColdAndWarmSource is serial (coreai_source_fixture.mm:87-96). Every worker takes the fixture's exclusive flock before calling prepare_source_bundle, so the test checks the lock contract, not concurrent code. This is fine if intended, but the name suggests more coverage than it gives.

Recommendation

Approve

Nothing here blocks merging. The case-folding and Unicode checks in validate_files are worth doing in this PR: without them a valid-looking manifest can leave a bundle that never loads, with an error that points at storage instead of the manifest. The test tightening can come in a follow-up.