Fix #189: federation cache should now delete properly, including orphaned files

api/funkwhale_api/federation/tasks.py

@@ -1,8 +1,10 @@
 import datetime
 import json
 import logging
+import os
 
 from django.conf import settings
+from django.db.models import Q
 from django.utils import timezone
 
 from requests.exceptions import RequestException
@@ -96,16 +98,38 @@ def clean_music_cache():
     delay = preferences['federation__music_cache_duration']
     if delay < 1:
         return  # cache clearing disabled
+    limit = timezone.now() - datetime.timedelta(minutes=delay)
 
     candidates = models.LibraryTrack.objects.filter(
-        audio_file__isnull=False
+        Q(audio_file__isnull=False) & (
-    ).values_list('local_track_file__track', flat=True)
+            Q(local_track_file__accessed_date__lt=limit) |
-    listenings = Listening.objects.filter(
+            Q(local_track_file__accessed_date=None)
-        creation_date__gte=timezone.now() - datetime.timedelta(minutes=delay),
+        )
-        track__pk__in=candidates).values_list('track', flat=True)
+    ).exclude(audio_file='').only('audio_file', 'id')
-    too_old = set(candidates) - set(listenings)
+    for lt in candidates:
-
-    to_remove = models.LibraryTrack.objects.filter(
-        local_track_file__track__pk__in=too_old).only('audio_file')
-    for lt in to_remove:
         lt.audio_file.delete()
+
+    # we also delete orphaned files, if any
+    storage = models.LibraryTrack._meta.get_field('audio_file').storage
+    files = get_files(storage, 'federation_cache')
+    existing = models.LibraryTrack.objects.filter(audio_file__in=files)
+    missing = set(files) - set(existing.values_list('audio_file', flat=True))
+    for m in missing:
+        storage.delete(m)
+
+
+def get_files(storage, *parts):
+    """
+    This is a recursive function that return all files available
+    in a given directory using django's storage.
+    """
+    if not parts:
+        raise ValueError('Missing path')
+
+    dirs, files = storage.listdir(os.path.join(*parts))
+    for dir in dirs:
+        files += get_files(storage, *(list(parts) + [dir]))
+    return [
+        os.path.join(parts[-1], path)
+        for path in files
+    ]

api/tests/federation/test_tasks.py

@@ -1,4 +1,7 @@
 import datetime
+import os
+import pathlib
+import pytest
 
 from django.core.paginator import Paginator
 from django.utils import timezone
@@ -117,17 +120,16 @@ def test_clean_federation_music_cache_if_no_listen(preferences, factories):
     lt1 = factories['federation.LibraryTrack'](with_audio_file=True)
     lt2 = factories['federation.LibraryTrack'](with_audio_file=True)
     lt3 = factories['federation.LibraryTrack'](with_audio_file=True)
-    tf1 = factories['music.TrackFile'](library_track=lt1)
+    tf1 = factories['music.TrackFile'](
-    tf2 = factories['music.TrackFile'](library_track=lt2)
+        accessed_date=timezone.now(), library_track=lt1)
-    tf3 = factories['music.TrackFile'](library_track=lt3)
+    tf2 = factories['music.TrackFile'](
-
+        accessed_date=timezone.now()-datetime.timedelta(minutes=61),
-    # we listen to the first one, and the second one (but weeks ago)
+        library_track=lt2)
-    listening1 = factories['history.Listening'](
+    tf3 = factories['music.TrackFile'](
-        track=tf1.track,
+        accessed_date=None, library_track=lt3)
-        creation_date=timezone.now())
+    path1 = lt1.audio_file.path
-    listening2 = factories['history.Listening'](
+    path2 = lt2.audio_file.path
-        track=tf2.track,
+    path3 = lt3.audio_file.path
-        creation_date=timezone.now() - datetime.timedelta(minutes=61))
 
     tasks.clean_music_cache()
 
@@ -138,3 +140,32 @@ def test_clean_federation_music_cache_if_no_listen(preferences, factories):
     assert bool(lt1.audio_file) is True
     assert bool(lt2.audio_file) is False
     assert bool(lt3.audio_file) is False
+    assert os.path.exists(path1) is True
+    assert os.path.exists(path2) is False
+    assert os.path.exists(path3) is False
+
+
+def test_clean_federation_music_cache_orphaned(
+        settings, preferences, factories):
+    preferences['federation__music_cache_duration'] = 60
+    path = os.path.join(settings.MEDIA_ROOT, 'federation_cache')
+    keep_path = os.path.join(os.path.join(path, '1a', 'b2'), 'keep.ogg')
+    remove_path = os.path.join(os.path.join(path, 'c3', 'd4'), 'remove.ogg')
+    os.makedirs(os.path.dirname(keep_path), exist_ok=True)
+    os.makedirs(os.path.dirname(remove_path), exist_ok=True)
+    pathlib.Path(keep_path).touch()
+    pathlib.Path(remove_path).touch()
+    lt = factories['federation.LibraryTrack'](
+        with_audio_file=True,
+        audio_file__path=keep_path)
+    tf = factories['music.TrackFile'](
+        library_track=lt,
+        accessed_date=timezone.now())
+
+    tasks.clean_music_cache()
+
+    lt.refresh_from_db()
+
+    assert bool(lt.audio_file) is True
+    assert os.path.exists(lt.audio_file.path) is True
+    assert os.path.exists(remove_path) is False

changes/changelog.d/189.bugfix

@@ -0,0 +1 @@
+Federation cache suppression is now simpler and also deletes orphaned files (#189)