Feature/bwdo 809 implement lock clone cache

[BenjiMilan]

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.

Edit 1:
Decided to go with 5 minutes and a bypass to match old sc instead of 10 minutes and an error.

[Copilot]

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 in RepoCloner with 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 filelock as 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.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

src/sc/clone/cloners/repo_cloner.py:93

• The cache lock is global (single ~/.caches/.lock), so clones against different manifest hosts will unnecessarily block each other. Also, the wait loop can exceed CACHE_MAX_WAIT by up to the per-attempt acquire timeout (20s), and the log/error messages don’t include the specific lock path, making manual recovery harder.

        started = time.monotonic()
        lock = FileLock(CACHE_LOCK_PATH)

        logger.info("Acquiring cache lock.")
        while True:

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/sc/clone/cloners/repo_cloner.py:115

• When the max wait time is exceeded, the code breaks and continues cloning without holding the cache lock, yet still logs "Cache lock acquired.". This both defeats the purpose of cache locking (possible concurrent cache mutation) and produces misleading logs. If the intended behavior is a hard timeout (per PR description), abort the command once CACHE_MAX_WAIT is exceeded.

                else:
                    logger.warning(
                        f"Cache remained locked past the set wait time of {CACHE_MAX_WAIT} seconds. "
                        "Force proceeding without acquiring cache lock..."
                    )
                    logger.warning(
                        "If you think the cache is incorrectly locked you can try deleting "
                        f"{CACHE_LOCK_PATH}"
                    )
                    break
        logger.info("Cache lock acquired.")

src/sc/clone/cloners/repo_cloner.py:35

• CACHE_MAX_WAIT is set to 300 seconds (5 minutes), but the PR description says users should wait up to 10 minutes before timing out. This makes the implemented behavior inconsistent with the stated requirement.

This issue also appears on line 105 of the same file.

CACHE_MAX_WAIT = 300

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (2)

src/sc/clone/cloners/repo_cloner.py:124

• When the wait loop breaks due to exceeding CACHE_MAX_WAIT, locked remains False but the code still logs "Cache lock acquired." and continues to create/use the cache (self._cache()), which defeats the point of locking and can cause concurrent cache mutation.

                    break
        logger.info("Cache lock acquired.")

        try:
            reference = self._cache()

src/sc/clone/cloners/repo_cloner.py:110

• first_warning is never set to True, so the “SC will wait…” and lockfile deletion guidance logs will repeat on every 20s timeout instead of only once.

This issue also appears on line 120 of the same file.

                    if first_warning == False:
                        logger.info(f"SC will wait for {CACHE_MAX_WAIT} seconds before bypassing.")
                        logger.info(
                            "The cache is user specific, if you believe there is no way your cache "
                            f"should be in use you could try deleting the lock: {CACHE_LOCK_PATH}")

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/sc/clone/cloners/repo_cloner.py:35

• CACHE_MAX_WAIT is set to 300 seconds (5 minutes), but the PR description says the lock wait timeout is 10 minutes. This mismatch makes it hard to reason about expected behavior and user-facing timing.

CACHE_MAX_WAIT = 300

src/sc/clone/cloners/repo_cloner.py:116

• If the lock times out, the loop breaks without acquiring the lock but still logs "Cache lock acquired." and proceeds to use the cache concurrently. This both misleads users and defeats the point of cache locking; per the PR description it should error once the max wait is exceeded.

                else:
                    logger.warning(
                        f"Cache remained locked past the set wait time of {CACHE_MAX_WAIT} seconds. "
                        "Force proceeding without acquiring cache lock..."
                    )

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/sc/clone/cloners/repo_cloner.py:35

• This change doesn’t match the PR description: CACHE_MAX_WAIT is 300s (5 minutes), and _acquire_cache_lock returns False and proceeds without the lock rather than erroring out after 10 minutes. Please align either the PR description or the implementation (timeout value + fail vs bypass) so user-facing behavior is unambiguous.

CACHE_MAX_WAIT = 300

src/sc/clone/cloners/repo_cloner.py:95

• If the cache lock is not acquired (bypassed), this still calls _cache() and clones using the cache as a reference. That can lead to concurrent writes/reads of the same cache directory while another process is syncing it, which defeats the purpose of locking and can cause inconsistent/failed clones. When the lock is bypassed, clone without using/updating the cache.

        lock_acquired = self._acquire_cache_lock(lock)

        try:
            reference = self._cache()
            self._clone(directory, reference)

src/sc/clone/cloners/repo_cloner.py:35

• The cache lock file is global (~/.caches/.lock), but the cache itself is per manifest hostname (~/.caches/<hostname>/...). This means cloning from different hosts unnecessarily blocks on the same lock, and it also makes the warning about a “user specific” cache less precise. Consider moving the lock to be per-host (e.g., host_cache_dir / ".lock") so unrelated clones don’t contend.

CACHE_LOCK_PATH = REPO_CACHE_DIR / ".sc_lock"
CACHE_MAX_WAIT = 300

src/sc/clone/cloners/repo_cloner.py:184

• Minor: if first_warning == False: is non-idiomatic and easier to misread than if not first_warning:. Switching improves readability without changing behavior.

                    if first_warning == False:

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/sc/clone/cloners/repo_cloner.py:173

• lock.acquire(timeout=20) in a loop means total waiting time can exceed CACHE_MAX_WAIT by up to ~20 seconds (the acquire timeout interval). To enforce the max wait more precisely, compute the remaining wait time and pass that into acquire().

        while True:
            try:
                lock.acquire(timeout=20)

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/sc/clone/cloners/repo_cloner.py:195

• The bypass path logs "proceeding without acquiring cache lock", but it then breaks the lock and immediately acquires it. Also, this path currently returns None, so lock_acquired is falsey and the lock may not be released.

                    )
                    lock.break_lock()
                    lock.acquire()
                    return

src/sc/clone/cloners/repo_cloner.py:175

• _acquire_cache_lock() is annotated/documented to return bool, but the successful acquire path returns None. This makes lock_acquired falsey in _clone_with_cache() and the lock may never be released in the finally block, leaving the cache effectively permanently locked for subsequent runs.

This issue also appears on line 191 of the same file.

    def _acquire_cache_lock(self, lock: SoftFileLock):
        """Try to acquire cache lock. After a certain time bypass it."""
        started = time.monotonic()
        first_warning = False

[TB-1993]

Ship it!

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Suppressed comments (1)

src/sc/clone/cloners/repo_cloner.py:191

• The warning says it's proceeding without acquiring the cache lock, but the code breaks the lock and then acquires it (lock.break_lock(); lock.acquire()). Updating the message (and including the lock path) will make the behavior and recovery steps clearer.

                    logger.warning(
                        f"Cache remained locked past the set wait time of {CACHE_MAX_WAIT} seconds. "
                        "Force proceeding without acquiring cache lock..."
                    )