From 5bed8cb7a9362d1c0549f51b6532d969c1e0d0ff Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 14 Nov 2024 13:13:23 -0500
Subject: [PATCH 01/12] Core setup

---
 librarian_background/rolling_deletion.py | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)
 create mode 100644 librarian_background/rolling_deletion.py

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
new file mode 100644
index 0000000..bd8a964
--- /dev/null
+++ b/librarian_background/rolling_deletion.py
@@ -0,0 +1,18 @@
+"""
+A (very) dangerous task that you may have to use. This task will delete files that
+are older than a certain age, subject to some (optional) constraints:
+
+a) The file must have $N$ remote instances available throughout the network
+b) The checksums of those files must match the original checksum
+"""
+
+from .task import Task
+
+
+class RollingDeletion(Task):
+    """
+    A background task that deletes _instances_ (not files!) that are older than
+    a certain age.
+    """
+
+    pass

From 81bc3915aaf1916b06084f4470098cf1f1ffed4d Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 14 Nov 2024 13:53:24 -0500
Subject: [PATCH 02/12] Added initial deletion code

---
 librarian_background/rolling_deletion.py | 165 ++++++++++++++++++++++-
 librarian_server/orm/instance.py         |   8 +-
 2 files changed, 171 insertions(+), 2 deletions(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index bd8a964..dbf4656 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -6,6 +6,18 @@
 b) The checksums of those files must match the original checksum
 """
 
+import time
+from datetime import datetime, timedelta, timezone
+
+from loguru import logger
+from schedule import CancelJob
+from sqlalchemy import select
+from sqlalchemy.orm import Session
+
+from librarian_server.api.validate import calculate_checksum_of_remote_copies
+from librarian_server.database import get_session
+from librarian_server.orm import Instance, Librarian, StoreMetadata
+
 from .task import Task
 
 
@@ -15,4 +27,155 @@ class RollingDeletion(Task):
     a certain age.
     """
 
-    pass
+    store_name: str
+    "Name of the store to delete instances from"
+    age_in_days: int
+    "Age of the instances to delete, in days; older files will be deleted if they pass the checks"
+
+    number_of_remote_copies: int = 3
+    "Number of remote copies that must be available to delete the file"
+    verify_downstream_checksums: bool = True
+    "Whether to verify the checksums of the remote copies"
+    mark_unavailable: bool = True
+    "Whether to mark the instances as unavailable after deletion, or to delete them (False)"
+    force_deletion: bool = True
+    "Whether to over-ride the deletion policy of instances"
+
+    def get_store(self, name: str, session: Session) -> StoreMetadata:
+        possible_metadata = session.query(StoreMetadata).filter_by(name=name).first()
+
+        if not possible_metadata:
+            raise ValueError(f"Store {name} does not exist.")
+
+        return possible_metadata
+
+    def on_call(self):
+        with get_session() as session:
+            return self.core(session=session)
+
+    def core(self, session: Session):
+        core_begin = datetime.now(timezone.utc)
+        age_cutoff = core_begin - timedelta(days=self.age_in_days)
+
+        try:
+            store = self.get_store(self.store_name, session)
+        except ValueError as e:
+            logger.error("Error getting store: {}, cannot continue; cancelling job", e)
+            return CancelJob
+
+        logger.info(
+            "Beginning rolling deletion for store {} (ID: {})", store.name, store.id
+        )
+
+        # Get the instances that are older than the age
+
+        query_begin = time.perf_counter()
+        stmt = select(Instance).filter(
+            Instance.store_id == store.id,
+            Instance.created_time < age_cutoff,
+            Instance.available == True,
+        )
+
+        instances = session.execute(stmt).scalars().all()
+        query_end = time.perf_counter()
+
+        logger.info("Queried for old instances in {} seconds", query_end - query_begin)
+
+        logger.info(
+            "Found {} instances that are older than {} days",
+            len(instances),
+            self.age_in_days,
+        )
+
+        deleted = 0
+        for instance in instances:
+            # Check that we got what we wanted.
+            try:
+                assert instance.created_at < age_cutoff
+                assert instance.store_id == store.id
+                assert instance.available
+            except AssertionError:
+                logger.error(
+                    "Instance {} does not meet the criteria, skipping", instance.id
+                )
+                continue
+
+            # Check if the file associated with the instance has enough copies.
+            remote_librarian_ids = {
+                remote_instance.librarian_id
+                for remote_instance in instance.remote_instances
+            }
+
+            logger.info(
+                "Calling up {} remote librarians to check for copies",
+                len(remote_librarian_ids),
+            )
+
+            downstream = []
+
+            for librarian_id in remote_librarian_ids:
+                stmt = select(Librarian).filter(Librarian.id == librarian_id)
+                librarian = session.execute(stmt).scalar()
+
+                if not librarian:
+                    continue
+
+                downstream += calculate_checksum_of_remote_copies(
+                    librarian=librarian, file_name=instance.file_name
+                )
+
+            # Now check if we have enough!
+            if len(downstream) < self.number_of_remote_copies:
+                logger.warning(
+                    "Instance {} does not have enough remote copies, skipping",
+                    instance.id,
+                )
+                continue
+
+            # Now check if the checksums match
+            if self.verify_downstream_checksums:
+                for info in downstream:
+                    if not info.computed_same_checksum:
+                        logger.warning(
+                            "Instance {} has a mismatched checksum on {}, skipping",
+                            instance.id,
+                            info.librarian,
+                        )
+                        continue
+
+            # If we're here, we can delete the instance.
+            logger.info(
+                "Verified that we have the correct number of copies of "
+                "{instance.id} ({instance.file_name}): {n}/{m}, proceeding to deletion",
+                instance=instance,
+                n=len(downstream),
+                m=self.number_of_remote_copies,
+            )
+
+            try:
+                instance.delete(
+                    session=session,
+                    commit=True,
+                    force=self.force_deletion,
+                    mark_unavailable=self.mark_unavailable,
+                )
+                logger.info("Deleted instance {} successfully", instance.id)
+                deleted += 1
+            except FileNotFoundError:
+                logger.error(
+                    "Instance {} does not exist on disk, skipping", instance.id
+                )
+                continue
+
+        core_end = datetime.now(timezone.utc)
+
+        logger.info(
+            "Finished rolling deletion for store {} (ID: {}) in {} seconds, deleted {}/{} instances",
+            store.name,
+            store.id,
+            (core_end - core_begin).total_seconds(),
+            deleted,
+            len(instances),
+        )
+
+        return deleted == len(instances)
diff --git a/librarian_server/orm/instance.py b/librarian_server/orm/instance.py
index 918d8e8..655e676 100644
--- a/librarian_server/orm/instance.py
+++ b/librarian_server/orm/instance.py
@@ -91,6 +91,7 @@ def delete(
         session: Session,
         commit: bool = True,
         force: bool = False,
+        mark_unavailable: bool = False,
     ):
         """
         Delete this instance.
@@ -103,12 +104,17 @@ def delete(
             Whether or not to commit the deletion.
         force : bool
             Whether or not to force the deletion (i.e. ignore DeletionPolicy)
+        mark_unavailable: bool
+            If true, only mark this as unavailable, don't delete the metadata
         """
 
         if self.deletion_policy == DeletionPolicy.ALLOWED or force:
             self.store.store_manager.delete(Path(self.path))
 
-        session.delete(self)
+        if mark_unavailable:
+            self.available = False
+        else:
+            session.delete(self)
 
         if commit:
             session.commit()

From b08ee0296e357c9d125a0b27dbf7a9e575e28dc8 Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 14 Nov 2024 15:14:46 -0500
Subject: [PATCH 03/12] add first test for rolling deletion

---
 librarian_background/rolling_deletion.py      |  7 +-
 .../test_rolling_deletion.py                  | 75 +++++++++++++++++++
 2 files changed, 79 insertions(+), 3 deletions(-)
 create mode 100644 tests/background_unit_test/test_rolling_deletion.py

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index dbf4656..e7e93f4 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -29,7 +29,7 @@ class RollingDeletion(Task):
 
     store_name: str
     "Name of the store to delete instances from"
-    age_in_days: int
+    age_in_days: float
     "Age of the instances to delete, in days; older files will be deleted if they pass the checks"
 
     number_of_remote_copies: int = 3
@@ -89,9 +89,10 @@ def core(self, session: Session):
 
         deleted = 0
         for instance in instances:
+            # TODO: Soft timeout
             # Check that we got what we wanted.
             try:
-                assert instance.created_at < age_cutoff
+                # assert instance.created_time < age_cutoff
                 assert instance.store_id == store.id
                 assert instance.available
             except AssertionError:
@@ -103,7 +104,7 @@ def core(self, session: Session):
             # Check if the file associated with the instance has enough copies.
             remote_librarian_ids = {
                 remote_instance.librarian_id
-                for remote_instance in instance.remote_instances
+                for remote_instance in instance.file.remote_instances
             }
 
             logger.info(
diff --git a/tests/background_unit_test/test_rolling_deletion.py b/tests/background_unit_test/test_rolling_deletion.py
new file mode 100644
index 0000000..b2278a2
--- /dev/null
+++ b/tests/background_unit_test/test_rolling_deletion.py
@@ -0,0 +1,75 @@
+"""
+Tests for the rolling deletion task.
+"""
+
+import shutil
+from pathlib import Path
+
+from hera_librarian.deletion import DeletionPolicy
+
+
+def test_rolling_deletion_with_single_instance(
+    test_client, test_server, test_orm, garbage_file
+):
+    """
+    Delete a single instance.
+    """
+    from librarian_background.rolling_deletion import RollingDeletion
+
+    _, get_session, _ = test_server
+
+    session = get_session()
+
+    store = session.query(test_orm.StoreMetadata).filter_by(ingestable=True).first()
+
+    info = store.store_manager.path_info(garbage_file)
+
+    FILE_NAME = "path/for/rolling/deletion"
+
+    store_path = store.store_manager.store(Path(FILE_NAME))
+
+    shutil.copy(garbage_file, store_path)
+
+    # Create file and instances
+    file = test_orm.File.new_file(
+        filename=FILE_NAME,
+        size=info.size,
+        checksum=info.checksum,
+        uploader="test_user",
+        source="test_source",
+    )
+
+    instance = test_orm.Instance.new_instance(
+        path=store_path, file=file, store=store, deletion_policy=DeletionPolicy.ALLOWED
+    )
+
+    session.add_all([file, instance])
+    session.commit()
+
+    INSTANCE_ID = instance.id
+
+    # Run the task
+    task = RollingDeletion(
+        name="Rolling deletion",
+        soft_timeout="6:00:00",
+        store_name=store.name,
+        age_in_days=0.0000000000000000001,
+        number_of_remote_copies=0,
+        verify_downstream_checksums=False,
+        mark_unavailable=False,
+        force_deletion=False,
+    )()
+
+    assert task
+
+    # Check that the instance is gone
+    assert (
+        session.query(test_orm.Instance).filter_by(id=INSTANCE_ID).one_or_none() is None
+    )
+
+    # Delete the file we created
+    session.get(test_orm.File, FILE_NAME).delete(
+        session=session, commit=True, force=True
+    )
+
+    return

From 20563cfc8af984cd377fb05deb3d99eae1c59a11 Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 14 Nov 2024 16:40:48 -0500
Subject: [PATCH 04/12] Add mark unavaialble test

---
 librarian_background/rolling_deletion.py      |  2 +-
 librarian_server/orm/instance.py              |  3 +
 .../test_rolling_deletion.py                  | 84 ++++++++++++++++---
 3 files changed, 77 insertions(+), 12 deletions(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index e7e93f4..0e88ff4 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -160,7 +160,7 @@ def core(self, session: Session):
                     force=self.force_deletion,
                     mark_unavailable=self.mark_unavailable,
                 )
-                logger.info("Deleted instance {} successfully", instance.id)
+                logger.info("Deleted data for instance {} successfully", instance.id)
                 deleted += 1
             except FileNotFoundError:
                 logger.error(
diff --git a/librarian_server/orm/instance.py b/librarian_server/orm/instance.py
index 655e676..8ee0240 100644
--- a/librarian_server/orm/instance.py
+++ b/librarian_server/orm/instance.py
@@ -7,6 +7,7 @@
 from datetime import datetime, timezone
 from pathlib import Path
 
+from loguru import logger
 from sqlalchemy.orm import Session
 
 from hera_librarian.deletion import DeletionPolicy
@@ -112,8 +113,10 @@ def delete(
             self.store.store_manager.delete(Path(self.path))
 
         if mark_unavailable:
+            logger.info("Marking instance {} as unavailable", self.id)
             self.available = False
         else:
+            logger.info("Deleting instance {}", self.id)
             session.delete(self)
 
         if commit:
diff --git a/tests/background_unit_test/test_rolling_deletion.py b/tests/background_unit_test/test_rolling_deletion.py
index b2278a2..4d59602 100644
--- a/tests/background_unit_test/test_rolling_deletion.py
+++ b/tests/background_unit_test/test_rolling_deletion.py
@@ -8,17 +8,7 @@
 from hera_librarian.deletion import DeletionPolicy
 
 
-def test_rolling_deletion_with_single_instance(
-    test_client, test_server, test_orm, garbage_file
-):
-    """
-    Delete a single instance.
-    """
-    from librarian_background.rolling_deletion import RollingDeletion
-
-    _, get_session, _ = test_server
-
-    session = get_session()
+def prep_file(garbage_file, test_orm, session):
 
     store = session.query(test_orm.StoreMetadata).filter_by(ingestable=True).first()
 
@@ -46,6 +36,24 @@ def test_rolling_deletion_with_single_instance(
     session.add_all([file, instance])
     session.commit()
 
+    return store, file, instance
+
+
+def test_rolling_deletion_with_single_instance(
+    test_client, test_server, test_orm, garbage_file
+):
+    """
+    Delete a single instance.
+    """
+    from librarian_background.rolling_deletion import RollingDeletion
+
+    _, get_session, _ = test_server
+
+    session = get_session()
+
+    store, file, instance = prep_file(garbage_file, test_orm, session)
+
+    FILE_NAME = file.name
     INSTANCE_ID = instance.id
 
     # Run the task
@@ -62,6 +70,10 @@ def test_rolling_deletion_with_single_instance(
 
     assert task
 
+    session.close()
+
+    session = get_session()
+
     # Check that the instance is gone
     assert (
         session.query(test_orm.Instance).filter_by(id=INSTANCE_ID).one_or_none() is None
@@ -73,3 +85,53 @@ def test_rolling_deletion_with_single_instance(
     )
 
     return
+
+
+def test_rolling_deletion_with_single_instance_unavailable(
+    test_client, test_server, test_orm, garbage_file
+):
+    """
+    Delete a single instance.
+    """
+    from librarian_background.rolling_deletion import RollingDeletion
+
+    _, get_session, _ = test_server
+
+    session = get_session()
+
+    store, file, instance = prep_file(garbage_file, test_orm, session)
+
+    FILE_NAME = file.name
+    INSTANCE_ID = instance.id
+
+    # Run the task
+    task = RollingDeletion(
+        name="Rolling deletion",
+        soft_timeout="6:00:00",
+        store_name=store.name,
+        age_in_days=0.0000000000000000001,
+        number_of_remote_copies=0,
+        verify_downstream_checksums=False,
+        mark_unavailable=True,
+        force_deletion=False,
+    )()
+
+    assert task
+
+    # bgtask uses a different session
+    session.close()
+
+    session = get_session()
+
+    # Check that the instance is gone
+    re_queried = (
+        session.query(test_orm.Instance).filter_by(id=INSTANCE_ID).one_or_none()
+    )
+    assert not re_queried.available
+
+    # Delete the file we created
+    session.get(test_orm.File, FILE_NAME).delete(
+        session=session, commit=True, force=True
+    )
+
+    return

From 28d1e3f5ca56899c5925d32142e2bb21e109ef3c Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Fri, 15 Nov 2024 11:40:27 -0500
Subject: [PATCH 05/12] Add test to specifically see if we delete the _right_
 files

---
 librarian_background/rolling_deletion.py      | 10 ++-
 .../test_rolling_deletion.py                  | 73 ++++++++++++++++++-
 tests/integration_test/test_send_queue.py     | 23 ++++++
 3 files changed, 101 insertions(+), 5 deletions(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index 0e88ff4..46ed70d 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -69,10 +69,16 @@ def core(self, session: Session):
 
         # Get the instances that are older than the age
 
+        logger.info(
+            "Querying for created_times later than {} UTC ({} local)",
+            age_cutoff,
+            age_cutoff.astimezone(),
+        )
+
         query_begin = time.perf_counter()
         stmt = select(Instance).filter(
             Instance.store_id == store.id,
-            Instance.created_time < age_cutoff,
+            Instance.created_time < age_cutoff.astimezone(timezone.utc),
             Instance.available == True,
         )
 
@@ -92,7 +98,7 @@ def core(self, session: Session):
             # TODO: Soft timeout
             # Check that we got what we wanted.
             try:
-                # assert instance.created_time < age_cutoff
+                assert instance.created_time.replace(tzinfo=timezone.utc) < age_cutoff
                 assert instance.store_id == store.id
                 assert instance.available
             except AssertionError:
diff --git a/tests/background_unit_test/test_rolling_deletion.py b/tests/background_unit_test/test_rolling_deletion.py
index 4d59602..d98c5d8 100644
--- a/tests/background_unit_test/test_rolling_deletion.py
+++ b/tests/background_unit_test/test_rolling_deletion.py
@@ -3,19 +3,18 @@
 """
 
 import shutil
+from datetime import datetime, timedelta
 from pathlib import Path
 
 from hera_librarian.deletion import DeletionPolicy
 
 
-def prep_file(garbage_file, test_orm, session):
+def prep_file(garbage_file, test_orm, session, FILE_NAME="path/for/rolling/deletion"):
 
     store = session.query(test_orm.StoreMetadata).filter_by(ingestable=True).first()
 
     info = store.store_manager.path_info(garbage_file)
 
-    FILE_NAME = "path/for/rolling/deletion"
-
     store_path = store.store_manager.store(Path(FILE_NAME))
 
     shutil.copy(garbage_file, store_path)
@@ -135,3 +134,71 @@ def test_rolling_deletion_with_single_instance_unavailable(
     )
 
     return
+
+
+def test_rolling_deletion_with_multiple_files_age_out(
+    test_client, test_server, test_orm, garbage_file
+):
+    """
+    See if we correctly age out several files
+    """
+    from librarian_background.rolling_deletion import RollingDeletion
+
+    _, get_session, _ = test_server
+
+    session = get_session()
+
+    file_names = []
+    file_ages = []
+    instance_ids = []
+
+    for file_id in range(1, 10):
+        store, file, instance = prep_file(
+            garbage_file, test_orm, session, f"TEST_FILE/{file_id}.txt"
+        )
+        file.create_time = file.create_time - timedelta(days=file_id)
+        instance.created_time = file.create_time
+
+        file_names.append(file.name)
+        file_ages.append(file_id)
+        instance_ids.append(instance.id)
+
+    session.commit()
+
+    # Run the task
+    task = RollingDeletion(
+        name="Rolling deletion",
+        soft_timeout="6:00:00",
+        store_name=store.name,
+        age_in_days=5.0,
+        number_of_remote_copies=0,
+        verify_downstream_checksums=False,
+        mark_unavailable=True,
+        force_deletion=False,
+    )()
+
+    assert task
+
+    session.close()
+
+    session = get_session()
+
+    # Check that the older instances are gone
+
+    instances = [
+        session.query(test_orm.Instance).filter_by(id=id).one_or_none()
+        for id in instance_ids
+    ]
+
+    for name, age, instance in zip(file_names, file_ages, instances):
+        if age >= 5:
+            assert not instance.available
+        else:
+            assert instance.available
+
+        # Delete the file we created
+        session.get(test_orm.File, name).delete(
+            session=session, commit=True, force=True
+        )
+
+    return
diff --git a/tests/integration_test/test_send_queue.py b/tests/integration_test/test_send_queue.py
index 72a47a0..88e31e8 100644
--- a/tests/integration_test/test_send_queue.py
+++ b/tests/integration_test/test_send_queue.py
@@ -379,6 +379,29 @@ def test_send_from_existing_file_row(
     checksums_from_validations = {x.current_checksum for x in instance_validations}
     assert len(checksums_from_validations) == 1  # Same file
 
+    # Ok, now try the deletion task.
+    from librarian_background.rolling_deletion import RollingDeletion
+
+    task = RollingDeletion(
+        name="rolling_deletion",
+        store_name="local_store",
+        age_in_days=0.0000000000000000001,
+        number_of_remote_copies=1,
+        verify_downstream_checksums=True,
+        mark_unavailable=True,
+        force_deletion=False,
+    )
+
+    with source_session_maker() as session:
+        task.core(session=session)
+
+    # Check that the instance is gone
+    with source_session_maker() as session:
+        for file_name in copied_files:
+            file = session.get(test_orm.File, file_name)
+            for instance in file.instances:
+                assert instance.available == False
+
     # Remove the librarians we added.
     assert mocked_admin_client.remove_librarian(name="live_server")
 

From ce2bfd6d2650525fe476a389d622c8fda047c47b Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Fri, 15 Nov 2024 11:50:25 -0500
Subject: [PATCH 06/12] Add do not delete test

---
 .../test_rolling_deletion.py                  | 67 +++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/tests/background_unit_test/test_rolling_deletion.py b/tests/background_unit_test/test_rolling_deletion.py
index d98c5d8..01ad9e1 100644
--- a/tests/background_unit_test/test_rolling_deletion.py
+++ b/tests/background_unit_test/test_rolling_deletion.py
@@ -202,3 +202,70 @@ def test_rolling_deletion_with_multiple_files_age_out(
         )
 
     return
+
+
+def test_rolling_deletion_with_multiple_files_age_out_no_deletion_due_to_policy(
+    test_client, test_server, test_orm, garbage_file
+):
+    """
+    See if we correctly age out several files, but don't actually delete them because
+    we can't find remote instances.
+    """
+    from librarian_background.rolling_deletion import RollingDeletion
+
+    _, get_session, _ = test_server
+
+    session = get_session()
+
+    file_names = []
+    file_ages = []
+    instance_ids = []
+
+    for file_id in range(1, 10):
+        store, file, instance = prep_file(
+            garbage_file, test_orm, session, f"TEST_FILE/{file_id}.txt"
+        )
+        file.create_time = file.create_time - timedelta(days=file_id)
+        instance.created_time = file.create_time
+
+        file_names.append(file.name)
+        file_ages.append(file_id)
+        instance_ids.append(instance.id)
+
+    session.commit()
+
+    # Run the task
+    task = RollingDeletion(
+        name="Rolling deletion",
+        soft_timeout="6:00:00",
+        store_name=store.name,
+        age_in_days=5.0,
+        number_of_remote_copies=1,
+        verify_downstream_checksums=True,
+        mark_unavailable=True,
+        force_deletion=False,
+    )()
+
+    # Task officially fails; it could not delete the required number of instances
+    assert not task
+
+    session.close()
+
+    session = get_session()
+
+    # Check that the older instances are gone
+
+    instances = [
+        session.query(test_orm.Instance).filter_by(id=id).one_or_none()
+        for id in instance_ids
+    ]
+
+    for name, age, instance in zip(file_names, file_ages, instances):
+        assert instance.available
+
+        # Delete the file we created
+        session.get(test_orm.File, name).delete(
+            session=session, commit=True, force=True
+        )
+
+    return

From c9f006327079e9a375b341f1d0a3afa11fbab3bb Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Fri, 15 Nov 2024 11:52:51 -0500
Subject: [PATCH 07/12] Add task timeout warning

---
 librarian_background/rolling_deletion.py | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index 46ed70d..4a7a4e7 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -95,7 +95,16 @@ def core(self, session: Session):
 
         deleted = 0
         for instance in instances:
-            # TODO: Soft timeout
+            # First, see if we've timed out.
+            if datetime.datetime.now(timezone.utc) - core_begin > self.soft_timeout:
+                logger.warning(
+                    "Ran out of time in deletion task! Only successfully deleted "
+                    "{n}/{m} instances; we will return later",
+                    n=deleted,
+                    m=len(instances),
+                )
+                return False
+
             # Check that we got what we wanted.
             try:
                 assert instance.created_time.replace(tzinfo=timezone.utc) < age_cutoff

From 81a9e568f0204f6b8c6959f134da679ab10d4ac2 Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Fri, 15 Nov 2024 11:55:41 -0500
Subject: [PATCH 08/12] Fix in timeout code

---
 librarian_background/rolling_deletion.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index 4a7a4e7..b9460ab 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -96,7 +96,7 @@ def core(self, session: Session):
         deleted = 0
         for instance in instances:
             # First, see if we've timed out.
-            if datetime.datetime.now(timezone.utc) - core_begin > self.soft_timeout:
+            if (datetime.now(timezone.utc) - core_begin) > self.soft_timeout:
                 logger.warning(
                     "Ran out of time in deletion task! Only successfully deleted "
                     "{n}/{m} instances; we will return later",

From 4b9b837a4195b6286d375fc10c7bb54075481ffb Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Fri, 15 Nov 2024 11:56:12 -0500
Subject: [PATCH 09/12] Guard against none soft timeout

---
 librarian_background/rolling_deletion.py | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index b9460ab..3983171 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -96,14 +96,15 @@ def core(self, session: Session):
         deleted = 0
         for instance in instances:
             # First, see if we've timed out.
-            if (datetime.now(timezone.utc) - core_begin) > self.soft_timeout:
-                logger.warning(
-                    "Ran out of time in deletion task! Only successfully deleted "
-                    "{n}/{m} instances; we will return later",
-                    n=deleted,
-                    m=len(instances),
-                )
-                return False
+            if self.soft_timeout is not None:
+                if (datetime.now(timezone.utc) - core_begin) > self.soft_timeout:
+                    logger.warning(
+                        "Ran out of time in deletion task! Only successfully deleted "
+                        "{n}/{m} instances; we will return later",
+                        n=deleted,
+                        m=len(instances),
+                    )
+                    return False
 
             # Check that we got what we wanted.
             try:

From 686d755ba7a4abfe322dae7d8371d20679a8c575 Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 5 Dec 2024 13:17:36 -0500
Subject: [PATCH 10/12] Replace asserts with boolean logic chaining

---
 librarian_background/rolling_deletion.py | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index 3983171..a364af8 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -107,11 +107,11 @@ def core(self, session: Session):
                     return False
 
             # Check that we got what we wanted.
-            try:
-                assert instance.created_time.replace(tzinfo=timezone.utc) < age_cutoff
-                assert instance.store_id == store.id
-                assert instance.available
-            except AssertionError:
+            valid_time = instance.created_time.replace(tzinfo=timezone.utc) < age_cutoff
+            valid_store = instance.store_id == store.id
+            all_ok = valid_time and valid_store and instance.available
+            
+            if not all_ok:
                 logger.error(
                     "Instance {} does not meet the criteria, skipping", instance.id
                 )

From 8eea53180bb9103f13d3d7d4a6c92ddd0eecf635 Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 5 Dec 2024 13:26:03 -0500
Subject: [PATCH 11/12] Add information on not enough files

---
 librarian_background/rolling_deletion.py | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index a364af8..7fa4db8 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -110,7 +110,7 @@ def core(self, session: Session):
             valid_time = instance.created_time.replace(tzinfo=timezone.utc) < age_cutoff
             valid_store = instance.store_id == store.id
             all_ok = valid_time and valid_store and instance.available
-            
+
             if not all_ok:
                 logger.error(
                     "Instance {} does not meet the criteria, skipping", instance.id
@@ -144,8 +144,10 @@ def core(self, session: Session):
             # Now check if we have enough!
             if len(downstream) < self.number_of_remote_copies:
                 logger.warning(
-                    "Instance {} does not have enough remote copies, skipping",
+                    "Instance {} does not have enough remote copies {}/{}, skipping",
                     instance.id,
+                    len(downstream),
+                    self.number_of_remote_copies,
                 )
                 continue
 

From ad5f4c2138d637b72cfacc0f35160f8f24d8f5f6 Mon Sep 17 00:00:00 2001
From: Josh Borrow <josh@joshborrow.com>
Date: Thu, 5 Dec 2024 13:28:03 -0500
Subject: [PATCH 12/12] Swap file for instance in comment

---
 librarian_background/rolling_deletion.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/librarian_background/rolling_deletion.py b/librarian_background/rolling_deletion.py
index 7fa4db8..c720223 100644
--- a/librarian_background/rolling_deletion.py
+++ b/librarian_background/rolling_deletion.py
@@ -30,7 +30,7 @@ class RollingDeletion(Task):
     store_name: str
     "Name of the store to delete instances from"
     age_in_days: float
-    "Age of the instances to delete, in days; older files will be deleted if they pass the checks"
+    "Age of the instances to delete, in days; older instances will be deleted if they pass the checks"
 
     number_of_remote_copies: int = 3
     "Number of remote copies that must be available to delete the file"