Feature/bwdo 809 implement lock clone cache#68
Conversation
There was a problem hiding this comment.
Pull request overview
This PR implements an inter-process lock around the local repo cache so that only one clone/cache refresh runs at a time, with a hard timeout (10 minutes) after which the waiting process exits.
Changes:
- Add a
filelock-based lock around cache creation/usage inRepoClonerwith periodic wait logging and a hard timeout. - Refactor clone flow to route cached clones through a new
_clone_with_cache()path and pass the cache reference into_clone(). - Add
filelockas an installation dependency.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/sc/clone/cloners/repo_cloner.py |
Adds cache locking + timeout logic and refactors clone flow to use a cache reference safely under a lock. |
setup.py |
Adds filelock dependency required for cache locking. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| started = time.monotonic() | ||
| lock = FileLock(CACHE_LOCK_PATH) | ||
|
|
||
| logger.info("Acquiring cache lock.") | ||
| while True: | ||
| try: | ||
| lock.acquire(timeout=20) | ||
| break | ||
| except Timeout: | ||
| waited = int(time.monotonic() - started) | ||
| if waited < CACHE_MAX_WAIT: | ||
| logger.info( | ||
| f"Cache is in use by another process, waited {waited} seconds..." | ||
| ) | ||
| else: | ||
| logger.error( | ||
| f"Cache remained locked past the set wait time of {CACHE_MAX_WAIT} seconds." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
I'm pretty sure old SC would clear the lock if you reached the 5 min timeout. We should probably do that. Also does this library handle the lock if it was there since before the start of the sc process, like when a clone gets killed halfway through and the lock is never cleared. Then the user tries to clone again?
| started = time.monotonic() | ||
| lock = FileLock(CACHE_LOCK_PATH) | ||
|
|
||
| logger.info("Acquiring cache lock.") | ||
| while True: | ||
| try: | ||
| lock.acquire(timeout=20) | ||
| break | ||
| except Timeout: | ||
| waited = int(time.monotonic() - started) | ||
| if waited < CACHE_MAX_WAIT: | ||
| logger.info( | ||
| f"Cache is in use by another process, waited {waited} seconds..." | ||
| ) | ||
| else: | ||
| logger.error( | ||
| f"Cache remained locked past the set wait time of {CACHE_MAX_WAIT} seconds." | ||
| ) | ||
| sys.exit(1) |
There was a problem hiding this comment.
I'm pretty sure old SC would clear the lock if you reached the 5 min timeout. We should probably do that. Also does this library handle the lock if it was there since before the start of the sc process, like when a clone gets killed halfway through and the lock is never cleared. Then the user tries to clone again?
| logger = logging.getLogger(__name__) | ||
|
|
||
| REPO_CACHE_DIR = Path.home() / ".caches" | ||
| CACHE_LOCK_PATH = REPO_CACHE_DIR / ".lock" |
There was a problem hiding this comment.
Might be better to call the lock .sc_lock. I doubt we'll ever re-use the cache in the future, but alwaty nicer to know where the lock came from.
Implement cache locking. Went with a timeout of 10 minutes where if a user is waiting for more than 10 mins it will error out for them.
The previous version waited for 5 minutes and if it was still locked beyond then it bypassed it for the user. This does mean it never gets hard locked but also could cause a long clone (that is locking the cache) to then error which could be frustrating for users.
I'm thinking of keeping the hardline solution of the cache is locked unless you go and delete the lockfile and see if any users get stuck and if so we can look at alternatives. But open to suggestions.