oficina / calendar Public

booking: the third review's four

GPT-6's third pass left three blockers and one hardening. All four
closed, with a regression for each behaviour.

"Nothing claimable" was read as "everything sent": a hold whose mail a
tick had already claimed told the booker it was sent while the tick
was still holding the lease. The immediate attempt now reports true
only when the booking owes nothing, whoever holds the rows.

A closed page's reschedule picker listed times the store would refuse
one by one, and the refusal redirected back to the same list with no
explanation. The picker says the page is closed and that the booking
stands; a refused move is explained on the picker it returns to.

The page-view drain recorded a failed or completed reminder on the
same context whose expiry had just failed the send; bookkeeping runs
on its own reserve there too. The reserve is now part of every budget:
one deadline covers the sweeps, sends stop two seconds before each
phase ends, and the last send's outcome is recorded inside those two
seconds, so a tick is forty-five seconds and a page view five, not
five plus five.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 1 month ago b84093836ac5d98ed64528b8e8c356363457ea45 parent 5e3f047

7 files changed, +141 −18

internal/calendar/app.go +4 −0
internal/calendar/booking_mail.go +12 −6
internal/calendar/handlers_booking_manage.go +11 −0
internal/calendar/handlers_jobs.go +33 −12
internal/calendar/store_booking_mail.go +12 −0
internal/calendar/testing.go +8 −0
internal/calendartest/booking_review2_test.go +61 −0

diff --git a/internal/calendar/app.go b/internal/calendar/app.go

index 1ac3056..44ae77c 100644

--- a/internal/calendar/app.go

+++ b/internal/calendar/app.go

@@ -36,6 +36,9 @@ type App struct {

// auth is held so the harness can mint a session the same way a

// real sign-in does, rather than by writing a session row by hand.

auth *rauth.Auth

+ // h is held for the test harness; requests reach it through mux.

+ h *handlers

}

// New resolves every fail-closed setting, applies the schema, and wires

@@ -129,6 +132,7 @@ func New(cfg Config) (*App, error) {

auth: authPlugin,

}

h := &handlers{store: store, logger: logger, origin: origin, mailer: mailer, gate: newGate()}

+ a.h = h

a.mux = a.routes(h, authPlugin, list, origin)

return a, nil

}

diff --git a/internal/calendar/booking_mail.go b/internal/calendar/booking_mail.go

index 7bc5691..8b48f9f 100644

--- a/internal/calendar/booking_mail.go

+++ b/internal/calendar/booking_mail.go

@@ -152,15 +152,21 @@ func obsolete(row BookingMail, v BookingView, now time.Time) bool {

func (h *handlers) trySendNow(ctx context.Context, bookingID int64, now time.Time) (allSent bool) {

ctx, cancel := context.WithTimeout(ctx, 2*maxSendWait)

defer cancel()

- allSent = true

+ sctx, scancel := sendBudget(ctx)

+ defer scancel()

for i := 0; i < 4; i++ {

row, ok, err := h.store.ClaimBookingMailFor(ctx, bookingID, now.Unix())

- if err != nil || !ok {

- return allSent && err == nil

+ if err != nil {

+ return false

}

- if err := h.sendBookingMail(ctx, row, now); err != nil {

- allSent = false

+ if !ok {

+ break

}

+ _ = h.sendBookingMail(sctx, row, now)

}

- return allSent

+ // Nothing claimable is not everything sent: a row another worker

+ // holds a lease on, or one this loop deferred, is still owed. The

+ // page says "sent" only when the booking owes nothing.

+ owes, err := h.store.OwesMail(keep(ctx), bookingID)

+ return err == nil && !owes

}

diff --git a/internal/calendar/handlers_booking_manage.go b/internal/calendar/handlers_booking_manage.go

index 5f16718..af1b9c9 100644

--- a/internal/calendar/handlers_booking_manage.go

+++ b/internal/calendar/handlers_booking_manage.go

@@ -132,6 +132,16 @@ func (h *handlers) bookReschedulePicker(w http.ResponseWriter, r *http.Request)

h.renderState(w, r, v, token)

return

}

+ // A page the host has closed offers no times to move to. The

+ // booking itself stands; the store would refuse every slot anyway,

+ // so say so here rather than after a choice.

+ if !v.Page.Published {

+ render(w, "book_gone", h.page(r, v.Title, map[string]any{

+ "Title": v.Title, "Zone": v.BookerZone,

+ "Why": "This page is no longer taking bookings, so this one cannot be moved. It still stands as it is; you can cancel it from the link in your mail.",

+ }))

+ return

+ }

zone, chosen := pickZone(r, v.Host)

if !chosen {

zone = v.BookerZone

@@ -143,6 +153,7 @@ func (h *handlers) bookReschedulePicker(w http.ResponseWriter, r *http.Request)

}

// The booking's own snapshot, not the page's live text (B20).

d.Title, d.Description = v.Title, v.Description

+ d.Gone = r.URL.Query().Get("gone") == "1"

d.RescheduleToken, d.Rev, d.ZoneChosen = token, v.Event.Sequence, true

d.Path = "/book/m/" + token + "/reschedule"

render(w, "book_page", h.page(r, "Move "+v.Title, d))

diff --git a/internal/calendar/handlers_jobs.go b/internal/calendar/handlers_jobs.go

index e1a0e54..bb67479 100644

--- a/internal/calendar/handlers_jobs.go

+++ b/internal/calendar/handlers_jobs.go

@@ -51,6 +51,8 @@ const drainDeadline = 45 * time.Second

// behind a slow relay cannot starve them.

func (h *handlers) drain(ctx context.Context, now time.Time) (int, error) {

deadline := now.Add(drainDeadline)

+ ctx, cancel := context.WithDeadline(ctx, deadline)

+ defer cancel()

if n, err := h.store.ExpireHolds(ctx, now, 200); err != nil {

return 0, err

} else if n > 0 {

@@ -63,18 +65,21 @@ func (h *handlers) drain(ctx context.Context, now time.Time) (int, error) {

}

// Each phase's deadline is a CONTEXT deadline, not only a loop

// condition: the bounded sender waits no longer than what is left,

- // so a send started at second 44 cannot run to second 64.

- rctx, rcancel := context.WithDeadline(ctx, now.Add(drainDeadline/2))

- sent, err := h.drainReminderQueue(rctx, now, now.Add(drainDeadline/2))

+ // so a send started at second 44 cannot run to second 64. Sends

+ // stop keepReserve before each phase's end, which is where the

+ // bookkeeping of the last send runs.

+ half := now.Add(drainDeadline / 2).Add(-keepReserve)

+ rctx, rcancel := context.WithDeadline(ctx, half)

+ sent, err := h.drainReminderQueue(rctx, now, half)

rcancel()

if err != nil && !errors.Is(err, context.DeadlineExceeded) {

return sent, err

}

// The reminder phase running out is a handover, not a failure:

// booking mail still gets the second half.

- mctx, mcancel := context.WithDeadline(ctx, deadline)

+ mctx, mcancel := context.WithDeadline(ctx, deadline.Add(-keepReserve))

defer mcancel()

- mailed, err := h.drainBookingMail(mctx, now, deadline)

+ mailed, err := h.drainBookingMail(mctx, now, deadline.Add(-keepReserve))

return sent + mailed, err

}

@@ -203,6 +208,8 @@ func (h *handlers) drainOpportunistically(next http.Handler) http.Handler {

func (h *handlers) DrainOnRequest(ctx context.Context) {

ctx, cancel := context.WithTimeout(ctx, 5*time.Second)

defer cancel()

+ sctx, scancel := sendBudget(ctx)

+ defer scancel()

now := time.Now()

if n, err := h.store.ExpireHolds(ctx, now, 1); err != nil || n > 0 {

return

@@ -213,21 +220,21 @@ func (h *handlers) DrainOnRequest(ctx context.Context) {

}

if !ok {

if row, ok, err := h.store.ClaimBookingMail(ctx, now.Unix()); err == nil && ok {

- _ = h.sendBookingMail(ctx, row, now)

+ _ = h.sendBookingMail(sctx, row, now)

}

return

}

- if err := h.sendReminder(ctx, due, now); err != nil {

+ if err := h.sendReminder(sctx, due, now); err != nil {

if errors.Is(err, ErrMailBusy) {

_, _ = h.store.DeferReminder(keep(ctx), due.Queue.ID, due.ClaimID, now.Unix())

return

}

h.logger.Error("calendar: opportunistic reminder", "stage", "reminders.send",

"why", mailCode(err))

- _ = h.store.FailReminder(ctx, due.Queue.ID, due.ClaimID, due.StartUTC)

+ _ = h.store.FailReminder(keep(ctx), due.Queue.ID, due.ClaimID, due.StartUTC)

return

}

- if err := h.store.CompleteReminder(ctx, due, now.Unix()); err != nil {

+ if err := h.store.CompleteReminder(keep(ctx), due, now.Unix()); err != nil {

h.logger.Error("calendar: mark reminder sent", "stage", "reminders.mark", "err", scrubErr(err))

}

@@ -252,13 +259,27 @@ func (h *handlers) drainBookingMail(ctx context.Context, now, deadline time.Time

return sent, nil

}

+// keepReserve is the part of every budget that belongs to bookkeeping,

+// not sending: sends stop this long before the deadline, and the

+// outcome of the last one is recorded inside it.

+const keepReserve = 2 * time.Second

+// sendBudget is what a send may use of ctx: everything but the

+// reserve. A context with no deadline is left alone.

+func sendBudget(ctx context.Context) (context.Context, context.CancelFunc) {

+ if dl, ok := ctx.Deadline(); ok {

+ return context.WithDeadline(ctx, dl.Add(-keepReserve))

+ }

+ return context.WithCancel(ctx)

// keep is the bookkeeping context: what a send's outcome is recorded

-// under. Detached from the phase deadline — a send that ran out its

+// under. Detached from the send's deadline — a send that ran out its

// budget still has to be marked failed or deferred, or the lease does

// the same job five minutes later at the cost of a duplicate — and

-// bounded on its own so bookkeeping cannot hang either.

+// bounded to the reserve so bookkeeping cannot hang either.

func keep(ctx context.Context) context.Context {

- k, cancel := context.WithTimeout(context.WithoutCancel(ctx), 5*time.Second)

+ k, cancel := context.WithTimeout(context.WithoutCancel(ctx), keepReserve)

_ = cancel // released by the timeout; the store calls are short

return k

}

diff --git a/internal/calendar/store_booking_mail.go b/internal/calendar/store_booking_mail.go

index 5758513..3e69401 100644

--- a/internal/calendar/store_booking_mail.go

+++ b/internal/calendar/store_booking_mail.go

@@ -163,6 +163,18 @@ func (s *Store) NextBookingMailDue(ctx context.Context) time.Time {

return time.Unix(*at, 0)

}

+// OwesMail says whether any of a booking's mail is still unsent —

+// claimed, deferred or not yet tried.

+func (s *Store) OwesMail(ctx context.Context, bookingID int64) (bool, error) {

+ var n int64

+ err := s.g.WithContext(ctx).Model(&BookingMail{}).

+ Where("booking_id = ? AND sent_at IS NULL", bookingID).Count(&n).Error

+ if err != nil {

+ return true, fmt.Errorf("calendar: owed mail: %w", err)

+ }

+ return n > 0, nil

// UnsentBookingMailForTest lists what a booking still owes.

func (s *Store) UnsentBookingMailForTest(bookingID int64) []BookingMail {

var rows []BookingMail

diff --git a/internal/calendar/testing.go b/internal/calendar/testing.go

index 2f5521d..02f48ef 100644

--- a/internal/calendar/testing.go

+++ b/internal/calendar/testing.go

@@ -13,6 +13,7 @@ import (

"os"

"path/filepath"

"strings"

+ "time"

)

// TB is the slice of *testing.T this file needs.

@@ -205,3 +206,10 @@ func ZoneCacheSizeForTest() int {

// commit an event over the slot and prove the in-transaction re-check

// behind the write lock catches it. nil removes it.

func (a *App) SetAfterSlotCheckForTest(f func()) { a.store.afterSlotCheckForTest = f }

+// TrySendNowForTest is the immediate attempt a transition makes for one

+// booking's owed mail, and what it reports: true only when nothing is

+// owed afterwards.

+func (a *App) TrySendNowForTest(bookingID int64) bool {

+ return a.h.trySendNow(context.Background(), bookingID, time.Now())

diff --git a/internal/calendartest/booking_review2_test.go b/internal/calendartest/booking_review2_test.go

index 567041b..dd5d957 100644

--- a/internal/calendartest/booking_review2_test.go

+++ b/internal/calendartest/booking_review2_test.go

@@ -200,3 +200,64 @@ func TestADeferredHoldMailIsNotReportedSent(t *testing.T) {

t.Errorf("owed rows with no attempt = %d, want the deferred one", n)

}

+// "Nothing claimable" is not "everything sent": a row another worker

+// holds a lease on is still owed, and the page must not say "sent"

+// about it (review 3, C1).

+func TestAForeignClaimIsNotReportedSent(t *testing.T) {

+ app, host, page := hostWithPage(t)

+ st := app.StoreForTest()

+ ctx := context.Background()

+ now := time.Now()

+ slot := slotsFor(t, app, page, host, now, 3)[0]

+ b, err := st.HoldBooking(ctx, holdInput(page, host, slot, "doc@tito.io", "ip"), now)

+ if err != nil {

+ t.Fatal(err)

+ }

+ // Another worker — a tick — claims the hold mail first.

+ row, ok, err := st.ClaimBookingMailFor(ctx, b.ID, now.Unix())

+ if err != nil || !ok {

+ t.Fatalf("claim: ok=%v err=%v", ok, err)

+ }

+ if app.TrySendNowForTest(b.ID) {

+ t.Error("with the row leased elsewhere, the attempt reported everything sent")

+ }

+ if ok, _ := st.MarkBookingMailSent(ctx, row.ID, row.ClaimID, now.Unix()); !ok {

+ t.Fatal("mark sent")

+ }

+ if !app.TrySendNowForTest(b.ID) {

+ t.Error("with nothing owed, the attempt reported something unsent")

+ }

+// A page the host closed offers no times to move to: the reschedule

+// picker says so instead of listing slots every one of which the store

+// would refuse (review 3, C2). And a refused slot is explained on the

+// picker it returns to.

+func TestTheReschedulePickerOnAClosedPageSaysSo(t *testing.T) {

+ app, mailer, path, _ := publishedPage(t)

+ h := app.Handler()

+ session := app.SignInForTest(t, "host@example.com")

+ mailer.reset() // the sign-in link is not the mail under test

+ _, starts := firstDaySlots(t, h, path)

+ strangerPost(t, h, path, holdForm(starts[0], "Doc", "doc@tito.io"))

+ link := reManage.FindString(mailer.body(0))

+ strangerPost(t, h, link+"/confirm", nil)

+ body := strangerGet(t, h, link+"/reschedule?gone=1").Body.String()

+ if !strings.Contains(body, "That time has just gone") {

+ t.Error("a refused move returns to a picker that does not say why")

+ }

+ postAs(t, h, session, "/settings/booking/1/publish", url.Values{"publish": {"0"}})

+ rec := strangerGet(t, h, link+"/reschedule")

+ if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), "no longer taking bookings") || strings.Contains(rec.Body.String(), `name="start"`) {

+ t.Errorf("reschedule picker on a closed page: %d\n%s", rec.Code, rec.Body.String())

+ }

+ rec = strangerPost(t, h, link+"/reschedule", url.Values{"start": {starts[1]}, "zone": {"UTC"}, "rev": {"1"}})

+ if rec.Code != http.StatusSeeOther {

+ t.Errorf("a move on a closed page: %d, want a redirect back to the explanation", rec.Code)

+ }

+ if strangerGet(t, h, link).Code != http.StatusOK {

+ t.Error("the booking itself no longer opens")

+ }