From 2552545265a802ef2bb38443d467633d1190db32 Mon Sep 17 00:00:00 2001 From: Tyler Perkins Date: Sun, 2 Aug 2026 14:01:33 -0400 Subject: [PATCH] Fix tasks reverting after a client marks them done MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Completing a task in a CalDAV client wrote through to the markdown correctly, then the client silently un-completed it moments later. `to_ics` stamped DTSTAMP and LAST-MODIFIED with the wall clock on every serialization, and Radicale derives an item's ETag by hashing that serialization. So an unchanged task minted a fresh ETag on every read, at one-second granularity. A client that read the task, waited for the user to tick the checkbox, then PUT with `If-Match: ` always got 412 Precondition Failed — and resolved that apparent conflict by re-downloading the server copy, discarding the completion. Persist a per-task `last_modified` in the sidecar instead, stamped only when the task's content or status actually changes, and serve DTSTAMP, LAST-MODIFIED and the Item's own last_modified from it. The ETag is now stable while the task is, and changes exactly when the task does. Schema goes to v2, migrated in place with ALTER TABLE: rebuilding the database would regenerate every UID and replace clients' whole task list. Also fixes a plain GET of an item returning 500 — Radicale asserts on `Item.last_modified`, which was never passed. Only REPORT was covered by tests, so nothing caught it. Co-Authored-By: Claude Opus 5 --- src/mdcaldav/ical.py | 8 ++- src/mdcaldav/index.py | 53 ++++++++++++++-- src/mdcaldav/storage.py | 20 +++++-- tests/test_caldav_integration.py | 100 +++++++++++++++++++++++++++++++ 4 files changed, 171 insertions(+), 10 deletions(-) diff --git a/src/mdcaldav/ical.py b/src/mdcaldav/ical.py index b23617a..d5e9640 100644 --- a/src/mdcaldav/ical.py +++ b/src/mdcaldav/ical.py @@ -63,7 +63,13 @@ def to_ics( last_modified: datetime | None = None, completed_at: datetime | None = None, ) -> str: - """Serialize one task as a standalone VCALENDAR containing a VTODO.""" + """Serialize one task as a standalone VCALENDAR containing a VTODO. + + `last_modified` must be stable for as long as the task is unchanged: it + lands in the output, and Radicale hashes the output to make the ETag. A + wall-clock default here would give every read a fresh ETag, and every + conditional PUT a 412. + """ now = last_modified or datetime.now(timezone.utc) lines = [ "BEGIN:VCALENDAR", diff --git a/src/mdcaldav/index.py b/src/mdcaldav/index.py index f836a79..cf925bb 100644 --- a/src/mdcaldav/index.py +++ b/src/mdcaldav/index.py @@ -23,7 +23,7 @@ from .globs import matches_any from .model import Status, Task, normalize_title from .parser import parse -SCHEMA_VERSION = 1 +SCHEMA_VERSION = 2 SCHEMA = """ CREATE TABLE IF NOT EXISTS meta (schema_version INT, generation INT); @@ -37,7 +37,8 @@ CREATE TABLE IF NOT EXISTS tasks ( title_norm TEXT, content_hash TEXT, status TEXT, completed_at TEXT, parent_uid TEXT, - first_seen_gen INT, last_modified_gen INT, deleted_gen INT); + first_seen_gen INT, last_modified_gen INT, deleted_gen INT, + last_modified TEXT); CREATE INDEX IF NOT EXISTS tasks_lookup ON tasks(rel_path, heading_path, title_norm); CREATE INDEX IF NOT EXISTS tasks_sync ON tasks(last_modified_gen); CREATE INDEX IF NOT EXISTS tasks_deleted ON tasks(deleted_gen); @@ -83,8 +84,20 @@ class Index: "INSERT INTO meta (schema_version, generation) VALUES (?, 0)", (SCHEMA_VERSION,), ) + self._migrate() self.db.commit() + def _migrate(self) -> None: + """Add columns in place rather than rebuilding. + + Dropping the database is safe but expensive: every UID is regenerated, + so clients see their whole task list replaced. Migrating keeps them. + """ + columns = {r["name"] for r in self.db.execute("PRAGMA table_info(tasks)")} + if "last_modified" not in columns: + self.db.execute("ALTER TABLE tasks ADD COLUMN last_modified TEXT") + self.db.execute("UPDATE meta SET schema_version = ?", (SCHEMA_VERSION,)) + # ---- generations ----------------------------------------------------- @property @@ -328,7 +341,7 @@ class Index: def _persist(self, task: Task, rel: str, gen: int, *, is_new: bool) -> None: prior = self.db.execute( "SELECT status, completed_at, content_hash, last_modified_gen," - " first_seen_gen FROM tasks WHERE uid = ?", + " first_seen_gen, last_modified FROM tasks WHERE uid = ?", (task.uid,), ).fetchone() @@ -347,8 +360,18 @@ class Index: ) last_gen = gen if changed else prior["last_modified_gen"] + # The ETag is a hash of the serialized VTODO, which carries this + # timestamp. Re-stamping an unchanged task would give it a new ETag on + # every read, and a client's conditional PUT would then always 412. + last_modified = None if changed else (prior["last_modified"] if prior else None) + last_modified = last_modified or _now() + self.db.execute( - "INSERT OR REPLACE INTO tasks VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", + "INSERT OR REPLACE INTO tasks (" + " uid, collection, rel_path, heading_path, group_path, sibling_index," + " depth, title_norm, content_hash, status, completed_at, parent_uid," + " first_seen_gen, last_modified_gen, deleted_gen, last_modified" + ") VALUES (?,?,?,?,?,?,?,?,?,?,?,?,?,?,?,?)", ( task.uid, task.collection, @@ -365,6 +388,7 @@ class Index: prior["first_seen_gen"] if prior else gen, last_gen, None, + last_modified, ), ) @@ -391,6 +415,27 @@ class Index: return datetime.fromisoformat(row["completed_at"]) return None + def last_modified(self, uid: str) -> datetime: + """When this task last actually changed — the basis of its ETag.""" + row = self.db.execute( + "SELECT last_modified FROM tasks WHERE uid = ?", (uid,) + ).fetchone() + if row and row["last_modified"]: + return datetime.fromisoformat(row["last_modified"]) + return datetime.now(timezone.utc) + + def newest(self, collection: str | None = None) -> datetime: + """Newest task modification, for collection-level Last-Modified.""" + sql = "SELECT MAX(last_modified) AS m FROM tasks WHERE deleted_gen IS NULL" + params: tuple = () + if collection is not None: + sql += " AND collection = ?" + params = (collection,) + row = self.db.execute(sql, params).fetchone() + if row and row["m"]: + return datetime.fromisoformat(row["m"]) + return datetime.now(timezone.utc) + def collections(self) -> list[str]: rows = self.db.execute( "SELECT DISTINCT collection FROM tasks WHERE deleted_gen IS NULL" diff --git a/src/mdcaldav/storage.py b/src/mdcaldav/storage.py index cf4109c..cb774d3 100644 --- a/src/mdcaldav/storage.py +++ b/src/mdcaldav/storage.py @@ -75,16 +75,26 @@ class Collection(BaseCollection): @property def last_modified(self) -> str: - return _http_date(datetime.now(timezone.utc)) + return _http_date(self._storage.index.newest(self._href)) # ---- reading --------------------------------------------------------- def _item(self, uid: str) -> radicale_item.Item | None: - task = self._storage.index.tasks.get(uid) + index = self._storage.index + task = index.tasks.get(uid) if task is None or task.collection != self._href: return None - text = to_ics(task, completed_at=self._storage.index.completed_at(uid)) - return radicale_item.Item(collection=self, href=f"{uid}.ics", text=text) + modified = index.last_modified(uid) + text = to_ics( + task, last_modified=modified, completed_at=index.completed_at(uid) + ) + # Radicale asserts on `last_modified` when serving a plain GET. + return radicale_item.Item( + collection=self, + href=f"{uid}.ics", + text=text, + last_modified=_http_date(modified), + ) def get_multi( self, hrefs: Iterable[str] @@ -260,7 +270,7 @@ class Principal(BaseCollection): @property def last_modified(self) -> str: - return _http_date(datetime.now(timezone.utc)) + return _http_date(self._storage.index.newest()) def get_multi(self, hrefs): for href in hrefs: diff --git a/tests/test_caldav_integration.py b/tests/test_caldav_integration.py index bf9f18f..a5ac136 100644 --- a/tests/test_caldav_integration.py +++ b/tests/test_caldav_integration.py @@ -8,6 +8,7 @@ from __future__ import annotations import threading from pathlib import Path +from urllib.parse import urlparse from wsgiref.simple_server import WSGIRequestHandler, make_server import pytest @@ -216,3 +217,102 @@ def test_external_vim_edit_becomes_visible(server): for t in daily.todos(include_completed=True) } assert "Order more compost" in summaries + + +def _etag(url: str, href: str) -> str: + """The ETag a client would cache for one task.""" + import re + import urllib.request + + body = ( + b'' + b"" + ) + request = urllib.request.Request( + url + href, + data=body, + method="PROPFIND", + headers={"Depth": "0", "Content-Type": "application/xml"}, + ) + payload = urllib.request.urlopen(request, timeout=10).read().decode() + match = re.search(r"<[^>]*getetag[^>]*>([^<]+)<", payload) + assert match, payload + return match.group(1) + + +def test_etag_is_stable_while_the_task_is_unchanged(server): + """Regression: an unchanged task must keep its ETag across reads. + + DTSTAMP/LAST-MODIFIED were stamped with the wall clock on every + serialization, and Radicale hashes the serialization to make the ETag. Every + read therefore minted a new ETag, so a client's conditional PUT always hit + 412 and the client reverted its own edit — a task ticked off in the app + would silently un-tick itself moments later. + """ + import time + + url, _ = server + daily = next(c for c in _calendars(url) if c.url.path.rstrip("/").endswith("daily")) + todo = next( + t + for t in daily.todos() + if str(t.icalendar_component.get("SUMMARY")) == "Call the arborist" + ) + href = urlparse(str(todo.url)).path + + first = _etag(url, href) + time.sleep(1.1) # cross a second boundary — the old bug's granularity + assert _etag(url, href) == first + + +def test_conditional_completion_survives_a_delay(server): + """The end-to-end symptom: tick a task off, and have it stay ticked off.""" + import time + import urllib.request + + url, vault = server + daily = next(c for c in _calendars(url) if c.url.path.rstrip("/").endswith("daily")) + todo = next( + t + for t in daily.todos() + if str(t.icalendar_component.get("SUMMARY")) == "Call the arborist" + ) + href = urlparse(str(todo.url)).path + uid = str(todo.icalendar_component.get("UID")) + held = _etag(url, href) + + time.sleep(1.1) # the user takes a moment before hitting the checkbox + payload = ( + "BEGIN:VCALENDAR\r\nVERSION:2.0\r\nPRODID:-//test//EN\r\nBEGIN:VTODO\r\n" + f"UID:{uid}\r\nDTSTAMP:20260802T120000Z\r\nSUMMARY:Call the arborist\r\n" + "STATUS:COMPLETED\r\nPERCENT-COMPLETE:100\r\nEND:VTODO\r\nEND:VCALENDAR\r\n" + ) + request = urllib.request.Request( + url + href, + data=payload.encode(), + method="PUT", + headers={"Content-Type": "text/calendar", "If-Match": held}, + ) + response = urllib.request.urlopen(request, timeout=10) + assert response.status in (200, 201, 204) + + assert "- [x] [#B] Call the arborist" in (vault / "daily/2026-01-07.md").read_text() + assert _etag(url, href) != held # the client must see the change + + +def test_item_get_returns_the_task(server): + """Regression: Item was built without `last_modified`, so GET asserted (500).""" + import urllib.request + + url, _ = server + daily = next(c for c in _calendars(url) if c.url.path.rstrip("/").endswith("daily")) + todo = next( + t + for t in daily.todos() + if str(t.icalendar_component.get("SUMMARY")) == "Call the arborist" + ) + href = urlparse(str(todo.url)).path + + response = urllib.request.urlopen(url + href, timeout=10) + assert response.status == 200 + assert "SUMMARY:Call the arborist" in response.read().decode()