Interval fetch moves into the detached updater

[blooop]

Closes #149.

The hourly +refs/heads/* freshness fetch gains a second home: the detached
dl --update-cache child devlaunch already spawns and forgets. This is the
first half of the #144 decision
— the sweep has to exist before the foreground can stop sweeping, which is
#150's job.

What changed

run_if_lock_free in worktree/locks.py — a non-blocking sibling of
hold_lock, which already attempts LOCK_NB first and then waits. It takes the
work rather than yielding a "did I get it" flag, so the not-acquired case has no
body to run: the lock is either held for the whole of the work or the work never
happens. Unlocked work is not expressible, which is what an earlier revision's
yielded boolean could not promise. Like hold_lock it is not reentrant and never
unlinks the lock file.

What that lock does and does not buy. It makes the caller never queue. It
does not make the lock cheap to hold: once the work starts this is an ordinary
exclusive lock, and it is the same one ensure_repo takes with blocking
hold_lock. So the honest statement is asymmetric — the sweep never queues for
a launch, but a launch can queue for the sweep — and that is now what the code,
the lock docstring and the changelog all say. An earlier revision of this branch
claimed in three places that the foreground never waits on the background, which
was not true.

It was reproduced with two real processes: a sweep holding the repo lock across
an 8s fetch made a foreground hold_lock wait 6.51s, printing dl: waiting for another dl run preparing owner/repo — an "other dl run" the user can neither
see nor Ctrl-C, since the child is spawned with start_new_session=True.
Untimed, that wait had no upper bound: fetch_repo's subprocess.run carried no
timeout=, so a remote that accepts a connection and then goes quiet holds the
repo for as long as the kernel keeps the socket. fetch_repo/lazy_fetch now
take an optional timeout, defaulting to None so the launch path is
byte-identical and stays #150's to decide, and the sweep passes 300s.

The module docstring also writes down the lock ordering that several call sites
already depend on and nothing stated: the metadata lock may be taken while a
repo lock is held; never the reverse. A single site taking them the other way
round would deadlock two dl runs against each other, and nothing would look wrong
at either site. The enumeration of sites is deliberately left out — it goes stale
the first time someone adds a writer.

sweep_repo_fetches() in dl.py, called by the --update-cache branch after
the completion refresh. For each repo in metadata it takes the repo lock
non-blockingly and runs the existing lazy_fetch — so the interval gating, the
refspec and the last_fetched bookkeeping are all unchanged code, just running
somewhere else. It reaches metadata through _get_clone_manager(), dl's single
construction point and where the one-shot cache migration runs, rather than
building its own storage: a detached child is the worst place to skip a
migration, since nobody is watching it write records in a shape the rest of dl no
longer reads.

A contended repo is skipped; a failed fetch is logged and stepped over, because
one unreachable remote must not cost every other repo its refresh and a detached
child has no terminal to complain to. A fetch that hits its deadline arrives as
subprocess.TimeoutExpired, which is a SubprocessError and not an OSError —
so fetch_repo converts it to RuntimeError like every other fetch failure,
without which it would have escaped the sweep's catch and ended the loop at the
first slow remote.

What has not changed yet

The foreground still runs its own interval fetch, and the sweep is a pure
optimisation — the launch path is correct whether or not the updater ever runs.
Both sides read the same last_fetched clock, so whichever gets there first
spares the other; the double gate is transient and harmless, and it is what makes
this landable on its own. #150 removes the foreground half and replaces it with a
single targeted fetch of the requested ref; _prepare_workspace's unused
workspace-clone fetch is also its to delete. Neither is touched here.

Tests

test/unit/test_locks.py — the lock's own promises: the work runs when the lock
is free, is held off entirely when another opener holds it (proved with a thread
and a deadline, so a regression fails instead of wedging the suite), the lock file
is left on disk, and a miss releases nothing out from under the real holder.

test/unit/test_updater_fetch_sweep.py — the child's behaviour pinned at the
subprocess boundary by argv, the seam test_devpod_spawn_counts.py established.
Past its interval → exactly the broad refspec fetch, and the shared clock
advances. Within its interval → nothing. Repo lock held by another opener →
nothing fetched, the clock untouched, and the child returns rather than queueing.
One unreachable remote → the next repo is still fetched. The background fetch
carries its bound and the launch path's does not. A fetch that hits the deadline
→ the next repo is still fetched. Plus the migration pins, and two guards that the
subprocess replacement cannot outlive the test that made it.

The updater runs inline under monkeypatch rather than on a daemon thread, with a
SIGALRM deadline standing in for a join, so a sweep that queued fails one test
instead of wedging the suite and no replacement can leak into a later test. The
clone-manager memo is reset per test by main's autouse fresh_clone_manager
fixture, and the suite refuses interactive git credentials so a stray real fetch
fails at once instead of reading as slow.

Green: pixi run test → 1336 passed, 23 deselected. pixi run lint → ruff, ty
and pylint 10.00/10 all clean. pixi run prek → all hooks pass.

[sourcery-ai]

Sorry @blooop, you have reached your weekly rate limit of 500000 diff characters.

Please try again later or upgrade to continue using Sourcery

[sourcery-ai]

Reviewer's Guide

Moves the hourly broad git fetch sweep off the foreground launch path into the detached dl --update-cache updater, using a new non-blocking lock helper and tests to ensure background work never blocks or interferes with launches while still respecting the existing fetch interval and metadata bookkeeping.

Sequence diagram for background updater cache fetch sweep

sequenceDiagram
    actor User
    participant dl_cli as _run_cli
    participant child_dl as update_cache_background
    participant sweep as sweep_repo_fetches
    participant storage as MetadataStorage
    participant repo_manager as RepositoryManager
    participant lock as try_hold_lock

    User->>dl_cli: dl <command>
    dl_cli->>child_dl: spawn dl --update-cache (detached)
    child_dl->>sweep: sweep_repo_fetches()
    sweep->>storage: list_repositories()
    storage-->>sweep: repositories
    loop each base_repo
        sweep->>repo_manager: lock_path(owner, repo)
        sweep->>lock: try_hold_lock(lock_path)
        alt lock acquired
            sweep->>repo_manager: lazy_fetch(owner, repo)
            repo_manager-->>sweep: update last_fetched
        else lock not acquired
            sweep-->>sweep: skip repo (no waiting)
        end
    end
    sweep-->>child_dl: background sweep completes

Loading

File-Level Changes

Change	Details	Files
Introduce a non-blocking lock helper for background work and document lock ordering invariants.	Add try_hold_lock context manager that attempts `LOCK_EX	LOCK_NBand yields a boolean indicating acquisition without ever blocking.</li><li>Ensuretry_hold_lockmirrorshold_locksemantics regarding non-reentrancy and not unlinking lock files, and that a miss does not release any existing lock.</li><li>Extendworktree/locks` module docstring to formalize repo-vs-metadata lock ordering (repo lock must always precede metadata lock) and non-reentrancy constraints.
Add a background sweep that fetches repositories whose interval has elapsed, driven by the detached updater.	Implement sweep_repo_fetches() in dl.py to instantiate MetadataStorage and RepositoryManager, iterate known repositories, take each repo lock via try_hold_lock, and call lazy_fetch when acquired. On lock contention, skip the repo and log a debug message without advancing its last_fetched clock; on fetch failures, log and continue to the next repo. Wire sweep_repo_fetches() into the --update-cache path in _run_cli, running it after completion cache refresh so the detached child handles both completions and freshness on the same hourly interval. Import try_hold_lock, RepositoryManager, and MetadataStorage into dl.py to support the sweep.	devlaunch/dl.py
Update changelog to describe the new background-driven freshness fetch behaviour.	Document that the hourly freshness fetch now runs in the background dl --update-cache child, which non-blockingly takes repo locks and skips contended repos. Clarify that both foreground and background still share the last_fetched interval gate for now, and that removing the launch-path fetch is planned as a follow-up.	CHANGELOG.md
Add tests to pin non-blocking lock semantics and the updater’s fetch sweep behaviour at the subprocess boundary.	Create test_unit/test_locks.py to validate try_hold_lock acquiring uncontended locks, reporting-but-not-waiting on held locks using a thread and timeout, releasing locks at block exit, preserving lock files, and never dropping another holder’s lock. Add test/unit/test_updater_fetch_sweep.py to exercise the detached updater: simulate cached repos with configurable last_fetched, record subprocess calls via a Subprocesses helper, and run dl --update-cache on a separate thread with a deadline to ensure non-blocking behaviour. Test that repos past the interval trigger the broad fetch, advance last_fetched, repos within the interval are skipped, held repo locks cause the sweep to skip without changing clocks, and a failing fetch for one repo does not prevent subsequent repos from being fetched and having their clocks advanced.	test/unit/test_locks.py test/unit/test_updater_fetch_sweep.py

Assessment against linked issues

Issue	Objective	Addressed	Explanation
#149	Add a non-blocking try_hold_lock to devlaunch/worktree/locks.py and document the repo→metadata lock ordering invariant in the module docstring.	✅
#149	In the dl --update-cache child, after update_completion_cache(), sweep the cache: for each repo from storage.list_repositories(), use try_hold_lock on repo_manager.lock_path(...) to run lazy_fetch, silently skipping when the lock is contended or the fetch fails.	✅
#149	Add tests ensuring (1) the detached updater fetches and advances last_fetched when the interval has elapsed, (2) it skips without blocking and does not record a fetch when the repo lock is held elsewhere, and (3) it performs no fetches when within the interval; plus tests for try_hold_lock’s non-blocking behavior.	✅

---

Tips and commands

Interacting with Sourcery

• Trigger a new review: Comment @sourcery-ai review on the pull request.
• Continue discussions: Reply directly to Sourcery's review comments.
• Generate a GitHub issue from a review comment: Ask Sourcery to create an
  issue from a review comment by replying to it. You can also reply to a
  review comment with @sourcery-ai issue to create an issue from it.
• Generate a pull request title: Write @sourcery-ai anywhere in the pull
  request title to generate a title at any time. You can also comment
  @sourcery-ai title on the pull request to (re-)generate the title at any time.
• Generate a pull request summary: Write @sourcery-ai summary anywhere in
  the pull request body to generate a PR summary at any time exactly where you
  want it. You can also comment @sourcery-ai summary on the pull request to
  (re-)generate the summary at any time.
• Generate reviewer's guide: Comment @sourcery-ai guide on the pull
  request to (re-)generate the reviewer's guide at any time.
• Resolve all Sourcery comments: Comment @sourcery-ai resolve on the
  pull request to resolve all Sourcery comments. Useful if you've already
  addressed all the comments and don't want to see them anymore.
• Dismiss all Sourcery reviews: Comment @sourcery-ai dismiss on the pull
  request to dismiss all existing Sourcery reviews. Especially useful if you
  want to start fresh with a new review - don't forget to comment
  @sourcery-ai review to trigger a new review!

Customizing Your Experience

Access your dashboard to:

• Enable or disable review features such as the Sourcery-generated pull request
  summary, the reviewer's guide, and others.
• Change the review language.
• Add, remove or edit custom review instructions.
• Adjust other review settings.

Getting Help

• Contact our support team for questions or feedback.
• Visit our documentation for detailed guides and information.
• Keep in touch with the Sourcery team by following us on X/Twitter, LinkedIn or GitHub.

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 94.73%. Comparing base (856e1b8) to head (b8a4914).

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #161      +/-   ##
==========================================
+ Coverage   94.67%   94.73%   +0.06%     
==========================================
  Files          21       21              
  Lines        2591     2621      +30     
==========================================
+ Hits         2453     2483      +30     
  Misses        138      138              

Files with missing lines	Coverage Δ
devlaunch/dl.py	93.56% <100.00%> (+0.10%)	⬆️
devlaunch/worktree/locks.py	84.37% <100.00%> (+8.18%)	⬆️
devlaunch/worktree/repo_manager.py	85.62% <100.00%> (+0.28%)	⬆️

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[blooop]

This was generated by AI during review.

Two-axis review of perf/149-updater-fetch-sweep @ ec08eee, fixed point = merge-base with main (f03158b), three-dot diff. Preflight: fixed point resolves, diff non-empty (5 files, +409/-1), and gh pr checks 161 reports every check pass (ci (py310) pass, e2e pass, gate pass, prek pass); actions/runs?head_sha=ec08eee → total_count: 3, so the checks are real and not absent.

The axes ran as independent subagents and are reported unmerged and unranked against each other.

---

Standards

• Inverted bool polarity between the two lock helpers — devlaunch/worktree/locks.py:51 vs :84. Both yield Iterator[bool], but True means "I had to wait" in hold_lock (yield waited, locks.py:77) and "I got it" in try_hold_lock (yield True on success, locks.py:108). The PR's own test shows the trap side by side: test/unit/test_locks.py asserts acquired is True, then contended is False, for the same successful outcome. Near-identical names, opposite semantics — principle 3 (constructive modeling) and Mysterious Name. blocking
• The yielded bool is an ignorable sentinel — locks.py:100-110. The block runs whether or not the lock was taken, so a caller that omits the if not acquired guard does the protected work unlocked and nothing reads as wrong at the call site. A shape where the body cannot run unguarded would make that state unrepresentable. blocking
• The sweep bypasses dl's single construction point — devlaunch/dl.py:466-471 builds its own MetadataStorage() + RepositoryManager(...), skipping _get_clone_manager() (dl.py:2126), which the code itself calls "dl's single construction point" and which runs migrate_cache under the metadata lock. The sweep therefore reads and writes metadata this process may not have migrated. It is also Feature Envy: it uses only repo_manager.lock_path and repo_manager.lazy_fetch and belongs on RepositoryManager. blocking
• The lock-ordering invariant is prose only, and its enumeration is already wrong — locks.py:26-39. Checked against the code: metadata-under-repo-lock happens at repo_manager.py:117 (_register_existing_bare), :143, :170, and workspace_clone.py:364 — four acquisitions, not the "Three call sites" the docstring names. remove_repository (repo_manager.py:344) and remove_worktree (workspace_clone.py:410) take metadata with no repo lock. "There are two kinds of lock in the cache" omits the per-workspace launch lock (dl.py:1676, PR #138). Nothing enforces the order. Good news, checked because the sweep depends on it: lazy_fetch (repo_manager.py:190) does not take the repo lock, so calling it under try_hold_lock is not a self-deadlock. non-blocking
• Unbounded fetch under the lock, now invisible — repo_manager.py:157 passes no timeout. A foreground ensure_repo can now block behind the background sweep, told only "waiting for another dl run" — naming a process the user cannot see or Ctrl-C. The normal-case wait is bounded by the shared _should_fetch clock; the hang case is new. non-blocking
• Error tuple too narrow, and duplicated — dl.py:481 catches (ValueError, RuntimeError, OSError). Metadata/JSON decode errors and non-OSError save() failures escape and kill the child mid-sweep, losing every remaining repo's refresh — exactly what the docstring promises will not happen. The same triple already appears at workspace_clone.py:191. non-blocking
• Docstring density — locks.py:83-99 is 17 prose lines for 11 code lines; dl.py:442-464 likewise. The why is genuinely high quality, but the same intent is restated in three places, and the finding above shows one copy has already gone stale relative to the code it describes. non-blocking
• Test timing crutches / patch leak — test/unit/test_locks.py:52 (wait(timeout=5)), test/unit/test_updater_fetch_sweep.py:121 (join(20.0)). Correct in intent, but patch("subprocess.run") is process-global and is entered inside a daemon thread: if the join times out, the thread survives and later restores subprocess.run out from under a subsequent test. Tests do follow the test_devpod_spawn_counts.py argv-pinning prior art, and grep '/home/' over the three-dot diff has no hits. non-blocking — but see the reproduced failure below.

Verified by execution (Standards axis): pixi run -e py310 pylint → Your code has been rated at 10.00/10, exit 0; pixi run -e py310 pytest test/unit -q → 322 passed in 32.09s; the two new files together → 11 passed (10 runs); one earlier run → 5 failed, 6 passed in 46.95s; grep '/home/' over the three-dot diff → no hits.
Not verified (Standards axis): full pixi run ci (format/ruff/ty/coverage), real multi-process lock behaviour, py311+ environments, the cause of the single failing run.

---

Spec

Spec claim	Status	Evidence
(i) broad interval fetch "moves into the detached dl --update-cache child"	Partial — added, not moved	sweep_repo_fetches() added in devlaunch/dl.py, called from the --update-cache branch after update_completion_cache(); the foreground ensure_repo _should_fetch/fetch_repo branch (repo_manager.py:222-228) is untouched
(ii) non-blocking try_hold_lock	Present	try_hold_lock in worktree/locks.py (LOCK_NB, yields bool, never unlinks); used by the sweep; repo→metadata ordering invariant added to the module docstring as the ticket asked
(iii) delete _prepare_workspace's workspace-clone fetch	Absent — deferred to #150	workspace_clone.py is not in the diff; #150 body: "Delete _prepare_workspace step 4 (workspace_clone.py:298-310)"
"push-then-dl lands on the pushed tip"	Absent — deferred to #150	#150: "remote gains a commit after the cache cloned → new workspace HEAD == remote tip"

The three ticket-mandated failing tests all exist in test/unit/test_updater_fetch_sweep.py, read rather than assumed:
(1) elapsed interval → test_a_repo_past_its_interval_is_fetched asserts the exact argv ["git","fetch","origin","+refs/heads/*:refs/heads/*","--tags","--prune"], and test_fetching_advances_the_shared_fetch_clock asserts last_fetched() > stale. (2) lock held elsewhere → test_a_repo_another_run_is_holding_is_skipped (no fetch, thread deadline proves non-blocking) plus test_a_skipped_repo_keeps_its_fetch_clock. (3) within interval → test_a_repo_within_its_interval_is_left_alone. The seam is the one the ticket named: patch("subprocess.run", side_effect=recorder.run) recording argv, per test_devpod_spawn_counts.py.

Findings:

• Spec-source discrepancy on "moves" — non-blocking. The map's recorded resolution says the fetch "moves into the detached dl --update-cache child"; ticket #149's own body says "Make the child sweep the cache; leave the foreground untouched for now (the transient double-gate is harmless — last_fetched is shared)". The diff follows the ticket, and both the PR body and CHANGELOG state the double-gate plainly. The resolution's phrasing overstates what #149 was scoped to do; #149's own contract is coherent, and the seam #150 builds on (unchanged lazy_fetch, unchanged last_fetched) is intact.
• No test points at the behavioural guarantee — non-blocking. "push-then-dl lands on the pushed tip" is correctly #150's: #149 changes no freshness guarantee, and #150 already names that exact integration test ("remote gains a commit after the cache cloned → new workspace HEAD == remote tip"). Nothing here should be blocked on it.
• The contention tests pass vacuously at merge-base — non-blocking. Nothing fetches at f03158b, so they are not red-before-green in the strict sense; confirmed they still discriminate by mutating the implementation.

Verified by execution (Spec axis): pixi run -e py310 pytest test/unit/test_updater_fetch_sweep.py test/unit/test_locks.py -q → 11 passed in 2.66s; red-before with merge-base dl.py/locks.py restored → 3 failed, 3 passed in 1.17s and ImportError: cannot import name 'try_hold_lock'; mutation try_hold_lock→hold_lock → 1 failed, 5 deselected in 26.00s.
Not verified (Spec axis): full pixi run -e py310 ci, the PR body's "1173 passed" and "pylint 10.00/10" claims, and the PR's real-git XDG_CACHE_HOME end-to-end check.

---

Reviewer's own verification (lead)

Because a reviewer on this map was previously caught reporting results it had not produced, here is exactly what I ran myself, with real output.

A failure I reproduced, independently of the Standards axis. My first run of the two new test files gave:

FAILED test/unit/test_updater_fetch_sweep.py::TestTheUpdaterSweepsTheCache::test_a_repo_past_its_interval_is_fetched
FAILED test/unit/test_updater_fetch_sweep.py::TestTheUpdaterSweepsTheCache::test_fetching_advances_the_shared_fetch_clock
FAILED test/unit/test_updater_fetch_sweep.py::TestTheUpdaterYieldsToLaunches::test_a_repo_another_run_is_holding_is_skipped
FAILED test/unit/test_updater_fetch_sweep.py::TestTheUpdaterYieldsToLaunches::test_a_skipped_repo_keeps_its_fetch_clock
FAILED test/unit/test_updater_fetch_sweep.py::TestTheSweepSurvivesABadRepo::test_a_failing_fetch_does_not_stop_the_next_repo
5 failed, 6 passed in 51.32s

with, on the last of them:

>       assert len(recorder.fetches) == 2
E       assert 0 == 2
...
INFO     devlaunch.worktree.repo_manager:repo_manager.py:172 Successfully fetched updates for owner/repo

The Standards axis hit the same signature (5 failed, 6 passed in 46.95s, same assert 0 == 2) in a separate process. Two independent observations, same signature. The captured log is the diagnostic: a fetch was reported successful in-process while the recorder saw nothing, which means subprocess.run was not patched at that moment — the run was making real network calls, which is what the ~50s wall time is.

What I then ruled out, each by execution:

• ordering (test_locks.py first vs. second): 11 passed in 3.37s, 11 passed in 3.20s, 11 passed in 3.19s
• an exported scratch XDG_CACHE_HOME: 11 passed in 7.10s, 11 passed in 1.11s
• the file in isolation, three times: 6 passed in 24.10s, 6 passed in 0.88s, 6 passed in 3.24s
• two concurrent pytest processes in one worktree: foreground 11 passed in 0.75s, background 322 passed in 1.51s
• a cold checkout — a brand-new worktree at ec08eee, first ever run: 11 passed in 2.32s

So I could not reproduce it on demand, and I am not claiming a root cause. The mechanism consistent with everything observed — and it is analysis, not a verified result — is the patch leak the Standards axis flagged independently: run_updater (test_updater_fetch_sweep.py:109-126) enters the process-global patch("subprocess.run") inside a daemon thread and joins with a 20s deadline. Under load, one slow test trips the deadline, the daemon thread survives the failed assertion, and when it later exits the with block it restores the real subprocess.run underneath the next test — turning one slow test into a cascade of tests that silently hit the network. Both observed failures were on loaded machines and both took ~50s. I flag this as a hypothesis worth ten minutes, not as a diagnosis.

I also confirmed by reading the source, not by grep-and-assume:

• hold_lock yields waited (locks.py:69,73,77) and try_hold_lock yields acquisition (locks.py:106,108) — the inverted polarity is real.
• _get_clone_manager (dl.py:2126) is the migration point the sweep does not go through — the bypass is real.
• the worktree was clean and at ec08eee (git status --porcelain empty) when the passing runs were made.

Not verified by me: the full pixi run ci pipeline end to end; the PR body's "1173 passed", "pylint 10.00/10" and real-git XDG_CACHE_HOME end-to-end claims (the Standards axis independently reproduced the pylint 10.00/10 and a 322-test unit green; I did not run the full ci task myself); any behaviour on py311/py312/py313 beyond CI's own green; and the patch-leak mechanism above.

---

Verdict

Request changes.

Blocking findings, all from the Standards axis (the Spec axis raised none):

1. try_hold_lock and hold_lock yield bools with opposite meanings under near-identical names (locks.py:51 vs :84) — a maintainability and constructive-modeling trap, and #150 will add callers.
2. The yielded bool is an ignorable sentinel (locks.py:100-110): the guarded block runs regardless, so an omitted if not acquired silently does unlocked work.
3. sweep_repo_fetches bypasses _get_clone_manager() (dl.py:466-471 vs dl.py:2126), so the detached child touches metadata without the migration that every other dl construction path performs.

Not blocking, but I would fix them before #150 builds on this seam: the stale lock-ordering enumeration in locks.py:26-39 (four acquisition sites and a third lock kind, not "three call sites" and "two kinds"), and the run_updater patch-leak hazard — a test harness that can put real network calls into a unit run is a poor foundation for #150's fetch pinning.

The Spec axis is otherwise clean: the sweep, the non-blocking lock, and all three ticket-mandated tests are present and assert what #149 asked for. The workspace-clone-fetch deletion and the push-then-dl guarantee are correctly #150's, not gaps here.

[blooop]

This was generated by AI during review.

Duplicate submission — this review was posted twice by a retry. The review of record is #pullrequestreview-4891152499; please read that one. Verdict there: Request changes.

[blooop]

This was generated by AI during review.

Re-verdict after the retry. Two-axis review of perf/149-updater-fetch-sweep @ 83b69e3, fixed point = merge-base with main (f03158b), three-dot diff. This is a fresh reviewer: I did not write this code and I did not write the prior review.

Preflight, confirmed myself: fixed point resolves; diff non-empty (6 files, +661/-2); gh api actions/runs?head_sha=83b69e3 → total_count: 3 (CI success, prek success, Auto-publish skipped), so the checks are real and not absent; gh pr checks 161 all pass across py310–py313, e2e, gate; mergeable: MERGEABLE, mergeStateStatus: CLEAN.

The axes ran as independent subagents and are reported unmerged and unranked.

---

Status of the three original blocking findings

#	Original finding	Status
1	Inverted bool polarity between hold_lock and try_hold_lock	Closed
2	The yielded bool is an ignorable sentinel — the block ran unlocked if a caller dropped the guard	Closed
3	sweep_repo_fetches bypasses _get_clone_manager(), and therefore migrate_cache	Closed

1 and 2 — closed, and closed at the right level. The helper was deleted rather than patched. run_if_lock_free(lock_path, work) -> bool (locks.py:86) takes the work instead of yielding a scope, so on the contended path there is no body to run: work() is reachable only after a successful LOCK_NB, inside the try/finally that closes the fd. Unlocked work is no longer expressible, which is the constructive-modeling answer rather than a better-documented sentinel — principle 3 satisfied on its own terms, not by test coverage.

Judging the shape as asked, not just the tests: the residual question is whether the ambiguity merely moved into the returned bool. I do not think it did. The return has exactly two states, "the work ran" / "the lock was busy", the name states the condition, and the single call site (dl.py:485) consumes it only for a logging.debug. The docstring's claim that "ignoring that answer is safe" is true — nothing is protected by it. A caller cannot misread False into doing the work anyway, because the caller never had the work to do. -> bool is not a boolean-blindness relapse here; it is a report with one consumer.

One residual, non-blocking: hold_lock still yields True for contended while run_if_lock_free returns True for uncontended. The trap the prior review named was the pair — same shape, same type, near-identical names — and all three of those are gone (context manager vs. plain function, different names, different questions). Worth noting only because hold_lock's yielded waited has no production consumer at all (storage.py:105, workspace_clone.py:184,243, repo_manager.py:219, dl.py:1818 are all bare with hold_lock(...):), and it predates this PR — so the one remaining value-yielding lock helper yields a value nobody reads. Not this PR's to fix.

3 — closed. sweep_repo_fetches now takes storage and repo_manager off _get_clone_manager() (dl.py:472-474), which is the migration point. Pinned by TestTheChildMigratesLikeEveryOtherRun, and the pin discriminates: reverting only the construction change fails exactly the two migration tests with assert 1 == 2 and the old clone directory still present — reproduced independently by both axes, under both a targeted and a full test/unit selection.

---

Standards

• Three places assert a safety invariant the code does not hold — locks.py:104 "it defers to the foreground, and the foreground never defers to it"; dl.py:454 "Background defers to foreground and never the reverse"; CHANGELOG.md:135 "the sweep defers to launches and never the other way round". The foreground does defer. I re-derived the path myself rather than taking the axis's word: ensure_repo takes the repo lock with blocking hold_lock (repo_manager.py:219); the sweep holds that same lock for the whole of lazy_fetch → fetch_repo, whose subprocess.run(["git","fetch",...]) has no timeout= (repo_manager.py:157-163); and lazy_fetch itself takes no lock (repo_manager.py:190-202), so the hold spans the entire network call. A launch colliding with the sweep waits an unbounded time, told dl: waiting for another dl run preparing owner/repo — naming a detached child spawned with stdout/stderr at DEVNULL and start_new_session=True (dl.py:428-433), which the user cannot see or Ctrl-C. This is precisely the builder's parked item (b), and the code documents its absence. blocking — see the verdict for why this one is not waved through.
• run_if_lock_free — "lock free" is a concurrency term of art (lock-free algorithms) meaning almost the opposite of what is meant here. run_if_lock_available / try_run_locked reads correctly. Mysterious Name. non-blocking
• reset_clone_manager (dl.py:2161) is public production API whose only caller is test/conftest.py. Mild test-induced design damage — but the alternative, a conftest reaching into dl._cache, is worse, and the memo genuinely is a real cross-test contamination source now that the sweep goes through it. A leading underscore would say what it is. non-blocking
• Duplicated mkdir/open/finally: os.close preamble between locks.py:69-71 and :106-108. Extract on the third. non-blocking
• The lock-ordering docstring is accurate now, and its refusal to enumerate the call sites is the right call — the prior review's complaint was that the enumeration had already gone stale. All acquisition sites take repo→metadata; none takes metadata→repo. resolved
• Parked item (a), the narrow (ValueError, RuntimeError, OSError) catch: agree with the parking. MetadataStorage._load is total by construction, save raises the OSError family, fetch_repo raises only ValueError/RuntimeError, and subprocess failures are OSError. Residual escape surface is negligible. non-blocking
• No hardcoded /home/<user> reintroduced: git diff f03158b...HEAD | grep '/home/' → no output (run by me).

Spec

Spec claim (#149)	Status	Evidence
"(1) child with elapsed interval fetches and advances last_fetched"	Met	test_a_repo_past_its_interval_is_fetched asserts the exact argv; test_fetching_advances_the_shared_fetch_clock asserts last_fetched() > stale
"(2) child with the repo lock held elsewhere skips without blocking and records no fetch"	Met, and genuinely pinned	fetches == [] plus clock unchanged; the non-blocking half proven by mutation (below)
"(3) child within interval fetches nothing"	Met, non-vacuous	Mutating lazy_fetch→fetch_repo turns it red
"Seam: fake subprocess.run recording git argv, prior art test_devpod_spawn_counts.py"	Met	Subprocesses.run installed via monkeypatch.setattr, assertions on argv
"Document the repo→metadata lock ordering as an invariant in the module docstring"	Met	locks.py:26-42
"leave the foreground untouched for now"	Met	Diff touches neither repo_manager.py nor workspace_clone.py; lazy_fetch/last_fetched unchanged, so #150's seam is intact

• The try_hold_lock → run_if_lock_free swap is legitimate supersession, not a spec deviation. #149's binding text is behavioural — "non-blocking sibling of hold_lock … never waits, never unlinks" — and all three hold and are pinned. "yields held-or-not" is an implementation shape, and that exact shape is what the prior review ruled a blocking defect. A ticket's suggested shape loses to a review finding against it. not a finding
• The PR body is stale. It still documents "try_hold_lock in worktree/locks.py" — a symbol this head deletes, with a test asserting its absence — describes property (2) as "proved with a thread and a deadline" when the harness is now inline plus SIGALRM, and claims "1173 passed" against a measured 1182. The body is what a reader reaches for first. Rewrite before merge. non-blocking
• The sweep sits behind the completion_cache_is_fresh() early return, so a fetch_interval shortened below COMPLETION_CACHE_TTL_SECONDS = 3600 is silently capped at an hour. Inherited from #149's own instruction to place it "after update_completion_cache()"; no regression. non-blocking
• reset_clone_manager() per test and GIT_TERMINAL_PROMPT=0 weaken no mandated pin — the reset is what makes the fetch and migration assertions non-vacuous; every mutation above went red with it in place. not a finding

---

Ruling on the network-escaping test, and on SIGALRM

The prior review reproduced a real escape — 5 failed, 6 passed in 51.32s, assert 0 == 2 alongside an in-process Successfully fetched updates for owner/repo, i.e. subprocess.run unpatched and the suite on the network — and could not reproduce it on demand, labelling the patch-leak explanation analysis, not diagnosis.

The fix is good enough. I accept it. The reasoning is not "the builder ran it a lot":

1. The mechanism is removed by construction, not hunted. A process-global replacement entered inside a daemon thread that can outlive its join is a sufficient cause for exactly the observed signature. monkeypatch.setattr(subprocess, "run", ...) is undone by pytest at teardown of the same test in the same thread, unconditionally. There is no longer a replacement that can survive its test, whatever else goes wrong.
2. The grep claim is true — I verified it myself, not by accepting it. grep -rn "threading\|Thread(\|thread" test/ --include=*.py matches in exactly two files, test/unit/test_locks.py and test/unit/test_updater_fetch_sweep.py, and nothing else in the whole test/ tree; grep -rn "multiprocessing\|ThreadPool\|concurrent.futures" test/ → no hits. The one surviving thread (test_locks.py:110) calls run_if_lock_free and patches nothing; the sweep file's only use of threading is get_ident.
3. The property, not the mechanism, is now pinned. test_the_updater_runs_in_the_thread_that_replaced_subprocess_run asserts every subprocess.run the updater makes comes from the test's own thread, and test_the_real_subprocess_run_is_back_once_a_test_ends asserts the real one is restored. A different mechanism producing the same signature would still have to defeat those. That is what makes an unreproduced flake closable.

Residual risk accepted: nobody has demonstrated that the observed event had this cause, and the claim is structural rather than statistical. The builder says so plainly, which is the right way to report it. GIT_TERMINAL_PROMPT=0 is a sound complement — it converts a future escape from a 50-second "slow run" into an immediate failure at the command that left the machine.

SIGALRM — acceptable, and better than the thread it replaces. Judged rather than waved through:

• It does the job the 20s join did. A genuinely blocking sweep blocks forever on a held lock, not merely slowly, so a 30s wall-clock alarm is a real pin and not a perf budget. Proven by mutation (LOCK_EX | LOCK_NB → LOCK_EX): the sweep file goes 2 failed, 8 passed in 63.45s, both with AssertionError: the cache updater blocked instead of moving on, the frame at locks.py … run_if_lock_free. Per PEP 475 the raising handler defeats the automatic EINTR retry, which is why the blocked flock actually surfaces.
• Its constraints do not bite in this repo as configured — I checked rather than assumed: pyproject.toml has no pytest-xdist or pytest-timeout configuration, neither plugin is importable in the py310 environment, addopts = "-m 'not e2e'", and ci runs coverage run -m pytest — serial, main thread. signal.signal off the main thread would raise; nothing here runs off it.
• Latent, non-blocking: the harness is silently main-thread-only and owns ITIMER_REAL for the duration. Adding -n auto later, or any second user of ITIMER_REAL, breaks it in a confusing way. One comment line saying "main thread only" would fix that.
• Observed, worth a look, non-blocking: the deadline discipline is applied in one file and not the other. Under my own blocking-flock mutation I ran the combined selection test_updater_fetch_sweep.py test_locks.py and it did not terminate within my 400s cap — I killed it, so I have no result from that run, only the non-termination. The sweep file alone fails cleanly in ~63s. The likely reason, by reading and explicitly analysis rather than diagnosis: test_locks.py has no deadline(), and test_a_lock_not_acquired_is_not_released_out_from_under_its_holder calls run_if_lock_free on a lock the same process holds, which a blocking regression wedges permanently. Since wedging-instead-of-failing is the exact hazard deadline() exists to prevent, the new file's discipline is worth extending to the old one.

---

What I re-derived myself, and what I took from the axes

Re-derived by me (lead), with real output:

• pixi run -e py310 ci in a clean worktree at 83b69e3 → exit 0; 75 files left unchanged; ruff All checks passed!; Your code has been rated at 10.00/10 (previous run: 10.00/10, +0.00); ty All checks passed!; 1182 passed, 23 deselected in 73.20s (0:01:13); TOTAL coverage 93%. The builder's CI claim, including the 1182, is confirmed by my own run — not accepted from its paste.
• The thread/patch grep claim (commands and results above).
• The pytest parallelism configuration and what ci actually executes.
• The who-waits-on-whom path by reading: repo_manager.py:219 blocking hold_lock, :157-163 untimed subprocess.run, :190-202 lazy_fetch takes no lock (so no self-deadlock, and the hold spans the whole fetch), dl.py:428-433 child stdio.
• run_if_lock_free's shape and its single call site; that hold_lock's yielded value has no production consumer.
• grep '/home/' over the three-dot diff → no hits.

Taken from the axes — each ran it independently, and the two agree test-name for test-name, but I did not run these two mutations myself:

• Mutation A (work() also called on the BlockingIOError path): 3 failed, 328 passed under full test/unit — test_work_is_held_off_entirely_when_another_run_holds_the_lock, test_a_repo_another_run_is_holding_is_skipped, test_a_skipped_repo_keeps_its_fetch_clock.
• Mutation B (revert only the _get_clone_manager() construction): 2 failed, 329 passed under full test/unit — the two TestTheChildMigratesLikeEveryOtherRun tests, assert 1 == 2.
• Greens under stated selections: 20 passed (the two files alone), 331 passed (full test/unit), 1182 passed, 23 deselected (full default suite). No guard was red only in isolation; both axes checked both selections.
• The blocking-flock mutation result (2 failed, 8 passed in 63.45s) is the Spec axis's; my own attempt at it did not complete, as described above.

Accepted as covered by the prior review, not re-derived: its non-blocking findings on docstring density and the then-stale lock-ordering enumeration (the latter is now accurate, which I did check).

Not verified by anyone here: py311/py312/py313 beyond CI's own green; the e2e suite locally; prek locally; the PR body's "verified end to end against real git in a scratch XDG_CACHE_HOME"; real multi-process contention between a live sweep and a live launch — the user-visible stall in the blocking finding is derived by reading, not reproduced; the cause of the original 5-failure escape, which remains unexplained by anyone.

Merge-order note, not a finding against this PR: sibling PR #147 adds a clone-manager reset to this same test/conftest.py under a different name (invalidate_clone_manager() plus an autouse fresh_clone_manager fixture) where this PR adds reset_clone_manager(). Both are correct alone; together they are a textual conflict and one concept under two names. Whichever lands second should adopt the first's name rather than keeping both.

---

Verdict

Request changes — narrowly, on one finding.

Blocking:

1. locks.py:104, dl.py:454 and CHANGELOG.md:135 assert that the foreground never waits on the background, and it does. ensure_repo blocks on the repo lock (repo_manager.py:219) while the sweep holds it across an untimed git fetch (repo_manager.py:157). Either bound the sweep's fetch or correct all three statements to say what is true: the sweep never queues for a launch, but a launch can still queue for the sweep.

I want to be plain about why this blocks when the prior review ruled the same behaviour non-blocking, and about what it is not. It is not a defect in the sweep, and I am not re-litigating the parked limitation on its own. What changed is the frame: the builder parked it on the grounds that it is "not in the seam #150 builds on", and that rationale does not hold — #150 replaces the foreground fetch with a targeted one, so who-waits-on-whom is the seam, and #150 is blocked on this ticket and will be read alongside these three sentences. This PR argues, in its own words, that "a stale list is worse than none because it is what the next reader trusts"; the same standard applied to its own prose is what produces this finding. The fix is a few lines of prose, or one timeout= — cheap now, and mispriced later once a false invariant has been built on.

Everything else is non-blocking, and the substance of this PR is in good shape. All three original findings are closed, and closed by construction rather than by documentation: the ignorable sentinel is gone because there is no longer a body that can run unlocked, and the migration bypass is gone through dl's single construction point. I proved both pins discriminate by mutation rather than trusting the pasted evidence, under both isolated and full-suite selections, and I reproduced the full CI green myself. The network-escaping harness is a legitimate close: honest non-reproduction plus removal of a sufficient mechanism plus a direct pin on the property is the right answer to an unreproducible flake, and the grep that supports it holds up.