Write against the file, not a stale cache; stop indexing conflict files
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 <noreply@anthropic.com>
This commit is contained in:
@@ -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"
|
||||
|
||||
@@ -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:
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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()
|
||||
|
||||
Reference in New Issue
Block a user