Address review feedback on pull repo thread safety

[anandhu-eng]

Resolves the six inline review comments on #265, plus the test issues raised in the review body. The theme across them: the lock gives mutual exclusion but not atomicity — nothing rolled back partial on-disk state, and the next process misread that state as healthy.

Comments resolved

1. Partial clone poisoned the path permanently (the blocking one)

A clone killed mid-flight leaves a .git with a configured origin but no HEAD. The existence check then took the "already exists" branch, and every subsequent mlc pull repo failed identically until the user deleted the directory by hand.

Clone now targets a <repo>.tmp-clone sibling and is os.rename'd into place inside the lock, with cleanup on BaseException so KeyboardInterrupt tidies up too. repo_path is only ever absent or complete.

2. Couldn't distinguish a healthy checkout from a half-cloned one

Confirmed the blind spot: git status --porcelain exits 0 with empty stdout on the wreckage, and 128 with empty stdout on a non-git directory — both read as "clean". Added _is_valid_git_repo(), which probes git rev-parse HEAD; anything failing it is removed and re-cloned rather than pulled.

3. repos.json writes were non-atomic, and readers never take the lock

Added _atomic_write_json() (temp file + os.replace), used by both register_repo and unregister_repo. This makes the existing lock-free readers in action.py safe without having to change them.

4. 300s timeout too short, and timing out was a hard failure

Now MLC_REPO_LOCK_TIMEOUT, defaulting to 1800s. On Timeout we re-check the repo: if the holder left a valid checkout, the work is done, so return success rather than a spurious error. Also added the explicit PermissionError branch — creating <repo>.lock needs write access to repos_path itself, and the generic handler only surfaced a bare [Errno 13].

5. Nested lock acquisition undocumented

Lock-ordering invariant (repo lock → repos.json lock) is now stated at all three acquisition sites.

6. Post-register re-read — ⚠️ the suggested fix is not safe, so I did something else

Moving load_repos_and_meta() inside the lock self-deadlocks. It calls rm_repo() for entries whose path has vanished, and rm_repo → unregister_repo takes the same repos.json lock on a fresh FileLock instance. filelock is only reentrant per instance:

outer acquired
  inner TIMED OUT after 2.0s -> SELF-DEADLOCK

So this would hang for the full 60s on every pull whose repos.json holds one stale entry. I kept the read outside the lock, documented why, and closed the actual reported symptom — the lookup missing the repo just registered — by falling back to the meta already in hand.

Test fixes

test_concurrent_pull_repo_clone_called_once patched subprocess.run inside each thread, so the first thread to exit restored the real one while the others were still in pull_repo and could shell out to real git against github.com/example/test-repo.git. The patch is now applied once in the main thread, a barrier makes the threads genuinely contend, and the assertions check the returned dicts — pull_repo returns error dicts rather than raising, so the old assertEqual(errors, []) stayed green on {'return': 1}.

Added coverage for each new behaviour (half-clone recovery, failed-clone cleanup, atomic-write reader safety, failed-write rollback, timeout env override and its invalid-value fallback), and extracted _RepoActionTestBase so the new classes don't re-run the existing class's cases.

Verification

Mutation-tested rather than assumed — each new test fails when its fix is removed:

Mutation	Result
_is_valid_git_repo always returns True	❌ half-clone test fails
_atomic_write_json writes directly	❌ both atomic-write tests fail
clone to repo_path, no rollback	❌ failed-clone test fails

Full suite 59 passed, stable over 5 consecutive runs. autopep8 --diff -a clean on both files.

Not addressed

• add_repo's os.makedirs + register_repo race — you flagged it as out of scope for a PR titled "pull repo", and I've left it.
• The PR description on Make mlc pull repo thread safe #265 says 2 tests; the file has 3 (now 9). Worth updating when merging.

🤖 Generated with Claude Code

[github-actions]

MLCommons CLA bot All contributors have signed the MLCommons CLA ✍️ ✅

[github-actions]

🤖 AI PR Review Summary

This PR improves concurrency safety and robustness in repo management by introducing atomic JSON writes, configurable repo lock timeouts, and stricter validation of git repo states. It also adds detailed lock ordering comments to prevent deadlocks and enhances error handling for permission issues. The changes reduce race conditions and partial state visibility, especially during cloning and repo registration. The test refactor introduces a base class for shared setup, improving test structure. Overall, the design is sound with clear attention to concurrency and failure modes. No major design issues found; comments focus on minor clarifications and potential improvements in logging and exception handling.

[github-actions]

The multiline f-string expression split across lines is invalid syntax. This line should be fixed to a single line or use parentheses to concatenate strings properly.

[github-actions]

The fallback to creating a new Repo object if not found in the loaded list is a good safety net. Consider logging this event for easier debugging.

[github-actions]

Cloning into a temporary directory and renaming is a robust approach to avoid partial clones. The cleanup in except block is good. Consider adding a debug log before removing tmp_clone_path.

[github-actions]

Refactoring to a base test class for shared setup is a good practice to avoid test duplication and improve maintainability.