Repository navigation
fix(booking): check local bookings across a shared calendar - #64
Conversation
|
All contributors have signed the CLA. ✅ |
|
I have read the CLA Document and I hereby sign the CLA |
|
This repo's model isn't fully set up yet. OPENAI_COMPATIBLE_MODEL env var is required when the model is set to "openai-compatible/byok". set it to the model ID served by your OpenAI-compatible endpoint (e.g. a Cloudflare AI Gateway or DashScope model). see https://docs.pullfrog.com/openai-compatible for setup.
|
|
recheck |
|
@pullfrog please review this PR |
There was a problem hiding this comment.
Important
The new cross-account match has one false-positive case that can block legitimate bookings and one false-negative case that leaves the original race open. Both are inline.
Reviewed changes
- Cross-account local conflict check (
internal/booking/service.go) —hostBusynow also counts overlapping bookings whose host has a destination calendar matching one of the requesting host's conflict-checked calendars, identified by(provider, account_email, calendar_id). Reached by create, reschedule, reassign, and every booking intake (bookingSvc.Create). - Shared-calendar regression test (
internal/booking/shared_calendar_test.go) — seeds two hosts with identicalconnection_calendarsrows and asserts exactly one of two concurrent creates wins; then asserts cancelling frees the slot. - Architecture note (
docs/ARCHITECTURE.md) — documents the in-transaction check and its identity limits.
ℹ️ Nitpicks
hostBusy's doc comment (just above the change) still says a host is busy only if they attend an overlapping booking; it no longer describes the function, since a host is now also busy when another account's booking lands in a shared calendar.- The test's second phase exercises cancellation, not the "allows a different calendar" case named in the PR description. Adding a distinct-
account_emailcase would pin the no-false-positive direction that the inline comment below describes.
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` and `calendar_id` defaults to `"primary"` (line 99).
- `internal/calendar/microsoft/microsoft.go:114-119` — same default and `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. Either require `account_email <> ''` on both `checked` and `destination`, or skip blank-identity rows.
## Evidence
A scratch test with two hosts, each holding `(google, '', 'primary', check_conflicts=1, is_destination=1)`, returns `ErrDoubleBooked` for the second host — two unrelated accounts (or the same account with two failed email lookups) collide. `go test ./internal/booking/` passes today only because no fixture seeds a blank `account_email`.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`, which is populated only by the calendar picker (`SetAccountCalendars`, `caldav.seedDiscoveredCalendars`) or the `00049` seed.
- `internal/calendar/conflicts.go:46-48` — when an account has 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, and 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 — which is the default state of a newly connected Google/Microsoft account.
## Suggested approach (optional)
Wrap the picker lookup with a fallback to `calendar_connections.calendar_id` for accounts with no `connection_calendars` rows on both the `checked` and `destination` sides; or resolve both sets in Go via tx-scoped helpers mirroring `ConflictCalendarIDs`/`DestinationCalendarID`. Note the pool is single-connection: 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 lets both bookings through (second create returns `nil`), confirming the gap.opencode-go/deepseek-v4.1-flash | 𝕏
| ON destination.provider = checked.provider | ||
| AND destination.account_email = checked.account_email | ||
| AND destination.calendar_id = checked.calendar_id |
There was a problem hiding this comment.
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`.| 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 |
There was a problem hiding this comment.
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`).
Two accounts can use the same destination calendar. If both book before the first external calendar write finishes, the existing host-only database check lets both through.
I extended the transaction-level conflict check to include bookings from accounts whose destination matches the requesting host's selected conflict calendar. Creation, rescheduling, and host reassignment use this same check.
The match uses provider, account email, and calendar ID. This does not yet identify a shared calendar connected through different account emails, and it cannot make an external calendar write atomic with SQLite.
Validation:
go test ./...andmake buildpass. The focused test rejects an overlap across two accounts and allows a different calendar.