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")