Make mlc pull repo thread safe#265
Conversation
🤖 AI PR Review Summary\n\nThis PR introduces file locking around critical file and directory operations to prevent concurrent access issues. It adds FileLock usage for repos.json read/write and for cloning or pulling git repositories. The design improves concurrency safety but adds complexity and potential deadlock risks if locks are not managed carefully. The stash/apply logic for force pulling with local changes is enhanced with better error handling and rollback. Overall, the changes improve robustness but require careful testing to avoid lock contention or git state inconsistencies. |
|
MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅ |
🤖 AI PR Review Summary\n\nThis PR introduces file locking around critical file operations to prevent concurrent access issues and adds locking around git repository cloning and pulling to avoid race conditions. It also refactors the force pull logic to stash local changes before pulling and attempts to reapply them after, with improved error handling and rollback on stash apply conflicts. The main risks involve potential deadlocks or lock contention due to long lock hold times during git operations, and complexity in stash conflict recovery. The design improves concurrency safety but increases complexity in error handling and recovery. |
anandhu-eng
left a comment
anandhu-eng
left a comment
There was a problem hiding this comment.
🤖 Claude Code Review (requested by @anandhusooraj)
Reviewed by Claude (claude-sonnet-4-6) at the request of a maintainer.
Overview
The race conditions described are real: concurrent pull_repo callers can both find the repo directory absent and race to git clone the same path; concurrent register_repo / unregister_repo callers can corrupt repos.json via a lost-update. The FileLock approach using an existing transitive dependency is the right tool. Lock ordering is consistent (repo.lock always acquired before repos.json.lock), so no deadlock risk.
Issues
🔴 The register_repo thread-safety test does not test RepoAction.register_repo
test_concurrent_register_repo_no_data_loss calls self._register_path(path) — a private helper defined in the test class that re-implements the FileLock pattern itself. It never calls RepoAction.register_repo at all.
def _register_path(self, path):
"""Thread-safe repos.json registration (mirrors register_repo logic)."""
repos_lock_file = self.repos_file + ".lock"
with FileLock(repos_lock_file, timeout=60):
...If someone removes FileLock from RepoAction.register_repo, this test will still pass. The test validates that FileLock works in general, not that register_repo uses it. The unregister test is correct — it calls the real unregister_repo function.
Fix: Call RepoAction.register_repo (or use RepoAction.pull_repo end-to-end with mocked git) in the register test, the same way the unregister test calls the real function.
🔴 No test for the double-clone race condition
The PR description identifies the double-clone race as the primary problem (two threads can both see a repo directory is absent and race to git clone). There is no test that exercises this path. The two tests cover register/unregister only, not the pull_repo clone guard.
A minimal test: spawn N threads all calling pull_repo for the same (absent) repo URL with git mocked; assert git clone is called exactly once.
🟡 Timeout is imported but never caught explicitly
from filelock import FileLock, TimeoutWhen FileLock exhausts its timeout it raises filelock.Timeout. Neither pull_repo nor register_repo / unregister_repo catches it specifically — it falls through to the generic except Exception as e handler, which returns:
"Error pulling repository: [Timeout was raised because the lock ... could not be acquired]"
This is confusing for users. The Timeout import suggests the intent was to handle it, but the handler was never written.
Fix: Catch Timeout explicitly and return a clear, actionable message:
except Timeout:
return {
'return': 1,
'error': (
f"Could not acquire lock for {repo_path} after 300 seconds. "
"Another mlc process may be cloning or pulling this repo. "
"Try again once the other operation completes."
)
}Similarly for the repos.json lock in register_repo / unregister_repo.
🟢 Lock files are left on disk next to repo directories
FileLock creates <repo_path>.lock (e.g. mlcommons@mlperf-automations.lock) next to each repo directory in the repos folder, and never removes them. The files are harmless — repo listing reads from repos.json, not by scanning the directory. But users who manually browse ~/MLC/repos/ will see unfamiliar .lock files alongside their repos.
This is a cosmetic issue with no functional impact, but worth documenting in a comment near the lock construction so future maintainers don't mistake them for corruption.
🟢 PR template checklist vs documented branch policy
The PR checklist item Target branch is 'dev' is unchecked, and this PR correctly targets main. AGENTS.md states PRs should target main. The checklist template appears to be outdated — it's a template issue, not a code issue, but it's causing friction across PRs.
Summary
| Area | Status |
|---|---|
| Core lock design (FileLock, lock ordering) | ✅ Correct |
register_repo lock |
✅ Correct implementation |
unregister_repo lock |
✅ Correct implementation |
pull_repo clone-race lock |
✅ Correct implementation |
register_repo thread-safety test |
❌ Tests a re-implementation, not the real method |
| Double-clone race test | ❌ Missing entirely |
Timeout handling |
|
| Lock file litter | 🟢 Cosmetic only |
The locking logic itself is sound. The two test gaps are the main things to fix before merging.
|
@copilot address the issue in the last comment |
Addressed all the issues raised:
Commit: the latest two commits on this PR. |
🤖 AI PR Review Summary\n\nThis PR introduces file locking using the filelock library to prevent concurrent modifications of critical files and directories during repo operations. It adds locking around updates to repos.json and around cloning/pulling git repositories. The design improves robustness against race conditions but introduces potential blocking and timeout errors. The force pull logic is enhanced to stash local changes before pulling and restore them after, with error handling for stash apply conflicts. Risks include deadlocks if locks are not released properly and complexity in error recovery during stash conflicts. |
🤖 AI PR Review Summary\n\nThis PR introduces file locking around critical file and directory operations to prevent concurrent access issues. It adds FileLock usage when reading/writing repos.json and when cloning or pulling git repositories. The locking improves robustness in multi-process scenarios but adds complexity and potential for deadlocks or timeouts. The git pull logic is enhanced to handle local changes with a forced stash and apply workflow, improving safety but increasing complexity and error handling paths. Overall, the design improves concurrency safety but requires careful testing of lock acquisition and stash conflict recovery. |
|
@copilot resolve the merge conflicts in this pull request |
…h FileLock thread safety
Resolved. The conflict was between the |
|
@copilot resolve the merge conflicts in this pull request |
…staller venv improvements
Resolved. The branch now includes all changes from the latest |
|
@copilot resolve the merge conflicts in this pull request - there are 3 more conflicts w.r.t the dev branch |
…th latest dev features
Resolved. Took
Commit: the latest merge commit on this PR. |
🤖 AI PR Review Summary\n\nThis PR introduces file locking to prevent concurrent modifications of critical files and directories during repository operations. It adds a FileLock around the repos.json update to avoid race conditions when multiple processes modify the repo list, and similarly adds a FileLock around cloning and pulling operations on individual repositories to ensure exclusive access. The force pull logic with stash handling is preserved but now protected by the lock. Risks include potential deadlocks or delays if locks are held too long, and the lock files remain on disk but are harmless. The design improves concurrency safety but adds complexity and potential timeout errors that are handled with user-friendly messages. |
🤖 AI PR Review Summary\n\nThis PR introduces file locking around critical file and directory operations to prevent concurrent modifications, specifically when updating repos.json and when cloning or pulling git repositories. It uses the filelock library to acquire locks with timeouts, improving robustness in multi-process scenarios. The design improves concurrency safety but introduces risks of deadlocks or delays if locks are held too long. The force pull logic with git stash is preserved but now wrapped inside the repo lock. Some error handling and logging are enhanced. Overall, the changes are positive but require careful testing under concurrent usage to avoid lock contention issues. |
Concurrent
mlc pull repocalls race onrepos.json— two threads can both read, both modify their local copy, and one write overwrites the other's changes. Similarly, two threads can both see a repo directory is absent and race togit clonethe same path.Changes
mlc/repo_action.pyfrom filelock import FileLock, Timeout(already a transitive dep viaindex.py)register_repo: Wrapsrepos.jsonread-modify-write inFileLock(repos.json.lock, timeout=60), eliminating lost-update races on concurrent registrationunregister_repo: SameFileLockprotection for the remove-and-write pathpull_repo: Wraps the entire clone/pull/register section in a per-repoFileLock(repo_path + ".lock", timeout=300), preventing double-clone races when two callers simultaneously find the repo absenttests/test_pull_repo_thread_safety.pytest_concurrent_register_repo_no_data_loss: 10 threads concurrently register unique paths → all 10 present inrepos.json, no duplicatestest_concurrent_unregister_repo_no_data_loss: 10 threads concurrently unregister unique paths → all 10 removed, no duplicates✅ PR Checklist
dev📌 Note: PRs must be raised against
dev. Do not commit directly tomain.✅ Testing & CI
📚 Documentation
📁 File Hygiene & Output Handling
🛡️ Safety & Security
🙌 Contribution Hygiene