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

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).

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

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.

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:

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.
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.

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.

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.

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:

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.
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.

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.

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).

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.

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:

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.
"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.
No before/after numbers — a miss against the map rule, found independently by both axes.
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.

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)

breadcrumb: Review of #161 requested changes on three constructive-modeling
findings, all Standards; the Spec axis raised nothing. All three are addressed,
each with a test that goes red without the fix.

The two lock findings were one defect. The blocking helper reports whether it
had to wait; the non-blocking one reported whether it acquired — opposite
polarity under near-identical names — and its block ran either way, so a caller
who dropped the flag did the protected work unlocked while the call site read as
correct. Both are gone because there is no flag: the non-blocking helper now
takes the work rather than yielding a scope, so a contended lock has no body to
run and no guard to forget. It still answers whether the work ran, but that
answer reports the past instead of protecting anything, so dropping it costs a
log line rather than the lock. Pinned by a test that a contended call performs
none of the work, one that the lock really is held while the work runs, and one
that the acquire-or-not context manager is not back. Mutating the helper so the
work runs on a miss fails the first of those.

The detached child no longer has its own way into metadata. It built its own
storage, which made it the one process writing records without the one-shot
cache migration every other path runs — the worst place to skip it, since nobody
reads a detached child's output and the damage would surface later somewhere
else. It now goes through the same construction point as everything else. Pinned
by two tests over a cache written by an older devlaunch: after the child runs,
the old-scheme clone directory has been renamed and the on-disk schema header is
current. Both fail against the previous shape.

The network-escaping test. The reviewer reproduced a failure twice, in
separate processes, where the recorder saw no fetch while the code logged a
successful one — the subprocess boundary had been un-replaced mid-test and the
unit run went to the network. It could not be reproduced on demand and I did not
try to; I removed the construction that made it possible instead. The updater
test entered a process-global replacement of the subprocess boundary inside a
daemon thread with a join deadline, so a thread that outlived its join could put
the real boundary back underneath a later test. There is now no thread: the
updater runs in the thread that made the replacement, the replacement is undone
by the test framework at teardown of that same test, and a wall-clock deadline
does the job the join was there for — a sweep that queued behind a held lock
still costs one failed test rather than a wedged suite. Two supporting changes:
the memoized cache manager is reset per test, because it binds to a cache
directory the suite moves between tests, and the suite now refuses interactive
git credentials so a stray real fetch fails immediately instead of reading as a
slow test. Grepping the suite, this was the only thread-plus-global-replacement
construction in it.

I cannot claim the flake is proven dead — nobody has a reproduction to re-run.
What I can say is what I ran: full CI green end to end on py310 and py313
(1182 passed each, pylint 10.00/10, ruff/ty clean), plus 15 runs of the two
affected files, three concurrent whole-directory unit runs under load, and the
pre-commit suite. No run escaped its mock, including one that took 16s under
load. The claim is structural, not statistical: the shape that could leak is
gone.

Also fixed, from the non-blocking list: the lock-ordering docstring enumerated
three acquisition sites when there are four and named two kinds of lock when
there are three. The enumeration is deleted rather than corrected — a list like
that goes stale the next time someone adds a writer, and a stale list is what
the next reader trusts. The invariant itself stays.

Parked, unchanged and still true: the sweep's error tuple is narrower than
everything a mid-sweep failure can raise, and an unbounded fetch under the repo
lock can now make a foreground launch wait on a process the user cannot see.
Both were called non-blocking and neither is in the seam #150 builds on.

breadcrumb: re-verdict after retry — Request changes (narrow, one finding): all three original blocking findings are closed (pins mutation-proven, full pixi run -e py310 ci reproduced green by the reviewer, 1182 passed / pylint 10.00/10); the remaining blocker is that locks.py:104, dl.py:454 and CHANGELOG.md:135 assert the foreground never waits on the background while ensure_repo blocks on the repo lock the sweep holds across an untimed git fetch. #161 (review)

handoff

Where we are. PR #161 is open on perf/149-updater-fetch-sweep, head 83b69e3, mergeable CLEAN, all checks green, and carries a second request-changes verdict (re-verdict). Per LIFECYCLE's gate-failure rule — one retry, then park — this node is parked. It is not abandoned and it is not failing: the substance is nearly done.

What is settled and must not be redone. All three original findings are closed by construction, not by documentation, and the reviewer verified that structurally rather than taking the builder's word:

try_hold_lock is deleted. run_if_lock_free(lock_path, work) takes the work instead of yielding a scope, so work() is reachable only after a successful LOCK_NB and unlocked work is no longer expressible. The ambiguity did not migrate into the return: two states, the name states the condition, and the single call site uses it only for a logging.debug.
The migration bypass is gone — sweep_repo_fetches takes storage/repo_manager from _get_clone_manager(), pinned by a test that seeds a pre-Build: parse workspace ids at one boundary (injective scheme) #64 cache and asserts the on-disk header reaches SCHEMA_VERSION.
The network-escaping test is fixed by removing the construction, not hunting the race: no thread, monkeypatch undone at teardown in the same thread, and a SIGALRM wall-clock deadline replacing the 20s join. The reviewer independently re-ran the grep (threading matches two test files tree-wide; the surviving thread patches nothing; no multiprocessing/concurrent.futures anywhere in test/) and accepted the fix, while recording that nobody ever explained the original escape event — that residual is accepted, not solved.

The open thread — one finding, and it is real. locks.py:104, dl.py:454 and CHANGELOG.md:135 all assert the foreground never waits on the background. It can: ensure_repo blocks on the repo lock (repo_manager.py:219) that the sweep holds across an untimed git fetch (repo_manager.py:157). So a foreground launch can stall on a process the user cannot see or Ctrl-C, while three places in the tree document that this cannot happen.

This was the builder's own parked item (b), dismissed as "not in the seam #150 builds on". That rationale is wrong — who-waits-on-whom is exactly that seam, and #150 builds directly on it. Being a consciously-dismissed item rather than a fresh discovery is why this node parked instead of getting a second retry.

First move on resume. Decide which way the claim resolves, because both are defensible and the choice is a design call, not a bug fix:

Make the claim true — bound the fetch under the repo lock (the sweep already has deadline() machinery), so the foreground's wait is bounded and interruptible; or
Make the docs true — retract the "never waits" claim in all three places and state the real contract, then ticket the bounded-wait work separately.

Option 1 is the better destination; option 2 is the smaller honest step. Do not merge with the claim as written either way.

Also waiting on this node. A merge-order hazard: PR #147 adds dl.invalidate_clone_manager() plus an autouse fresh_clone_manager fixture to the same test/conftest.py where this PR adds dl.reset_clone_manager(). Both are correct alone; together they are a textual conflict and two names for one concept. Whichever merges second must adopt the first's name rather than adding a parallel helper.

Non-blocking, recorded so it is not rediscovered. SIGALRM is silently main-thread-only and owns ITIMER_REAL (one comment would fix); the deadline discipline exists in one file but not the other; hold_lock still yields True=contended against run_if_lock_free's True=uncontended, though the trap was the pair and that is gone; hold_lock's yielded waited has no production consumer at all (pre-existing); the sweep's (ValueError, RuntimeError, OSError) catch is narrower than what a mid-sweep failure can raise.

Not verified by anyone, so do not inherit it as fact: py311/312/313 beyond CI's green; e2e and prek locally; the PR body's real-git XDG_CACHE_HOME end-to-end claim; and the blocking finding's user-visible stall, which is derived by reading, not reproduced. One reviewer mutation run did not terminate within a 400s cap and was killed, so it yielded no result — the likely cause (test_locks.py has no deadline()) is analysis, not diagnosis.

breadcrumb (manager): the merge-order hazard named in the handoff above is now decided by events — PR #147 merged first (cc95eec), so main now carries dl.invalidate_clone_manager() plus the autouse fresh_clone_manager fixture in test/conftest.py.

When this node resumes, PR #161 must rebase onto that and adopt the existing helper, deleting its own dl.reset_clone_manager() rather than landing a second name for the same concept. The reason #161 reached for one still stands — routing the sweep through the clone manager makes a stale memo a live contamination source — but that need is now already met by the merged fixture, so this should be a deletion, not a reconciliation.

Navigation Menu

Repository navigation

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

Question

handoff

handoff

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

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

Description

Question

Activity

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

handoff

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

blooop commented on Aug 9, 2026

handoff

blooop commented on Aug 9, 2026

Metadata

Metadata

Assignees

Labels

Projects

Milestone

Relationships

Development

Issue actions