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. |
anandhu-eng
left a comment
anandhu-eng
left a comment
There was a problem hiding this comment.
[Analysis by claude]
The locking itself is correct — I verified filelock 3.13.1 release semantics empirically rather than trusting the docs:
| Failure while the holder has the lock | Result |
|---|---|
Exception inside the with |
__exit__ calls release() unconditionally → lock freed, waiter proceeds immediately |
| Hard kill (SIGKILL / OOM / power loss) | fcntl.flock is bound to the fd, so the kernel drops it on process death — no stale-lock deadlock; the leftover .lock file is empty and unheld |
Timeout |
Handled by the new except Timeout: branches; the waiter corrupts nothing |
So the PR does what it says. The remaining issues are that the lock gives mutual exclusion but not atomicity — there is no rollback of partial on-disk state, and the next process misreads that state as healthy. Details inline; the clone-rollback one (:584-597 + :611) is the one I'd consider blocking, because I reproduced it leaving the repo permanently unpullable.
Out of diff range, same failure mode: add_repo at repo_action.py:275 does os.makedirs(repo_path) and then register_repo with no repo-level lock, so two concurrent mlc add repo on the same path still race (FileExistsError). The PR title scopes to pull repo, so this is a note rather than a request.
tests/test_pull_repo_thread_safety.py: test_concurrent_pull_repo_clone_called_once is itself not thread-safe — each of the 5 threads runs with patch('mlc.repo_action.subprocess.run', ...), and unittest.mock.patch mutates a module global. When the first thread exits its with, it restores the real subprocess.run while the other four are still inside pull_repo, which can shell out to real git against https://github.com/example/test-repo.git. Apply the patch once in the main thread before starting the threads. Separately, assertEqual(errors, []) asserts nothing useful here, because pull_repo returns error dicts rather than raising — threads 2–5 almost certainly return {'return': 1} and the test stays green. Assert on the returned dicts instead. (Also, the PR description lists 2 tests; the file has 3.)
| if not os.path.exists(repo_path): | ||
| logger.info( | ||
| f"Cloning repository {repo_url} to {repo_path}...") | ||
|
|
||
| else: | ||
| logger.info( | ||
| f"Repository {repo_name} already exists at {repo_path}. Checking for local changes...") | ||
|
|
||
| # Check for local changes | ||
| status_command = [ | ||
| 'git', | ||
| '-C', | ||
| repo_path, | ||
| 'status', | ||
| '--porcelain', | ||
| '--untracked-files=no'] | ||
| local_changes = subprocess.run( | ||
| status_command, capture_output=True, text=True) | ||
|
|
||
| if local_changes.stdout.strip(): | ||
| if not force: | ||
| logger.warning( | ||
| "There are local changes in the repository. Please commit or stash them before checking out.") | ||
| print(local_changes.stdout.strip()) | ||
| return { | ||
| "return": 0, "warning": f"Local changes detected in the already existing repository: {repo_path}, skipping the pull"} | ||
| # Build clone command | ||
| clone_command = ['git', 'clone'] | ||
| if branch: | ||
| clone_command += ['--branch', branch] | ||
| if clone_depth is not None: | ||
| clone_command += ['--depth', str(clone_depth)] | ||
| clone_command += extra_args | ||
| clone_command += [repo_url, repo_path] | ||
|
|
||
| logger.warning( | ||
| "Local changes detected. Running force pull with temporary git stash.") | ||
| stash_created = False | ||
| try: | ||
| stash_before = subprocess.run( | ||
| ['git', '-C', repo_path, 'stash', 'list'], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True | ||
| ) | ||
| stash_res = subprocess.run( | ||
| ['git', '-C', repo_path, 'stash', 'push', | ||
| '-m', 'mlc pull repo --force'], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True | ||
| ) | ||
| stash_after = subprocess.run( | ||
| ['git', '-C', repo_path, 'stash', 'list'], | ||
| capture_output=True, | ||
| text=True, | ||
| check=True | ||
| ) | ||
| stash_created = len(stash_after.stdout.splitlines() | ||
| ) > len(stash_before.stdout.splitlines()) | ||
| except subprocess.CalledProcessError as e: | ||
| stash_error = (e.stderr or e.stdout or str(e)).strip() | ||
| return { | ||
| "return": 1, | ||
| "error": f"Force pull failed while stashing local changes in {repo_path}: {stash_error}" | ||
| } | ||
| subprocess.run(clone_command, check=True) |
There was a problem hiding this comment.
[Analysis by claude] The lock serialises the clone, but nothing rolls back a partial clone, and the existence check on line 584 then treats the wreckage as a valid repo. I reproduced the full chain:
- Process 1 takes the lock,
git clonestarts, process is killed mid-clone (SIGKILL / OOM / Ctrl-C / dropped link). - The directory survives, containing a
.gitwith a configuredorigin, branchmaster, and noHEAD. - Lock is released correctly. Process 2 acquires it, hits
if not os.path.exists(repo_path)→ False → takes the "already exists" branch. - That branch ends in
git pull, which fails withfatal: no tracking information for the current branch.
It never recovers — every subsequent mlc pull repo re-enters step 3 and fails identically, so the user has to rm -rf ~/MLC/repos/<repo> by hand. So the answer to "does an error mid-clone lose the lock?" is no, but it does poison the path for everyone afterwards.
Worth noting the boundary: on a clean clone failure (bad URL, auth reject) git removes the directory itself — I confirmed that. The leak is specific to the abrupt-termination paths.
Suggested fix — clone to a sibling temp path and os.rename into place inside the lock, so repo_path is either absent or complete:
tmp_path = repo_path + ".tmp-clone"
shutil.rmtree(tmp_path, ignore_errors=True)
clone_command += [repo_url, tmp_path]
try:
subprocess.run(clone_command, check=True)
os.rename(tmp_path, repo_path)
except BaseException: # BaseException so KeyboardInterrupt cleans up too
shutil.rmtree(tmp_path, ignore_errors=True)
raise| status_command = [ | ||
| 'git', | ||
| '-C', | ||
| repo_path, | ||
| 'status', | ||
| '--porcelain', | ||
| '--untracked-files=no'] | ||
| local_changes = subprocess.run( | ||
| status_command, capture_output=True, text=True) | ||
|
|
||
| if local_changes.stdout.strip(): | ||
| if not force: |
There was a problem hiding this comment.
[Analysis by claude] Paired with the clone-rollback issue above: this branch cannot distinguish a healthy checkout from a half-cloned one, and it silently picks the wrong recovery.
local_changes.returncode is never inspected. On the partial clone from an interrupted git clone, git status --porcelain --untracked-files=no returns rc=0 with empty stdout (verified). So local_changes.stdout.strip() is falsy, control falls to the else at line 726 which logs "No local changes detected. Pulling latest changes...", and git pull then fails on a repo that should simply have been re-cloned.
The same blind spot applies to a directory that exists but isn't a git repo at all — rc=128, stderr populated, stdout empty, so it reads as "clean" too.
Even with the temp-dir+rename fix, a cheap guard here is worth having: validate with git -C repo_path rev-parse HEAD (or check returncode) and treat a broken/absent checkout as re-clone rather than pull.
| logger.info(f"Added new repo path: {repo_path}") | ||
|
|
||
| with open(repos_file_path, 'w') as f: | ||
| json.dump(repos_list, f, indent=2) |
There was a problem hiding this comment.
[Analysis by claude] The lock closes the writer-vs-writer race, but the write is still non-atomic and readers never take this lock, so the race just moves.
open(..., 'w') truncates and json.dump rewrites incrementally. Meanwhile Action.load_repos_and_meta() (mlc/action.py:95) and Action.load_repos() (mlc/action.py:174) both do a bare json.load with no FileLock. A read landing inside that truncate→rewrite window gets a truncated file and raises JSONDecodeError — with repos.json momentarily looking empty, which is the worst-case content to observe.
Writing to a temp file and os.replace-ing it fixes this and makes the existing lock-free readers safe without having to touch action.py:
tmp = repos_file_path + ".tmp"
with open(tmp, 'w') as f:
json.dump(repos_list, f, indent=2)
os.replace(tmp, repos_file_path) # atomic on POSIX and WindowsSame applies to the json.dump in unregister_repo at line 1078.
| # Lock file sits next to the repo directory; left on disk but | ||
| # harmless. | ||
| repo_lock_file = repo_path + ".lock" | ||
| with FileLock(repo_lock_file, timeout=300): |
There was a problem hiding this comment.
[Analysis by claude] 300 s is likely too short for the workload this guards, and timing out is currently a hard failure.
A cold clone of mlperf-automations on a slow or throttled link comfortably exceeds five minutes, and the large model/dataset repos more so. When that happens the waiter returns {'return': 1} even though the holder is making perfectly good progress — a false failure caused by the fix rather than by any real contention. In CI, where several jobs may warm the same MLC_REPOS, this is the likely first symptom.
Two options, either is fine:
- On
Timeout, re-checkos.path.exists(repo_path)(plus a validity check) before erroring — if the other process finished the clone, there is nothing left to do and this can return success. - Make the timeout configurable (e.g.
MLC_REPO_LOCK_TIMEOUT) and default it substantially higher.
Minor, related: creating the lock file now requires write permission on repos_path itself, not just on the repo directory. On a shared MLC_REPOS across Unix users that surfaces as a PermissionError reported through the generic handler as "Error pulling repository: [Errno 13]", which won't point anyone at the lock file. An explicit branch would help.
| return {"return": 1, | ||
| "error": f"Syntax error in {meta_file_path}: {e}"} | ||
|
|
||
| r = self.register_repo( |
There was a problem hiding this comment.
[Analysis by claude] Nit / future-proofing: this introduces nested lock acquisition — <repo_path>.lock is held here while register_repo goes on to acquire repos.json.lock (line 334).
There is no deadlock today because the ordering is consistent everywhere (repo lock → repos.json lock), but nothing documents or enforces that invariant. Anyone later adding a path that takes repos.json.lock first and then a repo lock gets a hang bounded only by the 60 s / 300 s timeouts, which is a genuinely unpleasant thing to debug.
A one-line comment stating the ordering rule at both acquisition sites would be enough.
| ) | ||
| } | ||
|
|
||
| self.repos = self.load_repos_and_meta() |
There was a problem hiding this comment.
[Analysis by claude] Nit: this re-read happens after the lock is released, so it can observe a repos.json that a concurrent process has modified in the interim — meaning self.repos may not reflect the list this call just wrote, and the next(...) lookup on line 356 can miss the repo that was just registered.
Low impact given the caller usually re-derives state anyway, but if the intent is "read back what I wrote", pulling this inside the with block is free and removes the window.
main gained repos.json locking of its own via #267, which touched the same two functions this branch does, so register_repo and unregister_repo conflicted four times. Both sides were semantically identical -- a 60s FileLock over the repos.json read-modify-write plus an error dict on Timeout -- so the resolution keeps main's structure and this branch's wording: - lock path: use main's shared _repos_lock_file() helper rather than this branch's inlined repos_file_path + ".lock". The helper already exists in the merged file, so inlining would duplicate it. - Timeout message: keep this branch's longer text, which names the likely cause and the remedy. It also drops the stray f-prefix on main's placeholder-free f-string. This branch's own contribution is untouched: the per-repo FileLock(repo_path + ".lock", timeout=300) around the clone/pull/register section of pull_repo, and its Timeout handler. That handler sits after RuntimeError and CalledProcessError but before the bare Exception -- correct, since filelock.Timeout derives from OSError and so is not swallowed by either earlier clause. Verified: no conflict markers, mlc/repo_action.py compiles, full suite 53 passed, and the net diff against main is exactly this branch's additions. The one autopep8 nit remaining in the file is pre-existing on main (identical 12-line diff), so it is left alone.
…urious clone path Co-authored-by: arjunsuresh <4791823+arjunsuresh@users.noreply.github.com>
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