Skip to content

fix(booking): check local bookings across a shared calendar #64

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
shockalotti merged 1 commit into from
Sep 21, 2026
Merged

fix(booking): check local bookings across a shared calendar #64

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions docs/ARCHITECTURE.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -449,6 +449,8 @@ committed booking) — which is why the reconciler (§11) exists.

## 10. Calendar integration (provider abstraction)

Bookings also check pending local bookings owned by other accounts that use the same selected conflict calendar. This check runs inside the booking transaction, before asynchronous calendar creation can finish. Calendar identity currently includes the provider, account email, and calendar ID.

Calnode talks to calendars through a **provider abstraction**, not a single vendor:

- **`internal/calendar`** defines the `Provider` interface (Name, InvitesGuests,
Expand Down
12 changes: 10 additions & 2 deletions internal/booking/service.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -285,9 +285,17 @@ func hostBusy(ctx context.Context, tx *sql.Tx, hostID, start, end, excludeBookin
err := tx.QueryRowContext(ctx, `
SELECT COUNT(*) FROM bookings b
JOIN booking_hosts bh ON bh.booking_id = b.id
WHERE bh.user_id = ? AND b.status != 'cancelled' AND b.id != ?
WHERE (bh.user_id = ? OR EXISTS (
SELECT 1 FROM connection_calendars checked

@pullfrog pullfrog Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This consults only connection_calendars, but an account that connected a calendar and never opened the picker has no rows there — ConflictCalendarIDs deliberately falls back to the connection's bound calendar_id in that case, and DestinationCalendarID returns ok=false so callers keep the account-level calendar. Those accounts are invisible to this check, so the local race the PR closes stays open for them (the default state of a newly connected Google/Microsoft account). Please mirror the no-rows fallback on both the checked and destination sides.

Technical details 
# Accounts without picker rows are invisible to the shared-calendar check

## Affected sites
- `internal/booking/service.go:289` — the `EXISTS` reads only `connection_calendars`, populated only by the picker (`SetAccountCalendars`, `caldav.seedDiscoveredCalendars`) or the `00049` seed.
- `internal/calendar/conflicts.go:46-48` — with no rows, the conflict set falls back to the connection's bound `calendar_id`.
- `internal/connstore/connstore.go:90-105` — `DestinationCalendarID` returns `ok=false` with no rows; callers keep the account-level `calendar_id` (`internal/gcal/gcal.go:207-213`).

## Required outcome
The local check must see the same calendar set the external free/busy path sees, including the no-rows fallback. Otherwise the race this PR closes (two accounts book before the first external write lands) stays open for any account that never opened the picker.

## Suggested approach (optional)
Wrap the picker lookup with a fallback to `calendar_connections.calendar_id` for accounts with no `connection_calendars` rows on both sides; or resolve both sets in Go via tx-scoped helpers mirroring `ConflictCalendarIDs`/`DestinationCalendarID`. The pool is single-connection, so any helper must accept the caller's `*sql.Tx`, not `*sql.DB`, or it deadlocks against the open transaction.

## Evidence
A scratch test with two hosts sharing `(google, shared@example.com, primary)` but no `connection_calendars` rows let both bookings through (second create returned `nil`).

JOIN connection_calendars destination
ON destination.provider = checked.provider
AND destination.account_email = checked.account_email
AND destination.calendar_id = checked.calendar_id
Comment on lines +291 to +293

@pullfrog pullfrog Bot Sep 21, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shared-calendar identity is (provider, account_email, calendar_id), but account_email is explicitly allowed to be empty — gcal.Exchange stores "" when its best-effort email fetch fails, Microsoft's accountEmailFromIDToken does the same, both default calendar_id to "primary", and 00039 backfills existing connections with ''. Two unrelated accounts that both ended up blank (or the same account connected twice with a failed lookup) therefore compare equal, and one host's booking blocks the other's. Please require a non-empty account_email on both sides before treating two rows as the same calendar.

Technical details 
# Cross-account calendar identity is unsound when account_email is blank

## Affected sites
- `internal/booking/service.go:291-293` — the match equates `(provider, account_email, calendar_id)` across users without requiring a non-empty `account_email`.
- `internal/gcal/gcal.go:101` — `email := c.fetchAccountEmail(ctx, tok) // best-effort; "" on failure`; `calendar_id` defaults to `"primary"` at line 99.
- `internal/calendar/microsoft/microsoft.go:114-119` — same default; `accountEmailFromIDToken` returns `""` when the claim is missing.
- `internal/db/migrations/00039_calendar_account_email.sql:6` — pre-existing connections backfill `account_email=''`; `00049` seeds `connection_calendars` from them with `COALESCE(account_email, '')`.

## Required outcome
Two rows that share only a blank `account_email` must not be treated as the same calendar. Require `account_email <> ''` on both `checked` and `destination`, or skip blank-identity rows.

## Suggested regression test
Seed two hosts with `(google, '', 'primary')`; the second create must succeed rather than return `ErrDoubleBooked`.

## Evidence
A scratch test returned `booking: time slot is no longer available` for the second host. `go test ./internal/booking/` passes today only because no fixture seeds a blank `account_email`.

WHERE checked.user_id = ? AND checked.check_conflicts = 1
AND destination.user_id = bh.user_id AND destination.is_destination = 1
)) AND b.status != 'cancelled' AND b.id != ?
AND b.start_at < ? AND b.end_at > ?`,
hostID, excludeBookingID, end, start).Scan(&n)
hostID, hostID, excludeBookingID, end, start).Scan(&n)
return n > 0, err
}

Expand Down
67 changes: 67 additions & 0 deletions internal/booking/shared_calendar_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,67 @@
package booking_test

import (
"context"
"errors"
"sync"
"testing"

"github.com/calnode/calnode/internal/booking"
)

func TestSharedCalendarAtomicConflict(t *testing.T) {
database := newTestDB(t)
svc := booking.New(database)
hosts := []string{seedHost(t, database), seedHost(t, database)}
events := []string{seedEventType(t, database, hosts[0]), seedEventType(t, database, hosts[1])}
for _, host := range hosts {
_, err := database.Exec(`INSERT INTO connection_calendars (id,user_id,provider,account_email,calendar_id,check_conflicts,is_destination) VALUES (?,?,'google','shared@example.com','primary',1,1)`, host, host)
if err != nil {
t.Fatal(err)
}
}
start := make(chan struct{})
results := make(chan error, 2)
ids := make(chan string, 2)
var wg sync.WaitGroup
for i := range hosts {
wg.Add(1)
go func(i int) {
defer wg.Done()
<-start
b, err := svc.Create(context.Background(), booking.CreateParams{EventTypeID: events[i], HostIDs: []string{hosts[i]}, StartAt: slot(9, 0), EndAt: slot(9, 30), Organizer: booking.Attendee{Name: "Test", Email: "test@example.com", IANATimezone: "UTC"}})
results <- err
if b != nil {
ids <- b.ID
}
}(i)
}
close(start)
wg.Wait()
close(results)
successes, conflicts := 0, 0
for err := range results {
if err == nil {
successes++
} else if errors.Is(err, booking.ErrDoubleBooked) {
conflicts++
} else {
t.Fatal(err)
}
}
if successes != 1 || conflicts != 1 {
t.Fatalf("successes=%d conflicts=%d", successes, conflicts)
}
if err := svc.CancelByID(context.Background(), <-ids, "test cleanup"); err != nil {
t.Fatal(err)
}
for i := range hosts {
b, err := svc.Create(context.Background(), booking.CreateParams{EventTypeID: events[i], HostIDs: []string{hosts[i]}, StartAt: slot(9, 0), EndAt: slot(9, 30), Organizer: booking.Attendee{Name: "Test", Email: "test@example.com", IANATimezone: "UTC"}})
if err != nil {
t.Fatal(err)
}
if err := svc.CancelByID(context.Background(), b.ID, "test cleanup"); err != nil {
t.Fatal(err)
}
}
}
Loading