Skip to content

Commit 6ced6b6

Browse files
jaaydenhjaaydenh
committed
fix: address round-7 review findings on runtime hours entitlements
measureUsage now owns the whole usage-failure policy: the cause is logged there instead of in each Entitlements closure, so the abort rule and the log-suppression rule can no longer drift apart (CRF-119), and the closures are reduced to plain queries, removing the duplicated logging rationale (CRF-116). Comment ownership is consolidated: the classifier story lives on the codersdk warning-text constants, the tolerate-and-warn rationale on decodeAgentRuntimeHours, and the unit rationale on agentRuntimeMsToHours, with pointers elsewhere (CRF-116, CRF-121). The claims-ignored warning dedup guard is pinned by a two-license test asserting one warning and per-license log detail (CRF-117), the AI Governance near-limit branch of isMutedWarning is pinned by its own story (CRF-118, verified by branch-local mutation), the four usage-failure subtests share a fixture helper (CRF-123), the migration comment now assigns SUM protection to uniqueness and attribution protection to the alignment CHECK (CRF-120), the test-local cutPrefix is renamed templatePrefix to stop colliding with strings.CutPrefix (CRF-122), and formatLicenseMessage's doc states its %d-only substitution rule (CRF-124).
1 parent 907c91a commit 6ced6b6

8 files changed

Lines changed: 187 additions & 217 deletions

File tree

‎coderd/database/migrations/000565_usage_events_agent_runtime_invariants.up.sql‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -1,12 +1,12 @@
11
-- The usage generator writes hb_agent_runtime_v1 rows with two invariants
22
-- nothing enforced: created_at is always the UTC hourly bucket start, and
3-
-- there is exactly one row per bucket. The entitlements read path
4-
-- (GetTotalUsageHBAgentRuntimeV1) depends on them differently: uniqueness is
5-
-- what keeps SUM from counting a bucket twice, while hour alignment protects
6-
-- the attribution model, which charges a bucket to the usage period
7-
-- containing its start. That rule is only meaningful if bucket starts are
8-
-- where the generator says they are; the SUM itself would add a misaligned
9-
-- timestamp just fine.
3+
-- there is exactly one row per bucket. Only uniqueness matters to the
4+
-- entitlements read path (GetTotalUsageHBAgentRuntimeV1): it is what keeps
5+
-- SUM from counting a bucket twice, and the SUM would add a misaligned
6+
-- timestamp just fine. The alignment CHECK instead protects the attribution
7+
-- model, which charges a bucket to the usage period containing its start: a
8+
-- rule that is only meaningful while bucket starts are where the generator
9+
-- says they are.
1010
--
1111
-- Both statements below validate existing rows, so a pre-existing violator
1212
-- aborts this migration and the upgrade. That is deliberate: every supported

‎codersdk/licenses.go‎

Lines changed: 15 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,14 @@ const (
2323
// in the license term, the allocation, then the soft limit.
2424
//
2525
// The two runtime hours texts must differ before their first placeholder:
26-
// the dashboard's LicenseBanner classifies warnings by the literal prefix
27-
// preceding the first placeholder, rendering the advisory soft-limit text
28-
// in the muted variant and the allocation-reached text prominently.
29-
// Neither text may begin with the prefix of
30-
// LicenseAIGovernance90PercentWarningText or
31-
// LicenseAIGovernanceOverLimitWarningText for the same reason. See
32-
// TestLicenseAgentRuntimeHoursWarningTexts.
26+
// the dashboard's LicenseBanner (isMutedWarning) renders warnings
27+
// matching this text's pre-placeholder prefix in the muted variant and
28+
// without a sales link, so the allocation-reached text must not match
29+
// it. Neither runtime text may begin with the prefix of
30+
// LicenseAIGovernance90PercentWarningText (also muted) or
31+
// LicenseAIGovernanceOverLimitWarningText (isAIGovernanceWarning matches
32+
// either AI Governance prefix to suppress the banner's client-side
33+
// over-limit fallback). See TestLicenseAgentRuntimeHoursWarningTexts.
3334
LicenseAgentRuntimeHoursSoftLimitWarningText = "Your deployment is approaching its Coder Agent runtime hours allocation: %d of the %d hours included in the current license term are used, at or above the advisory soft limit of %d hours."
3435
// LicenseAgentRuntimeHoursAllocationReachedWarningText is emitted once
3536
// the deployment reaches its runtime hour allocation. Both placeholders
@@ -38,7 +39,7 @@ const (
3839
LicenseAgentRuntimeHoursAllocationReachedWarningText = "Your deployment has used %d of the %d Coder Agent runtime hours included in the current license term."
3940
// LicenseManagedAgentUsageUnavailableErrorText is emitted when the
4041
// managed agent usage query fails while computing entitlements. The
41-
// cause is logged by the query closure; this stable text is served on
42+
// cause is logged server-side; this stable text is served on
4243
// the unauthenticated entitlements payload instead of the raw error. It
4344
// travels in the entitlements Errors channel, hence the ErrorText name,
4445
// so the alertable coderd_license_errors gauge keeps counting
@@ -51,15 +52,12 @@ const (
5152
// runtime hours sibling of LicenseManagedAgentUsageUnavailableErrorText.
5253
LicenseAgentRuntimeUsageUnavailableErrorText = "Unable to determine Coder Agent runtime usage. Reported runtime hours are unavailable until the next successful refresh; workspaces are unaffected. Check the coderd logs for details."
5354
// LicenseAgentRuntimeHoursClaimsIgnoredWarningText is emitted when a
54-
// license carries Coder Agent runtime hour claims that cannot be used,
55-
// including claims that do not fit together (for example a soft limit
56-
// at or above the allocation, or a threshold without an allocation) and
57-
// the feature name itself minted as a claim. The unusable claims are
58-
// ignored rather than invalidating the signed license, so without this
59-
// warning an incorrectly issued license would be undetectable from the
60-
// deployment. The coderd logs name the license and the dropped claims.
61-
// The dashboard recognizes the exact text and renders it as a muted
62-
// diagnostic without a sales link.
55+
// license carries Coder Agent runtime hour claims that cannot be used;
56+
// decodeAgentRuntimeHours in enterprise/coderd/license owns which claims
57+
// are dropped and why dropping beats invalidating the license. The
58+
// coderd logs name the license and the dropped claims. The dashboard
59+
// recognizes the exact text and renders it as a muted diagnostic without
60+
// a sales link.
6361
LicenseAgentRuntimeHoursClaimsIgnoredWarningText = "A license contains unusable Coder Agent runtime hour claims, which were ignored. The rest of that license is unaffected. Check the coderd logs for the affected license and claims, and contact support to have the license re-issued."
6462
)
6563

‎codersdk/licenses_test.go‎

Lines changed: 13 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -9,44 +9,42 @@ import (
99
"github.com/coder/coder/v2/codersdk"
1010
)
1111

12-
// TestLicenseAgentRuntimeHoursWarningTexts pins the string couplings the
13-
// dashboard's LicenseBanner (site/src/modules/dashboard/LicenseBanner)
14-
// depends on. The banner classifies warnings by the literal prefix before a
15-
// template's first placeholder: a warning starting with the AI Governance
16-
// near-limit prefix or the runtime hours soft-limit prefix renders in the
17-
// muted variant, and the AI Governance near-limit prefix additionally
18-
// suppresses the client-side over-limit fallback. A shared prefix would make
19-
// the classifier conflate two different warnings.
12+
// TestLicenseAgentRuntimeHoursWarningTexts pins the warning-text prefix
13+
// couplings consumed by the dashboard's LicenseBanner
14+
// (site/src/modules/dashboard/LicenseBanner); the
15+
// LicenseAgentRuntimeHoursSoftLimitWarningText doc owns the classifier story.
2016
func TestLicenseAgentRuntimeHoursWarningTexts(t *testing.T) {
2117
t.Parallel()
2218

2319
// Cut rather than Split so a template losing its placeholder fails the
2420
// test instead of silently turning the whole message into the "prefix".
25-
cutPrefix := func(text, placeholder string) string {
21+
templatePrefix := func(text, placeholder string) string {
2622
t.Helper()
2723
prefix, _, ok := strings.Cut(text, placeholder)
2824
require.True(t, ok, "template %q must contain placeholder %q", text, placeholder)
2925
return prefix
3026
}
3127

32-
aiGovNearLimitPrefix := cutPrefix(codersdk.LicenseAIGovernance90PercentWarningText, "%d%%")
33-
aiGovOverLimitPrefix := cutPrefix(codersdk.LicenseAIGovernanceOverLimitWarningText, "%d")
34-
softLimitPrefix := cutPrefix(codersdk.LicenseAgentRuntimeHoursSoftLimitWarningText, "%d")
28+
aiGovNearLimitPrefix := templatePrefix(codersdk.LicenseAIGovernance90PercentWarningText, "%d%%")
29+
aiGovOverLimitPrefix := templatePrefix(codersdk.LicenseAIGovernanceOverLimitWarningText, "%d")
30+
softLimitPrefix := templatePrefix(codersdk.LicenseAgentRuntimeHoursSoftLimitWarningText, "%d")
3531

3632
runtimeTexts := map[string]string{
3733
"SoftLimit": codersdk.LicenseAgentRuntimeHoursSoftLimitWarningText,
3834
"AllocationReached": codersdk.LicenseAgentRuntimeHoursAllocationReachedWarningText,
3935
}
4036
for name, text := range runtimeTexts {
37+
// isMutedWarning renders near-limit matches muted, and
38+
// isAIGovernanceWarning matches either AI Governance prefix to
39+
// suppress the banner's client-side over-limit fallback.
4140
require.False(t, strings.HasPrefix(text, aiGovNearLimitPrefix),
4241
"%s warning must not share the AI Governance near-limit prefix %q", name, aiGovNearLimitPrefix)
4342
require.False(t, strings.HasPrefix(text, aiGovOverLimitPrefix),
4443
"%s warning must not share the AI Governance over-limit prefix %q", name, aiGovOverLimitPrefix)
4544
}
4645

47-
// The banner classifies a warning starting with the soft-limit prefix
48-
// as advisory: muted variant, no sales link. The allocation-reached
49-
// warning must not match it, or it would render as an advisory.
46+
// isMutedWarning renders soft-limit matches muted and messageLink drops
47+
// their sales link, so the allocation-reached warning must not match.
5048
allocationReachedText := codersdk.LicenseAgentRuntimeHoursAllocationReachedWarningText
5149
require.False(t, strings.HasPrefix(allocationReachedText, softLimitPrefix),
5250
"the soft-limit prefix must not classify the allocation-reached warning")

‎enterprise/coderd/license/license.go‎

Lines changed: 21 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -116,36 +116,19 @@ func Entitlements(
116116
// usage.
117117
//
118118
// nolint:gocritic // Reading usage events requires the usage publisher subject.
119-
count, err := db.GetTotalUsageDCManagedAgentsV1(dbauthz.AsUsagePublisher(ctx), database.GetTotalUsageDCManagedAgentsV1Params{
119+
return db.GetTotalUsageDCManagedAgentsV1(dbauthz.AsUsagePublisher(ctx), database.GetTotalUsageDCManagedAgentsV1Params{
120120
StartDate: startTime,
121121
EndDate: endTime,
122122
})
123-
// measureUsage publishes a stable text, so the cause is only
124-
// logged here.
125-
if err != nil && !usageMeasurementAborted(ctx, err) {
126-
logger.Error(ctx, "get managed agent usage for entitlements", slog.Error(err))
127-
}
128-
return count, err
129123
},
130124
AgentRuntimeMsFn: func(ctx context.Context, startTime time.Time, endTime time.Time) (int64, error) {
131-
// Unlike the managed agent count above, this reads the raw
132-
// usage_events rows with the usage period bounds, so no rollup
133-
// is needed: hb_agent_runtime_v1 is one row per hour
134-
// deployment-wide and has a dedicated partial index. The result
135-
// is still bucket-granular rather than exact; see the query doc
136-
// for what the bounds do and do not promise.
125+
// Bounds and bucket semantics are documented on the query.
137126
//
138127
// nolint:gocritic // Reading usage events requires the usage publisher subject.
139-
runtimeMs, err := db.GetTotalUsageHBAgentRuntimeV1(dbauthz.AsUsagePublisher(ctx), database.GetTotalUsageHBAgentRuntimeV1Params{
128+
return db.GetTotalUsageHBAgentRuntimeV1(dbauthz.AsUsagePublisher(ctx), database.GetTotalUsageHBAgentRuntimeV1Params{
140129
StartTime: startTime,
141130
EndTime: endTime,
142131
})
143-
// measureUsage publishes a stable text, so the cause is only
144-
// logged here.
145-
if err != nil && !usageMeasurementAborted(ctx, err) {
146-
logger.Error(ctx, "get agent runtime usage for entitlements", slog.Error(err))
147-
}
148-
return runtimeMs, err
149132
},
150133
})
151134
if err != nil {
@@ -204,11 +187,10 @@ const (
204187

205188
type ManagedAgentCountFn func(ctx context.Context, from time.Time, to time.Time) (int64, error)
206189

207-
// AgentRuntimeMsFn returns the total Coder Agent runtime, in milliseconds,
208-
// recorded between from (inclusive) and to (exclusive). The result is in
209-
// milliseconds because that is the unit the hb_agent_runtime_v1 usage events
210-
// record; LicensesEntitlements converts it to whole hours, the unit of the
211-
// agent_runtime_hours_* license claims.
190+
// AgentRuntimeMsFn returns the total Coder Agent runtime, in milliseconds
191+
// (the unit the hb_agent_runtime_v1 usage events record), recorded between
192+
// from (inclusive) and to (exclusive); agentRuntimeMsToHours owns the unit
193+
// conversion and its rationale.
212194
type AgentRuntimeMsFn func(ctx context.Context, from time.Time, to time.Time) (int64, error)
213195

214196
type WorkspaceCapableUserCountFn func(ctx context.Context) (int64, error)
@@ -755,7 +737,7 @@ func LicensesEntitlements(
755737
// Calculate the amount of agents between the usage period start and
756738
// end.
757739
managedAgentCount, ok, err := measureUsage(ctx, &entitlements,
758-
featureArguments.ManagedAgentCountFn, *agentLimit.UsagePeriod,
740+
featureArguments.Logger, featureArguments.ManagedAgentCountFn, *agentLimit.UsagePeriod,
759741
"managed agent count", codersdk.LicenseManagedAgentUsageUnavailableErrorText)
760742
if err != nil {
761743
return entitlements, err
@@ -786,7 +768,7 @@ func LicensesEntitlements(
786768
runtimeHours := entitlements.Features[codersdk.FeatureAgentRuntimeHours]
787769
if entitlements.HasLicense && runtimeHours.UsagePeriod != nil {
788770
runtimeMs, ok, err := measureUsage(ctx, &entitlements,
789-
featureArguments.AgentRuntimeMsFn, *runtimeHours.UsagePeriod,
771+
featureArguments.Logger, featureArguments.AgentRuntimeMsFn, *runtimeHours.UsagePeriod,
790772
"agent runtime", codersdk.LicenseAgentRuntimeUsageUnavailableErrorText)
791773
if err != nil {
792774
return entitlements, err
@@ -934,12 +916,10 @@ func LicensesEntitlements(
934916
return entitlements, nil
935917
}
936918

937-
// usageMeasurementAborted is the single classification for usage-measurement
938-
// failures, shared by measureUsage (which decides whether to abort the whole
939-
// entitlements computation) and the measurement closures in Entitlements
940-
// (which decide whether to log the cause). A failure aborts only when the
941-
// computation's own context is dead: the caller went away or coderd is
942-
// shutting down, so nothing should be published or logged for it.
919+
// usageMeasurementAborted decides whether a usage-measurement failure aborts
920+
// the whole entitlements computation in measureUsage. A failure aborts only
921+
// when the computation's own context is dead: the caller went away or coderd
922+
// is shutting down, so nothing should be published or logged for it.
943923
//
944924
// The abort must not key on the error looking like a cancel: Postgres raises
945925
// SQLSTATE 57014 (query_canceled) for statement_timeout kills as well as
@@ -952,23 +932,25 @@ func usageMeasurementAborted(ctx context.Context, err error) bool {
952932
}
953933

954934
// measureUsage runs one usage-measurement closure over the feature's usage
955-
// period and applies the shared error policy, keeping the managed-agent and
956-
// runtime-hours paths from drifting apart:
935+
// period and owns the whole failure policy (classification, logging, and
936+
// publication), keeping the managed-agent and runtime-hours paths from
937+
// drifting apart:
957938
//
958939
// - A nil closure is a wiring bug (production always provides both), so it
959940
// fails the whole LicensesEntitlements call rather than degrading into an
960941
// operator-facing message for a state only a Coder developer can create.
961942
// - A failure while the computation is being canceled also fails the whole
962-
// call: see usageMeasurementAborted.
963-
// - Any other failure appends the stable unavailableText (the cause is
964-
// logged by the closure; this string is served publicly). See the
943+
// call, without logging: see usageMeasurementAborted.
944+
// - Any other failure logs the cause and appends the stable unavailableText
945+
// (this string is served publicly). See the
965946
// LicenseManagedAgentUsageUnavailableErrorText doc in codersdk for why
966947
// it lands in entitlements.Errors yet renders as a muted diagnostic.
967948
//
968949
// It returns the measured value and true only on success.
969950
func measureUsage(
970951
ctx context.Context,
971952
entitlements *codersdk.Entitlements,
953+
logger slog.Logger,
972954
fn func(ctx context.Context, from time.Time, to time.Time) (int64, error),
973955
usagePeriod codersdk.UsagePeriod,
974956
what string,
@@ -982,6 +964,7 @@ func measureUsage(
982964
case usageMeasurementAborted(ctx, err):
983965
return 0, false, xerrors.Errorf("get %s: %w", what, err)
984966
case err != nil:
967+
logger.Error(ctx, fmt.Sprintf("get %s for entitlements", what), slog.Error(err))
985968
entitlements.Errors = append(entitlements.Errors, unavailableText)
986969
return 0, false, nil
987970
}

0 commit comments

Comments
 (0)