From 36f9aefb76ab18f6b599e4a3cfc94116ac602712 Mon Sep 17 00:00:00 2001 From: npeter83 Date: Sat, 14 Nov 2026 19:05:43 +0100 Subject: [PATCH] fix(downloads): a file asked for again during an over-cap pass stays whatever the clock says The eviction pass called a file freshly used only when its last-access stamp was later than the pass's own clock. A hit stamped just before the pass began but committed after its SELECT, or stamped by a clock that had stepped back (the test VM's does), read as old, and the file the user had just asked for was evicted. The pass now keeps the stamp its SELECT saw and skips a file whose stamp changed since. --- backend/app/downloads/gc.py | 29 ++++++++++++++--------------- backend/tests/test_file_folders.py | 22 ++++++++++++++++++++++ 2 files changed, 36 insertions(+), 15 deletions(-) diff --git a/backend/app/downloads/gc.py b/backend/app/downloads/gc.py index 9cfd052f..3759dd10 100644 --- a/backend/app/downloads/gc.py +++ b/backend/app/downloads/gc.py @@ -282,33 +282,32 @@ def run_download_gc(db: Session) -> dict: kept_assets = select(DownloadJob.asset_id).where( DownloadJob.kept.is_(True), DownloadJob.asset_id.is_not(None) ) - candidate_ids = ( - db.execute( - select(MediaAsset.id) - .where( - MediaAsset.status == "ready", - MediaAsset.source_kind != "upload", - MediaAsset.id.not_in(kept_assets), - ) - .order_by(MediaAsset.last_access_at.asc().nulls_first()) + candidates = db.execute( + select(MediaAsset.id, MediaAsset.last_access_at) + .where( + MediaAsset.status == "ready", + MediaAsset.source_kind != "upload", + MediaAsset.id.not_in(kept_assets), ) - .scalars() - .all() - ) + .order_by(MediaAsset.last_access_at.asc().nulls_first()) + ).all() db.rollback() - for asset_id in candidate_ids: + for asset_id, seen_access in candidates: # Counted afresh before each: a file deleted since (this one, an upload, a candidate # further down) has freed its space, and no other user's file goes for that space. if _on_disk(db) <= cap: break # Locked and re-read like an expiry: since the SELECT a holder may have kept it, - # every holder deleted it, or someone asked for it again (a fresh cache hit). + # every holder deleted it, or someone asked for it again (a fresh cache hit). A hit + # is a CHANGED stamp, not one later than this pass's clock: a hit stamped just + # before the pass began but committed after the SELECT is still a hit, and a clock + # that steps back would make a fresh one look old. asset, holders = _relock_evictable(db, asset_id) if ( asset is None or asset.status != "ready" or any(j.kept for j in holders) - or (asset.last_access_at is not None and asset.last_access_at > now) + or asset.last_access_at != seen_access ): db.commit() continue diff --git a/backend/tests/test_file_folders.py b/backend/tests/test_file_folders.py index b13b6f79..46813297 100644 --- a/backend/tests/test_file_folders.py +++ b/backend/tests/test_file_folders.py @@ -739,6 +739,28 @@ def test_a_file_asked_for_again_while_the_cache_is_over_its_cap_stays( assert (_root / "old.mp4").exists() +@pytest.mark.parametrize("stamped", ["before_the_pass", "by_a_clock_that_stepped_back"]) +def test_a_cache_hit_stamped_earlier_than_the_pass_still_keeps_the_file( + db, make_user, _root, monkeypatch, stamped +): + # A hit is a stamp that changed since the pass's SELECT. Compared with the pass's own clock, a + # hit stamped before the pass began (committed after its SELECT), or by a clock that had stepped + # back, read as old and the file the user just asked for went. + jold, jnew = _two_over_the_cap(db, make_user, _root) + seen = NOW - timedelta(days=9) + stamp = seen + (timedelta(hours=1) if stamped == "before_the_pass" else timedelta(days=9)) + + def asked_again(other, asset_id): + if asset_id == jold.asset_id: + other.get(MediaAsset, asset_id).last_access_at = stamp - timedelta(seconds=1) + + _meanwhile(monkeypatch, asked_again, step="_relock_evictable") + _gc(db) + db.expire_all() + assert db.get(DownloadJob, jold.id) is not None + assert (_root / "old.mp4").exists() + + def test_a_file_asked_for_while_its_cached_copy_is_freed_gets_a_fresh_one(db, make_user): # The enqueue read the cached copy unlocked: freed meanwhile, its job pointed at a row that was gone # (a foreign-key failure, a 500).