diff --git a/cmd/mavcaldav/main.go b/cmd/mavcaldav/main.go index 1417ed0..4c120bd 100644 --- a/cmd/mavcaldav/main.go +++ b/cmd/mavcaldav/main.go @@ -66,7 +66,7 @@ func run(args []string) error { if *url == "" || *user == "" || *pass == "" { return fmt.Errorf("-url, -user, -pass are required") } - if err := checkRenderTarget(*url, *renderURL); err != nil { + if err := checkRenderTarget([]string{*url}, *renderURL); err != nil { return err } @@ -122,17 +122,26 @@ func run(args []string) error { } } -// checkRenderTarget refuses a render URL that is also a read URL. This is the -// structural half of #127's "cannot write to your work calendar": the write -// credential and the write URL are separate flags, and the one calendar maven -// is known to only read is rejected as a target at startup rather than trusted -// at runtime. -func checkRenderTarget(readURL, renderURL string) error { +// checkRenderTarget refuses a render URL that is also one of the read URLs. +// This is the structural half of #127's "cannot write to your work calendar": +// the write credential and the write URL are separate flags, and a calendar +// maven is known to only read is rejected as a target at startup rather than +// trusted at runtime. +// +// It takes the whole read set, not one URL. The guarantee in the package +// comment is about every calendar maven reads, and a second read target added +// later must not quietly fall outside the check. +func checkRenderTarget(readURLs []string, renderURL string) error { if renderURL == "" { return nil } - if sameCollection(readURL, renderURL) { - return fmt.Errorf("-render-url must differ from -url: maven renders into a calendar she owns, never into one she reads") + for _, read := range readURLs { + if read == "" { + continue + } + if sameCollection(read, renderURL) { + return fmt.Errorf("-render-url must differ from the read URL %s: maven renders into a calendar she owns, never into one she reads", read) + } } return nil } @@ -163,7 +172,7 @@ func (p *poller) pollOnce(ctx context.Context) { } // Write calendar_busy on change. - if err := p.writeIfChanged(ctx, "calendar_busy", calendar.SourcePersonal, busyVal, now, 1.0); err != nil { + if err := p.writeIfChanged(ctx, "calendar_busy", calendar.SourcePersonal, busyVal, now); err != nil { log.Printf("mavcaldav: write calendar_busy: %v", err) return } @@ -173,7 +182,7 @@ func (p *poller) pollOnce(ctx context.Context) { // reaching back to Radicale. for _, e := range events { key := calendar.FactKey(e) - if err := p.writeIfChanged(ctx, key, calendar.SourcePersonal, calendar.FactValue(e), e.Start, 1.0); err != nil { + if err := p.writeIfChanged(ctx, key, calendar.SourcePersonal, calendar.FactValue(e), e.Start); err != nil { log.Printf("mavcaldav: write %s: %v", key, err) } } @@ -206,7 +215,10 @@ func (p *poller) fetchEvents(ctx context.Context, now time.Time) ([]calendar.Eve } // writeIfChanged writes a fact only when the value differs from the latest. -func (p *poller) writeIfChanged(ctx context.Context, key, source, val string, ts time.Time, confidence float64) error { +// Everything this poller writes is a calendar read, which is full confidence by +// definition; a source that is not, such as the notification relay, does not +// come through here. +func (p *poller) writeIfChanged(ctx context.Context, key, source, val string, ts time.Time) error { prev, err := p.core.LatestFactBySource(ctx, key, source) switch { case err == nil && prev.Value == val: @@ -220,7 +232,7 @@ func (p *poller) writeIfChanged(ctx context.Context, key, source, val string, ts Key: key, Value: val, Source: source, - Confidence: confidence, + Confidence: 1.0, }) if err != nil { return fmt.Errorf("write %s: %w", key, err) diff --git a/cmd/mavcaldav/main_test.go b/cmd/mavcaldav/main_test.go index 30c46dc..c3d82d3 100644 --- a/cmd/mavcaldav/main_test.go +++ b/cmd/mavcaldav/main_test.go @@ -62,7 +62,7 @@ func TestWriteIfChanged(t *testing.T) { t.Run("no previous fact writes", func(t *testing.T) { fc := &fakeCore{} p := &poller{core: fc} - err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "hello", now, 1.0) + err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "hello", now) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -90,7 +90,7 @@ func TestWriteIfChanged(t *testing.T) { }, } p := &poller{core: fc} - err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "hello", now, 1.0) + err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "hello", now) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -106,7 +106,7 @@ func TestWriteIfChanged(t *testing.T) { }, } p := &poller{core: fc} - err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "new", now, 1.0) + err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "new", now) if err != nil { t.Fatalf("unexpected error: %v", err) } @@ -121,7 +121,7 @@ func TestWriteIfChanged(t *testing.T) { t.Run("read error other than ErrNoFact returns error", func(t *testing.T) { fc := &fakeCore{readErr: fmt.Errorf("connection refused")} p := &poller{core: fc} - err := p.writeIfChanged(ctx, "fail_key", "poll:caldav", "x", now, 1.0) + err := p.writeIfChanged(ctx, "fail_key", "poll:caldav", "x", now) if err == nil { t.Fatal("expected error, got nil") } @@ -133,7 +133,7 @@ func TestWriteIfChanged(t *testing.T) { writeErr: fmt.Errorf("disk full"), } p := &poller{core: fc} - err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "hello", now, 1.0) + err := p.writeIfChanged(ctx, "test_key", "poll:caldav", "hello", now) if err == nil { t.Fatal("expected error, got nil") } @@ -190,13 +190,15 @@ func TestPollOnce(t *testing.T) { t.Errorf("calendar_busy ts is zero") } - // Second write: calendar_event__ = " @ HH:MM-HH:MM" + // Second write: calendar_event__ = " @ HH:MM-HH:MM". + // The iCal states the event in UTC and the fact is stamped on the owner's + // clock, so the expected key date and times are the local reading of it. eventReq := fc.writeLog[1] - expectedKey := "calendar_event_" + start.Format("20060102") + "_Current-meeting" + expectedKey := "calendar_event_" + start.Local().Format("20060102") + "_Current-meeting" if eventReq.Key != expectedKey { t.Errorf("event key = %q, want %q", eventReq.Key, expectedKey) } - expectedVal := "Current meeting @ " + start.Format("15:04") + "-" + end.Format("15:04") + expectedVal := "Current meeting @ " + start.Local().Format("15:04") + "-" + end.Local().Format("15:04") if eventReq.Value != expectedVal { t.Errorf("event value = %q, want %q", eventReq.Value, expectedVal) } diff --git a/cmd/mavcaldav/render.go b/cmd/mavcaldav/render.go index ee54e2d..4568317 100644 --- a/cmd/mavcaldav/render.go +++ b/cmd/mavcaldav/render.go @@ -2,15 +2,18 @@ package main import ( "context" + "encoding/xml" "fmt" "io" "log" "net/http" + "net/url" "strings" "time" "github.com/kami/maven/internal/calendar" "github.com/kami/maven/internal/ipc" + "github.com/kami/maven/internal/store" ) // renderer is the write half of maven's own local calendar (Vikunja #127). @@ -38,6 +41,13 @@ type renderer struct { // unchanged reminder costs nothing. Purely an optimisation: a restart // re-publishes every reminder once, which is idempotent. published map[int64]string + + // reconciled — whether the collection has been read once since start. It + // has to be, because published is in-memory: withdrawal used to cover only + // the reminders THIS process published, so a reminder that fired while the + // daemon was down kept its event in the calendar forever, and nothing ever + // revisited it. + reconciled bool } func newRenderer(core ipc.CoreAPI, hc *http.Client, url, user, pass string, dur time.Duration) *renderer { @@ -64,7 +74,7 @@ func (r *renderer) renderOnce(ctx context.Context) { live := make(map[int64]bool, len(reminders)) for _, rem := range reminders { - if rem.Status != "pending" { + if rem.Status != store.ReminderPending { continue } live[rem.ID] = true @@ -81,10 +91,28 @@ func (r *renderer) renderOnce(ctx context.Context) { log.Printf("mavcaldav: rendered reminder %d (%s)", rem.ID, e.Summary) } + stale := make(map[int64]bool) for id := range r.published { - if live[id] { - continue + if !live[id] { + stale[id] = true } + } + if !r.reconciled { + remote, err := r.listPublished(ctx) + if err != nil { + // Try again next tick. A collection maven cannot read is not a + // reason to stop publishing to it. + log.Printf("mavcaldav: reconcile: %v", err) + } else { + r.reconciled = true + for _, id := range remote { + if !live[id] { + stale[id] = true + } + } + } + } + for id := range stale { if err := r.delete(ctx, calendar.ReminderPath(id)); err != nil { log.Printf("mavcaldav: withdraw reminder %d: %v", id, err) continue @@ -94,6 +122,55 @@ func (r *renderer) renderOnce(ctx context.Context) { } } +// listPublished PROPFINDs the collection and returns the reminder ids maven has +// events for in it. Only resources carrying calendar.ReminderUIDPrefix are +// reported, so a reconciliation pass can never propose deleting a file maven +// did not create — the same bound every other path in this file has. +func (r *renderer) listPublished(ctx context.Context) ([]int64, error) { + const body = `` + + `` + req, err := http.NewRequestWithContext(ctx, "PROPFIND", r.url+"/", strings.NewReader(body)) + if err != nil { + return nil, err + } + req.SetBasicAuth(r.user, r.pass) + req.Header.Set("Content-Type", "application/xml; charset=utf-8") + req.Header.Set("Depth", "1") + + resp, err := r.http.Do(req) + if err != nil { + return nil, err + } + defer resp.Body.Close() + raw, err := io.ReadAll(io.LimitReader(resp.Body, 4<<20)) + if err != nil { + return nil, err + } + if resp.StatusCode != http.StatusMultiStatus && (resp.StatusCode < 200 || resp.StatusCode >= 300) { + return nil, fmt.Errorf("PROPFIND %s: %s", r.url, resp.Status) + } + + var ms struct { + Responses []struct { + Href string `xml:"href"` + } `xml:"response"` + } + if err := xml.Unmarshal(raw, &ms); err != nil { + return nil, fmt.Errorf("PROPFIND %s: %w", r.url, err) + } + var ids []int64 + for _, resp := range ms.Responses { + href, err := url.PathUnescape(strings.TrimSpace(resp.Href)) + if err != nil { + continue + } + if id, ok := calendar.ReminderIDFromPath(href); ok { + ids = append(ids, id) + } + } + return ids, nil +} + // renderMaxReminders bounds the read. Reminders past this count are older than // anything a calendar view is useful for. const renderMaxReminders = 200 diff --git a/cmd/mavcaldav/render_test.go b/cmd/mavcaldav/render_test.go index 2ba0f07..53c759d 100644 --- a/cmd/mavcaldav/render_test.go +++ b/cmd/mavcaldav/render_test.go @@ -2,9 +2,11 @@ package main import ( "context" + "errors" "io" "net/http" "net/http/httptest" + "slices" "strings" "sync" "testing" @@ -27,12 +29,16 @@ func (c *reminderCore) ListReminders(context.Context, int) ([]ipc.Reminder, erro return c.reminders, nil } -// calSrv records what a CalDAV collection received. +// calSrv records what a CalDAV collection received. existing seeds resources +// that were already in the collection before this process started, which is +// what a restart looks like from the renderer's side. type calSrv struct { - mu sync.Mutex - puts map[string]string - dels []string - status int + mu sync.Mutex + puts map[string]string + dels []string + existing []string + propfind int + status int *httptest.Server } @@ -47,12 +53,43 @@ func newCalSrv() *calSrv { s.puts[strings.TrimPrefix(r.URL.Path, "/cal/")] = string(body) case http.MethodDelete: s.dels = append(s.dels, strings.TrimPrefix(r.URL.Path, "/cal/")) + case "PROPFIND": + s.propfind++ + w.Header().Set("Content-Type", "application/xml; charset=utf-8") + w.WriteHeader(http.StatusMultiStatus) + io.WriteString(w, s.multistatusLocked(r.URL.Path)) + return } w.WriteHeader(s.status) })) return s } +// multistatusLocked renders the collection listing. Caller holds the lock. +func (s *calSrv) multistatusLocked(base string) string { + var b strings.Builder + b.WriteString(``) + b.WriteString("" + base + "") + names := append([]string{}, s.existing...) + for name := range s.puts { + names = append(names, name) + } + for _, name := range names { + if slices.Contains(s.dels, name) { + continue + } + b.WriteString("/cal/" + name + "") + } + b.WriteString("") + return b.String() +} + +func (s *calSrv) deleted() []string { + s.mu.Lock() + defer s.mu.Unlock() + return append([]string{}, s.dels...) +} + func (s *calSrv) putCount() int { s.mu.Lock() defer s.mu.Unlock() @@ -168,19 +205,97 @@ func TestRenderOnceUsesNextFireForRecurring(t *testing.T) { func TestCheckRenderTargetRefusesTheCalendarItReads(t *testing.T) { read := "http://localhost:5232/kami/personal" - if err := checkRenderTarget(read, ""); err != nil { + if err := checkRenderTarget([]string{read}, ""); err != nil { t.Fatalf("rendering off must be fine: %v", err) } - if err := checkRenderTarget(read, "http://localhost:5232/kami/maven"); err != nil { + if err := checkRenderTarget([]string{read}, "http://localhost:5232/kami/maven"); err != nil { t.Fatalf("a distinct collection must be accepted: %v", err) } - if err := checkRenderTarget(read, read); err == nil { + if err := checkRenderTarget([]string{read}, read); err == nil { t.Error("rendering into the read calendar must be refused") } - if err := checkRenderTarget(read, read+"/"); err == nil { + if err := checkRenderTarget([]string{read}, read+"/"); err == nil { t.Error("a trailing slash must not defeat the check") } - if err := checkRenderTarget(read, strings.ToUpper(read)); err == nil { + if err := checkRenderTarget([]string{read}, strings.ToUpper(read)); err == nil { t.Error("case must not defeat the check") } + // Every read target is checked, not the first one. A second calendar to + // read must not fall outside the guarantee just by being added later. + work := "http://localhost:5232/kami/work" + if err := checkRenderTarget([]string{read, work}, work); err == nil { + t.Error("rendering into the second read calendar must be refused") + } + if err := checkRenderTarget([]string{read, work}, "http://localhost:5232/kami/maven"); err != nil { + t.Fatalf("a collection maven owns must still be accepted: %v", err) + } +} + +// Withdrawal has to survive a restart. published is in-memory, so a fresh +// process knows nothing about the events an earlier one wrote: fire a reminder, +// restart mavcaldav, and its event used to sit in the collection forever +// because nothing ever revisited it. The first tick reads the collection and +// reconciles what it finds against what is pending. +func TestRenderOnceWithdrawsAfterRestart(t *testing.T) { + srv := newCalSrv() + defer srv.Close() + // Left behind by a previous process: 4 is still pending, 5 has fired. + // The third file is not maven's and must not be touched. + srv.existing = []string{"maven-reminder-4.ics", "maven-reminder-5.ics", "dentist.ics"} + + core := &reminderCore{reminders: []ipc.Reminder{ + {ID: 4, FireTs: time.Date(2026, 8, 1, 9, 0, 0, 0, time.UTC), Payload: "выпить воды", Status: "pending"}, + {ID: 5, FireTs: time.Date(2026, 8, 1, 8, 0, 0, 0, time.UTC), Payload: "уже прозвенело", Status: "fired"}, + }} + r := newRenderer(core, srv.Client(), srv.URL+"/cal", "u", "p", 0) + r.renderOnce(context.Background()) + + dels := srv.deleted() + if len(dels) != 1 || dels[0] != "maven-reminder-5.ics" { + t.Fatalf("deleted %v, want only the fired reminder's event", dels) + } + + // The collection is read once, not on every tick. + r.renderOnce(context.Background()) + srv.mu.Lock() + n := srv.propfind + srv.mu.Unlock() + if n != 1 { + t.Errorf("PROPFIND ran %d times, want once per process", n) + } +} + +// A collection maven cannot read is not a reason to stop publishing to it, and +// the reconciliation must be retried rather than skipped for the process. +func TestRenderOnceRetriesReconcile(t *testing.T) { + srv := newCalSrv() + defer srv.Close() + srv.existing = []string{"maven-reminder-6.ics"} + failing := &http.Client{Transport: &propfindFailure{base: srv.Client().Transport}} + + core := &reminderCore{} + r := newRenderer(core, failing, srv.URL+"/cal", "u", "p", 0) + r.renderOnce(context.Background()) + if got := srv.deleted(); len(got) != 0 { + t.Fatalf("nothing can be withdrawn on a failed read: %v", got) + } + if r.reconciled { + t.Fatal("a failed read must not count as reconciled") + } + + r.http = srv.Client() + r.renderOnce(context.Background()) + if got := srv.deleted(); len(got) != 1 || got[0] != "maven-reminder-6.ics" { + t.Fatalf("deleted %v, want the orphaned event on the retry", got) + } +} + +// propfindFailure fails PROPFIND and passes everything else through. +type propfindFailure struct{ base http.RoundTripper } + +func (f *propfindFailure) RoundTrip(req *http.Request) (*http.Response, error) { + if req.Method == "PROPFIND" { + return nil, errors.New("collection unreachable") + } + return f.base.RoundTrip(req) } diff --git a/internal/calendar/reminder.go b/internal/calendar/reminder.go index 86356f2..0847c54 100644 --- a/internal/calendar/reminder.go +++ b/internal/calendar/reminder.go @@ -2,6 +2,7 @@ package calendar import ( "fmt" + "strconv" "strings" "time" ) @@ -43,3 +44,26 @@ func ReminderEvent(id int64, fire time.Time, payload string, dur time.Duration) func ReminderPath(id int64) string { return fmt.Sprintf("%s%d.ics", ReminderUIDPrefix, id) } + +// ReminderIDFromPath reads back what ReminderPath wrote, given an href out of a +// PROPFIND. It reports false for anything that is not a resource maven +// published, which is what keeps a reconciliation pass from touching a file it +// did not create. +func ReminderIDFromPath(href string) (int64, bool) { + name := href + if i := strings.LastIndex(name, "/"); i >= 0 { + name = name[i+1:] + } + if !strings.HasPrefix(name, ReminderUIDPrefix) || !strings.HasSuffix(name, ".ics") { + return 0, false + } + digits := name[len(ReminderUIDPrefix) : len(name)-len(".ics")] + if digits == "" { + return 0, false + } + id, err := strconv.ParseInt(digits, 10, 64) + if err != nil || id <= 0 { + return 0, false + } + return id, true +}