From 23ab466c521c3ea3b86cfaf24162df18eb3600d8 Mon Sep 17 00:00:00 2001 From: StasonJatham Date: Wed, 27 May 2026 16:52:18 +0200 Subject: [PATCH] Harden cleanup scheduler and audit cleanup deletions --- karldns/app.py | 7 +++++- karldns/event_statuses.py | 3 +++ karldns/store/rate_limit.py | 41 ++++++++++++++++++++++++++++++ tests/test_scheduler.py | 50 +++++++++++++++++++++++++++++++++++++ 4 files changed, 100 insertions(+), 1 deletion(-) diff --git a/karldns/app.py b/karldns/app.py index 093e233..4ed7b1e 100644 --- a/karldns/app.py +++ b/karldns/app.py @@ -104,7 +104,12 @@ def make_app(settings: DdnsSettings | None = None) -> FastAPI: async def _scheduler_loop(store: DdnsStore, settings: DdnsSettings) -> None: poll_seconds = min(settings.cleanup_interval_seconds, 60) while True: - await _run_cleanup_if_due(store, settings) + try: + await _run_cleanup_if_due(store, settings) + except asyncio.CancelledError: + raise + except Exception: + LOGGER.exception("Scheduled cleanup loop crashed; continuing") await asyncio.sleep(poll_seconds) diff --git a/karldns/event_statuses.py b/karldns/event_statuses.py index 61e615b..15576e2 100644 --- a/karldns/event_statuses.py +++ b/karldns/event_statuses.py @@ -16,3 +16,6 @@ DASHBOARD_HOSTNAME_ACTIVATED = "dashboard_hostname_activated" ADMIN_HOSTNAME_DELETED = "admin_hostname_deleted" ADMIN_UPDATE_URL_ROTATED = "admin_update_url_rotated" ADMIN_MANAGEMENT_URL_ROTATED = "admin_management_url_rotated" + +CLEANUP_PENDING_HOSTNAME_DELETED = "cleanup_pending_hostname_deleted" +CLEANUP_UNUSED_HOSTNAME_DELETED = "cleanup_unused_hostname_deleted" diff --git a/karldns/store/rate_limit.py b/karldns/store/rate_limit.py index 300b9dc..4b5dd54 100644 --- a/karldns/store/rate_limit.py +++ b/karldns/store/rate_limit.py @@ -4,6 +4,7 @@ import threading import time from datetime import UTC, datetime, timedelta +from ..event_statuses import CLEANUP_PENDING_HOSTNAME_DELETED, CLEANUP_UNUSED_HOSTNAME_DELETED from .base import StoreConnectionMixin from .types import CleanupRunRow, cleanup_run_row @@ -134,6 +135,28 @@ class RateLimitCleanupMixin(StoreConnectionMixin): pending_account_cutoff = (datetime.now(UTC) - timedelta(hours=pending_account_hours)).isoformat() event_history_cutoff = (datetime.now(UTC) - timedelta(hours=event_history_hours)).isoformat() with self.connect() as conn: + deleted_accounts = conn.execute( + """ + SELECT hostname, + CASE + WHEN accepted_at IS NULL THEN 'pending' + ELSE 'unused' + END AS cleanup_reason + FROM accounts + WHERE ( + accepted_at IS NULL + AND created_at < ? + ) + OR ( + accepted_at IS NOT NULL + AND created_at < ? + AND NOT EXISTS ( + SELECT 1 FROM records WHERE records.hostname = accounts.hostname + ) + ) + """, + (pending_account_cutoff, unused_account_cutoff), + ).fetchall() account_cursor = conn.execute( """ DELETE FROM accounts @@ -168,6 +191,24 @@ class RateLimitCleanupMixin(StoreConnectionMixin): """, (datetime.now(UTC).isoformat(), account_cursor.rowcount), ) + conn.executemany( + """ + INSERT INTO update_events(hostname, ipv4, ipv6, status, detail, source_ip, created_at) + VALUES (?, NULL, NULL, ?, ?, ?, ?) + """, + [ + ( + row["hostname"], + CLEANUP_PENDING_HOSTNAME_DELETED + if row["cleanup_reason"] == "pending" + else CLEANUP_UNUSED_HOSTNAME_DELETED, + "cleanup deleted abandoned hostname", + "127.0.0.1", + datetime.now(UTC).isoformat(), + ) + for row in deleted_accounts + ], + ) return { "unused_accounts": account_cursor.rowcount, "event_rollups": rollup_cursor.rowcount, diff --git a/tests/test_scheduler.py b/tests/test_scheduler.py index bd4a628..2d856c0 100644 --- a/tests/test_scheduler.py +++ b/tests/test_scheduler.py @@ -11,6 +11,7 @@ from fastapi.testclient import TestClient import karldns.app as app_module from karldns import DdnsSettings, DdnsStore, make_app +from karldns.event_statuses import CLEANUP_PENDING_HOSTNAME_DELETED, CLEANUP_UNUSED_HOSTNAME_DELETED from karldns.security import hash_lookup_token @@ -174,6 +175,9 @@ def test_cleanup_removes_pending_generated_setups_after_cutoff(tmp_path: Path) - assert result["unused_accounts"] == 1 assert store.get_account_by_slug(account["update_slug"]) is None + cleanup_events = store.list_update_events_for_hostname(account["hostname"]) + assert cleanup_events[0]["status"] == CLEANUP_PENDING_HOSTNAME_DELETED + assert cleanup_events[0]["detail"] == "cleanup deleted abandoned hostname" def test_cleanup_keeps_recent_pending_setup_until_pending_cutoff(tmp_path: Path) -> None: @@ -194,6 +198,26 @@ def test_cleanup_keeps_recent_pending_setup_until_pending_cutoff(tmp_path: Path) assert store.get_account_by_slug(account["update_slug"]) is not None +def test_cleanup_logs_event_for_accepted_unused_hostname(tmp_path: Path) -> None: + database_path = tmp_path / "ddns.sqlite3" + store = DdnsStore(database_path) + account = store.create_account("unused.ddns.example.net", "router", accepted=True) + + with store.connect() as conn: + conn.execute( + "UPDATE accounts SET created_at = ? WHERE hostname = ?", + ("2000-01-01T00:00:00+00:00", account["hostname"]), + ) + + result = store.cleanup(unused_account_hours=1) + + assert result["unused_accounts"] == 1 + assert store.get_account_by_slug(account["update_slug"]) is None + cleanup_events = store.list_update_events_for_hostname(account["hostname"]) + assert cleanup_events[0]["status"] == CLEANUP_UNUSED_HOSTNAME_DELETED + assert cleanup_events[0]["detail"] == "cleanup deleted abandoned hostname" + + def test_pending_generated_setup_stays_pending_after_restart(tmp_path: Path) -> None: database_path = tmp_path / "ddns.sqlite3" store = DdnsStore(database_path) @@ -389,3 +413,29 @@ def test_scheduled_cleanup_failure_is_logged(tmp_path: Path, monkeypatch, caplog assert scheduled_task["last_status"] == "error" assert "RuntimeError: cleanup exploded" in str(scheduled_task["last_error"]) assert "Scheduled cleanup failed" in caplog.text + + +def test_scheduler_loop_survives_iteration_exception(tmp_path: Path, monkeypatch, caplog) -> None: + settings = DdnsSettings(database_path=tmp_path / "ddns.sqlite3", admin_password="admin", cleanup_interval_seconds=60) + store = DdnsStore(settings.database_path) + calls = 0 + + async def fail_once_then_cancel(inner_store, inner_settings): + nonlocal calls + calls += 1 + if calls == 1: + raise RuntimeError("post-cleanup crash") + raise asyncio.CancelledError + + async def no_sleep(_seconds: float) -> None: + return None + + caplog.set_level(logging.ERROR, logger="karldns.app") + monkeypatch.setattr(app_module, "_run_cleanup_if_due", fail_once_then_cancel) + monkeypatch.setattr(app_module.asyncio, "sleep", no_sleep) + + with pytest.raises(asyncio.CancelledError): + asyncio.run(app_module._scheduler_loop(store, settings)) + + assert calls == 2 + assert "Scheduled cleanup loop crashed; continuing" in caplog.text