From 0877f7a6948d1f2f1dfa24bfe61f058f50c3420c Mon Sep 17 00:00:00 2001 From: Marijn Bent Date: Sun, 20 Sep 2026 20:33:05 +0200 Subject: [PATCH] fix(calendar): report incomplete availability checks --- docs/ARCHITECTURE.md | 2 ++ internal/caldav/caldav.go | 6 +----- internal/caldav/freebusy.go | 6 ++---- internal/calendar/calendar.go | 16 +------------- internal/calendar/microsoft/freebusy.go | 7 +----- internal/calendar/microsoft/microsoft.go | 4 +--- internal/gcal/freebusy.go | 20 ++++++++---------- internal/gcal/freebusy_test.go | 27 +++++++++++++++++++----- internal/gcal/gcal.go | 8 +------ 9 files changed, 40 insertions(+), 56 deletions(-) diff --git a/docs/ARCHITECTURE.md b/docs/ARCHITECTURE.md index 58defaab..cc78f654 100644 --- a/docs/ARCHITECTURE.md +++ b/docs/ARCHITECTURE.md @@ -449,6 +449,8 @@ committed booking) — which is why the reconciler (§11) exists. ## 10. Calendar integration (provider abstraction) +Availability checks return an error if any selected conflict calendar cannot be checked. Partial provider responses and unreadable busy periods are not treated as free time. An unavailable calendar can therefore temporarily prevent booking; reconnect it or deselect it from conflict checks to restore availability. + Calnode talks to calendars through a **provider abstraction**, not a single vendor: - **`internal/calendar`** defines the `Provider` interface (Name, InvitesGuests, diff --git a/internal/caldav/caldav.go b/internal/caldav/caldav.go index d37e0cf0..9dabe073 100644 --- a/internal/caldav/caldav.go +++ b/internal/caldav/caldav.go @@ -182,9 +182,6 @@ func (c *Client) loadConn(ctx context.Context, userID string, checkConflicts, is return cn, true, nil } -// conflictConns returns every CalDAV connection for the user with check_conflicts = 1, so a -// user can connect several CalDAV accounts and have them all checked. Decrypt failures on one -// row are logged and skipped (fail-open). func (c *Client) conflictConns(ctx context.Context, userID string) ([]conn, error) { rows, err := c.db.QueryContext(ctx, ` SELECT id, COALESCE(account_email,''), access_token_enc, calendar_id @@ -222,8 +219,7 @@ func (c *Client) conflictConns(ctx context.Context, userID string) ([]conn, erro } pw, err := c.decrypt(d.pwEnc) if err != nil { - c.logger.Warn("caldav: skipping connection with bad credentials", "user_id", userID, "error", err) - continue + return nil, err } for _, calURL := range calIDs { conns = append(conns, conn{id: d.id, username: d.username, password: string(pw), calURL: calURL}) diff --git a/internal/caldav/freebusy.go b/internal/caldav/freebusy.go index 186d1984..d7439b77 100644 --- a/internal/caldav/freebusy.go +++ b/internal/caldav/freebusy.go @@ -32,8 +32,7 @@ const calendarQueryTmpl = ` // FreeBusy returns the union of busy intervals across every CalDAV account the user has // connected with check_conflicts = 1. Each account is queried with a CalDAV calendar-query -// REPORT; transparent and cancelled events don't block. Fail-open per account: a connection -// that errors is logged and skipped (a flaky calendar never blocks availability). +// REPORT; transparent and cancelled events don't block. func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time) ([]slots.Interval, error) { conns, err := c.conflictConns(ctx, userID) if err != nil { @@ -47,8 +46,7 @@ func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time for _, cn := range conns { iv, err := c.freeBusyForConn(ctx, cn, body) if err != nil { - c.logger.Warn("caldav: freebusy connection failed", "user_id", userID, "calendar", cn.calURL, "error", err) - continue + return nil, err } out = append(out, iv...) } diff --git a/internal/calendar/calendar.go b/internal/calendar/calendar.go index afaade9c..09dbf14b 100644 --- a/internal/calendar/calendar.go +++ b/internal/calendar/calendar.go @@ -348,29 +348,15 @@ func (s *Service) InvitesGuests(ctx context.Context, userID string) bool { return false } -// FreeBusy returns the UNION of busy intervals for the user across EVERY connected provider -// (each provider internally unions its own connected accounts with check_conflicts = 1). This -// is what lets a user connect multiple calendars and have them all checked. Fail-open: a -// provider that errors is skipped; an error is returned only if every provider failed (so a -// flaky calendar never blocks availability or a booking). func (s *Service) FreeBusy(ctx context.Context, userID string, from, to time.Time) ([]slots.Interval, error) { var out []slots.Interval - var firstErr error - anyOK := false for _, p := range s.providers { iv, err := p.FreeBusy(ctx, userID, from, to) if err != nil { - if firstErr == nil { - firstErr = err - } - continue + return nil, err } - anyOK = true out = append(out, iv...) } - if !anyOK && firstErr != nil { - return nil, firstErr - } return out, nil } diff --git a/internal/calendar/microsoft/freebusy.go b/internal/calendar/microsoft/freebusy.go index 14083075..2122ae18 100644 --- a/internal/calendar/microsoft/freebusy.go +++ b/internal/calendar/microsoft/freebusy.go @@ -32,10 +32,6 @@ type calendarViewResp struct { NextLink string `json:"@odata.nextLink"` } -// FreeBusy returns the UNION of busy intervals for userID in [from, to) across every -// connected Microsoft account with check_conflicts = 1 (every event in the window counts as -// busy). Returns (nil, nil) if the user has no such connection. Fail-open: an account that -// errors is logged and skipped. func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time) ([]slots.Interval, error) { conns, err := c.freeBusyConnections(ctx, userID) if err != nil { @@ -45,8 +41,7 @@ func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time for _, fc := range conns { intervals, err := c.freeBusyForConn(ctx, fc, from, to) if err != nil { - c.logger.Warn("microsoft: freebusy for connection failed, skipping", "user_id", userID, "error", err) - continue + return nil, err } out = append(out, intervals...) } diff --git a/internal/calendar/microsoft/microsoft.go b/internal/calendar/microsoft/microsoft.go index 6194e0a2..8005fa7b 100644 --- a/internal/calendar/microsoft/microsoft.go +++ b/internal/calendar/microsoft/microsoft.go @@ -290,7 +290,6 @@ func (c *Client) httpClient(ctx context.Context, userID string, checkConflicts, // freeBusyConnections returns an authorized client for EVERY Microsoft connection the user // has with check_conflicts = 1 (so several connected Microsoft accounts are all checked). -// Bad-credential rows are logged and skipped (fail-open). // msFBConn is one conflict-check account's authorized client plus which calendars to read. // useDefault means query /me/calendarView (the account's default calendar) — the pre-picker // behaviour, used when the user hasn't selected sub-calendars; otherwise calIDs holds the @@ -336,8 +335,7 @@ func (c *Client) freeBusyConnections(ctx context.Context, userID string) ([]msFB } hc, err := c.buildClient(ctx, userID, d.accessEnc, d.refreshEnc, d.calID, d.expiryStr, d.accountEmail) if err != nil { - c.logger.Warn("microsoft: skipping connection with bad credentials", "user_id", userID, "error", err) - continue + return nil, err } // Unconfigured accounts fall back to the stored "primary" placeholder id, which is not a // real Graph calendar id — read the default calendar view for those. diff --git a/internal/gcal/freebusy.go b/internal/gcal/freebusy.go index 4e6a547f..23f0e27e 100644 --- a/internal/gcal/freebusy.go +++ b/internal/gcal/freebusy.go @@ -19,18 +19,14 @@ type freeBusyReq struct { type freeBusyResp struct { Calendars map[string]struct { - Busy []struct { + Errors []json.RawMessage `json:"errors"` + Busy []struct { Start string `json:"start"` End string `json:"end"` } `json:"busy"` } `json:"calendars"` } -// FreeBusy returns the UNION of Google Calendar busy intervals for userID in [from, to) -// across every connected Google account with check_conflicts = 1 (so all of a user's -// connected calendars are honoured, not just one). Returns (nil, nil) if the user has no such -// connection. Fail-open: a single account that errors is logged and skipped, so a flaky -// connection never blocks availability or a booking. func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time) ([]slots.Interval, error) { conns, err := c.freeBusyConnections(ctx, userID) if err != nil { @@ -40,8 +36,7 @@ func (c *Client) FreeBusy(ctx context.Context, userID string, from, to time.Time for _, conn := range conns { intervals, err := c.freeBusyForConn(ctx, conn, from, to) if err != nil { - c.logger.Warn("gcal: freebusy for connection failed, skipping", "user_id", userID, "error", err) - continue + return nil, err } out = append(out, intervals...) } @@ -84,13 +79,16 @@ func (c *Client) freeBusyForConn(ctx context.Context, conn fbConn, from, to time return nil, fmt.Errorf("gcal: freeBusy decode: %w", err) } var out []slots.Interval - for _, cal := range fbr.Calendars { + for _, id := range conn.calIDs { + cal, ok := fbr.Calendars[id] + if !ok || len(cal.Errors) > 0 { + return nil, fmt.Errorf("gcal: calendar busy status unavailable") + } for _, b := range cal.Busy { s, err1 := time.Parse(time.RFC3339, b.Start) e, err2 := time.Parse(time.RFC3339, b.End) if err1 != nil || err2 != nil { - c.logger.Warn("gcal: skipping unparseable busy interval", "start", b.Start, "end", b.End) - continue + return nil, fmt.Errorf("gcal: invalid busy interval") } out = append(out, slots.Interval{Start: s, End: e}) } diff --git a/internal/gcal/freebusy_test.go b/internal/gcal/freebusy_test.go index bc365712..b162b8a3 100644 --- a/internal/gcal/freebusy_test.go +++ b/internal/gcal/freebusy_test.go @@ -138,9 +138,7 @@ func TestFreeBusy_notConnected_returnsNil(t *testing.T) { } } -func TestFreeBusy_nonOK_failsOpen(t *testing.T) { - // A connection returning non-200 is skipped (fail-open), not surfaced as an error — - // a flaky calendar must never block availability or a booking. +func TestFreeBusy_nonOK_rejectsBookingCheck(t *testing.T) { srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { http.Error(w, "unauthorized", http.StatusUnauthorized) })) @@ -151,8 +149,8 @@ func TestFreeBusy_nonOK_failsOpen(t *testing.T) { saveAndConnectClient(t, c, "user-1", "primary", "bad-token") got, err := c.FreeBusy(context.Background(), "user-1", time.Now(), time.Now().Add(time.Hour)) - if err != nil { - t.Errorf("fail-open: expected nil error, got %v", err) + if err == nil { + t.Error("expected an error from failed calendar check") } if len(got) != 0 { t.Errorf("expected no busy intervals from a failed connection, got %d", len(got)) @@ -179,3 +177,22 @@ func TestFreeBusy_onlyCheckConflictsConnections(t *testing.T) { t.Errorf("got intervals from check_conflicts=0 connection; want nil") } } + +func TestFreeBusy_rejectsIncompleteCalendarResponse(t *testing.T) { + for _, body := range []string{ + `{"calendars":{}}`, + `{"calendars":{"primary":{"errors":[{"reason":"notFound"}],"busy":[]}}}`, + `{"calendars":{"primary":{"busy":[{"start":"invalid","end":"2026-06-15T10:00:00Z"}]}}}`, + } { + t.Run(body, func(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { w.Write([]byte(body)) })) + defer srv.Close() + c := newTestClient(t) + c.apiBase = srv.URL + saveAndConnectClient(t, c, "user-1", "primary", "token") + if _, err := c.FreeBusy(context.Background(), "user-1", time.Now(), time.Now().Add(time.Hour)); err == nil { + t.Fatal("expected calendar check failure") + } + }) + } +} diff --git a/internal/gcal/gcal.go b/internal/gcal/gcal.go index 29b13e0b..5deb5559 100644 --- a/internal/gcal/gcal.go +++ b/internal/gcal/gcal.go @@ -225,11 +225,6 @@ type fbConn struct { calIDs []string } -// freeBusyConnections returns an authorized client for EVERY Google connection the user has -// with check_conflicts = 1 (so a user can connect several Google accounts and have them all -// checked for conflicts), each paired with the set of that account's calendars to check for -// conflicts (per-account sub-calendar selection). Decrypt failures on one row are logged and -// skipped (fail-open); an account whose calendars are all deselected is dropped. func (c *Client) freeBusyConnections(ctx context.Context, userID string) ([]fbConn, error) { rows, err := c.db.QueryContext(ctx, ` SELECT access_token_enc, COALESCE(refresh_token_enc,''), calendar_id, COALESCE(expiry_at,''), COALESCE(account_email,'') @@ -265,8 +260,7 @@ func (c *Client) freeBusyConnections(ctx context.Context, userID string) ([]fbCo } hc, err := c.buildClient(ctx, userID, d.accessEnc, d.refreshEnc, d.calID, d.expiryStr, d.accountEmail) if err != nil { - c.logger.Warn("gcal: skipping connection with bad credentials", "user_id", userID, "error", err) - continue + return nil, fmt.Errorf("gcal: load conflict calendar credentials: %w", err) } conns = append(conns, fbConn{hc: hc, calIDs: calIDs}) }