Refactor: extract a shared "atomic check-then-save" primitive from `_transfer_one` and `CacheStack._backfill`

[pmrv]

Location

• src/fleche/caches.py:69-98 — BaseCache._transfer_one (added by
  PR fix(query): make transfer check-then-save atomic via target per-key lock #630, closes Cache race: BaseCache.transfer() contains→save TOCTOU #452).
• src/fleche/caches.py:792-809 — CacheStack._backfill (added by
  PR fix(caches): serialize CacheStack.load() auto-transfer to base cache #629, closes CacheStack.load() auto-transfer to base cache is not serialized #217).

Problem

Both methods close the same class of TOCTOU race — "check if the key
is already present, and if not, save it, all under the per-key
_operation_context" — and they have near-identical bodies:

# _transfer_one  (PR #630, #452)
def _transfer_one(self, c: LazyCall, *, overwrite: bool = False) -> bool:
    key = c.to_lookup_key()
    with self._operation_context(key):
        if not overwrite and self.contains(key):
            return False
        self.save(c.fetch())
        return True

# CacheStack._backfill  (PR #629, #217)
def _backfill(self, key, lc: LazyCall) -> None:
    with self._operation_context(key):
        if self.stack[0].contains(key):
            return
        try:
            self.save(lc.fetch())
            logger.info("Transferred hit for %s from higher cache to base cache", key)
        except Rejected as e:
            logger.warning("Failed to transfer hit for %s to base cache: %s", key, e)

The template is: acquire per-key context → check presence →
optionally save → optionally handle Rejected. This exact shape is
what both #217 and #452 fixed independently. Every future call site
that wants "atomic write-if-absent" (a hypothetical bulk_transfer,
a manual bootstrap helper, or the BackgroundSaveMixin #447) will
have to re-derive it — with the same risk that the lock scope is
narrowed, the Rejected branch is forgotten, or the deferred
.fetch() cost gets paid on the skip path.

The subtle differences today:

• _transfer_one returns a bool so QueryIterator.transfer can log
  the skip on its own logger with the full lookup key.
  _backfill swallows Rejected and logs internally.
• _transfer_one supports overwrite=; _backfill never overwrites
  (stack[0] is the write target and back-fill is unconditional).
• _backfill checks self.stack[0].contains(key), not
  self.contains(key), because CacheStack.contains fans out over
  every member and would return True for the higher cache the hit
  came from. This is the point of _backfill and any generic
  primitive has to expose that seam.

Suggested refactor

Extract a private BaseCache._locked_save_if_absent template that
owns the lock, the presence check, and the save, with hooks for the
two variants:

def _locked_save_if_absent(
    self,
    key: str | Digest,
    fetch: Callable[[], Call],
    *,
    overwrite: bool = False,
    presence_check: Callable[[str | Digest], bool] | None = None,
    on_reject: Callable[[Rejected], None] | None = None,
) -> bool:
    check = presence_check or self.contains
    with self._operation_context(key):
        if not overwrite and check(key):
            return False
        try:
            self.save(fetch())
        except Rejected as e:
            if on_reject is None:
                raise
            on_reject(e)
            return False
        return True

Then:

def _transfer_one(self, c, *, overwrite=False):
    return self._locked_save_if_absent(
        c.to_lookup_key(), c.fetch, overwrite=overwrite,
    )

# CacheStack._backfill
def _backfill(self, key, lc):
    written = self._locked_save_if_absent(
        key, lc.fetch,
        presence_check=self.stack[0].contains,
        on_reject=lambda e: logger.warning(
            "Failed to transfer hit for %s to base cache: %s", key, e),
    )
    if written:
        logger.info("Transferred hit for %s from higher cache to base cache", key)

Pros

• Single home for the "atomic write-if-absent" race window — future
  sites (BackgroundSaveMixin Design: BackgroundSaveMixin — background I/O for cache saves #447, bulk transfers, remote-side
  bootstraps) get correctness for free.
• Both CacheStack.load() auto-transfer to base cache is not serialized #217 and Cache race: BaseCache.transfer() contains→save TOCTOU #452 fixes now live behind one primitive rather than
  two parallel implementations that could drift.
• Makes the intent explicit at each caller (they express the
  presence-check target and the Rejected policy), rather than
  burying it inside a per-method body.
• No change in public API; both _transfer_one and _backfill keep
  their signatures.

Cons / risks

• The helper carries three hooks (fetch, presence_check,
  on_reject) — the signature isn't small, and the temptation to
  pile on more knobs (deferred-fetch strategy, custom lock timeout,
  ...) is real. Worth capping at the two current variants.
• Both fixes are already regression-pinned
  (tests/regression/test_issue_217.py,
  tests/regression/test_issue_452.py), so the safety net is strong;
  but any behaviour drift here would be a concurrency regression and
  those are the ones you notice last.
• The _backfill info-log-on-success path is asymmetric with
  _transfer_one (which returns a bool and lets the caller log).
  Preserving both requires either the callback-on-success form above
  or a boolean return that _backfill inspects.

Complexity / maintainability

Low. The two methods are ~15 lines each and their shared shape is
obvious once you sit them side by side. Landing after #688 (which
factors _operation_context(key) out of every Cache method) will
compose naturally — the with at the top of the primitive is the
single seam that both refactors converge on.

Out of scope

• The Rejected-with-message asymmetry between evict and save
  (SshCache-side; see PR refactor(remote): collapse SshCache method bodies behind a client dispatcher #715) — separate concern.
• Whether _backfill should also honour overwrite — today it never
  does, and that's intentional (stack[0] is authoritative for
  writes).

---

Filed from a codebase-wide refactoring scan; not yet implemented.