P2 [CRF-1] The budget notification is edge-triggered but delivered best-effort, so a single transient failure on the crossing interception drops it for the entire period. (Nami P2, Meruem P2, Mafuuu P2, Chopper P3, Pariston P3, Bisky P2)

Detection fires only on the one interception where oldSpend < at && newSpend >= at (notifications.go:84); every later interception has oldSpend >= at, so the edge never re-arms. This post-commit loop logs and swallows any error from GetGroupByID or EnqueueWithData, and the detect read inside the transaction is likewise swallowed (line 424-431). Meruem: "correctness (the user gets notified) depends on the enqueue succeeding on one specific interception, which nothing enforces."

Consequence: if that one send fails transiently, or the request-scoped ctx is cancelled once the gRPC call returns, the user hits their limit, gets blocked by IsBudgetExceeded, and the one message explaining why never arrives, with no retry. Bisky notes no test covers either failure branch (detect 82.4%, notify 75.0% coverage), so a regression here is invisible to CI.

Make detection level-triggered from observed state (detect newSpend >= at every interception and let the per-day dedupe_hash drop repeats, or record which thresholds fired this period) so a missed enqueue self-heals on the next interception. Alternatively, enqueue inside the transaction so it commits atomically with the spend. This needs a decision, not a silent best-effort acceptance.

🤖

I think a best-effort approach is acceptable here. Users can always check their AI spend and budget limits in the UI.

1. I don't like the idea of attempting to send a notification on every LLM request. It abuses the notification deduplication mechanism and would still result in notifications being sent every day.
2. I also don't want to introduce additional state (warn85, warn100) because it adds unnecessary complexity. We'd need to add two database fields, manage their lifecycle, mark them as sent after a notification is delivered, and then reset them whenever the effective budget changes. Resetting them is particularly tricky because a user's budget can change implicitly when they're added to or removed from a group.
3. I also think this is the convention throughout the Coder repository: notifications are treated as best-effort.

@ssncferreira wdyt, do you agree with current best-effort implementation?

P2 [CRF-2] oldSpend := newSpend - cost.costMicros is only correct if this interception is the sole writer inside the summed window, which breaks under cross-day concurrency and at period boundaries. (Knov P2, Mafuuu P2, Knuckle P3, Pariston P3, Hisoka P3, Komugi P3, Melody P3)

Two confirmed mechanisms:

1. Cross-day concurrency. InTx(..., nil) runs at READ COMMITTED and IncrementUserAIDailySpend only row-locks the single (user, group, day) row (both verified). GetUserAISpendSince sums across all days in the period. Knov: with period total 80, limit 100, two concurrent interceptions (+3 on day N, +3 on day N+1) each read 83, neither observes >= 85, and after both commit the true total is 86, past the threshold, with no notification ever sent. Same-day interceptions serialize on the shared row and are fine; the gap is cross-day within one period.

2. Clock-source mismatch. The spend row is bucketed by dbtime.StartOfDay(createdAt.UTC()) (aibridgedserver.go:415), but the period comes from budget.CurrentPeriod(s.clock.Now(), ...) (line 62). At a UTC month rollover (created 23:59 last day, processed 00:00 next day), the increment lands outside the new period's window, so newSpend excludes this cost while oldSpend = newSpend - cost goes negative, producing a spurious "limit reached / requests blocked" notification the user did not earn (Hisoka, Komugi, Melody), or the mirror missed-crossing case.

Knuckle's fix addresses both: take a pg_advisory_xact_lock keyed on the user/period at the top of the transaction and compute oldSpend as the period SUM read before the increment, and derive the period from the same time source the spend is bucketed by. Unverified only in that no reviewer reproduced the race; the mechanism is confirmed from the code.

🤖

1. This could be fixed by acquiring a lock in the recordTokenUsageAndSpend transaction:

if err := tx.AcquireLock(ctx, database.GenLockID(fmt.Sprintf(
	"ai-budget-spend:%s:%s",
	intc.InitiatorID,
	cost.effectiveGroupID.UUID,
))); err != nil {
	return xerrors.Errorf("acquire AI budget spend lock: %w", err)
}

That said, I think this race condition is very rare, so accepting the best-effort behavior without introducing the lock may be reasonable. @ssncferreira what do you think?

2. Great catch, fixed here: 6aa58d1 and 98ed750

P2 [CRF-3] The notification path runs under deployment-wide AsSystemRestricted to do two narrow things (read one group, enqueue one notification), when a purpose-scoped actor already exists. (Kurapika P2)

subjectSystemRestricted grants ResourceWildcard:{read} plus create/update/delete on users, API keys, workspaces, crypto keys, notification templates, and more. I confirmed subjectNotifier (AsNotifier), used by every other enqueue call site, holds only NotificationMessage, InboxNotification, WebpushSubscription, and DeploymentConfig, and cannot read groups, which is the only reason this path reaches for the wildcard actor. The //nolint comment asserts "enqueuing need[s] system access," but every other enqueuer disproves that.

recordTokenUsageAndSpend processes token-usage payloads fed from the aibridge daemon; any logic error on this path executes with read on every resource and write on users, keys, and workspaces. Kurapika's fix: add ResourceGroup:{read} to subjectNotifier and switch to AsNotifier, or better, carry group.Name out of budget.ResolveUserAIBudget (it already resolves the effective budget) so the path needs no group read at all and stays on AsNotifier unchanged.

🤖

Prebuilds also uses AsSystemRestricted, but fixed here anyway: aadb31f

P3 [CRF-4] Both templates hardcode "monthly" while the budget period is a configurable enum built to grow, with no mechanical guard tying the copy to the enum. (Meruem P3, and Note-level convergence from Netero, Hisoka, Mafuuu, Pariston, Chopper, Leorio, Luffy, Knov)

s.budgetPeriod is a codersdk.AIBudgetPeriod and detectBudgetThresholdCrossings already reads it to size the window. Today AIBudgetPeriods holds only "month", so the wording is accurate now, but notifyBudgetThresholdCrossing never passes the period as a label, so the template could not render it even if it wanted to. The day someone adds "week" or "day", spend resets on the new cadence and all four strings silently lie, with no compiler, runtime, or test signal (the golden files only assert the month wording). Meruem's mechanical fix: add the period as a label and template it (your {{.Labels.period}} AI budget), or pin len(codersdk.AIBudgetPeriods) == 1 in a test so growing the enum forces this file to be revisited.

🤖

Fixed here: 6173158 to avoid making additional DB migrations in the future

P3 [CRF-5] "Effective group: {{.Labels.group_name}}" leaks internal resolver jargon and is actively misleading when the limit came from a per-user override. (Leorio P3, Luffy P3, Mafuuu Note)

I confirmed ResolveUserAIBudget returns early for a per-user override with Source: AIBudgetLimitSourceUserOverride but still carries override.GroupID, so this line renders "Effective group: Engineering" for a limit that did not come from the group at all, sending the user to the wrong place ("ask my group admin") when the override is what governs them. Luffy: "What's an effective group? That's a word from the budget resolver, not a word the person spending the money thinks in." Drop the phrase or state what it means to the reader; in the override case, either name the real source or omit the group line rather than assert a false cause.

🤖

I updated the template - the new version looks better: 6fd2bb7

Note [CRF-14] budgetNotificationsCreatedBy = "aigateway" is a third spelling for this subsystem. (Gon Note)

The package is aibridgedserver, the path is AI Bridge, and the persisted createdBy label is "aigateway". Sibling enqueuers use a stable identifier tied to the emitting subsystem. Since this value is stored with every notification, a third spelling makes provenance filtering harder later. Worth aligning on one name before it ships.

🤖

Note [CRF-15] A nil Enqueuer silently degrades to a no-op, so a production miswiring disables every AI budget notification with no error. (Meruem Note)

NewServer substitutes notifications.NewNoopEnqueuer() when opts.Enqueuer == nil. Both real call sites wire it today, but a future call site that forgets would start clean and drop all budget notifications indefinitely, indistinguishable from "no thresholds crossed." The rest of this package injects explicit no-ops at the call site (e.g. agplaiseats.Noop{}) rather than relying on a nil default. Consider requiring a non-nil enqueuer, matching the AISeatTracker pattern.

🤖

Note [CRF-16] The 85% warning and 100% limit thresholds are hardcoded with no configurability. (Meruem Note)

warningThresholdPercent = 85 bakes a product decision into a constant. Fine if deliberately fixed, but worth confirming it is intended to be non-tunable per deployment, since the budget limit itself is configurable per group.

🤖

Note [CRF-17] A single large interception can cross both thresholds at once, so the user gets "you're approaching your limit" and "you've reached your limit" in the same instant. (Nami Note)

budgetThresholds is evaluated ascending and every crossed threshold is appended (documented as intentional). For a small budget or an expensive request, the two notifications have contradictory wording ("approaching" next to "reached"). Consider suppressing the warning when the limit-reached notification also fires on the same interception.

🤖

Note [CRF-18] group_name is interpolated into a markdown-bold span, so a group name with markdown characters renders as garbled emphasis. (Nami Note, Kurapika Note)

Group names are user-controlled and land in **{{.Labels.group_name}}**. A group named *prod* or a_b_c renders with stray or missing emphasis. Kurapika confirmed the markdown pipeline strips raw HTML (html.SkipHTML), so this is cosmetic, not injection, but it is still user-visible text.

🤖