amadan
booking: a spent budget is a deferral, not an attempt
GPT-6's fourth pass returned SHIP WITH FIXES, one blocking. A page view's drain could claim a reminder at second four of five, find the send budget already spent, get a timeout from the bounded sender without any relay having been asked, and count that as an attempt — the fifth such would retire the reminder for a relay that was never tried. The bounded sender now answers a spent budget with busy, which every caller already treats as a deferral with attempts untouched. The hardening it named alongside: bookkeeping is bounded by what is left of the overall deadline, floored at one second, so a budget can overrun by that second and no more. The budget helpers move to their own file.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
4 files changed,
+106
−29
internal/calendar/handlers_jobs.go+0 −25internal/calendar/handlers_jobs_budget.go+49 −0internal/calendar/mail_bounded.go+9 −4internal/calendar/mail_bounded_test.go+48 −0
CI failed — run details
diff --git a/internal/calendar/handlers_jobs.go b/internal/calendar/handlers_jobs.go| index bb67479..503db9c 100644 |
| --- a/internal/calendar/handlers_jobs.go |
| +++ b/internal/calendar/handlers_jobs.go |
| @@ -258,28 +258,3 @@ 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 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 to the reserve so bookkeeping cannot hang either. |
| -func keep(ctx context.Context) context.Context { |
| - 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/handlers_jobs_budget.go b/internal/calendar/handlers_jobs_budget.go| new file mode 100644 |
| index 0000000..524a1ee |
| --- /dev/null |
| +++ b/internal/calendar/handlers_jobs_budget.go |
| @@ -0,0 +1,49 @@ |
| +package calendar |
| + |
| +import ( |
| + "context" |
| + "time" |
| +) |
| + |
| +// The drain's budgets: what a send may use of one, and what is kept |
| +// back for recording its outcome. |
| + |
| +// 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 cancellation — 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 |
| +// by what is left of ctx's own deadline, floored at keepFloor so a |
| +// late claim still gets its outcome recorded. That floor is the whole |
| +// of the overrun a budget can suffer. |
| +func keep(ctx context.Context) context.Context { |
| + left := keepReserve |
| + if dl, ok := ctx.Deadline(); ok { |
| + if until := time.Until(dl); until < left { |
| + left = until |
| + } |
| + } |
| + if left < keepFloor { |
| + left = keepFloor |
| + } |
| + k, cancel := context.WithTimeout(context.WithoutCancel(ctx), left) |
| + _ = cancel // released by the timeout; the store calls are short |
| + return k |
| +} |
| + |
| +// keepFloor is the least a bookkeeping call is given, whatever the |
| +// clock says. |
| +const keepFloor = time.Second |
diff --git a/internal/calendar/mail_bounded.go b/internal/calendar/mail_bounded.go| index 1f5f413..3e4bced 100644 |
| --- a/internal/calendar/mail_bounded.go |
| +++ b/internal/calendar/mail_bounded.go |
| @@ -9,9 +9,10 @@ import ( |
| ) |
| // ErrMailBusy is what a send returns when every in-flight slot of the |
| -// bounded sender is taken. No SMTP call was made, so it is a DEFERRAL |
| -// and never an attempt: a row that hits it goes back to the queue with |
| -// its attempts untouched (booking spec B22). |
| +// bounded sender is taken, or when the caller's budget was already |
| +// spent before the sender was called. No SMTP call was made, so it is |
| +// a DEFERRAL and never an attempt: a row that hits it goes back to the |
| +// queue with its attempts untouched (booking spec B22). |
| var ErrMailBusy = errors.New("calendar: mail sender busy") |
| // ErrMailTimeout is what a send returns when the underlying sender did |
| @@ -67,8 +68,12 @@ func (b *boundedSender) Send(ctx context.Context, to, subject, body string) erro |
| } |
| } |
| if wait <= 0 { |
| + // The budget was gone before any sender was called. Nothing |
| + // was attempted, so this is a DEFERRAL like busy, never a |
| + // counted failure: a healthy relay must not lose an attempt |
| + // to a slow claim in front of it. |
| <-b.slots |
| - return ErrMailTimeout |
| + return ErrMailBusy |
| } |
| done := make(chan error, 1) |
diff --git a/internal/calendar/mail_bounded_test.go b/internal/calendar/mail_bounded_test.go| new file mode 100644 |
| index 0000000..9b63057 |
| --- /dev/null |
| +++ b/internal/calendar/mail_bounded_test.go |
| @@ -0,0 +1,48 @@ |
| +package calendar |
| + |
| +import ( |
| + "context" |
| + "errors" |
| + "testing" |
| + "time" |
| +) |
| + |
| +type countingSender struct{ calls int } |
| + |
| +func (c *countingSender) Send(context.Context, string, string, string) error { |
| + c.calls++ |
| + return nil |
| +} |
| + |
| +type refusingSender struct{} |
| + |
| +func (refusingSender) Send(context.Context, string, string, string) error { |
| + return errors.New("send to a@example.com: 550 no such user") |
| +} |
| + |
| +// A budget already spent before the sender is called is a DEFERRAL, |
| +// never a counted attempt: no relay was asked (code review 4, B.1). |
| +func TestASpentBudgetDefersRatherThanFails(t *testing.T) { |
| + next := &countingSender{} |
| + b := newBoundedSender(next) |
| + ctx, cancel := context.WithDeadline(context.Background(), time.Now().Add(-time.Second)) |
| + defer cancel() |
| + err := b.Send(ctx, "someone@example.com", "s", "b") |
| + if !errors.Is(err, ErrMailBusy) { |
| + t.Errorf("spent budget: %v, want ErrMailBusy", err) |
| + } |
| + if next.calls != 0 { |
| + t.Error("the sender was called with no budget") |
| + } |
| + if len(b.slots) != 0 { |
| + t.Error("the slot was not released") |
| + } |
| +} |
| + |
| +// The sender's own error text stops at the wrapper (code review 2, B1). |
| +func TestTheSendersErrorTextStopsAtTheWrapper(t *testing.T) { |
| + err := newBoundedSender(refusingSender{}).Send(context.Background(), "a@example.com", "s", "b") |
| + if !errors.Is(err, ErrMailRefused) || err.Error() != ErrMailRefused.Error() { |
| + t.Errorf("a refused send: %v, want the content-free ErrMailRefused", err) |
| + } |
| +} |