fix: preserve caller-owned Hugging Face cache sidecars - #1792
fix: preserve caller-owned Hugging Face cache sidecars#1792mldangelo-oai wants to merge 2 commits into
Conversation
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
Performance BenchmarksCompared
|
There was a problem hiding this comment.
Pull request overview
This PR hardens Hugging Face streaming download cleanup to avoid deleting caller-owned cache sidecar files (notably ONNX external data), by replacing directory-name heuristics with explicit provenance and by enforcing resolved-path containment checks before any cleanup.
Changes:
- Introduces explicit cache ownership provenance (
_cache_dir_owned) for Hugging Face streaming downloads and requires it for sidecar cleanup decisions. - Strengthens cleanup safety by requiring strict path resolution and containment checks (failing closed on resolution/symlink issues).
- Adds regression tests covering ownership, symlink escape, resolution-failure behavior, and generator-close cleanup behavior; documents the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
tests/utils/sources/test_huggingface.py |
Adds focused tests ensuring streaming cleanup only removes invocation-owned sidecars and fails closed on symlink/resolution edge cases. |
modelaudit/utils/sources/huggingface.py |
Reworks _should_cleanup_hf_streaming_context_file to require explicit ownership and strict containment; threads ownership through streaming flow. |
modelaudit/cli.py |
Marks CLI-created temporary HF cache roots as owned (_cache_dir_owned) and resolves the temp path strictly. |
CHANGELOG.md |
Records the behavior fix under [Unreleased] bug fixes. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
Security review completed. No security issues were found in this pull request. Reviewed commit: ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
…-caller-owned-cache-cleanup-20260730
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 53101456b7
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if not cache_dir_owned or cache_dir is None or download_path is None: | ||
| return False |
There was a problem hiding this comment.
Clean up newly downloaded persistent-cache sidecars
When --stream runs with caching enabled (the default), modelaudit/cli.py passes _cache_dir_owned=False, so this branch prevents cleanup of every context-only ONNX external-data file, including files freshly downloaded by this invocation. Those sidecars are never yielded to scan_model_streaming, so its delete_after_scan handling cannot remove them; large external-data files therefore accumulate in ~/.modelaudit/cache despite scan-and-delete mode. Track whether each sidecar predated the download and preserve only those preexisting files rather than treating the entire persistent cache as caller-owned.
Useful? React with 👍 / 👎.
Summary
modelaudit_hf_.[Unreleased].Validation
mainbefore the fix.