Interval fetch moves into the detached updater (try_hold_lock; sweep in dl --update-cache)

[blooop]

Question

The hourly broad fetch (git fetch origin +refs/heads/*:refs/heads/* --tags --prune, repo_manager.py:158) currently runs on the foreground launch path under the repo flock. Per the #144 decision, it belongs in the detached child devlaunch already spawns (update_cache_background, dl.py:408-436 → dl --update-cache, dl.py:2210). Make the child sweep the cache; leave the foreground untouched for now (the transient double-gate is harmless — last_fetched is shared).

• Add try_hold_lock to devlaunch/worktree/locks.py: non-blocking sibling of hold_lock (which already attempts LOCK_NB first, locks.py:56); yields held-or-not, never waits, never unlinks. Document the repo→metadata lock ordering as an invariant in the module docstring while there.
• In the --update-cache child, after update_completion_cache(): for each repo in storage.list_repositories(), under try_hold_lock on repo_manager.lock_path(...), run lazy_fetch; skip silently when contended or on fetch failure (it retries next interval).

Failing tests first: (1) child with elapsed interval fetches and advances last_fetched; (2) child with the repo lock held elsewhere skips without blocking and records no fetch; (3) child within interval fetches nothing. Seam: fake subprocess.run recording git argv, prior art test/test_devpod_spawn_counts.py.

Files: devlaunch/worktree/locks.py, devlaunch/dl.py (--update-cache branch ~:2210), devlaunch/worktree/repo_manager.py (no signature change here).

[blooop]

breadcrumb: build stage started — fresh wf-tdd subagent, branch perf/149-updater-fetch-sweep, base f03158b.

[blooop]

breadcrumb: resumed by a fresh wf-auto run. Verified against origin: branch perf/149-updater-fetch-sweep does NOT exist, no PR — the predecessor's build worktree was discarded with nothing pushed. Stage reset to ready; restarting the build stage from base main (f03158b) with a fresh wf-tdd subagent.

[blooop]

breadcrumb: building. Restating the contract: the broad interval fetch gains a second, non-blocking home in the detached dl --update-cache child; the foreground is left exactly as it is (the transient double-gate is harmless because last_fetched is shared state). The targeted foreground fetch and the workspace-clone fetch deletion stay with the sibling ticket.

Seams I will test at, both pre-agreed on the ticket:

1. The subprocess boundary — a fake subprocess.run recording every git argv, prior art test_devpod_spawn_counts.py. Three behaviours: elapsed interval → the broad refspec fetch runs and last_fetched advances; repo lock held by another open file description → nothing fetched and the sweep returns rather than waiting; within interval → nothing fetched. Plus one that the --update-cache entry point actually reaches the sweep, so the wiring is pinned and not just the helper.
2. The lock itself — try_hold_lock reports acquired-or-not, releases on exit, and leaves the lock file on disk (the never-unlink guarantee the module docstring already makes for hold_lock).

Fetch failure is swallowed by design: the next interval retries, and a detached child has nobody to tell.

[blooop]

breadcrumb: built — PR #161, CI green on every check (py310-py313, e2e, prek, gate).

The slice: the detached cache updater now sweeps the bare-clone cache after refreshing completions, taking each repo lock non-blockingly and running the existing interval-gated fetch. A repo a launch is holding is skipped, a failed fetch is stepped over, and the interval clock is the same shared one the launch path reads — so the transient double gate costs nothing and whichever side gets there first spares the other.

Two things deliberately left standing for the sibling ticket: the foreground still runs its own interval fetch, and the workspace-clone fetch is untouched. Nothing here changed lock placement, so that seam is clean.

Also written down while in locks.py: the repo-lock-before-metadata-lock ordering three call sites already depend on and nothing stated.

[blooop]

breadcrumb: build stage done — PR #161 open (#161), head ec08eee, all remote checks green (py310-313, e2e, prek, gate, GitGuardian, codecov) and MERGEABLE. Gate build→review passed, verified from GitHub state rather than the builder's say-so. Fresh wf-review subagent spawned. Seam left clean for #150: no lock placement changed, the foreground interval fetch and _prepare_workspace's unused workspace-clone fetch are untouched.

[blooop]

breadcrumb: review stage incomplete — stopped mid-run on a usage limit. No review was posted on PR #161 and there is no verdict. The node is still in-review, not approved.

What actually got done (all verified by me, not assumed):

• Preflight passed: head ec08eee resolves, merge-base is f03158b, diff is non-empty (5 files, +409/-1), all remote checks green, MERGEABLE. Existing PR comments are only codecov (all modified lines covered) and sourcery-ai, which was rate-limited and reviewed nothing — so PR Interval fetch moves into the detached updater #161 has had no substantive review from any source.
• Two axis subagents (Standards, Spec) were spawned in parallel and neither had reported when I was stopped. I could not stop them, so stray output may land after this comment — treat any such report as unreviewed and unsigned; it was never folded into a verdict.

Two findings I established first-hand by reading (devlaunch/worktree/locks.py, devlaunch/dl.py), offered as a starting point, not as a review:

1. hold_lock and try_hold_lock sit adjacent in locks.py and both yield a bare bool with inverted meaning: hold_lock yields True = contended (caller holds the lock either way), try_hold_lock yields True = acquired (False means the caller does not hold it). So False means "you hold it" in one and "you do not hold it" in the other. Worth a constructive-modeling look — test/unit/test_locks.py:60-63 flips polarity on adjacent lines.
2. On the "child bails before the sweep on the completion-cache TTL" concern, the builder’s defence is stronger than stated: the one non-forced spawn site (dl.py:2220) is itself gated by update_cache_background’s own completion_cache_is_fresh() check at dl.py:422, so a non-forced child is only ever spawned when the cache is already stale and will not bail. Every other spawn site (10 of them) passes force=True and appends --force. The residual concern is narrower: COMPLETION_CACHE_TTL_SECONDS is a hardcoded 3600 (dl.py:195) while the fetch interval is config-driven, so a user who lowers fetch_interval gets a sweep still gated by an unrelated, unconfigurable clock.

Next run should start by: re-running wf-review 161 from scratch in fresh context — do not trust the two points above as a completed axis, and do not carry over any late subagent output. The preflight can be taken as done.

[blooop]

breadcrumb (addendum, unsigned): the Standards subagent landed just after the stop. Recording it so the work is not lost. This is one raw axis, not a review — no Spec axis, no verdict, nothing posted on PR #161, node still in-review. It self-reports as incomplete: it stopped before running any test, pylint or pixi run ci.

Its findings, as it reported them:

• Its top finding, worth the next run chasing first: once the sweep does get the repo lock, lazy_fetch → fetch_repo → storage.add_repository → exclusive() takes the metadata lock blockingly (storage.py:105), and the git fetch itself is unbounded. So a foreground launch arriving after the sweep acquired the repo lock can block behind background work — which contradicts the new docstrings’ "It never waits" / "Background defers to foreground and never the reverse" (dl.py:452-456). Not a regression (the foreground paid this before), but the prose overstates. It marked this non-blocking; it may deserve a harder look.
• Lock-ordering invariant checks out against the three named call sites; no metadata-then-repo site found. Nits: fetch_repo/clone_repo do not take the repo lock themselves, and the docstring says "two kinds of lock" while a third (the launch lock, dl.py:1676) exists.
• subprocess.CalledProcessError is not caught by (ValueError, RuntimeError, OSError) directly, but fetch_repo:176 re-raises it as RuntimeError, so the handler is adequate.
• fd/lock safety sound: both arms close in finally, nothing unlinked, a missed fd’s close does not release the real holder. It verified by execution that distinct open file descriptions do conflict within one process, so the tests’ contention staging is valid.
• No before/after numbers anywhere (PR body, PR comments, Interval fetch moves into the detached updater (try_hold_lock; sweep in dl --update-cache) #149 comments) — a miss against the map rule "every perf change lands with before/after numbers".
• Not test-first by commit order: impl and tests land in the same two commits.

Explicitly not verified by it: the hold_lock/try_hold_lock duplication judgement, whether dl.py’s new top-level imports add startup cost (it noted workspace_clone.py already imported both, so likely zero — not measured), test flake behaviour, and CI/pylint.

Next run: still re-run wf-review 161 from scratch. Preflight is done; the Spec axis was never delivered.

[blooop]

handoff

Where we are: build done — PR #161 (#161), head ec08eee, all checks green and MERGEABLE; I verified the gate from GitHub state directly. A fresh wf-review subagent was spawned with the integrity instruction and was killed by the usage limit before posting any verdict — check the PR for a review comment before assuming one exists.

Open thread: the review is unrun. Specific things I asked that reviewer to be sceptical about, worth carrying over: whether the newly documented lock-ordering invariant (metadata lock may be taken while a repo lock is held, never the reverse) is actually true of the code rather than merely asserted; whether the flock contention test can wedge or flake in CI; whether a non-blocking miss can unlink another holder's lock file; and the deliberate choice that the sweep is skipped when the child exits early on the completion-cache TTL.

First move on resume: spawn a fresh two-axis review on PR #161. #150 stays blocked by this ticket, and its seam was left clean (no lock placement changed, foreground interval fetch and _prepare_workspace's unused fetch untouched).

[blooop]

breadcrumb (addendum 2, unsigned): the Spec subagent also landed after the stop, likewise partial and self-reported incomplete. Recording it so it is not lost. Still no verdict, nothing posted on PR #161, node still in-review.

Its findings, as it reported them:

• Scope boundary clean (pass). Diff touches only CHANGELOG.md, devlaunch/dl.py, devlaunch/worktree/locks.py and the two new test files. repo_manager.py and workspace_clone.py untouched — no Cold launch fetches exactly the requested ref; delete the workspace-clone fetch #150 creep, no signature change, matching Interval fetch moves into the detached updater (try_hold_lock; sweep in dl --update-cache) #149’s "no signature change here". Verified by git diff --stat f03158b...ec08eee.
• Spawn-site claim verified, and it agrees with what I found independently: 10 sites pass force=True, one passes the default (dl.py:2220, reached only for --ls/--repos/--completion-data/no-args), and that one is already gated by the same TTL in the parent at :422, so the child’s new gate at :2267 is near-redundant. It adds the precise coupling: COMPLETION_CACHE_TTL_SECONDS = 3600 is hardcoded (dl.py:195) while fetch_interval is user-configurable (worktree/config.py:24). Its leaning: non-blocking nit.
• The finding the next run must settle (it did not finish judging it): the sweep writes last_fetched through the same path the foreground reads (repo_manager.py:169,178-201; read by the foreground at :227 and workspace_clone.py:190). So a background sweep can consume the interval budget and make a later foreground launch skip a fetch it would have done before this PR — push-then-dl can land on a tip up to fetch_interval old where previously it fetched. Bounded by the existing interval contract and cured by Cold launch fetches exactly the requested ref; delete the workspace-clone fetch #150, but the CHANGELOG’s "transient and harmless" overstates it. Blocking-or-not is unresolved.
• Confirms independently: no before/after numbers anywhere, against the map rule.
• Explicitly not done: test/unit/test_locks.py unread, CHANGELOG claim ("on most machines the background child gets there first") unassessed.

Both axes are now on this ticket as raw partials. Neither was cross-checked, deduped, or turned into a verdict, and the two were never reconciled. Next run: re-run wf-review 161 from scratch — preflight is done; use these only as leads. The single most valuable thing to settle first is the last_fetched freshness question above.

[blooop]

breadcrumb: the review subagent confirmed it was killed with no verdict and nothing posted on PR #161 — it recorded three clearly-labelled partial states on this ticket rather than merging them into a verdict, which is the right behaviour. Note PR #161 has had no substantive review from any source: sourcery-ai was rate-limited and reviewed nothing.

Leads for the next reviewer, none of which is a finished review:

1. Possibly blocking — last_fetched freshness. The sweep writes last_fetched through the same path the foreground reads, so a background sweep can consume the interval budget and make a later launch skip a fetch it would otherwise have done. Push-then-dl could then land on a tip up to fetch_interval old — which touches Move the interval git fetch off the foreground launch path and out of the repo lock #144's stated guarantee directly. The CHANGELOG's "transient and harmless" likely overstates it. Resolve this first.
2. "It never waits" is overstated in the new docstrings. Once the sweep does acquire the repo lock, lazy_fetch → fetch_repo → storage.add_repository → exclusive() takes the metadata lock blockingly and the fetch is unbounded, so a foreground launch can block behind background work. Not a regression, but it contradicts dl.py:452-456.
3. No before/after numbers — a miss against the map rule, found independently by both axes.
4. Minor: hold_lock and try_hold_lock yield bare bools with inverted meaning (False = "you hold it" vs "you don't") — a principle-3 smell; and COMPLETION_CACHE_TTL_SECONDS is hardcoded 3600 while fetch_interval is configurable, entangling two unrelated clocks.

Verified good: the lock-ordering invariant checks out against all three named call sites; fd/lock safety is sound (nothing unlinked, a miss releases nothing); the flock contention staging in the tests is valid, confirmed by execution. Never verified: any test run, pylint, CI, or the duplication judgement.

[blooop]

breadcrumb: review stage complete on #161 — verdict Request changes (3 blocking Standards findings: inverted hold_lock/try_hold_lock bool polarity, the yielded bool being an ignorable sentinel, and sweep_repo_fetches bypassing _get_clone_manager()'s migration; Spec axis clean). Full two-axis report: #161 (review)