From 6dd3829cfa5cd6c77cd45f21c75da8bdebe30ce3 Mon Sep 17 00:00:00 2001 From: Tyler Perkins Date: Sun, 2 Aug 2026 15:58:16 -0400 Subject: [PATCH] Write against the file, not a stale cache; stop indexing conflict files MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two deployment bugs that both present as "the client says done but the markdown never changes, and nothing errors". 1. upload() diffed the client's VTODO against the cached in-memory task. When that cache disagreed with disk the diff came out empty, apply() returned early, and Radicale answered 2xx having written nothing. The cache goes stale on NFS: refresh_if_stale() compares stat(), and a `- [ ]` to `- [x]` toggle does not change the file's size, so mtime is the only signal — which NFS attribute caching hides for the length of the attr timeout. Add Index.refresh_file(), which compares the actual bytes, and call it before diffing. 2. docker/mdcaldav.toml set `exclude`, which REPLACES the built-in list rather than extending it, silently dropping the Syncthing patterns. Against a real Syncthing vault this reindexes `*.sync-conflict-*` (and `.stversions/`, which holds whole historical copies of every note): the task list gains duplicates, and completing one can write to a conflict file the user never opens while the note they are watching stays put. Drop the override so the defaults apply. Also switch the container to a polling observer. inotify reports nothing for a network mount written by another host, so `watch = true` was a silent no-op exactly where it was most wanted. Co-Authored-By: Claude Opus 5 --- docker/mdcaldav.toml | 18 +++++++++--- src/mdcaldav/index.py | 20 ++++++++++++++ src/mdcaldav/storage.py | 9 ++++++ tests/test_writer.py | 61 +++++++++++++++++++++++++++++++++++++++++ 4 files changed, 104 insertions(+), 4 deletions(-) diff --git a/docker/mdcaldav.toml b/docker/mdcaldav.toml index 622d5bb..4594775 100644 --- a/docker/mdcaldav.toml +++ b/docker/mdcaldav.toml @@ -1,11 +1,21 @@ [vault] path = "/vault" include = ["**/*.md"] -exclude = [".git/**", ".zk/**", ".obsidian/**", "assets/**"] -# Long-running server: keep the index warm off the request path. +# `exclude` is deliberately not set: setting it REPLACES the built-in list, +# which is where the Syncthing patterns live (`.stversions/**`, which holds +# whole historical copies of your notes, and `*.sync-conflict-*`). Overriding +# it with just the VCS/editor globs silently reindexes both — you get duplicate +# tasks, and a completion can land in a conflict file you never open while the +# note you are actually looking at stays unchanged. +# To add your own, restate the defaults from config.DEFAULT_EXCLUDE too. +# Keeps the index warm off the request path. Purely a latency optimization: +# every read path calls refresh_if_stale(), so freshness does not depend on it. watch = true -# Bind mounts sometimes drop inotify events; uncomment to poll instead. -# poll_interval = 2.0 +# Poll rather than wait on inotify. A containerized vault is usually a bind +# mount, and often a network one (NFS, CIFS) — inotify reports nothing at all +# for changes written by another host, so the watcher would sit silent. Set to +# 0 to use inotify when the vault is genuinely local. +poll_interval = 2.0 [collections] group_by = "directory" diff --git a/src/mdcaldav/index.py b/src/mdcaldav/index.py index 2beb60c..b4ce224 100644 --- a/src/mdcaldav/index.py +++ b/src/mdcaldav/index.py @@ -205,6 +205,26 @@ class Index: self.rescan(stale) return True + def refresh_file(self, rel: str) -> bool: + """Reindex `rel` if its bytes differ from what we recorded. + + `refresh_if_stale` trusts stat(), which is not good enough before a + write. On NFS, attribute caching can hide an edit for the length of the + attribute timeout, and toggling `- [ ]` to `- [x]` does not change the + file's size — so a stale entry can survive the stat comparison + indefinitely. Reading the bytes is authoritative. + """ + with self.lock: + path = self.vault / rel + try: + data = path.read_bytes() + except OSError: + return False + if self.file_hash_of(rel) == file_hash(data): + return False + self.rescan([rel]) + return True + def file_hash_of(self, rel: str) -> str | None: """The hash the index last recorded for a file, or None if unknown.""" with self.lock: diff --git a/src/mdcaldav/storage.py b/src/mdcaldav/storage.py index cb774d3..05272a5 100644 --- a/src/mdcaldav/storage.py +++ b/src/mdcaldav/storage.py @@ -128,6 +128,15 @@ class Collection(BaseCollection): uid = href.removesuffix(".ics") task = index.tasks.get(uid) + if task is not None: + # Diff against the bytes on disk, not against whatever the last + # read happened to cache. If the index is stale, the diff comes out + # empty, `apply` no-ops, and we answer 2xx without writing anything + # — the client shows the task done while the markdown still says + # `- [ ]`, with no error anywhere to explain it. + index.refresh_file(task.source.rel_path) + task = index.tasks.get(uid) + try: if task is None: fields.setdefault("uid", uid) diff --git a/tests/test_writer.py b/tests/test_writer.py index 292b2e6..c1ff007 100644 --- a/tests/test_writer.py +++ b/tests/test_writer.py @@ -3,11 +3,13 @@ from __future__ import annotations import difflib +import os from pathlib import Path import pytest from mdcaldav.config import Config +from mdcaldav.ical import apply_to_task from mdcaldav.index import Index from mdcaldav.model import Status from mdcaldav.writer import ConflictError, NotAllowedError, Writer @@ -283,3 +285,62 @@ def test_atomic_write_leaves_no_temp_files(env): {"status": Status.COMPLETED}, ) assert not list((vault / "daily").glob(".*tmp*")) + + +def test_upload_writes_even_when_the_index_is_stale(vault, tmp_path): + """Regression: a stale index made completions silently no-op. + + `upload` diffed the client's VTODO against the cached in-memory task. If + that cache disagreed with disk, the diff came out empty, `apply` returned + early, and Radicale answered 2xx without touching the markdown — the client + showed the task done while the file still said `- [ ]`. + + Reproduced the way NFS produces it: the file changes, but the stat() the + index recorded still matches, so `refresh_if_stale` sees nothing. A status + toggle does not change the file's size, so only mtime could betray it — + and NFS attribute caching hides that for the length of the attr timeout. + """ + from mdcaldav.config import Config + from mdcaldav.index import Index + from mdcaldav.model import Status + + cfg = Config() + cfg.vault.path = vault + cfg.index.db = tmp_path / "index.db" + index = Index(cfg) + index.rescan() + writer = Writer(cfg, index) + + rel = "daily/2026-01-07.md" + path = vault / rel + uid = next( + t.uid + for t in index.tasks.values() + if t.title == "Call the arborist" and t.source.rel_path == rel + ) + + # Complete it, so index and disk agree that it is done. + writer.apply(uid, {"status": Status.COMPLETED}) + assert "- [x] [#B] Call the arborist" in path.read_text() + + # Now it is un-ticked in vim, and the change is invisible to stat(): + # same size, and we pin mtime/size back to what the index recorded. + recorded = index.db.execute( + "SELECT mtime, size FROM files WHERE rel_path = ?", (rel,) + ).fetchone() + path.write_text(path.read_text().replace("- [x] [#B] Call", "- [ ] [#B] Call")) + os.utime(path, (recorded["mtime"], recorded["mtime"])) + assert path.stat().st_size == recorded["size"] + assert not index.refresh_if_stale() # the staleness is genuinely hidden + assert index.tasks[uid].status is Status.COMPLETED # index disagrees with disk + + # The client PUTs "completed" again. Diffing against the stale cache yields + # no changes at all; only re-reading the file reveals the real difference. + assert apply_to_task(index.tasks[uid], {"status": Status.COMPLETED}, cfg) == {} + + index.refresh_file(rel) + changes = apply_to_task(index.tasks[uid], {"status": Status.COMPLETED}, cfg) + assert changes == {"status": Status.COMPLETED} + writer.apply(uid, changes) + + assert "- [x] [#B] Call the arborist" in path.read_text()