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
This commit is contained in:
tanmayraut45
2026-07-11 17:33:24 +05:30
committed by GitHub
parent 6efc07c5fc
commit e6d9d68729
4 changed files with 347 additions and 193 deletions
+190 -188
View File
@@ -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:
+8 -5
View File
@@ -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:
+145
View File
@@ -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()
@@ -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")