From e6d9d68729bca2ccf4cbc247b4e268f83c06c074 Mon Sep 17 00:00:00 2001 From: tanmayraut45 Date: Sat, 11 Jul 2026 17:33:24 +0530 Subject: [PATCH] CalDAV: close the DAVClient on sync and write-back paths (#4793) _sync_blocking (src/caldav_sync.py) and _writeback_blocking (src/caldav_writeback.py) each open their own caldav.DAVClient via _build_dav_client, but never close it. The client owns an HTTP session with a pooled connection; without a close() that connection is held until process exit. Previously the fix added explicit client.close() calls before each early return and at the end of the DB finally block. This still leaked the client when SessionLocal() raised before the DB try/finally was entered. Now _sync_blocking wraps the entire post-construction path in an outer try/finally that calls client.close() unconditionally, covering: - AuthorizationError / NotFoundError early return - URL-fallback failure early return - no-calendars early return - normal return after sync - SessionLocal() construction failure (new regression coverage) _writeback_blocking already used a try/finally (unchanged). - src/caldav_sync.py: replace scattered client.close() calls with a single outer try/finally block around the discovery + DB sync path - tests/test_caldav_client_cleanup.py: add CalendarDeletedEvent to the database stub; add regression test for SessionLocal() failure path Closes #4593 --- src/caldav_sync.py | 378 +++++++++++----------- src/caldav_writeback.py | 13 +- tests/test_caldav_client_cleanup.py | 145 +++++++++ tests/test_caldav_google_principal_url.py | 4 + 4 files changed, 347 insertions(+), 193 deletions(-) create mode 100644 tests/test_caldav_client_cleanup.py diff --git a/src/caldav_sync.py b/src/caldav_sync.py index f91ebc1a0..47fb3333b 100644 --- a/src/caldav_sync.py +++ b/src/caldav_sync.py @@ -280,214 +280,216 @@ def _sync_blocking(owner: str, url: str, username: str, password: str, account_i result = {"calendars": 0, "events": 0, "deleted": 0, "errors": []} client = _build_dav_client(url, username, password) - - # Discovery: try principal → calendars first; if the server doesn't - # support discovery (or the URL points directly at a calendar), fall - # back to treating the URL as a single calendar. - calendars = [] try: - principal = client.principal() - calendars = principal.calendars() - except (AuthorizationError, NotFoundError) as e: - result["errors"].append(f"Discovery failed: {e}") - return result - except Exception as e: - logger.info(f"CalDAV principal discovery failed, trying URL as calendar: {e}") + # Discovery: try principal → calendars first; if the server doesn't + # support discovery (or the URL points directly at a calendar), fall + # back to treating the URL as a single calendar. + calendars = [] try: - calendars = [_open_url_as_calendar(client, url)] - except Exception as e2: - result["errors"].append(f"Could not open URL as calendar: {e2}") - return result - - if not calendars: - try: - calendars = [_open_url_as_calendar(client, url)] + principal = client.principal() + calendars = principal.calendars() + except (AuthorizationError, NotFoundError) as e: + result["errors"].append(f"Discovery failed: {e}") + return result # outer finally will call client.close() except Exception as e: - result["errors"].append(f"No calendars and URL fallback failed: {e}") - return result - - start = datetime.utcnow() - timedelta(days=_LOOKBACK_DAYS) - end = datetime.utcnow() + timedelta(days=_LOOKAHEAD_DAYS) - - db = SessionLocal() - try: - for remote_cal in calendars: + logger.info(f"CalDAV principal discovery failed, trying URL as calendar: {e}") try: - remote_url = str(remote_cal.url) - cal_id = _stable_cal_id(remote_url, owner=owner, account_id=account_id) - display_name = (remote_cal.name or "").strip() or "CalDAV" + calendars = [_open_url_as_calendar(client, url)] + except Exception as e2: + result["errors"].append(f"Could not open URL as calendar: {e2}") + return result # outer finally will call client.close() - local_cal = db.query(CalendarCal).filter( - CalendarCal.id == cal_id, - CalendarCal.owner == owner, - ).first() - if not local_cal: - local_cal = CalendarCal( - id=cal_id, - owner=owner, - name=display_name, - color="#5b8abf", - source="caldav", - account_id=account_id or None, - caldav_base_url=remote_url, - ) - db.add(local_cal) - db.commit() - else: - # Refresh display name and stamp CalDAV metadata if missing. - changed = False - if local_cal.name != display_name: - local_cal.name = display_name - changed = True - if account_id and not local_cal.account_id: - local_cal.account_id = account_id - changed = True - if local_cal.caldav_base_url != remote_url: - local_cal.caldav_base_url = remote_url - changed = True - if changed: - db.commit() - result["calendars"] += 1 + if not calendars: + try: + calendars = [_open_url_as_calendar(client, url)] + except Exception as e: + result["errors"].append(f"No calendars and URL fallback failed: {e}") + return result # outer finally will call client.close() - # Fetch events in window. `date_search` returns CalendarObject - # resources; each may contain one VEVENT (most servers) or - # several (rare). - from icalendar import Calendar as iCal + start = datetime.utcnow() - timedelta(days=_LOOKBACK_DAYS) + end = datetime.utcnow() + timedelta(days=_LOOKAHEAD_DAYS) - seen_uids = set() - # Track events added to the session but not yet committed so - # duplicate UIDs within the same batch are updated, not re-inserted - # (which would violate the UNIQUE constraint on commit). - pending: dict = {} - parse_failed = False + db = SessionLocal() # if this raises, outer finally still calls client.close() + try: + for remote_cal in calendars: try: - objs = remote_cal.date_search(start=start, end=end, expand=False) - except Exception as e: - result["errors"].append(f"{display_name}: date_search failed ({e})") - continue + remote_url = str(remote_cal.url) + cal_id = _stable_cal_id(remote_url, owner=owner, account_id=account_id) + display_name = (remote_cal.name or "").strip() or "CalDAV" - for obj in objs: + local_cal = db.query(CalendarCal).filter( + CalendarCal.id == cal_id, + CalendarCal.owner == owner, + ).first() + if not local_cal: + local_cal = CalendarCal( + id=cal_id, + owner=owner, + name=display_name, + color="#5b8abf", + source="caldav", + account_id=account_id or None, + caldav_base_url=remote_url, + ) + db.add(local_cal) + db.commit() + else: + # Refresh display name and stamp CalDAV metadata if missing. + changed = False + if local_cal.name != display_name: + local_cal.name = display_name + changed = True + if account_id and not local_cal.account_id: + local_cal.account_id = account_id + changed = True + if local_cal.caldav_base_url != remote_url: + local_cal.caldav_base_url = remote_url + changed = True + if changed: + db.commit() + result["calendars"] += 1 + + # Fetch events in window. `date_search` returns CalendarObject + # resources; each may contain one VEVENT (most servers) or + # several (rare). + from icalendar import Calendar as iCal + + seen_uids = set() + # Track events added to the session but not yet committed so + # duplicate UIDs within the same batch are updated, not re-inserted + # (which would violates the UNIQUE constraint on commit). + pending: dict = {} + parse_failed = False try: - ical = iCal.from_ical(obj.data) + objs = remote_cal.date_search(start=start, end=end, expand=False) except Exception as e: - result["errors"].append(f"{display_name}: parse failed ({e})") - parse_failed = True + result["errors"].append(f"{display_name}: date_search failed ({e})") continue - for comp in ical.walk(): - if comp.name != "VEVENT": + for obj in objs: + try: + ical = iCal.from_ical(obj.data) + except Exception as e: + result["errors"].append(f"{display_name}: parse failed ({e})") + parse_failed = True continue - uid_val = str(comp.get("uid", "")) or str(uuid.uuid4()) - seen_uids.add(uid_val) - dtstart_p = comp.get("dtstart") - if not dtstart_p: - continue - start_dt, all_day = _to_utc_naive(dtstart_p.dt) - - dtend_p = comp.get("dtend") - if dtend_p: - end_dt, _ = _to_utc_naive(dtend_p.dt) - elif all_day: - end_dt = start_dt + timedelta(days=1) - else: - end_dt = start_dt + timedelta(hours=1) - # A synced event with DTEND <= DTSTART (e.g. a single-day - # all-day event whose source wrote DTEND equal to DTSTART) - # would be stored zero-duration and silently dropped by the - # list_events overlap filter. Clamp to a positive span. - end_dt = _ensure_positive_duration(start_dt, end_dt, all_day) - - # is_utc reflects whether the source carried a TZ - # we converted from. All-day = no TZ semantics. - row_is_utc = ( - not all_day - and isinstance(dtstart_p.dt, datetime) - and dtstart_p.dt.tzinfo is not None - ) - - summary = str(comp.get("summary", "")) - description = str(comp.get("description", "")) - location = str(comp.get("location", "")) - rrule = ( - comp.get("rrule").to_ical().decode() - if comp.get("rrule") - else "" - ) - - existing = _find_existing_event(db, pending, uid_val, local_cal.id) - if existing: - if existing.caldav_sync_pending in {"create", "update"}: - result["events"] += 1 + for comp in ical.walk(): + if comp.name != "VEVENT": continue - existing.calendar_id = local_cal.id - existing.summary = summary - existing.description = description - existing.location = location - existing.dtstart = start_dt - existing.dtend = end_dt - existing.all_day = all_day - existing.is_utc = row_is_utc - existing.rrule = rrule - existing.origin = "caldav" - existing.remote_href = str(getattr(obj, "url", "") or "") or None - existing.remote_etag = _event_etag(obj) or None - existing.caldav_sync_pending = None - else: - new_ev = CalendarEvent( - uid=uid_val, - calendar_id=local_cal.id, - summary=summary, - description=description, - location=location, - dtstart=start_dt, - dtend=end_dt, - all_day=all_day, - is_utc=row_is_utc, - rrule=rrule, - origin="caldav", - remote_href=str(getattr(obj, "url", "") or "") or None, - remote_etag=_event_etag(obj) or None, + uid_val = str(comp.get("uid", "")) or str(uuid.uuid4()) + seen_uids.add(uid_val) + + dtstart_p = comp.get("dtstart") + if not dtstart_p: + continue + start_dt, all_day = _to_utc_naive(dtstart_p.dt) + + dtend_p = comp.get("dtend") + if dtend_p: + end_dt, _ = _to_utc_naive(dtend_p.dt) + elif all_day: + end_dt = start_dt + timedelta(days=1) + else: + end_dt = start_dt + timedelta(hours=1) + # A synced event with DTEND <= DTSTART (e.g. a single-day + # all-day event whose source wrote DTEND equal to DTSTART) + # would be stored zero-duration and silently dropped by the + # list_events overlap filter. Clamp to a positive span. + end_dt = _ensure_positive_duration(start_dt, end_dt, all_day) + + # is_utc reflects whether the source carried a TZ + # we converted from. All-day = no TZ semantics. + row_is_utc = ( + not all_day + and isinstance(dtstart_p.dt, datetime) + and dtstart_p.dt.tzinfo is not None ) - db.add(new_ev) - pending[uid_val] = new_ev - result["events"] += 1 - db.commit() - # Prune locally-cached CalDAV events that vanished - # upstream (only within our sync window — events outside - # the window aren't in `objs`, so we'd false-delete them). - # Only rows we previously pulled from the server (origin=="caldav") - # are prunable; locally-created events (agent / email triage / a - # UI event whose write-back failed) carry origin NULL and must - # never be deleted just because the server didn't return them. - # Skip the prune on any parse failure: seen_uids is then an - # incomplete view of the server, so pruning against it would - # delete events that still exist upstream but could not be read - # (the empty-seen_uids case wipes the whole window; a partial - # failure deletes just the unreadable rows). - if _should_prune_window(seen_uids, parse_failed): - stale = db.query(CalendarEvent).filter( - CalendarEvent.calendar_id == local_cal.id, - CalendarEvent.origin == "caldav", - CalendarEvent.dtstart >= start, - CalendarEvent.dtstart <= end, - CalendarEvent.remote_href.isnot(None), - CalendarEvent.caldav_sync_pending.is_(None), - ~CalendarEvent.uid.in_(seen_uids) if seen_uids else CalendarEvent.uid.isnot(None), - ).all() - for ev in stale: - db.delete(ev) - result["deleted"] += len(stale) + summary = str(comp.get("summary", "")) + description = str(comp.get("description", "")) + location = str(comp.get("location", "")) + rrule = ( + comp.get("rrule").to_ical().decode() + if comp.get("rrule") + else "" + ) + + existing = _find_existing_event(db, pending, uid_val, local_cal.id) + if existing: + if existing.caldav_sync_pending in {"create", "update"}: + result["events"] += 1 + continue + existing.calendar_id = local_cal.id + existing.summary = summary + existing.description = description + existing.location = location + existing.dtstart = start_dt + existing.dtend = end_dt + existing.all_day = all_day + existing.is_utc = row_is_utc + existing.rrule = rrule + existing.origin = "caldav" + existing.remote_href = str(getattr(obj, "url", "") or "") or None + existing.remote_etag = _event_etag(obj) or None + existing.caldav_sync_pending = None + else: + new_ev = CalendarEvent( + uid=uid_val, + calendar_id=local_cal.id, + summary=summary, + description=description, + location=location, + dtstart=start_dt, + dtend=end_dt, + all_day=all_day, + is_utc=row_is_utc, + rrule=rrule, + origin="caldav", + remote_href=str(getattr(obj, "url", "") or "") or None, + remote_etag=_event_etag(obj) or None, + ) + db.add(new_ev) + pending[uid_val] = new_ev + result["events"] += 1 db.commit() - except Exception as e: - logger.exception("CalDAV sync failed for one calendar") - result["errors"].append(str(e)[:200]) - db.rollback() - finally: - db.close() - return result + # Prune locally-cached CalDAV events that vanished + # upstream (only within our sync window — events outside + # the window aren't in `objs`, so we'd false-delete them). + # Only rows we previously pulled from the server (origin=="caldav") + # are prunable; locally-created events (agent / email triage / a + # UI event whose write-back failed) carry origin NULL and must + # never be deleted just because the server didn't return them. + # Skip the prune on any parse failure: seen_uids is then an + # incomplete view of the server, so pruning against it would + # delete events that still exist upstream but could not be read + # (the empty-seen_uids case wipes the whole window; a partial + # failure deletes just the unreadable rows). + if _should_prune_window(seen_uids, parse_failed): + stale = db.query(CalendarEvent).filter( + CalendarEvent.calendar_id == local_cal.id, + CalendarEvent.origin == "caldav", + CalendarEvent.dtstart >= start, + CalendarEvent.dtstart <= end, + CalendarEvent.remote_href.isnot(None), + CalendarEvent.caldav_sync_pending.is_(None), + ~CalendarEvent.uid.in_(seen_uids) if seen_uids else CalendarEvent.uid.isnot(None), + ).all() + for ev in stale: + db.delete(ev) + result["deleted"] += len(stale) + db.commit() + except Exception as e: + logger.exception("CalDAV sync failed for one calendar") + result["errors"].append(str(e)[:200]) + db.rollback() + finally: + db.close() # NOT client.close() here anymore + + return result + finally: + client.close() # always called def _event_payload(ev) -> dict: diff --git a/src/caldav_writeback.py b/src/caldav_writeback.py index b1cf288b1..2d5781091 100644 --- a/src/caldav_writeback.py +++ b/src/caldav_writeback.py @@ -192,11 +192,14 @@ def _writeback_blocking(local_cal_id, ev, delete, url, username, password, # Redirects disabled here too: the write-back path opens its own DAVClient, # so it needs the same SSRF-via-redirect protection as the pull path. client = _build_dav_client(url, username, password) - calendars = _discover_calendars(client) - if not calendars: - return {"ok": False, "error": "no remote calendars discovered"} - return push_event(calendars, local_cal_id, ev, delete=delete, - owner=owner, account_id=account_id) + try: + calendars = _discover_calendars(client) + if not calendars: + return {"ok": False, "error": "no remote calendars discovered"} + return push_event(calendars, local_cal_id, ev, delete=delete, + owner=owner, account_id=account_id) + finally: + client.close() def _persist_writeback_result(owner: str, calendar_id: str, uid: str, result: dict, *, delete: bool) -> None: diff --git a/tests/test_caldav_client_cleanup.py b/tests/test_caldav_client_cleanup.py new file mode 100644 index 000000000..eb91f37bc --- /dev/null +++ b/tests/test_caldav_client_cleanup.py @@ -0,0 +1,145 @@ +"""Issue #4593 — the CalDAV DAVClient must be closed on every path. + +`_sync_blocking` (src/caldav_sync.py) and `_writeback_blocking` +(src/caldav_writeback.py) each open their own DAVClient. The client holds an +HTTP session with pooled connections; if it is never closed those connections +leak for the lifetime of the process. These tests pin that the client is +closed on the discovery early-returns, the normal return, and the +write-back paths, using a fake client so no network or `caldav` install is +needed. +""" + +import sys +import types + +import pytest +from unittest.mock import MagicMock + + +def _stub_sync_deps(monkeypatch): + """Make `_sync_blocking`'s lazy imports resolve without a real caldav/db.""" + err_mod = types.ModuleType("caldav.lib.error") + + class AuthorizationError(Exception): + pass + + class NotFoundError(Exception): + pass + + err_mod.AuthorizationError = AuthorizationError + err_mod.NotFoundError = NotFoundError + monkeypatch.setitem(sys.modules, "caldav", types.ModuleType("caldav")) + monkeypatch.setitem(sys.modules, "caldav.lib", types.ModuleType("caldav.lib")) + monkeypatch.setitem(sys.modules, "caldav.lib.error", err_mod) + + db_mod = types.ModuleType("core.database") + db_mod.CalendarCal = MagicMock() + db_mod.CalendarEvent = MagicMock() + db_mod.CalendarDeletedEvent = MagicMock() + db_mod.SessionLocal = MagicMock() + if "core" not in sys.modules: + monkeypatch.setitem(sys.modules, "core", types.ModuleType("core")) + monkeypatch.setitem(sys.modules, "core.database", db_mod) + + # Stub routes.calendar_routes so the lazy import of _ensure_positive_duration + # inside _sync_blocking doesn't drag in dateutil / FastAPI / SQLAlchemy. + routes_mod = types.ModuleType("routes") + cal_routes_mod = types.ModuleType("routes.calendar_routes") + cal_routes_mod._ensure_positive_duration = lambda start, end, all_day: end + if "routes" not in sys.modules: + monkeypatch.setitem(sys.modules, "routes", routes_mod) + monkeypatch.setitem(sys.modules, "routes.calendar_routes", cal_routes_mod) + + return AuthorizationError + + +def test_sync_closes_client_on_discovery_auth_failure(monkeypatch): + import src.caldav_sync as sync + + AuthorizationError = _stub_sync_deps(monkeypatch) + client = MagicMock() + client.principal.side_effect = AuthorizationError("bad credentials") + monkeypatch.setattr(sync, "_build_dav_client", lambda *a, **k: client) + + result = sync._sync_blocking("alice", "https://dav.example.com/", "u", "p") + + client.close.assert_called_once() + assert any("Discovery failed" in e for e in result["errors"]) + + +def test_sync_closes_client_when_url_fallback_fails(monkeypatch): + import src.caldav_sync as sync + + _stub_sync_deps(monkeypatch) + client = MagicMock() + # principal() raises a generic error -> the URL-as-calendar fallback is + # tried; make that fail too so the function hits the early return. + client.principal.side_effect = RuntimeError("no principal endpoint") + monkeypatch.setattr(sync, "_build_dav_client", lambda *a, **k: client) + monkeypatch.setattr( + sync, "_open_url_as_calendar", + MagicMock(side_effect=RuntimeError("not a calendar")), + ) + + result = sync._sync_blocking("alice", "https://dav.example.com/", "u", "p") + + client.close.assert_called_once() + assert result["errors"] + + +def test_writeback_closes_client_when_no_calendars(monkeypatch): + import src.caldav_sync as sync + import src.caldav_writeback as wb + + client = MagicMock() + monkeypatch.setattr(sync, "_build_dav_client", lambda *a, **k: client) + monkeypatch.setattr(wb, "_discover_calendars", lambda c: []) + + result = wb._writeback_blocking( + "caldav-1", {"uid": "evt-1"}, False, "https://dav.example.com/", "u", "p" + ) + + client.close.assert_called_once() + assert result["ok"] is False + + +def test_writeback_closes_client_on_success(monkeypatch): + import src.caldav_sync as sync + import src.caldav_writeback as wb + + client = MagicMock() + monkeypatch.setattr(sync, "_build_dav_client", lambda *a, **k: client) + monkeypatch.setattr(wb, "_discover_calendars", lambda c: [MagicMock()]) + monkeypatch.setattr(wb, "push_event", lambda *a, **k: {"ok": True}) + + result = wb._writeback_blocking( + "caldav-1", {"uid": "evt-1"}, False, "https://dav.example.com/", "u", "p" + ) + + client.close.assert_called_once() + assert result["ok"] is True + + +def test_sync_closes_client_when_session_local_raises(monkeypatch): + import src.caldav_sync as sync + + AuthorizationError = _stub_sync_deps(monkeypatch) + + # Give principal() a working response so discovery passes + mock_principal = MagicMock() + mock_cal = MagicMock() + mock_cal.url = "https://dav.example.com/alice/home/" + mock_principal.calendars.return_value = [mock_cal] + + client = MagicMock() + client.principal.return_value = mock_principal + monkeypatch.setattr(sync, "_build_dav_client", lambda *a, **k: client) + + # Make SessionLocal blow up before any DB work + import sys + sys.modules["core.database"].SessionLocal.side_effect = RuntimeError("DB unavailable") + + with pytest.raises(RuntimeError, match="DB unavailable"): + sync._sync_blocking("alice", "https://dav.example.com/", "u", "p") + + client.close.assert_called_once() diff --git a/tests/test_caldav_google_principal_url.py b/tests/test_caldav_google_principal_url.py index f4eb06b0f..3274c9dfd 100644 --- a/tests/test_caldav_google_principal_url.py +++ b/tests/test_caldav_google_principal_url.py @@ -93,6 +93,10 @@ class _FakeClient: def calendar(self, url=None): return _FakeCalendar(url) + def close(self): + # Mirror the real DAVClient: sync now closes the client on every path. + self.closed = True + def _install_fake_caldav(monkeypatch): fake = types.ModuleType("caldav")