Repository navigation
fix(calendar): report incomplete availability checks - #63
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.
Caution
The fail-closed change never reaches a user-facing decision. The only production caller of Service.FreeBusy — hostAvailability (internal/handler/slots_handler.go:497) — catches the new error, logs it, and returns the host with an empty busy set, so every slot stays bookable. And because calendar.Service.FreeBusy now aborts on the first error instead of unioning the providers that succeeded, a healthy account's busy intervals are discarded too: for multi-account or multi-provider users this is strictly more permissive than the fail-open code it replaces.
Reviewed changes
- Per-account failures are now fatal in every provider —
gcal,microsoft, andcaldavno longer skip a connection whose credentials/authorization fail, andgcal/caldavreturn on the first failed bucket. - Google per-calendar errors are surfaced —
freeBusyForConniterates the requestedcalIDsand errors when a calendar is absent, carries anerrorsentry, or returns an unparseablestart/end. calendar.Service.FreeBusyreturns the first provider error rather than the union of the providers that succeeded.- Docs and tests updated —
ARCHITECTURE.md§10 states the new policy; the gcal tests flip the old fail-open assertion and add a table test for incomplete responses.
ℹ️ Fail-closed has no user-facing explanation
Once the error is propagated, computeSlots fails the request and GetSlots answers 500 (slots_handler.go:76-79), so the booker sees a broken/empty calendar rather than "no times", with nothing pointing the operator at the calendar that needs reconnecting. A single misconfigured calendar also blocks every host of that event type, including round-robin events where another host is free.
Technical details
# Fail-closed blast radius and operator signal
## Affected sites
- `internal/handler/slots_handler.go:223-227` — any non-nil `hostAvailability` error fails the whole `computeSlots` call for the event type.
- `internal/handler/slots_handler.go:76-79` — the resulting error is rendered as a generic HTTP 500.
- `docs/ARCHITECTURE.md:452` — the documented recovery ("reconnect it or deselect it") is operator-only; a booker has no way to act.
## Open question for the human
- Is a hard error the intended booker experience, or should an unreadable conflict calendar degrade to "this host offers nothing" (with the failure logged/alerted) so one host's breakage does not blank the whole event type? The current PR description accepts the block, but not the 500-shaped surface it produces.
## Suggested approach (optional)
- If fail-closed is wanted, consider returning a distinct sentinel error that the slots handler can map to a "temporarily unavailable" response rather than `internal error`, and log the offending host + calendar id.ℹ️ Nitpicks
internal/gcal/freebusy.go:85— the error drops the calendaridand theerrorspayload you now decode (Errors []json.RawMessage); including both would make the fail-closed block diagnosable from a log line.
opencode-go/deepseek-v4.1-flash | 𝕏
| firstErr = err | ||
| } | ||
| continue | ||
| return nil, err |
There was a problem hiding this comment.
The only production caller of Service.FreeBusy — hostAvailability at internal/handler/slots_handler.go:497 — still treats the result as non-fatal: it logs the error and returns the host with an empty Busy slice, so an unreadable calendar leaves every slot bookable. Because this method now returns on the first error instead of unioning the providers that succeeded, it also drops the busy intervals of accounts/providers that did respond, which makes multi-account and multi-provider users more available than under the old fail-open code.
Technical details
# Fail-closed policy is not wired into the availability path
## Affected sites
- `internal/handler/slots_handler.go:497-503` — `hostAvailability` logs `gc.FreeBusy` errors and returns `slots.HostAvailability` with no calendar busy intervals (this file is not part of the PR).
- `internal/calendar/calendar.go:356` — new `return nil, err` aborts the provider union on the first error, discarding intervals from providers/accounts that succeeded.
- `docs/ARCHITECTURE.md:452` — documents the fail-closed policy this path does not yet implement.
## Required outcome
- A free/busy failure for a host must not silently yield an empty conflict set; it must either prevent that host's slots from being offered or fail the slot computation.
- Keep the union semantics consistent with the chosen policy: if a host has several accounts/providers, either fail closed on any error, or keep the successful accounts' intervals and surface the failure separately — do not return `nil` intervals together with an error to a caller that ignores it.
- Add caller-level coverage. The new gcal tests stop at the provider boundary and pass while availability remains fail-open end to end.
## Suggested approach
- Return the error from `hostAvailability` so `computeSlots` (`slots_handler.go:223-227`) fails the request; that produces the fail-closed semantics the PR describes. Consider the booker-facing surface before making the whole event type error out (see the informational note in the review body).
A selected calendar can fail while another provider returns an empty result. Calnode currently treats that partial result as available. Google can also return HTTP 200 with errors for an individual calendar.
I made these failures visible instead of treating them as free time. This covers Google, Microsoft, CalDAV, and the combined provider result. Missing or invalid Google busy periods are rejected too.
This deliberately changes the existing fail-open policy: an unavailable selected calendar can temporarily block availability. I think that is safer than accepting a booking without checking it. The operator can reconnect or deselect that calendar.
Validation:
go test ./...andmake buildpass. Tests cover Google per-calendar errors and missing calendars. The recent CalDAV discovery changes in main are preserved.